governance: update migration logic in GovernanceDelegation
#342
Conversation
cairoeth
requested review from
protolambda,
tynes,
ajsutton,
sebastianst and
mslipper
as code owners
cairoeth
changed the title
GovernanceDelegationGovernanceDelegation
|
I'd like a review from @smartcontracts given he has the most context here |
specs/experimental/gov-delegation.md
Outdated
| ## Migration | ||
|
|
||
| All write functions in the `GovernanceDelegation` MUST check if the users interacting with it have been migrated by | ||
| checking the `migrated` mapping from its [storage](#storage). If a user has not been migrated, the `GovernanceDelegation` | ||
| MUST copy the delegation and checkpoint data from the token contract to its own state. After copying the data, the | ||
| `GovernanceDelegation` MUST update the `migrated` mapping to reflect that the address has been migrated. | ||
| `GovernanceDelegation` MUST update the `migrated` mapping to reflect that the address has been migrated, and remove the |
There was a problem hiding this comment.
I actually don't think we need the migrated mapping anymore? Since we're not forwarding stuff to the original contract.
There was a problem hiding this comment.
yeah the GovernanceDelegation contract doesn't really need it, but it could be a helper for external contracts like governor: if migrated is true, get data from GovernanceDelegation, otherwise from GovernanceToken.
if these contracts check delegates[_addr] only in GovernanceToken, they won't be able to know if there's a delegation in GovernanceDelegation, so they will always have to call both. wdyt?
There was a problem hiding this comment.
Ah since we maintain the invariant that you can't have a delegation in both contracts then what you can do is just check if delegates[_addr] inside GovernanceToken is address(0)
There was a problem hiding this comment.
agreed, updated in eee936c
| ## Migration | ||
| The `GovernanceDelegation` MUST enforce the following invariants for the migration logic: | ||
|
|
||
| 1. A user MUST NOT have an active delegation in both `GovernanceToken` and `GovernanceDelegation` at the same time. |
There was a problem hiding this comment.
Technically some of these statements are implied by the other statements but I think it's good enough to just be explicit and list all of the statements
There was a problem hiding this comment.
yeah i think it helps to have some redundancy in this specific case
Description & Context
For interop, the governor will have to be updated use the L1 block number instead of the OP L2 block number as networks in their superchain will have their own cadence of blocks.
Proposed by @smartcontracts, the interop update introduces breaking changes so it's better to have the delegation state separated: the legacy state in the
GovernanceToken(with L2 block number), and the new state in theGovernanceDelegationcontract. This means that theGovernanceTokendoens't call theGovernanceDelegationin its getter functions, and vice versa.