fix(sdk): guard container ism child updates#8905
Conversation
paulbalaji
left a comment
There was a problem hiding this comment.
Reviewed as the follow-up to #8902. This is the right shape for container ISM updates, and it closes the mis-pairing bug cleanly. Splitting discovery (containerSubModules) from a recursive preflight (canUpdateSubModuleInPlace) and running the full preflight before any side-effectful .update() is exactly what was missing. No blockers.
What I verified:
- The duplicate-sort-key guard fully closes the #8902 bug. Sort key =
.typematchesnormalizeConfig's own module sort, so distinct types never collide and same-type members (which are static and can't update in place anyway) correctly bail to redeploy. Not overly conservative in practice. - Preflight faithfully predicts
update()'s in-place vs redeploy decision across MUTABLE types, ROUTING/INCREMENTAL_ROUTING (matches thecalculateDomainRoutingDeltasemantics), AGGREGATION/AMOUNT_ROUTING recursion, and RATE_LIMITED. No case found where preflight says OK butupdate()redeploys. - Preflight is read-only (only
deriveIsmConfig+ view calls), so the preflight-before-mutate ordering genuinely avoids partial/wasted deploys. The retained post-updateeqAddresscheck is a worthwhile safety net, not redundant. - The
as any/as AggregationIsmConfig/as AmountRoutingIsmConfigcasts from #8902 are all gone. Compiles; all referenced symbols imported.
My only substantive feedback is on test coverage. Tests 1 and 3 are solid and would fail if the fix were reverted. The gaps worth filling, since these are the subtlest parts of the change:
- Nested containers (aggregation-of-aggregation, or aggregation containing amount-routing) - the recursive
canUpdateSubModuleInPlacepath is the most complex new code and is entirely untested. - Preflight-abort without side effects - a container where one sub-module can't update in place should fall back to redeploy without having emitted partial txs for the others. This is the key correctness property of the refactor and isn't pinned by a test.
- AMOUNT_ROUTING redeploy fallback (e.g. threshold change) - only the in-place success path is covered.
- Optional: RATE_LIMITED-with-recipient and INCREMENTAL_ROUTING sub-module cases, which have dedicated branches.
Inline notes below. Re draft status / merge order: keeping #8902 mergeable on its own (config + isInitialized + nonce) and landing this separately still seems like the right call.
Summary
recipient()before deciding whether to redeployTesting
pnpm -C typescript/sdk checkpnpm -C typescript/sdk exec oxlint -c ../../oxlint.json src/ism/EvmIsmModule.ts src/ism/EvmIsmModule.hardhat-test.tscd typescript/sdk && NODE_OPTIONS='--import tsx/esm' ./node_modules/.bin/hardhat --config hardhat.config.cts test ./src/ism/EvmIsmModule.hardhat-test.ts --grep 'nested|preflight|amountRoutingIsm|rateLimitedIsm|duplicate'cd typescript/sdk && NODE_OPTIONS='--import tsx/esm' ./node_modules/.bin/hardhat --config hardhat.config.cts test ./src/ism/EvmIsmModule.hardhat-test.ts(54 passing)Note: broad
pnpm -C typescript/sdk lintpreviously reported pre-existing package-wide issues; touched-file oxlint is clean.