LF-14786 Everclear [EverclearFacet v1.0.0, IEverclearFeeAdapter v1.0.0] #1413
Conversation
…e identifier, enhanced EverclearData struct with additional parameters, and added validation checks for receiver addresses. Introduced new unit tests for error handling on invalid output assets and receiver addresses
…Everclear protocol description, refined the EverclearData struct with detailed parameters, and added error handling for bridging to non-EVM chains. Introduced new unit tests for bridging functionality and validation of receiver addresses.
…meter to EverclearData struct, updated transaction execution to include native fees, and enhanced demo script for bridging functionality. Introduced unit tests to validate behavior with native assets and ensure proper error handling.
…clearFacet tests: Deleted MessageHashUtils.sol, updated EverclearFacetTest to include a local implementation of the toEthSignedMessageHash function, and added a MockEverclearFeeAdapter for testing native fee handling.
…ployed address in JSON logs, added support for Non-EVM chain handling in demo script, and refined ABI decoding logic for better compatibility with both EVM and Non-EVM chains
… from Arbitrum to Solana to enhance logging and traceability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// @dev Contains the business logic for the bridge via Everclear | ||
| /// @param _bridgeData The core information needed for bridging | ||
| /// @param _everclearData Data specific to Everclear | ||
| function _startBridge( |
Check notice
Code scanning / Olympix Integrated Security
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
| } | ||
|
|
||
| // destination chain is non-EVM | ||
| FEE_ADAPTER.newIntent{ value: _everclearData.nativeFee }( // value is ONLY the fee for the intent, FEE_ADAPTER does NOT handle the native token as an asset |
Check warning
Code scanning / Olympix Integrated Security
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
| // destination chain is EVM | ||
| // make sure that bridgeData and everclearData receiver addresses match | ||
| if ( | ||
| bytes32(uint256(uint160(_bridgeData.receiver))) != |
Check notice
Code scanning / Olympix Integrated Security
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
| uint32[] memory destinationChainIds = new uint32[](1); | ||
| destinationChainIds[0] = uint32(_bridgeData.destinationChainId); | ||
|
|
||
| FEE_ADAPTER.newIntent{ value: _everclearData.nativeFee }( // value is ONLY the fee for the intent, FEE_ADAPTER does NOT handle the native token as an asset |
Check warning
Code scanning / Olympix Integrated Security
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
WalkthroughAdds Everclear cross‑chain integration: new EverclearFacet contract and IEverclearFeeAdapter interface, network config and deploy metadata, deploy/update scripts and resource entry, tests, docs, demo scripts (including Solana helpers), template tweak, and deployment log updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
Test Coverage ReportLine Coverage: 85.83% (2932 / 3416 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/Facets/EverclearV2Facet.sol (2)
195-196: bytes32 → address downcastThis narrows to 160 bits. Team has previously accepted this risk; ensure input bytes32 is a left‑padded address.
Would you like an assertion added to validate the top 12 bytes are zero before casting?
208-209: Event emission after external call is intentional herePer team convention, LiFiTransferStarted is emitted at the end of _startBridge. The nonReentrant guard is on the entrypoints.
Based on learnings.
src/Facets/EverclearFacet.sol (1)
208-209: Event emission after external call is intentional and accepted.This pattern was previously discussed and accepted with nonReentrant-protected entrypoints; event is emitted after successful external interactions.
Based on learnings
🧹 Nitpick comments (12)
script/demoScripts/utils/demoScriptHelpers.ts (1)
97-99: Consider consolidating duplicate address padding functions.The new
addressToBytes32LeftPaddedfunction (lines 97-99) duplicates the functionality of the existingleftPadAddressToBytes32function (lines 92-95). Both convert an address to a 32-byte left-padded hex string, just using different approaches:
leftPadAddressToBytes32: Manual zero prefix concatenationaddressToBytes32LeftPadded: UsingpadStart(64, '0')Consider one of the following:
- Remove the duplicate: Keep only one implementation (preferably
padStartfor clarity)- Consolidate: Deprecate one and update all call sites to use a single function
- Document: If both are needed for specific use cases, add comments explaining when to use each
Example consolidation:
-export const leftPadAddressToBytes32 = (address: string): string => { - // Convert address to bytes32 format: pad with zeros to make it 32 bytes - return '0x000000000000000000000000' + address.slice(2) -} - -export const addressToBytes32LeftPadded = (address: string): string => { - return '0x' + address.slice(2).padStart(64, '0') -} +/** + * Convert an Ethereum address to a 32-byte left-padded hex string. + * The address is stripped of its "0x" prefix and zero-padded to 64 characters. + */ +export const addressToBytes32LeftPadded = (address: string): string => { + return '0x' + address.slice(2).padStart(64, '0') +}If keeping both, verify all existing call sites of
leftPadAddressToBytes32and update them to use the new function.script/demoScripts/demoEverclear.ts (2)
114-126: Harden API calls: check response.ok and handle errorsCurrent code assumes 200 OK and may throw opaque JSON errors. Guard all fetches.
Example:
- const quoteResp = await fetch(`https://api.everclear.org/routes/quotes`, { ... }) - const quoteData = await quoteResp.json() + const quoteResp = await fetch(`https://api.everclear.org/routes/quotes`, { ... }) + if (!quoteResp.ok) { + const err = await quoteResp.text().catch(() => '') + throw new Error(`Quote request failed (${quoteResp.status}): ${err}`) + } + const quoteData = await quoteResp.json()Repeat for the two /intents calls.
Also applies to: 130-145, 216-231
201-206: Avoid any; prefer typed contract or narrow call surfaceCasting lifiDiamondContract to any loses safety. Prefer a typed write call (e.g., via a viem type helper or a small wrapper) so arg/struct validation is enforced at compile time.
docs/EverclearFacet.md (1)
79-79: Improve link text per MD059Replace “here” links with descriptive text.
- The swap library can be found [here](../src/Libraries/LibSwap.sol). + See the swap library implementation in [LibSwap.sol](../src/Libraries/LibSwap.sol). - It's used to emit events ... can be found [here](../src/Interfaces/ILiFi.sol). + See [ILiFi.sol](../src/Interfaces/ILiFi.sol) for BridgeData and events. - A detailed explanation ... can be found [here](https://docs.li.fi/products/more-integration-options/li.fi-api/transferring-tokens-example). + See the guide: [Transferring tokens with the Li.Fi API](https://docs.li.fi/products/more-integration-options/li.fi-api/transferring-tokens-example).Also applies to: 85-85, 116-116
src/Interfaces/IEverclearFeeAdapterV2.sol (3)
4-6: Fix NatSpec header order and add missing @notice.Interfaces must include NatSpec in order: @title, @notice, @author, @Custom:version.
As per coding guidelines
-/// @title Interface for Everclear Fee Adapter V2 -/// @author LI.FI (https://li.fi) -/// @custom:version 1.0.0 +/// @title Interface for Everclear Fee Adapter V2 +/// @notice Defines the Everclear V2 fee adapter API (intents, fees, and overloads for EVM/non‑EVM identifiers) +/// @author LI.FI (https://li.fi) +/// @custom:version 1.0.0
14-28: Align param docs with actual types (bytes32 identifiers).Docstrings reference “address” while fields are bytes32 (supporting non‑EVM). Update wording to avoid confusion.
As per coding guidelines
- * @param receiver The address of the intent receiver - * @param inputAsset The address of the intent asset on origin - * @param outputAsset The address of the intent asset on destination + * @param receiver The receiver identifier (bytes32; EVM addresses are left‑padded) + * @param inputAsset The origin asset identifier (bytes32; EVM address left‑padded) + * @param outputAsset The destination asset identifier (bytes32; EVM address left‑padded)
44-66: Optional: Add brief NatSpec for the overloads.A one-liner purpose + param summaries for both newIntent overloads will help consumers and satisfy stricter linters.
src/Facets/EverclearFacet.sol (2)
81-89: Use explicit refund address parameter instead of msg.sender.Guideline: do not use msg.sender as refund address. Add a refundAddress param and pass it to refundExcessNative to avoid refunding an aggregator or unintended caller.
As per coding guidelines
Example:
- function startBridgeTokensViaEverclear( - ILiFi.BridgeData memory _bridgeData, - EverclearData calldata _everclearData - ) + function startBridgeTokensViaEverclear( + ILiFi.BridgeData memory _bridgeData, + EverclearData calldata _everclearData, + address payable refundAddress + ) external payable nonReentrant - refundExcessNative(payable(msg.sender)) + refundExcessNative(refundAddress)Apply similarly to swapAndStartBridgeTokensViaEverclear.
Also applies to: 106-114
181-199: Validate bytes32<->address narrowing to prevent truncation.When comparing/deriving EVM addresses from bytes32, ensure high bits are zero. Otherwise, implicit truncation may mask malformed inputs.
Example preconditions for the EVM branch:
// Receiver: bytes32(uint160(receiver)) == receiverAddress uint256 recv32 = uint256(_everclearData.receiverAddress); require(recv32 >> 160 == 0, "InvalidReceiverBytes32"); // Output asset: address from bytes32 uint256 out32 = uint256(_everclearData.outputAsset); require(out32 >> 160 == 0, "InvalidOutputAssetBytes32");If this risk is an accepted tradeoff (as with prior facets), please confirm the Everclear adapter/API guarantees canonical encoding. Based on learnings
src/Interfaces/IEverclearFeeAdapter.sol (2)
4-6: Fix NatSpec header order and add missing @notice.Interfaces must include NatSpec in order: @title, @notice, @author, @Custom:version.
As per coding guidelines
-/// @title Interface for Everclear Fee Adapter -/// @author LI.FI (https://li.fi) -/// @custom:version 1.0.0 +/// @title Interface for Everclear Fee Adapter +/// @notice Defines the Everclear fee adapter API (intents, fees, and EVM/non‑EVM overloads) +/// @author LI.FI (https://li.fi) +/// @custom:version 1.0.0
14-28: Clarify Intent docs to reflect canonical bytes32 fields.Fields are bytes32 (canonicalized addresses or chain‑specific IDs). Update wording to avoid implying Solidity address type.
- * @param receiver The address of the intent receiver - * @param inputAsset The address of the intent asset on origin - * @param outputAsset The address of the intent asset on destination + * @param receiver The receiver identifier (bytes32; EVM addresses are left‑padded) + * @param inputAsset The origin asset identifier (bytes32; EVM address left‑padded) + * @param outputAsset The destination asset identifier (bytes32; EVM address left‑padded)test/solidity/Facets/EverclearFacet.t.sol (1)
811-863: Add explicit test for fee > minAmount revert (if guard is added).Once FeeExceedsAmount guard is in place, add a unit test asserting the specific revert when fee exceeds minAmount to lock in UX behavior.
I can draft the test diff after you confirm the chosen error name.
📜 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 (18)
config/everclear.json(1 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)docs/EverclearFacet.md(1 hunks)script/demoScripts/demoEverclear.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(1 hunks)script/deploy/facets/DeployEverclearFacet.s.sol(1 hunks)script/deploy/facets/UpdateEverclearFacet.s.sol(1 hunks)script/deploy/tron/deploy-and-register-everclear-facet.ts(1 hunks)src/Facets/EverclearFacet.sol(1 hunks)src/Facets/EverclearV2Facet.sol(1 hunks)src/Interfaces/IEverclearFeeAdapter.sol(1 hunks)src/Interfaces/IEverclearFeeAdapterV2.sol(1 hunks)src/Periphery/LiFiDEXAggregator.sol(1 hunks)templates/facet.template.hbs(2 hunks)test/solidity/Facets/EverclearFacet.t.sol(1 hunks)test/solidity/utils/MockEverclearFeeAdapter.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
{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/Periphery/LiFiDEXAggregator.solsrc/Interfaces/IEverclearFeeAdapter.solsrc/Interfaces/IEverclearFeeAdapterV2.solscript/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.soltest/solidity/Facets/EverclearFacet.t.soltest/solidity/utils/MockEverclearFeeAdapter.solsrc/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.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/Periphery/LiFiDEXAggregator.solsrc/Interfaces/IEverclearFeeAdapter.solsrc/Interfaces/IEverclearFeeAdapterV2.solsrc/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Periphery/LiFiDEXAggregator.solsrc/Interfaces/IEverclearFeeAdapter.solsrc/Interfaces/IEverclearFeeAdapterV2.solscript/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.soltest/solidity/Facets/EverclearFacet.t.soltest/solidity/utils/MockEverclearFeeAdapter.solsrc/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.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/EverclearFacet.md
src/Interfaces/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/**/*.sol: All interfaces must reside under src/Interfaces, be in separate files from implementations, and use an I prefix (e.g., ILiFi)
Every interface must include NatSpec in this order: @title, @notice, @author LI.FI (https://li.fi), @Custom:version X.Y.Z
Files:
src/Interfaces/IEverclearFeeAdapter.solsrc/Interfaces/IEverclearFeeAdapterV2.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/utils/demoScriptHelpers.tsscript/deploy/tron/deploy-and-register-everclear-facet.tsscript/demoScripts/demoEverclear.ts
script/deploy/facets/Update*.s.sol
📄 CodeRabbit inference engine (conventions.md)
Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Files:
script/deploy/facets/UpdateEverclearFacet.s.sol
script/deploy/facets/Deploy*.s.sol
📄 CodeRabbit inference engine (conventions.md)
script/deploy/facets/Deploy*.s.sol: Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Files:
script/deploy/facets/DeployEverclearFacet.s.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/EverclearFacet.t.sol
src/Facets/**/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/**/*Facet.sol: Facet contracts must be located in src/Facets and include "Facet" in the contract name
Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Do not use msg.sender as refund address; require an explicit parameter for refund address
In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
For native fees in facets, use the _depositAndSwap variant and reserve required fees before execution
For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Files:
src/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.sol
templates/**
📄 CodeRabbit inference engine (conventions.md)
Store all code generation templates for plop facet under templates/
Files:
templates/facet.template.hbs
🧠 Learnings (40)
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2024-10-14T08:23:38.076Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-27T23:01:41.042Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.
Applied to files:
src/Periphery/LiFiDEXAggregator.sol
📚 Learning: 2025-08-27T23:01:41.042Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In modern OpenZeppelin versions (v4.0+), SafeERC20.sol re-exports IERC20, making the combined import `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` completely valid and working. Do not suggest splitting these imports as they work correctly as a combined import.
Applied to files:
src/Periphery/LiFiDEXAggregator.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/arbitrum.staging.jsondeployments/arbitrum.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/arbitrum.staging.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:
docs/EverclearFacet.mdtest/solidity/Facets/EverclearFacet.t.solsrc/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.soltemplates/facet.template.hbs
📚 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/deploy/facets/Deploy*.s.sol : Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.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/deploy/facets/Update*.s.sol : Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.sol
📚 Learning: 2025-08-26T15:19:07.800Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
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/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-10-02T18:14:45.047Z
Learnt from: mirooon
PR: lifinance/contracts#1406
File: script/deploy/facets/UpdateUnitFacet.s.sol:1-3
Timestamp: 2025-10-02T18:14:45.047Z
Learning: For update scripts in script/deploy/facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/UpdateUnitFacet.s.sol.
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.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/deploy/facets/Deploy*.s.sol : When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-09-25T00:04:16.437Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: script/deploy/zksync/DeployFeeForwarder.zksync.s.sol:4-4
Timestamp: 2025-09-25T00:04:16.437Z
Learning: ZkSync deployment scripts have their own DeployScriptBase.sol located at script/deploy/zksync/utils/DeployScriptBase.sol, and all ZkSync deployment scripts use the relative import path "./utils/DeployScriptBase.sol" to reference it, not "../facets/utils/DeployScriptBase.sol".
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.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/DeployEverclearFacet.s.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:
test/solidity/Facets/EverclearFacet.t.solsrc/Facets/EverclearV2Facet.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:
test/solidity/Facets/EverclearFacet.t.solsrc/Facets/EverclearV2Facet.solsrc/Facets/EverclearFacet.soltemplates/facet.template.hbs
📚 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/EverclearFacet.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/EverclearFacet.t.sol
📚 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-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-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/arbitrum.diamond.staging.json
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-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/arbitrum.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/arbitrum.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/arbitrum.diamond.staging.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 : 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/EverclearV2Facet.solsrc/Facets/EverclearFacet.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/EverclearV2Facet.sol
📚 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/EverclearV2Facet.solsrc/Facets/EverclearFacet.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/EverclearV2Facet.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/EverclearV2Facet.solsrc/Facets/EverclearFacet.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/EverclearV2Facet.solsrc/Facets/EverclearFacet.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 be located in src/Facets and include "Facet" in the contract name
Applied to files:
templates/facet.template.hbs
📚 Learning: 2025-10-02T18:14:51.385Z
Learnt from: mirooon
PR: lifinance/contracts#1406
File: src/Facets/UnitFacet.sol:1-2
Timestamp: 2025-10-02T18:14:51.385Z
Learning: For facet contracts in src/Facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for src/Facets/UnitFacet.sol.
Applied to files:
templates/facet.template.hbs
📚 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:
templates/facet.template.hbs
📚 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:
templates/facet.template.hbs
🧬 Code graph analysis (2)
script/deploy/tron/deploy-and-register-everclear-facet.ts (5)
script/deploy/tron/utils.ts (12)
getEnvironment(131-138)validateBalance(1115-1130)hexToTronAddress(1098-1103)confirmDeployment(1135-1173)checkExistingDeployment(665-695)getContractVersion(289-310)deployContractWithLogging(699-753)printDeploymentSummary(1178-1216)getContractAddress(221-240)getFacetSelectors(610-660)displayRegistrationInfo(1251-1271)registerFacetToDiamond(880-1040)script/demoScripts/utils/demoScriptHelpers.ts (2)
getEnvVar(591-597)getPrivateKeyForEnvironment(918-924)script/deploy/tron/types.ts (2)
ITronDeploymentConfig(1-12)IDeploymentResult(66-73)script/deploy/tron/TronContractDeployer.ts (1)
TronContractDeployer(18-529)script/deploy/tron/constants.ts (1)
MIN_BALANCE_WARNING(14-14)
script/demoScripts/demoEverclear.ts (2)
script/common/types.ts (1)
SupportedChain(3-3)script/demoScripts/utils/demoScriptHelpers.ts (5)
setupEnvironment(654-708)ensureBalance(847-868)ensureAllowance(873-907)addressToBytes32LeftPadded(97-99)executeTransaction(806-842)
🪛 GitHub Check: Olympix Integrated Security
src/Facets/EverclearV2Facet.sol
[notice] 129-129:
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] 158-158:
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
[notice] 184-184:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[warning] 191-191:
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
src/Facets/EverclearFacet.sol
[notice] 129-129:
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] 158-158:
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
[notice] 184-184:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
[warning] 191-191:
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
🪛 markdownlint-cli2 (0.18.1)
docs/EverclearFacet.md
79-79: Link text should be descriptive
(MD059, descriptive-link-text)
85-85: Link text should be descriptive
(MD059, descriptive-link-text)
116-116: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (14)
src/Periphery/LiFiDEXAggregator.sol (1)
5-6: LGTM: Import reorganization improves clarity.Splitting the IERC20Permit import from the SafeERC20 import is appropriate since IERC20Permit is defined in a separate file (
extensions/IERC20Permit.sol). This makes the import sources explicit and maintains the same functionality.templates/facet.template.hbs (3)
1-1: LGTM: License identifier updated to match guidelines.The license identifier has been corrected from
LGPL-3.0toLGPL-3.0-only, which aligns with the coding guidelines requirement for first-party Solidity files.As per coding guidelines
5-7: LGTM: Essential library imports added.The addition of LibDiamond, LibAsset, and LibSwap imports aligns with facet contract conventions. These libraries are used for:
- LibDiamond: Owner enforcement (line 59)
- LibAsset: Asset handling operations (line 84)
- LibSwap: Swap data structures and operations (line 108)
As per coding guidelines
12-12: LGTM: Title annotation format corrected.Removing the space between the name and "Facet" makes the title consistent with actual facet contract naming conventions (e.g.,
EverclearFacet,AcrossFacet), where the contract name and "Facet" suffix form a single word.script/deploy/facets/UpdateEverclearFacet.s.sol (1)
1-16: LGTM: Update script follows conventions.The update script correctly follows all coding guidelines and conventions:
- ✓ Named
Update{ContractName}.s.sol- ✓ Inherits
UpdateScriptBase- ✓ Calls
update("EverclearFacet")with the correct facet name- ✓ SPDX license identifier immediately followed by pragma
- ✓ Returns
(address[] memory facets, bytes memory cutData)as expectedAs per coding guidelines
deployments/arbitrum.staging.json (1)
64-65: Resolved: EverclearFacet address consistency confirmed across deployment files and logs. Verified presence in deployments/arbitrum.staging.json, deployments/arbitrum.diamond.staging.json, and deployments/_deployments_log_file.json.config/everclear.json (1)
1-10: Verify fee adapter deployment and V2 configuration.
- Confirm the fee adapter at
0x15a7cA97D1ed168fB34a4055CEFa2E2f9Bdb6C75is deployed on Mainnet and Arbitrum (e.g. via eth_getCode RPC or Etherscan/Ethplorer).- If V2 isn’t yet deployed, replace the empty-string value for
feeAdapterV2withnullor omit the field entirely to avoid ambiguity.- Update the config once V2 is live with the correct address.
script/deploy/tron/deploy-and-register-everclear-facet.ts (2)
37-90: Good CLI/script hygieneUses citty, consola, env validation, guarded exits, and retries. Looks solid.
3-6: TronWeb named import is correct TronWeb v6 uses named exports; no change is needed.Likely an incorrect or invalid review comment.
src/Facets/EverclearFacet.sol (2)
158-172: OK to ignore return values here.newIntent returns values but success/failure is conveyed by revert semantics; no action needed. Tool warning can be ignored.
Also applies to: 191-205
44-54: Non‑EVM receiver type differs from guideline (bytes vs bytes32).Guideline suggests bytes for non‑EVM receiverAddress. Here it’s bytes32 (and corresponding event BridgeToNonEVMChainBytes32). Confirm this deviation is intentional for Everclear and documented; otherwise consider switching to bytes for consistency.
As per coding guidelines
src/Interfaces/IEverclearFeeAdapter.sol (1)
29-42: Units: “amount normalized to 18 decimals” — verify producer responsibilities.Facet passes token amounts as-is; confirm the adapter normalizes to 18 decimals internally or require callers to normalize to avoid ambiguity in implementers.
test/solidity/Facets/EverclearFacet.t.sol (1)
247-260: Non‑EVM path coverage looks good.Validation for NON_EVM_ADDRESS, Solana mapping, event assertions, and native fee flows are covered well.
Please confirm Solana Everclear chain ID 1399811149 matches current Everclear routing for the targeted networks.
Also applies to: 791-807
deployments/arbitrum.diamond.staging.json (1)
184-215: Confirm empty facets, EverclearFacet duplicates, and Permit2Proxy update
- Several facet slots in deployments/arbitrum.diamond.staging.json have empty Name/Version entries—confirm these placeholders are intentional or remove them.
- EverclearFacet is registered at three addresses with version 1.0.0—confirm co-deployment of identical versions is expected.
- No references remain to the old Permit2Proxy address; ensure all configs/scripts now point to the new Permit2Proxy address.
…for invalid call data and unsupported chain IDs, changed SPDX license to LGPL-3.0-only in deployment scripts, and removed duplicate EverclearFacet entries from deployment JSON
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/deploy/facets/UpdateEverclearFacet.s.sol (2)
7-7: Inconsistent contract name for update script.The contract is named
DeployScriptbut this is an update script. Consider renaming toUpdateScriptor a more descriptive name that reflects its purpose as an update script rather than a deployment script.
5-8: Remove unused imports and declarations.The
stdJsonimport andusing stdJson for stringdeclaration are not used anywhere in this file. Remove them to reduce code clutter.Apply this diff:
import { UpdateScriptBase } from "./utils/UpdateScriptBase.sol"; -import { stdJson } from "forge-std/StdJson.sol"; contract DeployScript is UpdateScriptBase { - using stdJson for string; - function run()
📜 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 (5)
deployments/arbitrum.diamond.staging.json(1 hunks)script/deploy/facets/DeployEverclearFacet.s.sol(1 hunks)script/deploy/facets/UpdateEverclearFacet.s.sol(1 hunks)src/Facets/EverclearFacet.sol(1 hunks)src/Facets/EverclearV2Facet.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/arbitrum.diamond.staging.json
- src/Facets/EverclearV2Facet.sol
🧰 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/EverclearFacet.solscript/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.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/EverclearFacet.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/EverclearFacet.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
src/Facets/EverclearFacet.solscript/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.sol
script/deploy/facets/Update*.s.sol
📄 CodeRabbit inference engine (conventions.md)
Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Files:
script/deploy/facets/UpdateEverclearFacet.s.sol
script/deploy/facets/Deploy*.s.sol
📄 CodeRabbit inference engine (conventions.md)
script/deploy/facets/Deploy*.s.sol: Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Files:
script/deploy/facets/DeployEverclearFacet.s.sol
🧠 Learnings (27)
📚 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/EverclearFacet.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/EverclearFacet.sol
📚 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/EverclearFacet.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/EverclearFacet.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/EverclearFacet.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/EverclearFacet.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/EverclearFacet.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/EverclearFacet.sol
📚 Learning: 2024-12-02T06:33:33.309Z
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-22T03:14:52.578Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-16T01:04:11.083Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1275
File: src/Facets/AllBridgeFacet.sol:175-175
Timestamp: 2025-07-16T01:04:11.083Z
Learning: In AllBridgeFacet.sol, zero fee validation is not required at the contract level because the backend system ensures that calldata and msg.value are properly aligned before transactions reach the contract.
Applied to files:
src/Facets/EverclearFacet.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/deploy/facets/Deploy*.s.sol : Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.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/deploy/facets/Update*.s.sol : Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.sol
📚 Learning: 2025-10-02T18:14:45.047Z
Learnt from: mirooon
PR: lifinance/contracts#1406
File: script/deploy/facets/UpdateUnitFacet.s.sol:1-3
Timestamp: 2025-10-02T18:14:45.047Z
Learning: For update scripts in script/deploy/facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/UpdateUnitFacet.s.sol.
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-26T15:19:07.800Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
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/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.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/deploy/facets/Deploy*.s.sol : When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-09-25T00:04:16.437Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: script/deploy/zksync/DeployFeeForwarder.zksync.s.sol:4-4
Timestamp: 2025-09-25T00:04:16.437Z
Learning: ZkSync deployment scripts have their own DeployScriptBase.sol located at script/deploy/zksync/utils/DeployScriptBase.sol, and all ZkSync deployment scripts use the relative import path "./utils/DeployScriptBase.sol" to reference it, not "../facets/utils/DeployScriptBase.sol".
Applied to files:
script/deploy/facets/UpdateEverclearFacet.s.solscript/deploy/facets/DeployEverclearFacet.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/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-27T22:29:51.839Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: test/solidity/Periphery/Lda/Facets/SyncSwapV2Facet.t.sol:1-2
Timestamp: 2025-08-27T22:29:51.839Z
Learning: Test files in test/solidity/**/*.t.sol should use the LGPL-3.0-only SPDX license identifier, not Unlicense.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.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,test/solidity,script}/**/*.sol : Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-09-24T23:55:07.082Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: script/deploy/facets/DeployFeeForwarder.s.sol:1-1
Timestamp: 2025-09-24T23:55:07.082Z
Learning: All Solidity files in the lifinance/contracts repository, including deployment scripts in script/deploy/, must use the SPDX license identifier "// SPDX-License-Identifier: LGPL-3.0-only" as specified in conventions.md. This applies to all .sol files in src/, test/solidity/, and script/ directories.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T14:06:33.384Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/UpdateUniV2StyleFacet.s.sol:1-2
Timestamp: 2025-08-29T14:06:33.384Z
Learning: For LDA deployment and update scripts in script/deploy/facets/LDA/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/LDA/UpdateUniV2StyleFacet.s.sol.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T14:05:37.002Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployLDAOwnershipFacet.s.sol:1-3
Timestamp: 2025-08-29T14:05:37.002Z
Learning: For deployment script files in script/deploy/facets/LDA/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/LDA/DeployLDAOwnershipFacet.s.sol.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T14:06:01.072Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/UpdateCurveFacet.s.sol:1-2
Timestamp: 2025-08-29T14:06:01.072Z
Learning: For script/deploy/facets/LDA/UpdateCurveFacet.s.sol and similar LDA update scripts, the SPDX license identifier should be immediately followed by the pragma statement without requiring a blank line in between, as confirmed by mirooon.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T14:05:58.427Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployLDAPeripheryRegistryFacet.s.sol:1-3
Timestamp: 2025-08-29T14:05:58.427Z
Learning: For deployment scripts in script/deploy/facets/LDA/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, following the same convention as other LDA-related files, as confirmed by mirooon for script/deploy/facets/LDA/DeployLDAPeripheryRegistryFacet.s.sol.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T11:18:56.656Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
📚 Learning: 2025-08-29T14:05:31.360Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployLDAEmergencyPauseFacet.s.sol:1-2
Timestamp: 2025-08-29T14:05:31.360Z
Learning: For LDA deployment scripts in script/deploy/facets/LDA/, the SPDX license identifier should be immediately followed by the pragma statement without requiring a blank line in between, as confirmed by mirooon.
Applied to files:
script/deploy/facets/DeployEverclearFacet.s.sol
🔇 Additional comments (9)
script/deploy/facets/UpdateEverclearFacet.s.sol (1)
10-15: LGTM!The
run()function correctly delegates toupdate("EverclearFacet")and returns the facets and cutData as expected for an update script.script/deploy/facets/DeployEverclearFacet.s.sol (3)
1-2: License identifier correctly updated.The SPDX license identifier has been updated to
LGPL-3.0-onlyas required by the coding guidelines, addressing the previous review comment.
8-20: LGTM!The deployment script correctly:
- Extends
DeployScriptBasewith proper contract name- Implements
getConstructorArgs()to read configuration- Deploys using
deploy(type(EverclearFacet).creationCode)pattern- Returns both the deployed contract and constructor arguments
The implementation follows all deployment script conventions.
22-31: JSON path is valid:.mainnet.feeAdapter&.arbitrum.feeAdapterexist inconfig/everclear.json, and_getConfigContractAddress(viastdJson.readAddress) handles the leading dot.src/Facets/EverclearFacet.sol (5)
56-59: Implement the agreed-upon custom error for fee validation.In previous review comments, you agreed to add a specific
FeeExceedsAmountcustom error for better user experience when the fee exceeds the bridged amount. This custom error has not yet been added to the contract.Referencing past review discussion.
Apply this diff to add the custom error:
/// @notice Reverts when the destination chain is not supported by Everclear error UnsupportedEverclearChainId(); +/// @notice Reverts when the Everclear fee exceeds the bridged amount +error FeeExceedsAmount();
133-138: Use the specific custom error for fee validation.The validation currently uses the generic
InvalidCallData()error when the fee exceedsminAmount. Per the agreed-upon improvement from previous review, this should use a specificFeeExceedsAmount()error for clearer user feedback.Referencing past review discussion.
Apply this diff once the custom error is defined:
// make sure receiver address has a value to prevent potential loss of funds // contract does NOT validate _everclearData.deadline and _everclearData.sig to save gas here. Fee adapter will signature with fee and deadline in message anyway. if ( _everclearData.outputAsset == bytes32(0) || _bridgeData.minAmount < _everclearData.fee -) revert InvalidCallData(); +) { + if (_everclearData.outputAsset == bytes32(0)) revert InvalidCallData(); + if (_bridgeData.minAmount < _everclearData.fee) revert FeeExceedsAmount(); +}
77-95: LGTM! Entry point functions are properly structured.Both entry point functions correctly implement:
- All required modifiers (
nonReentrant,refundExcessNative,validateBridgeData,doesNotContainDestinationCalls,noNativeAsset)- Appropriate source swap validation (
doesNotContainSourceSwapsvscontainsSourceSwaps)- Proper asset handling via
LibAsset.depositAssetand_depositAndSwap- Clear NatSpec documentation
Also applies to: 101-122
129-215: LGTM! Internal bridging logic is well-implemented.The
_startBridgefunction correctly handles:
- Asset validation and approval (lines 135-144)
- Non-EVM vs EVM destination routing with proper address validation (lines 147-212)
- Receiver address validation for both paths
- Chain ID validation and mapping (Solana → Everclear chain ID)
- Fee deduction arithmetic protected by the minAmount check
- Event emission at the end after all operations complete
The event emission order (line 214) after external calls is acceptable given the
nonReentrantprotection on public entry points.Based on learnings.
161-175: Ignore unchecked return values from newIntentThe call to newIntent will revert on failure and only returns the intentId and Intent struct (not a success flag). Since this facet does not need those returned values, it’s safe to leave them unchecked.
Likely an incorrect or invalid review comment.
- Add _feeAdapter config pointing to everclear.json - Required for deployment script validation Addresses review comment GH-2579265264
- Change from 2.0.0 to 1.0.0 since this is the initial release - Version 1.0.0 was never audited/deployed Addresses review comment GH-2579257364
- Add MULTIPLIER_BASE constant for decimal adjustments - Add amountOutMinMultiplier field to EverclearData struct - Recalculate amountOutMin after swaps to capture positive slippage - Update _startBridge to accept memory parameter instead of calldata - Update TestEverclearFacet to inherit TestWhitelistManagerBase - Replace deprecated addDex/setFunctionApprovalBySignature methods Addresses review comments GH-2579279199 and GH-2579299731
- Change condition from < to <= in fee validation - Prevents case where fee equals minAmount resulting in 0 bridge amount Addresses review comment GH-2579281402
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/solidity/Facets/EverclearFacet.t.sol (2)
28-48: Replace hard-coded99934901with computed amount and dedicated variableYou already have
usdCAmountToSendandfee, plus comments explaining that99934901is100000000 - 65099. Keeping the raw constant in assertions makes the tests harder to maintain and was previously called out in review.You can both compute the send amount and use it in assertions:
- // values defaultUSDCAmount and fee taken from quote data where totalFeeBps is 0.6509 + // values defaultUSDCAmount and fee taken from quote data where totalFeeBps is 0.6509 @@ - uint256 internal usdCAmountToSend = 99934901; // its defaultUSDCAmount - fee (100000000 - 65099) - uint256 internal fee = 65099; + uint256 internal constant DEFAULT_USDC_AMOUNT = 100_000_000; + uint256 internal fee = 65_099; + uint256 internal usdCAmountToSend = DEFAULT_USDC_AMOUNT - fee; @@ - assertBalanceChange( - ADDRESS_USDC, - USER_SENDER, - -int256(99934901 + validEverclearData.fee) // 99934901 is defaultUSDCAmount - fee (100000000 - 65099) - ) + assertBalanceChange( + ADDRESS_USDC, + USER_SENDER, + -int256(usdCAmountToSend + validEverclearData.fee) + ) @@ - assertBalanceChange( - ADDRESS_USDC, - USER_SENDER, - -int256(99934901 + validEverclearData.fee) // 99934901 is defaultUSDCAmount - fee (100000000 - 65099) - ) + assertBalanceChange( + ADDRESS_USDC, + USER_SENDER, + -int256(usdCAmountToSend + validEverclearData.fee) + )This keeps the tests self-explanatory and avoids duplicating the same magic number.
Also applies to: 297-309, 322-333
925-968: Align remaining native-fee revert tests with V2 signature helperTwo native-fee revert tests still construct signatures with a custom 4‑parameter hash instead of using
createEverclearV2Signature, whereas all the positive native-fee paths (and the skipped bridge test) now use the V2 helper that matches the FeeAdapter’s_sigData/_hashformat.While these tests only assert a generic revert, keeping the signature format consistent across all native-fee tests reduces the risk of diverging from the adapter’s real behavior.
You can mirror the pattern from
test_ExcessNativeFeeGetsRefunded:function testRevert_InsufficientNativeFee() public { @@ - // create signature with native fee - bytes32 messageHash = keccak256( - abi.encode(fee, nativeFee, bridgeData.sendingAssetId, deadline) - ); - bytes32 ethSignedMessageHash = toEthSignedMessageHash(messageHash); - - (uint8 v, bytes32 r, bytes32 s) = vm.sign( - signerPrivateKey, - ethSignedMessageHash - ); - bytes memory signature = abi.encodePacked(r, s, v); + // Create destinations array + uint32[] memory destinations = new uint32[](1); + destinations[0] = uint32(bridgeData.destinationChainId); + + // Create V2 signature with native fee + bytes memory signature = createEverclearV2Signature( + nativeFee, + destinations, + USER_RECEIVER, + ADDRESS_USDC, + ADDRESS_USDC_BASE, + usdCAmountToSend, + 0, // amountOutMin + 0, // ttl + "", + fee, + deadline, + signerPrivateKey + ); @@ function testRevert_SwapAndBridgeInsufficientNativeFee() public { @@ - // create signature - bytes32 hash = keccak256( - abi.encode(fee, nativeFee, bridgeData.sendingAssetId, deadline) - ); - (uint8 v, bytes32 r, bytes32 s) = vm.sign( - signerPrivateKey, - toEthSignedMessageHash(hash) - ); - - // update data + // Create destinations array + uint32[] memory destinations = new uint32[](1); + destinations[0] = uint32(bridgeData.destinationChainId); + + // Create V2 signature with native fee + bytes memory signature = createEverclearV2Signature( + nativeFee, + destinations, + USER_RECEIVER, + ADDRESS_USDC, + ADDRESS_USDC_BASE, + usdCAmountToSend, + 0, // amountOutMin + 0, // ttl + "", + fee, + deadline, + signerPrivateKey + ); + + // update data EverclearFacet.EverclearData memory data = validEverclearData; data.nativeFee = nativeFee; data.deadline = deadline; - data.sig = abi.encodePacked(r, s, v); + data.sig = signature;This way, all native-fee tests (success and revert) share the same, accurate signature format.
Also applies to: 1030-1063
🧹 Nitpick comments (1)
src/Facets/EverclearFacet.sol (1)
158-168: Fix stale comment above_startBridgeguardThe comment currently talks about validating the receiver address, but the condition actually validates
outputAssetand ensuresminAmount > fee. This mismatch can confuse future reviewers.I’d update the comment to reflect what the code is doing:
- // make sure receiver address has a value to prevent potential loss of funds - // contract does NOT validate _everclearData.deadline and _everclearData.sig to save gas here. Fee adapter will signature with fee and deadline in message anyway. + // Make sure a valid output asset is provided and the fee is strictly less than the bridged amount + // Contract does NOT validate _everclearData.deadline and _everclearData.sig to save gas here. + // The fee adapter validates the signature (including fee and deadline) instead.
📜 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)
script/demoScripts/demoEverclear.ts(1 hunks)script/deploy/resources/deployRequirements.json(1 hunks)src/Facets/EverclearFacet.sol(1 hunks)test/solidity/Facets/EverclearFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/demoScripts/demoEverclear.ts
🧰 Additional context used
📓 Path-based instructions (5)
script/deploy/resources/deployRequirements.json
📄 CodeRabbit inference engine (conventions.md)
deployRequirements.json dictates deployment rules, manages contract dependencies, handles network-specific parameters, specifies zero address restrictions, and defines required external config files. Used by helperFunctions.sh for deployment validation
Files:
script/deploy/resources/deployRequirements.json
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings
Files:
src/Facets/EverclearFacet.sol
src/Facets/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/*.sol: Facet-specific business logic errors should be defined within the facet contract itself, not in GenericErrors.sol
Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Files:
src/Facets/EverclearFacet.sol
test/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
All Solidity test files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement
Files:
test/solidity/Facets/EverclearFacet.t.sol
test/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/**/*.t.sol: Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
In test files, use blank lines between vm.expectRevert() and function call, before vm.stopPrank() if a separate logical block, and before assertions. Single blank line between test cases, after vm.startPrank(address), and before/after vm.expectEmit blocks (no blank line between vm.expectEmit and its event definition)
Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, ) for event testing
For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Files:
test/solidity/Facets/EverclearFacet.t.sol
🧠 Learnings (70)
📓 Common learnings
Learnt from: mirooon
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 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: ezynda3
Repo: lifinance/contracts PR: 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.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: docs/LDAPeripheryRegistryFacet.md:14-27
Timestamp: 2025-08-29T14:05:25.335Z
Learning: mirooon prefers interface-level documentation to focus on the API contract and function purposes rather than implementation details like access control restrictions or specific modifiers, keeping documentation clean and consumable at the interface level.
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/deploy/resources/deployRequirements.json : deployRequirements.json dictates deployment rules, manages contract dependencies, handles network-specific parameters, specifies zero address restrictions, and defines required external config files. Used by helperFunctions.sh for deployment validation
Applied to files:
script/deploy/resources/deployRequirements.jsonsrc/Facets/EverclearFacet.sol
📚 Learning: 2025-09-22T00:52:26.172Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2024-09-27T07:10:15.586Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/resources/deployRequirements.jsonsrc/Facets/EverclearFacet.sol
📚 Learning: 2025-09-09T10:39:26.383Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2024-11-21T08:25:26.214Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/_targetState.json:1364-1390
Timestamp: 2024-11-21T08:25:26.214Z
Learning: For the Cronos network configuration in `script/deploy/_targetState.json`, the absence of bridge facets such as `StargateFacet`, `AcrossFacet`, `HopFacet`, and `SymbiosisFacet` is acceptable and expected.
Applied to files:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Applied to files:
script/deploy/resources/deployRequirements.jsonsrc/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-10-10T10:56:04.861Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Applied to files:
script/deploy/resources/deployRequirements.jsonsrc/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/deploy/resources/deployRequirements.json
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-02-11T10:33:52.791Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.
Applied to files:
script/deploy/resources/deployRequirements.json
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-13T00:53:43.774Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: src/Facets/PolymerCCTPFacet.sol:220-224
Timestamp: 2025-11-13T00:53:43.774Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event intentionally emits _bridgeData.minAmount (the pre-fee total) as its first parameter rather than the net bridged amount (bridgeAmount after fee deduction), because off-chain logic requires this pre-fee value to pick up deposits.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-13T00:53:25.372Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: test/solidity/Facets/PolymerCCTPFacet.t.sol:325-333
Timestamp: 2025-11-13T00:53:25.372Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event must emit the original bridgeData.minAmount (before polymer fee deduction) as the first parameter, not the net bridge amount, because off-chain logic requires this value to properly pick up and process the deposit.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-10T03:33:59.733Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:15-21
Timestamp: 2024-10-10T03:33:59.733Z
Learning: In Solidity, events cannot be imported from another contract; they need to be redefined or imported from an interface.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `GasZipPeriphery` contract, it's acceptable to let low-level calls like `liFiDEXAggregator.call` fail without explicit error handling, as failing the entire transaction is sufficient and saves gas.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-02T06:33:33.309Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-17T04:21:26.825Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-14T08:23:38.076Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:04:16.323Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T10:02:15.095Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1299
File: src/Facets/AcrossFacetPackedV4.sol:186-214
Timestamp: 2025-08-29T10:02:15.095Z
Learning: In AcrossFacetPackedV4.sol, calldata length validation is intentionally omitted to prioritize gas optimization over safety checks. The team accepts the risk of potential out-of-bounds reads in favor of lower gas costs for the packed implementation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-22T03:14:52.578Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-09-25T07:48:53.243Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-07-16T01:04:11.083Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:175-175
Timestamp: 2025-07-16T01:04:11.083Z
Learning: In AllBridgeFacet.sol, zero fee validation is not required at the contract level because the backend system ensures that calldata and msg.value are properly aligned before transactions reach the contract.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:51:52.704Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-08-27T13:47:28.646Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-02T18:14:51.385Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: src/Facets/UnitFacet.sol:1-2
Timestamp: 2025-10-02T18:14:51.385Z
Learning: For facet contracts in src/Facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for src/Facets/UnitFacet.sol.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:18:56.656Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T23:01:41.042Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: In the LiFi contracts codebase, ICurveLegacy interface is defined inline in src/Periphery/LiFiDEXAggregator.sol with a non-payable exchange function. The CurveFacet diff shows importing it from a separate file that doesn't exist, creating a compilation issue.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-11-25T09:05:03.917Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:05:35.264Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: script/deploy/resources/deployRequirements.json:466-474
Timestamp: 2024-09-23T02:05:35.264Z
Learning: In the `deployRequirements.json` file, do not report issues suggesting adding `config/` prefix to `configFileName` entries, as all entries are designed this way.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-07T14:57:59.929Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1452
File: deployments/_deployments_log_file.json:42713-42727
Timestamp: 2025-11-07T14:57:59.929Z
Learning: In the `deployments/_deployments_log_file.json` file, the `ZK_SOLC_VERSION` field is required for all deployment entries but should be set to an empty string `""` for non-zkEVM chains.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-30T03:52:27.281Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 812
File: deployments/_deployments_log_file.json:1914-1927
Timestamp: 2024-09-30T03:52:27.281Z
Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-24T06:17:25.211Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-17T11:24:43.164Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: src/Periphery/LiFiDEXAggregator.sol:877-881
Timestamp: 2025-07-17T11:24:43.164Z
Learning: KatanaV3 pools (being UniV3-style implementations) have a requirement that amountSpecified != 0, which means they cannot work with the processOnePool command that passes amountSpecified = 0. This makes KatanaV3 incompatible with multi-hop swaps where a second hop would use INTERNAL_INPUT_SOURCE.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-01-28T11:29:09.566Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-08-28T02:41:07.505Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1321
File: docs/RelayDepositoryFacet.md:9-10
Timestamp: 2025-08-28T02:41:07.505Z
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/EverclearFacet.t.sol
📚 Learning: 2025-08-29T11:07:57.743Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2024-10-31T09:10:16.115Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: The CurveFacet implementation is fundamentally broken: ICurveLegacy interface doesn't exist in the codebase but is imported and used, and the code attempts to send ETH to non-payable exchange functions. The entire native asset handling logic needs to be redesigned.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-06-15T13:22:56.249Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/utils/TestWhitelistManagerBase.sol:11-19
Timestamp: 2025-06-15T13:22:56.249Z
Learning: For test utility contracts like TestWhitelistManagerBase, it's acceptable and preferred to have relaxed access controls (external functions without modifiers) to make testing more flexible and convenient, rather than mirroring production access control patterns.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-28T14:23:44.819Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:40-42
Timestamp: 2025-01-28T14:23:44.819Z
Learning: Explicit error handling for vm.readFile operations in Foundry scripts is not required as the implicit error handling is sufficient.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2024-10-22T03:16:28.754Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
🔇 Additional comments (4)
script/deploy/resources/deployRequirements.json (1)
680-688: EverclearFacet entry is well-structured and consistent.The added EverclearFacet configuration follows the established pattern used by other facets in this file. It correctly declares the external config file dependency (
everclear.json), uses the standard network-aware key pattern (.<NETWORK>.feeAdapter), and appropriately restricts zero-address deployment for this critical component.src/Facets/EverclearFacet.sol (2)
1-36: Header, types, and constructor are consistent and robustLicense, pragma, imports, EverclearData layout, custom error, and the constructor’s non-zero validation for
FEE_ADAPTERall look correct and align with facet patterns in this repo. Using an immutable adapter and a genericInvalidConfigrevert is a good fit here.Also applies to: 62-77
81-151: Bridge entrypoints correctly enforce invariants and funds flow
startBridgeTokensViaEverclearandswapAndStartBridgeTokensViaEverclearapply the expected modifiers (nonReentrant,refundExcessNative(msg.sender),validateBridgeData, source/destination swap guards,noNativeAsset) and route funds viaLibAsset.depositAsset/_depositAndSwapbefore delegating to_startBridge. This matches the facet conventions and keeps the external surface tight.test/solidity/Facets/EverclearFacet.t.sol (1)
4-24: Test harness and signature helpers accurately mirror FeeAdapter behaviorUsing
TestEverclearFacet+TestWhitelistManagerBase, the fork-basedsetUp, and thecreateEverclearV2Signature/createEverclearV2SignatureNonEVMhelpers gives a realistic model of how the Everclear FeeAdapter signs and verifies intents (including the adapter address +chainidwrap and EIP-191 prefix). This wiring should make the tests very representative of production flows.Also applies to: 49-157, 159-244
- Update comments to specify FeeAdapter pulls from diamond, not user - Makes it clear the user has already deposited to diamond Addresses review comment GH-2579294941
- Replace hardcoded values with calculated usdCAmountToSend - Add event emission checks to testBase_CanBridgeTokens - Add event emission checks to test_CanBridgeTokensToNonEVMChain - Remove skip_ prefix to enable test_CanBridgeTokensWithNativeFee - Clarify error handling comment for FeeAdapter revert - 31/33 tests passing (2 swap tests need signature updates) Addresses review comments GH-2579302938, GH-2579316574, GH-2579318141, GH-2579316882, GH-2579417235, GH-2579421315
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Facets/EverclearFacet.sol (1)
62-65: Consider adding the FeeExceedsAmount custom error as previously agreed.In a past review, you agreed to add a descriptive custom error for the fee validation case. While the current validation at line 166 (
_bridgeData.minAmount <= _everclearData.fee) works correctly and reverts withInvalidCallData, using a specific error likeFeeExceedsAmount()would make debugging and error handling clearer for integrators.Apply this diff to add the custom error:
/// Errors /// /// @notice Reverts when the destination chain is not supported by Everclear error UnsupportedEverclearChainId(); + +/// @notice Reverts when the Everclear fee exceeds or equals the bridged amount +error FeeExceedsAmount();Then update line 167 to use it:
if ( _everclearData.outputAsset == bytes32(0) || - _bridgeData.minAmount <= _everclearData.fee - ) revert InvalidCallData(); + _everclearData.outputAsset == bytes32(0) + ) revert InvalidCallData(); + + if (_bridgeData.minAmount <= _everclearData.fee) { + revert FeeExceedsAmount(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Facets/EverclearFacet.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings
Files:
src/Facets/EverclearFacet.sol
src/Facets/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/*.sol: Facet-specific business logic errors should be defined within the facet contract itself, not in GenericErrors.sol
Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Files:
src/Facets/EverclearFacet.sol
🧠 Learnings (49)
📓 Common learnings
Learnt from: mirooon
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 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
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: docs/LDAPeripheryRegistryFacet.md:14-27
Timestamp: 2025-08-29T14:05:25.335Z
Learning: mirooon prefers interface-level documentation to focus on the API contract and function purposes rather than implementation details like access control restrictions or specific modifiers, keeping documentation clean and consumable at the interface level.
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-10T10:56:04.861Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-13T00:53:43.774Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: src/Facets/PolymerCCTPFacet.sol:220-224
Timestamp: 2025-11-13T00:53:43.774Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event intentionally emits _bridgeData.minAmount (the pre-fee total) as its first parameter rather than the net bridged amount (bridgeAmount after fee deduction), because off-chain logic requires this pre-fee value to pick up deposits.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-13T00:53:25.372Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: test/solidity/Facets/PolymerCCTPFacet.t.sol:325-333
Timestamp: 2025-11-13T00:53:25.372Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event must emit the original bridgeData.minAmount (before polymer fee deduction) as the first parameter, not the net bridge amount, because off-chain logic requires this value to properly pick up and process the deposit.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-10T03:33:59.733Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:15-21
Timestamp: 2024-10-10T03:33:59.733Z
Learning: In Solidity, events cannot be imported from another contract; they need to be redefined or imported from an interface.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `GasZipPeriphery` contract, it's acceptable to let low-level calls like `liFiDEXAggregator.call` fail without explicit error handling, as failing the entire transaction is sufficient and saves gas.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-02T06:33:33.309Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-17T04:21:26.825Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-06-15T08:47:22.079Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1208
File: test/solidity/utils/TestAMM.sol:18-22
Timestamp: 2025-06-15T08:47:22.079Z
Learning: For test contracts in the lifinance/contracts repository, simpler error handling approaches are acceptable and preferred over more complex error bubbling mechanisms, as the focus is on functionality rather than detailed error reporting.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-14T08:23:38.076Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:04:16.323Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T10:02:15.095Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1299
File: src/Facets/AcrossFacetPackedV4.sol:186-214
Timestamp: 2025-08-29T10:02:15.095Z
Learning: In AcrossFacetPackedV4.sol, calldata length validation is intentionally omitted to prioritize gas optimization over safety checks. The team accepts the risk of potential out-of-bounds reads in favor of lower gas costs for the packed implementation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-22T03:14:52.578Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-25T07:48:53.243Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-07-16T01:04:11.083Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:175-175
Timestamp: 2025-07-16T01:04:11.083Z
Learning: In AllBridgeFacet.sol, zero fee validation is not required at the contract level because the backend system ensures that calldata and msg.value are properly aligned before transactions reach the contract.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-08-27T13:51:52.704Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-08-27T13:47:28.646Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-02T18:14:51.385Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: src/Facets/UnitFacet.sol:1-2
Timestamp: 2025-10-02T18:14:51.385Z
Learning: For facet contracts in src/Facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for src/Facets/UnitFacet.sol.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:18:56.656Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T23:01:41.042Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: In the LiFi contracts codebase, ICurveLegacy interface is defined inline in src/Periphery/LiFiDEXAggregator.sol with a non-payable exchange function. The CurveFacet diff shows importing it from a separate file that doesn't exist, creating a compilation issue.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-11-25T09:05:03.917Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/deploy/resources/deployRequirements.json : deployRequirements.json dictates deployment rules, manages contract dependencies, handles network-specific parameters, specifies zero address restrictions, and defines required external config files. Used by helperFunctions.sh for deployment validation
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:05:35.264Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: script/deploy/resources/deployRequirements.json:466-474
Timestamp: 2024-09-23T02:05:35.264Z
Learning: In the `deployRequirements.json` file, do not report issues suggesting adding `config/` prefix to `configFileName` entries, as all entries are designed this way.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-07T14:57:59.929Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1452
File: deployments/_deployments_log_file.json:42713-42727
Timestamp: 2025-11-07T14:57:59.929Z
Learning: In the `deployments/_deployments_log_file.json` file, the `ZK_SOLC_VERSION` field is required for all deployment entries but should be set to an empty string `""` for non-zkEVM chains.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-30T03:52:27.281Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 812
File: deployments/_deployments_log_file.json:1914-1927
Timestamp: 2024-09-30T03:52:27.281Z
Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-24T06:17:25.211Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-17T11:24:43.164Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: src/Periphery/LiFiDEXAggregator.sol:877-881
Timestamp: 2025-07-17T11:24:43.164Z
Learning: KatanaV3 pools (being UniV3-style implementations) have a requirement that amountSpecified != 0, which means they cannot work with the processOnePool command that passes amountSpecified = 0. This makes KatanaV3 incompatible with multi-hop swaps where a second hop would use INTERNAL_INPUT_SOURCE.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
🔇 Additional comments (2)
src/Facets/EverclearFacet.sol (2)
129-141: LGTM: Positive slippage protection implemented correctly.The
amountOutMinadjustment logic properly handles positive slippage from pre-bridge swaps. When_bridgeData.minAmountincreases due to positive slippage, the proportional recalculation usingamountOutMinMultiplierensures the user receives the benefit rather than the solver, addressing the intent-bridge concern.
243-243: LGTM: Event emission follows team convention.The
LiFiTransferStartedevent is correctly emitted at the end of_startBridgeafter all external calls, which aligns with the team's established convention for facet contracts. ThenonReentrantmodifier on the public entry points provides reentrancy protection.
- Remove AI confirmation comments and architecture section - Add amountOutMinMultiplier field to everclearData struct Addresses review comment GH-2579424181
- Move deriveSolanaAddress and solanaAddressToBytes32 to helpers module - Add @solana/web3.js import to demoScriptHelpers - Export functions for reuse in other Solana-related scripts - Update demoEverclear.ts to import from helpers Addresses review comment GH-2579431874
ezynda3
left a comment
ezynda3
left a comment
There was a problem hiding this comment.
The 4 chains (mainnet, base, optimism, arbitrum) represent our initial deployment scope focusing on the most popular networks. While Everclear supports 22 chains total, this is an intentional business decision for the initial release. We can expand to additional Everclear-supported chains in future deployments as needed.
- Remove amountOutMinMultiplier field from EverclearData struct - Remove MULTIPLIER_BASE constant - Restore original swap+bridge flow without amountOutMin recalculation - Revert _startBridge parameter from memory to calldata The positive slippage handling approach is incompatible with Everclear's signature-based validation. The API generates signatures that include amountOutMin, which cannot be modified after the signature is created. This needs further discussion with the Everclear team. All tests now passing: 33/33 Reverts part of commit 06ff1a2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/demoScripts/utils/demoScriptHelpers.ts (1)
1024-1028: Minor:padStartis unnecessary for Solana public keys.Solana public keys are always exactly 32 bytes, so
.padStart(64, '0')will never add padding. Consider removing it for clarity, though it doesn't cause any issues.export function solanaAddressToBytes32(solanaAddress: string): `0x${string}` { const publicKey = new PublicKey(solanaAddress) const bytes = publicKey.toBytes() - const hex = '0x' + Buffer.from(bytes).toString('hex').padStart(64, '0') + const hex = '0x' + Buffer.from(bytes).toString('hex') return hex as `0x${string}` }
📜 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)
script/demoScripts/demoEverclear.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(2 hunks)src/Facets/EverclearFacet.sol(1 hunks)test/solidity/Facets/EverclearFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
script/demoScripts/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/demoScripts/**/*.ts: All demo scripts must use viem for contract interactions (ethers.js is NOT allowed). Use createPublicClient, createWalletClient, getContract from viem, encodeFunctionData for encoding, parseUnits/formatUnits instead of BigNumber, zeroAddress from viem, and randomBytes from crypto module
Use setupEnvironment() from demoScriptHelpers.ts for viem client setup, ensureBalance() and ensureAllowance() for token checks, executeTransaction() for transaction execution, createContractObject() for ERC20 contracts, and getEnvVar() for environment variable access
Files:
script/demoScripts/demoEverclear.tsscript/demoScripts/utils/demoScriptHelpers.ts
script/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/**/*.ts: Do NOT use deprecated ethers-based helpers: getProvider(), getWalletFromPrivateKeyInDotEnv(), deprecated sendTransaction(), or ensureBalanceAndAllowanceToDiamond(). Use viem clients and viem-based helpers instead
All TypeScript scripts must follow rules defined in .eslintrc.cjs, use async/await for asynchronous operations, handle errors with try/catch blocks, use citty for CLI argument parsing, consola for logging, getEnvVar() for environment variables, and exit with appropriate codes (0 for success, 1 for error)
Files:
script/demoScripts/demoEverclear.tsscript/demoScripts/utils/demoScriptHelpers.ts
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings
Files:
src/Facets/EverclearFacet.sol
src/Facets/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/*.sol: Facet-specific business logic errors should be defined within the facet contract itself, not in GenericErrors.sol
Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Files:
src/Facets/EverclearFacet.sol
test/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
All Solidity test files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement
Files:
test/solidity/Facets/EverclearFacet.t.sol
test/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/**/*.t.sol: Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
In test files, use blank lines between vm.expectRevert() and function call, before vm.stopPrank() if a separate logical block, and before assertions. Single blank line between test cases, after vm.startPrank(address), and before/after vm.expectEmit blocks (no blank line between vm.expectEmit and its event definition)
Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, ) for event testing
For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Files:
test/solidity/Facets/EverclearFacet.t.sol
🧠 Learnings (89)
📓 Common learnings
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Learnt from: mirooon
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
Learnt from: mirooon
Repo: lifinance/contracts PR: 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
Repo: lifinance/contracts PR: 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.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: docs/LDAPeripheryRegistryFacet.md:14-27
Timestamp: 2025-08-29T14:05:25.335Z
Learning: mirooon prefers interface-level documentation to focus on the API contract and function purposes rather than implementation details like access control restrictions or specific modifiers, keeping documentation clean and consumable at the interface level.
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/demoScripts/**/*.ts : All demo scripts must use viem for contract interactions (ethers.js is NOT allowed). Use createPublicClient, createWalletClient, getContract from viem, encodeFunctionData for encoding, parseUnits/formatUnits instead of BigNumber, zeroAddress from viem, and randomBytes from crypto module
Applied to files:
script/demoScripts/demoEverclear.tsscript/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/demoScripts/**/*.ts : Use setupEnvironment() from demoScriptHelpers.ts for viem client setup, ensureBalance() and ensureAllowance() for token checks, executeTransaction() for transaction execution, createContractObject() for ERC20 contracts, and getEnvVar() for environment variable access
Applied to files:
script/demoScripts/demoEverclear.tsscript/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/utils/**/*.ts : TypeScript helper functions must use proper TypeScript types instead of any, use ILiFi.BridgeDataStruct from typechain for bridge data, and be thoroughly documented
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2025-08-22T10:03:58.794Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/demoEverclear.ts
📚 Learning: 2024-11-05T17:14:24.059Z
Learnt from: maxklenk
Repo: lifinance/contracts PR: 782
File: script/demoScripts/demoPermit2.ts:119-124
Timestamp: 2024-11-05T17:14:24.059Z
Learning: In the file `script/demoScripts/demoPermit2.ts`, adding error handling for external API calls is not preferred as it may overload the demo.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2025-06-05T14:27:30.714Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcherDest.ts:0-0
Timestamp: 2025-06-05T14:27:30.714Z
Learning: For demo scripts in the `script/demoScripts/` directory, hardcoded values and simplified implementations are acceptable since they're designed to showcase functionality rather than be production-ready.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2025-06-05T14:25:19.137Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcherDest_new.ts:0-0
Timestamp: 2025-06-05T14:25:19.137Z
Learning: Error handling for `findNeedleOffset` function calls is not needed in demo scripts in the lifinance/contracts repository, as indicated by the team's preference to keep demo scripts simpler.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2024-09-23T02:08:02.297Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: deployments/bsc.staging.json:28-28
Timestamp: 2024-09-23T02:08:02.297Z
Learning: For staging files in deployments, avoid making comments regarding incomplete contract deployments or missing bytecode, as these comments are not necessary.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2025-06-05T14:51:39.210Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcherDest.ts:0-0
Timestamp: 2025-06-05T14:51:39.210Z
Learning: In demo scripts like those in script/demoScripts/, code duplication is acceptable and refactoring suggestions to reduce duplication may not be necessary since these are demonstration files rather than production code.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2024-10-31T09:11:15.238Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 843
File: script/demoScripts/demoRelay.ts:27-89
Timestamp: 2024-10-31T09:11:15.238Z
Learning: When reviewing demo scripts in `script/demoScripts`, avoid refactoring suggestions for code reuse, as they may not be necessary.
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2024-11-05T17:16:19.946Z
Learnt from: maxklenk
Repo: lifinance/contracts PR: 782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
script/demoScripts/demoEverclear.ts
📚 Learning: 2025-08-26T15:19:07.800Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
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/demoScripts/demoEverclear.tsscript/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed
Applied to files:
script/demoScripts/demoEverclear.tssrc/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Facet contracts must implement _startBridge (internal), swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName} functions. Inherit from ILiFi, LibAsset, LibSwap, LibAllowList, ReentrancyGuard, SwapperV2, and Validatable
Applied to files:
script/demoScripts/demoEverclear.tssrc/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : Use nonReentrant, refundExcessNative, validateBridgeData, and appropriate source/destination call modifiers (doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls) on facet functions
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-10-10T10:56:04.861Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/Facets/*.sol : In facet contracts, emit LiFiTransferStarted event at transaction start (before external calls) and never emit LiFiTransferCompleted (only use in Executor) or LiFiTransferRecovered (only use in Receiver contracts)
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-11-13T00:53:43.774Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: src/Facets/PolymerCCTPFacet.sol:220-224
Timestamp: 2025-11-13T00:53:43.774Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event intentionally emits _bridgeData.minAmount (the pre-fee total) as its first parameter rather than the net bridged amount (bridgeAmount after fee deduction), because off-chain logic requires this pre-fee value to pick up deposits.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-13T00:53:25.372Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1456
File: test/solidity/Facets/PolymerCCTPFacet.t.sol:325-333
Timestamp: 2025-11-13T00:53:25.372Z
Learning: In src/Facets/PolymerCCTPFacet.sol, the PolymerCCTPFeeSent event must emit the original bridgeData.minAmount (before polymer fee deduction) as the first parameter, not the net bridge amount, because off-chain logic requires this value to properly pick up and process the deposit.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-16T01:04:55.857Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-10T03:33:59.733Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:15-21
Timestamp: 2024-10-10T03:33:59.733Z
Learning: In Solidity, events cannot be imported from another contract; they need to be redefined or imported from an interface.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `GasZipPeriphery` contract, it's acceptable to let low-level calls like `liFiDEXAggregator.call` fail without explicit error handling, as failing the entire transaction is sufficient and saves gas.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-02T06:33:33.309Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-17T04:21:26.825Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-06-15T08:47:22.079Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1208
File: test/solidity/utils/TestAMM.sol:18-22
Timestamp: 2025-06-15T08:47:22.079Z
Learning: For test contracts in the lifinance/contracts repository, simpler error handling approaches are acceptable and preferred over more complex error bubbling mechanisms, as the focus is on functionality rather than detailed error reporting.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-14T08:23:38.076Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:04:16.323Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T10:02:15.095Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1299
File: src/Facets/AcrossFacetPackedV4.sol:186-214
Timestamp: 2025-08-29T10:02:15.095Z
Learning: In AcrossFacetPackedV4.sol, calldata length validation is intentionally omitted to prioritize gas optimization over safety checks. The team accepts the risk of potential out-of-bounds reads in favor of lower gas costs for the packed implementation.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-22T03:14:52.578Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Facets/GasZipFacet.sol:53-53
Timestamp: 2024-10-22T03:14:52.578Z
Learning: In `src/Facets/GasZipFacet.sol`, when checking `msg.value`, the team prefers to use strict equality (`msg.value == _bridgeData.minAmount`) to save gas and avoid refunding excess Ether.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-09-25T07:48:53.243Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-16T01:04:11.083Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:175-175
Timestamp: 2025-07-16T01:04:11.083Z
Learning: In AllBridgeFacet.sol, zero fee validation is not required at the contract level because the backend system ensures that calldata and msg.value are properly aligned before transactions reach the contract.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:51:52.704Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-08-27T13:47:28.646Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-10-02T18:14:51.385Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: src/Facets/UnitFacet.sol:1-2
Timestamp: 2025-10-02T18:14:51.385Z
Learning: For facet contracts in src/Facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for src/Facets/UnitFacet.sol.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-29T11:18:56.656Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-27T23:01:41.042Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: In the LiFi contracts codebase, ICurveLegacy interface is defined inline in src/Periphery/LiFiDEXAggregator.sol with a non-payable exchange function. The CurveFacet diff shows importing it from a separate file that doesn't exist, creating a compilation issue.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-11-25T09:05:03.917Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/deploy/resources/deployRequirements.json : deployRequirements.json dictates deployment rules, manages contract dependencies, handles network-specific parameters, specifies zero address restrictions, and defines required external config files. Used by helperFunctions.sh for deployment validation
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-23T02:05:35.264Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: script/deploy/resources/deployRequirements.json:466-474
Timestamp: 2024-09-23T02:05:35.264Z
Learning: In the `deployRequirements.json` file, do not report issues suggesting adding `config/` prefix to `configFileName` entries, as all entries are designed this way.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-11-07T14:57:59.929Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1452
File: deployments/_deployments_log_file.json:42713-42727
Timestamp: 2025-11-07T14:57:59.929Z
Learning: In the `deployments/_deployments_log_file.json` file, the `ZK_SOLC_VERSION` field is required for all deployment entries but should be set to an empty string `""` for non-zkEVM chains.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-09-30T03:52:27.281Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 812
File: deployments/_deployments_log_file.json:1914-1927
Timestamp: 2024-09-30T03:52:27.281Z
Learning: Duplicate entries in `deployments/_deployments_log_file.json` that are outdated do not require changes.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-10-24T06:17:25.211Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-07-17T11:24:43.164Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: src/Periphery/LiFiDEXAggregator.sol:877-881
Timestamp: 2025-07-17T11:24:43.164Z
Learning: KatanaV3 pools (being UniV3-style implementations) have a requirement that amountSpecified != 0, which means they cannot work with the processOnePool command that passes amountSpecified = 0. This makes KatanaV3 incompatible with multi-hop swaps where a second hop would use INTERNAL_INPUT_SOURCE.
Applied to files:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-02-21T09:00:28.226Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-09-09T10:39:26.383Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
src/Facets/EverclearFacet.sol
📚 Learning: 2025-09-19T14:10:55.064Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2024-10-04T09:01:56.514Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/base.diamond.json:123-123
Timestamp: 2024-10-04T09:01:56.514Z
Learning: In the `lifinance/contracts` repository, it's acceptable to retain references to the old `LiFuelFeeCollector` address (`0xc4f7A34b8d283f66925eF0f5CCdFC2AF3030DeaE`) in deployment files when updating them is not necessary.
Applied to files:
src/Facets/EverclearFacet.soltest/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/**/*.ts : Do NOT use deprecated ethers-based helpers: getProvider(), getWalletFromPrivateKeyInDotEnv(), deprecated sendTransaction(), or ensureBalanceAndAllowanceToDiamond(). Use viem clients and viem-based helpers instead
Applied to files:
script/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-28T11:29:09.566Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-08-28T02:41:07.505Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1321
File: docs/RelayDepositoryFacet.md:9-10
Timestamp: 2025-08-28T02:41:07.505Z
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/EverclearFacet.t.sol
📚 Learning: 2025-08-29T11:07:57.743Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2024-10-31T09:10:16.115Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: The CurveFacet implementation is fundamentally broken: ICurveLegacy interface doesn't exist in the codebase but is imported and used, and the code attempts to send ETH to non-payable exchange functions. The entire native asset handling logic needs to be redesigned.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-06-15T13:22:56.249Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/utils/TestWhitelistManagerBase.sol:11-19
Timestamp: 2025-06-15T13:22:56.249Z
Learning: For test utility contracts like TestWhitelistManagerBase, it's acceptable and preferred to have relaxed access controls (external functions without modifiers) to make testing more flexible and convenient, rather than mirroring production access control patterns.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-28T14:23:44.819Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:40-42
Timestamp: 2025-01-28T14:23:44.819Z
Learning: Explicit error handling for vm.readFile operations in Foundry scripts is not required as the implicit error handling is sufficient.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-11-13T06:58:47.547Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1464
File: src/Facets/DexManagerFacet.sol:44-68
Timestamp: 2025-11-13T06:58:47.547Z
Learning: The DexManagerFacet in src/Facets/DexManagerFacet.sol is planned for deprecation soon and should be kept as-is without further improvements or modifications.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-08-01T22:11:04.478Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: deployments/base.staging.json:6-6
Timestamp: 2025-08-01T22:11:04.478Z
Learning: The user mirooon prefers to maintain both DexManagerFacet and WhitelistManagerFacet entries in staging deployment configurations during migration periods, even when the DexManagerFacet contract no longer exists in the codebase. This approach allows for backward compatibility testing and rollback capabilities in the staging environment.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to script/deploy/facets/*.s.sol : Facet update scripts must inherit UpdateScriptBase, call update("{ContractName}"), and use getExcludes() for special cases returning array of excluded function selectors
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-01-30T10:38:18.041Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: script/demoScripts/demoGlacis.ts:180-189
Timestamp: 2025-01-30T10:38:18.041Z
Learning: The executeTransaction helper function in the LiFi codebase handles error cases and transaction logging comprehensively, including try/catch structures, transaction receipt validation, and detailed console logging for transaction status tracking.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-01-28T11:59:27.898Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 950
File: script/demoScripts/utils/demoScriptHelpers.ts:618-654
Timestamp: 2025-01-28T11:59:27.898Z
Learning: In the LiFi codebase, basic error details in console.error are sufficient for transaction failures, without the need for gas estimation or detailed error object destructuring.
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, <contractAddress>) for event testing
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
📚 Learning: 2024-10-22T03:16:28.754Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/EverclearFacet.t.sol
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to src/**/*.sol : Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Applied to files:
test/solidity/Facets/EverclearFacet.t.sol
🧬 Code graph analysis (1)
script/demoScripts/demoEverclear.ts (2)
script/common/types.ts (1)
SupportedChain(3-3)script/demoScripts/utils/demoScriptHelpers.ts (13)
setupEnvironment(654-708)ADDRESS_USDT_ARB(65-65)ADDRESS_USDC_ARB(64-64)ensureBalance(847-868)ensureAllowance(873-907)getUniswapDataERC20toExactERC20(335-407)ADDRESS_UNISWAP_ARB(83-83)deriveSolanaAddress(1003-1016)ADDRESS_USDC_BASE(76-76)solanaAddressToBytes32(1024-1029)ADDRESS_USDC_SOL(68-68)zeroPadAddressToBytes32(563-569)executeTransaction(806-842)
- Capture positive slippage from pre-bridge swaps - Transfer excess amount back to user instead of adjusting amountOutMin - This approach maintains signature validity (unlike AcrossV4 multiplier) - Follows the same pattern as EcoFacet The key difference from the reverted approach: - OLD: Tried to adjust amountOutMin (broke signature validation) - NEW: Send excess directly to user, bridge expected amount only This ensures users receive the benefit of favorable swaps while maintaining compatibility with Everclear's signature-based validation. All tests passing: 33/33 Addresses review comment GH-2579279199
ezynda3
changed the title
lifi-action-bot
changed the title
- Clarify that the revert is an EVM-level OutOfFunds error - Occurs when trying to forward more native value than was sent - Generic vm.expectRevert() is appropriate for low-level EVM errors Addresses review comment GH-2579421315
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
script/demoScripts/utils/demoScriptHelpers.ts (2)
1011-1031: Add input validation and error handling for robustness.The function should validate the private key format and handle potential errors from Solana library calls. Other helper functions in this file (e.g.,
zeroPadAddressToBytes32,normalizePrivateKey) follow the pattern of explicit validation with descriptive error messages.Consider adding validation and error handling:
export function deriveSolanaAddress(ethPrivateKey: string): string { + // Validate input format + const cleanKey = ethPrivateKey.replace(/^0x/, '') + if (!/^[a-fA-F0-9]{64}$/.test(cleanKey)) { + throw new Error('Invalid Ethereum private key format: expected 64 hex characters') + } + // Remove '0x' prefix if present const seed = ethPrivateKey.replace('0x', '') // Use first 32 bytes (64 hex chars) of the private key as seed for Ed25519 const seedBytes = new Uint8Array(32) for (let i = 0; i < 32; i++) seedBytes[i] = parseInt(seed.slice(i * 2, i * 2 + 2), 16) - // Create Solana keypair from seed - const keypair = Keypair.fromSeed(seedBytes) + try { + // Create Solana keypair from seed + const keypair = Keypair.fromSeed(seedBytes) + return keypair.publicKey.toBase58() + } catch (error) { + throw new Error(`Failed to derive Solana address: ${error}`) + } - - return keypair.publicKey.toBase58() }
1033-1044: Add input validation and error handling for Solana address conversion.The
PublicKeyconstructor will throw an error if the input is not a valid Solana base58 address. Adding validation and error handling will provide clearer error messages and prevent unexpected failures.Consider adding validation and error handling:
export function solanaAddressToBytes32(solanaAddress: string): `0x${string}` { + // Validate input is not empty + if (!solanaAddress || solanaAddress.trim().length === 0) { + throw new Error('Solana address cannot be empty') + } + + try { - const publicKey = new PublicKey(solanaAddress) - const bytes = publicKey.toBytes() - const hex = '0x' + Buffer.from(bytes).toString('hex').padStart(64, '0') - return hex as `0x${string}` + const publicKey = new PublicKey(solanaAddress) + const bytes = publicKey.toBytes() + const hex = '0x' + Buffer.from(bytes).toString('hex').padStart(64, '0') + return hex as `0x${string}` + } catch (error) { + throw new Error(`Invalid Solana address format '${solanaAddress}': ${error}`) + } }
- Added 18 additional chains beyond initial 4 - All chains verified to exist in networks.json - FeeAdapter addresses sourced from Everclear docs - Chains: bsc, unichain, polygon, zksync, ronin, apechain, mode, avalanche, linea, blast, scroll, taiko, berachain, gnosis, mantle, sonic, ink Addresses review comment GH-2579252980
Replace manual 4-parameter signature creation with proper createEverclearV2Signature helper in: - testRevert_InsufficientNativeFee (GH-2581029572) - testRevert_SwapAndBridgeInsufficientNativeFee (GH-2581029580) These tests were using simplified signature formats that would fail on signature validation instead of testing the intended insufficient native fee scenarios. Now properly test the native fee validation logic.
Fixed test signature issues (commits de1cb2b)GH-2581029572 & GH-2581029580: Fixed incorrect signature formats in:
Both tests were using manual 4-parameter signature creation that doesn't match the V2 FeeAdapter format. Replaced with proper Result: Tests now properly validate insufficient native fee scenarios instead of failing on signature validation first. All 33 tests passing ✅ |
Consistent with EverclearFacet - this is the initial release, not 2.0.0
lifi-action-bot
changed the title
src/Facets/EverclearFacet.sol
Outdated
| actualAmountAfterSwap = _bridgeData.minAmount; | ||
| } | ||
|
|
||
| _bridgeData.minAmount = actualAmountAfterSwap; |
There was a problem hiding this comment.
actualAmountAfterSwap cannot be lower than _bridgeData.minAmount since _depositAndSwap function will ensure the minAmount.
So we have two cases:
Case 1: actualAmountAfterSwap == _bridgeData.minAmount (no slippage)
Case 2: actualAmountAfterSwap > _bridgeData.minAmount (positive slippage)
in both cases _bridgeData.minAmount should remain the same, I dont think it makes sense to update it.
Also line 136 can be removed imho.
There was a problem hiding this comment.
Removed the redundant variable assignments on lines 136 and 139 as they were no-ops.
| // If we got more than expected from the swap (positive slippage), | ||
| // send the extra back to the user. We cannot adjust amountOutMin | ||
| // because Everclear's signature validation includes the original value. | ||
| if (actualAmountAfterSwap > _bridgeData.minAmount) { |
There was a problem hiding this comment.
There is question here that I need to check with BE team. Please lets keep this open until I resolve it.
There was a problem hiding this comment.
OK, received the answer I was waiting for.
- we cannot use msg.sender here (in case of gasless tx etc. this goes to the relayer)
- we need to add a parameter (senderAddress) that we will use for these refunds.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/EverclearFacet.md (1)
18-21: Keep public method signature in docs in sync with the contractThe second method bullet still lacks the
functionkeyword and useseverclearData(lowercase) withmemory, which doesn't match the facet:
- Use
function swapAndStartBridgeTokensViaEverclear(...)- Keep
EverclearDatacapitalized- Use
EverclearData calldata _everclearDatato mirror the contract’s signatureThis prevents ABI/type drift between docs and implementation.
🧹 Nitpick comments (5)
docs/EverclearFacet.md (1)
90-92: Minor doc polish: hyphenation and descriptive link texts
- Line 90: change “swap specific library” → “swap-specific library”.
- Replace generic link labels like “here” with descriptive text (e.g., “LibSwap library”, “ILiFi interface”, “quote endpoint usage guide”) to satisfy MD059 and improve readability.
Also applies to: 98-99, 129-129
deployments/arbitrum.diamond.staging.json (1)
188-207: Verify intentional duplication of EverclearFacet and LiFiIntentEscrowFacet entriesThis staging diamond now contains:
- Two
LiFiIntentEscrowFacetentries (0x1A3c… and 0xb3B9…) both at version1.0.0.- Two
EverclearFacetentries (0xed26… and 0x508D…) both at version1.0.0.If both addresses are meant to remain active (e.g., for migration/backward-compat), maybe fine. If one of them is legacy or a placeholder, consider removing or bumping the version to avoid confusion when reading deployment state.
src/Facets/EverclearFacet.sol (1)
81-99: Bridge + fee logic is consistent; refundReceiver requirement is slightly stricter than necessary
- Entry points use the standard modifier stack (nonReentrant, refundExcessNative, validateBridgeData, swap/no-swap flags, noNativeAsset) and delegate all core logic into
_startBridge, which is clean and consistent with other facets._startBridgecorrectly:
- Guards against
outputAsset == 0,minAmount <= fee, andrefundReceiver == address(0)before proceeding.- Maps Solana’s LiFi chain ID to the Everclear domain ID and reverts on unsupported non‑EVM destinations.
- Validates EVM receivers via bytes32 equality and enforces a
uint32destination chain ID.- Deducts
feefrom_bridgeData.minAmountand passes that net amount plus FeeParams intoFEE_ADAPTER.newIntentfor both EVM and non‑EVM paths, then emitsLiFiTransferStartedat the end.The only nit:
refundReceiveris mandatory even forstartBridgeTokensViaEverclear(no swaps), where it’s never used. That’s harmless but slightly stricter than the doc’s “positive slippage refunds” description; if you want to relax UX later, you could default to_bridgeData.receiverormsg.senderin the pure-bridge path.Also applies to: 105-140, 147-234
script/demoScripts/demoEverclear.ts (1)
92-99: Keep FeeAdapter decode ABIs aligned with IEverclearFeeAdapter
NEW_INTENT_EVM_ABI/NEW_INTENT_NON_EVM_ABIand thedecodeNewIntent*Calldatahelpers re-specify thenewIntentsignature strings manually. If IEverclearFeeAdapter’snewIntentABI changes (extra params, different tuple order, etc.), this script will silently drift.Consider deriving the decode shape from the generated IEverclearFeeAdapter/TypeChain types (or importing the ABI from the same source used for deployment) instead of hardcoding
parseAbi([...])strings. That way any signature change breaks compilation/tests instead of the demo at runtime.Also applies to: 101-153
test/solidity/Facets/EverclearFacet.t.sol (1)
17-24: Tests comprehensively exercise EverclearFacet, with a small selector-array nit
- setUp configures the facet with the expected selectors, sets up a real FeeAdapter signer, and builds
validEverclearDatafrom a V2-format signature, matching the facet’s fee-handling and docs.- The helper signatures (
createEverclearV2Signature,createEverclearV2SignatureNonEVM) correctly reproduce FeeAdapter’s_sigData/_hashscheme (nativeFee + destinations + receiver + assets + amount + amountOutMin + ttl + data + tokenFee + deadline, then adapter address + chainid, then ERC‑191 prefix).- Tests cover:
- EVM and non‑EVM bridging (including native-fee variants).
- Invalid output assets, invalid/mismatched receivers, invalid refundReceiver.
- Native asset disallowance and unsupported Everclear chain IDs.
- Fuzzing around
minAmountvsfee.refundExcessNativebehavior viatest_ExcessNativeFeeGetsRefunded.Minor nit:
bytes4[] memory functionSelectors = new bytes4[](4);only populates indices 0–2. You can drop the unused slot by making this array length 3 to avoid registering a spurious 0x00000000 selector in the test diamond.Also applies to: 64-109, 159-244
Which Jira task belongs to this PR?
LF-14786
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)