adds WithdrawablePeriphery base contract for token withdrawals #831
Conversation
WalkthroughThis pull request introduces an abstract contract named Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Learnings (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Coverage ReportLine Coverage: 78.44% (1700 / 2167 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/Helpers/WithdrawablePeriphery.sol (1 hunks)
- test/solidity/Helpers/WithdrawablePeriphery.t.sol (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Helpers/WithdrawablePeriphery.sol (2)
Learnt from: 0xDEnYO PR: lifinance/contracts#782 File: src/Helpers/WithdrawablePeriphery.sol:20-34 Timestamp: 2024-10-09T03:47:21.269Z Learning: In the `WithdrawablePeriphery.sol` contract, for admin-only functions protected by the `onlyOwner` modifier, it's acceptable to omit balance checks and reentrancy protection since admins are trusted to handle these functions appropriately.Learnt from: 0xDEnYO PR: lifinance/contracts#782 File: src/Helpers/WithdrawablePeriphery.sol:20-34 Timestamp: 2024-10-07T02:44:36.866Z Learning: In the `WithdrawablePeriphery.sol` contract, for admin-only functions protected by the `onlyOwner` modifier, it's acceptable to omit balance checks and reentrancy protection since admins are trusted to handle these functions appropriately.
| if (LibAsset.isNativeAsset(assetId)) { | ||
| // solhint-disable-next-line avoid-low-level-calls | ||
| (bool success, ) = receiver.call{ value: amount }(""); | ||
| if (!success) revert ExternalCallFailed(); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safeTransferETH instead of low-level call for Ether transfers
Consider using SafeTransferLib.safeTransferETH for transferring Ether to the receiver. This method provides a safer abstraction over the low-level call and handles potential issues internally.
Apply this diff to replace the low-level call with safeTransferETH:
if (LibAsset.isNativeAsset(assetId)) {
- // solhint-disable-next-line avoid-low-level-calls
- (bool success, ) = receiver.call{ value: amount }("");
- if (!success) revert ExternalCallFailed();
+ receiver.safeTransferETH(amount);
} else {
assetId.safeTransfer(receiver, amount);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (LibAsset.isNativeAsset(assetId)) { | |
| // solhint-disable-next-line avoid-low-level-calls | |
| (bool success, ) = receiver.call{ value: amount }(""); | |
| if (!success) revert ExternalCallFailed(); | |
| } else { | |
| if (LibAsset.isNativeAsset(assetId)) { | |
| receiver.safeTransferETH(amount); | |
| } else { |
| event TokensWithdrawn( | ||
| address assetId, | ||
| address payable receiver, | ||
| uint256 amount | ||
| ); | ||
| error UnAuthorized(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider importing event and error declarations from the main contract
Rather than redeclaring the TokensWithdrawn event and UnAuthorized error within the test contract, it would be better to import these declarations directly from the WithdrawablePeriphery contract. This ensures consistency and reduces the risk of discrepancies if the definitions change in the main contract.
| address payable receiver, | ||
| uint256 amount | ||
| ); | ||
| error UnAuthorized(); |
There was a problem hiding this comment.
Correct the typo in the error name 'UnAuthorized'
The error name UnAuthorized contains an uppercase 'A' in the middle, which is unconventional and may lead to confusion. Consider renaming it to Unauthorized to adhere to standard naming conventions and improve readability.
| function testRevert_FailsIfNativeTokenTransferFails() public { | ||
| uint256 withdrawAmount = 10 * 10 ** usdc.decimals(); | ||
|
|
||
| address nonETHReceiver = address(new NonETHReceiver()); | ||
|
|
||
| vm.startPrank(USER_SENDER); | ||
|
|
||
| vm.expectRevert(UnAuthorized.selector); | ||
|
|
||
| withdrawable.withdrawToken( | ||
| ADDRESS_USDC, | ||
| payable(nonETHReceiver), | ||
| withdrawAmount | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mismatch between test function name and its implementation
The function testRevert_FailsIfNativeTokenTransferFails suggests that it tests the failure of native token transfers. However, the implementation is attempting to withdraw ERC20 tokens (ADDRESS_USDC). To maintain clarity, either update the function name to reflect that it tests ERC20 token transfer failures or modify the implementation to test native token transfer failures.
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-10387
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)