feat: MultiCollateral solidity package#8246
Merged
Merged
Conversation
Separate pnpm workspace package at solidity/multicollateral/ containing MultiCollateral.sol (multi-router collateral with cross-stablecoin and same-chain swap support), IMultiCollateralFee interface, and MultiCollateralRoutingFee (standalone per-router fee routing). No changes to @hyperlane-xyz/core — all new code imports via @hyperlane-xyz/core/ prefix. 27 forge tests covering cross-chain, same-chain, fees, decimal scaling, enrollment, and security.
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:
📝 WalkthroughWalkthroughA new solidity/multicollateral package was added: MultiCollateral (router enrollment, router-aware quoting/fees, same-chain direct handle and mailbox cross-chain dispatch), MultiCollateralRoutingFee, IMultiCollateralFee, package tooling/configs, typechain exports, and an extensive Forge test suite. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MC_O as MultiCollateral_O
participant FeeO as FeeLookup_O
participant MB_O as Mailbox_O
participant MB_D as Mailbox_D
participant MC_D as MultiCollateral_D
participant FeeD as FeeLookup_D
U->>MC_O: transferRemoteTo(dest, recipient, amount, targetRouter)
activate MC_O
MC_O->>MC_O: auth & enroll checks
MC_O->>FeeO: quoteTransferRemoteTo(dest, recipient, amount, targetRouter)
MC_O->>MC_O: calculate & charge fees
MC_O->>MC_O: burn/prepare message
alt destination is same chain
MC_O->>MC_D: direct handle(origin, sender, message)
else cross-chain
MC_O->>MB_O: dispatch(message)
MB_O->>MB_D: deliver
MB_D->>MC_D: handle(origin, sender, message)
end
deactivate MC_O
activate MC_D
MC_D->>MC_D: verify sender/enrolled router
MC_D->>FeeD: quoteTransferRemoteTo(...) (if required)
MC_D->>MC_D: mint/distribute net amount and fees
deactivate MC_D
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8246 +/- ##
==========================================
+ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
paulbalaji
reviewed
Feb 27, 2026
![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: 1
🧹 Nitpick comments (2)
solidity/multicollateral/contracts/MultiCollateral.sol (1)
40-43: Missing documentation on special token types.The contract handles ERC20 collateral but doesn't document its stance on rebasing tokens, fee-on-transfer tokens, or ERC-777 tokens. Given these can cause accounting mismatches (actual balance vs expected), it'd be wise to add a note in the contract docs about what's supported and what's not.
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 40 - 43, Add a NatSpec/top-level comment to MultiCollateral (and note inheritance from HypERC20Collateral and IMultiCollateralFee) explicitly stating support and limitations for rebasing tokens, fee-on-transfer tokens, and ERC-777 tokens: declare whether each is supported or unsupported, explain that the contract assumes ERC20 semantics where token balance changes match transfer amounts (so rebasing/fee-on-transfer/ERC‑777 hooks can break accounting), and recommend using non‑rebasing/non‑fee tokens or wrapping adapters (or relying on wrappers in HypERC20Collateral) if such tokens must be used; reference the contract name MultiCollateral and the inherited HypERC20Collateral/SafeERC20 usage so reviewers can locate where assumptions are made.solidity/multicollateral/test/MultiCollateral.t.sol (1)
87-90: Add explicit coverage or statement for non-standard ERC20 behavior.Right now the suite exercises vanilla ERC20 flows; it’d help to add dedicated cases (or explicit support-status docs) for rebasing, fee-on-transfer, and ERC-777 semantics so integrators know the guardrails.
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/test/MultiCollateral.t.sol` around lines 87 - 90, Add explicit coverage and/or a clear support-status statement for non-standard token behavior: update the MultiCollateral.t.sol tests to include dedicated cases (or a top-of-file doc comment) addressing rebasing tokens, fee-on-transfer tokens, and ERC-777 semantics by referencing the existing ERC20Test fixtures (originUSDC, originUSDT, destUSDC, destUSDT) and either (a) add mock token variants that simulate rebasing and fee-on-transfer and ERC777 behavior and exercise deposit/withdraw/transfer paths in the MultiCollateral test suite, or (b) add a documented section in the test file that states whether MultiCollateral supports or rejects rebasing, fee-on-transfer, and ERC-777 tokens; ensure each new test or doc entry names the relevant functions (deposit, withdraw, transferCollateral) so integrators can map behavior to code paths.
🤖 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/package.json`:
- Around line 8-13: The package exports currently expose "./contracts" as a
directory but not the per-file wildcard; update the exports map in package.json
by adding a wildcard subpath entry for "./contracts/*" that maps to
"./contracts/*" so consumers can import individual contract files (e.g.,
MultiCollateral.sol, MultiCollateralRoutingFee.sol and their interfaces)
directly; modify the "exports" object (near the existing "./contracts" entry) to
include the "./contracts/*" => "./contracts/*" mapping.
---
Nitpick comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 40-43: Add a NatSpec/top-level comment to MultiCollateral (and
note inheritance from HypERC20Collateral and IMultiCollateralFee) explicitly
stating support and limitations for rebasing tokens, fee-on-transfer tokens, and
ERC-777 tokens: declare whether each is supported or unsupported, explain that
the contract assumes ERC20 semantics where token balance changes match transfer
amounts (so rebasing/fee-on-transfer/ERC‑777 hooks can break accounting), and
recommend using non‑rebasing/non‑fee tokens or wrapping adapters (or relying on
wrappers in HypERC20Collateral) if such tokens must be used; reference the
contract name MultiCollateral and the inherited HypERC20Collateral/SafeERC20
usage so reviewers can locate where assumptions are made.
In `@solidity/multicollateral/test/MultiCollateral.t.sol`:
- Around line 87-90: Add explicit coverage and/or a clear support-status
statement for non-standard token behavior: update the MultiCollateral.t.sol
tests to include dedicated cases (or a top-of-file doc comment) addressing
rebasing tokens, fee-on-transfer tokens, and ERC-777 semantics by referencing
the existing ERC20Test fixtures (originUSDC, originUSDT, destUSDC, destUSDT) and
either (a) add mock token variants that simulate rebasing and fee-on-transfer
and ERC777 behavior and exercise deposit/withdraw/transfer paths in the
MultiCollateral test suite, or (b) add a documented section in the test file
that states whether MultiCollateral supports or rejects rebasing,
fee-on-transfer, and ERC-777 tokens; ensure each new test or doc entry names the
relevant functions (deposit, withdraw, transferCollateral) so integrators can
map behavior to code paths.
ℹ️ 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 (14)
.changeset/multicollateral-typechain-package.mdDockerfilepnpm-workspace.yamlsolidity/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.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
solidity/multicollateral/package.json (1)
8-13:⚠️ Potential issue | 🟠 MajorExpose
./contracts/*inexportsfor file-level imports.
"./contracts"alone can be too narrow for consumers importing specific files (e.g..../contracts/MultiCollateral.sol).Suggested fix
"exports": { ".": "./dist/index.js", "./typechain": "./dist/typechain/index.js", "./typechain/*": "./dist/typechain/*", - "./contracts": "./contracts" + "./contracts": "./contracts", + "./contracts/*": "./contracts/*" },In Node.js package "exports", does having only "./contracts": "./contracts" allow consumers to import subpaths like "@hyperlane-xyz/multicollateral/contracts/MultiCollateral.sol", or is "./contracts/*" also required?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/package.json` around lines 8 - 13, The package exports currently expose "./contracts": "./contracts" which blocks consumers from importing individual files; update the package.json "exports" object to also include a "./contracts/*": "./contracts/*" entry so subpath imports like imports of individual solidity files (e.g. MultiCollateral.sol) resolve correctly; edit the "exports" map in package.json to add that wildcard entry alongside the existing "./contracts" key.
🧹 Nitpick comments (2)
solidity/multicollateral/contracts/MultiCollateral.sol (2)
29-39: Please document token-behavior support status in the contract header.Add explicit notes for rebasing tokens, fee-on-transfer tokens, and ERC-777 compatibility so integrators know the accounting assumptions up front.
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/MultiCollateral.sol` around lines 29 - 39, Update the MultiCollateral contract NatSpec header to explicitly state the token-behavior support and accounting assumptions: document whether rebasing tokens are supported (and if not, that rebases will break accounting or that absolute balances are used), whether fee-on-transfer tokens are supported (and if fees are not accounted for, that amounts are gross and recipient may receive less), and whether ERC-777 tokens (with hooks) are compatible or discouraged; reference the contract name MultiCollateral and the inherited HypERC20Collateral so integrators understand where token transfers occur and that handle()/transfer logic assumes standard ERC-20 non-rebasing, non-fee-on-transfer semantics.
250-255: Useexternalvisibility fortransferRemoteTosince it's only called externally.This function isn't called anywhere within the contract, and it's not declared in the
IMultiCollateralFeeinterface either. Flipping it toexternaltrims the calldata overhead—like ogres have layers, contracts have optimization layers too.Suggested change
- ) public payable returns (bytes32 messageId) { + ) external payable returns (bytes32 messageId) {🤖 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 250 - 255, The function transferRemoteTo is only invoked externally, so change its visibility from public to external to reduce calldata copying and gas; update the function declaration transferRemoteTo(uint32,uint256,bytes32,bytes32) to use external payable and ensure any external callers still match the new signature, and if this function is intended to be part of an interface, add/adjust its declaration in IMultiCollateralFee to external as well so signatures remain consistent.
🤖 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 275-279: The call to
MultiCollateral(_targetRouter.bytes32ToAddress()).handle(...) can silently no-op
if the enrolled _targetRouter resolves to an EOA/non-contract; update the
same-domain branch (where _destination == localDomain) to first convert
_targetRouter to address via bytes32ToAddress(), check that the resulting
address has contract code (e.g., address(code).length > 0 or an isContract
helper) before invoking handle, and revert or return an error if it's not a
contract so fees/tokens aren't consumed for a non-contract target; reference
MultiCollateral, _targetRouter.bytes32ToAddress(), handle, localDomain,
tokenMsg, and remainingValue when adding this guard.
In `@solidity/multicollateral/package.json`:
- Line 32: The build:typechain script references the wrong artifact path for
IMultiCollateralFee; update the command in the "build:typechain" npm script so
the IMultiCollateralFee artifact points to
out/interfaces/IMultiCollateralFee.sol/IMultiCollateralFee.json (instead of
out/IMultiCollateralFee.sol/IMultiCollateralFee.json) so TypeChain includes the
interface artifact alongside the existing MultiCollateral and
MultiCollateralRoutingFee entries.
---
Duplicate comments:
In `@solidity/multicollateral/package.json`:
- Around line 8-13: The package exports currently expose "./contracts":
"./contracts" which blocks consumers from importing individual files; update the
package.json "exports" object to also include a "./contracts/*": "./contracts/*"
entry so subpath imports like imports of individual solidity files (e.g.
MultiCollateral.sol) resolve correctly; edit the "exports" map in package.json
to add that wildcard entry alongside the existing "./contracts" key.
---
Nitpick comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 29-39: Update the MultiCollateral contract NatSpec header to
explicitly state the token-behavior support and accounting assumptions: document
whether rebasing tokens are supported (and if not, that rebases will break
accounting or that absolute balances are used), whether fee-on-transfer tokens
are supported (and if fees are not accounted for, that amounts are gross and
recipient may receive less), and whether ERC-777 tokens (with hooks) are
compatible or discouraged; reference the contract name MultiCollateral and the
inherited HypERC20Collateral so integrators understand where token transfers
occur and that handle()/transfer logic assumes standard ERC-20 non-rebasing,
non-fee-on-transfer semantics.
- Around line 250-255: The function transferRemoteTo is only invoked externally,
so change its visibility from public to external to reduce calldata copying and
gas; update the function declaration
transferRemoteTo(uint32,uint256,bytes32,bytes32) to use external payable and
ensure any external callers still match the new signature, and if this function
is intended to be part of an interface, add/adjust its declaration in
IMultiCollateralFee to external as well so signatures remain consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
solidity/eslint.config.mjssolidity/multicollateral/contracts/MultiCollateral.solsolidity/multicollateral/eslint.config.mjssolidity/multicollateral/package.jsonsolidity/multicollateral/test/MultiCollateral.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
solidity/multicollateral/contracts/MultiCollateral.sol (2)
109-118: Consider EnumerableSet for O(1) removal if router lists grow large.The swap-and-pop is fine for small lists, but if a domain ever has many enrolled routers, unenrolling becomes a bit of a slog. Not urgent - just somethin' to keep in mind if this thing scales up like a fairy tale.
♻️ Alternative using OpenZeppelin's EnumerableSet
// In storage declarations: import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; using EnumerableSet for EnumerableSet.Bytes32Set; mapping(uint32 => EnumerableSet.Bytes32Set) internal _enrolledRouterSet; // Then removal becomes O(1): // _enrolledRouterSet[_domain].remove(_router);🤖 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 109 - 118, The current _removeFromList function does a swap-and-pop on the bytes32[] mapping _enrolledRouterList which is O(n) and can be slow if lists grow; replace the array-based storage with OpenZeppelin's EnumerableSet (import EnumerableSet and add using EnumerableSet for EnumerableSet.Bytes32Set) and introduce mapping(uint32 => EnumerableSet.Bytes32Set) _enrolledRouterSet, then update all enroll/unenroll codepaths to use _enrolledRouterSet[_domain].add(_router) and .remove(_router) (O(1)) while keeping any array-based accessor helpers in sync or providing EnumerableSet.values() where needed; update or remove _removeFromList to call the set remove or delete it if unused.
28-39: Document rebasing token, fee-on-transfer, and ERC-777 support status.The contract's NatSpec doesn't mention how it handles non-standard ERC20 variants. Given this deals with collateral, users should know whether their tokens will work as expected or if they'll end up in a pickle.
📝 Suggested documentation addition
/** * `@title` MultiCollateral * `@notice` Multi-router collateral: direct 1-message atomic transfers between * collateral routers, both cross-chain and same-chain. * `@dev` Extends HypERC20Collateral. Each deployed instance holds collateral for * one ERC20. Enrolled routers are other MultiCollateral instances (same or * different token) that this instance trusts to send/receive transfers. + * + * Token compatibility: + * - Fee-on-transfer tokens: NOT SUPPORTED. Amount accounting assumes 1:1 transfers. + * - Rebasing tokens: NOT SUPPORTED. Balance changes outside transfers break accounting. + * - ERC-777 tokens: Use with caution. Reentrancy guards in SafeERC20 mitigate hooks, + * but unusual callback behavior may cause unexpected results. * * Overrides: * - handle(): accepts messages from the mailbox (cross-chain) or directly * from enrolled routers on the same chain. */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 contract NatSpec for MultiCollateral to explicitly state its compatibility with non-standard ERC20s: indicate whether rebasing tokens, fee-on-transfer tokens, and ERC-777 tokens are supported or not, describe any loss/behaviour users should expect during transfers, and list recommended mitigations (e.g., wrapping tokens or using a non-rebasing wrapper). Reference the contract and inheritance (MultiCollateral, HypERC20Collateral) and mention the key message entrypoint handle() so reviewers know where this behaviour affects cross-chain/same-chain transfers; keep the note concise and add guidance for integrators on how to proceed if their token is non-standard.
🤖 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 137-147: The direct-call branch of handle() currently only checks
that msg.sender is an enrolled router but then passes unchecked _origin and
_sender into _handle, allowing spoofing; update the same-chain branch in
handle() to assert that _origin equals localDomain and that _sender equals
TypeCasts.addressToBytes32(msg.sender) (or otherwise canonicalize _sender from
msg.sender) before calling _handle, using require(...) with a clear revert
message so only messages that truly originate from the local router are
processed.
---
Nitpick comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 109-118: The current _removeFromList function does a swap-and-pop
on the bytes32[] mapping _enrolledRouterList which is O(n) and can be slow if
lists grow; replace the array-based storage with OpenZeppelin's EnumerableSet
(import EnumerableSet and add using EnumerableSet for EnumerableSet.Bytes32Set)
and introduce mapping(uint32 => EnumerableSet.Bytes32Set) _enrolledRouterSet,
then update all enroll/unenroll codepaths to use
_enrolledRouterSet[_domain].add(_router) and .remove(_router) (O(1)) while
keeping any array-based accessor helpers in sync or providing
EnumerableSet.values() where needed; update or remove _removeFromList to call
the set remove or delete it if unused.
- Around line 28-39: Update the contract NatSpec for MultiCollateral to
explicitly state its compatibility with non-standard ERC20s: indicate whether
rebasing tokens, fee-on-transfer tokens, and ERC-777 tokens are supported or
not, describe any loss/behaviour users should expect during transfers, and list
recommended mitigations (e.g., wrapping tokens or using a non-rebasing wrapper).
Reference the contract and inheritance (MultiCollateral, HypERC20Collateral) and
mention the key message entrypoint handle() so reviewers know where this
behaviour affects cross-chain/same-chain transfers; keep the note concise and
add guidance for integrators on how to proceed if their token is non-standard.
nambrot
commented
Mar 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
solidity/multicollateral/contracts/MultiCollateral.sol (1)
29-40: Document special-token support assumptions in contract docs.One more layer worth adding: spell out whether rebasing, fee-on-transfer, and ERC-777 tokens are supported here, since fee/charge accounting assumes predictable transfer semantics.
Based on learnings, Applies to **/*.sol : 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 29 - 40, Update the contract NatSpec for MultiCollateral (and similarly for other .sol files) to explicitly state support/limitations for special ERC20 behaviors: declare whether rebasing tokens are supported or not, whether fee-on-transfer tokens are supported (and how fees affect accounting), and whether ERC-777 hooks are compatible; include this note near the top-level contract comment and on the handle() method documentation so integrators know the assumptions and failure modes (reference MultiCollateral and handle()).solidity/multicollateral/contracts/MultiCollateralRoutingFee.sol (1)
36-43: Add contract-code validation when setting fee contracts.Quick heads-up: owner can currently set a non-contract address, and later
quoteTransferRemote*calls will blow up for that route. Better to fail fast in setters.🛠️ Suggested patch
function setRouterFeeContract( uint32 destination, bytes32 targetRouter, address feeContract ) external onlyOwner { + require( + feeContract == address(0) || feeContract.code.length > 0, + "MCF: fee contract not contract" + ); feeContracts[destination][targetRouter] = feeContract; emit FeeContractSet(destination, targetRouter, feeContract); } function setRouterFeeContracts( uint32[] calldata destinations, bytes32[] calldata targetRouters, address[] calldata _feeContracts ) external onlyOwner { @@ for (uint256 i = 0; i < destinations.length; i++) { + require( + _feeContracts[i] == address(0) || + _feeContracts[i].code.length > 0, + "MCF: fee contract not contract" + ); feeContracts[destinations[i]][targetRouters[i]] = _feeContracts[i]; emit FeeContractSet( destinations[i], targetRouters[i], _feeContracts[i] ); } }As per coding guidelines,
Check contract existence before low-level callsandValidate all function parameters within safe bounds.Also applies to: 45-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/contracts/MultiCollateralRoutingFee.sol` around lines 36 - 43, The setter setRouterFeeContract currently allows non-contract addresses and can cause later failures in functions like quoteTransferRemote*, so update setRouterFeeContract (and the other fee setter functions referenced around lines 45-64) to validate that the provided feeContract is a deployed contract before assignment: check contract existence via extcodesize (or Address.isContract) and revert with a clear error if not a contract; only then assign feeContracts[destination][targetRouter] and emit FeeContractSet. Ensure the validation uses the exact symbols setRouterFeeContract, feeContracts, and FeeContractSet so maintainers can locate and apply the same check to the other setter functions.solidity/multicollateral/test/MultiCollateral.t.sol (1)
963-1005: Solid test coverage; add one edge case for hook-fee scaling parity.This block is good stuff. I’d add a variant where hook fee depends on message amount and token scaling is non-1, to lock in that
quoteTransferRemoteToandtransferRemoteTostay aligned under non-trivial math.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 `@solidity/multicollateral/test/MultiCollateral.t.sol` around lines 963 - 1005, Add a new test variant of test_routingFee_quoteMatchesCharge that exercises a non-1 token scaling factor and a hook fee that scales with the message amount to ensure quoteTransferRemoteTo and transferRemoteTo compute identically under non-trivial math: instantiate a LinearFee (e.g., LinearFee linearFeeX) configured to charge proportional to amount, register it on MultiCollateralRoutingFee via setRouterFeeContract (same pattern as routingFee.setRouterFeeContract(...) used in test_routingFee_quoteMatchesCharge), ensure the token/router used has a different scaling (non-1) so scaling math runs (or simulate by adjusting the fee parameters), then call usdcRouterA.quoteTransferRemoteTo(...) and usdcRouterA.transferRemoteTo(...) as in the original test and assert quotedFee == actualFee; reference the existing test_routingFee_quoteMatchesCharge, LinearFee, MultiCollateralRoutingFee, quoteTransferRemoteTo, and transferRemoteTo when adding the new case.
🤖 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 149-160: The fee flow can revert if feeRecipient() returns a
non-contract EOA; before calling IMultiCollateralFee.quoteTransferRemoteTo,
check that the resolved _feeRecipient is a contract and fail fast if not. In the
function that currently assigns _feeRecipient = feeRecipient() and then calls
IMultiCollateralFee(_feeRecipient).quoteTransferRemoteTo(...), add a
contract-existence guard (e.g., require(Address.isContract(_feeRecipient),
"feeRecipient not contract") or equivalent extcodesize check) referencing
feeRecipient(), IMultiCollateralFee.quoteTransferRemoteTo, and the local
_feeRecipient variable so misconfiguration errors surface immediately. Ensure
you import or implement the Address.isContract helper if using it.
---
Nitpick comments:
In `@solidity/multicollateral/contracts/MultiCollateral.sol`:
- Around line 29-40: Update the contract NatSpec for MultiCollateral (and
similarly for other .sol files) to explicitly state support/limitations for
special ERC20 behaviors: declare whether rebasing tokens are supported or not,
whether fee-on-transfer tokens are supported (and how fees affect accounting),
and whether ERC-777 hooks are compatible; include this note near the top-level
contract comment and on the handle() method documentation so integrators know
the assumptions and failure modes (reference MultiCollateral and handle()).
In `@solidity/multicollateral/contracts/MultiCollateralRoutingFee.sol`:
- Around line 36-43: The setter setRouterFeeContract currently allows
non-contract addresses and can cause later failures in functions like
quoteTransferRemote*, so update setRouterFeeContract (and the other fee setter
functions referenced around lines 45-64) to validate that the provided
feeContract is a deployed contract before assignment: check contract existence
via extcodesize (or Address.isContract) and revert with a clear error if not a
contract; only then assign feeContracts[destination][targetRouter] and emit
FeeContractSet. Ensure the validation uses the exact symbols
setRouterFeeContract, feeContracts, and FeeContractSet so maintainers can locate
and apply the same check to the other setter functions.
In `@solidity/multicollateral/test/MultiCollateral.t.sol`:
- Around line 963-1005: Add a new test variant of
test_routingFee_quoteMatchesCharge that exercises a non-1 token scaling factor
and a hook fee that scales with the message amount to ensure
quoteTransferRemoteTo and transferRemoteTo compute identically under non-trivial
math: instantiate a LinearFee (e.g., LinearFee linearFeeX) configured to charge
proportional to amount, register it on MultiCollateralRoutingFee via
setRouterFeeContract (same pattern as routingFee.setRouterFeeContract(...) used
in test_routingFee_quoteMatchesCharge), ensure the token/router used has a
different scaling (non-1) so scaling math runs (or simulate by adjusting the fee
parameters), then call usdcRouterA.quoteTransferRemoteTo(...) and
usdcRouterA.transferRemoteTo(...) as in the original test and assert quotedFee
== actualFee; reference the existing test_routingFee_quoteMatchesCharge,
LinearFee, MultiCollateralRoutingFee, quoteTransferRemoteTo, and
transferRemoteTo when adding the new case.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
solidity/multicollateral/contracts/MultiCollateral.solsolidity/multicollateral/contracts/MultiCollateralRoutingFee.solsolidity/multicollateral/test/MultiCollateral.t.sol
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
Approve with 2 suggested fixes. Architecture is solid — clean separation from core, good test coverage (27 tests), well-designed fee routing. Latest commits (a3c48ce, 5a5bc58) are clean cleanup: cross-chain-only SentTransferRemote emission and batch-only setRouterFeeContracts consolidation.
Positives
- Clean
EnumerableSet.Bytes32Setfor enrollment with conditional event emission forceApprovein fee charge handles non-standard tokens better than base'sapprove- Contract code length check on same-chain target prevents EOA calls
- Batch-only enrollment API prevents single-router admin footguns
paulbalaji
reviewed
Mar 2, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Two net-new findings from team review (not already covered in existing comments):
Contributor
⚙️ Node Service Docker Images Built Successfully
Full image paths |
paulbalaji
approved these changes
Mar 2, 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
2 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
Extracts MultiCollateral contracts into a standalone pnpm workspace package (
solidity/multicollateral/) so they ship separately from the audited@hyperlane-xyz/core.MultiCollateral.sol— extendsHypERC20Collateralwith multi-router support: cross-chain transfers to any enrolled router (not just the single enrolled remote), same-chain atomic swaps between different collateral tokens, and per-router fee lookup viaIMultiCollateralFeeIMultiCollateralFee.sol— new interface withquoteTransferRemoteTo(dest, recipient, amount, targetRouter), deliberately a separate name fromITokenFee.quoteTransferRemoteto avoid overload ambiguityMultiCollateralRoutingFee.sol— standalone fee router (does NOT extendRoutingFee). Routes fee lookups by(destination, targetRouter)with fallback to destination-only default. Delegates to existingITokenFee(3-param) fee contracts — compatible withLinearFee,ProgressiveFee, etc.Key design decisions
@hyperlane-xyz/multicollateraldepends on@hyperlane-xyz/corevia workspace link. Imports use@hyperlane-xyz/core/prefix. Zero changes to core contracts.quoteTransferRemoteToinstead of overloadedquoteTransferRemote— avoids modifyingITokenFee/ITokenBridgeinterfaces in coreMultiCollateralRoutingFeeis standalone — does not inherit fromRoutingFee/BaseFeeto keep core untouched. Simpler: just owner + nested mapping + delegation to 3-paramITokenFeecontracts.(scaleNumerator, scaleDenominator)— matches current main'sHypERC20CollateralAPIWhat's NOT modified
RoutingFee.sol,ITokenBridge.sol,BaseFee.sol,TokenRouter.sol— all untouched.Test plan
cd solidity/multicollateral && forge build— compiles cleanlycd solidity/multicollateral && forge test -vvv— 27/27 tests passcd solidity && forge build— core builds unaffectedcd solidity && forge test— all existing core tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores