feat(multicollateral): add TokenBridgeOft warp route adapter for LayerZero OFTs#8306
feat(multicollateral): add TokenBridgeOft warp route adapter for LayerZero OFTs#8306
Conversation
…rZero OFTs Added TokenBridgeOft contract that extends TokenRouter to bridge tokens via LayerZero OFT.send() instead of Hyperlane message dispatch. Supports all OFT patterns: native OFT (burn/mint), OFTAdapter (lock/unlock), and OFTWrapper (Paxos-style burn/mint). Wired into the SDK as TokenType.collateralOft with full deployer, reader, and CLI support following the same pattern as CCTP.
nambrot-agent
requested review from
Xaroz,
nambrot,
paulbalaji,
xeno097 and
yorhodes
as code owners
📝 WalkthroughWalkthroughAdds a LayerZero OFT bridge: new Changes
Sequence DiagramsequenceDiagram
rect rgba(60,179,113,0.5)
actor User
end
rect rgba(65,105,225,0.5)
participant TokenBridgeOft
participant OFT
end
rect rgba(255,165,0,0.5)
participant LayerZero
participant Destination
end
User->>TokenBridgeOft: transferRemote(hyperlaneDomain, to, amount)
TokenBridgeOft->>TokenBridgeOft: compute gross amount, pull tokens (incl external fee)
TokenBridgeOft->>OFT: quoteSend(sendParam, payInLzToken=false)
OFT-->>TokenBridgeOft: MessagingFee(nativeFee, lzTokenFee)
TokenBridgeOft->>OFT: send(sendParam, MessagingFee, refundAddress) payable
OFT->>LayerZero: emit message (token transfer + payload)
LayerZero->>Destination: deliver message
Destination-->>OFT: mint/credit tokens on remote chain
OFT-->>TokenBridgeOft: MessagingReceipt / OFTReceipt
TokenBridgeOft-->>User: return messageId / emit SentTransferRemote
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8306 +/- ##
=======================================
Coverage 76.49% 76.49%
=======================================
Files 128 128
Lines 3416 3416
Branches 290 290
=======================================
Hits 2613 2613
Misses 786 786
Partials 17 17
🚀 New features to boost your workflow:
|
- Add mock-based unit tests (23 tests) covering construction, domain mapping, quoting, transfers, admin, handle, and slippage protection - Expand NatSpec docs explaining TokenRouter coupling and token support - Add zero-EID validation in addDomain - Fix CI by excluding fork tests from test:ci - Bound domainMappings schema to uint32 range - Add ERC20 metadata fetching in warp route reader
There was a problem hiding this comment.
🧹 Nitpick comments (2)
solidity/multicollateral/contracts/TokenBridgeOft.sol (1)
174-181: Consider emitting an event when overwriting an existing domain mapping.Right now,
addDomainwill silently overwrite an existing mapping without any indication. While theDomainAddedevent fires regardless, it might be helpful to distinguish between "add new" and "update existing" for off-chain indexers. Not a blocker — just somethin' to think about if domain management becomes more complex later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/contracts/TokenBridgeOft.sol` around lines 174 - 181, The addDomain function currently overwrites hyperlaneDomainToLzEid[_hyperlaneDomain] and always emits DomainAdded, which hides whether this was a new mapping or an update; modify addDomain to check the existing value (hyperlaneDomainToLzEid[_hyperlaneDomain]) and if non-zero emit a distinct event (e.g., DomainUpdated with old and new _lzEid) or include the previous value in DomainAdded so off‑chain indexers can tell adds from updates; ensure the check happens before assignment and that the chosen event signature is declared and emitted (reference addDomain, hyperlaneDomainToLzEid, and DomainAdded/DomainUpdated).solidity/multicollateral/test/TokenBridgeOft.t.sol (1)
18-126: MockOFT is well-designed for deterministic testing.The mock covers all the IOFT interface methods with configurable return values. Nice touch with
shouldRevertOnSendfor failure path testing — though I notice it ain't used in any tests yet. Might want to add a test that exercises that path to ensure transfer failures from the OFT layer are handled gracefully.Consider adding a test for OFT.send failure
function test_transferRemote_oftSendReverts() public { mockOft.setShouldRevertOnSend(true); vm.startPrank(caller); token.approve(address(bridge), type(uint256).max); vm.expectRevert("MockOFT: send reverted"); bridge.transferRemote{value: 0.001 ether}(DOMAIN_ETH, recipient, 100e6); vm.stopPrank(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/multicollateral/test/TokenBridgeOft.t.sol` around lines 18 - 126, Add a unit test that exercises the OFT failure path by calling MockOFT.setShouldRevertOnSend(true), then in a prank as caller approve the bridge via token.approve(address(bridge), type(uint256).max) and call bridge.transferRemote{value: 0.001 ether}(DOMAIN_ETH, recipient, 100e6) while asserting the revert with vm.expectRevert("MockOFT: send reverted"); reference MockOFT.setShouldRevertOnSend, bridge.transferRemote, token.approve, vm.expectRevert, DOMAIN_ETH, recipient, and caller to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@solidity/multicollateral/contracts/TokenBridgeOft.sol`:
- Around line 174-181: The addDomain function currently overwrites
hyperlaneDomainToLzEid[_hyperlaneDomain] and always emits DomainAdded, which
hides whether this was a new mapping or an update; modify addDomain to check the
existing value (hyperlaneDomainToLzEid[_hyperlaneDomain]) and if non-zero emit a
distinct event (e.g., DomainUpdated with old and new _lzEid) or include the
previous value in DomainAdded so off‑chain indexers can tell adds from updates;
ensure the check happens before assignment and that the chosen event signature
is declared and emitted (reference addDomain, hyperlaneDomainToLzEid, and
DomainAdded/DomainUpdated).
In `@solidity/multicollateral/test/TokenBridgeOft.t.sol`:
- Around line 18-126: Add a unit test that exercises the OFT failure path by
calling MockOFT.setShouldRevertOnSend(true), then in a prank as caller approve
the bridge via token.approve(address(bridge), type(uint256).max) and call
bridge.transferRemote{value: 0.001 ether}(DOMAIN_ETH, recipient, 100e6) while
asserting the revert with vm.expectRevert("MockOFT: send reverted"); reference
MockOFT.setShouldRevertOnSend, bridge.transferRemote, token.approve,
vm.expectRevert, DOMAIN_ETH, recipient, and caller to locate where to add the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40980632-8ddf-47c5-9fd9-6ef8541a7bcb
📒 Files selected for processing (5)
solidity/multicollateral/contracts/TokenBridgeOft.solsolidity/multicollateral/package.jsonsolidity/multicollateral/test/TokenBridgeOft.t.soltypescript/sdk/src/token/EvmWarpRouteReader.tstypescript/sdk/src/token/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- solidity/multicollateral/package.json
- typescript/sdk/src/token/EvmWarpRouteReader.ts
- Replace plain mapping with EnumerableMap.UintToUintMap for domain mappings
- Add getDomainMappings() view to enumerate all configured domains on-chain
- Update reader to fetch domain mappings from chain instead of returning {}
- Filter collateralOft and collateralCctp from interactive CLI prompt
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nambrot
left a comment
nambrot
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured adapter that bridges Hyperlane warp routes to LayerZero OFTs. Good test coverage with unit tests (mock OFT) and fork tests (USDT0, pyUSD on Arb/Eth). SDK integration is clean — reader fetches domain mappings from chain via EnumerableMap, deploy flow configures domains/options/refund.
CodeRabbit's earlier comments on ERC20 metadata, uint32 validation, and CLI TYPE_CHOICES have all been addressed. The refundAddress constructor issue (CodeRabbit's latest comment) is part of a larger design concern detailed below.
Main concerns
-
Native value handling / refund flow — see inline comments on
_transferRemoteandrefundAddress. Excessmsg.valueand LZ refunds accumulate in the contract with no recovery path. This is especially problematic when called from MCR'srebalance(), where MultiCollateral has noreceive()to accept refunds. -
Fork tests not running in CI — see inline comment on
test:ci. The existingsolidity-fork-tests.ymlworkflow runs from./solidityand picks upForkTestcontracts there, but doesn't runsolidity/multicollateralfork tests. These are the only tests validating real OFT burn/mint/lock flows. -
removeDomainsilently succeeds on non-existent domains —EnumerableMap.removereturnsfalseif key doesn't exist, but the function ignores the return value and still emitsDomainRemoved. Minor but misleading for indexers.
Nits
_handlereverts with a string; could use a custom error for gas consistency withLzEidNotConfigured
Removed refundAddress storage/setter in favor of always refunding msg.sender. Added excess value sweep with revert on failure. Made removeDomain revert on non-existent domains. Removed refundAddress from SDK reader/deployer/types. Added vm.createSelectFork to fork tests and CI workflow step for multicollateral.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Maintains Hyperlane warp route semantics where transferRemote(_amount) means recipient gets _amount and fees are surfaced via quoteTransferRemote.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
OFTs with localDecimals > sharedDecimals (e.g. 18/6) truncate sub-sharedDecimals precision via _removeDust(). Without rounding, ceilDiv produces dusty gross amounts that cause SlippageExceeded after the OFT's internal truncation. Now: - grossAmount is rounded UP to the next dust-free boundary - minAmountLD is rounded DOWN (OFT can't deliver sub-dust precision) - decimalConversionRate stored as immutable for gas efficiency
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Use as unknown as HyperlaneContracts<Factories> with a CAST comment explaining why instead of as any. Type the oftDeployedMap with the proper Factories generic.
Fork tests now derive the total token charge (including OFT fees) from quoteTransferRemote instead of using hardcoded AMOUNT for deals, approvals, and balance assertions.
|
Re: CodeRabbit's comment on fork tests — fixed in 06d6fdf. All four |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Leverage addDeployedContracts (accepts HyperlaneContracts<any>) and the fact that super.deploy() returns this.deployedContracts which already includes the OFT entries. No intermediate map or cast needed.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
collateralOft requires a real OFT contract and cannot be tested with randomly generated configs in the CLI e2e test harness.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
OFT contracts were added to this.deployedContracts before super.deploy(), causing the parent HyperlaneRouterDeployer to call Router-specific methods (enrollRemoteRouters, configureClients) on OFT entries that lack the Router interface. Store OFT contracts in a local map and merge after super.deploy() returns.
|
Re: Devin's comment on OFT deploy ordering — fixed in e224644. OFT contracts are now stored in a local map and only merged into |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
typescript/sdk/src/token/deploy.ts (2)
454-462: Consider batch domain addition if supported.These domain mappings are added one at a time, like peeling layers off an onion individually. If the contract ever gets a batch
addDomains(similar to how CCTP does it at line 346), could speed things up. Not blocking, just something to keep in the swamp log.🤖 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 454 - 462, The loop that calls tokenBridge.addDomain for each entry in resolvedMappings should be updated to use a batch call if the contract supports it: detect whether tokenBridge.addDomains (or similar batch method) exists and, if so, construct arrays of domain IDs and lzEids from resolvedMappings and call tokenBridge.addDomains in a single multiProvider.handleTx(chain, tokenBridge.addDomains(...)) call; if the batch method is not present, fall back to the existing per-entry loop that uses multiProvider.handleTx(chain, tokenBridge.addDomain(Number(domainId), lzEid)). Ensure you reference resolvedMappings, tokenBridge.addDomain, tokenBridge.addDomains, multiProvider.handleTx, and chain when implementing this change.
785-785: Type could be tightened.This
Record<string, Record<string, unknown>>is a bit loose. If you're fixing the merge intodeployedContractsMapanyway, might as well give this proper typing from the start.💡 Tighter typing suggestion
- const oftContracts: Record<string, Record<string, unknown>> = {}; + const oftContracts: HyperlaneContractsMap<Factories> = {};Then the merge and return will have consistent types without needing casts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/deploy.ts` at line 785, Define and use a specific type alias instead of the loose inline type: add e.g. type DeployedContractsMap = Record<string, Record<string, unknown>> and replace the inline annotation on oftContracts with const oftContracts: DeployedContractsMap = {}; then update any references/merges with deployedContractsMap and the function return signature to use DeployedContractsMap so the merge and return have consistent, tighter typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/sdk/src/token/deploy.ts`:
- Around line 800-814: The OFT entries merged into this.deployedContracts via
this.addDeployedContracts(...) are not being merged into the local
deployedContractsMap returned by super.deploy(...), causing
configureOftDomains(configMap, deployedContractsMap) and the function's return
to miss OFT deployments; after the loop that iterates oftContracts (where
this.addDeployedContracts is called), also merge the same OFT contract entries
into deployedContractsMap (preserving chain keys and contract lists) so that
configureOftDomains and the eventual return include the OFT deployments—modify
the block that handles oftContracts to update both this.deployedContracts and
deployedContractsMap consistently.
---
Nitpick comments:
In `@typescript/sdk/src/token/deploy.ts`:
- Around line 454-462: The loop that calls tokenBridge.addDomain for each entry
in resolvedMappings should be updated to use a batch call if the contract
supports it: detect whether tokenBridge.addDomains (or similar batch method)
exists and, if so, construct arrays of domain IDs and lzEids from
resolvedMappings and call tokenBridge.addDomains in a single
multiProvider.handleTx(chain, tokenBridge.addDomains(...)) call; if the batch
method is not present, fall back to the existing per-entry loop that uses
multiProvider.handleTx(chain, tokenBridge.addDomain(Number(domainId), lzEid)).
Ensure you reference resolvedMappings, tokenBridge.addDomain,
tokenBridge.addDomains, multiProvider.handleTx, and chain when implementing this
change.
- Line 785: Define and use a specific type alias instead of the loose inline
type: add e.g. type DeployedContractsMap = Record<string, Record<string,
unknown>> and replace the inline annotation on oftContracts with const
oftContracts: DeployedContractsMap = {}; then update any references/merges with
deployedContractsMap and the function return signature to use
DeployedContractsMap so the merge and return have consistent, tighter typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4b1835a-a897-4f7f-b08e-ccaf9c0d4ad4
📒 Files selected for processing (1)
typescript/sdk/src/token/deploy.ts
|
Re: CodeRabbit's comment on OFT contracts missing from deployedContractsMap This is a false positive. @coderabbitai can you confirm this is not an issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 802
return this.deployedContracts;This is a direct reference return — no copy or spread — so 🧠 Learnings used |
3 participants
Summary
TokenBridgeOftcontract insolidity/multicollateral/that extendsTokenRouterto bridge tokens via LayerZeroOFT.send()instead of Hyperlane message dispatchTokenType.collateralOftwith deployer, reader, and CLI support following the CCTP patternDetails
Solidity:
TokenBridgeOft.sol— extendsTokenRouter, overrides_quoteGasPayment(OFT native fee) and_transferRemote(OFT.send)IOFT.sol— minimal LayerZero OFT interface (SendParam, MessagingFee, OFTReceipt, etc.)TokenBridgeOft.fork.t.sol— 4 fork test contracts covering USDT0 OFT (Arb), USDT0 OFT Adapter (Eth), pyUSD OFTWrapper (Arb), pyUSD OFTWrapper (Eth)EnumerableMapfor domain→LZ EID mappings (enumerable on-chain for SDK reader)Refund handling:
refundAddressstorage was removed. Both the LZ refund and any excessmsg.valueare returned tomsg.senderdirectly, with a hard revert if the refund transfer fails.receive(), so the refundcallwill revert if there's any excess. We accept this because MCR quotes atomically right before callingtransferRemote, so themsg.valuepassed should exactly match the required fee with no excess to refund.removeDomainnow reverts on non-existent keys instead of silently no-oping.SDK:
TokenType.collateralOftadded to config, contracts, types, deploy, reader, TokenStandardOftTokenConfigSchemawith fields:oft,domainMappings,extraOptionsconfigureOftDomains()in deployer maps Hyperlane domain IDs → LZ endpoint IDs post-deployderiveHypCollateralOftTokenConfig()in EvmWarpRouteReader for on-chain config derivationoft()method inderiveTokenType()CI:
vm.createSelectFork()with RPC endpoint names fromfoundry.tomlsolidity-fork-tests.ymlworkflowTest plan
forge buildpasses in bothsolidity/andsolidity/multicollateral/pnpm buildpassesvm.createSelectFork()and run in CI viasolidity-fork-tests.yml🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
CLI