EcoFacet Updates [EcoFacet v1.1.0, IEcoPortal v1.1.0] #1421
Conversation
WalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)audit/auditLog.json📄 CodeRabbit inference engine (conventions.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-09-12T10:17:51.686ZApplied to files:
⏰ 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). (2)
🔇 Additional comments (2)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 86.57% (2722 / 3144 lines) |
![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 (8)
docs/EcoFacet.md (3)
31-39: Document non-zero requirement for refundRecipientAdd that refundRecipient must be non-zero; contract reverts otherwise (InvalidConfig). Improves integration safety.
As per coding guidelines
79-84: Re-iterate duplicate protection pre-checkConsider adding that duplicates are prevented via PORTAL.getRewardStatus(intentHash) pre-flight check before funding, based on route+reward hashing, so integrators expect a revert if intent is already funded.
111-112: Minor: tighten ATA byte-range phrasingClarify “bytes 251–283 (exclusive end) of the route contain the 32-byte destination ATA” to match 32-byte slice semantics.
script/demoScripts/demoEco.ts (2)
551-557: Set refundRecipient from signer — also log it for clarityAssignment is correct. Add a log line so users see which address receives refunds.
Example (near other Eco data logs):
console.log(' - refundRecipient:', ecoData.refundRecipient)As per coding guidelines
1-713: Optional: align with script guidelines (consola + env validation + error handling)
- Prefer consola over console.* for structured logs.
- Validate ECO_API_URL and ECO_DAPP_ID via a getEnvVar helper.
- Wrap main flow in try/catch and exit non‑zero on errors.
Can address in a follow‑up to keep this PR focused.
As per coding guidelines
test/solidity/Facets/EcoFacet.t.sol (1)
915-945: Optional: assert reward creator equals refundRecipientIf the test stack can read the Reward created (via events or a mock Portal), assert that creator == customRefundRecipient to harden this check.
src/Facets/EcoFacet.sol (2)
241-242: Emit LiFiTransferStarted (and BridgeToNonEVMChain) before external callPer facet guidelines, emit LiFiTransferStarted at the start of the tx (before external calls). Move both events above publishAndFund while preserving their relative order.
As per coding guidelines
- PORTAL.publishAndFund( - destination, - _ecoData.encodedRoute, - reward, - ALLOW_PARTIAL_FILL - ); - - if (_bridgeData.destinationChainId == LIFI_CHAIN_ID_SOLANA) { - emit BridgeToNonEVMChain( - _bridgeData.transactionId, - _bridgeData.destinationChainId, - _ecoData.nonEVMReceiver - ); - } - - emit LiFiTransferStarted(_bridgeData); + if (_bridgeData.destinationChainId == LIFI_CHAIN_ID_SOLANA) { + emit BridgeToNonEVMChain( + _bridgeData.transactionId, + _bridgeData.destinationChainId, + _ecoData.nonEVMReceiver + ); + } + emit LiFiTransferStarted(_bridgeData); + + PORTAL.publishAndFund( + destination, + _ecoData.encodedRoute, + reward, + ALLOW_PARTIAL_FILL + );Also applies to: 226-233, 233-239
244-300: Validation set is good; minor optional tightenings
- Consider also verifying that the EVM Route.tokens entry matches sendingAssetId and totalAmount to catch mismatched route/token (Portal may already enforce).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(2 hunks)deployments/optimism.staging.json(1 hunks)docs/EcoFacet.md(4 hunks)script/demoScripts/demoEco.ts(1 hunks)src/Facets/EcoFacet.sol(8 hunks)src/Interfaces/IEcoPortal.sol(2 hunks)test/solidity/Facets/EcoFacet.t.sol(21 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
{README.md,Deploy.md,docs/**}
📄 CodeRabbit inference engine (conventions.md)
Keep primary documentation up to date: README.md (overview/setup), Deploy.md (deployment), and docs/ (technical docs)
Files:
docs/EcoFacet.md
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Interfaces/IEcoPortal.soltest/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
src/Interfaces/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/**/*.sol: All interfaces must reside under src/Interfaces, be in separate files from implementations, and use an I prefix (e.g., ILiFi)
Every interface must include NatSpec in this order: @title, @notice, @author LI.FI (https://li.fi), @Custom:version X.Y.Z
Files:
src/Interfaces/IEcoPortal.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Interfaces/IEcoPortal.solsrc/Facets/EcoFacet.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Interfaces/IEcoPortal.soltest/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Facets/EcoFacet.t.sol
src/Facets/**/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/**/*Facet.sol: Facet contracts must be located in src/Facets and include "Facet" in the contract name
Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Do not use msg.sender as refund address; require an explicit parameter for refund address
In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
For native fees in facets, use the _depositAndSwap variant and reserve required fees before execution
For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Files:
src/Facets/EcoFacet.sol
script/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/**/*.ts: All TypeScript scripts must follow .eslintrc.cjs, use async/await, try/catch error handling, proper logging, environment variables, correct typings, use citty for CLI parsing, consola for logging, validate env vars via getEnvVar(), and exit with appropriate codes
Prefer existing helper functions over reimplementation (e.g., getDeployments, getProvider, getWalletFromPrivateKeyInDotEnv, sendTransaction, ensureBalanceAndAllowanceToDiamond, getUniswapData*)
Always use proper TypeChain types (e.g., ILiFi.BridgeDataStruct) and never use any for bridge data or contract structures
Before/after changes: verify imports and types exist, ensure typechain is generated, keep changes comprehensive and consistent, run TS compilation, remove unused imports, and ensure function signatures match usage
Scripts must execute with bunx tsx
Files:
script/demoScripts/demoEco.ts
🧠 Learnings (22)
📚 Learning: 2025-09-09T10:39:26.383Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1357
File: deployments/lens.diamond.json:48-51
Timestamp: 2025-09-09T10:39:26.383Z
Learning: In the lifinance/contracts repository, when deployment JSON files show address changes (like AcrossFacetV3 address updates in deployments/*.diamond.json), the corresponding _deployments_log_file.json updates may be handled in separate PRs rather than the same PR that updates the diamond configuration files.
Applied to files:
deployments/optimism.diamond.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2025-04-21T03:15:12.236Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-09-27T07:10:15.586Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#812
File: deployments/polygon.diamond.json:4-11
Timestamp: 2024-09-27T07:10:15.586Z
Learning: In `deployments/polygon.diamond.json`, it's acceptable for certain facets to have empty names and versions when specified by the developer.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-09-22T00:52:26.172Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1388
File: deployments/hyperevm.diamond.json:72-75
Timestamp: 2025-09-22T00:52:26.172Z
Learning: In diamond configuration files (deployments/*.diamond.json), it's acceptable to have multiple versions of the same facet (e.g., GlacisFacet v1.0.0 and v1.1.0) deployed at different contract addresses. This is intentional design for version coexistence, gradual migration, or backward compatibility purposes.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-09-19T14:10:55.064Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:68-71
Timestamp: 2025-09-19T14:10:55.064Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to use msg.sender as the refund address for native tokens in the refundExcessNative modifier, contrary to the general guideline of requiring explicit refund parameters.
Applied to files:
docs/EcoFacet.mdtest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Applied to files:
docs/EcoFacet.mdtest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Applied to files:
docs/EcoFacet.mdtest/solidity/Facets/EcoFacet.t.soldeployments/optimism.staging.json
📚 Learning: 2025-10-13T11:13:48.819Z
Learnt from: mirooon
PR: lifinance/contracts#1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.819Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Do not use msg.sender as refund address; require an explicit parameter for refund address
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-25T07:47:30.735Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:306-336
Timestamp: 2025-09-25T07:47:30.735Z
Learning: In EcoFacet (src/Facets/EcoFacet.sol), the team intentionally uses variable-length validation for nonEVMReceiver addresses (up to 44 bytes) to support cross-ecosystem bridging with different address formats, rather than enforcing fixed 32-byte raw pubkeys.
Applied to files:
test/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.soldeployments/optimism.staging.json
📚 Learning: 2025-09-25T07:48:53.243Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:280-303
Timestamp: 2025-09-25T07:48:53.243Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to allow natural Solidity reverts (array bounds, insufficient data) in route validation rather than adding explicit guards with custom errors, as the reverts achieve the same security outcome while keeping code simpler.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
PR: lifinance/contracts#1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) and deployment log files have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
PR: lifinance/contracts#1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, understand that _targetState.json tracks contract version numbers (not addresses) and _deployments_log_file.json contains deployment records with addresses that may not be organized in obvious network sections. Always verify the actual file structure before flagging missing entries.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-05-28T17:33:33.959Z
Learnt from: mirooon
PR: lifinance/contracts#1170
File: deployments/_deployments_log_file.json:28060-28072
Timestamp: 2025-05-28T17:33:33.959Z
Learning: Deployment log files like `deployments/_deployments_log_file.json` are historical records that document the actual versions deployed at specific times. They should not be updated to match current contract versions - they must accurately reflect what was deployed when the deployment occurred.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-05-28T17:33:10.529Z
Learnt from: mirooon
PR: lifinance/contracts#1170
File: deployments/_deployments_log_file.json:28706-28717
Timestamp: 2025-05-28T17:33:10.529Z
Learning: Deployment log files (like `_deployments_log_file.json`) are historical records that should not be updated to match current contract versions. They should accurately reflect the versions that were actually deployed at specific timestamps.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#832
File: deployments/_deployments_log_file.json:23712-23720
Timestamp: 2024-12-04T01:59:34.045Z
Learning: In `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-01-09T04:17:46.190Z
Learnt from: ezynda3
PR: lifinance/contracts#914
File: deployments/_deployments_log_file.json:26902-26916
Timestamp: 2025-01-09T04:17:46.190Z
Learning: Updates to _deployments_log_file.json can represent backfilling of historical deployment data, not just new deployments. Such updates don't require new audits.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-09-30T03:52:27.281Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#812
File: deployments/_deployments_log_file.json:1914-1927
Timestamp: 2024-09-30T03:52:27.281Z
Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.
Applied to files:
deployments/_deployments_log_file.json
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Facets/EcoFacet.sol
[error] 1-1: Could not find any logged audit for contract EcoFacet in version 1.1.0.
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EcoFacet.sol
[failure] 332-332:
The contract is vulnerable to signature replay attacks, potentially allowing malicious actors to reuse valid signatures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/signature-replay-attacks
🪛 LanguageTool
docs/EcoFacet.md
[grammar] ~54-~54: There might be a mistake here.
Context: ...er - **For Solana destination chain**: - SetbridgeData.receivertoNON_EVM_AD...
(QB_NEW_EN)
⏰ 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). (5)
- GitHub Check: version-control
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (15)
deployments/optimism.diamond.staging.json (2)
193-193: Empty FeeForwarder is acceptable in diamond manifestsEmpty string for FeeForwarder is allowed for chains where it’s not deployed; no action needed.
Based on learnings
184-187: Approve EcoFacet update
Staging DIAMOND manifest correctly maps EcoFacet to 0xe9cF0bad… at v1.0.0; manually verify this matches the deployed facet bytecode/version.src/Interfaces/IEcoPortal.sol (2)
8-15: Version bump and Status enum LGTMHeader order and new Status enum align with usage in EcoFacet.
As per coding guidelines
37-39: Confirm intentHash schema alignment
The interface signature matches EcoFacet’s usage. Ensure the IEcoPortal implementation computes the intent hash as:keccak256(abi.encodePacked(destination, keccak256(route), keccak256(abi.encode(reward))))to match
EcoFacet._getIntentHash.test/solidity/Facets/EcoFacet.t.sol (1)
111-118: Tests updated for refundRecipient and new validations — LGTMStruct initializations and new negative tests validate the new field and constraints correctly.
Also applies to: 166-173, 336-343, 381-387, 411-417, 441-447, 479-485, 512-519, 547-554, 577-584, 605-612, 637-643, 668-675, 704-710, 731-738, 765-771, 795-801, 829-835, 865-872, 887-913, 915-945, 1000-1002
src/Facets/EcoFacet.sol (8)
142-158: Positive slippage refund — LGTMRefunding excess to msg.sender after swap is correct given requiresDeposit flow. This aligns with team practice for refundExcessNative.
Based on learnings
22-24: Error naming and placement — LGTMIntentAlreadyFunded is PascalCase and precise.
As per coding guidelines
28-30: Chain ID constants scoping — LGTMPrivate uint64 constants for Eco’s chain IDs are appropriate.
75-83: refundRecipient in EcoData and Reward.creator — LGTMExplicit refundRecipient parameter and its use as Reward.creator align with “don’t use msg.sender” for refund destinations at the protocol level.
As per coding guidelines
Also applies to: 164-184
248-253: Zero-address and deadline checks — LGTMGood guardrails; clear InvalidConfig usage.
1-341: Olympix “signature replay” warning appears inapplicableNo signature verification flow present; likely a false positive for this facet.
1-1: Add audit log entry for EcoFacet v1.1.0 before deploying to production.EcoFacet source code declares
@custom:version 1.1.0, but the audit log (audit/auditLog.json) only contains an entry for v1.0.0. Add a new audit reference for v1.1.0 underauditedContracts.EcoFacetin the audit metadata file (e.g.,"1.1.0": ["auditYYYYMMDD"]) and ensure the corresponding audit report exists inaudit/reports/before release.
210-219: Pre-flight duplicate intent guard—verify intentHash schema alignment
Ensure EcoFacet’s _getIntentHash (lines 332–340) computes intentHash with the same encoding and Reward field order as the Portal:
keccak256(abi.encodePacked(destination, keccak256(route), keccak256(abi.encode(reward))))deployments/optimism.staging.json (1)
49-49: EcoFacet address consistently updated across staging deployment filesVerification confirms the new EcoFacet address (
0xe9cF0bad93090f26051CCCe9A15a5c7395635D35) is correctly synchronized across optimism.staging.json, optimism.diamond.staging.json, and the deployment log. Production networks are unaffected. Changes look good.deployments/_deployments_log_file.json (1)
43370-43376: Do not overwrite historical deployment records.This edit replaces the existing 1.0.0 staging entry with new address/timestamp data, erasing the prior deployment history. If EcoFacet was redeployed (now v1.1.0 per the PR), append a new record under the correct version (or at least a new object in the array) and preserve the original 1.0.0 entry instead of mutating it. Based on learnings.
⛔ Skipped due to learnings
Learnt from: 0xDEnYO PR: lifinance/contracts#832 File: deployments/_deployments_log_file.json:23712-23720 Timestamp: 2024-12-04T01:59:34.045Z Learning: In `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.Learnt from: ezynda3 PR: lifinance/contracts#823 File: deployments/_deployments_log_file.json:10542-10543 Timestamp: 2024-11-21T08:17:27.878Z Learning: When reviewing deployment timestamps in `deployments/_deployments_log_file.json`, ensure that the timestamps are accurately compared with the current date to avoid incorrectly flagging valid timestamps as future deployment dates.Learnt from: mirooon PR: lifinance/contracts#1170 File: deployments/_deployments_log_file.json:28706-28717 Timestamp: 2025-05-28T17:33:10.529Z Learning: Deployment log files (like `_deployments_log_file.json`) are historical records that should not be updated to match current contract versions. They should accurately reflect the versions that were actually deployed at specific timestamps.Learnt from: mirooon PR: lifinance/contracts#1170 File: deployments/_deployments_log_file.json:28060-28072 Timestamp: 2025-05-28T17:33:33.959Z Learning: Deployment log files like `deployments/_deployments_log_file.json` are historical records that document the actual versions deployed at specific times. They should not be updated to match current contract versions - they must accurately reflect what was deployed when the deployment occurred.Learnt from: 0xDEnYO PR: lifinance/contracts#1357 File: deployments/lens.diamond.json:48-51 Timestamp: 2025-09-09T10:39:26.383Z Learning: In the lifinance/contracts repository, when deployment JSON files show address changes (like AcrossFacetV3 address updates in deployments/*.diamond.json), the corresponding _deployments_log_file.json updates may be handled in separate PRs rather than the same PR that updates the diamond configuration files.Learnt from: 0xDEnYO PR: lifinance/contracts#812 File: deployments/_deployments_log_file.json:1914-1927 Timestamp: 2024-09-30T03:52:27.281Z Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.Learnt from: 0xDEnYO PR: lifinance/contracts#812 File: deployments/_deployments_log_file.json:1914-1927 Timestamp: 2024-10-09T03:47:21.269Z Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.Learnt from: mirooon PR: lifinance/contracts#1283 File: deployments/ronin.diamond.json:65-68 Timestamp: 2025-08-07T10:20:01.383Z Learning: When analyzing deployment PRs in the lifinance/contracts repository, understand that _targetState.json tracks contract version numbers (not addresses) and _deployments_log_file.json contains deployment records with addresses that may not be organized in obvious network sections. Always verify the actual file structure before flagging missing entries.Learnt from: ezynda3 PR: lifinance/contracts#914 File: deployments/_deployments_log_file.json:26902-26916 Timestamp: 2025-01-09T04:17:46.190Z Learning: Updates to _deployments_log_file.json can represent backfilling of historical deployment data, not just new deployments. Such updates don't require new audits.Learnt from: 0xDEnYO PR: lifinance/contracts#807 File: deployments/bsc.diamond.staging.json:4-7 Timestamp: 2024-10-09T03:47:21.269Z Learning: In files with "staging" in their filename, such as `deployments/bsc.diamond.staging.json`, it is acceptable for facets to have empty "Name" and "Version" fields. Do not flag missing facet information in such files during reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
759-770: Critical: Test not using the variable it creates.The test creates
tooLongAddress(45 bytes) on line 759 but setsnonEVMReceiver: ""on line 765. This means the test is not validating the too-long address scenario it's named for and instead duplicates the empty receiver test.Apply this diff to fix the test:
EcoFacet.EcoData memory ecoData = EcoFacet.EcoData({ - nonEVMReceiver: "", + nonEVMReceiver: tooLongAddress, prover: address(0x1234), rewardDeadline: uint64(block.timestamp + 2 days), encodedRoute: solanaRoute,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/solidity/Facets/EcoFacet.t.sol(21 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
test/solidity/Facets/EcoFacet.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (9)
📚 Learning: 2025-09-19T14:10:55.064Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:68-71
Timestamp: 2025-09-19T14:10:55.064Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to use msg.sender as the refund address for native tokens in the refundExcessNative modifier, contrary to the general guideline of requiring explicit refund parameters.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
PR: lifinance/contracts#1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Do not use msg.sender as refund address; require an explicit parameter for refund address
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-25T07:47:30.735Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:306-336
Timestamp: 2025-09-25T07:47:30.735Z
Learning: In EcoFacet (src/Facets/EcoFacet.sol), the team intentionally uses variable-length validation for nonEVMReceiver addresses (up to 44 bytes) to support cross-ecosystem bridging with different address formats, rather than enforcing fixed 32-byte raw pubkeys.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:16:28.754Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
⏰ 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). (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (5)
test/solidity/Facets/EcoFacet.t.sol (5)
1005-1020: LGTM! Good coverage for duplicate intent prevention.This test properly validates that duplicate bridge calls with the same intent hash are rejected. The setup correctly funds the user with double the amount and approves sufficient tokens for two calls.
1022-1077: LGTM! Excellent test for positive slippage refund.The test properly verifies that when a swap results in more tokens than the minimum required, the excess is refunded to the user. The use of
assertGtcorrectly validates the refund behavior.
1079-1133: LGTM! Good validation coverage.These tests properly validate critical configuration parameters:
testRevert_ProverZeroAddress: Ensures prover cannot be zerotestRevert_RewardDeadlineExpired: Ensures deadline is in the futureBoth follow proper test structure and use specific error selectors.
1135-1194: LGTM! Comprehensive address validation tests.These tests provide good coverage for edge cases:
testRevert_NonEVMAddressForNonSolanaChain: Validates NON_EVM_ADDRESS restricted to SolanatestRevert_SolanaAddressTooShort: Validates minimum address length for SolanaBoth properly verify InvalidReceiver/InvalidConfig errors.
111-118: LGTM! Consistent refundRecipient additions.The
refundRecipientfield has been systematically added to allEcoDatainitializations throughout the test suite, usingUSER_SENDERas a sensible default value. This ensures tests cover the new required field.Also applies to: 166-173, 380-387, 410-417, 440-447, 478-485, 512-519, 547-554, 668-675, 703-710, 731-738, 794-801, 828-835, 865-872, 983-1003
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Facets/EcoFacet.sol (1)
332-340: Consider adding NatSpec for better documentation.While NatSpec is not required for private functions per the guidelines, documenting this helper would clarify:
- The purpose of the intent hash
- Why the hash is computed in this specific order (destination, routeHash, rewardHash)
- How it relates to the Eco Portal's intent tracking
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Facets/EcoFacet.sol(8 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
src/Facets/EcoFacet.sol
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Facets/EcoFacet.sol
src/Facets/**/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/**/*Facet.sol: Facet contracts must be located in src/Facets and include "Facet" in the contract name
Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Do not use msg.sender as refund address; require an explicit parameter for refund address
In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
For native fees in facets, use the _depositAndSwap variant and reserve required fees before execution
For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Files:
src/Facets/EcoFacet.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Facets/EcoFacet.sol
🧠 Learnings (4)
📚 Learning: 2025-07-03T01:44:43.968Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: In the lifinance/contracts codebase, use floating pragma directives (e.g., `pragma solidity ^0.8.17;`) rather than locking to exact versions. This allows the contracts to support multiple solc versions as preferred by the team.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-25T07:47:30.735Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:306-336
Timestamp: 2025-09-25T07:47:30.735Z
Learning: In EcoFacet (src/Facets/EcoFacet.sol), the team intentionally uses variable-length validation for nonEVMReceiver addresses (up to 44 bytes) to support cross-ecosystem bridging with different address formats, rather than enforcing fixed 32-byte raw pubkeys.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-25T07:48:53.243Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:280-303
Timestamp: 2025-09-25T07:48:53.243Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to allow natural Solidity reverts (array bounds, insufficient data) in route validation rather than adding explicit guards with custom errors, as the reverts achieve the same security outcome while keeping code simpler.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Facets/EcoFacet.sol
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Facets/EcoFacet.sol
[error] 1-1: Could not find any logged audit for contract EcoFacet in version 1.1.0.
⏰ 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). (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (8)
src/Facets/EcoFacet.sol (8)
19-19: Version bump noted; audit pending per pipeline.The version update to 1.1.0 is reflected correctly. The pipeline failure for the missing audit log is expected given the
AuditRequiredlabel on the PR.
28-29: Good optimization: immutable → constant.Since these chain IDs are known at compile time, declaring them as
constantrather thanimmutableeliminates storage slots and reduces deployment gas.
75-76: Explicit refund recipient aligns with facet guidelines.Adding
refundRecipientas an explicit parameter follows the coding guideline that prohibits usingmsg.senderas the refund address. The NatSpec clearly documents its purpose.As per coding guidelines
Also applies to: 82-83
142-157: Add test coverage for positive slippage handling.The logic correctly returns excess swap proceeds to the user while bridging exactly
minAmount. However, per previous review feedback, these lines lack test coverage.Please add test cases covering:
- Scenario where
actualAmountAfterSwap > minAmount(positive slippage returned to user)- Scenario where
actualAmountAfterSwap == minAmount(no excess transfer)- Verify the bridge receives exactly
minAmountin both casesBased on learnings
178-178: Correct: reward creator now uses explicit refundRecipient.This change ensures the refund recipient is explicitly specified rather than implicitly using
msg.sender, improving flexibility and aligning with the facet coding guidelines.As per coding guidelines
210-218: Strong replay protection; add test coverage.The intent hash validation correctly prevents duplicate funding of the same intent. The hash incorporates destination, route (including salt), and reward, ensuring uniqueness.
However, per previous review feedback, line 217 lacks test coverage. Please add a test case that:
- Funds an intent successfully
- Attempts to fund the same intent again with identical parameters
- Verifies the
IntentAlreadyFundedrevertThe previous review from mirooon suggested adapting the
testPoc_DuplicateBridgeCallAllowsFundTheftPoC from burrasec. Consider using that as a template.Based on learnings
249-250: Proper validation for refundRecipient.The zero-address check ensures a valid refund recipient is always specified, preventing funds from being locked or sent to an invalid address.
312-324: Clearer documentation for Solana ATA extraction.The updated comments better explain the Borsh serialization layout and the position of the destination Associated Token Account within the encoded route.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
750-781: Test logic doesn't match test name.The test
testRevert_SolanaRouteValidation_TooLongNonEVMReceivercreates atooLongAddress(45 bytes, lines 759-762) but never uses it. Line 765 setsnonEVMReceiver: ""instead ofnonEVMReceiver: tooLongAddress, so the test validates an empty address rather than an overly long one, contradicting its name and intent.Apply this diff to fix the test:
EcoFacet.EcoData memory ecoData = EcoFacet.EcoData({ - nonEVMReceiver: "", + nonEVMReceiver: tooLongAddress, prover: address(0x1234), rewardDeadline: uint64(block.timestamp + 2 days), encodedRoute: solanaRoute, solanaATA: bytes32(uint256(1)), refundRecipient: USER_SENDER });
♻️ Duplicate comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
915-971: Verify portal state after the bridge call.While
vm.expectCall(lines 954-963) checks thatpublishAndFundis invoked with the expected parameters includingcustomRefundRecipient, the test doesn't verify that the portal actually stored the reward with the correctcreatorfield.To strengthen the test, consider capturing the
intentHashand queryingPORTAL.getRewardStatus(intentHash)after the bridge operation to assert the stored reward's creator equalscustomRefundRecipient. This ensures end-to-end correctness beyond call parameter matching.Based on past review comments.
🧹 Nitpick comments (2)
test/solidity/Facets/EcoFacet.t.sol (2)
1048-1103: Consider more precise slippage verification.The test correctly verifies that positive slippage is refunded to the user by checking
usdcBalanceAfter > usdcBalanceBefore.For more robust validation, calculate the expected slippage refund and assert the exact amount:
// After line 1090 uint256[] memory amountsOut = uniswap.getAmountsOut( amountInWithSlippage, path ); uint256 expectedRefund = amountsOut[1] - totalAmountNeeded; // Replace lines 1096-1100 with: assertEq( usdcBalanceAfter, usdcBalanceBefore + expectedRefund, "User should receive exact positive slippage amount" );This provides stronger guarantees that the refund calculation is correct rather than just non-zero.
578-578: Inconsistent syntax for empty bytes.Lines 578 and 606 use
bytes("")while the rest of the file consistently uses just""for emptynonEVMReceivervalues (e.g., lines 112, 167, 381, 441).For consistency, use the simpler syntax:
- nonEVMReceiver: bytes(""), + nonEVMReceiver: "",Also applies to: 606-606
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/solidity/Facets/EcoFacet.t.sol(21 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
test/solidity/Facets/EcoFacet.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (9)
📚 Learning: 2025-09-19T14:10:55.064Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:68-71
Timestamp: 2025-09-19T14:10:55.064Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to use msg.sender as the refund address for native tokens in the refundExcessNative modifier, contrary to the general guideline of requiring explicit refund parameters.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
PR: lifinance/contracts#1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Do not use msg.sender as refund address; require an explicit parameter for refund address
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-25T07:47:30.735Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:306-336
Timestamp: 2025-09-25T07:47:30.735Z
Learning: In EcoFacet (src/Facets/EcoFacet.sol), the team intentionally uses variable-length validation for nonEVMReceiver addresses (up to 44 bytes) to support cross-ecosystem bridging with different address formats, rather than enforcing fixed 32-byte raw pubkeys.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:16:28.754Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
⏰ 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). (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (3)
test/solidity/Facets/EcoFacet.t.sol (3)
887-913: LGTM!Zero-address validation test for
refundRecipientis well-structured and correctly expectsInvalidConfig.selector.
1031-1046: LGTM!Duplicate funding prevention test is correctly structured. It appropriately provides sufficient funds, calls the bridge function twice with identical parameters, and expects
IntentAlreadyFunded.selectoron the second call.
1105-1220: LGTM!The validation tests (
testRevert_ProverZeroAddress,testRevert_RewardDeadlineExpired,testRevert_NonEVMAddressForNonSolanaChain,testRevert_SolanaAddressTooShort) are well-structured and correctly validate the new business rules for prover, reward deadline, nonEVM address usage, and Solana address length constraints. All follow the propertestRevert_naming convention and use appropriate error selectors.
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!!!)