refactor(StakeVault): BRAKING CHANGE migration system #243
Conversation
0x-r4bbit
left a comment
0x-r4bbit
left a comment
There was a problem hiding this comment.
@3esmit I left some comments here.
Overall, I'm not really sure what's the benefit of this change.
To me it looks like we're introducing more complexity than necessary.
StakeVaultnow has aVaultFactorydependencyStakeVaultnow needs an additionalmigrate()functionStakeVaultstill calls intoStakeManager(which I think is necessary anyway)
The only thing we seem to gain from this is that StakeManager doesn't have to call into StakeVault.migrateFromVault() anymore at the cost of needing additional checks in migrateToVault().
Are there any other things I'm missing here?
If not, I'm not sure this is improving the current code.
@gravityblast I'd also like your opinion on this.
| } | ||
|
|
||
| if (STAKING_TOKEN.balanceOf(address(newVaultAddr)) < vaultData[migrateTo].stakedBalance) { | ||
| revert StakeManager__InvalidMigration(); |
There was a problem hiding this comment.
The check in L:384 is vacuous because in L:380 we already require that vaultData[migrateTo].stakedBalance == 0. STAKING_TOKEN.balanceOf(address(newVaultAddr)) can therefore never be < vault[migrateTo].stakedBalance
| function migrateToVault(address migrateTo) external onlyNotEmergencyMode onlyTrustedCodehash onlyRegisteredVault { | ||
| if (vaultOwners[migrateTo] == address(0)) { | ||
| revert StakeManager__InvalidVault(); | ||
| revert StakeManager__InvalidMigration(); |
There was a problem hiding this comment.
Any reason we're introducing a breaking change in the errors being emitted?
That is better than StakeManager depending on StakeVault, because VaultFactory code can be considered StakeVault code.
Actually it's a new initializer, which can only be called on non initialized Clones, so its like an "overloaded constructor".
The goal is to StakeManager be independent of StakeVault, as possible. Ideally, StakeManager should not even know StakeVault ABI. This would make the code more future proof and better testable. |
|
Let's move this PR to https://github.com/status-im/status-network-monorepo/pulls |
Description
Introduce a new migration system for StakeVault being able to migrate on its own.
Fixes #242
Checklist
Ensure you completed all of the steps below before submitting your pull request:
pnpm adorno?pnpm verify?