-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/emergency shutdown #19
base: feat/slash
Are you sure you want to change the base?
Conversation
…utdown - Update IStakedTokenV3 interface - Remove modifier from slashing actions - Let cooldown admin be only caller to `setEmergencyShutdown`
|
||
contract StakeTokenUpgradeProposalExecutor { | ||
// TODO: Replace with constant address when implementation is deployed | ||
address immutable NEW_STAKED_AAVE_TOKEN_IMPLEMENTATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems clearer to fix the address here directly (instead of using the constructor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be fixed when we have an implementation that is deployed.
Using the constructor for passing freshly deployed implementation made testing easier.
IBaseAdminUpgradabilityProxy(0x4da27a545c0c5B758a6BA100e3a049001de870f5); | ||
|
||
function execute() external { | ||
STAKED_AAVE_TOKEN_PROXY.upgradeTo(NEW_STAKED_AAVE_TOKEN_IMPLEMENTATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should initialize, so upgradeToAndCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Is updated in the newest commit.
- Check emissions are identical before and after upgrade - Check that staking across update behaves as expected - Update contract upgrade test for impersonating executor - Update `SHORT_EXECUTOR` to a checksummed address
fix: Introduce dynamic domain separator
@@ -161,15 +166,36 @@ contract StakedTokenV3 is StakedTokenV2, IStakedTokenV3, RoleManager { | |||
_maxSlashablePercentage = maxSlashablePercentage; | |||
} | |||
|
|||
function setEmergencyShutdown(bool emergencyShutdown) external override onlyCooldownAdmin { | |||
_emergencyShutdown = emergencyShutdown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to check that the current _emergencyShutdown status != the input emergencyShutdown
As is, you could have a EmergencyShutdownChanged event from true to true. A lot would have to go wrong in the gov process to end up with that, but could be added to avoid events that don't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think so. We allow writing the same value to state for other configurations in pool etc, so think it is fine to keep the consistency here to also allow "rewriting" the same value.
closes #18