Add Governor component #1180
Open
Add Governor component #1180
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eleased-package-#1141
…emove-dual-dispatchers
…eat/remove-mocks-from-released-package-#1141
* feat: remove modules * fix: mock * fix: linter * fix: tests * fix: mock * feat: apply review suggestions
…om-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
…eat/remove-mocks-from-released-package-#1141
packages/governance/src/governor/extensions/governor_timelock_execution.cairo
Show resolved
Hide resolved
packages/governance/src/governor/extensions/governor_timelock_execution.cairo
Outdated
Show resolved
Hide resolved
packages/governance/src/governor/extensions/governor_votes_quorum_fraction.cairo
Outdated
Show resolved
Hide resolved
packages/governance/src/governor/extensions/governor_votes_quorum_fraction.cairo
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
…contracts into feat/governance2-#294
immrsd
reviewed
Nov 13, 2024
| canceled: canceled > 0, | ||
| eta_seconds: eta_seconds.try_into().unwrap() | ||
| } | ||
| } |
There was a problem hiding this comment.
What do you think about using u256_safe_divmod to reduce the number of arithmetic operations? The impl could look like this if we put the boolean value at the latest bit indices
let val: u256 = second_felt.into();
let (val, executed) = u256_safe_divmod(val, 2);
let (val, canceled) = u256_safe_divmod(val, 2);
let (val, eta_seconds) = u256_safe_divmod(val, _2_POW_64);
let (val, vote_duration) = u256_safe_divmod(val, _2_POW_64);
let (val, vote_start) = u256_safe_divmod(val, _2_POW_64);
And this way we'll need only single bits constant instead of 7
There was a problem hiding this comment.
Looks a lot cleaner indeed. The issue is that u256_safe_divmod is not public, but we can use the DivRem trait which is the exposed mechanism by design.
packages/governance/src/governor/extensions/governor_settings.cairo
Outdated
Show resolved
Hide resolved
packages/governance/src/governor/extensions/governor_votes_quorum_fraction.cairo
Outdated
Show resolved
Hide resolved
packages/governance/src/governor/extensions/governor_votes_quorum_fraction.cairo
Show resolved
Hide resolved
Comment on lines
+502
to
+505
| self | ||
| .validate_state( | ||
| proposal_id, array![ProposalState::Succeeded, ProposalState::Queued].span() | ||
| ); |
There was a problem hiding this comment.
It seems to be overkill for checking the state: we have to create an array and iterate through it. Maybe a more direct approach would be better
Suggested change
| self | |
| .validate_state( | |
| proposal_id, array![ProposalState::Succeeded, ProposalState::Queued].span() | |
| ); | |
| match self.state(proposal_id) { | |
| ProposalState::Succeeded => (), | |
| ProposalState::Queued => (), | |
| _ => panic_with_felt252(Errors:: UNEXPECTED_PROPOSAL_STATE), |
There was a problem hiding this comment.
This is probably more efficient, but the validate_state helper can be used by users as well. We can refactor it into bitmap operations to avoid looping or matching, which should be even more efficient, and also usable by users. Let's keep the thread opened until the pending stuff is finished and I will work on that then.
packages/governance/src/governor/extensions/governor_counting_simple.cairo
Show resolved
Hide resolved
Co-authored-by: immrsd <[email protected]>
…contracts into feat/governance2-#294
…eat/governance2-#294
immrsd
reviewed
Nov 15, 2024
| pub enum VoteType { | ||
| Against, | ||
| For, | ||
| Abstain, |
There was a problem hiding this comment.
Suggested change
| Abstain, | |
| Abstain |
Comment on lines
+95
to
+103
| if timelock_dispatcher.is_operation_pending(queue_id) { | ||
| ProposalState::Queued | ||
| } else if timelock_dispatcher.is_operation_done(queue_id) { | ||
| // This can happen if the proposal is executed directly on the timelock. | ||
| ProposalState::Executed | ||
| } else { | ||
| // This can happen if the proposal is canceled directly on the timelock. | ||
| ProposalState::Canceled | ||
| } |
There was a problem hiding this comment.
Correct me if I'm wrong, but in the case of Executed or Canceled proposal state we'll have to make 2 external calls to timelock contract (is_operation_pending + is_operation_done) instead of a single get_operation_state call
Comment on lines
+137
to
+147
| let (_, key, value) = self | ||
| .Governor_quorum_numerator_history | ||
| .deref() | ||
| .latest_checkpoint(); | ||
|
|
||
| if key <= timepoint { | ||
| return value; | ||
| } | ||
|
|
||
| // Fallback to the binary search | ||
| self.Governor_quorum_numerator_history.deref().upper_lookup(timepoint) |
There was a problem hiding this comment.
Suggested change
| let (_, key, value) = self | |
| .Governor_quorum_numerator_history | |
| .deref() | |
| .latest_checkpoint(); | |
| if key <= timepoint { | |
| return value; | |
| } | |
| // Fallback to the binary search | |
| self.Governor_quorum_numerator_history.deref().upper_lookup(timepoint) | |
| let history = self.Governor_quorum_numerator_history.deref(); | |
| let (_, key, value) = history.latest_checkpoint(); | |
| if key < timepoint { | |
| value | |
| } else { | |
| // Fallback to the binary search | |
| history.upper_lookup(timepoint) |
| ref self: ComponentState<TContractState>, calls: Span<Call>, description_hash: felt252 | ||
| ) -> felt252 { | ||
| let proposal_id = self._hash_proposal(calls, description_hash); | ||
| self.validate_state(proposal_id, array![ProposalState::Pending].span()); |
There was a problem hiding this comment.
Is it possible to cancel a queued proposal?
Comment on lines
+627
to
+634
| let is_valid_signature_felt = ISRC6Dispatcher { contract_address: voter } | ||
| .is_valid_signature(hash, signature.into()); | ||
|
|
||
| // 3. Check either 'VALID' or true for backwards compatibility | ||
| let is_valid_signature = is_valid_signature_felt == starknet::VALIDATED | ||
| || is_valid_signature_felt == 1; | ||
|
|
||
| assert(is_valid_signature, Errors::INVALID_SIGNATURE); |
There was a problem hiding this comment.
This is the third place we use the same boilerplate code to validate a signature, shall we consider extracting it into a util function?
| /// governor as the only proposer, canceller, and executor. | ||
| /// | ||
| /// WARNING: When the executor is not the governor itself (i.e. a timelock), it can call | ||
| /// functions that are restricted with the `assert_only_governance` modifier, and also |
There was a problem hiding this comment.
Suggested change
| /// functions that are restricted with the `assert_only_governance` modifier, and also | |
| /// functions that are restricted with the `assert_only_governance` check, and also |
or something other than modifier
Comment on lines
+824
to
+828
| /// WARNING: When the executor is not the governor itself (i.e. a timelock), it can call | ||
| /// functions that are restricted with this modifier, and also potentially execute | ||
| /// transactions on behalf of the governor. Because of this, this module is designed to work | ||
| /// with the TimelockController as the unique potential external executor. The timelock | ||
| /// MUST have the governor as the only proposer, canceller, and executor. |
Comment on lines
+649
to
+650
| /// Emits a `VoteCast` event if no params are provided. | ||
| /// Emits a `VoteCastWithParams` event otherwise. |
There was a problem hiding this comment.
Suggested change
| /// Emits a `VoteCast` event if no params are provided. | |
| /// Emits a `VoteCastWithParams` event otherwise. | |
| /// Emits either: | |
| /// - `VotesCast`... | |
| /// - `VoteCastWithParams` ... |
Nitpick-ish: Maybe presenting it like this might be easier to follow? When I see two "Emits" at first glance, I think it emits two events
| /// Returns a unique hash given a ByteArray. | ||
| /// | ||
| /// The hash is computed by serializing the data into a span of felts, and | ||
| /// then hashing the span using the Poseidon hash algorithm. |
There was a problem hiding this comment.
Suggested change
| /// then hashing the span using the Poseidon hash algorithm. | |
| /// then hashing the span using the Pedersen hash algorithm. |
| to_byte_array(self, base, padding) | ||
| } | ||
|
|
||
| /// Hashes a byte array using the Poseidon hash algorithm. |
There was a problem hiding this comment.
Suggested change
| /// Hashes a byte array using the Poseidon hash algorithm. | |
| /// Hashes a byte array using the Pedersen hash algorithm. |
Comment on lines
+1247
to
+1250
| // Use invalid nonce (not the account's current nonce) | ||
| let (governor, r, s, proposal_id, support, voter, _) = | ||
| prepare_governor_and_signature_with_reason_and_params( | ||
| @reason, params, 0 |
There was a problem hiding this comment.
Suggested change
| // Use invalid nonce (not the account's current nonce) | |
| let (governor, r, s, proposal_id, support, voter, _) = | |
| prepare_governor_and_signature_with_reason_and_params( | |
| @reason, params, 0 | |
| let (governor, r, s, proposal_id, support, voter, _) = | |
| prepare_governor_and_signature_with_reason_and_params( | |
| @reason, params, 0 |
The nonce looks valid here
Comment on lines
+1265
to
+1269
|
|
||
| // Use invalid nonce (not the account's current nonce) | ||
| let (governor, r, s, proposal_id, support, voter, _) = | ||
| prepare_governor_and_signature_with_reason_and_params( | ||
| @reason, params, 1 |
There was a problem hiding this comment.
Suggested change
| // Use invalid nonce (not the account's current nonce) | |
| let (governor, r, s, proposal_id, support, voter, _) = | |
| prepare_governor_and_signature_with_reason_and_params( | |
| @reason, params, 1 | |
| let invalid_nonce = 1; | |
| // Use invalid nonce (not the account's current nonce) | |
| let (governor, r, s, proposal_id, support, voter, _) = | |
| prepare_governor_and_signature_with_reason_and_params( | |
| @reason, params, invalid_nonce |
Easier to follow IMO
Comment on lines
+1353
to
+1361
| #[test] | ||
| #[should_panic(expected: 'Executor only')] | ||
| fn test_relay_invalid_caller() { | ||
| let mut state = COMPONENT_STATE(); | ||
| let call = Call { to: ADMIN(), selector: selector!("foo"), calldata: array![].span() }; | ||
|
|
||
| start_cheat_caller_address(test_address(), OTHER()); | ||
| state.relay(call); | ||
| } |
There was a problem hiding this comment.
We should include a failing tx test for the syscall (confirming it won't silently fail)
| impl SRC5: SRC5Component::HasComponent<TContractState>, | ||
| +Drop<TContractState> | ||
| > of InternalTrait<TContractState> { | ||
| /// Initializes the contract by registering the supported interface Ids. |
There was a problem hiding this comment.
Suggested change
| /// Initializes the contract by registering the supported interface Ids. | |
| /// Initializes the contract by registering the supported interface id. |
Comment on lines
+125
to
+126
| /// Temporary defined as a function since constant ByteArray is not supported yet. | ||
| fn DEFAULT_PARAMS() -> Span<felt252>; |
There was a problem hiding this comment.
We should adjust the comment. Maybe test this with a non-empty span, WDYT?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #294
PR Checklist