Add XCM Escrow Input Settler for Polkadot Cross-Chain Intent Settlement#7
Add XCM Escrow Input Settler for Polkadot Cross-Chain Intent Settlement#7
Conversation
4meta5
left a comment
4meta5
left a comment
There was a problem hiding this comment.
comments were questions so they are non-blocking unless a solidity expert chimes in otherwise
|
@CodeRabbit review |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces InputSettlerXCMEscrow contract that extends InputSettlerEscrow to enable XCM-based cross-chain token settlement. The contract manages a whitelist of allowed token-destination pairs, validates orders, collects/approves tokens, constructs and executes XCM teleport messages via precompile, then reverts approvals. Falls back to base settler for incompatible flows. Includes supporting interfaces, mock contracts for testing, deployment module, and comprehensive test suites covering admin controls, validation, edge cases, and XCM execution. Changes
Sequence DiagramsequenceDiagram
participant User
participant XCMEscrow as InputSettlerXCMEscrow
participant BaseSettler as InputSettlerEscrow
participant MockXcm as XCM Precompile
participant MockLibrary as Teleport Library
participant Token as ERC20 Token
User->>XCMEscrow: open(order)
activate XCMEscrow
alt XCM Path Available
XCMEscrow->>XCMEscrow: _checkXCMAvailable()
XCMEscrow->>XCMEscrow: Calculate transfer amounts from outputs
XCMEscrow->>Token: approve(address, amount)
Token-->>XCMEscrow: Approved
loop For each output
XCMEscrow->>MockLibrary: teleport(paraId, recipient, amount)
MockLibrary->>Token: transferFrom(user, library, amount)
Token-->>MockLibrary: Transferred
MockLibrary-->>XCMEscrow: teleportMessage
XCMEscrow->>MockXcm: send(destination, message)
MockXcm-->>XCMEscrow: Sent
XCMEscrow->>XCMEscrow: emit XCMTeleportExecuted
end
XCMEscrow->>Token: approve(address, 0)
Token-->>XCMEscrow: Revoked
else XCM Not Available
XCMEscrow->>BaseSettler: open(order)
BaseSettler-->>User: Open flow executed
end
deactivate XCMEscrow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ignition/modules/InputSettlerXCMEscrow.ts (1)
8-10: Zero address default forinkLibrarymay cause silent deployment failures.While the comment notes users should provide this parameter for real networks, deploying with the zero address will result in failed XCM teleport calls at runtime. Consider adding deployment-time validation or a more explicit warning mechanism.
You could add a validation in the deployment script or emit a warning:
const inkLibrary = m.getParameter("inkLibrary", "0x0000000000000000000000000000000000000000"); // Add a comment that deployment will fail XCM calls if inkLibrary is not configuredcontracts/test/MockERC20.sol (1)
1-16: LGTM for test purposes.The mock is appropriate for testing. The public
mint/burnfunctions without access control are acceptable for a test-only contract.Minor note: This uses
pragma ^0.8.20while other new contracts use^0.8.28or^0.8.26. Consider aligning pragma versions for consistency across the codebase.test/InputSettlerXCMEscrow.admin.test.js (1)
61-71: Consider adding access control test forforbidTeleport.The
allowTeleportfunction has an access control test (lines 51-58), butforbidTeleportlacks one. For completeness, consider adding a test verifying that non-owners cannot callforbidTeleport.it("Should revert if forbidTeleport not called by owner", async function () { const tokenAddress = await token.getAddress(); await inputSettlerXCMEscrow.allowTeleport(DESTINATION_CHAIN_ID, tokenAddress); await expect( inputSettlerXCMEscrow.connect(user).forbidTeleport(DESTINATION_CHAIN_ID, tokenAddress) ).to.be.revertedWithCustomError(inputSettlerXCMEscrow, "OwnableUnauthorizedAccount") .withArgs(user.address); });test/InputSettlerXCMEscrow.xcm.test.js (1)
47-59: Minor:to.be.emitvsto.emitsyntax.Line 57-58 uses
.to.be.emit(baseSettler, "Open")while other tests use.to.emit(...). Both syntaxes work in Chai, but for consistency with the rest of the test suite (e.g., line 68), consider using.to.emit(...).- await expect(inputSettlerXCMEscrow.connect(user).open(order)) - .to.be.emit(baseSettler, "Open"); + await expect(inputSettlerXCMEscrow.connect(user).open(order)) + .to.emit(baseSettler, "Open");contracts/test/MockLibrary.sol (1)
22-33: Mock burn behavior may not match how approvals are wired in the XCM path
teleportpulls tokens frommsg.senderusingsafeTransferFrom, which requiresmsg.sender(the XCM escrow contract) to approve this mock as spender. In the XCM path,_collectAndApproveTokensapproves the XCM precompile, not theinkLibrary/MockLibrary, so this mock transfer will revert iftokenAddressis set and the function is actually exercised.If the intent is only to simulate message construction and an event, you can either:
- Keep
tokenAddressataddress(0)in tests, or- Adjust the mock to not rely on ERC20 allowances (e.g., just emit
TeleportCalledand returnteleportMessage), or- Change the XCM path setup in tests to also approve
MockLibraryif you want to assert token “burning”.Please double‑check how this is used in the tests and whether you actually need the
safeTransferFromhere.contracts/InputSettlerXCMEscrow.sol (2)
94-107: Verify escrow fallback token flow matchesInputSettlerEscrow.openexpectationsIn the non‑XCM branch, you:
- Pre‑collect tokens from
msg.senderinto this contract and approvebaseSettler, then- Call
InputSettlerEscrow(baseSettler).open(order).This assumes that
InputSettlerEscrow.openwill pull funds frommsg.sender(the XCM escrow wrapper) using allowances, not directly fromorder.user, and that there’s no additional expectation about balances or allowances on the original user.If
InputSettlerEscrow.openinstead:
- Pulls directly from
order.user, or- Relies on allowances from
order.usertobaseSettler,then this pre‑collection/approval pattern could be redundant or even break flows.
Please double‑check the base settler’s implementation and ensure:
- Its notion of “payer” matches this wrapper’s behavior, and
- There’s no path where inputs are unintentionally collected twice or from the wrong address.
197-244: Coverage check logic is correct but slightly more complex than necessary
_verifyInputsCoverOutputscorrectly aggregates required output amounts per token and subtracts available inputs, then verifies that all requirements are fully covered. The approach is O(n²) due to the linear scan in_findInArray, but given expected order sizes that’s acceptable.If you want to simplify a bit without changing behavior, you could replace the manual overflow check:
uint256 newAmount = tempOutputAmounts[idx] + amount; require(newAmount >= tempOutputAmounts[idx], "Output amount overflow");with just the assignment, since Solidity 0.8+ already reverts on overflow:
uint256 newAmount = tempOutputAmounts[idx] + amount;This is optional and only reduces redundant checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
contracts/InputSettlerXCMEscrow.sol(1 hunks)contracts/interfaces/ILibrary.sol(1 hunks)contracts/interfaces/IXcm.sol(1 hunks)contracts/test/MockERC20.sol(1 hunks)contracts/test/MockLibrary.sol(1 hunks)contracts/test/MockXcm.sol(1 hunks)ignition/modules/InputSettlerXCMEscrow.ts(1 hunks)test/InputSettlerXCMEscrow.admin.test.js(1 hunks)test/InputSettlerXCMEscrow.edge-cases.test.js(1 hunks)test/InputSettlerXCMEscrow.validation.test.js(1 hunks)test/InputSettlerXCMEscrow.xcm.test.js(1 hunks)test/helpers/inputSettlerXCMEscrowHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/InputSettlerXCMEscrow.edge-cases.test.js (1)
test/helpers/inputSettlerXCMEscrowHelper.js (8)
inputSettlerXCMEscrow(72-78)token(81-81)STANDARD_AMOUNT(19-19)DESTINATION_CHAIN_ID(8-8)ZERO_BYTES32(37-37)MOCK_XCM_MESSAGE_BYTES(32-32)MAX_TOKENS_TEST_COUNT(41-41)INITIAL_TOKEN_BALANCE(18-18)
test/helpers/inputSettlerXCMEscrowHelper.js (4)
test/InputSettlerXCMEscrow.admin.test.js (5)
require(1-1)require(2-2)require(3-10)inputSettlerXCMEscrow(13-13)createOrder(21-21)test/InputSettlerXCMEscrow.edge-cases.test.js (5)
require(1-1)require(2-2)require(3-14)inputSettlerXCMEscrow(17-17)createOrder(25-25)test/InputSettlerXCMEscrow.validation.test.js (5)
require(1-1)require(2-2)require(3-16)inputSettlerXCMEscrow(19-19)createOrder(27-27)test/InputSettlerXCMEscrow.xcm.test.js (5)
require(1-1)require(2-2)require(3-21)inputSettlerXCMEscrow(24-24)createOrder(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (15)
contracts/interfaces/ILibrary.sol (1)
1-10: LGTM!The interface is clean and minimal. The parameter types (
uint32,bytes32,uint128) align well with XCM/Polkadot conventions where chain IDs are 32-bit, beneficiaries are 32-byte account IDs, and amounts are 128-bit.ignition/modules/InputSettlerXCMEscrow.ts (1)
12-24: LGTM!The deployment module correctly chains the base settler deployment with the XCM settler, passing the required constructor arguments. Returning both contracts enables proper access for subsequent configuration.
test/InputSettlerXCMEscrow.admin.test.js (1)
74-97: Approval amount exceeds required amount—intentional?At line 80, the approval uses
DOUBLE_AMOUNTwhile the default order usesSTANDARD_AMOUNT. This works but may be intentional to test that excess approval doesn't cause issues. If so, consider adding a comment clarifying this is deliberate testing of over-approval scenarios.test/InputSettlerXCMEscrow.edge-cases.test.js (2)
114-142: Verify near-overflow test arithmetic.The test computes
totalAmount - ethers.parseEther(INITIAL_TOKEN_BALANCE)at line 122. This assumes the user already hasINITIAL_TOKEN_BALANCEminted (likely insetupInputSettlerXCMEscrow). The arithmetic is correct for testing uint256 boundaries.However, note that
halfAmount = totalAmount / BigInt("2")at line 124 will truncate for odd values. SincetotalAmountis(2^256 - 1)which is odd, the twohalfAmountoutputs will sum tototalAmount - 1, nottotalAmount. This means the test is actually testinginput > sum(outputs)scenario rather than exact match. This is fine but may not match the comment's intent at line 118.
40-62: Good boundary testing for empty arrays.These tests appropriately verify that the contract handles empty inputs and outputs gracefully without reverting unexpectedly. The comment clarifies that empty inputs trigger the fallback path.
test/InputSettlerXCMEscrow.validation.test.js (3)
178-201: Fallback test logic depends on amount constant values.The comment at line 198 states "Total outputs = 160, input = 150". This expects
LARGE_AMOUNT<2 * MEDIUM_AMOUNT. IfLARGE_AMOUNT = "150"andMEDIUM_AMOUNT = "80", then 150 < 160, which triggers fallback correctly. The test logic is sound assuming these values.
204-231: LGTM!Good test coverage for multiple outputs to different destinations with proper event assertions.
151-176: Amount constants match test comments accurately.The constants are correctly defined:
MEDIUM_AMOUNT = "80"→ Two outputs of 80 each = 160 totalDOUBLE_AMOUNT = "200"→ Input amountThe comment at line 173 stating "Total outputs = 160, input = 200" is correct and the test properly validates the aggregation scenario.
contracts/interfaces/IXcm.sol (1)
1-41: LGTM!Well-documented interface that accurately reflects the Polkadot XCM precompile. The
Weightstruct properly models the two-dimensional weight system (ref_time, proof_size), and the NatSpec documentation with links to Polkadot docs is helpful for developers.test/InputSettlerXCMEscrow.xcm.test.js (4)
146-164: Good atomicity testing.This test properly verifies that when the XCM precompile fails, the entire transaction reverts and user funds remain safe. This is critical for security.
167-181: Appropriate use ofdescribe.skipfor unimplemented feature.The
openForXCM path is marked as TODO per PR objectives. Usingdescribe.skipkeeps the test structure in place for future implementation while clearly indicating it's not yet active.
183-271: Thorough token collection semantics testing.These tests properly verify the key behavioral difference between XCM and baseSettler paths:
- XCM path: collects only output amounts (excess stays with user)
- BaseSettler path: collects full input amounts
This is important for users to understand the economic implications of each path.
273-296: Good allowance verification.Verifying that XCM path approves
xcmPrecompileand notbaseSettlerensures the correct contract receives spending approval. The post-execution check that both allowances are zero confirms proper cleanup.contracts/test/MockXcm.sol (1)
6-41: Mock XCM precompile behavior looks appropriate for testingThe mock cleanly separates
execute,send, andweighMessage, with configurable success and weight and clear events. This is sufficient and easy to reason about for unit tests.test/helpers/inputSettlerXCMEscrowHelper.js (1)
29-33: Based on my verification, I found thatMOCK_XCM_MESSAGE_BYTESis always wrapped withethers.toUtf8Bytes()before being passed to contract calls across all test files (InputSettlerXCMEscrow.xcm.test.js, InputSettlerXCMEscrow.validation.test.js, and InputSettlerXCMEscrow.edge-cases.test.js).Since
ethers.toUtf8Bytes()interprets its input as a plain UTF-8 string (not hex), the value"0xmsg"is intentionally treated as literal text. The presence of "g" is not problematic—it's simply part of the string being converted to bytes. The original review comment's concern about invalid hex rejection does not apply here.The constant value and its usage are correct and consistent.
Likely an incorrect or invalid review comment.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Added docs to the XCM message building library Fixed review comments
bidzyyys
left a comment
bidzyyys
left a comment
There was a problem hiding this comment.
Looks much better!
Can you investigate this comment: https://github.com/OpenZeppelin/openzeppelin-polkadot-xcm/pull/7/files#r2565488382
3 participants
Summary
This PR introduces
InputSettlerXCMEscrow, a smart contract that extends the OIF (Open Intents Framework) to support direct XCM (Cross-Consensus Messaging) execution on Polkadot parachains. When a user's intent can be fully satisfied through XCM teleport, the contract bypasses the standard escrow flow and executes the transfer immediately via the XCM precompile.Features
Core Contract (
InputSettlerXCMEscrow.sol)InputSettlerEscrowfrom the OIF framework, providing seamless fallback for non-XCM scenariosuint32(Polkadot parachain limitation)uint128(XCM amount limitation)ILibrary.teleport) to construct properly formatted XCM messagesIXcm.execute)allowTeleport()andforbidTeleport()Flow Behavior
open()baseSettleropenFor()baseSettler(XCM path TODO)finalise()baseSettlerfinaliseWithSignature()baseSettlerpurchaseOrder()baseSettlerorderIdentifier()baseSettlerImplementation Details
Token Handling
Interfaces Added
IXcm.sol: Interface for Polkadot's XCM precompile withexecute(),send(), andweighMessage()functionsILibrary.sol: Interface for XCM message construction libraryTesting
Comprehensive test suite (
InputSettlerXCMEscrow.test.js) with 28 test cases covering:allowTeleport/forbidTeleport)Files Changed
contracts/InputSettlerXCMEscrow.solcontracts/interfaces/IXcm.solcontracts/interfaces/ILibrary.solcontracts/test/MockXcm.solcontracts/test/MockLibrary.solcontracts/test/MockERC20.solignition/modules/InputSettlerXCMEscrow.tstest/InputSettlerXCMEscrow.test.jsSecurity Considerations
uint128amount validation prevents silent truncation in XCM messagesFuture Work
openFor()with signature validationRelated Issue
Closes #2
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.