Fix DS MCMS contracts loading issue for legacy contracts#21291
Fix DS MCMS contracts loading issue for legacy contracts#21291
Conversation
simsonraj
requested review from
agusaldasoro,
asoliman92,
chris-de-leon-cll,
makramkd,
matYang,
tt-cll and
winder
|
👋 simsonraj, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Fixes a crash when loading MCMS contract state from the datastore on legacy networks where the Bypasser and Canceller roles resolve to the same contract address (e.g., BSC), by ensuring both role bindings are present before ABI registration.
Changes:
- Backfill
BypasserMcmfromCancellerMcm(and vice versa) when only one is loaded from the datastore for a chain. - Prevent nil dereference during ABI population for legacy role-collapsed deployments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When Bypasser and Canceller share the same contract address for some legacy networks, fill the gap. | ||
| if mcmsState.BypasserMcm == nil && mcmsState.CancellerMcm != nil { | ||
| mcmsState.BypasserMcm = mcmsState.CancellerMcm | ||
| } | ||
| if mcmsState.CancellerMcm == nil && mcmsState.BypasserMcm != nil { | ||
| mcmsState.CancellerMcm = mcmsState.BypasserMcm | ||
| } |
There was a problem hiding this comment.
Please add a regression test for the legacy case where only one of BypasserMcm/CancellerMcm is present (same on-chain address) to ensure this function no longer panics and that ABIByAddress is populated correctly. There are existing tests in this package, but none appear to cover this scenario.
| if mcmsState.BypasserMcm == nil && mcmsState.CancellerMcm != nil { | ||
| mcmsState.BypasserMcm = mcmsState.CancellerMcm | ||
| } | ||
| if mcmsState.CancellerMcm == nil && mcmsState.BypasserMcm != nil { | ||
| mcmsState.CancellerMcm = mcmsState.BypasserMcm |
There was a problem hiding this comment.
this looks a bit risky. We cannot possibly deterministically know that if one of these is nil and the other is not nil the values should be assignable to another
| cancellerMCMS = cldf.NewTypeAndVersion(types.ManyChainMultisig, deployment.Version1_0_0) | ||
| ) | ||
|
|
||
| proposerMCMS.Labels.Add(types.ProposerRole.String()) |
There was a problem hiding this comment.
do you see these labels in Datastore as well? we decided to let go of types.ManyChainMultisig and only use specific contract type like ProposerManyChainMultisig, .CancellerManyChainMultisig in datastore to keep things simple
|
4 participants
This fixes a critical bug where the MCMS contract loading will panic for any network where Bypasser & cancellers role belong to same address. ( BSC-testnet for example)