feat(sdk-router): cctp v2#3954
Open
ChiTimesChi wants to merge 30 commits intomasterfrom
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 Circle CCTP v2 support: API client with caching, CCTPv2 module for calldata encoding and status checks, a CCTPv2 ModuleSet for multi-chain routing and fee logic, constants and SDK integration, plus extensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SynapseSDK
participant ModuleSet as CCTPv2ModuleSet
participant API as CCTP_v2_API
participant Circle as Circle_Iris_API
Client->>SDK: getBridgeRouteV2(params)
SDK->>ModuleSet: getBridgeRouteV2(params)
ModuleSet->>ModuleSet: validate inputs, tokens, modules
ModuleSet->>API: getBurnUSDCFees(srcDomain, dstDomain)
API->>Circle: HTTP GET /v2/burn-tokens/fees
Circle-->>API: fee response
API-->>ModuleSet: parsed CctpV2Fee[] (cached)
ModuleSet->>ModuleSet: select slowest finality, compute fees, build zapData
ModuleSet-->>SDK: BridgeRouteV2
SDK-->>Client: quote with CCTPv2 route
sequenceDiagram
participant Client
participant Module as CCTPv2Module
participant API as CCTP_v2_API
participant Circle as Circle_Iris_API
Client->>Module: getBridgeTxStatus(synapseTxId)
Module->>Module: parse synapseTxId (txHash:chainId)
Module->>API: getMessages(srcDomain, txHash)
API->>Circle: HTTP GET /v2/messages
Circle-->>API: messages response
API-->>Module: parsed CctpV2Message[] (not cached)
Module->>Module: evaluate status + forwardState
Module-->>Client: true / false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
Deploying sanguine-fe with
|
| Latest commit: |
0d47393
|
| Status: | 🚫 Build failed. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3954 +/- ##
===================================================
+ Coverage 52.96703% 55.05366% +2.08662%
===================================================
Files 133 140 +7
Lines 3640 4007 +367
Branches 627 728 +101
===================================================
+ Hits 1928 2206 +278
- Misses 1667 1740 +73
- Partials 45 61 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 3
🧹 Nitpick comments (3)
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
14-15: Avoid duplicating module-name literals across packages.Consider sourcing this value from a shared constant (or centralized bridge-module enum) to prevent silent drift if naming changes later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/synapse-interface/slices/bridgeQuote/thunks.ts` around lines 14 - 15, The literal module name CCTP_V2_MODULE_NAME is hardcoded here; replace it with a shared constant imported from the central constants/enum (e.g., BridgeModuleNames or a shared constant file) so the module name is not duplicated across packages; update the import and use the shared symbol (e.g., BridgeModuleNames.CircleCCTPV2 or SHARED_CCTP_V2_MODULE_NAME) in place of CCTP_V2_MODULE_NAME within slices/bridgeQuote/thunks.ts and remove the local declaration.packages/sdk-router/src/cctpV2/api.test.ts (1)
20-80: Add a direct cache-behavior test forgetBurnUSDCFees.These tests validate row parsing well, but a focused assertion on cache-hit behavior (second call reuses cached value without another
getWithTimeout) would protect a critical resilience path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/api.test.ts` around lines 20 - 80, Add a focused cache-behavior test for getBurnUSDCFees: after clearing cache via clearCctpV2ApiCache() and stubbing mockGetWithTimeout.mockResolvedValueOnce(responseWithJson(feeRows)), call getBurnUSDCFees(SOURCE_DOMAIN_ID, DEST_DOMAIN_ID) twice and assert the first call resolves to the expected feeRows and mockGetWithTimeout was called once while the second call resolves to the same value but does not trigger another mockGetWithTimeout call (i.e., mockGetWithTimeout call count remains unchanged); place this alongside the existing tests and ensure mockGetWithTimeout is reset in beforeEach.spec_cctp_v2_forwarder.md (1)
35-38: Clarify fee units/precision explicitly in the spec.Please add one explicit sentence that these fee constants are in human-readable USDC units (6-decimal token) and must be scaled when converted to on-chain amounts.
Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec_cctp_v2_forwarder.md` around lines 35 - 38, Add a single explicit sentence after the "Forwarding-service fixed service fees documented by Circle" list item and again where the CCTP fee/finality API is described (near the text "CCTP fee/finality data is available via Circle API GET /v2/burn/USDC/fees/{sourceDomainId}/{destDomainId}") stating: "These fee constants are expressed in human-readable USDC units (6-decimal token) and must be scaled to the token's on-chain integer representation (multiply by 10^6) when constructing on-chain amounts." This makes clear the unit/precision and the required scaling for on-chain use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-router/src/cctpV2/cctpV2Module.ts`:
- Around line 153-156: In the loop over messagesByDomain in cctpV2Module.ts,
don't always return messages[0]; instead, locate the message that matches the
forwarding hook data first (compare each message's forwarding/hook fields
against the forwardingHook data available in the scope, e.g., forwardingHookData
or similar), and only if no match is found fall back to a sensible default (such
as the first message). Update the logic inside the for (const messages of
messagesByDomain) block to search messages for a match (e.g.,
Array.prototype.find by the hook/forwarding identifiers) and return that matched
message, otherwise return the fallback.
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Around line 161-167: The computed minToAmount used for slippage protection is
not being passed into the zap data, so getBurnZapData should include
minFinalAmount: minToAmount when calling encodeZapData; locate getBurnZapData
and the encodeZapData call and add the minFinalAmount field set to the
already-computed minToAmount (and do the same for the other similar call paths
noted around lines 181-183), ensuring the BridgeRouteV2.minToAmount returned
matches the encoded zapData's minFinalAmount.
In `@packages/sdk-router/src/constants/cctpV2.ts`:
- Around line 3-37: Update the checksummed USDC addresses in
CCTP_V2_USDC_ADDRESS_MAP: replace the value for SupportedChainId.BASE with
0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913 and replace the value for
SupportedChainId.POLYGON with 0x3c499c542cEF5E3811e1192ce70d8cC03d5c3359 so
CCTP_V2_USDC_ADDRESS_MAP uses the correct checksums for those chains; verify
changes touch only the map entries for SupportedChainId.BASE and
SupportedChainId.POLYGON.
---
Nitpick comments:
In `@packages/sdk-router/src/cctpV2/api.test.ts`:
- Around line 20-80: Add a focused cache-behavior test for getBurnUSDCFees:
after clearing cache via clearCctpV2ApiCache() and stubbing
mockGetWithTimeout.mockResolvedValueOnce(responseWithJson(feeRows)), call
getBurnUSDCFees(SOURCE_DOMAIN_ID, DEST_DOMAIN_ID) twice and assert the first
call resolves to the expected feeRows and mockGetWithTimeout was called once
while the second call resolves to the same value but does not trigger another
mockGetWithTimeout call (i.e., mockGetWithTimeout call count remains unchanged);
place this alongside the existing tests and ensure mockGetWithTimeout is reset
in beforeEach.
In `@packages/synapse-interface/slices/bridgeQuote/thunks.ts`:
- Around line 14-15: The literal module name CCTP_V2_MODULE_NAME is hardcoded
here; replace it with a shared constant imported from the central constants/enum
(e.g., BridgeModuleNames or a shared constant file) so the module name is not
duplicated across packages; update the import and use the shared symbol (e.g.,
BridgeModuleNames.CircleCCTPV2 or SHARED_CCTP_V2_MODULE_NAME) in place of
CCTP_V2_MODULE_NAME within slices/bridgeQuote/thunks.ts and remove the local
declaration.
In `@spec_cctp_v2_forwarder.md`:
- Around line 35-38: Add a single explicit sentence after the
"Forwarding-service fixed service fees documented by Circle" list item and again
where the CCTP fee/finality API is described (near the text "CCTP fee/finality
data is available via Circle API GET
/v2/burn/USDC/fees/{sourceDomainId}/{destDomainId}") stating: "These fee
constants are expressed in human-readable USDC units (6-decimal token) and must
be scaled to the token's on-chain integer representation (multiply by 10^6) when
constructing on-chain amounts." This makes clear the unit/precision and the
required scaling for on-chain use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccb7172d-fb80-4158-98af-526fa7cfb49f
📒 Files selected for processing (15)
packages/sdk-router/CHANGELOG.mdpackages/sdk-router/README.mdpackages/sdk-router/src/cctpV2/api.test.tspackages/sdk-router/src/cctpV2/api.tspackages/sdk-router/src/cctpV2/cctpV2Module.test.tspackages/sdk-router/src/cctpV2/cctpV2Module.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.test.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.tspackages/sdk-router/src/cctpV2/index.tspackages/sdk-router/src/constants/cctpV2.tspackages/sdk-router/src/constants/index.tspackages/sdk-router/src/sdk.test.tspackages/sdk-router/src/sdk.tspackages/synapse-interface/slices/bridgeQuote/thunks.tsspec_cctp_v2_forwarder.md
Comment on lines
+153
to
+156
| for (const messages of messagesByDomain) { | ||
| if (messages?.length) { | ||
| return messages[0] | ||
| } |
Contributor
There was a problem hiding this comment.
Select the correct message instead of always returning messages[0].
If the API returns multiple messages for a tx, taking the first one can yield false status (wrong message/hook). Match by forwarding hook data first, then fallback.
🔧 Proposed fix
private async getMessageByTxHash(
txHash: string
): Promise<CctpV2Message | null> {
@@
- for (const messages of messagesByDomain) {
- if (messages?.length) {
- return messages[0]
- }
- }
- return null
+ let fallback: CctpV2Message | null = null
+ const expectedHookData = CCTP_V2_FORWARD_HOOK_DATA.toLowerCase()
+ for (const messages of messagesByDomain) {
+ if (!messages?.length) {
+ continue
+ }
+ const matched = messages.find(
+ (message) =>
+ message.decodedMessage?.decodedMessageBody?.hookData?.toLowerCase() ===
+ expectedHookData
+ )
+ if (matched) {
+ return matched
+ }
+ fallback ??= messages[0]
+ }
+ return fallback
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const messages of messagesByDomain) { | |
| if (messages?.length) { | |
| return messages[0] | |
| } | |
| let fallback: CctpV2Message | null = null | |
| const expectedHookData = CCTP_V2_FORWARD_HOOK_DATA.toLowerCase() | |
| for (const messages of messagesByDomain) { | |
| if (!messages?.length) { | |
| continue | |
| } | |
| const matched = messages.find( | |
| (message) => | |
| message.decodedMessage?.decodedMessageBody?.hookData?.toLowerCase() === | |
| expectedHookData | |
| ) | |
| if (matched) { | |
| return matched | |
| } | |
| fallback ??= messages[0] | |
| } | |
| return fallback |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-router/src/cctpV2/cctpV2Module.ts` around lines 153 - 156, In
the loop over messagesByDomain in cctpV2Module.ts, don't always return
messages[0]; instead, locate the message that matches the forwarding hook data
first (compare each message's forwarding/hook fields against the forwardingHook
data available in the scope, e.g., forwardingHookData or similar), and only if
no match is found fall back to a sensible default (such as the first message).
Update the logic inside the for (const messages of messagesByDomain) block to
search messages for a match (e.g., Array.prototype.find by the hook/forwarding
identifiers) and return that matched message, otherwise return the fallback.
Comment on lines
+161
to
+167
| const maxFee = protocolFee.add(forwardingFee) | ||
| const maxFeeForBurn = this.getBurnMaxFee(maxFee) | ||
| if (amountInMin.lte(maxFeeForBurn)) { | ||
| return undefined | ||
| } | ||
| const expectedToAmount = amountInExpected.sub(maxFee) | ||
| const minToAmount = amountInMin.sub(maxFee) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find packages/sdk-router/src/cctpV2 -name "*.ts" | head -20Repository: synapsecns/sanguine
Length of output: 387
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '150,195p'Repository: synapsecns/sanguine
Length of output: 1558
🏁 Script executed:
rg -n 'minToAmount' packages/sdk-router/src --type=ts -C 3Repository: synapsecns/sanguine
Length of output: 32173
🏁 Script executed:
rg -n 'getBurnMaxFee|maxFeeForBurn' packages/sdk-router/src --type=ts -C 3Repository: synapsecns/sanguine
Length of output: 8393
🏁 Script executed:
rg -n 'finalizeBridgeRouteV2|bridgeV2' packages/sdk-router/src --type=ts -C 3Repository: synapsecns/sanguine
Length of output: 10159
🏁 Script executed:
cat -n packages/sdk-router/src/sir/synapseIntentRouterSet.ts | sed -n '46,100p'Repository: synapsecns/sanguine
Length of output: 2234
🏁 Script executed:
rg -n 'encodeZapData|minFinalAmount' packages/sdk-router/src/cctpV2 --type=ts -B 2 -A 2Repository: synapsecns/sanguine
Length of output: 957
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '320,340p'Repository: synapsecns/sanguine
Length of output: 911
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '340,390p'Repository: synapsecns/sanguine
Length of output: 1381
🏁 Script executed:
rg -n 'minFinalAmount|minFinalityThreshold' packages/sdk-router/src/cctpV2 --type=ts -B 2 -A 2Repository: synapsecns/sanguine
Length of output: 4415
🏁 Script executed:
rg -n 'BURN_MAX_FEE' packages/sdk-router/src/cctpV2 --type=tsRepository: synapsecns/sanguine
Length of output: 513
🏁 Script executed:
rg -n 'function encodeZapData|minFinalAmount' packages/sdk-router/src/swap --type=ts -B 2 -A 5 | head -80Repository: synapsecns/sanguine
Length of output: 5813
🏁 Script executed:
grep -r "minFinalAmount" packages/sdk-router/src --type=ts | grep -E "(encodeZapData|minFinalAmount)" | head -20Repository: synapsecns/sanguine
Length of output: 183
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '372,380p'Repository: synapsecns/sanguine
Length of output: 396
🏁 Script executed:
rg -A 10 'return encodeZapData' packages/sdk-router/src/cctpV2/cctpV2ModuleSet.tsRepository: synapsecns/sanguine
Length of output: 335
🏁 Script executed:
rg -A 15 'applyDefaultValues' packages/sdk-router/src/swap/core/zapData.tsRepository: synapsecns/sanguine
Length of output: 943
🏁 Script executed:
cat -n packages/sdk-router/src/swap/core/zapData.ts | sed -n '1,65p'Repository: synapsecns/sanguine
Length of output: 2050
🏁 Script executed:
cat -n packages/sdk-router/src/swap/core/zapData.ts | sed -n '90,110p'Repository: synapsecns/sanguine
Length of output: 835
🏁 Script executed:
rg -B 5 -A 10 'completeIntentWithBalanceChecks' packages/sdk-router/src/sir/synapseIntentRouterSet.ts | head -50Repository: synapsecns/sanguine
Length of output: 1383
🏁 Script executed:
rg -B 5 -A 5 'minFinalAmount.*minToAmount|minToAmount.*minFinalAmount' packages/sdk-router/src --type=tsRepository: synapsecns/sanguine
Length of output: 897
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '160,185p'Repository: synapsecns/sanguine
Length of output: 954
🏁 Script executed:
rg -n 'modifyMinFinalAmount|setMinFinalAmount' packages/sdk-router/src --type=ts -B 2 -A 2Repository: synapsecns/sanguine
Length of output: 2457
🏁 Script executed:
cat -n packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts | sed -n '344,355p'Repository: synapsecns/sanguine
Length of output: 462
Set minFinalAmount in zapData to enforce promised minToAmount protection.
minToAmount (line 167) is correctly computed using base maxFee, but the zapData encoding (line 378 via encodeZapData) does not include a minFinalAmount parameter. This causes minFinalAmount to default to zero, undermining the minimum received amount guarantee. The getBurnZapData method must pass minFinalAmount: minToAmount to encodeZapData so that the encoded bridge transaction enforces the quoted minimum.
This is a more severe issue than the fee buffer difference: the SDK returns a minToAmount in the BridgeRouteV2 struct (line 176), but the actual encoded transaction has zero slippage protection.
Also applies to: 181-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts` around lines 161 - 167,
The computed minToAmount used for slippage protection is not being passed into
the zap data, so getBurnZapData should include minFinalAmount: minToAmount when
calling encodeZapData; locate getBurnZapData and the encodeZapData call and add
the minFinalAmount field set to the already-computed minToAmount (and do the
same for the other similar call paths noted around lines 181-183), ensuring the
BridgeRouteV2.minToAmount returned matches the encoded zapData's minFinalAmount.
Bundle ReportChanges will increase total bundle size by 65.13kB (0.19%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sdk-router-@synapsecns/sdk-router-esmAssets Changed:
Files in
view changes for bundle: synapse-interface-client-array-pushAssets Changed:
Files in
view changes for bundle: sdk-router-@synapsecns/sdk-router-cjsAssets Changed:
Files in
view changes for bundle: synapse-interface-server-cjsAssets Changed:
Files in
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts (1)
178-183:⚠️ Potential issue | 🔴 CriticalEnforce the quoted
minToAmountin encoded zapData.
minToAmountis computed and returned (Line 167, Line 176), but the zap payload encoding does not setminFinalAmount(Line 374), which can leave execution-level min protection at its default and allow fills below the quoted minimum.🔧 Proposed fix
@@ zapData: this.getBurnZapData({ ...params, toToken: routeToToken, maxFee: maxFeeForBurn, minFinalityThreshold: selectedFee.finalityThreshold, + minFinalAmount: minToAmount, }), @@ private getBurnZapData({ @@ maxFee, minFinalityThreshold, + minFinalAmount, }: GetBridgeRouteV2Parameters & { maxFee: BigNumberish minFinalityThreshold: number + minFinalAmount: BigNumberish }): string | undefined { @@ return encodeZapData({ target: module.address, payload: module.populateDepositForBurnWithHook({ ...burnParams, amount: originSwapRoute.expectedToAmount, }).data, amountPosition: module.getAmountPosition(burnParams), + minFinalAmount, }) }Also applies to: 346-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts` around lines 178 - 183, The zap payload currently returned by getBurnZapData computes minToAmount but never sets it into the encoded zapData field minFinalAmount, so executions can bypass the quoted minimum; update the code path that builds/encodes the zapData (the getBurnZapData return or the encoder used where zapData is constructed) to assign minFinalAmount = minToAmount (or the computed minToAmount variable) before encoding/returning the zap payload so the runtime min protection matches the quoted value; ensure the same change is applied to the other encoding branch referenced (the second occurrence around the encode block covering lines ~346-381).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Around line 178-183: The zap payload currently returned by getBurnZapData
computes minToAmount but never sets it into the encoded zapData field
minFinalAmount, so executions can bypass the quoted minimum; update the code
path that builds/encodes the zapData (the getBurnZapData return or the encoder
used where zapData is constructed) to assign minFinalAmount = minToAmount (or
the computed minToAmount variable) before encoding/returning the zap payload so
the runtime min protection matches the quoted value; ensure the same change is
applied to the other encoding branch referenced (the second occurrence around
the encode block covering lines ~346-381).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2db0e5d-85b5-45a7-9461-6be856ed9073
📒 Files selected for processing (2)
packages/sdk-router/src/cctpV2/cctpV2ModuleSet.test.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts (1)
207-213:⚠️ Potential issue | 🔴 CriticalEncode
minFinalAmountin zapData to enforce quotedminToAmount.
minToAmountis computed for the route, but the zap payload is encoded withoutminFinalAmount, so on-chain execution can default to zero protection instead of enforcing the quote floor.🔧 Proposed fix
return { @@ zapData: this.getBurnZapData({ ...params, toToken: routeToToken, maxFee: maxFeeForBurn, minFinalityThreshold: selectedFee.finalityThreshold, + minFinalAmount: minToAmount, }), } } @@ private getBurnZapData({ bridgeToken, originSwapRoute, fromSender, toRecipient, maxFee, minFinalityThreshold, + minFinalAmount, }: GetBridgeRouteV2Parameters & { maxFee: BigNumberish minFinalityThreshold: number + minFinalAmount: BigNumberish }): string | undefined { @@ return encodeZapData({ target: module.address, payload: module.populateDepositForBurnWithHook({ ...burnParams, amount: originSwapRoute.expectedToAmount, }).data, amountPosition: module.getAmountPosition(burnParams), + minFinalAmount, }) }Also applies to: 376-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts` around lines 207 - 213, The zap payload built in zapData (via getBurnZapData) lacks the minFinalAmount field, so on-chain execution can ignore the computed route minToAmount; update calls to getBurnZapData (and any analogous get*ZapData uses around lines where routeToToken, maxFeeForBurn, selectedFee.finalityThreshold are passed) to include minFinalAmount: set it to the computed minToAmount for the route before encoding so the zap enforces the quoted floor; ensure getBurnZapData’s input shape supports minFinalAmount and propagate the value wherever zapData is constructed.packages/sdk-router/src/cctpV2/cctpV2Module.ts (1)
181-184:⚠️ Potential issue | 🟠 MajorPick the forwarding message instead of always taking the first one.
At Line 183, returning
messages[0]can bind status to the wrong message when the API returns multiple messages for the same tx. Match by forwarding hook data first, then fallback.🔧 Proposed fix
private async getMessageByTxHash( txHash: string, originChainId: number ): Promise<CctpV2Message | null> { @@ const messages = await getMessages(sourceDomain, txHash) - if (messages?.length) { - return messages[0] - } - return null + if (!messages?.length) { + return null + } + const expectedHookData = CCTP_V2_FORWARD_HOOK_DATA.toLowerCase() + return ( + messages.find( + (message) => + message.decodedMessage?.decodedMessageBody?.hookData?.toLowerCase() === + expectedHookData + ) ?? messages[0] + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/cctpV2Module.ts` around lines 181 - 184, The code currently returns messages[0] from getMessages(sourceDomain, txHash), which can select the wrong message when multiple messages exist; update the logic in the function using getMessages to search the returned messages array for the one whose hook/forwarding data matches the expected forwarding hook (e.g., compare the message's forwarding hook payload or identifier) and return that matched message, falling back to the first message only if no forwarding match is found; reference the getMessages call and the messages variable/array in cctpV2Module.ts and ensure the matching checks the message forwarding hook fields rather than positional index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Line 82: The getEstimatedTime method currently declares a parameter
_destChainId but does not use it, triggering `@typescript-eslint/no-unused-vars`;
fix it by explicitly consuming the parameter inside getEstimatedTime (e.g., add
a no-op reference like void _destChainId; or pass it into any existing logic) so
the symbol `_destChainId` is referenced and the linter error is resolved while
preserving the method signature.
In `@spec_cctp_v2_forwarder.md`:
- Line 48: The date "March 4, 2026" in the sentence about spot checks needs a
trailing comma for correct date style: update the text that currently reads
"March 4, 2026 show fee tiers..." to "March 4, 2026, show fee tiers..." (locate
the exact string "Spot checks against live Iris fee responses on March 4, 2026"
in the document and insert the comma after the year).
- Around line 94-97: Spec and implementation disagree: the spec requires a
5-second in-memory TTL cache for status API calls but
packages/sdk-router/src/cctpV2/api.ts currently does uncached message fetches;
to fix, add a 5s TTL cache around the status-fetching routine (the uncached
message/status fetch function in cctpV2/api.ts), mirroring the fee cache pattern
(Fee API TTL: 15s) — implement an in-memory cache keyed by message/tx
identifier, return cached value when not expired, and invalidate/refresh after 5
seconds so behavior matches the spec text.
---
Duplicate comments:
In `@packages/sdk-router/src/cctpV2/cctpV2Module.ts`:
- Around line 181-184: The code currently returns messages[0] from
getMessages(sourceDomain, txHash), which can select the wrong message when
multiple messages exist; update the logic in the function using getMessages to
search the returned messages array for the one whose hook/forwarding data
matches the expected forwarding hook (e.g., compare the message's forwarding
hook payload or identifier) and return that matched message, falling back to the
first message only if no forwarding match is found; reference the getMessages
call and the messages variable/array in cctpV2Module.ts and ensure the matching
checks the message forwarding hook fields rather than positional index.
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Around line 207-213: The zap payload built in zapData (via getBurnZapData)
lacks the minFinalAmount field, so on-chain execution can ignore the computed
route minToAmount; update calls to getBurnZapData (and any analogous get*ZapData
uses around lines where routeToToken, maxFeeForBurn,
selectedFee.finalityThreshold are passed) to include minFinalAmount: set it to
the computed minToAmount for the route before encoding so the zap enforces the
quoted floor; ensure getBurnZapData’s input shape supports minFinalAmount and
propagate the value wherever zapData is constructed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0d4d56d-e6c5-4d83-a2ce-e4c0306f3c17
📒 Files selected for processing (12)
packages/sdk-router/CHANGELOG.mdpackages/sdk-router/README.mdpackages/sdk-router/src/cctpV2/api.test.tspackages/sdk-router/src/cctpV2/api.tspackages/sdk-router/src/cctpV2/cctpV2Module.test.tspackages/sdk-router/src/cctpV2/cctpV2Module.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.test.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.tspackages/sdk-router/src/sdk.test.tspackages/sdk-router/src/sdk.tspackages/synapse-interface/slices/bridgeQuote/thunks.tsspec_cctp_v2_forwarder.md
✅ Files skipped from review due to trivial changes (1)
- packages/sdk-router/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk-router/src/cctpV2/cctpV2Module.test.ts
- packages/sdk-router/src/cctpV2/api.test.ts
- packages/sdk-router/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts (1)
198-214:⚠️ Potential issue | 🔴 CriticalPropagate quoted
minToAmountinto encoded zapData protection.Line 205 returns
minToAmount, but theencodeZapDatapayload at Line 407 currently omitsminFinalAmount. This can leave encoded min-received protection out of sync with the quoted route floor.💡 Suggested fix
- zapData: this.getBurnZapData({ + zapData: this.getBurnZapData({ ...params, toToken: routeToToken, maxFee: maxFeeForBurn, minFinalityThreshold: selectedFee.finalityThreshold, + minFinalAmount: minToAmount, }), @@ private getBurnZapData({ @@ maxFee, minFinalityThreshold, + minFinalAmount, }: GetBridgeRouteV2Parameters & { maxFee: BigNumberish minFinalityThreshold: number + minFinalAmount: BigNumberish }): string | undefined { @@ return encodeZapData({ target: module.address, payload: module.populateDepositForBurnWithHook({ ...burnParams, amount: originSwapRoute.expectedToAmount, }).data, amountPosition: module.getAmountPosition(burnParams), + minFinalAmount, }) }Also applies to: 407-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts` around lines 198 - 214, The returned object includes minToAmount but the encoded zap payload created in getBurnZapData/encodeZapData omits that quoted floor, so update getBurnZapData (and the encodeZapData call it uses) to accept and pass through the quoted minToAmount as the minFinalAmount (or equivalent min-final field) in the encoded payload; specifically, ensure the call site that builds zapData in cctpV2ModuleSet (the call to this.getBurnZapData) forwards minToAmount and that encodeZapData includes that value under minFinalAmount so encoded zap protection matches the returned minToAmount.
🧹 Nitpick comments (2)
packages/sdk-router/src/cctpV2/api.test.ts (2)
91-104: Assert request arguments, not only call count.
toHaveBeenCalledTimes(2)confirms no caching, but not request correctness. Consider assertinggetWithTimeoutcall args (endpoint andtransactionHash) to lock the API contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/api.test.ts` around lines 91 - 104, The test only checks mockGetWithTimeout call count; assert the actual request arguments to lock the API contract: verify that getMessages(SOURCE_DOMAIN_ID, MESSAGE_TX_HASH) invokes mockGetWithTimeout with the expected endpoint/URL and includes transactionHash (or the expected query/body) by using assertions like toHaveBeenNthCalledWith for the first and second calls; update the test around getMessages and mockGetWithTimeout to assert the exact call args (endpoint string and params) in addition to the call count.
28-82: Add an explicit cache-hit test forgetBurnUSDCFees.These cases validate parsing well, but the cache-backed path is still untested. Add one test that calls
getBurnUSDCFeestwice with the same domains and assertsgetWithTimeoutis called once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-router/src/cctpV2/api.test.ts` around lines 28 - 82, Add a cache-hit test for getBurnUSDCFees: mockGetWithTimeout to return a valid responseWithJson once, call getBurnUSDCFees(SOURCE_DOMAIN_ID, DEST_DOMAIN_ID) twice (await both), and assert the second call returns the same result while mockGetWithTimeout was invoked only once (e.g., expect(mockGetWithTimeout).toHaveBeenCalledTimes(1)); reference getBurnUSDCFees and mockGetWithTimeout so the test sits alongside the other specs and uses the same feeRows/responseWithJson setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Around line 162-166: The call to getBurnUSDCFees can throw and currently
causes getBridgeRouteV2 to reject; wrap the call in a try/catch inside
getBridgeRouteV2 (where feeEntries is retrieved), and on any error return
undefined (or fallback to treating feeEntries as empty) so route discovery
doesn't hard-fail; afterwards keep using this.getSlowestFeeEntry(selectedFee) as
before and optionally log a warning including the error to aid debugging.
---
Duplicate comments:
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts`:
- Around line 198-214: The returned object includes minToAmount but the encoded
zap payload created in getBurnZapData/encodeZapData omits that quoted floor, so
update getBurnZapData (and the encodeZapData call it uses) to accept and pass
through the quoted minToAmount as the minFinalAmount (or equivalent min-final
field) in the encoded payload; specifically, ensure the call site that builds
zapData in cctpV2ModuleSet (the call to this.getBurnZapData) forwards
minToAmount and that encodeZapData includes that value under minFinalAmount so
encoded zap protection matches the returned minToAmount.
---
Nitpick comments:
In `@packages/sdk-router/src/cctpV2/api.test.ts`:
- Around line 91-104: The test only checks mockGetWithTimeout call count; assert
the actual request arguments to lock the API contract: verify that
getMessages(SOURCE_DOMAIN_ID, MESSAGE_TX_HASH) invokes mockGetWithTimeout with
the expected endpoint/URL and includes transactionHash (or the expected
query/body) by using assertions like toHaveBeenNthCalledWith for the first and
second calls; update the test around getMessages and mockGetWithTimeout to
assert the exact call args (endpoint string and params) in addition to the call
count.
- Around line 28-82: Add a cache-hit test for getBurnUSDCFees:
mockGetWithTimeout to return a valid responseWithJson once, call
getBurnUSDCFees(SOURCE_DOMAIN_ID, DEST_DOMAIN_ID) twice (await both), and assert
the second call returns the same result while mockGetWithTimeout was invoked
only once (e.g., expect(mockGetWithTimeout).toHaveBeenCalledTimes(1)); reference
getBurnUSDCFees and mockGetWithTimeout so the test sits alongside the other
specs and uses the same feeRows/responseWithJson setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8066a6f-588c-49a8-a094-ea3562c4ac2a
📒 Files selected for processing (4)
packages/sdk-router/src/cctpV2/api.test.tspackages/sdk-router/src/cctpV2/cctpV2Module.test.tspackages/sdk-router/src/cctpV2/cctpV2Module.tspackages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk-router/src/cctpV2/cctpV2Module.ts
Comment on lines
+162
to
+166
| const feeEntries = await getBurnUSDCFees(originDomain, destDomain) | ||
| const selectedFee = feeEntries && this.getSlowestFeeEntry(feeEntries) | ||
| if (!selectedFee) { | ||
| return undefined | ||
| } |
Contributor
There was a problem hiding this comment.
Handle Circle fee API failures in-route to avoid hard rejection.
getBurnUSDCFees is an external call, and if it rejects, getBridgeRouteV2 rejects instead of safely returning undefined. That can break route discovery callers.
💡 Suggested fix
- const feeEntries = await getBurnUSDCFees(originDomain, destDomain)
+ let feeEntries: CctpV2Fee[] | undefined
+ try {
+ feeEntries = await getBurnUSDCFees(originDomain, destDomain)
+ } catch {
+ return undefined
+ }
const selectedFee = feeEntries && this.getSlowestFeeEntry(feeEntries)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const feeEntries = await getBurnUSDCFees(originDomain, destDomain) | |
| const selectedFee = feeEntries && this.getSlowestFeeEntry(feeEntries) | |
| if (!selectedFee) { | |
| return undefined | |
| } | |
| let feeEntries: CctpV2Fee[] | undefined | |
| try { | |
| feeEntries = await getBurnUSDCFees(originDomain, destDomain) | |
| } catch { | |
| return undefined | |
| } | |
| const selectedFee = feeEntries && this.getSlowestFeeEntry(feeEntries) | |
| if (!selectedFee) { | |
| return undefined | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts` around lines 162 - 166,
The call to getBurnUSDCFees can throw and currently causes getBridgeRouteV2 to
reject; wrap the call in a try/catch inside getBridgeRouteV2 (where feeEntries
is retrieved), and on any error return undefined (or fallback to treating
feeEntries as empty) so route discovery doesn't hard-fail; afterwards keep using
this.getSlowestFeeEntry(selectedFee) as before and optionally log a warning
including the error to aid debugging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
14-15: Prefer a shared export for the CCTPv2 module name.The SDK already defines the authoritative
'CCTPv2'identifier inpackages/sdk-router/src/cctpV2/cctpV2ModuleSet.ts. Re-declaring it here means a future rename can silently disable this preference logic until both packages are updated together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/synapse-interface/slices/bridgeQuote/thunks.ts` around lines 14 - 15, Remove the locally declared CCTP_V2_MODULE_NAME constant and instead import the authoritative CCTPv2 module name from the SDK's cctpV2ModuleSet export; replace uses of the local CCTP_V2_MODULE_NAME in this file (thunks.ts) with that imported symbol so the code relies on the single shared identifier provided by the SDK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/synapse-interface/slices/bridgeQuote/thunks.ts`:
- Around line 81-85: The selection currently finds cctpV2Quote via cctpV2Quote =
activeQuotes.find(q => q.moduleNames.includes(CCTP_V2_MODULE_NAME)) and falls
back to activeQuotes[0], but when persisting the chosen module it writes
bridgeModuleName as the last entry of quote.moduleNames; update the persistence
to store the actual chosen module: derive chosenModule =
quote.moduleNames.includes(CCTP_V2_MODULE_NAME) ? CCTP_V2_MODULE_NAME :
quote.moduleNames[0] (or quote.moduleNames.at(-1) if that was intended) and
persist chosenModule as bridgeModuleName so the saved state matches the route
selection (references: cctpV2Quote, activeQuotes, quote, bridgeModuleName,
moduleNames).
---
Nitpick comments:
In `@packages/synapse-interface/slices/bridgeQuote/thunks.ts`:
- Around line 14-15: Remove the locally declared CCTP_V2_MODULE_NAME constant
and instead import the authoritative CCTPv2 module name from the SDK's
cctpV2ModuleSet export; replace uses of the local CCTP_V2_MODULE_NAME in this
file (thunks.ts) with that imported symbol so the code relies on the single
shared identifier provided by the SDK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 382334a7-3f29-4c7e-beeb-8acb4b9019f3
📒 Files selected for processing (3)
packages/sdk-router/CHANGELOG.mdpackages/sdk-router/README.mdpackages/synapse-interface/slices/bridgeQuote/thunks.ts
✅ Files skipped from review due to trivial changes (1)
- packages/sdk-router/README.md
ChiTimesChi
force-pushed
the
feat/cctp-v2-forwarder
branch
from
0bbcd9e to
0c1545b
Compare
# Conflicts: # packages/sdk-router/src/sdk.test.ts
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
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
1 participant
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 by CodeRabbit
4c42861: synapse-interface preview link
2a13089: synapse-interface preview link
b12cd8c: synapse-interface preview link
22bb3ca: synapse-interface preview link
7e7e86b: synapse-interface preview link
0a3be1a: synapse-interface preview link