fix: moonpay bsc owner#8902
Conversation
troykessler
requested review from
Mo-Hussain,
Xaroz,
paulbalaji,
tkporter and
xeno097
as code owners
There was a problem hiding this comment.
Feels like an extraneous optimisation
Would prefer to focus the PRS on bsc changes and move sdk changes to a separate or for reviewing independently
📝 WalkthroughWalkthroughActivates the BSC entry in ChangesInfra: BSC owner config and Safe nonce
SDK: ISM in-place updates and init guard
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.changeset/hot-bears-enjoy.md (1)
6-6: ⚡ Quick winConsider expanding the changeset description for clarity.
The changeset is in past tense as required, but it's quite vague given the scope of these changes. Folk reading the changelog won't get much sense of what actually changed without context. Since this touches both infra (BSC owner config, dynamic nonce fetching) and SDK (ISM updates, routing ISM initialization), a bit more specificity would help.
For example, something like: "Fixed BSC owner configuration in Moonpay warp setup, replaced static Safe nonce overrides with dynamic fetching, and improved ISM in-place update handling with idempotent routing ISM initialization."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/hot-bears-enjoy.md at line 6, The changeset description at line 6 of the hot-bears-enjoy.md file is too vague and doesn't clearly communicate what changed. Replace the current description "Fixed bsc owner and owner updates in sdk" with a more detailed version that specifically mentions the three main changes: the BSC owner configuration fix in Moonpay warp setup, the replacement of static Safe nonce overrides with dynamic fetching, and the improved ISM in-place update handling with idempotent routing ISM initialization. This will help readers understand the scope and nature of the changes when they review the changelog.Source: Coding guidelines
typescript/infra/src/govern/multisend.ts (1)
103-108: 💤 Low valueConsider adding explicit radix to
parseInt.Now, I'm not one to get all worked up about the little things—got better things to do in me swamp—but using
parseInt(nextNonce, 10)makes the intent crystal clear for anyone wandering through later. Works fine as-is, just a wee bit tidier with the radix spelled out.private async getNextNonce(): Promise<number> { const nextNonce = await retrySafeApi(() => this.safeService.getNextNonce(this.safeAddress), ); - return parseInt(nextNonce); + return parseInt(nextNonce, 10); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript/infra/src/govern/multisend.ts` around lines 103 - 108, In the getNextNonce method, add an explicit radix parameter to the parseInt call. Change the parseInt invocation to include 10 as the second argument to explicitly specify decimal parsing, making the intent clear that the value is being parsed as a base-10 number rather than relying on implicit radix detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@typescript/sdk/src/ism/EvmIsmModule.ts`:
- Around line 564-570: The sorting logic in sortedTargetModules uses unsafe `as
any` casts to access the type property. Since IsmConfig is either a string
address or an object with a type field, extract the type directly from the
object after confirming it is not null without using the `as any` cast—simply
access the type property as (a as {type: string}).type or create a small helper
function that safely extracts the type from either a string or an object form of
IsmConfig.
---
Nitpick comments:
In @.changeset/hot-bears-enjoy.md:
- Line 6: The changeset description at line 6 of the hot-bears-enjoy.md file is
too vague and doesn't clearly communicate what changed. Replace the current
description "Fixed bsc owner and owner updates in sdk" with a more detailed
version that specifically mentions the three main changes: the BSC owner
configuration fix in Moonpay warp setup, the replacement of static Safe nonce
overrides with dynamic fetching, and the improved ISM in-place update handling
with idempotent routing ISM initialization. This will help readers understand
the scope and nature of the changes when they review the changelog.
In `@typescript/infra/src/govern/multisend.ts`:
- Around line 103-108: In the getNextNonce method, add an explicit radix
parameter to the parseInt call. Change the parseInt invocation to include 10 as
the second argument to explicitly specify decimal parsing, making the intent
clear that the value is being parsed as a base-10 number rather than relying on
implicit radix detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74591b40-05ef-494c-b040-db6dc1c1f7c1
📒 Files selected for processing (6)
.changeset/hot-bears-enjoy.mdtypescript/infra/config/environments/mainnet3/governance/ica/aw.tstypescript/infra/config/environments/mainnet3/warp/configGetters/getUSDCCitreaMoonpayWarpConfig.tstypescript/infra/src/govern/multisend.tstypescript/sdk/src/ism/EvmIsmModule.tstypescript/sdk/src/ism/HyperlaneIsmFactory.ts
| const sortedTargetModules = [...targetAgg.modules].sort((a, b) => { | ||
| const aType = | ||
| typeof a === 'object' && a !== null ? (a as any).type : String(a); | ||
| const bType = | ||
| typeof b === 'object' && b !== null ? (b as any).type : String(b); | ||
| return aType < bType ? -1 : aType > bType ? 1 : 0; | ||
| }); |
There was a problem hiding this comment.
Avoid as any cast when extracting type from IsmConfig.
This wee bit here does the job but tramples on the guidelines, aye? The type can be pulled out properly without conjuring up any. IsmConfig is either a string address or an object with a type field—no need to go off into the swamp with unsafe casts.
🧅 Proposed fix to extract type safely
const sortedTargetModules = [...targetAgg.modules].sort((a, b) => {
- const aType =
- typeof a === 'object' && a !== null ? (a as any).type : String(a);
- const bType =
- typeof b === 'object' && b !== null ? (b as any).type : String(b);
+ const aType = typeof a === 'object' && a !== null ? a.type : String(a);
+ const bType = typeof b === 'object' && b !== null ? b.type : String(b);
return aType < bType ? -1 : aType > bType ? 1 : 0;
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript/sdk/src/ism/EvmIsmModule.ts` around lines 564 - 570, The sorting
logic in sortedTargetModules uses unsafe `as any` casts to access the type
property. Since IsmConfig is either a string address or an object with a type
field, extract the type directly from the object after confirming it is not null
without using the `as any` cast—simply access the type property as (a as {type:
string}).type or create a small helper function that safely extracts the type
from either a string or an object form of IsmConfig.
Source: Coding guidelines
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
BSC/moonpay config fix looks clean and is ready to go: awIcas.bsc restored, USDC + USDT Citrea/Moonpay owners switched to it consistently, and the removed SAFE_NONCE_OVERRIDES map was empty on main so nothing active is lost. The dynamic getNextNonce() matches the Safe Kit's prior default behavior.
My main ask is to pull the tryUpdateContainerIsm change out of this PR so the config fix isn't held up. This is an area I've looked at before, and in-place container ISM updates are meaningfully more involved than the implementation here. The duplicate same-type pairing bug below is the clearest example, and the spread of follow-on issues/fixes surfaced in the stacked refactor (#8905) reinforces that it deserves its own PR with proper tests (duplicate same-type mutable children, preserved-address preflight) rather than riding along with a config change.
The isInitialized guard is worth keeping in this PR. It's low-risk and useful. Only the comment's rationale is a bit off (see inline). The nonce change is also fine to keep here.
Suggested split:
- Keep here: BSC config,
isInitializedguard, dynamic nonce. - Move to #8905 (or its own PR):
tryUpdateContainerIsm+ theupdate()early in-place path.
Non-blocking nits inline. Not requesting changes.
| onChainTyped.sort((a, b) => | ||
| a.type < b.type ? -1 : a.type > b.type ? 1 : 0, | ||
| ); | ||
| const sortedTargetModules = [...targetAgg.modules].sort((a, b) => { | ||
| const aType = | ||
| typeof a === 'object' && a !== null ? (a as any).type : String(a); | ||
| const bType = | ||
| typeof b === 'object' && b !== null ? (b as any).type : String(b); | ||
| return aType < bType ? -1 : aType > bType ? 1 : 0; | ||
| }); | ||
| subModules = onChainTyped.map(({ addr }, i) => ({ | ||
| address: addr, | ||
| targetConfig: sortedTargetModules[i], | ||
| })); |
There was a problem hiding this comment.
Pairing-by-sorted-type is unsafe when sub-modules share a type. On-chain modules are sorted by derived .type and target modules by config .type, then zipped by index. If two children have the same IsmType (e.g. two MERKLE_ROOT_MULTISIG, or two routing modules), the comparator returns 0 and ordering is ambiguous, so an on-chain address can bind to the wrong target config. The eqAddress guard at line 611 does not catch this: a mutable child intentionally keeps its address after an in-place update, so a wrong-child mutation passes the check and silently misconfigures the aggregation.
Not triggered by the Citrea/Moonpay routes (their aggregations are [AMOUNT_ROUTING, FALLBACK_ROUTING], distinct types), but it's a latent landmine for any future aggregation with repeated types. Minimum fix: bail to redeploy (return null) when duplicate types are present. Better handled in the dedicated PR (#8905) with tests. Also (a as any).type / String(a) here is dead + a cast violation: at this point target modules are already fully derived objects with .type, and normalizeConfig already sorts modules by .type, so only the on-chain side needs sorting.
| overrides, | ||
| ), | ||
| ); | ||
| // Guard against re-initialization when a previous run deployed this |
There was a problem hiding this comment.
Worth keeping the guard, but the rationale is slightly misleading. The fallback routing ISM is deployed via a fresh handleDeploy (new CREATE address each run), so on a re-run after a mid-deploy crash this lands on a brand-new uninitialized address and the guard never fires. So it doesn't actually make the fallback path idempotent across runs (the ZkSync path with a deterministic address is where it can hit). Suggest softening the comment to reflect that it's a defensive double-init guard rather than crash-recovery. The detection itself is correct (reads slot 0, OZ 4.9.6 _initialized == 1 or 255).
| const nextNonce = await retrySafeApi(() => | ||
| this.safeService.getNextNonce(this.safeAddress), | ||
| ); | ||
| return parseInt(nextNonce); |
There was a problem hiding this comment.
Nit: add explicit radix. safeService.getNextNonce returns a Promise<string>, so parseInt is the right call, but parseInt(nextNonce, 10) matches the convention already used in safe.ts.
| "@hyperlane-xyz/sdk": patch | ||
| --- | ||
|
|
||
| Fixed bsc owner and owner updates in sdk |
There was a problem hiding this comment.
Nit: a bit vague. Once the SDK change is split out, this becomes infra-only and could read closer to: "Migrated Moonpay BSC USDC/USDT warp route owners to the BSC ICA and replaced the static Safe nonce override map with dynamic next-nonce fetching."
2 participants
Description
Migrates ownership of the moonpay BSC USDC and USDT warp routes from the old BSC Safe (
0x7bB2AD...) to the new ICA (awIcas.bsc = 0x269Af9...).Three fixes were needed to make
warp applywork correctly for this migration:HyperlaneIsmFactory: AddedisInitializedguard before callinginitialize()onFALLBACK_ROUTINGISMs (both standard and ZkSync paths). On retry, the contract was already initialized, causing a revert.EvmIsmModule: AddedtryUpdateContainerIsmto updateAGGREGATIONandAMOUNT_ROUTINGISM sub-modules in-place when only owners change, avoiding a full redeploy of the ISM tree.multisend: Replaced staticSAFE_NONCE_OVERRIDESmap with automaticgetNextNonce()lookup via the Safe API, which accounts for pending transactions and avoids nonce conflicts.Drive-by changes
aw.ts: Uncommented/addedbscICA entry.getUSDCCitreaMoonpayWarpConfig.ts: BSC owner updated fromawSafes.bsc→awIcas.bsc.getUSDTCitreaMoonpayWarpConfig.ts: BSC owner updated fromawSafes.bsc→awIcas.bsc.Backward compatibility
Yes
Testing
Manual — ran
warp applyfor USDC/moonpay and USDT/moonpay on BSC end-to-end.