NONEVM-298: timelock contract #528
Conversation
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Error
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Error
augustbleeds
had a problem deploying
to
integration
GitHub Actions
Error
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
| data: Span<felt252>, | ||
| } | ||
|
|
||
| fn _hash_operation_batch(calls: Span<Call>, predecessor: u256, salt: u256) -> u256 { |
There was a problem hiding this comment.
im not across the latest Cairo standards, why does this need to be split out into its own fn outside of the module?
There was a problem hiding this comment.
it doesn't. I split it out so that it could be used as a utility function in tests. I didn't end up using it though
| } | ||
|
|
||
| fn _execute(ref self: ContractState, call: Call) { | ||
| let _response = call_contract_syscall(call.target, call.selector, call.data) |
There was a problem hiding this comment.
does the response contain any useful information? if not, is there any point in assigning it to a variable (or is this a linter thing?)
| fn update_delay(ref self: ContractState, new_delay: u256) { | ||
| self.access_control.assert_only_role(ADMIN_ROLE); | ||
|
|
||
| self | ||
| .emit( | ||
| Event::MinDelayChange( | ||
| MinDelayChange { | ||
| old_duration: self._min_delay.read(), new_duration: new_delay, | ||
| } | ||
| ) | ||
| ); | ||
| self._min_delay.write(new_delay); | ||
| } | ||
|
|
||
| // | ||
| // ONLY ADMIN | ||
| // |
There was a problem hiding this comment.
update_delay should be under ONLY ADMIN as well right
| assert(!mock.contains(MOCK_SET_ID, 300), 'does not contain 300'); | ||
| assert(mock.contains(MOCK_SET_ID, 200), 'contains 200'); | ||
| assert(mock.at(MOCK_SET_ID, 1) == 200, 'indexes match'); | ||
| assert(mock.at(MOCK_SET_ID, 2) == 0, 'no entry at 2nd index'); |
There was a problem hiding this comment.
an interesting behaviour of this enumerable set implementation - you wouldn't be able to tell if the entry at an index is actually zero or if there is no entry?
There was a problem hiding this comment.
yeah, you can't store the 0 value in this implementation. I guess I could solve for this by introducing a special map that only stores 0? EVM doesn't have this problem because storage values can be arrays (not the case in Starknet).
There was a problem hiding this comment.
actually, i think as long as i do a length check, we can assume that 0's retrieved from indexes less than or equal to the length are valid
There was a problem hiding this comment.
i fixed it so that .at() does a length check and panics if the index is invalid. that way if you get back a 0 from .at() it will be valid
| assert(actual_toggle, 'toggle true'); | ||
|
|
||
| assert(timelock.is_operation_done(id), 'operation is done'); | ||
| } |
There was a problem hiding this comment.
could also add another schedule/execute batch with predecessor = first batch to this test to demonstrate that works as intended too
There was a problem hiding this comment.
good point, i'll add that 👍
| } | ||
| } | ||
|
|
||
| // snforge does not treat contract invocation failures as a panic (yet) |
There was a problem hiding this comment.
so executing this on a real environment would result in a panic? is the timelock contract able to tell which op failed?
There was a problem hiding this comment.
in a real environment you'd get a panic / revert. in general, the EVM version also does not explictly tell you which operation failed
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
augustbleeds
temporarily deployed
to
integration
GitHub Actions
Inactive
|
This is the last set of contract changes necessary for the upcoming audit. There are some optional contract changes as well we can add further down the road.