Conversation
|
👋 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
Risk Rating: MEDIUM — changes affect deployment changeset logic for role-granting and how MCMS+Timelock state is resolved (qualifier-aware DataStore loading).
This PR updates the GrantRoleInTimeLock changeset to load MCMS-with-timelock state on a per-chain basis, using TimelockQualifierPerChain when provided so the correct qualifier-isolated deployment is pulled from the DataStore.
Changes:
- Replace bulk state loading (
maps.Keys(...)) with per-chain loading that can apply per-chain timelock qualifiers. - Add
loadMCMSStatePerChainWithQualifierhelper to centralize qualifier-aware loading for both preconditions and execution paths. - Remove the
golang.org/x/exp/mapsdependency from this changeset.
| } | ||
| chainState, err := state.MaybeLoadMCMSWithTimelockStateWithQualifier(e, []uint64{selector}, qualifier) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
In loadMCMSStatePerChainWithQualifier, the error returned from MaybeLoadMCMSWithTimelockStateWithQualifier is propagated without chain/qualifier context. When multiple chains are processed, this makes it harder to identify which selector/qualifier caused the failure (e.g., when DataStore is missing for a qualifier). Consider wrapping the error with the selector and qualifier to make troubleshooting deterministic.
| return nil, err | |
| return nil, fmt.Errorf("failed to load MCMS state for selector %d with qualifier %q: %w", selector, qualifier, err) |
| mcmsState, err := loadMCMSStatePerChainWithQualifier(e, cfg) | ||
| if err != nil { |
There was a problem hiding this comment.
This PR changes GrantRoleInTimeLock to load MCMS state using per-chain timelock qualifiers (DataStore-qualified isolation). There’s no test covering the TimelockQualifierPerChain path to ensure GrantRoleInTimeLock loads the correct timelock bundle from DataStore (and does not fall back to AddressBook/unqualified DataStore when a qualifier is provided). Adding a test that populates DataStore with a non-empty qualifier and verifies roles are granted against that qualified timelock would prevent regressions.
|
No description provided.