Polymer hypercore EXSC-169 [PolymerCCTPFacet v3.0.0, ITokenMessenger v1.1.0]#1652
Open
0xDEnYO wants to merge 16 commits into
Open
Polymer hypercore EXSC-169 [PolymerCCTPFacet v3.0.0, ITokenMessenger v1.1.0]#16520xDEnYO wants to merge 16 commits into
0xDEnYO wants to merge 16 commits into
Conversation
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:
WalkthroughUpdates the PolymerCCTPFacet to support hook data and destination caller parameters in bridge operations, adding conditional burn logic based on hook data presence, extending domain mappings for HyperCore, and updating related deployment metadata and tests. Bumps PolymerCCTPFacet version to 3.0.0 and improves deployment script robustness. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
0xDEnYO
changed the title
0xDEnYO
changed the title
Collaborator
Test Coverage ReportLine Coverage: 88.10% (3216 / 3650 lines) |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
mirooon
previously approved these changes
Mar 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Facets/PolymerCCTPFacet.sol (1)
246-267: Consider extracting duplicated hook-routing logic.The hook-routing conditional (
if hookData.length > 0) is duplicated between the EVM (lines 210-231) and non-EVM (lines 246-267) branches. While the current implementation is clear and correct, extracting this into an internal helper could reduce code duplication.That said, the duplication is minor and the explicit branching improves readability for auditors reviewing the two distinct paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facets/PolymerCCTPFacet.sol` around lines 246 - 267, The hook-routing conditional (checking _polymerData.hookData.length and calling TOKEN_MESSENGER.depositForBurnWithHook vs TOKEN_MESSENGER.depositForBurn) is duplicated in both the EVM and non‑EVM branches; extract that logic into a single internal helper (e.g., _depositWithOptionalHook or similar) that accepts the common parameters (bridgeAmount, domainId, mintRecipient, token constant like USDC, destinationCaller/UNRESTRICTED_DESTINATION_CALLER, maxCCTPFee, minFinalityThreshold, hookData) and invoke that helper from both places (replace the two inline if/else blocks with calls to your new internal function) so the hook routing is centralized while preserving existing behavior and parameter choices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Facets/PolymerCCTPFacet.sol`:
- Around line 246-267: The hook-routing conditional (checking
_polymerData.hookData.length and calling TOKEN_MESSENGER.depositForBurnWithHook
vs TOKEN_MESSENGER.depositForBurn) is duplicated in both the EVM and non‑EVM
branches; extract that logic into a single internal helper (e.g.,
_depositWithOptionalHook or similar) that accepts the common parameters
(bridgeAmount, domainId, mintRecipient, token constant like USDC,
destinationCaller/UNRESTRICTED_DESTINATION_CALLER, maxCCTPFee,
minFinalityThreshold, hookData) and invoke that helper from both places (replace
the two inline if/else blocks with calls to your new internal function) so the
hook routing is centralized while preserving existing behavior and parameter
choices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcc088f6-4002-48e6-ba33-298dc9e2e52e
📒 Files selected for processing (6)
.cursor/rules/100-solidity-basics.mdcdeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsonsrc/Facets/PolymerCCTPFacet.solsrc/Interfaces/ITokenMessenger.soltest/solidity/Facets/PolymerCCTPFacet.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deployments/base.diamond.staging.json (1)
72-75: PolymerCCTPFacet Version field is empty but PR title indicates v3.0.0.The PR title explicitly references "PolymerCCTPFacet v3.0.0", yet the Version field here is an empty string. Consider populating the version for consistency with the PR scope and to support downstream scripts that may rely on version information for tracking or duplicate detection.
Suggested fix
"0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570": { "Name": "PolymerCCTPFacet", - "Version": "" + "Version": "3.0.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/base.diamond.staging.json` around lines 72 - 75, The JSON entry for the contract at address "0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570" lists "Name": "PolymerCCTPFacet" but has an empty "Version" field; update that "Version" value to "v3.0.0" (matching the PR title) so downstream tooling and duplicate-detection scripts can rely on consistent version metadata for the PolymerCCTPFacet entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/deploy/deploySingleContract.sh`:
- Around line 381-385: The trailing "0x" cleanup currently only runs when
CONSTRUCTOR_ARGS is >322 chars; remove that hardcoded size gate and run the
cleanup whenever CONSTRUCTOR_ARGS ends with "0x" by changing the outer condition
to only check the suffix (i.e., the "${CONSTRUCTOR_ARGS: -2}" == "0x" check) and
leave the inner alignment check on WITHOUT_TRAILING (the
($((${`#WITHOUT_TRAILING`} % 64)) -eq 2) logic) intact so shorter blobs ending
with "0x" are also fixed; refer to the CONSTRUCTOR_ARGS variable,
WITHOUT_TRAILING local, and the existing alignment check when updating the if
statement.
- Around line 367-378: The current logic sets CONSTRUCTOR_ARGS from the
broadcast JSON using jq with a default of "0x", which prevents the subsequent
fallback parser from running when the field is missing; update the assignment
and check so that CONSTRUCTOR_ARGS is unset/null when the broadcast file lacks
.returns.constructorArgs.value (for example remove the jq default // "0x" or
explicitly convert "null"/"0x" to empty), and adjust the conditional that
triggers the stdout/raw parser to run when CONSTRUCTOR_ARGS is empty or
"null"/"0x"; reference the BROADCAST_JSON lookup and the CONSTRUCTOR_ARGS /
JSON_DATA variables in the block so the fallback parser (the sed/grep/jq
sequence that populates JSON_DATA and then CONSTRUCTOR_ARGS) executes when the
broadcast value is absent.
---
Nitpick comments:
In `@deployments/base.diamond.staging.json`:
- Around line 72-75: The JSON entry for the contract at address
"0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570" lists "Name": "PolymerCCTPFacet"
but has an empty "Version" field; update that "Version" value to "v3.0.0"
(matching the PR title) so downstream tooling and duplicate-detection scripts
can rely on consistent version metadata for the PolymerCCTPFacet entry.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f61ab3af-8382-440c-8b79-ea2dd255d09b
📒 Files selected for processing (3)
deployments/base.diamond.staging.jsondeployments/base.staging.jsonscript/deploy/deploySingleContract.sh
✅ Files skipped from review due to trivial changes (1)
- deployments/base.staging.json
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/deploySingleContract.sh (1)
365-389:⚠️ Potential issue | 🟠 MajorThe new constructor-args extraction is overwritten before it is used.
This block computes and sanitizes
CONSTRUCTOR_ARGS, but Line 387 immediately reassigns it fromJSON_DATA. When the broadcast artifact path succeeds,JSON_DATAwas never populated, so non-empty constructor args can collapse back to0x. Even in the fallback path, the newline / trailing-0xcleanup here never survives into verification or logging.🛠️ Proposed fix
if [[ -z "${CONSTRUCTOR_ARGS:-}" || "$CONSTRUCTOR_ARGS" == "null" ]]; then local JSON_DATA JSON_DATA=$(echo "$RAW_RETURN_DATA" | sed -n '/{"logs":/,/}$/p' | tr -d '\n' | sed 's/} *$/}/') if [[ -z "$JSON_DATA" || "$JSON_DATA" == "" ]]; then JSON_DATA=$(echo "$RAW_RETURN_DATA" | grep -o '{"logs":.*}' | tail -1) fi - CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // "0x"' 2>/dev/null) + CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null) fi # Sanitize: forge stdout can embed newline + "0x" in the value, which breaks Etherscan verification CONSTRUCTOR_ARGS=$(printf '%s' "${CONSTRUCTOR_ARGS:-0x}" | tr -d '\n\r\t ') if [[ ${`#CONSTRUCTOR_ARGS`} -gt 322 && "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}" if [[ $((${`#WITHOUT_TRAILING`} % 64)) -eq 2 ]]; then CONSTRUCTOR_ARGS="$WITHOUT_TRAILING" fi fi - - CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head -1 | tr -d '\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/deploy/deploySingleContract.sh` around lines 365 - 389, The computed and sanitized CONSTRUCTOR_ARGS get clobbered by the later unconditional reassignment from JSON_DATA; remove or make that reassignment conditional so the earlier extraction/sanitization (the block that reads BROADCAST_JSON, RAW_RETURN_DATA, JSON_DATA and the sanitization/trailing-"0x" fix) survives; specifically, modify or remove the line that does CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head -1 | tr -d '\n') so that it only sets CONSTRUCTOR_ARGS when CONSTRUCTOR_ARGS is empty or unset, leaving the existing sanitized CONSTRUCTOR_ARGS intact for later validation and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@script/deploy/deploySingleContract.sh`:
- Around line 365-389: The computed and sanitized CONSTRUCTOR_ARGS get clobbered
by the later unconditional reassignment from JSON_DATA; remove or make that
reassignment conditional so the earlier extraction/sanitization (the block that
reads BROADCAST_JSON, RAW_RETURN_DATA, JSON_DATA and the
sanitization/trailing-"0x" fix) survives; specifically, modify or remove the
line that does CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r
'.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head
-1 | tr -d '\n') so that it only sets CONSTRUCTOR_ARGS when CONSTRUCTOR_ARGS is
empty or unset, leaving the existing sanitized CONSTRUCTOR_ARGS intact for later
validation and logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc93911a-3e06-4345-9030-527075fea66d
📒 Files selected for processing (4)
.cursor/rules/100-solidity-basics.mdcdeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsonscript/deploy/deploySingleContract.sh
✅ Files skipped from review due to trivial changes (1)
- deployments/arbitrum.staging.json
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/arbitrum.diamond.staging.json
- .cursor/rules/100-solidity-basics.mdc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/solidity/Facets/PolymerCCTPFacet.t.sol (1)
362-452: Consider adding a negative test for the hook-data path.Both new tests cover the happy path with non-empty
hookDataand a non-zerodestinationCaller. To strengthen coverage of the new branching introduced in_startBridge, please consider adding:
- A test asserting that
hookData = ""with a non-zerodestinationCallerfalls back todepositForBurn(and emitsLiFiTransferStartedwith the unadjusted flow). This will lock in the fallback semantics in case the condition at lines 210/246 is later changed.- If the source-side validation suggested in the related comment is adopted, a
vm.expectRevert(InvalidCallData.selector)test for thehookData != "" && destinationCaller == bytes32(0)case.Otherwise the new tests look correct — fork-executing against the live TokenMessenger gives reasonable assurance that
depositForBurnWithHookis wired up correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/PolymerCCTPFacet.t.sol` around lines 362 - 452, Add two negative tests in test/solidity/Facets/PolymerCCTPFacet.t.sol: (1) create a test (e.g., test_FallbackToDepositForBurnWhenHookEmpty) that sets hookData = "" and a non-zero destinationCaller, invokes PolymerCCTPFacet.startBridgeTokensViaPolymerCCTP and asserts the code follows the depositForBurn path by expecting the LiFiTransferStarted emit with the unadjusted flow (and any BridgeToNonEVMChainBytes32 emit when appropriate) to cover the _startBridge fallback to depositForBurn/depositForBurnWithHook; (2) create a test (e.g., test_RevertWhenHookProvidedButDestinationCallerZero) that sets hookData != "" and destinationCaller == bytes32(0) and uses vm.expectRevert(InvalidCallData.selector) before calling PolymerCCTPFacet.startBridgeTokensViaPolymerCCTP to lock in the source-side validation behavior. Ensure tests reference depositForBurn, depositForBurnWithHook, _startBridge, InvalidCallData.selector, and LiFiTransferStarted so reviewers can locate the covered branches.src/Facets/PolymerCCTPFacet.sol (1)
210-267: Optional: extract the hook/no-hook branching to remove duplication.The 22-line block selecting between
depositForBurnWithHookanddepositForBurnis repeated verbatim in both the EVM and non-EVM paths (onlymintRecipientdiffers). OncemintRecipientanddomainIdare computed, a single private helper would deduplicate the burn dispatch and reduce the chance of the two branches diverging in future updates.♻️ Suggested helper extraction
+ function _executeBurn( + uint256 _bridgeAmount, + uint32 _domainId, + bytes32 _mintRecipient, + PolymerCCTPData calldata _polymerData + ) private { + if (_polymerData.hookData.length > 0) { + TOKEN_MESSENGER.depositForBurnWithHook( + _bridgeAmount, + _domainId, + _mintRecipient, + USDC, + _polymerData.destinationCaller, + _polymerData.maxCCTPFee, + _polymerData.minFinalityThreshold, + _polymerData.hookData + ); + } else { + TOKEN_MESSENGER.depositForBurn( + _bridgeAmount, + _domainId, + _mintRecipient, + USDC, + UNRESTRICTED_DESTINATION_CALLER, + _polymerData.maxCCTPFee, + _polymerData.minFinalityThreshold + ); + } + }Then the two branches in
_startBridgecollapse to a single call each.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facets/PolymerCCTPFacet.sol` around lines 210 - 267, Extract the duplicated branching logic that chooses between TOKEN_MESSENGER.depositForBurnWithHook and TOKEN_MESSENGER.depositForBurn into a private helper (e.g., _dispatchCCTPBurn) that accepts domainId, bytes32 mintRecipient, uint256 bridgeAmount, PolymerData _polymerData (or the specific fields: destinationCaller, maxCCTPFee, minFinalityThreshold, hookData) and calls depositForBurnWithHook when hookData.length>0 otherwise depositForBurn with UNRESTRICTED_DESTINATION_CALLER; then replace the two identical blocks in _startBridge (the EVM path and the non-EVM path that compute mintRecipient) with a single call to this new helper to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Facets/PolymerCCTPFacet.sol`:
- Around line 354-356: The inline comment next to the conditional that checks
"chainId == 999 || chainId == LIFI_CHAIN_ID_HYPERCORE" is misleading because it
only mentions HyperEVM; update the trailing comment to indicate both HyperEVM
(999) and HyperCore (LIFI_CHAIN_ID_HYPERCORE) map to domain 19 (e.g., "//
HyperEVM / HyperCore (same domain)"), locating the change at the branch that
uses chainId and the LIFI_CHAIN_ID_HYPERCORE constant which returns 19.
- Around line 210-231: When _polymerData.hookData is non-empty the code passes
_polymerData.destinationCaller into TOKEN_MESSENGER.depositForBurnWithHook
without ensuring it's non-zero, which would allow anyone on the destination to
execute the hook; add a check in the PolymerCCTPFacet flow (the branches that
call depositForBurnWithHook) that reverts if _polymerData.hookData.length > 0
and _polymerData.destinationCaller == bytes32(0) (use a clear revert message),
and apply this validation before calling depositForBurnWithHook so both entry
points startBridgeTokensViaPolymerCCTP and
swapAndStartBridgeTokensViaPolymerCCTP are protected; alternatively (if
intended) update the struct docs for destinationCaller to explicitly allow zero
and describe the security implications.
---
Nitpick comments:
In `@src/Facets/PolymerCCTPFacet.sol`:
- Around line 210-267: Extract the duplicated branching logic that chooses
between TOKEN_MESSENGER.depositForBurnWithHook and
TOKEN_MESSENGER.depositForBurn into a private helper (e.g., _dispatchCCTPBurn)
that accepts domainId, bytes32 mintRecipient, uint256 bridgeAmount, PolymerData
_polymerData (or the specific fields: destinationCaller, maxCCTPFee,
minFinalityThreshold, hookData) and calls depositForBurnWithHook when
hookData.length>0 otherwise depositForBurn with UNRESTRICTED_DESTINATION_CALLER;
then replace the two identical blocks in _startBridge (the EVM path and the
non-EVM path that compute mintRecipient) with a single call to this new helper
to remove duplication and keep behavior identical.
In `@test/solidity/Facets/PolymerCCTPFacet.t.sol`:
- Around line 362-452: Add two negative tests in
test/solidity/Facets/PolymerCCTPFacet.t.sol: (1) create a test (e.g.,
test_FallbackToDepositForBurnWhenHookEmpty) that sets hookData = "" and a
non-zero destinationCaller, invokes
PolymerCCTPFacet.startBridgeTokensViaPolymerCCTP and asserts the code follows
the depositForBurn path by expecting the LiFiTransferStarted emit with the
unadjusted flow (and any BridgeToNonEVMChainBytes32 emit when appropriate) to
cover the _startBridge fallback to depositForBurn/depositForBurnWithHook; (2)
create a test (e.g., test_RevertWhenHookProvidedButDestinationCallerZero) that
sets hookData != "" and destinationCaller == bytes32(0) and uses
vm.expectRevert(InvalidCallData.selector) before calling
PolymerCCTPFacet.startBridgeTokensViaPolymerCCTP to lock in the source-side
validation behavior. Ensure tests reference depositForBurn,
depositForBurnWithHook, _startBridge, InvalidCallData.selector, and
LiFiTransferStarted so reviewers can locate the covered branches.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 636319af-8f62-418c-8633-7d560657d4d7
📒 Files selected for processing (4)
deployments/arbitrum.diamond.staging.jsonscript/deploy/deploySingleContract.shsrc/Facets/PolymerCCTPFacet.soltest/solidity/Facets/PolymerCCTPFacet.t.sol
✅ Files skipped from review due to trivial changes (1)
- deployments/arbitrum.diamond.staging.json
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/deploySingleContract.sh
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8 tasks
# Conflicts: # deployments/arbitrum.diamond.staging.json
- PolymerCCTPFacet: revert with InvalidConfig when hookData is non-empty but destinationCaller is bytes32(0); enforces the documented restricted- forwarder model on the destination domain. Adds two revert tests. - PolymerCCTPFacet: update inline comment to reflect that domain 19 covers both HyperEVM (chainId 999) and HyperCore (LIFI_CHAIN_ID_HYPERCORE). - deploySingleContract.sh: switch broadcast-file jq fallback from `// "0x"` to `// empty` so the stdout fallback parser runs when the broadcast artifact lacks `.returns.constructorArgs.value`. Remove the hardcoded 322-char gate on the trailing `0x` cleanup. Drop a redundant re-extraction that overwrote the earlier value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restore the post-sanitize re-extraction step that the previous commit removed wholesale. The original line was Daniel's fix for newline- corrupted constructor args coming from forge stdout (commit abba53f, "remove newline char from constructor args"). The bug it had was running unconditionally, which clobbered a clean value extracted from the broadcast artifact (Path 1) with an empty re-read against an unset JSON_DATA. Gate it on JSON_DATA being non- empty so it only runs in the stdout-fallback path (Path 2), and only overwrite when the re-extract itself yielded something non-empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
4 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.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)