add converter parameter to TokenWrapper for WETH9-compatible interfac… [IWrapper v1.0.0, TokenWrapper v1.2.0] #1531
Conversation
WalkthroughThis PR extends the TokenWrapper contract to support an optional converter address for wrapping/unwrapping native tokens through an intermediary. Updates deployment scripts across TRON and zkSync chains to pass the converter address, adds test utilities and comprehensive test coverage for converter scenarios, and includes a new stable network configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // solhint-disable-next-line no-empty-blocks | ||
| constructor( | ||
| address _wrappedToken, | ||
| address _converter, |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
There was a problem hiding this comment.
I do not see the validation of the constructor parameters. Why was this marked as fixed? @ezynda3
There was a problem hiding this comment.
we use InvalidConfig() error for this
Test Coverage ReportLine Coverage: 86.11% (2984 / 3465 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Periphery/TokenWrapper.sol (1)
1-2: Update core periphery contract SPDX to matchsrc/**/*.sollicensing standardFor LI.FI-owned Solidity sources under
src/**, the documented standard is// SPDX-License-Identifier: LGPL-3.0-onlywith a blank line beforepragma.TokenWrappercurrently usesUNLICENSED. Unless there is a deliberate legal exception for this contract, it should be updated to the LGPL header to stay consistent with the rest of the codebase and the stated guidelines.
🧹 Nitpick comments (10)
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol (2)
32-43: Converter handling and constructor arg ordering look correctThe optional
converterAddressparsing with a try/catch andaddress(0)fallback is a clean way to keep existing deployments working while enabling the new parameter. Encoding constructor args as(wrappedNativeAddress, converterAddress, refundWalletAddress)matches the newTokenWrapperconstructor signature and keeps ordering consistent across scripts.You could slightly reduce I/O by reading
pathonce into a localstringand reusing it for bothwrappedNativeAddressandconverterAddress, but that’s purely cosmetic here.Also applies to: 59-64
1-2: Align deployment script SPDX header and spacing with repo guidelinesPer
script/**/*.solguidelines, deployment scripts should use// SPDX-License-Identifier: LGPL-3.0-onlyand have a blank line before thepragmastatement. This file currently usesUNLICENSEDand no blank line; consider updating to match the standard.script/deploy/facets/DeployTokenWrapper.s.sol (2)
32-43: Facet deploy constructor args are consistent with core contract and other scriptsThe converter lookup and fallback to
address(0)match the zksync deploy script, and the encoded args(wrappedNativeAddress, converterAddress, refundWalletAddress)align withTokenWrapper’s constructor ordering. That keeps deployments coherent across environments.Given the zksync and facet scripts now share almost identical
getConstructorArgslogic, you might consider extracting a small helper inDeployScriptBaseor a shared util to avoid future drift, but it’s not strictly necessary.Also applies to: 59-64
1-2: Update SPDX header and spacing to matchsrc/**/*.sol/script/**/*.solconventionsAs with the zksync script, this file uses
UNLICENSEDand no blank line beforepragma. The repo guidelines call forLGPL-3.0-onlyand a blank line before the pragma in Solidity scripts. Updating this keeps deployment scripts consistent with the documented standard.script/deploy/tron/deploy-and-register-periphery.ts (1)
805-815: TokenWrapper constructor args and converter handling on Tron are consistent, with minor DRY opportunityThe new
converterHexderivation and expandedconstructorArgs[wrappedNativeHex, converterHex, refundWalletHex]correctly mirror the updatedTokenWrapperconstructor and keep behavior backward-compatible whentronConfig.converterAddressis unset (zero address). The additional logging around the converter address is also useful operationally.The only nit is duplication: the
converterHexcomputation and the associatedconsola.info("Using converter: ...")block are repeated in both redeploy and fresh-deploy branches. You could computeconverterHexonce just before those branches and reuse it, which would make future changes easier. Also, please double-check thattronConfig.converterAddressinconfig/networks.jsonis always in the formatTronWeb.address.toHexexpects (e.g., base58 Tron address), otherwise the hex conversion here could misbehave.Also applies to: 820-826, 891-901, 906-912
test/solidity/Periphery/TokenWrapper.t.sol (2)
19-23: Constructor usage matches updated TokenWrapper signatureThe updated instantiation
new TokenWrapper(address(wrappedToken), address(0), address(this))correctly matches the new constructor parameters(_wrappedToken, _converter, _owner)and keeps existing tests exercising the “no converter configured” path.If you plan to rely on non-zero converters in production, it could be valuable to add a small harness test with a dummy
IWrapper-compatible converter to cover that branch end-to-end (deposit + withdraw), but the current change itself is sound.
1-2: Test file SPDX header differs from documentedtest/**/*.solconventionGuidelines call for Solidity test files to use
// SPDX-License-Identifier: LGPL-3.0-onlyand a blank line beforepragma. This test currently usesUnlicense. Consider aligning it with the project’s standard unless this file is intentionally kept under a different license for external reasons.src/Periphery/TokenWrapper.sol (3)
21-25: Constructor extension and newconverterstate are consistent, but rely on deployment-time correctnessThe added
converterstate and extended constructorconstructor(address _wrappedToken, address _converter, address _owner)match how all deployment scripts and tests now encode arguments (wrappedNative,converter,refundWallet/owner), so the wiring looks consistent.You’re intentionally allowing
_converterto beaddress(0)as a sentinel for “no converter”, which deposit/withdraw respect. That’s fine, but note that there is still no validation on_wrappedTokenor_owner; a misconfigured deployment could leave the contract unusable. Given prior patterns in this repo (constructor arg validation handled in scripts rather than on-chain), this is probably acceptable, but worth double-checking that all new networks set a sensible_wrappedTokenand_ownerin config.Also applies to: 33-39
45-49: Converter-based wrap/unwrap flow is coherent; ensure configured converters truly match the expected interfaceThe updated
deposit/withdrawlogic correctly:
- Selects
wrapperAddress = converter != address(0) ? converter : wrappedTokenso legacy deployments (no converter) keep using the wrapped token directly.- On
withdraw, transfers the caller’s entirewrappedTokenbalance into this contract, optionally sets an allowance forconverterviaLibAsset.maxApproveERC20, and then callsIWrapper(wrapperAddress).withdraw(wad)before forwarding the received native tomsg.senderviaSafeTransferLib.safeTransferETH.- Adds
receive()so the contract can accept native tokens fromwrappedTokenor any configured converter.Functionally this is sound and preserves the previous behavior when
converter == address(0). The main assumption is that any non-zeroconverteryou wire in:
- Implements the exact
IWrapperinterface (deposit() payable,withdraw(uint wad)), and- Mints/burns the same
wrappedTokenthe contract tracks, in such a way thatTokenWrappercan hold/burn those tokens (i.e.,depositcreditsTokenWrapper, andwithdrawconsumes tokens fromTokenWrapperusing the allowance you set).If a converter deviates from that pattern, these functions could revert or misbehave. I recommend explicitly verifying the deployed converter contracts against this expectation and, if possible, documenting that invariant near the
converterstate variable or in the configuration docs.Also,
LibAsset.maxApproveERC20will (by design) leave a large allowance to the converter; that matches existing patterns in this repo, but it reinforces the requirement thatconverterbe treated as a trusted component.Also applies to: 60-71, 73-74
23-28: Minor clean-up: unused constant and error
MAX_INTandWithdrawFailuredon’t appear to be used anymore. They can be safely removed to reduce clutter unless they’re kept for backward compatibility or near-term planned usage.
📜 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)
script/deploy/facets/DeployTokenWrapper.s.sol(2 hunks)script/deploy/tron/deploy-and-register-periphery.ts(2 hunks)script/deploy/zksync/DeployTokenWrapper.zksync.s.sol(2 hunks)src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Periphery/TokenWrapper.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
script/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
All Solidity deployment and task script files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement
Files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/facets/*.s.sol
📄 CodeRabbit inference engine (conventions.md)
script/deploy/facets/*.s.sol: Facet deployment scripts must inherit DeployScriptBase, call deploy() with type(FacetName).creationCode. For facets without constructor args, no getConstructorArgs() function needed. For facets with constructor args, implement getConstructorArgs() returning encoded abi.encode(arg1, arg2)
Facet update scripts must inherit UpdateScriptBase, call update("{ContractName}"), and use getExcludes() for special cases returning array of excluded function selectors
Files:
script/deploy/facets/DeployTokenWrapper.s.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/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.sol
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/Periphery/TokenWrapper.sol
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/deploy/tron/deploy-and-register-periphery.ts
🧠 Learnings (30)
📓 Common learnings
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1140
File: deployments/fantom.diamond.json:97-105
Timestamp: 2025-06-25T06:27:38.873Z
Learning: When contracts are redeployed, they receive new addresses. Permit2Proxy addresses in deployment files reflect the actual deployed contract addresses for each network, not a standardized address across all networks.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.
📚 Learning: 2024-11-25T13:49:40.464Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.solscript/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2024-11-25T13:50:34.522Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:13-20
Timestamp: 2024-11-25T13:50:34.522Z
Learning: Contracts on ZkSync will always have different addresses compared to other EVM networks. Do not highlight address differences for ZkSync when verifying deployment configurations or in log files.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
📚 Learning: 2025-01-30T08:16:28.174Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.zksync.s.sol:40-46
Timestamp: 2025-01-30T08:16:28.174Z
Learning: Validation for withdraw wallet address is not required in DeployExecutor.zksync.s.sol as it's handled elsewhere in the system.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
📚 Learning: 2025-01-23T02:26:29.900Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.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:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.solscript/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2025-06-25T06:27:38.873Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1140
File: deployments/fantom.diamond.json:97-105
Timestamp: 2025-06-25T06:27:38.873Z
Learning: When contracts are redeployed, they receive new addresses. Permit2Proxy addresses in deployment files reflect the actual deployed contract addresses for each network, not a standardized address across all networks.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.solscript/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2024-10-04T09:17:19.275Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
📚 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/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.solscript/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2025-01-09T04:34:00.778Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 909
File: script/deploy/zksync/DeployTokenWrapper.s.sol:43-49
Timestamp: 2025-01-09T04:34:00.778Z
Learning: In deployment scripts for LiFi contracts, validation for refundWallet address is not required as it's handled elsewhere or through configuration validation.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
Applied to files:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.solsrc/Periphery/TokenWrapper.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:
script/deploy/zksync/DeployTokenWrapper.zksync.s.solscript/deploy/facets/DeployTokenWrapper.s.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
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/facets/DeployTokenWrapper.s.sol
📚 Learning: 2024-10-31T09:07:36.301Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 843
File: script/deploy/facets/DeployRelayFacet.s.sol:0-0
Timestamp: 2024-10-31T09:07:36.301Z
Learning: In `script/deploy/facets/DeployRelayFacet.s.sol`, additional validation for the configuration file existence and address validity is unnecessary because the code will already fail appropriately if the configuration file is missing or the addresses are invalid.
Applied to files:
script/deploy/facets/DeployTokenWrapper.s.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/facets/DeployTokenWrapper.s.sol
📚 Learning: 2025-07-03T07:34:47.349Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1184
File: script/deploy/safe/deploy-safe.ts:0-0
Timestamp: 2025-07-03T07:34:47.349Z
Learning: In `script/deploy/safe/deploy-safe.ts`, 0xDEnYO intentionally changed the `allowOverride` flag default from `false` to `true` to allow overwriting existing Safe addresses by default in deployment workflows.
Applied to files:
script/deploy/facets/DeployTokenWrapper.s.solscript/deploy/tron/deploy-and-register-periphery.ts
📚 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/facets/DeployTokenWrapper.s.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:
script/deploy/facets/DeployTokenWrapper.s.soltest/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-08-29T11:12:02.184Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.
Applied to files:
test/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
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:
test/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Periphery/TokenWrapper.t.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-28T14:26:29.054Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-20T09:56:24.259Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-04-28T07:46:24.084Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-02-24T09:35:34.908Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
Applied to files:
script/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2025-04-22T09:04:44.244Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.
Applied to files:
script/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2025-04-21T03:15:12.236Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.
Applied to files:
script/deploy/tron/deploy-and-register-periphery.ts
📚 Learning: 2025-01-21T11:07:36.236Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 937
File: script/deploy/facets/DeployLiFiDEXAggregator.s.sol:33-35
Timestamp: 2025-01-21T11:07:36.236Z
Learning: In the LiFiDEXAggregator contract deployment (DeployLiFiDEXAggregator.s.sol), the refundWallet address from global.json is used as the owner parameter in the contract's constructor, which is then passed to the WithdrawablePeriphery base contract.
Applied to files:
script/deploy/tron/deploy-and-register-periphery.ts
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/TokenWrapper.sol
[notice] 34-34:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
… converter support
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Periphery/TokenWrapper.sol (1)
1-2: Update SPDX identifier for periphery contractPer the coding guidelines, LI.FI Solidity contracts under
src/should useLGPL-3.0-onlyas the license identifier. This file currently usesUNLICENSED.Suggested change:
-// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: LGPL-3.0-only
🧹 Nitpick comments (4)
test/solidity/utils/TestBasicToken.sol (1)
1-22: Align SPDX license with repo conventionsImplementation looks fine for a test helper (simple ERC20 with unrestricted mint/burn is appropriate in this context). However, the SPDX header currently uses
Unlicense, while the conventions call forLGPL-3.0-onlyon LI.FI-authored Solidity files.Consider updating the header:
-// SPDX-License-Identifier: Unlicense +// SPDX-License-Identifier: LGPL-3.0-onlytest/solidity/utils/TestConverterWithDecimals.sol (1)
1-56: Decimal converter helper is sound; consider minor cleanupsThe 18↔6 decimal conversion and balance flows look correct for the intended test usage, and they integrate cleanly with
TokenWrapper’s balance-delta accounting.Two small optional cleanups:
error InvalidAmount();is currently unused; either remove it or switch therequireindeposit()to use the custom error, e.g.:if (msg.value % 1e12 != 0) { revert InvalidAmount(); }To keep SPDX identifiers consistent with the rest of the repo, you may want to switch to
LGPL-3.0-onlyhere as well:-// SPDX-License-Identifier: Unlicense
+// SPDX-License-Identifier: LGPL-3.0-only</blockquote></details> <details> <summary>src/Periphery/TokenWrapper.sol (1)</summary><blockquote> `21-43`: **Converter integration and balance‑delta accounting look correct; a couple of small nits** The new converter wiring (`converter`, `wrapperAddress`, `useConverter`) and the updated `deposit`/`withdraw` flows look sound: - With `useConverter == false`, behavior matches the previous WETH9-style wrapper: deposit forwards ETH to the wrapped token and transfers `msg.value` WETH, withdraw pulls the full token balance and unwraps 1:1. - With `useConverter == true`, using balance deltas (`IERC20(wrappedToken).balanceOf` and `address(this).balance`) to compute `amountReceived` is a good way to make the wrapper agnostic to converter-specific decimals and fees and will revert on underflow if the converter misbehaves. - The approval pattern `LibAsset.maxApproveERC20(IERC20(wrappedToken), converter, wad)` is appropriate to let the converter pull tokens from the wrapper without repeated approvals. Constructor-level parameter validation isn’t needed here because deployRequirements.json enforces correct deployment config before these scripts run. Based on learnings, this matches existing repo practice. Minor cleanups you might consider: - `error WithdrawFailure();` is no longer used; removing it would slightly reduce bytecode size. - `IERC20(wrappedToken).approve(address(this), MAX_INT);` in the constructor sets an allowance from the deployer to this contract, which isn’t used anywhere in the current logic (all flows rely on user → wrapper approvals and wrapper → converter approvals). It can likely be dropped. Also applies to: 48-87 </blockquote></details> <details> <summary>test/solidity/utils/TestWrappedConverter.sol (1)</summary><blockquote> `1-44`: **WETH9‑style test converter looks correct; optional tidy‑ups** The mock converter behavior (deposit sending wrapped tokens equal to `msg.value`, withdraw pulling tokens via `transferFrom` and sending back ETH) matches how `TokenWrapper` uses a WETH9-compatible interface, so it’s suitable for testing. Optional cleanups: - `error InsufficientBalance(uint256 available, uint256 required);` is unused; you could remove it or use it in `withdraw` if you later add explicit balance checks. - For consistency with other LI.FI Solidity, consider switching the SPDX to `LGPL-3.0-only` if these test helpers are meant to follow the same licensing: ```diff -// SPDX-License-Identifier: Unlicense +// SPDX-License-Identifier: LGPL-3.0-only
📜 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)
src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Periphery/TokenWrapper.t.sol(2 hunks)test/solidity/utils/TestBasicToken.sol(1 hunks)test/solidity/utils/TestConverterWithDecimals.sol(1 hunks)test/solidity/utils/TestWrappedConverter.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/utils/TestWrappedConverter.soltest/solidity/utils/TestConverterWithDecimals.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.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/Periphery/TokenWrapper.t.sol
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/Periphery/TokenWrapper.sol
🧠 Learnings (30)
📓 Common learnings
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.
📚 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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/utils/TestConverterWithDecimals.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/utils/TestConverterWithDecimals.soltest/solidity/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/utils/TestConverterWithDecimals.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
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/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.sol
📚 Learning: 2025-06-07T04:11:20.100Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.
Applied to files:
test/solidity/utils/TestWrappedConverter.sol
📚 Learning: 2025-01-22T12:38:37.557Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/utils/TestBase.sol:446-470
Timestamp: 2025-01-22T12:38:37.557Z
Learning: Test utility functions in TestBase.sol don't require production-level validations like slippage checks and input validation since they operate in a controlled test environment.
Applied to files:
test/solidity/utils/TestConverterWithDecimals.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.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:
test/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-06-05T14:48:58.816Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: src/Periphery/Patcher.sol:160-167
Timestamp: 2025-06-05T14:48:58.816Z
Learning: In src/Periphery/Patcher.sol, the unlimited token approval to finalTarget in depositAndExecuteWithDynamicPatches and depositAndExecuteWithMultiplePatches functions is intentional behavior, as confirmed by ezynda3. This design choice aligns with the contract's stateless/ephemeral architecture.
Applied to files:
test/solidity/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.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/Periphery/TokenWrapper.t.soltest/solidity/utils/TestBasicToken.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.sol
📚 Learning: 2024-09-23T01:48:39.325Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-16T09:28:18.027Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-24T08:13:01.516Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 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:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:50.790Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-29T11:12:02.184Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-05T14:28:34.813Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:55.549Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-23T02:26:29.900Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-28T14:26:29.054Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-20T09:56:24.259Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-04-28T07:46:24.084Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/TokenWrapper.sol
[notice] 36-36:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
🔇 Additional comments (1)
test/solidity/Periphery/TokenWrapper.t.sol (1)
8-27: New converter and decimal‑converter tests align well with the updated TokenWrapperThe additional setup and tests exercise the key new paths:
tokenWrapperWithConverter+TestWrappedConverterverify that ETH→ERC20 and ERC20→ETH flows work via the converter and that converter allowances are granted only when needed (testConverterReceivesApproval).tokenWrapperWithDecimalConverter+TestConverterWithDecimalscorrectly model 18↔6 decimal conversions, including a full round-trip (testDecimalConverterRoundTrip), and assert balances in the right units (1e6 vs 1e18).This gives solid coverage for both the WETH9-compatible and converter-based behaviors introduced in
TokenWrapper.Also applies to: 39-66, 100-215
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Periphery/TokenWrapper.sol (1)
34-43: Constructor logic is correct for optional converter support.The handling of
_converterbeingaddress(0)is intentional and correctly setsUSE_CONVERTER = falsefor the non-converter path. The static analysis warning is a false positive - based on learnings, the deployment flow validates constructor arguments throughdeployRequirements.json.
🧹 Nitpick comments (1)
src/Periphery/TokenWrapper.sol (1)
1-1: License identifier should be LGPL-3.0-only for LI.FI code.As per coding guidelines, all LI.FI Solidity files should use
LGPL-3.0-onlylicense identifier.-// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: LGPL-3.0-only
📜 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/Periphery/TokenWrapper.sol(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/Periphery/TokenWrapper.sol
🧠 Learnings (20)
📓 Common learnings
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.
📚 Learning: 2024-09-23T01:48:39.325Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-16T09:28:18.027Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-24T08:13:01.516Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 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:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:50.790Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-29T11:12:02.184Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-05T14:28:34.813Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:55.549Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-23T02:26:29.900Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-28T14:26:29.054Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-20T09:56:24.259Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-04-28T07:46:24.084Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-28T22:41:58.814Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol:387-395
Timestamp: 2025-08-28T22:41:58.814Z
Learning: In the lifinance/contracts repository, SafeERC20's safeApprove() method handles the two-step approval pattern internally when needed, so manual reset-to-zero followed by approval is unnecessary. Simply changing from IERC20.approve() to IERC20.safeApprove() provides the safety benefits.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/TokenWrapper.sol
[notice] 36-36:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
🔇 Additional comments (3)
src/Periphery/TokenWrapper.sol (3)
18-27: Good use of immutable variables for gas optimization and security.The state variable design is clean - using immutable for all configuration values ensures they cannot be modified after deployment and saves gas on reads.
74-86: Withdrawal logic correctly handles both converter and non-converter paths.The balance-before/after pattern properly accounts for any differences in the converted amount, and the approval is correctly scoped to the converter. Good use of
SafeTransferLib.safeTransferETHfor native token transfers.
89-91: LGTM!The
receive()function is necessary to accept ETH from the wrapped token'swithdraw()call.
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Periphery/TokenWrapper.sol (1)
18-41: Immutable configuration and wrapper/converter routing are well-structuredUsing immutables for
WRAPPED_TOKEN,CONVERTER,WRAPPER_ADDRESS, andUSE_CONVERTER, withWRAPPER_ADDRESSset to either the converter or the wrapped token based on_converter != address(0), gives a clear and gas-efficient split between the direct-wrapper and converter-backed modes. This matches how the updated tests and deploy scripts intend to useTokenWrapper, and is consistent with the repository’s pattern of relying on external deployment validation instead of in-contract constructor parameter checks.
🧹 Nitpick comments (3)
test/solidity/Periphery/TokenWrapper.t.sol (1)
163-216: Decimal-converter tests thoroughly validate 18↔6 conversion; consider a minor extra assertionThe decimal tests validate:
- 1 ETH → 1e6 units of the 6-dec token on deposit.
- 1e6 units → 1 ETH on withdraw.
- Round-trip behavior for 5 ETH using both deposit and withdraw.
To tighten the round-trip test slightly, you could also assert that
token6Decimals.balanceOf(address(this)) == 0after the final withdraw to mirror the non-decimal converter test pattern, but this is purely optional.src/Periphery/TokenWrapper.sol (2)
12-16: Align contract-level NatSpec order with guidelines and reconcile version bump with audit checkPer the repo’s Solidity guidelines, the contract header should order tags as
@title,@notice,@author,@custom:version. Here@authorprecedes@notice; consider swapping those two lines to be consistent with the standard format.Also, the custom version was bumped to
1.2.0and the pipeline reports no logged audit for this version. Please ensure the audit records/config are updated accordingly (or adjust the version) so theVersionControlAndAuditVerificationcheck passes.
45-99: Deposit/withdraw logic correctly handles both direct-wrapper and converter paths; consider a tiny withdraw(0) optimizationThe updated
depositandwithdrawimplementations:
- Correctly use balance-before/after deltas in the converter path so that decimal conversions or fees don’t assume 1:1 behavior.
- Keep the non-converter path equivalent to the original WETH9-style 1:1 wrap/unwrap semantics.
- Use
SafeTransferLibconsistently for ERC20 and ETH transfers, which addresses prior concerns about rawtransfer/transferFrom.As a minor optional optimization, you could early-return in
withdraw()whenwad == 0to avoid a no-op approval and externalwithdraw(0)call in edge cases, but functionally the current code is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
script/deploy/facets/DeployTokenWrapper.s.sol(3 hunks)script/deploy/zksync/DeployTokenWrapper.zksync.s.sol(3 hunks)src/Interfaces/IWrapper.sol(1 hunks)src/Periphery/TokenWrapper.sol(1 hunks)test/solidity/Periphery/TokenWrapper.t.sol(2 hunks)test/solidity/utils/TestBasicToken.sol(1 hunks)test/solidity/utils/TestConverterWithDecimals.sol(1 hunks)test/solidity/utils/TestWrappedConverter.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/solidity/utils/TestBasicToken.sol
- script/deploy/facets/DeployTokenWrapper.s.sol
- script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
🧰 Additional context used
📓 Path-based instructions (4)
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/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.soltest/solidity/utils/TestConverterWithDecimals.sol
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/Interfaces/IWrapper.solsrc/Periphery/TokenWrapper.sol
src/Interfaces/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/Interfaces/*.sol: All interfaces must start with I prefix (e.g., ILiFi, IStargate) and be placed in separate files in src/Interfaces directory, not in the same file as their implementation
Every interface must have NatSpec documentation with @title, @notice, @author (always 'LI.FI (https://li.fi)'), and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Files:
src/Interfaces/IWrapper.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/Periphery/TokenWrapper.t.sol
🧠 Learnings (40)
📓 Common learnings
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.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.
📚 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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.soltest/solidity/utils/TestConverterWithDecimals.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:
test/solidity/utils/TestWrappedConverter.solsrc/Interfaces/IWrapper.soltest/solidity/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.soltest/solidity/utils/TestConverterWithDecimals.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-06-07T04:11:20.100Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.
Applied to files:
test/solidity/utils/TestWrappedConverter.solsrc/Periphery/TokenWrapper.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:
test/solidity/utils/TestWrappedConverter.soltest/solidity/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.soltest/solidity/utils/TestConverterWithDecimals.sol
📚 Learning: 2025-12-02T10:08:06.478Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to src/Interfaces/*.sol : All interfaces must start with I prefix (e.g., ILiFi, IStargate) and be placed in separate files in src/Interfaces directory, not in the same file as their implementation
Applied to files:
src/Interfaces/IWrapper.sol
📚 Learning: 2025-12-12T11:16:09.236Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1530
File: src/Interfaces/ICircleBridgeProxy.sol:20-29
Timestamp: 2025-12-12T11:16:09.236Z
Learning: In src/Interfaces/ICircleBridgeProxy.sol, ensure the ICircleBridgeProxy interface corresponds to Celer's CircleBridgeProxy contract (not Circle's native TokenMessenger). The depositForBurn signature must be: depositForBurn(uint256 _amount, uint64 _dstChid, bytes32 _mintRecipient, address _burnToken, uint256 _maxFee, uint32 _minFinalityThreshold) external. This change should be scoped specifically to this file; pattern is not needed since it targets a single file. This improves correctness by aligning the interface with the correct contract and parameter types (dst chain id as uint64, no bytes32 destinationCaller parameter).
Applied to files:
src/Interfaces/IWrapper.solsrc/Periphery/TokenWrapper.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:
test/solidity/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-29T11:33:37.368Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:1-3
Timestamp: 2025-08-29T11:33:37.368Z
Learning: For test files in test/solidity/Periphery/LDA/**, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol.
Applied to files:
test/solidity/Periphery/TokenWrapper.t.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:
test/solidity/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-08-27T22:28:47.277Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/Facets/SyncSwapV2Facet.t.sol:278-304
Timestamp: 2025-08-27T22:28:47.277Z
Learning: The _buildRouteAndExecuteSwap helper function in LDA tests (like in test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol and similar) handles token approvals internally, so manual tokenIn.approve() calls are not needed in individual test functions when using this helper.
Applied to files:
test/solidity/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-01-22T12:38:37.557Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/utils/TestBase.sol:446-470
Timestamp: 2025-01-22T12:38:37.557Z
Learning: Test utility functions in TestBase.sol don't require production-level validations like slippage checks and input validation since they operate in a controlled test environment.
Applied to files:
test/solidity/Periphery/TokenWrapper.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/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.solsrc/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-27T22:29:00.940Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/Facets/SyncSwapV2Facet.t.sol:312-323
Timestamp: 2025-08-27T22:29:00.940Z
Learning: In the LDA test suite, the `_buildRouteAndExecuteSwap` helper function already handles token approvals internally, so manual approval calls are not needed in individual test functions.
Applied to files:
test/solidity/Periphery/TokenWrapper.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/Periphery/TokenWrapper.t.sol
📚 Learning: 2025-11-04T16:27:25.773Z
Learnt from: RnkSngh
Repo: lifinance/contracts PR: 1422
File: src/Interfaces/ITokenMessenger.sol:1-3
Timestamp: 2025-11-04T16:27:25.773Z
Learning: For external/third-party interfaces in src/Interfaces/ (e.g., ITokenMessenger.sol for Polymer/CCTP), the team does not require following the project's NatSpec, licensing (LGPL-3.0-only), or pragma conventions, as these interfaces represent contracts the team doesn't control.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2024-09-23T01:48:39.325Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-16T09:28:18.027Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-24T08:13:01.516Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 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:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:50.790Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-29T11:12:02.184Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-05T14:28:34.813Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-07-17T04:21:55.549Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-06-05T15:04:58.212Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/cowSwapHelpers.ts:112-114
Timestamp: 2025-06-05T15:04:58.212Z
Learning: For demo scripts in the lifinance/contracts repository, security concerns like using Math.random() for nonce generation are acceptable since the focus is on demonstrating functionality rather than production-ready security implementation.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-20T09:56:24.259Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.
Applied to files:
src/Periphery/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/TokenWrapper.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/TokenWrapper.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/TokenWrapper.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/TokenWrapper.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/Periphery/TokenWrapper.sol
📚 Learning: 2025-08-26T12:11:21.073Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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/TokenWrapper.sol
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
Applied to files:
src/Periphery/TokenWrapper.sol
📚 Learning: 2025-01-23T02:26:29.900Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.
Applied to files:
src/Periphery/TokenWrapper.sol
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Periphery/TokenWrapper.sol
[error] 1-1: Could not find any logged audit for contract TokenWrapper in version 1.2.0.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (5)
src/Interfaces/IWrapper.sol (1)
5-13: IWrapper definition matches WETH9-style interface and project conventionsLicense, pragma, interface-level NatSpec, and function signatures (including
uint wadwith the solhint override) are all consistent with the intended WETH9-compatible usage inTokenWrapper. No changes needed.test/solidity/utils/TestWrappedConverter.sol (1)
7-42: TestWrappedConverter correctly models a WETH9-compatible converter for TokenWrapper testsDeposit/withdraw flows, error handling, and the receive hook line up with how
TokenWrapperinteracts with a converter (ETH in, wrapped tokens out; wrapped tokens in, ETH out). This is appropriate as a focused test helper.test/solidity/utils/TestConverterWithDecimals.sol (1)
7-54: Decimal conversion helper behaves consistently with GasUSDT0-style semanticsThe 1e12 scaling, divisibility check in
deposit, and mirrored conversion inwithdrawgive predictable 18↔6 decimal behavior that matches the new TokenWrapper decimal-converter tests. This is suitable as a dedicated test utility.test/solidity/Periphery/TokenWrapper.t.sol (2)
31-67: Constructor wiring for all three TokenWrapper instances looks correct
tokenWrapper,tokenWrapperWithConverter, andtokenWrapperWithDecimalConverterall pass(wrappedToken, converterOrZero, owner)in line with the updated 3-arg constructor. Dealing and minting to the converter contracts ensures they have sufficient ERC20 and ETH balances for the deposit/withdraw test flows.
101-161: Converter-path tests exercise both deposit and withdraw, including allowance behaviorThe new tests for the converter case (
test_CanDepositWithConverter,test_CanWithdrawWithConverter,test_ConverterReceivesApproval) correctly:
- Verify 1:1 token credit on deposit via the converter-backed wrapper.
- Cover the full withdraw path (tokens pulled from the user, ETH forwarded back).
- Assert that
LibAsset.maxApproveERC20results intype(uint256).maxallowance from the wrapper to the converter.This gives good coverage of the new USE_CONVERTER branch and approval logic.
| @@ -1,48 +1,183 @@ | |||
| { | |||
| "comment": "Note: this file is generated by a backend script, do not change it manually!", | |||
There was a problem hiding this comment.
is this comment really true? I doubt it
There was a problem hiding this comment.
Fixed dcb8a6d: File deleted with comment
archive/config/tokenwrapper.json
Outdated
| "wrappedToken": "0x6969696969696969696969696969696969696969", | ||
| "converter": "0x0000000000000000000000000000000000000000" | ||
| }, | ||
| "optimism": { |
There was a problem hiding this comment.
please sort alphabetically as in other config files, too
There was a problem hiding this comment.
Fixed dcb8a6d: Deleted, networks.json already sorted
archive/config/tokenwrapper.json
Outdated
| "wrappedToken": "0xc9bdeed33cd01541e1eed10f90519d2c06fe3feb", | ||
| "converter": "0x0000000000000000000000000000000000000000" | ||
| }, | ||
| "harmony": { |
There was a problem hiding this comment.
nitpick: could be removed entirely
There was a problem hiding this comment.
Fixed dcb8a6d: File removed as suggested
| string.concat(".", network, ".wrappedNativeAddress") | ||
| ); | ||
|
|
||
| // try to get converter address, default to address(0) if not found |
There was a problem hiding this comment.
this means if there is an error in the JSON we deploy with address(0) despite wanting to deploy something with a different address. I dont think this is in our interest.
Since the config file has a converterAddress value for supported network, we dont really need a try/catch here imho.
Instead, we can just read the address from the config file and if it's != address(0) then I believe we should make sure it's a contract address. Wdyt?
| "_wrappedToken": { | ||
| "configFileName": "tokenwrapper.json", | ||
| "keyInConfigFile": ".<NETWORK>.wrappedToken", | ||
| "allowToDeployWithZeroAddress": "true" |
There was a problem hiding this comment.
why would we want to allow to deploy the contract with zero address arguments for _wrappedToken or _owner? This doesnt make any sense to me
And with regards to the converter:
Option 1:
- we require to have a value for _converter , but we allow address(0) values
- we will need a address(0) value for each network in the config file
- we remove the try/catch logic from deploy scripts (since that doesnt match with the intended behaviour)
Option 2 (my preference):
- we do not mention
_converterin deployRequirements at all - in config file:
- we only add converter address for networks where it's non-zero
- we add a comment explaining converterAddress handling that at the top of the file
- in deploy scripts we apply fallback to address(0) logic
There was a problem hiding this comment.
Fixed dcb8a6d: Implemented Option 2 approach
|
|
||
| // ========== CONVERTER TESTS ========== | ||
|
|
||
| function testCanDepositWithConverter() public { |
There was a problem hiding this comment.
all tests are not named according to our naming convention.....
Please also update existing (untouched) tests to match our conventions.
There was a problem hiding this comment.
Fixed: 3ffe919. All tests use test_ prefix
| tokenWrapperWithDecimalConverter.deposit{ value: 1 ether }(); | ||
|
|
||
| // User should now have 1 USDT (1e6, not 1e18) | ||
| assertEq(token6Decimals.balanceOf(address(this)), 1_000_000); |
There was a problem hiding this comment.
the 1_000_000 could be a very meaningful constant in this test contract
There was a problem hiding this comment.
Fixed dcb8a6d: Created ONE_USDT constant
| @@ -0,0 +1,22 @@ | |||
| // SPDX-License-Identifier: Unlicense | |||
There was a problem hiding this comment.
Fixed: 3ffe919. License changed to LGPL-3.0-only
There was a problem hiding this comment.
why is this file in archive folder?
There was a problem hiding this comment.
Fixed dcb8a6d: File deleted, uses networks.json
archive/config/tokenwrapper.json
Outdated
| "wrappedToken": "0x4200000000000000000000000000000000000006", | ||
| "converter": "0x0000000000000000000000000000000000000000" | ||
| }, | ||
| "sonic": { |
There was a problem hiding this comment.
seems like the chain which actually has the converter (stable) is missing here?
There was a problem hiding this comment.
Fixed dcb8a6d: Added stable to networks.json
| "nativeAddress": "0x0000000000000000000000000000000000000000", | ||
| "nativeCurrency": "gUSDT", | ||
| "wrappedNativeAddress": "0x779ded0c9e1022225f8e0630b35a9b54be713736", | ||
| "converterAddress": "0xded1660192d4d82e7c0b628ba556861edbb5cada", |
There was a problem hiding this comment.
if you add this here, we also need to adjust the type in script/common/types.ts
However, I am not sure if this is the right place. I feel a config file for tokenWrapper.json (only containing the converter entry for stable) would be the cleaner approach. But that's just an opinion
There was a problem hiding this comment.
So we actually archived the tokenwrapper.json file in favor of networks.json (that's why it appeared in the archive folder). Not sure we should bring it back if we don't use like 90% of it anyway...
@mirooon what do you think?
There was a problem hiding this comment.
@ezynda3 I think it's more tokenWrapper-specific, I would keep it in tokenWrapper.json
There was a problem hiding this comment.
@ezynda3 to clarify on this:
I was not suggesting to unarchive the file as-is.
It is absolutely correct to pull the wrappedNative address from networks.json to avoid duplicate content.
But the tokenWrapper.json file can just contain the converter address (and only for networks for which one exists), so it would currently be only one entry: stable
| // solhint-disable-next-line no-empty-blocks | ||
| constructor( | ||
| address _wrappedToken, | ||
| address _converter, |
There was a problem hiding this comment.
we use InvalidConfig() error for this
| IWrapper(wrappedToken).deposit{ value: msg.value }(); | ||
| IERC20(wrappedToken).transfer(msg.sender, msg.value); | ||
| if (USE_CONVERTER) { | ||
| uint256 balanceBefore = IERC20(WRAPPED_TOKEN).balanceOf( |
…e support
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)