Tweaks to upgrade #50
Conversation
sgc-code
requested review from
Leonard-Pat and
gaetbout
and removed request for
Leonard-Pat
src/contracts/timelock_upgrade.cairo
Outdated
| impl PendingUpgradeZero of core::num::traits::Zero<PendingUpgrade> { | ||
| fn zero() -> PendingUpgrade { | ||
| PendingUpgrade { implementation: Zero::zero(), ready_at: 0, calldata_hash: 0 } | ||
| } | ||
| fn is_zero(self: @PendingUpgrade) -> bool { | ||
| *self.calldata_hash == 0 | ||
| } | ||
| fn is_non_zero(self: @PendingUpgrade) -> bool { | ||
| !self.is_zero() | ||
| } |
There was a problem hiding this comment.
Actually since we never use is_zero() I would optimize is_non_zero() instead.
Although now that I think about it, maybe it makes more sense to derive Default and just compare against Default::default()?
There was a problem hiding this comment.
i like it, Default seems more appropriate 👍
|
please @gaetbout take another look, i did some more changes, including the Updated event, i removed the ready_at field from it as it won't be easy to include in the upgrade |
src/contracts/timelock_upgrade.cairo
Outdated
|
|
||
| fn reset_storage(ref self: ComponentState<TContractState>) { | ||
| self.pending_implementation.write(Zero::zero()); | ||
| self.ready_at.write(0); | ||
| self.calldata_hash.write(0); | ||
| } | ||
|
|
There was a problem hiding this comment.
| fn reset_storage(ref self: ComponentState<TContractState>) { | |
| self.pending_implementation.write(Zero::zero()); | |
| self.ready_at.write(0); | |
| self.calldata_hash.write(0); | |
| } |
There was a problem hiding this comment.
there is just an extra line return 🤣
src/contracts/timelock_upgrade.cairo
Outdated
| } | ||
|
|
||
| #[derive(Drop, starknet::Event)] | ||
| struct Upgraded { | ||
| new_implementation: ClassHash | ||
| struct UpgradedExecuted { |
There was a problem hiding this comment.
This event is actually never emitted
There was a problem hiding this comment.
guessing it should be emitted by the callback impl?
There was a problem hiding this comment.
Yeah, it's supposed to be emitted by the next implementatio
There was a problem hiding this comment.
i just meant the spelling is wrong is should be UpgradeExecuted
Not sure I follow that sentence 🤔 |
In some previous commits the event after the upgrade used to have a field called 'ready_at' but it's gone now |
tests-integration/upgrade.test.ts
Outdated
There was a problem hiding this comment.
why did the asserts change to == and not should.equal..
There was a problem hiding this comment.
i'm not a big fan of chai, feels like reinventing the wheel trying to replace all the stuff we use with functions.
> -> .isBelow
=== -> equals but there is also deep.equals , eq and eql
await -> eventually
i'm not really sure having to introducing all this stuff is worth it to get slightly better errors
i leave my rant here but i changed it to align with the rest of the project and get this merged
| #[derive(Serde, Drop, Copy, Default, PartialEq, starknet::Store)] | ||
| struct PendingUpgrade { | ||
| // Gets the classhash after | ||
| implementation: ClassHash, |
There was a problem hiding this comment.
should this be proposed implementation ?
also dont get the comment of 'gets classhash after?'
nit: 0 if no upgrade ongoing ?
There was a problem hiding this comment.
updated, i drop the proposed_ since it's already in a struct called PendingUpgrade, calldata would also be proposed_
|
Feel free to ignore but if we insantiate Default::default() on multiple lines would it be better to have: then use empty upgrade? Really doesnt matter but im curious |
Uses structs to try to achieve the following:
Also i tweaked the error messages:
upgrade/no-new-implementationto `'upgrade/no-pending-upgrade'upgrade/invalid-calldatatoupgrade/no-pending-upgrade