feat: port multicollateral sdk/cli + warp monitor metrics (public split)#8248
Merged
paulbalaji merged 26 commits intomainfrom Mar 4, 2026
Merged
feat: port multicollateral sdk/cli + warp monitor metrics (public split)#8248paulbalaji merged 26 commits intomainfrom
paulbalaji merged 26 commits intomainfrom
Conversation
nambrot
requested review from
Xaroz,
antigremlin,
ltyu,
paulbalaji,
xeno097 and
yorhodes
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8248 +/- ##
==========================================
+ Coverage 75.82% 76.82% +1.00%
==========================================
Files 124 128 +4
Lines 3069 3280 +211
Branches 253 282 +29
==========================================
+ Hits 2327 2520 +193
- Misses 726 743 +17
- Partials 16 17 +1
🚀 New features to boost your workflow:
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new multicollateral Solidity package (contracts, tests, tooling), SDK support (token types, adapters, deployer, warp/core changes), CLI route-merge and transfer options, workspace/build updates, and Warp Monitor explorer & metrics integrations for pending transfers and inventory tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Warp CLI
participant WC as WarpCore
participant O as Origin MultiCollateral
participant MB as Mailbox
participant R as Destination Router
participant D as Destination Token
U->>CLI: transferRemoteTo(origin,dest,amt,targetRouter[,destinationToken])
CLI->>WC: getTransferRemoteTxs(with destinationToken)
WC->>O: quoteTransferRemoteToGas(destination,recipient,amount,targetRouter)
O-->>WC: Quote[] (IGP + router fees)
WC->>O: populateApproveTx / populateTransferRemoteToTx
WC->>MB: dispatch message (targetRouter metadata)
MB->>R: handle(origin,sender,message)
R->>D: process transfer, charge fees
R-->>U: recipient balance updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
typescript/sdk/src/token/EvmWarpModule.hardhat-test.ts-219-224 (1)
219-224:⚠️ Potential issue | 🟡 Minor
multiCollateralconfig branch added, but not exercised here.This helper now defines
TokenType.multiCollateral, but this file’s loops never hit it because Line 172 excludes it. Add a lightweight non-deploy assertion test for this branch so SDK config path still gets coverage.Suggested minimal test
+ it('should build multiCollateral movable config without deploying', async () => { + const cfg = getMovableTokenConfig([randomAddress()])[TokenType.multiCollateral]; + expect(cfg.type).to.equal(TokenType.multiCollateral); + expect(cfg.token).to.equal(token.address); + });As per coding guidelines,
**/*.{ts,tsx,sol,rs}: Include tests for new functionality, especially edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/EvmWarpModule.hardhat-test.ts` around lines 219 - 224, Add a lightweight test that exercises the TokenType.multiCollateral branch by creating a config object using the existing baseConfig, token.address and allowedRebalancers (mirroring the diff block that sets type: TokenType.multiCollateral) and assert the SDK/config helper returns or accepts that config (e.g., no-throw or matches expected shape) without deploying; specifically add a non-deploy unit test that constructs the multiCollateral config, calls the same config/validation helper used elsewhere in this file, and asserts the result is valid so the TokenType.multiCollateral path is covered.typescript/cli/src/tests/ethereum/warp/warp-multi-collateral.e2e-test.ts-176-177 (1)
176-177:⚠️ Potential issue | 🟡 MinorValidation path is skipped in both send flows.
Line 176 and Line 278 set
skipValidation: true, so this suite won’t catch regressions in destination-token-aware validation. Keep at least one happy-path send with validation enabled.Based on learnings,
Applies to **/*.{ts,tsx,sol,rs} : Include tests for new functionality, especially edge cases.Also applies to: 278-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/cli/src/tests/ethereum/warp/warp-multi-collateral.e2e-test.ts` around lines 176 - 177, Tests currently set skipValidation: true in both send flows, so destination-token-aware validation regressions won't be caught; change one of the happy-path send calls that currently passes skipValidation: true to skipValidation: false (or remove the flag) so validation runs, or add a separate happy-path test that calls the same send helper with skipValidation: false; update assertions to reflect successful validated sends. Locate the objects/ calls that include the skipValidation property (the send/send flow helper in warp-multi-collateral.e2e-test.ts) and ensure at least one invocation uses skipValidation: false and asserts the expected validated outcome.typescript/warp-monitor/src/service.ts-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorTighten
EXPLORER_QUERY_LIMITparsing to reject malformed input.
parseIntis like a donkey—it'll take what it can get. Give it"200ms"and it's happy with200, ignoring the trailing garbage.Number()withNumber.isInteger()is stricter and will properly reject that nonsense.🔧 Suggested patch
- if (process.env.EXPLORER_QUERY_LIMIT) { - const parsed = parseInt(process.env.EXPLORER_QUERY_LIMIT, 10); - if (isNaN(parsed) || parsed <= 0) { + if (process.env.EXPLORER_QUERY_LIMIT) { + const parsed = Number(process.env.EXPLORER_QUERY_LIMIT); + if (!Number.isInteger(parsed) || parsed <= 0) { rootLogger.error('EXPLORER_QUERY_LIMIT must be a positive integer'); process.exit(1); } explorerQueryLimit = parsed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/warp-monitor/src/service.ts` around lines 59 - 63, The current parsing of EXPLORER_QUERY_LIMIT uses parseInt which accepts trailing non-numeric characters; replace that logic to use Number() and Number.isInteger to strictly validate the entire string is an integer and > 0. In the EXPLORER_QUERY_LIMIT block, compute parsed = Number(process.env.EXPLORER_QUERY_LIMIT), then check Number.isInteger(parsed) && parsed > 0; if the check fails call rootLogger.error('EXPLORER_QUERY_LIMIT must be a positive integer') and process.exit(1) as before. Ensure you keep the same variable names (EXPLORER_QUERY_LIMIT, parsed) and error/exit behavior (rootLogger.error, process.exit) so callers and logs remain consistent.typescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.test.ts-32-70 (1)
32-70:⚠️ Potential issue | 🟡 MinorAdd coverage for
tokenQuoteAmount < inputAmountpath.Current tests miss the branch where token quote is lower than input; that path should return
externalFeeAmountonly.Suggested extra test
+ it('uses only external fee when quoted token out is below input', async () => { + const quoteTransferRemoteTo = sinon.stub().resolves([ + { amount: BigNumber.from('9'), token: COLLATERAL_ADDRESS }, // igp + { amount: BigNumber.from('900'), token: COLLATERAL_ADDRESS }, // token out < input + { amount: BigNumber.from('33'), token: COLLATERAL_ADDRESS }, // external fee + ] as any); + (adapter as any).contract = { quoteTransferRemoteTo }; + + const quote = await adapter.quoteTransferRemoteToGas({ + destination: DESTINATION_DOMAIN, + recipient: RECIPIENT, + amount: 1000n, + targetRouter: TARGET_ROUTER, + }); + + expect(quote.igpQuote.amount).to.equal(9n); + expect(quote.tokenFeeQuote?.amount).to.equal(33n); + expect(quote.tokenFeeQuote?.addressOrDenom).to.equal(COLLATERAL_ADDRESS); + });As per coding guidelines "Include tests for new functionality, especially edge cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.test.ts` around lines 32 - 70, Add a test in EvmMultiCollateralAdapter.test.ts to cover the branch where the summed token quote amount is less than the input amount: stub the adapter.contract.quoteTransferRemoteTo to return amounts that make tokenQuoteAmount < inputAmount (e.g., two small token amounts plus an external fee), call adapter.quoteTransferRemoteToGas with that input, and assert that quote.igpQuote.amount matches the IGP value, quote.tokenFeeQuote.amount equals only the external fee (not a negative or combined value), and quote.tokenFeeQuote.addressOrDenom equals COLLATERAL_ADDRESS; use the existing patterns (quoteTransferRemoteTo stub, adapter as any, EXPECT assertions) to match the other tests.typescript/sdk/src/token/adapters/EvmTokenAdapter.ts-324-329 (1)
324-329:⚠️ Potential issue | 🟡 MinorReplace
anywith explicit typing for quote parameters.Both locations use
anyfor contract return values—violates the guideline to prefer proper typing. Since ethers.js contract calls with dynamic method selection return untyped results, define an intermediate type for the raw contract return instead of using inlineany:type RawQuoteResult = { 0: string; 1: BigNumber | ethers.BigNumberish };Then update both maps:
- Line 324:
(quote: RawQuoteResult)instead of(quote: { 0: string; 1: any })- Line 561: Create matching type for named properties
{ token: string; amount: BigNumber | ethers.BigNumberish }instead of{ token: string; amount: any }This preserves safety while removing the
anyescape hatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/adapters/EvmTokenAdapter.ts` around lines 324 - 329, Define a proper RawQuoteResult type and use it in the feeQuotes.map callback instead of any: declare a type like RawQuoteResult = { 0: string; 1: BigNumber | ethers.BigNumberish } and change the tokenFeeQuotes mapping callback signature from (quote: { 0: string; 1: any }) to (quote: RawQuoteResult) so amount is converted from a known BigNumber-ish type; likewise add a matching typed interface for the other map (the one at the later mapping that currently uses { token: string; amount: any })—e.g., { token: string; amount: BigNumber | ethers.BigNumberish }—and replace the any there to ensure both mappings in EvmTokenAdapter.ts use explicit BigNumber-compatible types.typescript/cli/src/deploy/warp.ts-1356-1357 (1)
1356-1357:⚠️ Potential issue | 🟡 MinorPrevent duplicate route IDs in combine input.
Line 1356 checks count only. Duplicate IDs can silently generate invalid combine output.
💡 Proposed fix
assert(routeIds.length >= 2, 'At least 2 route IDs are required to combine'); + assert( + new Set(routeIds).size === routeIds.length, + 'Duplicate route IDs are not allowed in --routes', + );As per coding guidelines: Use
assert()for validating preconditions and invariants with clear error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/cli/src/deploy/warp.ts` around lines 1356 - 1357, The current precondition only checks count via assert(routeIds.length >= 2) but allows duplicate IDs; update the validation to assert that the input contains no duplicates by verifying new Set(routeIds).size === routeIds.length and assert that there are at least 2 unique IDs (e.g. assert(new Set(routeIds).size >= 2, 'At least 2 unique route IDs are required to combine')), so locate the assert call around routeIds (the assert(routeIds.length >= 2...) statement) and replace/augment it with a uniqueness check and a clear error message.solidity/multicollateral/contracts/MultiCollateral.sol-28-39 (1)
28-39:⚠️ Potential issue | 🟡 MinorAdd explicit token-behavior support note in NatSpec.
The contract docs should state support/non-support for rebasing tokens, fee-on-transfer tokens, and ERC-777 hooks.
Based on learnings: Document rebasing token, fee-on-transfer, and ERC-777 support status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/contracts/MultiCollateral.sol` around lines 28 - 39, Update the NatSpec for the MultiCollateral contract to explicitly state token-behavior support: mention whether rebasing tokens are supported or not and what implications that has for accounting, whether fee-on-transfer (tax) tokens are supported and how transfer amounts/expectations are handled, and whether ERC-777 hooks (tokens with send/receive hooks) are supported or disallowed; reference the contract name MultiCollateral and the overridden entrypoint handle() and the parent HypERC20Collateral so readers know where behavior matters and where to look for transfer/approval logic. Include short guidance for integrators (e.g., "not supported — will break accounting" or "supported with caveats — callers must use balance checks") for each of the three behaviors. Ensure the note is placed in the contract-level NatSpec block at the top of MultiCollateral.
🧹 Nitpick comments (6)
solidity/multicollateral/tsconfig.json (1)
1-14: Consider extending the shared tsconfig for consistency.Most packages in this monorepo extend
@hyperlane-xyz/tsconfig/tsconfig.json(likesolidity/tsconfig.jsondoes). This standalone config works fine, but extendin' the shared base keeps things consistent across the swamp and ensures any monorepo-wide TypeScript settings get picked up.♻️ Suggested refactor
{ + "extends": "@hyperlane-xyz/tsconfig/tsconfig.json", "compilerOptions": { - "target": "ES2020", - "module": "Node16", - "moduleResolution": "Node16", - "strict": true, - "esModuleInterop": true, - "skipLibCheck": true, "outDir": "./dist", "declaration": true }, "include": ["index.ts", "typechain/**/*.ts"], "exclude": ["dist", "artifacts", "cache", "forge-cache", "out", "test"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/tsconfig.json` around lines 1 - 14, Replace the standalone compiler settings in this package tsconfig.json with an extension of the shared base (extend "@hyperlane-xyz/tsconfig/tsconfig.json") so the package inherits monorepo-wide TypeScript defaults; keep or override only package-specific keys such as "include" and "exclude" and any necessary compilerOptions differences (e.g., "outDir" or "declaration") while removing duplicated options already defined in the shared config to avoid drift—update the tsconfig at the top-level of this file (the root JSON object) so it uses extends and preserves the existing include/exclude entries.solidity/multicollateral/contracts/interfaces/IMultiCollateralFee.sol (1)
6-13: Add support-status note on the public fee interface.Please add NatSpec clarifying rebasing-token, fee-on-transfer, and ERC-777 support expectations for implementers/integrators.
📝 Suggested patch
interface IMultiCollateralFee { + /// `@notice` Quotes fees for routing to a specific target router. + /// `@dev` Implementations should explicitly document support status for: + /// rebasing tokens, fee-on-transfer tokens, and ERC-777 tokens. function quoteTransferRemoteTo( uint32 _destination, bytes32 _recipient, uint256 _amount, bytes32 _targetRouter ) external view returns (Quote[] memory); }As per coding guidelines "Document rebasing token, fee-on-transfer, and ERC-777 support status".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/contracts/interfaces/IMultiCollateralFee.sol` around lines 6 - 13, Add NatSpec comments to the IMultiCollateralFee interface explaining support expectations: document on the interface level (IMultiCollateralFee) that implementers should assume no special handling for rebasing tokens, that fee-on-transfer tokens will change quoted amounts and thus callers must handle slippage or use wrappers, and that ERC-777 hooks are not relied upon by quoteTransferRemoteTo so integrators must not depend on operator hooks for fee calculation; attach these notes above the interface and the quoteTransferRemoteTo declaration so integrators/implementers see both general and function-specific guidance.solidity/multicollateral/foundry.toml (1)
12-13: Consider increasing fuzz runs for security-critical contracts.50 fuzz runs is a bit light for contracts handling cross-chain collateral transfers and fee routing. Might want to bump this up for better coverage when you're not in a rush, maybe 256 or more in CI. Though I get it - sometimes you gotta move fast and break... actually no, let's not break things in the swamp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/foundry.toml` around lines 12 - 13, The fuzz configuration currently sets runs = 50 under the [fuzz] section; update the foundry fuzz runs to a higher value (e.g., set runs = 256 or greater) in the [fuzz] block so CI and local security testing perform many more iterations for multicollateral/foundry tests; locate the [fuzz] section in foundry.toml and change the runs value accordingly (consider 256+ for CI, optionally parameterize via an env variable if you want different local vs CI settings).typescript/warp-monitor/src/monitor.test.ts (1)
40-41: Drop theas anybandages in test scaffolding; type those stubs properly.Like ogres, tests got layers—and right now there's three spots where we're just castin' to
anyinstead of buildin' 'em right. Line 40 passes an empty object, line 74 casts a well-formed array, and line 106 reaches into private methods. Use actual typed test helpers or partial objects that respect the real types, so refactors don't slip through invisible cracks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/warp-monitor/src/monitor.test.ts` around lines 40 - 41, Replace the three unsafe "as any" test bandages with properly typed test doubles: instead of "{} as any" provide a Partial<T> or a jest.Mocked<T> that matches the real dependency/interface used in monitor.test.ts (e.g., Partial<MyDependency> or jest.Mocked<MyDependency>), replace the "[] as any" array cast with a typed array literal (e.g., MyItemType[] or Array<MyItemType>) or a factory helper that returns correctly typed test items, and stop reaching into private methods by exercising the public API of the class under test (e.g., WarpMonitor) or by creating a typed test instance via "as unknown as WarpMonitor" only as a last resort; where helpful add small factory helpers (createMockDependency(), createTestItems()) to produce the correctly typed stubs instead of using any.typescript/cli/src/commands/warp.ts (1)
204-214: Redundant assertion foroutputId.Look here, the
output-warp-route-idoption is already declared withdemandOption: trueon line 208, so by the time the handler runs,outputIdis guaranteed to exist. Thatassert(outputId, ...)on line 214 is doin' the same job twice – like puttin' an extra lock on the outhouse door when you already got one.You can safely drop the assert or keep it for belt-and-suspenders if that's how ye like it.
🔧 Optional: Remove redundant assertion
handler: async ({ context, routes, 'output-warp-route-id': outputId }) => { logCommandHeader('Hyperlane Warp Combine'); - assert(outputId, '--output-warp-route-id is required'); const routeIds = routes.split(',').map((r) => r.trim());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/cli/src/commands/warp.ts` around lines 204 - 214, The handler for the warp command redundantly checks outputId with assert even though the 'output-warp-route-id' option is declared with demandOption: true; remove the assert(outputId, '--output-warp-route-id is required') from the handler (or alternatively keep it if you intentionally want defensive runtime checks) so the handler no longer duplicates validation; look for the handler function and the 'output-warp-route-id' option definition to locate and edit the redundant assertion.typescript/sdk/src/token/deploy.ts (1)
767-773: Remove the double cast—contracts should satisfy the interface directly.The
as unknown as TokenRouteron line 770 works but sidesteps a real type issue. UnlikeHypERC721Deployer.router()(which casts nothing), the ERC20 deployer's heterogeneous factory contracts don't align withTokenRouter. All these different contract types—HypERC20,HypFiatToken,HypNative, etc.—end up getting cast the same way, which suggests they should share a common interface or the type definitions need fixing.Either ensure all contracts in
HypERC20Factoriesproperly satisfyTokenRouter, or reconsider the factory structure. Casting masks the root cause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/deploy.ts` around lines 767 - 773, The router function is masking a typing problem by using "as unknown as TokenRouter" on the returned value; remove the double cast and make the types align instead: update the declaration/definition of HypERC20Factories (or the router parameter type) so that the values are typed as or extend TokenRouter, or change the function signature to accept HyperlaneContracts<TokenRouter>, then return contracts[key] directly in router(contracts: ...); ensure objKeys(hypERC20factories) and the lookup contracts[key] remain consistent with the new type so no cast is necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 207-224: The current block always charges and approves a hook fee
when _feeHook != address(0) even if _quoteGasPayment(...) returns zero (causing
same-domain transfers to be incorrectly charged); update the logic in the branch
that calls _quoteGasPayment (the code referencing _feeHook, _quoteGasPayment,
IERC20(...).safeTransferFrom, and IERC20(...).approve) to first store hookFee
and only perform charge/transfer/approve when hookFee > 0, handling both ERC20
token paths (token == address(this) vs token != address(this)) inside that
conditional so zero quotes are no-ops.
- Around line 275-277: Before invoking MultiCollateral(...).handle(...) on the
enrolled router address, add the same contract-existence guard used elsewhere:
check that MultiCollateral(_targetRouter.bytes32ToAddress()) is a contract
(e.g., extcodesize > 0) and revert or skip with a clear error if not; update the
same-domain call site that uses _targetRouter, bytes32ToAddress, and handle to
perform this check and only proceed to transfer remainingValue + call handle
when the check passes to avoid calling an EOA.
- Line 223: Replace the raw IERC20(...).approve(...) call with the SafeERC20
"force approve" helper to handle non-standard tokens: add or ensure "using
SafeERC20 for IERC20;" and call the forceApprove variant on the token (e.g.,
IERC20(_token).forceApprove(_feeHook, hookFee) or
SafeERC20.forceApprove(IERC20(_token), _feeHook, hookFee)), and import
OpenZeppelin's SafeERC20 so approvals are executed safely for tokens that
require zeroing first or return no bool.
In `@solidity/multicollateral/contracts/MultiCollateralRoutingFee.sol`:
- Around line 25-27: The constructor currently allows deploying with address(0)
as owner which locks out admin actions; in the MultiCollateralRoutingFee
constructor add a validation that _owner != address(0) (e.g. require check)
before calling _transferOwnership(_owner) so deployment reverts on a zero
address and ensures valid Ownable ownership.
- Around line 32-50: The setters setFeeContract and setRouterFeeContract
currently accept EOAs which later cause delegation reverts; update both to
validate that the feeContract is either the zero address (to unset) or a
deployed contract by checking extcodesize (or using OpenZeppelin
Address.isContract) and revert with a clear message if not; keep the existing
assignments to feeContracts and the FeeContractSet emit but add the require
check (referencing setFeeContract, setRouterFeeContract, feeContracts, and
FeeContractSet) before writing state.
In `@solidity/multicollateral/package.json`:
- Around line 37-43: The package.json currently lists "@typechain/ethers-v5",
"typechain", and "typescript" under "dependencies"; move these three entries
into "devDependencies" instead (leaving runtime deps like "@hyperlane-xyz/core"
in "dependencies") so build-only tools are not shipped to consumers; update the
"devDependencies" object to include "@typechain/ethers-v5", "typechain", and
"typescript" with the same versions and remove them from "dependencies".
In `@typescript/cli/src/send/transfer.ts`:
- Around line 210-213: Add precondition checks using assert from
`@hyperlane-xyz/utils` before parsing receipts: import assert and
assert(txReceipts.length > 0, 'No transaction receipts found for
ProviderType.EthersV5') before accessing transferTxReceipt (the result of
txReceipts[txReceipts.length - 1]), and also assert that the first dispatched
message exists (e.g., assert(message, 'No dispatched message found in
transferTxReceipt')) before calling HyperlaneCore.getDispatchedMessages or using
message to avoid downstream undefined errors.
In `@typescript/cli/src/tests/ethereum/warp/warp-multi-collateral.e2e-test.ts`:
- Around line 168-183: Record the recipient's pre-send balance with
usdtChain3.balanceOf(ownerAddress) before calling hyperlaneWarpSendRelay, then
after the call compute the delta (postBalance.sub(preBalance)) and assert that
delta equals the expected received amount (use parseUnits('1', 18) or derive
from sendAmount) instead of using recipientBalance.gte(...); update the test
around the sendAmount, hyperlaneWarpSendRelay invocation, and
usdtChain3.balanceOf(ownerAddress) to perform this pre/post delta assertion.
In `@typescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.ts`:
- Around line 209-216: The code assumes the array returned by
this.contract.quoteTransferRemoteTo has at least three entries and directly
indexes quotes[0] (and elsewhere at lines ~238-247); add a precondition using
assert() from `@hyperlane-xyz/utils` to validate the quotes shape/length before
positional indexing (e.g., assert(Array.isArray(quotes) && quotes.length >= 3,
"Invalid quote shape from quoteTransferRemoteTo")), then safely read nativeGas
and the other quoted amounts after the assertion so the
EvmMultiCollateralAdapter method using quotes, nativeGas, recipientBytes32, and
targetRouterBytes32 will not crash or misquote if the returned shape changes.
In `@typescript/sdk/src/token/Token.ts`:
- Around line 252-256: The code in Token.ts currently constructs an
EvmHypMultiCollateralAdapter and silently falls back collateralToken to
addressOrDenom when collateralAddressOrDenom is missing; instead, add an
invariant/assert that collateralAddressOrDenom is provided before creating the
adapter (e.g., assert(collateralAddressOrDenom, "collateralAddressOrDenom is
required for EvmHypMultiCollateral")), and then pass collateralAddressOrDenom
into the EvmHypMultiCollateralAdapter constructor so we fail fast rather than
defaulting to the router address.
In `@typescript/sdk/src/token/types.ts`:
- Around line 264-266: enrolledRouters currently allows arbitrary string keys
which lets invalid router domain keys through; change its key schema to the
existing router-domain validator instead of z.string() (e.g. replace z.string()
with the shared RouterDomainSchema or a z.string().regex(...) used elsewhere) so
the property becomes z.record(RouterDomainSchema, z.array(ZHash)).optional()
(and import/ reuse the same RouterDomainSchema symbol used across config
schemas) to match existing Zod patterns and prevent invalid keys from reaching
router-enrollment tx generation.
In `@typescript/sdk/src/warp/WarpCore.ts`:
- Around line 1143-1147: The conversion direction is reversed when computing
minOriginTransferAmount: convertDecimalsToIntegerString is being called with
originToken.decimals then resolvedDestinationToken.decimals but
minDestinationTransferAmount is expressed in destination decimals; update the
call in the minOriginTransferAmount computation (the line invoking
resolvedDestinationToken.amount and convertDecimalsToIntegerString) to pass
resolvedDestinationToken.decimals first and originToken.decimals second so the
conversion is destination → origin for minDestinationTransferAmount before
creating the origin amount.
- Around line 619-636: The multi-collateral approval branch uses
adapter.isApproveRequired and directly pushes an Approval tx via
adapter.populateApproveTx, but it must mirror the revoke-first logic used in the
other path: when a token requires revoke-first (e.g., USDT) insert a revoke
transaction before the approve. Modify the branch around
adapter.isApproveRequired to detect the revoke-first condition (reuse the same
helper/condition used in the standard path) and, if needed, call the existing
revoke population routine to push a WarpTxCategory.Revoke WarpTypedTransaction
into transactions prior to pushing the Approval populated by
adapter.populateApproveTx; ensure you use the same transaction shape and
ordering as the standard flow so allowance updates succeed.
In `@typescript/warp-monitor/src/explorer.ts`:
- Around line 247-248: The code currently returns an empty array from the
GraphQL response which hides server-side errors; update the logic in the
function that awaits response.json() (the code around the const payload = await
response.json() / return ... as ExplorerMessageRow[]) to check payload.errors
and handle them instead of silently falling back to []: if payload.errors exists
and is non-empty, either throw an Error (including payload.errors and relevant
context) or propagate a rejected result so callers can distinguish a failed
GraphQL query from a legitimately empty data set; only return
payload.data.message_view as ExplorerMessageRow[] when payload.errors is absent
and payload.data is present.
- Around line 228-232: The fetch call that posts to this.apiUrl must be guarded
by a timeout and must surface GraphQL errors instead of returning empty arrays:
create an AbortController, pass its signal into fetch(this.apiUrl, ...), and set
a clearable setTimeout to call controller.abort() after a configured timeout
(e.g., 10s); after awaiting the response check response.ok and throw a
descriptive error if not; parse the JSON and if json.errors exists or json.data
is missing throw an error containing those details so callers fail instead of
receiving empty results; ensure the timeout is cleared on success so the
controller isn't left pending and let fetch exceptions (including abort)
propagate or be wrapped with context.
In `@typescript/warp-monitor/src/monitor.ts`:
- Around line 355-383: The code currently calls updateInventoryBalanceMetrics
with inventoryBalance defaulting to 0n even when the balance read failed; change
the flow so updateInventoryBalanceMetrics is only invoked when the balance read
actually succeeded. Use the result/boolean from tryFn (or wrap the
adapter.getBalance call in a try/catch) to detect failure and skip calling
updateInventoryBalanceMetrics if the read failed; keep references to tryFn,
adapter.getBalance, inventoryBalance, updateInventoryBalanceMetrics, and
this.formatTokenAmount to locate and update the logic.
---
Minor comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 28-39: Update the NatSpec for the MultiCollateral contract to
explicitly state token-behavior support: mention whether rebasing tokens are
supported or not and what implications that has for accounting, whether
fee-on-transfer (tax) tokens are supported and how transfer amounts/expectations
are handled, and whether ERC-777 hooks (tokens with send/receive hooks) are
supported or disallowed; reference the contract name MultiCollateral and the
overridden entrypoint handle() and the parent HypERC20Collateral so readers know
where behavior matters and where to look for transfer/approval logic. Include
short guidance for integrators (e.g., "not supported — will break accounting" or
"supported with caveats — callers must use balance checks") for each of the
three behaviors. Ensure the note is placed in the contract-level NatSpec block
at the top of MultiCollateral.
In `@typescript/cli/src/deploy/warp.ts`:
- Around line 1356-1357: The current precondition only checks count via
assert(routeIds.length >= 2) but allows duplicate IDs; update the validation to
assert that the input contains no duplicates by verifying new Set(routeIds).size
=== routeIds.length and assert that there are at least 2 unique IDs (e.g.
assert(new Set(routeIds).size >= 2, 'At least 2 unique route IDs are required to
combine')), so locate the assert call around routeIds (the
assert(routeIds.length >= 2...) statement) and replace/augment it with a
uniqueness check and a clear error message.
In `@typescript/cli/src/tests/ethereum/warp/warp-multi-collateral.e2e-test.ts`:
- Around line 176-177: Tests currently set skipValidation: true in both send
flows, so destination-token-aware validation regressions won't be caught; change
one of the happy-path send calls that currently passes skipValidation: true to
skipValidation: false (or remove the flag) so validation runs, or add a separate
happy-path test that calls the same send helper with skipValidation: false;
update assertions to reflect successful validated sends. Locate the objects/
calls that include the skipValidation property (the send/send flow helper in
warp-multi-collateral.e2e-test.ts) and ensure at least one invocation uses
skipValidation: false and asserts the expected validated outcome.
In `@typescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.test.ts`:
- Around line 32-70: Add a test in EvmMultiCollateralAdapter.test.ts to cover
the branch where the summed token quote amount is less than the input amount:
stub the adapter.contract.quoteTransferRemoteTo to return amounts that make
tokenQuoteAmount < inputAmount (e.g., two small token amounts plus an external
fee), call adapter.quoteTransferRemoteToGas with that input, and assert that
quote.igpQuote.amount matches the IGP value, quote.tokenFeeQuote.amount equals
only the external fee (not a negative or combined value), and
quote.tokenFeeQuote.addressOrDenom equals COLLATERAL_ADDRESS; use the existing
patterns (quoteTransferRemoteTo stub, adapter as any, EXPECT assertions) to
match the other tests.
In `@typescript/sdk/src/token/adapters/EvmTokenAdapter.ts`:
- Around line 324-329: Define a proper RawQuoteResult type and use it in the
feeQuotes.map callback instead of any: declare a type like RawQuoteResult = { 0:
string; 1: BigNumber | ethers.BigNumberish } and change the tokenFeeQuotes
mapping callback signature from (quote: { 0: string; 1: any }) to (quote:
RawQuoteResult) so amount is converted from a known BigNumber-ish type; likewise
add a matching typed interface for the other map (the one at the later mapping
that currently uses { token: string; amount: any })—e.g., { token: string;
amount: BigNumber | ethers.BigNumberish }—and replace the any there to ensure
both mappings in EvmTokenAdapter.ts use explicit BigNumber-compatible types.
In `@typescript/sdk/src/token/EvmWarpModule.hardhat-test.ts`:
- Around line 219-224: Add a lightweight test that exercises the
TokenType.multiCollateral branch by creating a config object using the existing
baseConfig, token.address and allowedRebalancers (mirroring the diff block that
sets type: TokenType.multiCollateral) and assert the SDK/config helper returns
or accepts that config (e.g., no-throw or matches expected shape) without
deploying; specifically add a non-deploy unit test that constructs the
multiCollateral config, calls the same config/validation helper used elsewhere
in this file, and asserts the result is valid so the TokenType.multiCollateral
path is covered.
In `@typescript/warp-monitor/src/service.ts`:
- Around line 59-63: The current parsing of EXPLORER_QUERY_LIMIT uses parseInt
which accepts trailing non-numeric characters; replace that logic to use
Number() and Number.isInteger to strictly validate the entire string is an
integer and > 0. In the EXPLORER_QUERY_LIMIT block, compute parsed =
Number(process.env.EXPLORER_QUERY_LIMIT), then check Number.isInteger(parsed) &&
parsed > 0; if the check fails call rootLogger.error('EXPLORER_QUERY_LIMIT must
be a positive integer') and process.exit(1) as before. Ensure you keep the same
variable names (EXPLORER_QUERY_LIMIT, parsed) and error/exit behavior
(rootLogger.error, process.exit) so callers and logs remain consistent.
---
Nitpick comments:
In `@solidity/multicollateral/contracts/interfaces/IMultiCollateralFee.sol`:
- Around line 6-13: Add NatSpec comments to the IMultiCollateralFee interface
explaining support expectations: document on the interface level
(IMultiCollateralFee) that implementers should assume no special handling for
rebasing tokens, that fee-on-transfer tokens will change quoted amounts and thus
callers must handle slippage or use wrappers, and that ERC-777 hooks are not
relied upon by quoteTransferRemoteTo so integrators must not depend on operator
hooks for fee calculation; attach these notes above the interface and the
quoteTransferRemoteTo declaration so integrators/implementers see both general
and function-specific guidance.
In `@solidity/multicollateral/foundry.toml`:
- Around line 12-13: The fuzz configuration currently sets runs = 50 under the
[fuzz] section; update the foundry fuzz runs to a higher value (e.g., set runs =
256 or greater) in the [fuzz] block so CI and local security testing perform
many more iterations for multicollateral/foundry tests; locate the [fuzz]
section in foundry.toml and change the runs value accordingly (consider 256+ for
CI, optionally parameterize via an env variable if you want different local vs
CI settings).
In `@solidity/multicollateral/tsconfig.json`:
- Around line 1-14: Replace the standalone compiler settings in this package
tsconfig.json with an extension of the shared base (extend
"@hyperlane-xyz/tsconfig/tsconfig.json") so the package inherits monorepo-wide
TypeScript defaults; keep or override only package-specific keys such as
"include" and "exclude" and any necessary compilerOptions differences (e.g.,
"outDir" or "declaration") while removing duplicated options already defined in
the shared config to avoid drift—update the tsconfig at the top-level of this
file (the root JSON object) so it uses extends and preserves the existing
include/exclude entries.
In `@typescript/cli/src/commands/warp.ts`:
- Around line 204-214: The handler for the warp command redundantly checks
outputId with assert even though the 'output-warp-route-id' option is declared
with demandOption: true; remove the assert(outputId, '--output-warp-route-id is
required') from the handler (or alternatively keep it if you intentionally want
defensive runtime checks) so the handler no longer duplicates validation; look
for the handler function and the 'output-warp-route-id' option definition to
locate and edit the redundant assertion.
In `@typescript/sdk/src/token/deploy.ts`:
- Around line 767-773: The router function is masking a typing problem by using
"as unknown as TokenRouter" on the returned value; remove the double cast and
make the types align instead: update the declaration/definition of
HypERC20Factories (or the router parameter type) so that the values are typed as
or extend TokenRouter, or change the function signature to accept
HyperlaneContracts<TokenRouter>, then return contracts[key] directly in
router(contracts: ...); ensure objKeys(hypERC20factories) and the lookup
contracts[key] remain consistent with the new type so no cast is necessary.
In `@typescript/warp-monitor/src/monitor.test.ts`:
- Around line 40-41: Replace the three unsafe "as any" test bandages with
properly typed test doubles: instead of "{} as any" provide a Partial<T> or a
jest.Mocked<T> that matches the real dependency/interface used in
monitor.test.ts (e.g., Partial<MyDependency> or jest.Mocked<MyDependency>),
replace the "[] as any" array cast with a typed array literal (e.g.,
MyItemType[] or Array<MyItemType>) or a factory helper that returns correctly
typed test items, and stop reaching into private methods by exercising the
public API of the class under test (e.g., WarpMonitor) or by creating a typed
test instance via "as unknown as WarpMonitor" only as a last resort; where
helpful add small factory helpers (createMockDependency(), createTestItems()) to
produce the correctly typed stubs instead of using any.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.changeset/multicollateral-typechain-package.mdDockerfiledocs/multi-collateral-design.mdpnpm-workspace.yamlsolidity/eslint.config.mjssolidity/multicollateral/contracts/MultiCollateral.solsolidity/multicollateral/contracts/MultiCollateralRoutingFee.solsolidity/multicollateral/contracts/interfaces/IMultiCollateralFee.solsolidity/multicollateral/foundry.tomlsolidity/multicollateral/hardhat.config.ctssolidity/multicollateral/index.tssolidity/multicollateral/package.jsonsolidity/multicollateral/remappings.txtsolidity/multicollateral/test/MultiCollateral.t.solsolidity/multicollateral/tsconfig.jsonsolidity/tsconfig.jsontypescript/Dockerfile.node-servicetypescript/cli/src/commands/warp.tstypescript/cli/src/config/warp.tstypescript/cli/src/deploy/warp.test.tstypescript/cli/src/deploy/warp.tstypescript/cli/src/send/transfer.tstypescript/cli/src/tests/commands/helpers.tstypescript/cli/src/tests/ethereum/commands/warp.tstypescript/cli/src/tests/ethereum/warp/warp-multi-collateral.e2e-test.tstypescript/sdk/package.jsontypescript/sdk/src/index.tstypescript/sdk/src/token/EvmWarpModule.hardhat-test.tstypescript/sdk/src/token/EvmWarpModule.tstypescript/sdk/src/token/EvmWarpRouteReader.tstypescript/sdk/src/token/IToken.tstypescript/sdk/src/token/Token.test.tstypescript/sdk/src/token/Token.tstypescript/sdk/src/token/TokenStandard.tstypescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.test.tstypescript/sdk/src/token/adapters/EvmMultiCollateralAdapter.tstypescript/sdk/src/token/adapters/EvmTokenAdapter.tstypescript/sdk/src/token/config.tstypescript/sdk/src/token/contracts.tstypescript/sdk/src/token/deploy.tstypescript/sdk/src/token/tokenMetadataUtils.tstypescript/sdk/src/token/types.tstypescript/sdk/src/warp/WarpCore.test.tstypescript/sdk/src/warp/WarpCore.tstypescript/warp-monitor/src/explorer.test.tstypescript/warp-monitor/src/explorer.tstypescript/warp-monitor/src/metrics.test.tstypescript/warp-monitor/src/metrics.tstypescript/warp-monitor/src/monitor.test.tstypescript/warp-monitor/src/monitor.tstypescript/warp-monitor/src/service.tstypescript/warp-monitor/src/types.test.tstypescript/warp-monitor/src/types.ts
solidity/multicollateral/contracts/MultiCollateralRoutingFee.sol
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
typescript/warp-monitor/src/explorer.ts (1)
232-253:⚠️ Potential issue | 🟠 MajorDon’t treat explorer failures as empty inflight state.
Right now GraphQL body errors can collapse to
[], and fetch has no timeout. That can hide backend failures and stall polling.Patch idea
private async queryInflightTransfers( limit: number, ): Promise<ExplorerMessageRow[]> { @@ - const response = await fetch(this.apiUrl, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ query, variables }), - }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10_000); + let response: Response; + try { + response = await fetch(this.apiUrl, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ query, variables }), + signal: controller.signal, + }); + } finally { + clearTimeout(timeoutId); + } @@ - const payload = await response.json(); - return (payload?.data?.message_view ?? []) as ExplorerMessageRow[]; + const payload: { + data?: { message_view?: ExplorerMessageRow[] }; + errors?: unknown; + } = await response.json(); + + if (payload.errors) { + throw new Error( + `Explorer GraphQL error: ${JSON.stringify(payload.errors)}`, + ); + } + if (!payload.data) { + throw new Error('Explorer query missing data field'); + } + + return payload.data.message_view ?? []; }As per coding guidelines: "Use try/catch for external system calls (RPC requests, file I/O)" and "Do not swallow errors silently; always log or re-throw caught exceptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/warp-monitor/src/explorer.ts` around lines 232 - 253, The current explorer query swallows backend failures (no timeout on fetch and GraphQL errors collapse to an empty array), so wrap the network call in try/catch, use an AbortController with a reasonable timeout to cancel stalled fetches, and after receiving the response validate the GraphQL payload: if payload.errors exists or payload.data?.message_view is missing/undefined, log/throw an Error containing response.status, statusText and the parsed error/details instead of returning [] — update the method that uses this.apiUrl/query/variables and return the typed ExplorerMessageRow[] only when payload.data.message_view is present.
🧹 Nitpick comments (2)
typescript/warp-monitor/src/explorer.ts (2)
125-136: Useunknownnarrowing instead ofas Errorcast.Tiny cleanup: avoid bandaid cast in catch path and narrow safely.
Patch idea
- } catch (error) { + } catch (error: unknown) { + const errorMessage = + error instanceof Error ? error.message : String(error); this.logger.debug( { messageId: row.msg_id, destinationDomainId: row.destination_domain_id, destinationRouter, - error: (error as Error).message, + error: errorMessage, }, 'Skipping explorer message with unparsable warp message body', ); continue; }As per coding guidelines: "In catch blocks, catch unknown and narrow with type guards" and "Avoid unnecessary type casts (
asassertions)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/warp-monitor/src/explorer.ts` around lines 125 - 136, The catch block currently casts the caught error with (error as Error).message; instead change the catch parameter to catch (error: unknown) and narrow it before use—e.g., compute a message variable using a type guard like error instanceof Error ? error.message : String(error) and pass that variable into this.logger.debug (within the same try/catch around parseWarpRouteMessage/normalizeExplorerHex) along with row.msg_id, row.destination_domain_id, destinationRouter and parsedMessage handling to avoid the unsafe as-cast.
73-85: Assert input invariants for decimals and query limit.Good place for fail-fast guards so bad config doesn’t leak into math/query behavior.
Patch idea
import { + assert, bytes32ToAddress, isValidAddressEvm, isZeroishAddress, parseWarpRouteMessage, } from '@hyperlane-xyz/utils'; @@ export function canonical18ToTokenBaseUnits( amountCanonical18: bigint, tokenDecimals: number, ): bigint { + assert( + Number.isInteger(tokenDecimals) && tokenDecimals >= 0, + `Invalid tokenDecimals: ${tokenDecimals}`, + ); if (tokenDecimals === CANONICAL_DECIMALS) return amountCanonical18; @@ async getPendingDestinationTransfers( limit = 200, ): Promise<PendingDestinationTransfer[]> { + assert(Number.isInteger(limit) && limit > 0, `Invalid limit: ${limit}`); const rows = await this.queryInflightTransfers(limit);As per coding guidelines: "Use assert() from
@hyperlane-xyz/utilsfor validating preconditions and invariants with clear error messages."Also applies to: 112-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/warp-monitor/src/explorer.ts` around lines 73 - 85, Add fail-fast assertions using assert() from `@hyperlane-xyz/utils` in canonical18ToTokenBaseUnits and the related function at lines 112-114: validate that tokenDecimals is a safe non-negative integer within sensible bounds (e.g., 0 <= tokenDecimals <= CANONICAL_DECIMALS or another documented max), and assert amountCanonical18 is a bigint (and non-negative if appropriate); also add an assert for any query limit variables referenced on lines 112-114 to ensure they are positive integers and within expected bounds. Locate canonical18ToTokenBaseUnits and the other function referenced (lines 112-114) and insert clear error messages in the assert calls describing which invariant failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@typescript/warp-monitor/src/explorer.ts`:
- Around line 232-253: The current explorer query swallows backend failures (no
timeout on fetch and GraphQL errors collapse to an empty array), so wrap the
network call in try/catch, use an AbortController with a reasonable timeout to
cancel stalled fetches, and after receiving the response validate the GraphQL
payload: if payload.errors exists or payload.data?.message_view is
missing/undefined, log/throw an Error containing response.status, statusText and
the parsed error/details instead of returning [] — update the method that uses
this.apiUrl/query/variables and return the typed ExplorerMessageRow[] only when
payload.data.message_view is present.
---
Nitpick comments:
In `@typescript/warp-monitor/src/explorer.ts`:
- Around line 125-136: The catch block currently casts the caught error with
(error as Error).message; instead change the catch parameter to catch (error:
unknown) and narrow it before use—e.g., compute a message variable using a type
guard like error instanceof Error ? error.message : String(error) and pass that
variable into this.logger.debug (within the same try/catch around
parseWarpRouteMessage/normalizeExplorerHex) along with row.msg_id,
row.destination_domain_id, destinationRouter and parsedMessage handling to avoid
the unsafe as-cast.
- Around line 73-85: Add fail-fast assertions using assert() from
`@hyperlane-xyz/utils` in canonical18ToTokenBaseUnits and the related function at
lines 112-114: validate that tokenDecimals is a safe non-negative integer within
sensible bounds (e.g., 0 <= tokenDecimals <= CANONICAL_DECIMALS or another
documented max), and assert amountCanonical18 is a bigint (and non-negative if
appropriate); also add an assert for any query limit variables referenced on
lines 112-114 to ensure they are positive integers and within expected bounds.
Locate canonical18ToTokenBaseUnits and the other function referenced (lines
112-114) and insert clear error messages in the assert calls describing which
invariant failed.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nambrot
force-pushed
the
codex/mc-sdk-cli-monitor-public
branch
from
45ebe2d to
42c7f85
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
paulbalaji
reviewed
Mar 2, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review Summary
Request changes — 2 blockers. SDK/CLI/monitor integration is well-structured with strong test coverage (357 SDK, 43 CLI, 28 monitor). Two concrete bugs need fixing before merge.
Positives
- Clean
resolveDestinationTokenabstraction reducing duplication across validation - Comprehensive e2e tests (cross-chain + same-chain local swap via CLI)
- Well-structured
warp combinewith decimal compatibility validation - Warp monitor pending-transfer tracking and projected deficit metrics are operationally valuable
- Explorer client is defensive with proper timeout/abort handling
nambrot
force-pushed
the
codex/mc-sdk-cli-monitor-public
branch
from
42c7f85 to
a514786
Compare
paulbalaji
reviewed
Mar 2, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
3 new findings not yet covered in existing PR comments. All actionable items from the prior team review were either already raised or stale against current HEAD.
nambrot
force-pushed
the
codex/mc-sdk-cli-monitor-public
branch
3 times, most recently
from
f42b97f to
a155230
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
nambrot
commented
Mar 2, 2026
nambrot
commented
Mar 2, 2026
nambrot
commented
Mar 2, 2026
paulbalaji
reviewed
Mar 3, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review Summary
5 net-new findings (deduped against existing PR thread comments).
Medium (2): Double quote RPC in MultiCollateral transfer path; duplicate tokens possible in warp combine merged config.
Low (2): No outputWarpRouteId format validation; magic length-42 check instead of isAddressEvm().
Process (1): No changeset added despite SDK/CLI/monitor package changes.
Process: Missing changeset
This PR touches typescript/sdk, typescript/cli, and typescript/warp-monitor — all published packages. A .changeset/ entry is required per repo guidelines. Past tense, user-impact focused description.
Reviewed with Claude Code agent team (4 parallel reviewers: SDK, WarpCore, CLI, warp-monitor)
paulbalaji
reviewed
Mar 3, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Type Cast Review (follow-up pass)
Scanned all new code for unnecessary as assertions, ! non-null assertions, and double-casts.
4 medium findings posted inline below. Additionally:
WarpCore.ts:663,675,692—as WarpTypedTransactioncasts follow the established pattern in the same file (lines 510, 548, 567). Acceptable.deploy/warp.ts:1385—Object.entries(...) as Array<[string, HypTokenRouterConfig]>is the standardObject.entriesworkaround. Acceptable.WarpCore.ts:182,629—as EvmHypMultiCollateralAdapterdowncasts are guarded byisMultiCollateralTransfer(). Acceptable, though a typedgetMultiCollateralAdapter()would be cleaner long-term.deploy.ts:770—contracts[key] as TokenRouteris likely a typechain limitation. Acceptable with a comment.
- Accept optional pre-fetched interchainGas in populateTransferRemoteToTx to avoid double-quoting - Remove unnecessary non-null assertions on addressOrDenom in WarpCore MC path - Make isMultiCollateralTransfer a type guard to narrow destinationToken - Replace magic number 42 with isAddressEvm in toCanonicalRouterId - Assert addressOrDenom before use in combine instead of ! - Validate outputId is non-empty in combine handler - Assert no duplicate tokens before writing merged config - Use instanceof Error narrowing in explorer.ts catch block
paulbalaji
reviewed
Mar 3, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review: Latest Commits
4ee1934 — "fix: address PR review feedback from paulbalaji"
All 8 fixes from our earlier review are correctly implemented. No new issues introduced.
406c680 — "rebalancer-sim: package scenarios and support SCENARIOS_DIR override"
Core change is clean. Two findings inline below.
typescript/rebalancer-sim/test/scenarios/scenario-loader.test.ts
Outdated
Show resolved
Hide resolved
paulbalaji
reviewed
Mar 3, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review: 03e2cac — "rebalancer-sim: add shared result exporter API"
Clean extraction from test helper to shared module. Logic is 1:1. Two findings inline, one nit in summary.
Nit: No direct unit test for saveSimulationResults. Tested indirectly via simulation-helpers.ts's saveResults, but edge cases (empty results, missing comparison, empty defaultBridgeConfig) are uncovered.
paulbalaji
approved these changes
Mar 4, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Approved
All findings from our review have been addressed across commits 4ee1934, 6532996, and 70e12c8. CI is fully green.
One remaining item before merge: No .changeset/ entry added. SDK, CLI, and warp-monitor are published packages — please add a changeset (past tense, user-impact focused).
Non-blocking nit: ResultsExporter.ts:40,83 still uses any types (const output: any, Record<string, any>). These pre-existed in the test helper and were just extracted — fine to address separately.
Contributor
⚙️ Node Service Docker Images Built Successfully
Full image paths |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
paulbalaji
pushed a commit
that referenced
this pull request
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This stacks on top of
nam/multicollateral-solidityand ports the public TypeScript + monitor portions of MultiCollateral work, while keeping proprietary rebalancer logic private.What This PR Adds
TokenStandard+ token typing paths formultiCollateralEvmMultiCollateralAdapterintegrationWarpCoreImportant Implementation Choices
@hyperlane-xyz/multicollateral(no local ABI wrapper package).TokenType.multiCollateralswitch branch in CLI config by reusing existing token-address case group.Explicitly Out of Scope
ITokenBridge/ routing-fee core changes)Testing
pnpm -C typescript/sdk test:unit(357 passing)pnpm -C typescript/cli test:ci(43 passing)pnpm -C typescript/warp-monitor test(28 passing)Stacking
nam/multicollateral-soliditySummary by CodeRabbit
New Features
Tests
Chores