test: add fee forwarding after swap test case#1705
Draft
Conversation
Contributor
WalkthroughAdded FeeForwarder usage and a new test: deploys FeeForwarder in setup, whitelists its selector, adds a helper that builds a two-step swap (DAI→USDC then feeForwarder.forwardERC20Fees), and a test asserting ERC20 fee forwarding after an ERC20→ERC20 swap. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/solidity/Facets/GenericSwapFacetV3.t.sol (2)
1644-1656: Clarify thefromAmountvalue for the fee forwarding step.The
fromAmountat line 1649 is set tominAmountOut - 10000, but this appears inconsistent with the fee amounts being forwarded:
integratorFee = 10 ** usdc.decimals()= 1,000,000 (1 USDC)fromAmount = minAmountOut - 10000subtracts only 10,000 (0.01 USDC)While this works (the approval will exceed the transfer amount), it's semantically confusing. Consider using the total fees being forwarded as
fromAmountfor clarity:♻️ Suggested clarification
swapData[1] = LibSwap.SwapData( address(feeForwarder), address(feeForwarder), ADDRESS_USDC, ADDRESS_USDC, - minAmountOut - 10000, + integratorFee, abi.encodeWithSelector( feeForwarder.forwardERC20Fees.selector, ADDRESS_USDC, distributions ), false );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1644 - 1656, The fromAmount in the second swap step (the LibSwap.SwapData entry in swapData[1]) is set to minAmountOut - 10000 which is semantically misleading given the fees being forwarded; change the fromAmount to explicitly reflect the actual fee sum being forwarded (e.g., the integratorFee or the sum of values in distributions) so readers can see the approval/transfer intent; update the LibSwap.SwapData.fromAmount for the entry that calls feeForwarder.forwardERC20Fees.selector to use the total fees variable (or compute sum(distributions)) instead of minAmountOut - 10000.
1662-1705: Add balance assertions to verify token flows.The test verifies the event emission and successful execution but lacks assertions to confirm the fee was actually forwarded correctly. Consider adding balance checks similar to other tests in this file (e.g.,
test_leavesNoERC20SendingAssetDustMultiSwap):♻️ Suggested balance assertions
uint256 gasUsed = gasLeftBef - gasleft(); emit log_named_uint("gas used V2: ", gasUsed); + // Verify integrator received the fee + assertEq(usdc.balanceOf(address(0xb33f)), 10 ** usdc.decimals()); + + // Verify receiver got the expected output minus fees + assertGe(usdc.balanceOf(SOME_WALLET), minAmountOut); + + // Verify no tokens left in diamond + assertEq(usdc.balanceOf(address(genericSwapFacet)), 0); + vm.stopPrank(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1662 - 1705, Capture balances before and after calling genericSwapFacetV3.swapTokensMultipleV3ERC20ToERC20 and assert the token flows: record dai.balanceOf(DAI_HOLDER) and usdc.balanceOf(SOME_WALLET) (and optionally dai/usdc balance of address(diamond) or genericSwapFacet if relevant) before the swap, call swapTokensMultipleV3ERC20ToERC20, then assert DAI_HOLDER decreased by the expected fromAmount and SOME_WALLET increased by the expected fee/received amount (use amountIn/minAmountOut or the exact fee value your helper _produceSwapDataMultiswapERC20SwapAndERC20FeeAfter implies); mirror the pattern used in test_leavesNoERC20SendingAssetDustMultiSwap to ensure no dust remains and the fee was forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/solidity/Facets/GenericSwapFacetV3.t.sol`:
- Around line 1644-1656: The fromAmount in the second swap step (the
LibSwap.SwapData entry in swapData[1]) is set to minAmountOut - 10000 which is
semantically misleading given the fees being forwarded; change the fromAmount to
explicitly reflect the actual fee sum being forwarded (e.g., the integratorFee
or the sum of values in distributions) so readers can see the approval/transfer
intent; update the LibSwap.SwapData.fromAmount for the entry that calls
feeForwarder.forwardERC20Fees.selector to use the total fees variable (or
compute sum(distributions)) instead of minAmountOut - 10000.
- Around line 1662-1705: Capture balances before and after calling
genericSwapFacetV3.swapTokensMultipleV3ERC20ToERC20 and assert the token flows:
record dai.balanceOf(DAI_HOLDER) and usdc.balanceOf(SOME_WALLET) (and optionally
dai/usdc balance of address(diamond) or genericSwapFacet if relevant) before the
swap, call swapTokensMultipleV3ERC20ToERC20, then assert DAI_HOLDER decreased by
the expected fromAmount and SOME_WALLET increased by the expected fee/received
amount (use amountIn/minAmountOut or the exact fee value your helper
_produceSwapDataMultiswapERC20SwapAndERC20FeeAfter implies); mirror the pattern
used in test_leavesNoERC20SendingAssetDustMultiSwap to ensure no dust remains
and the fee was forwarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d482f911-5876-4548-8986-83fb82db579b
📒 Files selected for processing (1)
test/solidity/Facets/GenericSwapFacetV3.t.sol
Collaborator
Test Coverage ReportLine Coverage: 87.33% (3007 / 3443 lines) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/solidity/Facets/GenericSwapFacetV3.t.sol (4)
1659-1661: Remove extra blank lines.There are multiple consecutive blank lines before and after the test function. Per coding guidelines, use a single blank line between function declarations.
Suggested fix
minAmountOut = minAmountOut - integratorFee; } - function test_CanCollectERC20FeesAfterSwapToERC20_V2() public {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1659 - 1661, Remove the extra blank lines surrounding the test function in test/solidity/Facets/GenericSwapFacetV3.t.sol so that there is only a single blank line between consecutive function declarations; locate the trailing blank lines after the test function (near the end of the file where the closing brace for the test is present) and collapse them down to one blank line to comply with the coding guideline.
1705-1707: Remove extra blank lines after test function.There are multiple consecutive blank lines after the test function.
Suggested fix
vm.stopPrank(); } - // MULTISWAP COLLECT NATIVE FEE AND SWAP TO ERC20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1705 - 1707, There are multiple consecutive blank lines after the end of a test function in test/solidity/Facets/GenericSwapFacetV3.t.sol — remove the extra blank lines immediately following the function closing brace (the trailing "}" that ends the test) so there's only a single newline separating this function from the next code block; in short, trim consecutive blank lines after the test function's closing brace to one.
1634-1636: Remove extra blank line.There are two consecutive blank lines between the swap data assignments. Per coding guidelines, use a single blank line between logical sections.
Suggested fix
true ); - // Swap2: Forward ERC20 fees via FeeForwarder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1634 - 1636, There is an extra blank line between the Swap2 comment ("// Swap2: Forward ERC20 fees via FeeForwarder") and the surrounding swap data assignments; remove the extra empty line so only a single blank line separates logical sections (ensure the "// Swap2: Forward ERC20 fees via FeeForwarder" comment sits with a single blank line above/below the adjacent swap assignment code in the GenericSwapFacetV3 test around the Swap2 setup).
1662-1705: Consider adding balance assertions to verify fee forwarding behavior.The test only verifies event emission but doesn't assert the actual state changes. Given the commit message emphasizes "only forward exact fee to forwarder", adding balance checks would strengthen the test coverage:
- Verify the integrator (
0xb33f) received exactlyintegratorFee- Verify
SOME_WALLETreceivedminAmountOutof USDC- Verify the diamond has no USDC dust remaining
Suggested assertions
uint256 gasUsed = gasLeftBef - gasleft(); emit log_named_uint("gas used V2: ", gasUsed); + // Verify fee was forwarded to integrator + assertEq( + usdc.balanceOf(address(0xb33f)), + 1 * 10 ** usdc.decimals(), + "Integrator should receive exact fee" + ); + + // Verify receiver got the expected amount + assertEq( + usdc.balanceOf(SOME_WALLET), + minAmountOut, + "Receiver should receive minAmountOut" + ); + + // Verify no dust left in diamond + assertEq( + usdc.balanceOf(address(genericSwapFacet)), + 0, + "Diamond should have no USDC dust" + ); + vm.stopPrank(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/solidity/Facets/GenericSwapFacetV3.t.sol` around lines 1662 - 1705, The test test_CanCollectERC20FeesAfterSwapToERC20_V2 currently only checks the event; add assertions after calling genericSwapFacetV3.swapTokensMultipleV3ERC20ToERC20 to verify actual balances: assert the integrator address (0xb33f or the constant used for integrator) received exactly integratorFee, assert SOME_WALLET's USDC balance increased by minAmountOut, and assert the diamond contract's USDC balance is zero (no dust). Use the ERC20 balanceOf on ADDRESS_USDC (and the integrator address/SOME_WALLET/diamond) and existing assertEq helpers to perform these checks; reference the test function name and variables integratorFee, minAmountOut, SOME_WALLET, ADDRESS_USDC, and diamond to locate where to insert the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/solidity/Facets/GenericSwapFacetV3.t.sol`:
- Around line 1659-1661: Remove the extra blank lines surrounding the test
function in test/solidity/Facets/GenericSwapFacetV3.t.sol so that there is only
a single blank line between consecutive function declarations; locate the
trailing blank lines after the test function (near the end of the file where the
closing brace for the test is present) and collapse them down to one blank line
to comply with the coding guideline.
- Around line 1705-1707: There are multiple consecutive blank lines after the
end of a test function in test/solidity/Facets/GenericSwapFacetV3.t.sol — remove
the extra blank lines immediately following the function closing brace (the
trailing "}" that ends the test) so there's only a single newline separating
this function from the next code block; in short, trim consecutive blank lines
after the test function's closing brace to one.
- Around line 1634-1636: There is an extra blank line between the Swap2 comment
("// Swap2: Forward ERC20 fees via FeeForwarder") and the surrounding swap data
assignments; remove the extra empty line so only a single blank line separates
logical sections (ensure the "// Swap2: Forward ERC20 fees via FeeForwarder"
comment sits with a single blank line above/below the adjacent swap assignment
code in the GenericSwapFacetV3 test around the Swap2 setup).
- Around line 1662-1705: The test test_CanCollectERC20FeesAfterSwapToERC20_V2
currently only checks the event; add assertions after calling
genericSwapFacetV3.swapTokensMultipleV3ERC20ToERC20 to verify actual balances:
assert the integrator address (0xb33f or the constant used for integrator)
received exactly integratorFee, assert SOME_WALLET's USDC balance increased by
minAmountOut, and assert the diamond contract's USDC balance is zero (no dust).
Use the ERC20 balanceOf on ADDRESS_USDC (and the integrator
address/SOME_WALLET/diamond) and existing assertEq helpers to perform these
checks; reference the test function name and variables integratorFee,
minAmountOut, SOME_WALLET, ADDRESS_USDC, and diamond to locate where to insert
the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53570718-ae4a-47bb-bb39-0289a71a9979
📒 Files selected for processing (1)
test/solidity/Facets/GenericSwapFacetV3.t.sol
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test how flexible we can collect fees in different tokens/order in our processes.