Implement EcoFacet [EcoFacet v1.0.0,IEcoPortal v1.0.0] #1324
Conversation
WalkthroughAdds EcoFacet and IEcoPortal, docs, tests, deploy/update scripts, a TypeScript demo and helper constants, deployment metadata/log entries, a devDependency, and updates Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 0
🔭 Outside diff range comments (1)
flake.nix (1)
1-44: PR title mismatch — "Implement EcoFacet" but no EcoFacet added; only devShell change (flake.nix)Verified findings (checked HEAD on branch coderabbit_1324):
- HEAD commit: "fix missing packages for nix"
- Files changed in this PR: flake.nix (only)
- No occurrences of "EcoFacet" in the repository (search returned nothing)
- src/Facets/ exists and contains many facets, but no EcoFacet
Action required — please do one of the following:
- Add the EcoFacet contract (src/Facets/EcoFacet.sol) plus tests/docs, following the project's Diamond conventions (name must include "Facet"), or
- Retitle the PR to reflect the actual change, e.g. "DevShell: add jq/bc/gum/mongosh"
🧹 Nitpick comments (2)
flake.nix (2)
26-29: Useful tooling additions; consider splitting optional/heavy tools into a separate devShell to keep default leanjq/bc are lightweight and commonly used in scripts; gum and especially mongosh tend to be optional and significantly increase the initial download/activation cost for all contributors. Recommend moving gum and mongosh into a secondary dev shell (e.g., .#tools) so default stays fast.
Apply this diff to keep default minimal:
- jq - bc - gum - mongosh + jq + bcAdd this new dev shell (outside the selected lines) to offer the extras on demand:
devShells.tools = pkgs.mkShell { buildInputs = with pkgs; [ gum mongosh ]; };Developers who need these can run: nix develop .#tools
22-30: mkShell best practice: preferpackagesoverbuildInputsfor readabilityIn mkShell,
packagesis the modern, clearer attribute for PATH-population. Functionally similar here, but improves intent and consistency.- buildInputs = with pkgs; [ + packages = with pkgs; [ bun foundry stdenv.cc.cc.lib jq bc gum mongosh ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flake.nix(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
flake.nix (1)
26-29: Confirm: mongosh & gum available on nixpkgs-unstable; mongosh licensed Apache-2.0Both mongosh and gum are packaged on nixpkgs-unstable for Linux and macOS (x86_64-linux / x86_64-darwin). The MongoDB Shell (mongosh) is released under Apache-2.0. No blocking license or portability issues found.
- Files to note: flake.nix — lines 26–29 (jq, bc, gum, mongosh) — leave as-is.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Facets/EcoFacet.sol (1)
178-181: Consider checking the return value ofpublishAndFund.The static analysis hint indicates that the return value from
publishAndFundis not being used. While this might be intentional, consider whether the returned intent ID should be logged or used for tracking purposes.#!/bin/bash # Description: Check how other facets handle return values from bridge protocols # Search for similar external bridge calls in other facets ast-grep --pattern '$_($$$).$METHOD{value: $_}($$$)' # Check if other facets capture and use return values rg -A 2 -B 2 'returns \(bytes32\)' src/Interfaces/If the return value contains important information like an intent ID, consider capturing and emitting it in an event for better observability:
- IEco(_ecoData.intentSource).publishAndFund{ value: nativeValue }( - intent, - _ecoData.allowPartial - ); + bytes32 intentId = IEco(_ecoData.intentSource).publishAndFund{ value: nativeValue }( + intent, + _ecoData.allowPartial + ); + // Consider emitting the intentId if needed for tracking
🧹 Nitpick comments (4)
src/Facets/EcoFacet.sol (1)
46-47: Consider reusing existing error types.The
InvalidDeadlineerror is already defined inGenericErrors.solaccording to the AI summary. Consider importing and reusing it instead of redefining it here.-error InvalidProver(); -error InvalidDeadline(); +import { InvalidDeadline, InvalidProver } from "../Errors/GenericErrors.sol";Then remove the error definitions on lines 46-47.
test/solidity/Facets/EcoFacet.t.sol (3)
160-170: Test assertion could be more specific.The test only checks for event emission but doesn't verify that the DEFAULT_PROVER is actually used in the intent. Consider enhancing the test to decode and verify the event data.
function test_EcoFacet_UsesDefaultProverWhenNotProvided() public { // validEcoData already has prover set to address(0) // Should use DEFAULT_PROVER from constructor - vm.expectEmit(false, false, false, true, address(mockIntentSource)); - - // We expect the intent to have DEFAULT_PROVER - // The actual event checking would require decoding the Intent struct + // Expect the MockIntentSource to emit with the correct prover + vm.expectEmit(true, true, true, true, address(mockIntentSource)); + + // Construct expected intent with DEFAULT_PROVER + IEco.Intent memory expectedIntent = IEco.Intent({ + route: IEco.Route({ + source: address(ecoFacet), + destination: bridgeData.destinationChainId, + data: validEcoData.routeData + }), + reward: IEco.Reward({ + prover: DEFAULT_PROVER, + tokens: _createTokenArray(bridgeData.sendingAssetId), + amounts: _createAmountArray(bridgeData.minAmount), + deadline: validEcoData.deadline, + nonce: validEcoData.nonce + }) + }); + + emit MockIntentSource.IntentPublishedAndFunded(expectedIntent, false, 0); ecoFacet.startBridgeTokensViaEco(bridgeData, validEcoData); } + + function _createTokenArray(address token) private pure returns (address[] memory) { + address[] memory tokens = new address[](1); + tokens[0] = token; + return tokens; + } + + function _createAmountArray(uint256 amount) private pure returns (uint256[] memory) { + uint256[] memory amounts = new uint256[](1); + amounts[0] = amount; + return amounts; + }
172-179: Consider adding assertion for custom prover usage.Similar to the default prover test, this test should verify that the custom prover is actually used in the intent.
Add event verification to ensure the custom prover is used:
function test_EcoFacet_UsesProvidedProverWhenSet() public { address customProver = address(0x9999); EcoFacet.EcoData memory customData = validEcoData; customData.prover = customProver; + // Expect the event with custom prover + vm.expectEmit(true, true, true, true, address(mockIntentSource)); + + // Construct expected intent with custom prover + IEco.Intent memory expectedIntent = IEco.Intent({ + route: IEco.Route({ + source: address(ecoFacet), + destination: bridgeData.destinationChainId, + data: customData.routeData + }), + reward: IEco.Reward({ + prover: customProver, + tokens: _createTokenArray(bridgeData.sendingAssetId), + amounts: _createAmountArray(bridgeData.minAmount), + deadline: customData.deadline, + nonce: customData.nonce + }) + }); + + emit MockIntentSource.IntentPublishedAndFunded(expectedIntent, false, 0); + // Should use the provided prover instead of default ecoFacet.startBridgeTokensViaEco(bridgeData, customData); }
140-179: Consider adding more test coverage.While the test suite covers basic functionality, consider adding tests for:
- Native token bridging scenarios (with msg.value)
- Swap and bridge functionality
- Partial fulfillment scenarios (allowPartial = true)
- Edge cases around deadline (e.g., deadline == block.timestamp)
Would you like me to generate additional test cases to improve coverage for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (9)
config/eco.json(1 hunks)docs/EcoFacet.md(1 hunks)script/demoScripts/demoEco.ts(1 hunks)script/deploy/facets/DeployEcoFacet.s.sol(1 hunks)script/deploy/facets/UpdateEcoFacet.s.sol(1 hunks)src/Errors/GenericErrors.sol(1 hunks)src/Facets/EcoFacet.sol(1 hunks)src/Interfaces/IEco.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/EcoFacet.md
- config/eco.json
🧰 Additional context used
📓 Path-based instructions (9)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
script/deploy/facets/UpdateEcoFacet.s.solsrc/Interfaces/IEco.solsrc/Facets/EcoFacet.solsrc/Errors/GenericErrors.solscript/deploy/facets/DeployEcoFacet.s.soltest/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
script/deploy/facets/UpdateEcoFacet.s.solsrc/Interfaces/IEco.solsrc/Facets/EcoFacet.solsrc/Errors/GenericErrors.solscript/deploy/facets/DeployEcoFacet.s.soltest/solidity/Facets/EcoFacet.t.sol
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}: Deployment scripts must reside inscript/deploy/facets/for base deployments andscript/deploy/zksync/for ZKSync-specific scripts.
Deployment scripts must inheritDeployScriptBaseand use JSON config withstdJson.
Update scripts must inheritUpdateScriptBaseand useupdate("{ContractName}").
Files:
script/deploy/facets/UpdateEcoFacet.s.solscript/deploy/facets/DeployEcoFacet.s.sol
src/Interfaces/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
src/Interfaces/*.sol: All interfaces must start withIprefix (e.g.,ILiFi,IStargate).
All interfaces must be placed in separate files in thesrc/Interfacesdirectory.
Every interface MUST have the following NatSpec tags in this order: @title, @notice, @author, @Custom:version.
Files:
src/Interfaces/IEco.sol
src/Interfaces/I*.sol
📄 CodeRabbit Inference Engine (conventions.md)
All interfaces must be placed in separate files in the
src/Interfacesdirectory and start withI.
Files:
src/Interfaces/IEco.sol
src/Facets/*Facet.sol
📄 CodeRabbit Inference Engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
script/**/*.ts
📄 CodeRabbit Inference Engine (conventions.md)
script/**/*.ts: All scripts must follow the rules defined in.eslintrc.cjs.
Use async/await for asynchronous operations in TypeScript scripts.
Handle errors appropriately with try/catch blocks in TypeScript scripts.
Include proper logging for debugging and monitoring in TypeScript scripts.
Use environment variables for configuration in TypeScript scripts.
All scripts must usecittyfor CLI argument parsing.
Useconsolafor consistent logging across TypeScript scripts.
Environment variables should be validated usinggetEnvVar()helper in TypeScript scripts.
Scripts should exit with appropriate exit codes (0 for success, 1 for error) in TypeScript scripts.
Files:
script/demoScripts/demoEco.ts
src/Errors/GenericErrors.sol
📄 CodeRabbit Inference Engine (conventions.md)
src/Errors/GenericErrors.sol: All custom errors must be defined insrc/Errors/GenericErrors.sol.
Error names should be descriptive and follow PascalCase.
Errors should not include error messages (for gas optimization).
All custom errors should be defined insrc/Errors/GenericErrors.soland not include error messages.
Files:
src/Errors/GenericErrors.sol
test/solidity/**/*.t.sol
📄 CodeRabbit Inference Engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Update scripts must inherit `UpdateScriptBase` and use `update("{ContractName}")`.
Applied to files:
script/deploy/facets/UpdateEcoFacet.s.solscript/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must inherit `DeployScriptBase` and use JSON config with `stdJson`.
Applied to files:
script/deploy/facets/UpdateEcoFacet.s.solscript/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must reside in `script/deploy/facets/` for base deployments and `script/deploy/zksync/` for ZKSync-specific scripts.
Applied to files:
script/deploy/facets/UpdateEcoFacet.s.solscript/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-01-28T14:27:50.689Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
Applied to files:
script/deploy/facets/UpdateEcoFacet.s.solscript/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-11-25T09:04:55.880Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Errors/GenericErrors.sol : All custom errors must be defined in `src/Errors/GenericErrors.sol`.
Applied to files:
src/Errors/GenericErrors.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Errors/GenericErrors.sol : All custom errors should be defined in `src/Errors/GenericErrors.sol` and not include error messages.
Applied to files:
src/Errors/GenericErrors.sol
📚 Learning: 2024-09-23T01:42:03.075Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipFacet.s.sol:22-35
Timestamp: 2024-09-23T01:42:03.075Z
Learning: In deployment scripts like `DeployGasZipFacet.s.sol`, do not report issues suggesting adding error handling for missing configuration files or unexpected JSON structures, as the script will fail if the file is missing.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-06-13T08:30:28.665Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/facets/DeployPioneerFacet.s.sol:17-27
Timestamp: 2025-06-13T08:30:28.665Z
Learning: Deployment scripts rely on external automation (pre-script checks) to validate constructor arguments such as non-zero addresses, so adding explicit `require` guards inside the Foundry deployment scripts is generally unnecessary.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-10-31T09:07:36.301Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: script/deploy/facets/DeployRelayFacet.s.sol:0-0
Timestamp: 2024-10-31T09:07:36.301Z
Learning: In `script/deploy/facets/DeployRelayFacet.s.sol`, additional validation for the configuration file existence and address validity is unnecessary because the code will already fail appropriately if the configuration file is missing or the addresses are invalid.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-11-25T13:49:40.464Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-01-20T04:21:41.973Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#931
File: script/deploy/facets/DeployCBridgeFacetPacked.s.sol:27-29
Timestamp: 2025-01-20T04:21:41.973Z
Learning: In deployment scripts, validation of deployerAddress is not needed as it's guaranteed to be available through the deploy script's context.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-06-05T10:00:01.583Z
Learnt from: mirooon
PR: lifinance/contracts#1145
File: script/tasks/diamondSyncWhitelistedAddresses.sh:208-209
Timestamp: 2025-06-05T10:00:01.583Z
Learning: Task scripts in script/tasks/ directory (like diamondSyncWhitelistedAddresses.sh) define functions that are sourced by deployAllContracts.sh and called from there, rather than executing directly. They don't need to call their own functions at the end of the file.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-23T08:12:29.064Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/deploy/facets/UpdateWhitelistManagerFacet.s.sol:1-1
Timestamp: 2025-07-23T08:12:29.064Z
Learning: Deployment scripts in script/deploy/ should not use the LGPL-3.0 license identifier, contrary to other Solidity files. Scripts can use UNLICENSED or other appropriate license identifiers.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
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
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EcoFacet.sol
[warning] 178-178:
Calling a function without checking the return value may lead to silent failures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unused-return-function-call
🔇 Additional comments (19)
src/Errors/GenericErrors.sol (1)
20-25: New errors are well-scoped, centralized, and convention-compliantBoth
InvalidDeadline()andInvalidProver()follow our error conventions (PascalCase, no messages) and are correctly added to GenericErrors for reuse across the codebase.script/deploy/facets/UpdateEcoFacet.s.sol (2)
10-15: Update script pattern looks correctInherits
UpdateScriptBaseand delegates viaupdate("EcoFacet"). This aligns with our deployment/update workflow.
1-1: Confirm SPDX identifier policy for deployment scriptsThere’s an org-level nuance: coding guidelines say scripts use LGPL-3.0, while team learnings (PR #1193) indicate deploy scripts should use UNLICENSED. Please confirm which to follow for consistency across this repo before merging.
If UNLICENSED is preferred, apply:
-// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-License-Identifier: UNLICENSEDscript/deploy/facets/DeployEcoFacet.s.sol (1)
1-1: Confirm SPDX identifier policy for deployment scriptsSame nuance as the update script: guidelines vs. team practice differ. Please confirm before merge.
If UNLICENSED is preferred:
-// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-License-Identifier: UNLICENSEDsrc/Interfaces/IEco.sol (1)
5-58: Interface conforms to repo standards and is well-documented
- Correct location and I-prefixed name.
- Required NatSpec headers present and in the correct order.
- Function NatSpec with params/returns provided.
- Structs capture the needed fields for the Eco intent system.
Looks good.
src/Facets/EcoFacet.sol (10)
14-17: LGTM! Contract metadata is properly documented.The NatSpec documentation follows the required format with all mandatory tags (
@title,@author,@notice,@custom:version) in the correct order.
53-56: LGTM! Constructor properly validates the default prover.The constructor correctly validates that the default prover is not the zero address, preventing misconfiguration.
60-80: LGTM! Entry point is properly protected and validated.The function has appropriate modifiers for reentrancy protection, validation, and excess native refund. It correctly deposits assets before initiating the bridge.
86-106: LGTM! Swap and bridge function is well-structured.The function correctly performs swaps before bridging with proper validation and protection modifiers. The updated
minAmountfrom swaps is correctly passed to the bridge.
117-120: LGTM! Intent source validation is correct.The function properly validates that the intent source address is not zero before proceeding with the bridge operation.
123-125: LGTM! Deadline validation prevents expired intents.The validation correctly ensures the deadline is in the future by checking it's greater than the current block timestamp.
141-143: LGTM! Prover selection logic is correct.The ternary operator correctly uses the provided prover if non-zero, otherwise falls back to the DEFAULT_PROVER.
159-165: LGTM! Token approval logic is correct.The function correctly checks if the asset is native before attempting to approve the ERC20 token to the intent source.
174-175: LGTM! Event emission follows best practices.The
LiFiTransferStartedevent is correctly emitted before the external call, which is safe due to thenonReentrantmodifier protection on the entry points.
128-132: No change required —address(this)is correct (it will be the diamond address at runtime)The facet is invoked via the diamond (delegatecall), so address(this) inside the facet resolves to the diamond address. Tests demonstrate this usage.
- src/Facets/EcoFacet.sol — IEco.Route { source: address(this), ... } (snapshot around lines 128–132).
- test/solidity/Facets/EcoFacet.t.sol — setUp adds the facet to the diamond and then does
ecoFacet = TestEcoFacet(address(diamond));, confirming calls run through the diamond.Likely an incorrect or invalid review comment.
test/solidity/Facets/EcoFacet.t.sol (4)
11-37: LGTM! Mock implementation is well-structured.The mock Intent Source correctly implements the IEco interface for testing purposes with appropriate event emission and deterministic return values.
40-50: LGTM! Test wrapper provides necessary access to internal functions.The test wrapper correctly exposes LibAllowList functionality needed for test setup.
58-110: LGTM! Test setup is comprehensive and well-organized.The setup correctly:
- Initializes the test base
- Deploys mock contracts
- Configures the diamond with the facet
- Sets up DEX allowlists
- Creates valid test data
149-158: LGTM! Deadline validation test is correct.The test properly validates that expired deadlines are rejected with the correct error selector.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
140-151: Use a specific revert selector and follow failure test naming convention
- Replace the generic vm.expectRevert() with the specific InvalidContract selector per our guidelines.
- Rename the test to start with testRevert_ to comply with our test naming convention for failure cases.
Apply within this hunk:
-function test_EcoFacet_RevertWhenIntentSourceNotProvided() public { +function testRevert_EcoFacet_WhenIntentSourceNotProvided() public { vm.startPrank(USER_SENDER); // approval usdc.approve(_facetTestContractAddress, bridgeData.minAmount); EcoFacet.EcoData memory invalidData = validEcoData; invalidData.intentSource = address(0); - vm.expectRevert(); + vm.expectRevert(abi.encodeWithSelector(InvalidContract.selector)); ecoFacet.startBridgeTokensViaEco(bridgeData, invalidData); vm.stopPrank(); }Additionally add the missing import at the top of the file:
import { InvalidContract } from "../../../src/Errors/GenericErrors.sol";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
test/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
test/solidity/Facets/EcoFacet.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit Inference Engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (9)
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
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
🔇 Additional comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
68-81: LGTM: Facet registration into the diamond is correctProperly collecting function selectors and re-binding the facet handle to the diamond address is aligned with our diamond test pattern. This ensures subsequent calls go through the diamond.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Facets/EcoFacet.sol (1)
189-195: Unused return value from IEco.fund() callThe
fundfunction returns anintentHashvalue that is not captured or used. While this doesn't affect functionality, capturing the return value would enable better traceability and could be useful for debugging or future enhancements.Consider capturing the return value:
// Fund the intent (publish is optional for solver discovery) - IEco(_ecoData.portal).fund{ value: nativeValue }( + bytes32 intentHash = IEco(_ecoData.portal).fund{ value: nativeValue }( uint64(_bridgeData.destinationChainId), routeHash, reward, _ecoData.allowPartial );test/solidity/Facets/EcoFacet.t.sol (2)
195-215: Test doesn't verify that custom prover is actually usedThe test sets a custom prover but doesn't verify that it's actually used in the fund call. The test only checks event emission.
Add verification that the custom prover was used:
function test_CanBridgeNativeTokensWithCustomProver() public { EcoFacet.EcoData memory ecoData = getDefaultEcoData(); ecoData.prover = address(0x999); vm.label(ecoData.prover, "CustomProver"); // Set up for native token bridging bridgeData.sendingAssetId = address(0); bridgeData.minAmount = 1 ether; vm.expectEmit(true, true, true, true, _facetTestContractAddress); emit LiFiTransferStarted(bridgeData); + + // Expect the mock portal to receive the fund call with the custom prover in reward + vm.expectEmit(true, true, true, true, address(mockPortal)); + emit MockPortal.IntentFunded( + uint64(bridgeData.destinationChainId), + keccak256(abi.encode(/* expected route */)), + IEco.Reward({ + deadline: ecoData.rewardDeadline, + creator: USER_SENDER, + prover: address(0x999), // Custom prover + nativeAmount: 0, + tokens: new IEco.TokenAmount[](0) + }), + ecoData.allowPartial, + bridgeData.minAmount + ); vm.startPrank(USER_SENDER); ecoFacet.startBridgeTokensViaEco{ value: bridgeData.minAmount }( bridgeData, ecoData ); vm.stopPrank(); }
312-333: Test name doesn't accurately reflect what's being testedThe test is named
test_UsesDefaultProverWhenNotProvidedbut it only verifies that theDEFAULT_PROVERgetter returns the expected value, not that it's actually used in the fund call.Either rename the test to reflect what it actually tests or add verification that the default prover is used:
-function test_UsesDefaultProverWhenNotProvided() public { +function test_DefaultProverGetterReturnsCorrectValue() public { EcoFacet.EcoData memory ecoData = getDefaultEcoData(); ecoData.prover = address(0); // Set up for native token bridging bridgeData.sendingAssetId = address(0); bridgeData.minAmount = 1 ether; vm.expectEmit(true, true, true, true, _facetTestContractAddress); emit LiFiTransferStarted(bridgeData); vm.startPrank(USER_SENDER); ecoFacet.startBridgeTokensViaEco{ value: bridgeData.minAmount }( bridgeData, ecoData ); vm.stopPrank(); assertEq(ecoFacet.DEFAULT_PROVER(), defaultProver); }Or add proper verification:
function test_UsesDefaultProverWhenNotProvided() public { EcoFacet.EcoData memory ecoData = getDefaultEcoData(); ecoData.prover = address(0); // Set up for native token bridging bridgeData.sendingAssetId = address(0); bridgeData.minAmount = 1 ether; vm.expectEmit(true, true, true, true, _facetTestContractAddress); emit LiFiTransferStarted(bridgeData); + + // Verify that DEFAULT_PROVER is used in the fund call + vm.expectEmit(true, true, true, true, address(mockPortal)); + emit MockPortal.IntentFunded( + uint64(bridgeData.destinationChainId), + keccak256(abi.encode(/* expected route */)), + IEco.Reward({ + deadline: ecoData.rewardDeadline, + creator: USER_SENDER, + prover: defaultProver, // Should use DEFAULT_PROVER + nativeAmount: 0, + tokens: new IEco.TokenAmount[](0) + }), + ecoData.allowPartial, + bridgeData.minAmount + ); vm.startPrank(USER_SENDER); ecoFacet.startBridgeTokensViaEco{ value: bridgeData.minAmount }( bridgeData, ecoData ); vm.stopPrank(); assertEq(ecoFacet.DEFAULT_PROVER(), defaultProver); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Facets/EcoFacet.sol(1 hunks)src/Interfaces/IEco.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
src/Interfaces/IEco.solsrc/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
src/Interfaces/IEco.solsrc/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
src/Interfaces/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
src/Interfaces/*.sol: All interfaces must start withIprefix (e.g.,ILiFi,IStargate).
All interfaces must be placed in separate files in thesrc/Interfacesdirectory.
Every interface MUST have the following NatSpec tags in this order: @title, @notice, @author, @Custom:version.
Files:
src/Interfaces/IEco.sol
src/Interfaces/I*.sol
📄 CodeRabbit Inference Engine (conventions.md)
All interfaces must be placed in separate files in the
src/Interfacesdirectory and start withI.
Files:
src/Interfaces/IEco.sol
src/Facets/*Facet.sol
📄 CodeRabbit Inference Engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
test/solidity/**/*.t.sol
📄 CodeRabbit Inference Engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (12)
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
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: 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-28T11:29:09.566Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EcoFacet.sol
[failure] 130-130:
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
[warning] 190-190:
Calling a function without checking the return value may lead to silent failures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unused-return-function-call
🔇 Additional comments (3)
src/Interfaces/IEco.sol (1)
1-117: LGTM!The interface is well-structured with comprehensive NatSpec documentation, properly defined structs, and clear function signatures that align with the cross-chain intent system architecture.
src/Facets/EcoFacet.sol (1)
151-151: No replay vulnerability — routeHash is the route component; intent uniqueness & replay protection are enforced elsewhereShort: EcoFacet deliberately computes routeHash = keccak256(abi.encode(route)) as the route component and passes (destination, routeHash, reward) to the portal. The intent identity and replay protection are provided by the portal/prover layer (intentHash is built from destination + routeHash + reward; reward.creator is set to msg.sender in EcoFacet), and the protocol tracks proven/fulfilled intents to prevent replays.
Relevant locations:
- src/Facets/EcoFacet.sol — computes routeHash (bytes32 routeHash = keccak256(abi.encode(route))); builds Reward with creator = msg.sender; calls IEco(...).fund(...)
- src/Interfaces/IEco.sol — Route/Reward/Intent structs and fund/publish signatures (intent includes destination, routeHash, reward)
- test/solidity/Facets/EcoFacet.t.sol — MockPortal.publish / fund compute intentHash using destination + keccak(route) + keccak(reward) and emit IntentFunded (demonstrates expected behaviour)
Conclusion: no code change required in this facet — replay protection is provided by the Eco contracts and the reward/destination combination used when funding intents.
test/solidity/Facets/EcoFacet.t.sol (1)
9-9: Fix incorrect import statementThe import statement incorrectly imports
InvalidProverandInvalidDeadlinefromGenericErrors.sol, but these errors are defined inEcoFacet.solaccording to the implementation.Apply this diff to fix the import:
-import { InvalidContract, InvalidProver, InvalidDeadline } from "../../../src/Errors/GenericErrors.sol"; +import { InvalidContract } from "../../../src/Errors/GenericErrors.sol";⛔ Skipped due to learnings
Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-07-28T07:54:14.853Z Learning: Applies to src/Errors/GenericErrors.sol : All custom errors must be defined in `src/Errors/GenericErrors.sol`.Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-07-28T07:54:14.853Z Learning: Applies to src/Errors/GenericErrors.sol : All custom errors should be defined in `src/Errors/GenericErrors.sol` and not include error messages.Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-07-28T07:54:14.853Z Learning: Applies to src/Errors/GenericErrors.sol : Errors should not include error messages (for gas optimization).Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-07-28T07:54:14.853Z Learning: Applies to src/Errors/GenericErrors.sol : Error names should be descriptive and follow PascalCase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Facets/EcoFacet.sol (2)
130-174: Signature replay vulnerability flagged by static analysis needs investigationThe static analysis tool has flagged a potential signature replay attack vulnerability at line 130. However, upon reviewing the code, this appears to be a false positive since:
- The function doesn't handle signatures directly
- The route hash is computed from the entire route struct including salt and deadline
- The salt parameter in EcoData provides replay protection
- The deadline parameters provide time-bound protection
Let me verify the signature handling in the broader codebase:
#!/bin/bash # Description: Search for signature-related functionality in the Eco integration # to understand if there are any actual signature replay vulnerabilities echo "=== Searching for signature handling in Eco-related files ===" rg -n -A5 -B5 'signature|ecrecover|recover' --type=sol src/Interfaces/IEco.sol src/Facets/EcoFacet.sol || echo "No signature handling found" echo -e "\n=== Checking for nonce or replay protection mechanisms ===" rg -n -A3 -B3 'nonce|salt|replay|deadline' --type=sol src/Interfaces/IEco.sol src/Facets/EcoFacet.sol echo -e "\n=== Examining the route hash computation ===" rg -n -A10 'keccak256.*encode' --type=sol src/Facets/EcoFacet.sol
196-201: Unchecked return value from external callThe call to
IEco(_ecoData.portal).fund()doesn't check the return value, which could lead to silent failures as flagged by static analysis.Based on the retrieved learnings, when public entry point functions have
nonReentrantmodifier protection, the event emission order after external calls is acceptable from a security perspective. However, we should still consider checking the return value for better error handling:- // Fund the intent (publish is optional for solver discovery) - IEco(_ecoData.portal).fund{ value: nativeValue }( - uint64(_bridgeData.destinationChainId), - routeHash, - reward, - _ecoData.allowPartial - ); + // Fund the intent (publish is optional for solver discovery) + bytes32 intentId = IEco(_ecoData.portal).fund{ value: nativeValue }( + uint64(_bridgeData.destinationChainId), + routeHash, + reward, + _ecoData.allowPartial + ); + + // Optional: emit intentId for tracking purposes + // emit IntentCreated(intentId, _bridgeData.transactionId);
🧹 Nitpick comments (1)
test/solidity/Facets/EcoFacet.t.sol (1)
252-255: Consider adding swap test with mock setupWhile the swap test is appropriately commented out due to Optimism DEX complexity, consider adding a simplified version using mocks to test the swap functionality without requiring complex DEX setup.
You could add a simple mock-based swap test:
function test_CanSwapAndBridgeWithMockDex() public { // Create mock swap data that will succeed LibSwap.SwapData[] memory mockSwapData = new LibSwap.SwapData[](1); mockSwapData[0] = LibSwap.SwapData({ callTo: address(this), // Use test contract as mock DEX approveTo: address(this), sendingAssetId: ADDRESS_USDC, receivingAssetId: ADDRESS_USDC, // Same asset for simplicity fromAmount: 100 * 10**6, callData: "", // Empty call data requiresDeposit: true }); // Setup test with mock data EcoFacet.EcoData memory ecoData = getDefaultEcoData(); // ... rest of test implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Facets/EcoFacet.sol(1 hunks)src/Interfaces/IEco.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Interfaces/IEco.sol
🧰 Additional context used
📓 Path-based instructions (4)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
test/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
**/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
test/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
test/solidity/**/*.t.sol
📄 CodeRabbit Inference Engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
src/Facets/*Facet.sol
📄 CodeRabbit Inference Engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
🧠 Learnings (13)
📓 Common learnings
Learnt from: mirooon
PR: lifinance/contracts#945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
📚 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
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-01-28T11:29:09.566Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Facets/EcoFacet.sol
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EcoFacet.sol
[failure] 130-130:
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
[warning] 196-196:
Calling a function without checking the return value may lead to silent failures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unused-return-function-call
🔇 Additional comments (24)
src/Facets/EcoFacet.sol (10)
1-17: LGTM - Proper license and importsThe contract follows the required LGPL-3.0 license and has proper imports for the EcoFacet functionality.
18-22: LGTM - Contract structure follows LiFi patternsThe contract properly inherits from required base contracts and follows the established LiFi facet architecture with immutable default prover setup.
35-44: LGTM - Well-documented EcoData structThe struct is properly documented with comprehensive NatSpec comments explaining each parameter's purpose and usage.
48-49: Custom errors properly definedThe custom errors
InvalidProverandInvalidDeadlinefollow the coding guidelines for using custom error types instead of generic reverts.
53-62: LGTM - Comprehensive validation in modifierThe
validateEcoDatamodifier properly validates both the portal address and deadline constraints with appropriate custom errors.
68-71: LGTM - Constructor validationThe constructor properly validates the default prover address and uses the custom
InvalidProvererror as required.
78-96: LGTM - Entry point follows LiFi patternsThe
startBridgeTokensViaEcofunction properly implements the LiFi facet pattern with appropriate modifiers for reentrancy protection, validation, and asset handling.
102-123: LGTM - Swap and bridge entry pointThe
swapAndStartBridgeTokensViaEcofunction correctly implements the swap-then-bridge pattern with proper validation modifiers and follows established LiFi conventions.
175-183: LGTM - Proper token approval handlingThe code correctly handles token approvals only for non-native assets and uses the established
LibAsset.maxApproveERC20pattern.
192-194: LGTM - Event emission follows established patternThe
LiFiTransferStartedevent is properly emitted before the external call, following the established LiFi pattern and retrieved learnings about event emission order in protected functions.test/solidity/Facets/EcoFacet.t.sol (14)
1-11: LGTM - Proper test file setupThe test file follows the required conventions with proper license, imports, and error imports from the correct locations.
13-23: LGTM - Test wrapper contractThe
TestEcoFacetwrapper properly extends the main facet and exposes necessary functions for testing setup.
35-96: LGTM - Comprehensive test setupThe setUp function properly configures the test environment with:
- Fork configuration for Optimism
- Facet deployment and diamond integration
- DEX and function selector approvals
- Proper labeling for debugging
99-126: LGTM - Override functions for Optimism forkThe override functions properly handle the complexity of swap data setup on Optimism fork, providing reasonable alternatives for testing.
Also applies to: 130-146
148-168: LGTM - Helper function for test dataThe
getDefaultEcoDatahelper function creates properly structured test data with reasonable defaults.
170-197: LGTM - Override hooks follow base class patternThe override functions properly implement the required interface for base class test execution with native and non-native token handling.
200-206: LGTM - Appropriate test skippingThe overridden swap tests are appropriately skipped with clear comments explaining the reason (Optimism DEX setup complexity).
210-230: LGTM - Native token bridging testThe test properly verifies native token bridging with a custom prover and follows the established testing patterns with proper event emission verification.
232-250: LGTM - Default prover testThe test correctly verifies bridging with the default prover when no custom prover is provided, including proper token setup and approval.
257-271: LGTM - Zero portal validation testThe test properly verifies the
InvalidContractrevert when portal address is zero, following the testRevert_ naming convention.
273-287: LGTM - Deadline validation testsBoth deadline validation tests properly verify the
InvalidDeadlineerror for expired route and reward deadlines, following the correct naming convention.Also applies to: 289-303
305-309: LGTM - Constructor validation testThe test properly verifies that the constructor reverts with
InvalidProverwhen given a zero address for the default prover.
311-332: LGTM - Default prover usage verificationThe test correctly verifies that the default prover is used when none is provided and includes an assertion to confirm the DEFAULT_PROVER value.
334-359: LGTM - Calls data testThe test properly verifies that the facet can handle EcoData with calls array, demonstrating the full functionality of the bridge system.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/solidity/Facets/EcoFacet.t.sol (2)
214-234: Strengthen: assert the custom prover was actually usedAs in prior reviews, assert behavioral outcomes, not just event emission. This is easiest with a mock portal that records the prover used.
Option A (preferred): Use a minimal mock IEco portal in this test file that records
proveron fund() and expose a getter/event. Then add:ecoFacet.startBridgeTokensViaEco{ value: bridgeData.minAmount }( bridgeData, ecoData ); vm.stopPrank(); + + // Assert the provided custom prover was used + assertEq(mockPortal.lastProver(), ecoData.prover, "should use provided prover");Option B: If you must stick to an Optimism fork, add an event in EcoFacet emitted with the effective prover and assert it here via
vm.expectEmit.
315-336: Strengthen: assert default prover pathMirror the custom-prover assertion to verify fallback to DEFAULT_PROVER at execution time (not only the getter).
ecoFacet.startBridgeTokensViaEco{ value: bridgeData.minAmount }( bridgeData, ecoData ); vm.stopPrank(); - assertEq(ecoFacet.DEFAULT_PROVER(), OPTIMISM_DEFAULT_PROVER); + assertEq(ecoFacet.DEFAULT_PROVER(), OPTIMISM_DEFAULT_PROVER); + // If using a mock portal: + // assertEq(mockPortal.lastProver(), OPTIMISM_DEFAULT_PROVER, "should use default prover");
🧹 Nitpick comments (11)
docs/EcoFacet.md (2)
60-66: Fix parameter type and clarify wording for swap input
- The function takes an array of swaps, not a single swap.
- Tighten grammar.
-Some methods accept a `SwapData _swapData` parameter. +Some methods accept a `LibSwap.SwapData[] calldata _swapData` parameter. @@ -Swapping is performed by a swap specific library that expects an array of calldata to can be run on various DEXs (i.e. Uniswap) to make one or multiple swaps before performing another action. +Swapping is performed by a swap-specific library that expects an array of calldata that can be executed on various DEXs (e.g., Uniswap) to make one or multiple swaps before performing another action.
28-33: Document deadline semantics to match revert behaviorTests expect InvalidDeadline when deadline <= block.timestamp. Suggest clarifying this in the docs to help integrators avoid off-by-one issues.
/// @param routeDeadline The deadline for route execution (timestamp) -/// @param rewardDeadline The deadline for claiming rewards (timestamp) +/// @param rewardDeadline The deadline for claiming rewards (timestamp) +/// Note: Both deadlines must be strictly greater than the current block timestamp +/// at the time of execution (deadline > block.timestamp). @@ -This parameter is strictly for analytics purposes. It's used to emit events that we can later track and index in our subgraphs and provide data on how our contracts are being used. `BridgeData` and the events we can emit can be found [here](../src/Interfaces/ILiFi.sol). +This parameter is strictly for analytics purposes. It's used to emit events that we can later track and index in our subgraphs and provide data on how our contracts are being used. `BridgeData` and the events we can emit can be found [here](../src/Interfaces/ILiFi.sol). Ensure your deadlines are set safely in the future to avoid reverts.Also applies to: 66-71
src/Interfaces/IEco.sol (2)
11-14: Add per-field NatSpec on TokenAmount for consistencyMinor doc polish to match the level of detail given in Call.
struct TokenAmount { - address token; - uint256 amount; + /// @notice ERC20 token address (use address(0) for native, if applicable) + address token; + /// @notice Amount of the token + uint256 amount; }
33-39: Add per-field NatSpec on Reward fieldsImproves generated docs and consumer clarity.
struct Reward { - uint64 deadline; - address creator; - address prover; - uint256 nativeAmount; - TokenAmount[] tokens; + /// @notice Reward claim deadline on the destination chain + uint64 deadline; + /// @notice Address that created the intent (credited as creator) + address creator; + /// @notice Address authorized to prove the route + address prover; + /// @notice Amount of native token escrowed as incentive + uint256 nativeAmount; + /// @notice Additional ERC20 token incentives + TokenAmount[] tokens; }test/solidity/Facets/EcoFacet.t.sol (7)
5-11: Import EcoFacet errors instead of re-declaring them laterAvoids drift if the contract error names change and keeps selectors authoritative.
import { EcoFacet } from "../../../src/Facets/EcoFacet.sol"; import { IEco } from "../../../src/Interfaces/IEco.sol"; import { InvalidContract } from "../../../src/Errors/GenericErrors.sol"; +import { InvalidProver, InvalidDeadline } from "../../../src/Facets/EcoFacet.sol";
12-15: Remove locally re-declared errors (use imported ones)Locally shadowing custom errors invites selector drift and maintenance issues.
-// Errors from EcoFacet -error InvalidProver(); -error InvalidDeadline(); +// Errors from EcoFacet are imported above
31-35: Replace hardcoded DEFAULT_PROVER with config-driven valueThe TODO indicates this should be synced with deployment config. Load from config/eco.json to ensure tests track deploys.
- address internal constant OPTIMISM_DEFAULT_PROVER = - 0x1234567890123456789012345678901234567890; // TODO: Replace with actual prover address from Eco Protocol + address internal OPTIMISM_DEFAULT_PROVER;Then in setUp (see next comment) parse config. Confirm JSON shape (e.g.,
.optimism.defaultProver) before applying.
39-48: Initialize DEFAULT_PROVER from config/eco.jsonKeeps tests aligned with deployment parameters.
function setUp() public { // Setup Optimism fork at block 139757044 customBlockNumberForForking = 139757044; customRpcUrlForForking = "ETH_NODE_URI_OPTIMISM"; initTestBase(); - // Deploy contracts with actual prover address - ecoFacet = new TestEcoFacet(OPTIMISM_DEFAULT_PROVER); + // Load default prover from config + // Requires: import "forge-std/StdJson.sol"; and `using stdJson for string;` at top + string memory ecoCfg = vm.readFile("config/eco.json"); + OPTIMISM_DEFAULT_PROVER = vm.parseJsonAddress(ecoCfg, ".optimism.defaultProver"); + + // Deploy contracts with actual prover address + ecoFacet = new TestEcoFacet(OPTIMISM_DEFAULT_PROVER); portal = IEco(OPTIMISM_PORTAL);If stdJson is not already used in this test suite, add:
import "forge-std/StdJson.sol"; using stdJson for string;Please verify the JSON path matches config/eco.json.
36-38: Remove unused state variableportal
portalis assigned but never read in this file.- IEco internal portal;
64-70: Allowlist clarity: avoid labeling portal as a DEXNit:
addDex(OPTIMISM_PORTAL)conceptually allows an external contract, not a DEX. Consider renaming helper in TestEcoFacet to a more neutral name (e.g., addAllowedContract) for readability, or add a comment clarifying its purpose.
133-150: Stubbed swap data with empty calldata can mask issuesThis keeps array access safe, but it silently ensures no swap occurs. Consider gating tests that depend on swaps behind feature flags or using a dedicated mock that always succeeds to keep swap-and-bridge flows covered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/EcoFacet.md(1 hunks)src/Interfaces/IEco.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit inference engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
src/Interfaces/IEco.soltest/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
src/Interfaces/IEco.soltest/solidity/Facets/EcoFacet.t.sol
src/Interfaces/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/*.sol: All interfaces must start withIprefix (e.g.,ILiFi,IStargate).
All interfaces must be placed in separate files in thesrc/Interfacesdirectory.
Every interface MUST have the following NatSpec tags in this order: @title, @notice, @author, @Custom:version.
Files:
src/Interfaces/IEco.sol
src/Interfaces/I*.sol
📄 CodeRabbit inference engine (conventions.md)
All interfaces must be placed in separate files in the
src/Interfacesdirectory and start withI.
Files:
src/Interfaces/IEco.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (12)
📓 Common learnings
Learnt from: mirooon
PR: lifinance/contracts#945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
docs/EcoFacet.md
📚 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
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-01-28T11:29:09.566Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
🔇 Additional comments (1)
src/Interfaces/IEco.sol (1)
1-53: Interface looks correct and policy-compliant
- SPDX is LGPL-3.0-only (matches policy).
- NatSpec present with required tags and order.
- Interface is in src/Interfaces and prefixed with I.
- Public function has complete NatSpec.
ezynda3
force-pushed
the
LF-14918-Eco-Facet
branch
from
9f6d871 to
38e383f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
script/deploy/facets/DeployEcoFacet.s.sol (1)
20-21: Double-encoding issue resolved — good fixPassing only creationCode relies on DeployScriptBase to append constructor args once. Matches repo pattern.
🧹 Nitpick comments (6)
script/deploy/facets/UpdateEcoFacet.s.sol (1)
7-14: Missing NatSpec on contract and run()Add minimal NatSpec to satisfy repo rules and solhint.
-contract DeployScript is UpdateScriptBase { +/// @title UpdateEcoFacet Script +/// @author LI.FI (https://li.fi) +/// @notice Updates EcoFacet on existing diamonds +/// @custom:version 1.0.0 +contract DeployScript is UpdateScriptBase { - function run() + /// @notice Executes the EcoFacet update cut. + /// @return facets The facet addresses involved in the cut. + /// @return cutData Encoded diamond cut data. + function run() public returns (address[] memory facets, bytes memory cutData) { return update("EcoFacet"); } }script/deploy/facets/DeployEcoFacet.s.sol (3)
1-1: License header for deploy scripts — align with team conventionSwitch to UNLICENSED to match deploy script practice.
-// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-License-Identifier: UNLICENSED
9-21: Add NatSpec and consider explicit return for clarityDocs are required; explicit return is optional but clearer.
-contract DeployScript is DeployScriptBase { +/// @title DeployEcoFacet Script +/// @author LI.FI (https://li.fi) +/// @notice Deploys EcoFacet using config/eco.json +/// @custom:version 1.0.0 +contract DeployScript is DeployScriptBase { @@ - function run() + /// @notice Deploy EcoFacet. + /// @return deployed The deployed EcoFacet instance. + /// @return constructorArgs ABI-encoded constructor args used. + function run() public returns (EcoFacet deployed, bytes memory constructorArgs) { constructorArgs = getConstructorArgs(); deployed = EcoFacet(deploy(type(EcoFacet).creationCode)); + return (deployed, constructorArgs); }
23-33: NatSpec missing on getConstructorArgs()Add brief docs.
- function getConstructorArgs() internal override returns (bytes memory) { + /// @notice Builds constructor args from config/eco.json. + /// @return ABI-encoded IEcoPortal address. + function getConstructorArgs() internal override returns (bytes memory) {src/Facets/EcoFacet.sol (2)
142-167: Add validation for destination portal address.Prevent malformed intents by rejecting zero destinationPortal.
) private pure { @@ if ( _bridgeData.receiver == NON_EVM_ADDRESS && _ecoData.nonEVMReceiver.length == 0 ) { revert InvalidReceiver(); } + if (_ecoData.destinationPortal == address(0)) { + revert InvalidConfig(); + }
253-276: Return value intentionally unused; document to appease scanners.publishAndFund likely returns an intent id. If we’re not using it, add a short comment to clarify intent and silence “unused return” warnings.
- intentSource.publishAndFund{ + // Intentionally ignore returned intent id; tracking is handled off-chain/by portal + intentSource.publishAndFund{ value: _bridgeData.minAmount + _ecoData.solverReward }(intent, false); @@ - intentSource.publishAndFund{ value: _ecoData.solverReward }( + // Intentionally ignore returned intent id + intentSource.publishAndFund{ value: _ecoData.solverReward }( intent, false );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
config/dexs.json(12 hunks)config/eco.json(1 hunks)docs/EcoFacet.md(1 hunks)script/deploy/facets/DeployEcoFacet.s.sol(1 hunks)script/deploy/facets/UpdateEcoFacet.s.sol(1 hunks)src/Facets/EcoFacet.sol(1 hunks)src/Interfaces/IEcoPortal.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/EcoFacet.md
🧰 Additional context used
📓 Path-based instructions (7)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit inference engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
script/deploy/facets/DeployEcoFacet.s.solsrc/Interfaces/IEcoPortal.solscript/deploy/facets/UpdateEcoFacet.s.soltest/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
script/deploy/facets/DeployEcoFacet.s.solsrc/Interfaces/IEcoPortal.solscript/deploy/facets/UpdateEcoFacet.s.soltest/solidity/Facets/EcoFacet.t.solsrc/Facets/EcoFacet.sol
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}
📄 CodeRabbit inference engine (conventions.md)
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}: Deployment scripts must reside inscript/deploy/facets/for base deployments andscript/deploy/zksync/for ZKSync-specific scripts.
Deployment scripts must inheritDeployScriptBaseand use JSON config withstdJson.
Update scripts must inheritUpdateScriptBaseand useupdate("{ContractName}").
Files:
script/deploy/facets/DeployEcoFacet.s.solscript/deploy/facets/UpdateEcoFacet.s.sol
src/Interfaces/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/*.sol: All interfaces must start withIprefix (e.g.,ILiFi,IStargate).
All interfaces must be placed in separate files in thesrc/Interfacesdirectory.
Every interface MUST have the following NatSpec tags in this order: @title, @notice, @author, @Custom:version.
Files:
src/Interfaces/IEcoPortal.sol
src/Interfaces/I*.sol
📄 CodeRabbit inference engine (conventions.md)
All interfaces must be placed in separate files in the
src/Interfacesdirectory and start withI.
Files:
src/Interfaces/IEcoPortal.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
src/Facets/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
🧠 Learnings (33)
📓 Common learnings
Learnt from: mirooon
PR: lifinance/contracts#945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
📚 Learning: 2025-08-26T15:19:07.775Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.775Z
Learning: DeployScriptBase.sol is located at script/deploy/facets/utils/DeployScriptBase.sol, not script/deploy/utils/DeployScriptBase.sol. Import paths from script/deploy/facets/LDA/ should use "../utils/DeployScriptBase.sol" to reference it correctly.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.solscript/deploy/facets/UpdateEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must reside in `script/deploy/facets/` for base deployments and `script/deploy/zksync/` for ZKSync-specific scripts.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.solscript/deploy/facets/UpdateEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must inherit `DeployScriptBase` and use JSON config with `stdJson`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.solscript/deploy/facets/UpdateEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Update scripts must inherit `UpdateScriptBase` and use `update("{ContractName}")`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.solscript/deploy/facets/UpdateEcoFacet.s.sol
📚 Learning: 2024-09-23T01:42:03.075Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipFacet.s.sol:22-35
Timestamp: 2024-09-23T01:42:03.075Z
Learning: In deployment scripts like `DeployGasZipFacet.s.sol`, do not report issues suggesting adding error handling for missing configuration files or unexpected JSON structures, as the script will fail if the file is missing.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-01-28T14:27:50.689Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-06-13T08:30:28.665Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/facets/DeployPioneerFacet.s.sol:17-27
Timestamp: 2025-06-13T08:30:28.665Z
Learning: Deployment scripts rely on external automation (pre-script checks) to validate constructor arguments such as non-zero addresses, so adding explicit `require` guards inside the Foundry deployment scripts is generally unnecessary.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-10-04T09:21:59.708Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-04T09:21:59.708Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 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:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-11-25T13:49:40.464Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 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:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-10-14T00:50:53.603Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/facets/DeployPermit2Proxy.s.sol:17-20
Timestamp: 2024-10-14T00:50:53.603Z
Learning: In `DeployScriptBase`, `getConstructorArgs()` is called and constructor arguments are handled, so child deploy scripts do not need to pass constructor arguments explicitly to the `deploy` function.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-23T08:12:29.064Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/deploy/facets/UpdateWhitelistManagerFacet.s.sol:1-1
Timestamp: 2025-07-23T08:12:29.064Z
Learning: Deployment scripts in script/deploy/ should not use the LGPL-3.0 license identifier, contrary to other Solidity files. Scripts can use UNLICENSED or other appropriate license identifiers.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
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: 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
PR: lifinance/contracts#1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-31T09:10:16.115Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:0-0
Timestamp: 2024-10-31T09:10:16.115Z
Learning: In the `signData` function within `RelayFacetTest` in `test/solidity/Facets/RelayFacet.t.sol`, the use of the EIP-712 standard for hashing and signing is not required.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-11-21T08:39:29.530Z
Learnt from: ezynda3
PR: lifinance/contracts#861
File: config/dexs.json:748-752
Timestamp: 2024-11-21T08:39:29.530Z
Learning: In the 'worldchain' network, the addresses `0x50D5a8aCFAe13Dceb217E9a071F6c6Bd5bDB4155`, `0x8f023b4193a6b18C227B4a755f8e28B3D30Ef9a1`, and `0x603a538477d44064eA5A5d8C345b4Ff6fca1142a` are used as DEXs and should be included in `config/dexs.json`.
Applied to files:
config/dexs.json
📚 Learning: 2024-11-04T02:25:07.478Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#846
File: config/dexs.json:296-300
Timestamp: 2024-11-04T02:25:07.478Z
Learning: In `config/dexs.json`, it's expected that some addresses appear multiple times across different networks.
Applied to files:
config/dexs.json
📚 Learning: 2024-10-21T01:27:47.808Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#836
File: config/dexs.json:396-397
Timestamp: 2024-10-21T01:27:47.808Z
Learning: In `config/dexs.json`, it's acceptable for a whitelisted DEX address to be the same across multiple networks.
Applied to files:
config/dexs.json
📚 Learning: 2025-06-11T08:55:44.396Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1201
File: config/dexs.json:536-537
Timestamp: 2025-06-11T08:55:44.396Z
Learning: In lifinance/contracts, the addresses stored in config/dexs.json are handled case-insensitively, so they do not need to be written in EIP-55 checksummed format.
Applied to files:
config/dexs.json
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
src/Facets/EcoFacet.sol
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EcoFacet.sol
[notice] 114-114:
Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events
[warning] 261-261:
Calling a function without checking the return value may lead to silent failures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unused-return-function-call
[warning] 271-271:
Calling a function without checking the return value may lead to silent failures. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unused-return-function-call
🔇 Additional comments (7)
script/deploy/facets/UpdateEcoFacet.s.sol (1)
1-1: License header for deploy scripts — align with team conventionOur deploy/update scripts use UNLICENSED (per team learnings), not LGPL.
[ suggest_nitpick ]
-// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-License-Identifier: UNLICENSEDtest/solidity/Facets/EcoFacet.t.sol (3)
1-3: License and pragma look good.LGPL-3.0 identifier and Solidity pragma present per guidelines.
30-79: Solid test harness wiring.Fork setup, liquidity seeding, facet attachment, and allowlist configuration align with our testing conventions.
138-141: Good negative test for zero portal config.Accurately expects InvalidConfig on constructor with address(0).
src/Facets/EcoFacet.sol (3)
278-291: Event emission after external call is acceptable here.Protected by nonReentrant in the public entrypoints; ordering is fine per prior practice. Referencing our earlier learnings.
1-20: NatSpec and licensing comply.LGPL-3.0 license, required NatSpec tags (title/author/notice/version), and facet placement look correct.
3-3: Confirm compiler version matches foundry.toml.Ensure 0.8.17 matches the repo’s configured Solidity/EVM version.
Would you like a quick script to parse foundry.toml and check pragma consistency?
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
script/deploy/facets/DeployEcoFacet.s.sol (1)
20-21: Fix deployment call to follow DeployScriptBase patterns.Based on past review comments,
DeployScriptBase.deployalready handles concatenating constructor arguments with creation code. Passing pre-concatenated code causes double-encoding of constructor arguments.Apply this diff:
- deployed = EcoFacet(deploy(type(EcoFacet).creationCode)); + deployed = EcoFacet(deploy(type(EcoFacet).creationCode));The
getConstructorArgs()override already handles returningabi.encode(portal), so the deploy call only needs the creation code.src/Facets/EcoFacet.sol (2)
208-210: Fix route tokens logic to use receiving asset type.The current implementation uses
sendingAssetIdto determine if route tokens are needed, but should usereceivingAssetIdsince route tokens represent what gets delivered on the destination chain.Apply this diff:
- if (LibAsset.isNativeAsset(_bridgeData.sendingAssetId)) { + if (LibAsset.isNativeAsset(_ecoData.receivingAssetId)) { return new IEcoPortal.TokenAmount[](0); }
228-250: Fix native amount calculation logic and remove double-counting.The intent construction has two issues:
- Uses sending asset to determine native amounts but should use receiving asset
- Double-counts
minAmountin the reward, causing underfundingApply this diff:
- bool isNative = LibAsset.isNativeAsset(_bridgeData.sendingAssetId); + bool receiveNative = LibAsset.isNativeAsset(_ecoData.receivingAssetId); return IEcoPortal.Intent({ destination: uint64(_bridgeData.destinationChainId), route: IEcoPortal.Route({ salt: _ecoData.salt, deadline: _ecoData.routeDeadline, portal: _ecoData.destinationPortal, - nativeAmount: isNative ? _bridgeData.minAmount : 0, + nativeAmount: receiveNative ? _bridgeData.minAmount : 0, tokens: routeTokens, calls: routeCalls }), reward: IEcoPortal.Reward({ deadline: _ecoData.rewardDeadline, creator: msg.sender, prover: _ecoData.prover, - nativeAmount: isNative - ? _ecoData.solverReward + _bridgeData.minAmount - : _ecoData.solverReward, + nativeAmount: _ecoData.solverReward, tokens: new IEcoPortal.TokenAmount[](0) }) });
🧹 Nitpick comments (5)
deployments/_deployments_log_file.json (1)
39136-39141: Timestamp/verification housekeeping (non-blocking).If verification completes, remember to flip VERIFIED to "true". Ensure timestamp convention (UTC vs local) matches prior entries before merging.
script/demoScripts/demoEco.ts (4)
27-30: Avoid hardcoding portal/prover; load from config by chain/environment.Hardcoded addresses risk drift across staging/prod. Prefer reading from config/eco.json (and/or env) keyed by src/dst chain.
If helpful, I can wire this to getEnvVar() and config/eco.json in a follow-up.
42-44: Use consola for consistent logging.Replace console.info/log/error with consola.info/log/error to align with script conventions.
- console.info(`Bridge 5 USDC from ${srcChain} --> Base`) - console.info(`Connected wallet address: ${signerAddress}`) + consola.info(`Bridge 5 USDC from ${srcChain} --> Base`) + consola.info(`Connected wallet address: ${signerAddress}`)
93-120: Simplify ERC20 wrappers by using viem getContract.You can instantiate an ERC20 contract once and reuse read/write calls; it reduces boilerplate and improves readability.
- const tokenContract = { - read: { allowance: async (args: [`0x${string}`, `0x${string}`]): Promise<bigint> => { - return (await publicClient.readContract({ address: SRC_TOKEN_ADDRESS, abi: erc20Abi, functionName: 'allowance', args })) as bigint - }}, - write: { approve: async (args: [`0x${string}`, bigint]): Promise<`0x${string}`> => { - return walletClient.writeContract({ address: SRC_TOKEN_ADDRESS, abi: erc20Abi, functionName: 'approve', args } as any) - }}, - } + const tokenContract = getContract({ + address: SRC_TOKEN_ADDRESS, + abi: erc20Abi, + client: { public: publicClient, wallet: walletClient }, + })This remains compatible with ensureAllowance’s read/write usage.
129-147: Standardize logging and confirmed executeTransaction flag semantics
- Verified that
executeTransactionis defined asSo the fourth boolean argument (export const executeTransaction = async <T>( transaction: () => Promise<T>, transactionDescription: string, publicClient?: any, validateReceipt = false ) => { … }true) correctly enables receipt validation—no change required there.- For consistency with the rest of the demo scripts (which use
consola.info,consola.error, etc.), consider:
- Importing
consolaindemoEco.ts.- Replacing the existing
console.log(…)calls around line 129 withconsola.info(…).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
config/eco.json(1 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(4 hunks)deployments/optimism.staging.json(1 hunks)script/demoScripts/demoEco.ts(1 hunks)script/deploy/facets/DeployEcoFacet.s.sol(1 hunks)src/Facets/EcoFacet.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/eco.json
🧰 Additional context used
📓 Path-based instructions (6)
script/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/**/*.ts: All scripts must follow the rules defined in.eslintrc.cjs.
Use async/await for asynchronous operations in TypeScript scripts.
Handle errors appropriately with try/catch blocks in TypeScript scripts.
Include proper logging for debugging and monitoring in TypeScript scripts.
Use environment variables for configuration in TypeScript scripts.
All scripts must usecittyfor CLI argument parsing.
Useconsolafor consistent logging across TypeScript scripts.
Environment variables should be validated usinggetEnvVar()helper in TypeScript scripts.
Scripts should exit with appropriate exit codes (0 for success, 1 for error) in TypeScript scripts.
Files:
script/demoScripts/demoEco.ts
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit inference engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
script/deploy/facets/DeployEcoFacet.s.solsrc/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
script/deploy/facets/DeployEcoFacet.s.solsrc/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}
📄 CodeRabbit inference engine (conventions.md)
{script/deploy/facets/*.sol,script/deploy/zksync/*.sol}: Deployment scripts must reside inscript/deploy/facets/for base deployments andscript/deploy/zksync/for ZKSync-specific scripts.
Deployment scripts must inheritDeployScriptBaseand use JSON config withstdJson.
Update scripts must inheritUpdateScriptBaseand useupdate("{ContractName}").
Files:
script/deploy/facets/DeployEcoFacet.s.sol
src/Facets/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (48)
📓 Common learnings
Learnt from: mirooon
PR: lifinance/contracts#945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
📚 Learning: 2024-10-09T03:47:21.269Z
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.
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-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-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-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.jsonscript/deploy/facets/DeployEcoFacet.s.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, 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-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.jsonscript/deploy/facets/DeployEcoFacet.s.soldeployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2024-11-25T09:04:55.880Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2024-11-25T06:25:01.232Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-26T15:19:07.775Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.775Z
Learning: DeployScriptBase.sol is located at script/deploy/facets/utils/DeployScriptBase.sol, not script/deploy/utils/DeployScriptBase.sol. Import paths from script/deploy/facets/LDA/ should use "../utils/DeployScriptBase.sol" to reference it correctly.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must reside in `script/deploy/facets/` for base deployments and `script/deploy/zksync/` for ZKSync-specific scripts.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Deployment scripts must inherit `DeployScriptBase` and use JSON config with `stdJson`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to {script/deploy/facets/*.sol,script/deploy/zksync/*.sol} : Update scripts must inherit `UpdateScriptBase` and use `update("{ContractName}")`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-09-23T01:42:03.075Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipFacet.s.sol:22-35
Timestamp: 2024-09-23T01:42:03.075Z
Learning: In deployment scripts like `DeployGasZipFacet.s.sol`, do not report issues suggesting adding error handling for missing configuration files or unexpected JSON structures, as the script will fail if the file is missing.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-01-28T14:27:50.689Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-06-13T08:30:28.665Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/facets/DeployPioneerFacet.s.sol:17-27
Timestamp: 2025-06-13T08:30:28.665Z
Learning: Deployment scripts rely on external automation (pre-script checks) to validate constructor arguments such as non-zero addresses, so adding explicit `require` guards inside the Foundry deployment scripts is generally unnecessary.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-10-04T09:21:59.708Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-04T09:21:59.708Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-11-25T13:49:40.464Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2024-10-14T00:50:53.603Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/facets/DeployPermit2Proxy.s.sol:17-20
Timestamp: 2024-10-14T00:50:53.603Z
Learning: In `DeployScriptBase`, `getConstructorArgs()` is called and constructor arguments are handled, so child deploy scripts do not need to pass constructor arguments explicitly to the `deploy` function.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 Learning: 2025-07-23T08:12:29.064Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/deploy/facets/UpdateWhitelistManagerFacet.s.sol:1-1
Timestamp: 2025-07-23T08:12:29.064Z
Learning: Deployment scripts in script/deploy/ should not use the LGPL-3.0 license identifier, contrary to other Solidity files. Scripts can use UNLICENSED or other appropriate license identifiers.
Applied to files:
script/deploy/facets/DeployEcoFacet.s.sol
📚 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.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2024-10-09T03:47:21.269Z
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.
Applied to files:
deployments/optimism.staging.jsondeployments/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.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2024-10-24T06:17:25.211Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: deployments/bsc.diamond.staging.json:100-101
Timestamp: 2024-10-24T06:17:25.211Z
Learning: In `deployments/bsc.diamond.staging.json`, the trailing comma after the last property in the `Periphery` object is acceptable and should not be flagged as a JSON formatting error.
Applied to files:
deployments/optimism.staging.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Applied to files:
test/solidity/Facets/EcoFacet.t.soldeployments/optimism.diamond.staging.json
📚 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: 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-31T09:10:16.115Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:0-0
Timestamp: 2024-10-31T09:10:16.115Z
Learning: In the `signData` function within `RelayFacetTest` in `test/solidity/Facets/RelayFacet.t.sol`, the use of the EIP-712 standard for hashing and signing is not required.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
PR: lifinance/contracts#1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/fantom.diamond.json:92-94
Timestamp: 2024-11-26T01:04:16.637Z
Learning: In `deployments/*.diamond.json` files, it's acceptable for facets to have empty `Name` and `Version` fields, and these should not be flagged as issues.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:14:58.163Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:5-63
Timestamp: 2024-11-26T01:14:58.163Z
Learning: When facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report this issue, as it is already known and will be addressed separately.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-02-12T09:44:12.961Z
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
Applied to files:
deployments/optimism.diamond.staging.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
🧬 Code graph analysis (1)
script/demoScripts/demoEco.ts (2)
script/common/types.ts (1)
SupportedChain(3-3)script/demoScripts/utils/demoScriptHelpers.ts (4)
setupEnvironment(636-688)ADDRESS_USDC_OPT(61-61)ensureBalance(827-848)ensureAllowance(853-887)
🔇 Additional comments (16)
deployments/optimism.staging.json (1)
48-49: Cross-file consistency confirmed – manual checksum verification requiredAll automated consistency checks passed:
- deployments/optimism.staging.json defines
“EcoFacet”: 0x26B5c627471C1c057020aCFC06DF4Bd08e7207e6- deployments/optimism.diamond.staging.json contains the same
Name (“EcoFacet”) and matching address- _deployments_log_file.json includes an
EcoFacet.optimism.stagingentry with the identical addressEth_utils was unavailable in the sandbox, so the EIP-55 checksum couldn’t be automatically validated. Please manually confirm that 0x26B5c627471C1c057020aCFC06DF4Bd08e7207e6 is correctly checksummed before merging.
deployments/_deployments_log_file.json (1)
39128-39144: ✅ Verified: EcoFacet.optimism.staging deployment entry is fully consistentAll checks passed with the provided script:
- JSON integrity confirmed (
jq . deployments/_deployments_log_file.jsonsucceeded).- Constructor argument decodes to portal
0x90f0c8acc1e083bcb4f487f84fc349ae8d5e28d7, matching the value inconfig/eco.json(case-insensitive match).- Facet address
0x26B5c627471C1c057020aCFC06DF4Bd08e7207e6appears in bothdeployments/optimism.staging.jsonanddeployments/optimism.diamond.staging.json.- No CREATE2/CREATE3 or SALT usage detected in
script/deploy/facets/DeployEcoFacet.s.sol.Everything aligns with our logging conventions and the “historical record” rule.
deployments/optimism.diamond.staging.json (1)
88-91: LGTM! New EcoFacet deployment and staging placeholders are properly configured.The new EcoFacet entry at
0x26B5c627471C1c057020aCFC06DF4Bd08e7207e6with version1.0.0is properly added. The empty Name/Version fields for staging facets and the new Patcher periphery placeholder are acceptable in staging deployment files.Also applies to: 148-151, 160-167, 180-180
test/solidity/Facets/EcoFacet.t.sol (5)
12-22: LGTM! TestEcoFacet wrapper provides necessary access controls.The public wrapper methods
addDexandsetFunctionApprovalBySignaturecorrectly expose the LibAllowList functionality needed for testing the facet's allowlist-based restrictions.
30-79: LGTM! Comprehensive test setup with proper diamond integration.The setup correctly:
- Forks testnet state and adds liquidity
- Deploys TestEcoFacet with portal configuration
- Attaches facet to diamond with all required selectors
- Configures allowlist for Uniswap DEX and required function selectors
- Sets appropriate test constants for bridge data and solver rewards
81-101: LGTM! Valid EcoData construction with proper test values.The
getValidEcoData()helper constructs realistic EcoData with appropriate defaults for testing, including proper deadline calculations and empty destination calls array.
103-136: LGTM! Correct implementation of TestBaseFacet overrides.Both bridge methods correctly handle native vs ERC20 flows by sending appropriate
msg.value(bridge amount + solver reward for native, only solver reward for ERC20 tokens).
138-141: LGTM! Proper invalid config test with correct error expectation.The test correctly verifies that
EcoFacetconstruction with a zero address portal reverts withInvalidConfig.selector.src/Facets/EcoFacet.sol (6)
1-19: LGTM! Proper contract header with all required NatSpec tags.The contract follows LiFi standards with:
- Correct LGPL-3.0-only license
- All required NatSpec tags (@title, @author, @notice, @Custom:version)
- Proper author attribution to "LI.FI (https://li.fi)"
- Semantic version 1.0.0
54-59: LGTM! Proper constructor validation with appropriate error handling.The constructor correctly validates the portal address and reverts with
InvalidConfigfor zero addresses, ensuring the immutable portal is always valid.
63-83: LGTM! Well-structured public entry points with proper modifiers.Both functions correctly:
- Apply all necessary modifiers (nonReentrant, refundExcessNative, validateBridgeData, source swap validation)
- Handle asset deposits appropriately (direct deposit vs swap+deposit)
- Delegate to internal
_startBridgefor core logicAlso applies to: 85-110
142-167: LGTM! Comprehensive bridge data validation.The validation correctly ensures:
- Consistency between destination calls and hasDestinationCall flag
- Proper receiver address matching for non-destination-call scenarios
- Non-EVM receiver validation when targeting non-EVM chains
169-202: LGTM! Smart route call construction based on context.The logic appropriately handles different scenarios:
- Uses provided destination calls when available
- Returns empty calls for non-EVM receivers
- Constructs appropriate transfer calls (ERC20 transfer vs native value transfer) based on asset type
278-291: LGTM! Appropriate event emissions for different bridge scenarios.The function correctly emits
BridgeToNonEVMChainfor non-EVM destinations and always emitsLiFiTransferStartedfor tracking purposes.script/demoScripts/demoEco.ts (2)
21-25: Good fix: ABI source is now well-typed and local.Switching to the compiled artifact and Narrow typing resolves the earlier undefined ABI issue and avoids runtime mismatches.
8-8: Please verify the TypeChain output directory and import path forILiFi
I wasn’t able to locate atypechainortypechain-typesfolder in the repo, so the import inscript/demoScripts/demoEco.tswill likely fail to resolve. Please confirm where your generated TypeChain definitions live (and that they’re committed), then update the import accordingly.• Check your TypeChain configuration (e.g., in
hardhat.config.tsortypechain.config.ts) to see the output directory
• Ensure the generated types folder (commonlytypechainortypechain-types) exists at the project root
• Adjust the import inscript/demoScripts/demoEco.tsto point at the correct relative path forILiFi
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
script/demoScripts/demoEco.ts (3)
105-131: Consider using the createContractObject helper for consistency.Similar to the previous suggestion, the tokenContract creation duplicates logic that could be handled by the helper.
- const tokenContract = { - read: { - allowance: async ( - args: [`0x${string}`, `0x${string}`] - ): Promise<bigint> => { - return (await publicClient.readContract({ - address: SRC_TOKEN_ADDRESS, - abi: erc20Abi, - functionName: 'allowance', - args, - })) as bigint - }, - }, - write: { - approve: async ( - args: [`0x${string}`, bigint] - ): Promise<`0x${string}`> => { - return walletClient.writeContract({ - address: SRC_TOKEN_ADDRESS, - abi: erc20Abi, - functionName: 'approve', - args, - } as any) - }, - }, - } + const tokenContract = createContractObject( + SRC_TOKEN_ADDRESS, + erc20Abi, + publicClient, + walletClient + )
100-100: Clarify the inconsistent comment about the fee amount.The comment states "0.25 USDC fee" but the code uses the
feeAmountvariable which is set to 0.015 USDC. This creates confusion about the actual fee amount.- solverReward: feeAmount, // 0.25 USDC fee instead of ETH + solverReward: feeAmount, // 0.015 USDC fee
77-88: Verify destinationChainId type: should be bigint for ILiFi.BridgeDataStruct.The ILiFi.BridgeDataStruct expects
destinationChainIdas a bigint (uint256), but the current implementation uses a number type which may cause TypeScript compilation errors.const bridgeData: ILiFi.BridgeDataStruct = { transactionId: `0x${randomBytes(32).toString('hex')}`, bridge: 'eco', integrator: 'demoScript', referrer: zeroAddress, sendingAssetId: SRC_TOKEN_ADDRESS, receiver: signerAddress, // Receiver on destination chain (same as signer) - destinationChainId: args.dstChain === 'base' ? 8453 : 8453, // Default to Base for now + destinationChainId: BigInt(args.dstChain === 'base' ? 8453 : 8453), // Default to Base for now minAmount: amount, hasSourceSwaps: false, hasDestinationCall: false, }src/Facets/EcoFacet.sol (3)
227-243: Route tokens should be based on the receiving asset, not the sending asset.The function currently checks if the sending asset is native to determine route tokens, but route tokens represent what will be delivered on the destination chain. This should be based on the receiving asset instead.
function _buildRouteTokens( ILiFi.BridgeData memory _bridgeData, EcoData calldata _ecoData ) private pure returns (IEcoPortal.TokenAmount[] memory) { - if (LibAsset.isNativeAsset(_bridgeData.sendingAssetId)) { + if (LibAsset.isNativeAsset(_ecoData.receivingAssetId)) { return new IEcoPortal.TokenAmount[](0); } IEcoPortal.TokenAmount[] memory routeTokens = new IEcoPortal.TokenAmount[](1); routeTokens[0] = IEcoPortal.TokenAmount({ token: _ecoData.receivingAssetId, amount: _bridgeData.minAmount }); return routeTokens; }
280-282: Reward native amount includes minAmount unnecessarily.For native token transfers, the reward's native amount currently includes both the solver reward and the bridge amount. The bridge amount is already accounted for in the route, so including it in the reward causes double-counting and may lead to incorrect funding.
nativeAmount: isNative - ? _ecoData.solverReward + _bridgeData.minAmount - : 0, // No native amount for ERC20 + ? _ecoData.solverReward + : 0, // Only solver reward, not bridge amount
272-272: Route native amount should be based on the receiving asset.The route's native amount represents what needs to be delivered on the destination chain. This should be determined by whether the receiving asset is native, not the sending asset.
- nativeAmount: isNative ? _bridgeData.minAmount : 0, + nativeAmount: LibAsset.isNativeAsset(_ecoData.receivingAssetId) ? _bridgeData.minAmount : 0,
🧹 Nitpick comments (6)
script/demoScripts/demoEco.ts (1)
61-73: Consider using the createContractObject helper for consistency.The current manual creation of the usdcContract object duplicates logic from the existing
createContractObjecthelper. Consider using the helper to reduce code duplication and ensure consistent error handling.- const usdcContract = { - read: { - balanceOf: async (args: [`0x${string}`]): Promise<bigint> => { - return (await publicClient.readContract({ - address: SRC_TOKEN_ADDRESS, - abi: erc20Abi, - functionName: 'balanceOf', - args, - })) as bigint - }, - }, - } as const + const usdcContract = createContractObject( + SRC_TOKEN_ADDRESS, + erc20Abi, + publicClient, + walletClient + )You'll need to import the helper:
import { createContractObject } from './utils/demoScriptHelpers'deployments/optimism.diamond.staging.json (4)
88-91: Missing facet metadata for CelerIMFacetMutable.The CelerIMFacetMutable facet at address
0x2Af20933E5886aFe275c0EEE4A2e65daA4E8b169has empty Name and Version fields. While this is acceptable for staging environments, consider adding the metadata for better traceability.
120-123: Missing facet metadata for two unidentified facets.The facets at addresses
0xa137Fe4C41A2E04ca34578DC9023ad45cC194389and0x6124C65B6264bE13f059b7C3A891a5b77DA8Bd95have empty Name and Version fields. Consider documenting what these facets are for better maintainability.
148-151: Missing facet metadata for multiple facets.Several facets have empty Name and Version fields:
0x91559A75bd9045681265C77922b3cAeDB3D5120d(lines 149-150)0x8962d191Ba0f2Bc29b949ACA222f8251B241190b(lines 153-154)0x0f4D0D8890bAF9036D3895315559a6F0d573e2EC(lines 161-162)0x59A1Bcaa32EdB1a233fEF945857529BBD6df247f(lines 165-166)While acceptable for staging, documenting these facets would improve maintainability.
Also applies to: 152-155, 160-163, 164-167
180-180: Empty Patcher address in periphery.The Patcher periphery contract has an empty address. If this contract is not needed for the staging environment, consider documenting why it's empty or removing the entry entirely.
test/solidity/Facets/EcoFacet.t.sol (1)
71-72: Add the missing SwapRouter02exactInputfunction selector.For completeness and to support Uniswap V3 SwapRouter02 operations, consider adding the
exactInputfunction selector approval. Based on web search results, the correct signature for SwapRouter02'sexactInputuses a 5-field tuple.ecoFacet.setFunctionApprovalBySignature( uniswap.swapETHForExactTokens.selector ); + // Approve SwapRouter02.exactInput(ExactInputParams) selector + // ExactInputParams: (bytes path, address recipient, uint256 deadline, uint256 amountIn, uint256 amountOutMinimum) + ecoFacet.setFunctionApprovalBySignature( + bytes4(keccak256("exactInput((bytes,address,uint256,uint256,uint256))")) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(4 hunks)deployments/optimism.staging.json(1 hunks)script/demoScripts/demoEco.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(1 hunks)src/Facets/EcoFacet.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/_deployments_log_file.json
- deployments/optimism.staging.json
🧰 Additional context used
📓 Path-based instructions (5)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit inference engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
src/Facets/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
Facet contracts must reside in
src/Facets/and names must include "Facet".
Files:
src/Facets/EcoFacet.sol
script/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/**/*.ts: All scripts must follow the rules defined in.eslintrc.cjs.
Use async/await for asynchronous operations in TypeScript scripts.
Handle errors appropriately with try/catch blocks in TypeScript scripts.
Include proper logging for debugging and monitoring in TypeScript scripts.
Use environment variables for configuration in TypeScript scripts.
All scripts must usecittyfor CLI argument parsing.
Useconsolafor consistent logging across TypeScript scripts.
Environment variables should be validated usinggetEnvVar()helper in TypeScript scripts.
Scripts should exit with appropriate exit codes (0 for success, 1 for error) in TypeScript scripts.
Files:
script/demoScripts/utils/demoScriptHelpers.tsscript/demoScripts/demoEco.ts
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Test files must have a.t.solextension.
All tests that verify a successful execution must be prefixed with:test_.
All tests that verify a failure case must be prefixed with:testRevert_.
For base or inherited tests, prefix withtestBase_.
Every test contract must include asetUp()function.
Any contract inheriting fromTestBase.solmust callinitTestBase()insetUp().
UseassertEq()for checking equality of values in tests.
Usevm.expectRevert()to verify specific revert reasons in failure test cases.
Usevm.expectEmit(true, true, true, true, <contractAddress>)for event testing.
All Solidity test files must be organized mirroring thesrc/structure.
All Solidity test files must use Foundry.
Files:
test/solidity/Facets/EcoFacet.t.sol
🧠 Learnings (41)
📓 Common learnings
Learnt from: mirooon
PR: lifinance/contracts#945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T22:23:51.247Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.247Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T23:36:40.706Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.706Z
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-04-04T07:21:52.878Z
Learnt from: mirooon
PR: lifinance/contracts#1086
File: test/solidity/utils/TestBase.sol:195-205
Timestamp: 2025-04-04T07:21:52.878Z
Learning: In the TestBase.sol file, the Optimism network constants are set correctly with:
- ADDRESS_UNISWAP_OPTIMISM: 0x4A7b5Da61326A6379179b40d00F57E5bbDC962c2 (Uniswap V2 Router)
- ADDRESS_USDC_OPTIMISM: 0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85 (Native USDC)
- USDC.e (bridged USDC from Ethereum) on Optimism has address 0x7f5c764cbc14f9669b88837ca1490cca17c31607
Applied to files:
script/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2024-10-10T09:26:14.420Z
Learnt from: maxklenk
PR: lifinance/contracts#830
File: config/hop.json:536-582
Timestamp: 2024-10-10T09:26:14.420Z
Learning: In the `config/hop.json` file, for the `MATIC` token in the `polygon` section, having the zero address `0x0000000000000000000000000000000000000000` is acceptable because it references WMATIC.
Applied to files:
script/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2024-11-21T08:39:29.530Z
Learnt from: ezynda3
PR: lifinance/contracts#861
File: config/dexs.json:748-752
Timestamp: 2024-11-21T08:39:29.530Z
Learning: In the 'worldchain' network, the addresses `0x50D5a8aCFAe13Dceb217E9a071F6c6Bd5bDB4155`, `0x8f023b4193a6b18C227B4a755f8e28B3D30Ef9a1`, and `0x603a538477d44064eA5A5d8C345b4Ff6fca1142a` are used as DEXs and should be included in `config/dexs.json`.
Applied to files:
script/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2024-11-25T09:04:55.880Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2024-11-25T06:25:01.232Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-19T05:10:12.922Z
Learnt from: ezynda3
PR: lifinance/contracts#1303
File: script/utils/network.ts:27-32
Timestamp: 2025-08-19T05:10:12.922Z
Learning: In the lifinance/contracts repository, the team decided to standardize RPC environment variable naming by converting all hyphens to underscores in network names. For example, "tron-shasta" becomes "ETH_NODE_URI_TRON_SHASTA". The getRPCEnvVarName() helper in script/utils/network.ts implements this standard and should be used consistently across the codebase instead of direct string manipulation.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2024-11-04T03:47:47.465Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#847
File: script/tasks/fundNewWalletOnAllChains.ts:156-164
Timestamp: 2024-11-04T03:47:47.465Z
Learning: In the `fundNewWalletOnAllChains.ts` script, the gas estimation code involving `publicClient.estimateGas()` is intentionally commented out and should remain that way unless specified otherwise.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to script/**/*.ts : All scripts must use `citty` for CLI argument parsing.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-17T04:21:55.549Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-17T04:21:50.790Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-22T10:03:58.794Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1299
File: script/demoScripts/demoAcrossV4.ts:728-737
Timestamp: 2025-08-22T10:03:58.794Z
Learning: Demo scripts in `script/demoScripts` are exempt from the citty CLI argument parsing requirement that applies to other TypeScript scripts. Templates for demo scripts don't use citty and this is acceptable.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Applied to files:
test/solidity/Facets/EcoFacet.t.soldeployments/optimism.diamond.staging.json
📚 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectRevert()` to verify specific revert reasons in failure test cases.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a failure case must be prefixed with: `testRevert_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : Use `vm.expectEmit(true, true, true, true, <contractAddress>)` for event testing.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-28T07:54:14.853Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to test/solidity/**/*.t.sol : All tests that verify a successful execution must be prefixed with: `test_`.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-08-28T02:41:07.483Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1321
File: docs/RelayDepositoryFacet.md:9-10
Timestamp: 2025-08-28T02:41:07.483Z
Learning: Unit tests for RelayDepositoryFacet cannot verify fund forwarding behavior after deposits because the facet delegates to external IRelayDepository contracts. The forwarding logic is implemented in the Relay Protocol V2 Depository contracts, not in the facet itself.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
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-01-28T11:29:09.566Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
PR: lifinance/contracts#1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 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: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/fantom.diamond.json:92-94
Timestamp: 2024-11-26T01:04:16.637Z
Learning: In `deployments/*.diamond.json` files, it's acceptable for facets to have empty `Name` and `Version` fields, and these should not be flagged as issues.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-02-12T09:44:12.961Z
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
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-10-09T03:47:21.269Z
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.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:14:58.163Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:5-63
Timestamp: 2024-11-26T01:14:58.163Z
Learning: When facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report this issue, as it is already known and will be addressed separately.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:16:27.721Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:40-43
Timestamp: 2024-11-26T01:16:27.721Z
Learning: Version inconsistencies of 'GenericSwapFacetV3' across deployment configurations are acceptable if they are only due to differences in Solidity pragma directives.
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 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/optimism.diamond.staging.json
🧬 Code graph analysis (1)
script/demoScripts/demoEco.ts (2)
script/common/types.ts (1)
SupportedChain(3-3)script/demoScripts/utils/demoScriptHelpers.ts (6)
setupEnvironment(637-689)ADDRESS_USDC_OPT(61-61)ensureBalance(828-849)ADDRESS_USDC_BASE(71-71)ensureAllowance(854-888)executeTransaction(787-823)
🔇 Additional comments (9)
script/demoScripts/utils/demoScriptHelpers.ts (1)
70-71: LGTM!The removal of the trailing space from
ADDRESS_WMATIC_POLand the addition ofADDRESS_USDC_BASEare correct. These changes support the Eco bridging demo script that targets USDC on Base.deployments/optimism.diamond.staging.json (1)
168-171: LGTM! EcoFacet properly configured.The EcoFacet is correctly configured with:
- Address:
0xc4884225aeFe7218f9f489A5Eb8beB504ab272AA- Name: "EcoFacet"
- Version: "1.0.0"
This aligns with the semantic versioning requirements and the facet implementation.
test/solidity/Facets/EcoFacet.t.sol (4)
150-153: LGTM! Proper test for invalid configuration.The test correctly verifies that the EcoFacet constructor reverts with
InvalidConfigwhen passed a zero address for the portal, ensuring proper validation of the immutable portal reference.
156-182: LGTM! Correctly handles ERC20 token rewards.The test override properly accounts for the TOKEN_SOLVER_REWARD in the balance assertions and approval amount, ensuring the EcoFacet's unique reward mechanism is properly tested.
185-213: LGTM! Fuzzed test properly validates token reward handling.The fuzzed test correctly ensures the user has sufficient balance for both the bridge amount and the TOKEN_SOLVER_REWARD, with proper bounds checking to prevent overflow.
216-292: LGTM! Comprehensive swap and bridge test with proper reward handling.The test correctly:
- Calculates the total amount needed (bridge amount + solver reward)
- Sets up the swap to produce the exact total amount
- Verifies balance changes match expectations
- Properly emits the expected events
src/Facets/EcoFacet.sol (3)
54-59: LGTM! Constructor properly validates portal address.The constructor correctly reverts with
InvalidConfigwhen the portal address is zero, ensuring the immutable portal reference is always valid.
165-190: LGTM! Comprehensive validation logic.The
_validateEcoDatafunction properly validates:
- Consistency between destination calls and the hasDestinationCall flag
- Receiver address consistency for EVM destinations
- Non-empty nonEVMReceiver for non-EVM destinations
295-298: Fix msg.value calculation to avoid double-counting.Since the reward native amount was incorrectly including the bridge amount, this publishAndFund call sends the correct total value. However, after fixing the reward native amount above, this should remain unchanged as it correctly sends the total needed (bridge amount + solver reward).
The msg.value calculation is correct and should remain as is after fixing the reward native amount calculation.
Define and use constants for: - NATIVE_REWARD_AMOUNT (0) - ALLOW_PARTIAL_FILL (false) - SOLANA_ENCODED_ROUTE_LENGTH (319) - SOLANA_RECEIVER_OFFSET (251) - SOLANA_RECEIVER_END (283) - SOLANA_ADDRESS_MIN_LENGTH (32) - SOLANA_ADDRESS_MAX_LENGTH (44) Improves code maintainability and makes the intent behind each value explicit. Resolves audit issue #1
Remove '{ value: 0 }' from PORTAL.publishAndFund call as it's redundant
(default is 0). Simplifies code and slightly reduces bytecode size.
Resolves audit issue #8
BREAKING CHANGE: Remove receiverAddress field from EcoData struct as it duplicates _bridgeData.receiver. The receiver is already validated from the encodedRoute for EVM chains and through nonEVMReceiver for Solana. This saves gas and simplifies the API by removing redundant data. Resolves audit issue #2
Remove receiverAddress field from EcoData struct usage in demo script to match the updated contract interface.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
deployments/optimism.diamond.staging.json (1)
164-183: Remove duplicate AcrossFacet V4 entriesAcrossFacetV4 and AcrossFacetPackedV4 each appear twice (Lines 164-167 & 176-179, 148-151 & 180-183) with identical Name/Version but different addresses, reintroducing the ambiguity we previously asked to resolve. Please keep only the active deployment (or rename the inactive entry with a disambiguating suffix) so downstream tooling can map facets deterministically.
script/demoScripts/demoEco.ts (1)
521-534: Critical: Use bs58.decode() to convert Solana address, not TextEncoder.
TextEncoderencodes the base58 string characters, not the underlying 32-byte public key. This will produce incorrect bytes for the Solana recipient and cause on-chain failures or send funds to the wrong address.Apply this diff:
+import bs58 from 'bs58'Then replace the encoding logic:
if (isDestinationSolana) { if (!privateKey) throw new Error('Private key required for Solana destination') const solanaAddress = deriveSolanaAddress(privateKey) - // Convert Solana base58 address to hex bytes for nonEVMReceiver field - const encoder = new TextEncoder() - const solanaAddressBytes = encoder.encode(solanaAddress) - nonEVMReceiverBytes = toHex(solanaAddressBytes) + // Convert Solana base58 address to raw 32-byte public key + const solanaAddressBytes = bs58.decode(solanaAddress) + if (solanaAddressBytes.length !== 32) { + throw new Error(`Invalid Solana address: expected 32 bytes, got ${solanaAddressBytes.length}`) + } + nonEVMReceiverBytes = toHex(solanaAddressBytes) console.log('Solana destination details:')Note: Verify that the
bs58package is installed. If not, add it withnpm install bs58orpnpm add bs58.
🧹 Nitpick comments (2)
script/demoScripts/demoEco.ts (2)
122-135: Document that derived Solana address is demo-only; consider accepting user input.The function derives a Solana address from an Ethereum private key, which creates an unrelated ed25519 keypair. While acceptable for demo purposes, this should either:
- Accept a user-provided Solana recipient address via CLI arg/env var as the primary path
- Clearly document in code comments that this derivation is DEMO-ONLY and unsuitable for real funds
Consider adding a CLI argument
--solanaRecipientand using this function only as a fallback for demo mode. Example:/** * Derives a Solana address from an Ethereum private key * * ⚠️ DEMO ONLY: This creates an unrelated ed25519 keypair. * For production, use a proper Solana wallet or accept recipient via CLI. * * Uses the Ethereum private key as a seed for Ed25519 keypair generation * * @param ethPrivateKey - Ethereum private key (with or without 0x prefix) * @returns Solana address in base58 format */Based on learnings.
383-384: Consider using getEnvVar() for PRIVATE_KEY access.While
setupEnvironment()validates the private key internally viagetPrivateKeyForEnvironment(), directly accessingprocess.env.PRIVATE_KEYhere bypasses explicit validation and violates the guideline to usegetEnvVar()consistently.Consider:
- const privateKey = process.env.PRIVATE_KEY + const privateKey = getEnvVar('PRIVATE_KEY', false)This makes the validation explicit at the point of use. However, since
setupEnvironment()already validates it, this is a minor style improvement rather than a critical fix.Based on coding guidelines.
📜 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 (4)
deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(5 hunks)deployments/optimism.staging.json(1 hunks)script/demoScripts/demoEco.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/optimism.staging.json
🧰 Additional context used
📓 Path-based instructions (1)
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 (23)
📚 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/_deployments_log_file.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2025-09-15T12:33:51.069Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1377
File: deployments/_deployments_log_file.json:5607-5619
Timestamp: 2025-09-15T12:33:51.069Z
Learning: The deployments/_deployments_log_file.json is structured as network → environment → version → array of deployment records. Always understand the file hierarchy before attempting validation, and target specific paths rather than trying to validate all objects in the JSON.
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-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-01-16T00:24:10.002Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#927
File: foundry.toml:78-78
Timestamp: 2025-01-16T00:24:10.002Z
Learning: Avalanche chain (chainid=43114) is supported by Etherscan V2 API and can use the endpoint https://api.etherscan.io/v2/api?chainid=43114 instead of Snowtrace.
Applied to files:
deployments/_deployments_log_file.json
📚 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 script/**/*.ts : Prefer existing helper functions over reimplementation (e.g., getDeployments, getProvider, getWalletFromPrivateKeyInDotEnv, sendTransaction, ensureBalanceAndAllowanceToDiamond, getUniswapData*)
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-19T05:10:12.922Z
Learnt from: ezynda3
PR: lifinance/contracts#1303
File: script/utils/network.ts:27-32
Timestamp: 2025-08-19T05:10:12.922Z
Learning: In the lifinance/contracts repository, the team decided to standardize RPC environment variable naming by converting all hyphens to underscores in network names. For example, "tron-shasta" becomes "ETH_NODE_URI_TRON_SHASTA". The getRPCEnvVarName() helper in script/utils/network.ts implements this standard and should be used consistently across the codebase instead of direct string manipulation.
Applied to files:
script/demoScripts/demoEco.ts
📚 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 script/**/*.ts : Always use proper TypeChain types (e.g., ILiFi.BridgeDataStruct) and never use any for bridge data or contract structures
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-05-27T16:19:16.726Z
Learnt from: mirooon
PR: lifinance/contracts#1165
File: src/Facets/GnosisBridgeFacet.sol:22-22
Timestamp: 2025-05-27T16:19:16.726Z
Learning: The correct USDS token address on Ethereum mainnet is 0xdC035D45d973E3EC169d2276DDab16f1e407384F, not 0xA4Bdb11dc0a2bEC88d24A3aa1E6Bb17201112eBe.
Applied to files:
script/demoScripts/demoEco.ts
📚 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:
script/demoScripts/demoEco.ts
📚 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: 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: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1381
File: deployments/arbitrum.json:65-69
Timestamp: 2025-09-16T01:39:54.099Z
Learning: When verifying facet deployments across .json and .diamond.json files, search by facet name rather than trying to cross-reference addresses between the files, as the same contract can have different addresses in different deployment files.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/fantom.diamond.json:92-94
Timestamp: 2024-11-26T01:04:16.637Z
Learning: In `deployments/*.diamond.json` files, it's acceptable for facets to have empty `Name` and `Version` fields, and these should not be flagged as issues.
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: 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/optimism.diamond.staging.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:14:58.163Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:5-63
Timestamp: 2024-11-26T01:14:58.163Z
Learning: When facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report this issue, as it is already known and will be addressed separately.
Applied to files:
deployments/optimism.diamond.staging.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: 2024-10-09T03:47:21.269Z
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.
Applied to files:
deployments/optimism.diamond.staging.json
🧬 Code graph analysis (1)
script/demoScripts/demoEco.ts (2)
script/demoScripts/utils/demoScriptHelpers.ts (21)
ADDRESS_USDC_OPT(61-61)ADDRESS_USDC_BASE(75-75)ADDRESS_USDC_ARB(63-63)ADDRESS_USDC_ETH(57-57)ADDRESS_USDC_POL(59-59)ADDRESS_USDC_SOL(67-67)ADDRESS_WETH_OPT(70-70)ADDRESS_WETH_BASE(73-73)ADDRESS_WETH_ARB(72-72)ADDRESS_WETH_ETH(69-69)ADDRESS_WETH_POL(71-71)ADDRESS_UNISWAP_OPT(81-81)ADDRESS_UNISWAP_BASE(83-83)ADDRESS_UNISWAP_ARB(82-82)ADDRESS_UNISWAP_ETH(78-78)ADDRESS_UNISWAP_POL(80-80)setupEnvironment(653-707)getUniswapDataERC20toExactERC20(334-406)ensureBalance(846-867)ensureAllowance(872-906)executeTransaction(805-841)script/common/types.ts (1)
SupportedChain(3-3)
⏰ 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 (1)
deployments/_deployments_log_file.json (1)
43054-43060: Timestamp is set in the future.The TIMESTAMP
2025-10-06 17:59:14post-dates this PR by ~2 months, so this log can’t reflect an executed deployment. These records must document real deployments with accurate metadata—please update after the deployment occurs or correct the timestamp now. Based on learnings⛔ Skipped due to learnings
Learnt from: ezynda3 PR: lifinance/contracts#861 File: deployments/_deployments_log_file.json:22488-22488 Timestamp: 2024-11-22T07:19:51.143Z Learning: In `deployments/_deployments_log_file.json`, deployment timestamps may appear to be set in the future and should not be flagged as invalid.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: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: 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: 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#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.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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docs/EcoFacet.md (1)
152-154: Use unambiguous chain identifiers in /quote examples (prefer numeric IDs 10 = Optimism, 8453 = Base)Replace non-standard slugs OPT/BAS with chain IDs to avoid drift.
-```shell -curl 'https://li.quest/v1/quote?fromChain=OPT&fromAmount=5000000&fromToken=USDC&toChain=BAS&toToken=USDC&slippage=0.03&allowBridges=eco&fromAddress={YOUR_WALLET_ADDRESS}' -``` +```shell +curl 'https://li.quest/v1/quote?fromChain=10&fromAmount=5000000&fromToken=USDC&toChain=8453&toToken=USDC&slippage=0.03&allowBridges=eco&fromAddress={YOUR_WALLET_ADDRESS}' +``` @@ -```shell -curl 'https://li.quest/v1/quote?fromChain=OPT&fromAmount=5000000&fromToken=USDT&toChain=BAS&toToken=USDC&slippage=0.03&allowBridges=eco&fromAddress={YOUR_WALLET_ADDRESS}' -``` +```shell +curl 'https://li.quest/v1/quote?fromChain=10&fromAmount=5000000&fromToken=USDT&toChain=8453&toToken=USDC&slippage=0.03&allowBridges=eco&fromAddress={YOUR_WALLET_ADDRESS}' +```Also applies to: 160-162
deployments/_deployments_log_file.json (1)
43049-43061: Verify verification status, address/args integrity, and field types for EcoFacet record
- VERIFIED is "false" (string). If the contract is now verified (per prior thread), flip to true and keep type consistent with the rest of this file. Also prefer consistent timestamp format across entries.
- Sanity-check ADDRESS checksum and that CONSTRUCTOR_ARGS encodes the configured Eco portal address for optimism.
- Confirm this address is referenced in optimism staging facet/diamond JSONs.
Run this script from repo root:
#!/bin/bash set -euo pipefail LOG=deployments/_deployments_log_file.json echo "1) Extract EcoFacet optimism staging 1.0.0 record" jq -r '.EcoFacet.optimism.staging["1.0.0"][0]' "$LOG" echo -e "\n2) Check VERIFIED field type consistency across file" echo "Types found for VERIFIED:" jq -r ' paths(strings) as $p | getpath($p) as $v | if ($p|tostring|test("VERIFIED$")) then (type) else empty end ' "$LOG" | sort -u echo -e "\n3) Validate checksum casing of ADDRESS and constructor arg encoding" python - << 'PY' import json, sys, re from pathlib import Path try: from eth_utils import to_checksum_address except ImportError: print("Installing eth_utils...", file=sys.stderr) import subprocess; subprocess.check_call([sys.executable, "-m", "pip", "install", "eth_utils"], stdout=subprocess.DEVNULL) from eth_utils import to_checksum_address log = json.loads(Path("deployments/_deployments_log_file.json").read_text()) rec = log["EcoFacet"]["optimism"]["staging"]["1.0.0"][0] addr = rec["ADDRESS"] args = rec["CONSTRUCTOR_ARGS"] def tail20(hexstr): s = hexstr.lower().removeprefix("0x") return "0x"+s[-40:] ok_checksum = (addr == to_checksum_address(addr)) print(f"ADDRESS: {addr}") print(f"Checksum valid/cased: {ok_checksum}") # Extract last 20 bytes of constructor args arg_tail = tail20(args) print(f"Constructor arg tail (address-encoded): {arg_tail}") # Try to locate the same address in config/eco.json import subprocess, json as js try: cfg = js.loads(Path("config/eco.json").read_text()) # naive scan for values equal to arg_tail (case-insensitive) found = [] def walk(o, path=""): if isinstance(o, dict): for k,v in o.items(): walk(v, f"{path}/{k}") elif isinstance(o, list): for i,v in enumerate(o): walk(v, f"{path}[{i}]") elif isinstance(o, str): if o.lower() == arg_tail.lower(): found.append(path) walk(cfg) print(f"Portal address matches in config/eco.json at paths: {found if found else 'NOT FOUND'}") except FileNotFoundError: print("config/eco.json not found; please verify manually.") PY echo -e "\n4) Ensure the facet address is referenced in optimism staging facet/diamond files" echo "Matches in deployments/*optimism*staging*.json:" rg -n -C2 -i 'EcoFacet|0xA64C0263B1a37B3241Da01046134b7707D39f00b' deployments | sed -n '1,120p'Based on learnings
script/demoScripts/demoEco.ts (1)
519-524: Non‑EVM receiver encoding for Solana (ack prior)The TextEncoder approach encodes base58 chars, not raw pubkey bytes. This was discussed earlier; if you revisit, consider bs58.decode for Solana only. Otherwise, current variable-length policy is acceptable here.
🧹 Nitpick comments (9)
docs/EcoFacet.md (3)
107-107: Make link text descriptive (MD059)
Replace “here” with meaningful text.-The swap library can be found [here](../src/Libraries/LibSwap.sol). +See the LibSwap library source (../src/Libraries/LibSwap.sol). @@ -… can be found [here](../src/Interfaces/ILiFi.sol). +… see ILiFi interface and events (../src/Interfaces/ILiFi.sol). @@ -… can be found [here](https://docs.li.fi/products/more-integration-options/li.fi-api/transferring-tokens-example). +… see Transferring tokens with the LiFi API (https://docs.li.fi/products/more-integration-options/li.fi-api/transferring-tokens-example).Also applies to: 113-113, 144-144
75-84: Clarify native bridging vs swap-from-native
State that native token as the bridge asset is unsupported, but swapping from native to ERC20 before bridging is supported (set msg.value to swapData[0].fromAmount).Based on learnings
-**Native Token Bridging**: The EcoFacet contract does not support native token bridging. Only ERC20 token transfers are supported. Transactions will revert if native tokens are specified as the sending asset. +**Native Token Bridging**: Bridging a native token directly is not supported and will revert. Only ERC20 tokens can be bridged. +However, you may swap from a native token to the ERC20 bridge token in the same transaction (set `msg.value = _swapData[0].fromAmount` when the swap input is native).
54-57: Specify Solana address encoding precisely
Use “base58-decoded 32-byte public key,” not “base58 address encoded as bytes” to avoid ambiguity.-Provide the Solana address in `nonEVMReceiver` as bytes (base58 address encoded as bytes) +Provide the Solana address in `nonEVMReceiver` as the base58‑decoded 32‑byte public key (bytes)script/demoScripts/demoEco.ts (3)
106-114: Use getEnvVar() and consola for env + logging per script standardsReplace direct process.env and console.* with repo helpers/logging.
Apply:
+import consola from 'consola' +// If your repo exposes getEnvVar, import from its utils module: +// import { getEnvVar } from '../utils/utils' // adjust path to your utils @@ -const ECO_API_URL = process.env.ECO_API_URL || 'https://quotes.eco.com' -const DAPP_ID = process.env.ECO_DAPP_ID || 'lifi-demo' +const ECO_API_URL = (typeof getEnvVar === 'function' ? getEnvVar('ECO_API_URL', false) : process.env.ECO_API_URL) || 'https://quotes.eco.com' +const DAPP_ID = (typeof getEnvVar === 'function' ? getEnvVar('ECO_DAPP_ID', false) : process.env.ECO_DAPP_ID) || 'lifi-demo' - -console.log('Eco API Configuration:') -console.log(' API URL:', ECO_API_URL) -console.log(' DAPP ID:', DAPP_ID) -console.log(' Set ECO_API_URL and ECO_DAPP_ID env vars to override defaults\n') +consola.info('Eco API Configuration:') +consola.info(' API URL:', ECO_API_URL) +consola.info(' DAPP ID:', DAPP_ID) +consola.info(' Set ECO_API_URL and ECO_DAPP_ID env vars to override defaults\n')Based on coding guidelines.
311-354: Switch API-call logs to consola and keep error paths consistentUse consola for uniform logging in scripts.
- console.log('Fetching quote from Eco API...') - console.log('Quote request:', JSON.stringify(quoteRequest, null, 2)) + consola.info('Fetching quote from Eco API...') + consola.info('Quote request:', JSON.stringify(quoteRequest, null, 2)) @@ - console.log('Quote received:', JSON.stringify(data, null, 2)) + consola.info('Quote received:', JSON.stringify(data, null, 2)) @@ - console.error('API returned error:', data.error) + consola.error('API returned error:', data.error) @@ - console.error('Invalid quote response structure.') - console.error('Expected structure with data.quoteResponse, but got:', data) + consola.error('Invalid quote response structure.') + consola.error('Expected structure with data.quoteResponse, but got:', data)Based on coding guidelines.
376-382: Replace remaining console. in main() with consola for consistency*Use consola.info/log/error across main(); keep messages identical. This aligns with repo logging standards and avoids mixed logging sinks.
Based on coding guidelines.
Also applies to: 599-621, 632-655
src/Facets/EcoFacet.sol (2)
24-26: Prefer constant over immutable for compile-time chain IDsThese values are known at compile time; using constant slightly reduces bytecode/gas.
- uint64 private immutable ECO_CHAIN_ID_TRON = 728126428; - uint64 private immutable ECO_CHAIN_ID_SOLANA = 1399811149; + uint64 private constant ECO_CHAIN_ID_TRON = 728126428; + uint64 private constant ECO_CHAIN_ID_SOLANA = 1399811149;
122-130: Clarify native-token note in docstringNative bridging is disabled via noNativeAsset; the “Native tokens handle positive slippage...” line can confuse readers. Clarify it refers to potential future native support or swap input ETH only.
test/solidity/Facets/EcoFacet.t.sol (1)
29-31: Use a mock IEcoPortal and assert publishAndFund inputsPointing PORTAL to a hard-coded address on a fork can lead to false positives (EOA/no-code calls “succeed”). Replace with a mock that records args or add vm.expectCall to assert destination, encodedRoute, reward.tokens[0], and allowPartial.
As a follow-up, I can provide a minimal MockEcoPortal that captures the last call and exposes getters for assertions.
Also applies to: 50-67, 380-387
📜 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 (8)
config/eco.json(1 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(5 hunks)deployments/optimism.staging.json(1 hunks)docs/EcoFacet.md(1 hunks)script/demoScripts/demoEco.ts(1 hunks)src/Facets/EcoFacet.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/optimism.staging.json
- deployments/optimism.diamond.staging.json
🧰 Additional context used
📓 Path-based instructions (7)
{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.soltest/solidity/Facets/EcoFacet.t.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.soltest/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
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
{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
🧠 Learnings (39)
📚 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:
config/eco.json
📚 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 : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.soldocs/EcoFacet.md
📚 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.solscript/demoScripts/demoEco.ts
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.
Applied to files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Facets/EcoFacet.solscript/demoScripts/demoEco.ts
📚 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:
src/Facets/EcoFacet.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:
src/Facets/EcoFacet.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 : Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Applied to files:
src/Facets/EcoFacet.soltest/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:
src/Facets/EcoFacet.soltest/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:
src/Facets/EcoFacet.soldocs/EcoFacet.md
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
PR: lifinance/contracts#1406
File: src/Facets/UnitFacet.sol:75-81
Timestamp: 2025-10-02T18:10:09.934Z
Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.
Applied to files:
src/Facets/EcoFacet.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 : Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T13:51:52.704Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:33-0
Timestamp: 2025-08-27T13:51:52.704Z
Learning: In LDA facets like NativeWrapperFacet, mirooon prefers to rely on CoreRouteFacet's upstream validation for parameters like `from` rather than adding defensive validation checks in individual facets, since CoreRouteFacet ensures only valid values (msg.sender, address(this), or INTERNAL_INPUT_SOURCE) are passed to the facets.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
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.soltest/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 : Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Applied to files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-02-21T09:00:28.226Z
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-02-21T09:00:28.226Z
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-11-22T08:49:35.205Z
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-30T12:00:56.667Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:1-1
Timestamp: 2025-09-30T12:00:56.667Z
Learning: In the lifinance/contracts repository, audit logging is handled manually by the team, and merging is automatically restricted until an audit has been logged. Missing audit entries in auditLog.json should not be flagged as critical blocking issues during review, as the team has process controls in place to prevent merging without proper audits.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-12-12T02:22:38.502Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#902
File: .github/workflows/forceMergePRBypassAudit.yml:0-0
Timestamp: 2024-12-12T02:22:38.502Z
Learning: Only the CTO and Security Architect can use the force-merge PR bypass audit action, and it will be documented and communicated to prevent unauthorized use.
Applied to files:
src/Facets/EcoFacet.sol
📚 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/_deployments_log_file.json
📚 Learning: 2025-09-15T12:33:51.069Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1377
File: deployments/_deployments_log_file.json:5607-5619
Timestamp: 2025-09-15T12:33:51.069Z
Learning: The deployments/_deployments_log_file.json is structured as network → environment → version → array of deployment records. Always understand the file hierarchy before attempting validation, and target specific paths rather than trying to validate all objects in the JSON.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-01-16T00:24:10.002Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#927
File: foundry.toml:78-78
Timestamp: 2025-01-16T00:24:10.002Z
Learning: Avalanche chain (chainid=43114) is supported by Etherscan V2 API and can use the endpoint https://api.etherscan.io/v2/api?chainid=43114 instead of Snowtrace.
Applied to files:
deployments/_deployments_log_file.json
📚 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
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 test/solidity/**/*.t.sol : Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: test/solidity/Facets/EcoFacet.t.sol:135-141
Timestamp: 2025-09-16T07:56:45.093Z
Learning: In EcoFacet test setup, the isNative parameter in swap-and-bridge flows refers to the bridge token type (output), not the swap input type. The ERC20 branch (isNative=false) can still swap from native ETH to ERC20 tokens, requiring msg.value = swapData[0].fromAmount for the ETH input to the swap.
Applied to files:
test/solidity/Facets/EcoFacet.t.soldocs/EcoFacet.md
📚 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:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-08-29T11:07:57.743Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.
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 script/**/*.ts : Prefer existing helper functions over reimplementation (e.g., getDeployments, getProvider, getWalletFromPrivateKeyInDotEnv, sendTransaction, ensureBalanceAndAllowanceToDiamond, getUniswapData*)
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-19T05:10:12.922Z
Learnt from: ezynda3
PR: lifinance/contracts#1303
File: script/utils/network.ts:27-32
Timestamp: 2025-08-19T05:10:12.922Z
Learning: In the lifinance/contracts repository, the team decided to standardize RPC environment variable naming by converting all hyphens to underscores in network names. For example, "tron-shasta" becomes "ETH_NODE_URI_TRON_SHASTA". The getRPCEnvVarName() helper in script/utils/network.ts implements this standard and should be used consistently across the codebase instead of direct string manipulation.
Applied to files:
script/demoScripts/demoEco.ts
📚 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 script/**/*.ts : Always use proper TypeChain types (e.g., ILiFi.BridgeDataStruct) and never use any for bridge data or contract structures
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-05-27T16:19:16.726Z
Learnt from: mirooon
PR: lifinance/contracts#1165
File: src/Facets/GnosisBridgeFacet.sol:22-22
Timestamp: 2025-05-27T16:19:16.726Z
Learning: The correct USDS token address on Ethereum mainnet is 0xdC035D45d973E3EC169d2276DDab16f1e407384F, not 0xA4Bdb11dc0a2bEC88d24A3aa1E6Bb17201112eBe.
Applied to files:
script/demoScripts/demoEco.ts
📚 Learning: 2025-08-19T05:07:19.448Z
Learnt from: ezynda3
PR: lifinance/contracts#1303
File: script/deploy/tron/utils.ts:3-5
Timestamp: 2025-08-19T05:07:19.448Z
Learning: In the lifinance/contracts repository, consola and TronWeb should be imported as named exports: `import { consola } from 'consola'` and `import { TronWeb } from 'tronweb'`. Do not flag these as incorrect default export usage.
Applied to files:
script/demoScripts/demoEco.ts
🧬 Code graph analysis (1)
script/demoScripts/demoEco.ts (2)
script/demoScripts/utils/demoScriptHelpers.ts (21)
ADDRESS_USDC_OPT(61-61)ADDRESS_USDC_BASE(75-75)ADDRESS_USDC_ARB(63-63)ADDRESS_USDC_ETH(57-57)ADDRESS_USDC_POL(59-59)ADDRESS_USDC_SOL(67-67)ADDRESS_WETH_OPT(70-70)ADDRESS_WETH_BASE(73-73)ADDRESS_WETH_ARB(72-72)ADDRESS_WETH_ETH(69-69)ADDRESS_WETH_POL(71-71)ADDRESS_UNISWAP_OPT(81-81)ADDRESS_UNISWAP_BASE(83-83)ADDRESS_UNISWAP_ARB(82-82)ADDRESS_UNISWAP_ETH(78-78)ADDRESS_UNISWAP_POL(80-80)setupEnvironment(653-707)getUniswapDataERC20toExactERC20(334-406)ensureBalance(846-867)ensureAllowance(872-906)executeTransaction(805-841)script/common/types.ts (1)
SupportedChain(3-3)
🪛 markdownlint-cli2 (0.18.1)
docs/EcoFacet.md
107-107: Link text should be descriptive
(MD059, descriptive-link-text)
113-113: Link text should be descriptive
(MD059, descriptive-link-text)
144-144: Link text should be descriptive
(MD059, descriptive-link-text)
🔇 Additional comments (4)
config/eco.json (2)
29-31: TRON entry uses an EVM-style address — confirm intentIf TRON is only a destination (not a source chain running the facet), a portal address here may be unused. If it is used, confirm Eco Portal exists on TRON and that 0x399D…e97 is valid on TRON’s addressing scheme.
2-31: Ignore identical portal address across networks
This CREATE2 deployment yields the same portal address on each chain by design. No changes needed.Likely an incorrect or invalid review comment.
docs/EcoFacet.md (1)
97-100: Chain ID mappings verified
Constants in EcoFacet.sol (ECO_CHAIN_ID_TRON = 728126428, ECO_CHAIN_ID_SOLANA = 1399811149) match the documentation and IEcoPortal interface usage.src/Facets/EcoFacet.sol (1)
213-229: Event order and unused return are acceptable hereEmitting after external call and not checking publishAndFund’s return is an intentional choice with nonReentrant protection and portal semantics.
Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployments/optimism.diamond.staging.json (1)
148-187: Duplicate AcrossFacet entries remain — resolve ambiguity.There are still duplicate facet entries with identical Name and Version at different addresses:
AcrossFacetPackedV4 1.0.0:
- 0x8962d191Ba0f2Bc29b949ACA222f8251B241190b (Line 148)
- 0xf536ed5A4310455FF39dBf90336e17d11550E7b4 (Line 180)
AcrossFacetV4 1.0.0:
- 0xc2A0D799744536C621Af9B2933CdB4Ad959980bF (Line 164)
- 0x91559A75bd9045681265C77922b3cAeDB3D5120d (Line 176)
Per previous feedback, EcoFacet duplicates were marked as "removed" but these AcrossFacet duplicates persist. Deployment tooling that selects facets by Name/Version cannot disambiguate between these entries.
Recommended action: For each duplicate pair, identify the correct/active deployment address and remove the obsolete entry, or suffix the Name field of inactive entries (e.g., "AcrossFacetV4_Deprecated") to disambiguate.
Based on learnings.
🧹 Nitpick comments (1)
docs/EcoFacet.md (1)
105-142: Replace “here” links with descriptive text.Multiple instances link the word “here,” which hurts accessibility and was flagged by markdownlint (MD059). Please rewrite the link text to describe the destination (e.g., “LibSwap library”).
📜 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 (7)
deployments/_deployments_log_file.json(1 hunks)deployments/optimism.diamond.staging.json(5 hunks)deployments/optimism.staging.json(1 hunks)docs/EcoFacet.md(1 hunks)script/demoScripts/demoEco.ts(1 hunks)src/Facets/EcoFacet.sol(1 hunks)test/solidity/Facets/EcoFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/demoScripts/demoEco.ts
🧰 Additional context used
📓 Path-based instructions (6)
{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.soltest/solidity/Facets/EcoFacet.t.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.soltest/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
{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
🧠 Learnings (47)
📚 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 : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.soldocs/EcoFacet.md
📚 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.soldeployments/optimism.staging.json
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:186-188
Timestamp: 2025-07-16T01:04:55.857Z
Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.
Applied to files:
src/Facets/EcoFacet.soltest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
src/Facets/EcoFacet.sol
📚 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:
src/Facets/EcoFacet.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:
src/Facets/EcoFacet.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 : Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Applied to files:
src/Facets/EcoFacet.soltest/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:
src/Facets/EcoFacet.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:
src/Facets/EcoFacet.soldocs/EcoFacet.md
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
PR: lifinance/contracts#1406
File: src/Facets/UnitFacet.sol:75-81
Timestamp: 2025-10-02T18:10:09.934Z
Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.
Applied to files:
src/Facets/EcoFacet.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 : Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `Permit2Proxy` contract (`src/Periphery/Permit2Proxy.sol`), reentrancy protection is not necessary for functions like `callDiamondWithEIP2612Signature` when calling our own trusted diamond contract (`LIFI_DIAMOND`).
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-08-27T13:51:52.704Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:33-0
Timestamp: 2025-08-27T13:51:52.704Z
Learning: In LDA facets like NativeWrapperFacet, mirooon prefers to rely on CoreRouteFacet's upstream validation for parameters like `from` rather than adding defensive validation checks in individual facets, since CoreRouteFacet ensures only valid values (msg.sender, address(this), or INTERNAL_INPUT_SOURCE) are passed to the facets.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
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.soltest/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-02-21T09:00:28.226Z
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` already validates that the receiver address is not zero, making additional zero-address checks redundant in functions using this modifier.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-02-21T09:00:28.226Z
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Facets/ChainflipFacet.sol:127-146
Timestamp: 2025-02-21T09:00:28.226Z
Learning: The `validateBridgeData` modifier in `Validatable.sol` validates that the receiver address is not zero using `LibUtil.isZeroAddress`, making additional zero-address checks redundant in functions using this modifier.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-11-22T08:49:35.205Z
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2025-09-30T12:00:56.667Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: src/Facets/EcoFacet.sol:1-1
Timestamp: 2025-09-30T12:00:56.667Z
Learning: In the lifinance/contracts repository, audit logging is handled manually by the team, and merging is automatically restricted until an audit has been logged. Missing audit entries in auditLog.json should not be flagged as critical blocking issues during review, as the team has process controls in place to prevent merging without proper audits.
Applied to files:
src/Facets/EcoFacet.sol
📚 Learning: 2024-12-12T02:22:38.502Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#902
File: .github/workflows/forceMergePRBypassAudit.yml:0-0
Timestamp: 2024-12-12T02:22:38.502Z
Learning: Only the CTO and Security Architect can use the force-merge PR bypass audit action, and it will be documented and communicated to prevent unauthorized use.
Applied to files:
src/Facets/EcoFacet.sol
📚 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.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: 2025-09-15T12:33:51.069Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1377
File: deployments/_deployments_log_file.json:5607-5619
Timestamp: 2025-09-15T12:33:51.069Z
Learning: The deployments/_deployments_log_file.json is structured as network → environment → version → array of deployment records. Always understand the file hierarchy before attempting validation, and target specific paths rather than trying to validate all objects in the JSON.
Applied to files:
deployments/_deployments_log_file.json
📚 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/_deployments_log_file.jsondeployments/optimism.diamond.staging.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-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-01-16T00:24:10.002Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#927
File: foundry.toml:78-78
Timestamp: 2025-01-16T00:24:10.002Z
Learning: Avalanche chain (chainid=43114) is supported by Etherscan V2 API and can use the endpoint https://api.etherscan.io/v2/api?chainid=43114 instead of Snowtrace.
Applied to files:
deployments/_deployments_log_file.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: 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: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/fantom.diamond.json:92-94
Timestamp: 2024-11-26T01:04:16.637Z
Learning: In `deployments/*.diamond.json` files, it's acceptable for facets to have empty `Name` and `Version` fields, and these should not be flagged as issues.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1381
File: deployments/arbitrum.json:65-69
Timestamp: 2025-09-16T01:39:54.099Z
Learning: When verifying facet deployments across .json and .diamond.json files, search by facet name rather than trying to cross-reference addresses between the files, as the same contract can have different addresses in different deployment files.
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 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/optimism.diamond.staging.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: 2024-11-26T01:14:58.163Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:5-63
Timestamp: 2024-11-26T01:14:58.163Z
Learning: When facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report this issue, as it is already known and will be addressed separately.
Applied to files:
deployments/optimism.diamond.staging.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: 2024-10-09T03:47:21.269Z
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.
Applied to files:
deployments/optimism.diamond.staging.json
📚 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
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
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 test/solidity/**/*.t.sol : Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
PR: lifinance/contracts#1324
File: test/solidity/Facets/EcoFacet.t.sol:135-141
Timestamp: 2025-09-16T07:56:45.093Z
Learning: In EcoFacet test setup, the isNative parameter in swap-and-bridge flows refers to the bridge token type (output), not the swap input type. The ERC20 branch (isNative=false) can still swap from native ETH to ERC20 tokens, requiring msg.value = swapData[0].fromAmount for the ETH input to the swap.
Applied to files:
test/solidity/Facets/EcoFacet.t.soldocs/EcoFacet.md
📚 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:
test/solidity/Facets/EcoFacet.t.sol
📚 Learning: 2025-08-29T11:07:57.743Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.
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 native fees in facets, use the _depositAndSwap variant and reserve required fees before execution
Applied to files:
test/solidity/Facets/EcoFacet.t.sol
🪛 markdownlint-cli2 (0.18.1)
docs/EcoFacet.md
105-105: Link text should be descriptive
(MD059, descriptive-link-text)
111-111: Link text should be descriptive
(MD059, descriptive-link-text)
142-142: Link text should be descriptive
(MD059, descriptive-link-text)
🔇 Additional comments (3)
deployments/_deployments_log_file.json (1)
43049-43062: Confirm EcoFacet staging verification status.
TheVERIFIEDflag remains"false", but deployment logs are meant to mirror the post‑deployment state. If the contract has since been verified on Optimism staging, please update the log (and include any missing metadata like verification URL/TX if applicable); otherwise call out why verification is blocked so we have a clear audit trail.src/Facets/EcoFacet.sol (1)
1-316: LGTM! EcoFacet implementation looks solid.The facet follows all required conventions and guidelines:
✅ Proper license identifier and pragma
✅ Required imports (ILiFi, LibAsset, LibSwap, security mixins)
✅ Required entry points (startBridgeTokensViaEco, swapAndStartBridgeTokensViaEco)
✅ Internal _startBridge implementation
✅ Appropriate modifiers (nonReentrant, refundExcessNative, validateBridgeData, etc.)
✅ Custom errors for gas optimization
✅ Constants defined for magic numbers
✅ Comprehensive validation logic for EVM/non-EVM chains
✅ NatSpec documentation for all public functions
✅ Immutable PORTAL variable with constructor validation
✅ Consolidated validation logic (previous gas optimization feedback addressed)
✅ Proper event emissionsThe implementation handles cross-chain bridging for EVM chains (including Tron), Solana, and validates receivers appropriately. The team's preference to use msg.sender for native refunds is documented in learnings.
As per learnings.
test/solidity/Facets/EcoFacet.t.sol (1)
1-924: LGTM! Comprehensive test coverage for EcoFacet.The test suite is well-structured and provides thorough coverage:
✅ Proper test naming conventions (testRevert_, testBase_, test_)
✅ Extends TestBaseFacet correctly
✅ Comprehensive setUp() with forked environment, liquidity, and facet wiring
✅ Helper functions (_getValidEcoData, _createEncodedRoute, initiate* dispatchers)
✅ Tests for invalid configurations and error codes
✅ Native token rejection tests (both bridge and swap flows)
✅ EVM chain bridging tests (Optimism)
✅ Tron bridging tests (EVM-compatible)
✅ Solana bridging tests with encoded routes and ATA validation
✅ Chain ID boundary tests (uint64.max, overflow)
✅ Route validation tests (empty routes, invalid ABI encoding, receiver mismatches)
✅ ATA validation tests (zero ATA, mismatched ATA)
✅ Event emission verification
✅ Balance change assertions with TOKEN_SOLVER_REWARD handlingThe test coverage addresses both positive and negative scenarios, including edge cases for different chain types and validation paths.
As per learnings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deployments/_deployments_log_file.json (1)
43365-43376: Update EcoFacet optimism-staging deployment to verifiedThis new staging deployment is recorded as unverified. We normally require explorer verification for freshly deployed facets before merging deployment metadata; otherwise reviewers and integrators can't inspect the bytecode source, which is a security and ops blocker. Please complete the contract verification on Optimism and flip
"VERIFIED"to"true"(with any other verification metadata, if applicable) before merging.
🧹 Nitpick comments (1)
audit/auditLog.json (1)
389-400: Restore chronological ordering for audit entries.
audit20251001should sit beforeaudit20251007to keep theauditsmap in ascending date order like the rest of the file. Please move the block up.- "audit20251007": { - "auditCompletedOn": "07.10.2025", - "auditedBy": "Sujith Somraaj (individual security researcher)", - "auditorGitHandle": "sujithsomraaj", - "auditReportPath": "./audit/reports/2025.10.07_UnitFacet(v1.0.0).pdf", - "auditCommitHash": "626dd4a927ed84c1c8de9e6b22af66669da62356" - }, - "audit20251001": { + "audit20251001": { "auditCompletedOn": "01.10.2025", "auditedBy": "Sujith Somraaj (individual security researcher)", "auditorGitHandle": "sujithsomraaj", "auditReportPath": "./audit/reports/2025.10.01_EcoFacet(v1.0.0).pdf", "auditCommitHash": "8d0e66a69c765e83c13339d7a77071ce2f26968a" + }, + "audit20251007": { + "auditCompletedOn": "07.10.2025", + "auditedBy": "Sujith Somraaj (individual security researcher)", + "auditorGitHandle": "sujithsomraaj", + "auditReportPath": "./audit/reports/2025.10.07_UnitFacet(v1.0.0).pdf", + "auditCommitHash": "626dd4a927ed84c1c8de9e6b22af66669da62356" }
📜 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)
audit/reports/2025.10.01_EcoFacet(v1.0.0).pdfis excluded by!**/*.pdf
📒 Files selected for processing (3)
audit/auditLog.json(3 hunks)deployments/_deployments_log_file.json(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
audit/auditLog.json
📄 CodeRabbit inference engine (conventions.md)
Maintain auditLog.json with audits entries (id, dates, auditor, report path, commit) and auditedContracts mapping
Files:
audit/auditLog.json
🧠 Learnings (4)
📚 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 audit/auditLog.json : Maintain auditLog.json with audits entries (id, dates, auditor, report path, commit) and auditedContracts mapping
Applied to files:
audit/auditLog.json
📚 Learning: 2025-09-15T12:33:51.069Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1377
File: deployments/_deployments_log_file.json:5607-5619
Timestamp: 2025-09-15T12:33:51.069Z
Learning: The deployments/_deployments_log_file.json is structured as network → environment → version → array of deployment records. Always understand the file hierarchy before attempting validation, and target specific paths rather than trying to validate all objects in the JSON.
Applied to files:
deployments/_deployments_log_file.json
📚 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/_deployments_log_file.json
📚 Learning: 2025-01-16T00:24:10.002Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#927
File: foundry.toml:78-78
Timestamp: 2025-01-16T00:24:10.002Z
Learning: Avalanche chain (chainid=43114) is supported by Etherscan V2 API and can use the endpoint https://api.etherscan.io/v2/api?chainid=43114 instead of Snowtrace.
Applied to files:
deployments/_deployments_log_file.json
⏰ 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
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!!!)