PolymerCCTP add support for more chains [PolymerCCTPFacet v1.1.0]#1630
PolymerCCTP add support for more chains [PolymerCCTPFacet v1.1.0]#1630
Conversation
…hainId 143) and Bsc (chainId 56) in bridging logic.
lifi-action-bot
changed the title
|
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:
WalkthroughAdds Solana-specific routing to PolymerCCTP (new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 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🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Facets/PolymerCCTPFacet.sol`:
- Around line 297-305: The code in PolymerCCTPFacet exposes BSC (chainId 56) as
a valid Circle domain while the facet only allows USDC (immutable USDC and
onlyAllowSourceToken), causing USDC→BSC attempts to fail; fix by removing the
BSC branch from the chain->domain mapping (the if (chainId == 56) return 17
block in the function that maps chainId to Circle domain), or alternatively add
an explicit validation in the bridging entrypoint (the function that performs
the CCTP transfer, guarded by onlyAllowSourceToken) to revert when destination
chainId == 56 to reject USDC→BSC routes; if you choose to support BSC, expand
the facet to accept USYC and update onlyAllowSourceToken/USDC handling
accordingly.
Test Coverage ReportLine Coverage: 86.79% (2983 / 3437 lines) |
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/demoScripts/demoAcrossV4.ts (1)
13-37:⚠️ Potential issue | 🟠 MajorMigrate demoAcrossV4.ts from deprecated ethers helpers to viem-based functions.
This script actively uses deprecated ethers-based helpers (
getProvider,getWalletFromPrivateKeyInDotEnv,sendTransaction,ensureBalanceAndAllowanceToDiamond) across multiple locations (lines 383–384, 526–534, 701). Replace with viem equivalents already exported fromdemoScriptHelpers:setupEnvironment()(returnspublicClient,walletAccount,lifiDiamondContract),executeTransaction(),ensureBalance(), andensureAllowance(). SeedemoUnit.tsfor the correct structural pattern.
🤖 Fix all issues with AI agents
In `@audit/auditLog.json`:
- Around line 542-548: The audit log entry "audit20260213" currently records
PolymerCCTPFacet v1.0.1 but the facet's `@custom`:version in the Solidity contract
is v1.1.0, causing the pipeline failure; update the auditedContracts mapping in
auditLog.json to include an entry for PolymerCCTPFacet v1.1.0 (or change the
contract's `@custom`:version to v1.0.1 if that was intended) so the key/value for
PolymerCCTPFacet in auditedContracts matches the contract's `@custom`:version and
the audit report path/commit hash reflect the correct version.
In `@script/demoScripts/demoPolymerCCTP.ts`:
- Around line 68-87: The code currently uses LIFI_AND_POLYMER_CHAIN_ID_SOLANA
(1151111081099710) for DST_CHAIN_ID which incorrectly sends a non-Polymer chain
id to Polymer API calls; define two separate constants (e.g.,
LIFI_SOLANA_CHAIN_ID = 1151111081099710 for on-chain bridgeData and
POLYMER_SOLANA_CHAIN_ID = 2 for Polymer API), update DST_CHAIN_ID or create a
new apiChainId variable so when BRIDGE_TO_SOLANA is true you pass
POLYMER_SOLANA_CHAIN_ID to getPolymerQuote() (and keep using
LIFI_SOLANA_CHAIN_ID where on-chain LiFi bridgeData requires it), and ensure
DST_CHAIN_ID_EVM logic (using LIFI_CHAIN_ID_ARBITRUM / LIFI_CHAIN_ID_OPTIMISM)
remains unchanged.
In `@script/demoScripts/utils/demoScriptHelpers.ts`:
- Around line 4-5: Add explicit Solana address validation before any ATA
derivation: where getAssociatedTokenAddress is called, validate external address
inputs the same way solanaAddressToBytes32 does by attempting to construct a
PublicKey in a try/catch and throw a clear, descriptive error if construction
fails (e.g., "invalid Solana address: <input>"). Update the functions that
accept external addresses (the call sites of getAssociatedTokenAddress) to
perform this validation prior to calling getAssociatedTokenAddress so invalid
base58 inputs are caught and surfaced with a clear message; follow the
solanaAddressToBytes32 validation pattern for exact behavior.
In `@test/solidity/Facets/PolymerCCTPFacet.t.sol`:
- Around line 366-377: Insert a blank line between the
vm.expectRevert(InvalidReceiver.selector); call and the subsequent
initiateBridgeTxWithFacet(false); invocation in the
testRevert_SolanaDestWithZeroSolanaReceiverATA test to follow the test style
guide; locate the vm.expectRevert usage and add a single empty line before the
initiateBridgeTxWithFacet call.
🧹 Nitpick comments (1)
src/Facets/PolymerCCTPFacet.sol (1)
308-310: Add test coverage for Monad (chainId 143 → domain 15) mapping.The new mapping in
_chainIdToDomainIdis not reflected intest_ChainIdToDomainId; consider adding a case so this stays covered.Test update sketch
-ChainMapping[] memory mappings = new ChainMapping[](16); +ChainMapping[] memory mappings = new ChainMapping[](17); @@ - mappings[11] = ChainMapping({ chainId: 1329, domainId: 16 }); // Sei + mappings[11] = ChainMapping({ chainId: 143, domainId: 15 }); // Monad + mappings[12] = ChainMapping({ chainId: 1329, domainId: 16 }); // Sei @@ - mappings[15] = ChainMapping({ chainId: 98866, domainId: 22 }); // Plume + mappings[16] = ChainMapping({ chainId: 98866, domainId: 22 }); // PlumeAs per coding guidelines, **/*.{test.ts,t.sol}: Prefer adding or updating tests alongside logic changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@script/demoScripts/utils/demoScriptHelpers.ts`:
- Around line 4-5: Replace the async ATA helper with the sync variant: change
imports of getAssociatedTokenAddress to getAssociatedTokenAddressSync in both
demoScriptHelpers.ts and demoEco.ts, remove any await when calling
getAssociatedTokenAddress (e.g. in functions where ata is derived) and call
getAssociatedTokenAddressSync(mintPublicKey, ownerPublicKey) instead so
derivation is done synchronously and no RPC/await is used.
🧹 Nitpick comments (1)
script/demoScripts/demoPolymerCCTP.ts (1)
431-434: Avoid logging raw balance before validation.This logs the wallet balance before
ensureBalancechecks it. It's fine for a demo script, but consider that logging the raw bigint withoutformatUnitsmakes it less readable.Suggested improvement
consola.info(`Wallet: ${walletAddress}`) - consola.info( - `Wallet balance: ${await tokenContract.read.balanceOf([walletAddress])}` - ) + const balance = await tokenContract.read.balanceOf([walletAddress]) + consola.info(`Wallet balance: ${formatUnits(balance, 6)} USDC`)
| /// @author LI.FI (https://li.fi) | ||
| /// @notice Provides functionality for bridging USDC through Polymer CCTP | ||
| /// @custom:version 1.0.0 | ||
| /// @custom:version 1.1.0 |
There was a problem hiding this comment.
I am wondering if 1.1.0 is really the right version here since the contract has breaking changes in the function calls the selectors are different now.
@0xDEnYO What do you think makes more sense. 1.1.0 or 2.0.0?
4 participants
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!!!)