test: cover EthTransferFailed when pre-funded ETH skips WETH withdrawal - #21
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe test suite adds coverage for failed native ETH settlement when prefunded exchange ETH skips WETH withdrawal. It verifies transaction rollback and updates recorded gas values, including the new test. ChangesPrefunded ETH settlement coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adds regression coverage for rejected native-ETH payouts from a prefunded exchange and records updated gas snapshots. The failure path and rollback behavior are covered, with no remaining merge-blocking risk identified. Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@aniket866 please do review and merge the PR |
3169a54 to
77f2394
Compare
_safeTransferTokenOrETH can reach the failing ETH send by two routes: after unwrapping WETH, or directly from an existing native ETH balance with the unwrap skipped. StabilityNexus#17 covered the first, starting the exchange at zero ETH. StabilityNexus#18 reached the skip-unwrap branch but with an ETH-accepting seller, so settlement succeeded. Skipping the unwrap and failing the send was untested. Pre-fund the exchange with native ETH equal to the seller payout and match against an ETH-rejecting maker, so the failing send lands on the branch that never called withdraw. Assert EthTransferFailed, that both orders stay active and unfilled, that the WETH and token escrows are intact, that the pre-funded ETH is untouched, and that the rejecting maker received nothing. Fund the buy order with plain WETH rather than native value, so the mock holds no ETH and any stray withdraw would revert with the mock's own string instead of EthTransferFailed. That makes the skipped unwrap provable without relying on cheatcode behaviour. Regenerate .gas-snapshot for the 55-test suite, rebased onto main after StabilityNexus#22 merged. The 13 shifted entries are solc dispatch-table movement from adding a public test function; production bytecode is unchanged. Closes StabilityNexus#19 Signed-off-by: IIITManjeet <manjeetpathak2003@gmail.com>
77f2394 to
8d967b4
Compare
|
@coderabbitai review and flag issues with this PR if any |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed files =='
git diff --name-status HEAD^ HEAD
echo '== PR diff =='
git diff --unified=80 HEAD^ HEAD -- test/WindmillExchange.t.sol .gas-snapshot
echo '== Relevant Solidity outlines =='
fd -e sol . | sort | while IFS= read -r file; do
if rg -q 'safeTransferTokenOrETH|skipUnwrapWhenPrefunded|revertOnFailedEthTransferWhenPrefunded|RejectETHReceiver' "$file"; then
echo "--- $file"
ast-grep outline "$file" --items all --type function,contract,struct,error 2>/dev/null || true
fi
done
echo '== Relevant source occurrences =='
rg -n -C 12 '(_safeTransferTokenOrETH|skipUnwrapWhenPrefunded|revertOnFailedEthTransferWhenPrefunded|RejectETHReceiver|EthTransferFailed)' -g '*.sol' .Length of output: 36321 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Test fixtures, mock WETH, and rejecting receiver =='
sed -n '1,165p' test/WindmillExchange.t.sol
echo '== Test contract setup and helper functions =='
rg -n -A150 -B10 'function setUp\s*\(' test/WindmillExchange.t.sol
echo '== Exchange order creation and matching =='
sed -n '145,270p' src/core/WindmillExchange.sol
echo '== Imports and token-transfer library =='
rg -n -C 8 'library TokenTransfer|function safeTransfer|function safeTransferFrom' -g '*.sol' src testLength of output: 18697
The new test correctly covers the pre-funded native-ETH path. It verifies that ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Addressed Issues:
Fixes #19
Summary:
_safeTransferTokenOrETHcan reachEthTransferFailedtwo ways: afterWETH.withdraw(), or straight from an existing native ETH balance with the unwrap skipped. #17 covered the first (exchange starts at zero ETH). #18 reached the skip-unwrap branch but with an ETH-accepting seller, so settlement succeeded. Skipping the unwrap and failing the send was untested.test_nativeETH_revertOnFailedEthTransferWhenPrefundedpre-funds the exchange with exactly the 99.9 ETH seller payout and matches against aRejectETHReceivermaker. The payout goes straight from the pre-funded balance, the send fails, and the whole match rolls back.Contract Scope:
test/WindmillExchange.t.sol— one new test. NatSpec added to it and totest_nativeETH_skipUnwrapWhenPrefunded, resolving CodeRabbit's comment on test: cover native ETH settlement that skips WETH withdrawal #20..gas-snapshot— regenerated.No
src/orscript/changes.RejectETHReceiveris reused unchanged.Security Considerations:
None — test only, no production code touched. It pins the rollback guarantee on this route, which matters because the route deactivates both orders and completes a token transfer before the ETH send fails.
Storage/Layout Impact:
Gas and Performance:
No production gas change —
WindmillExchangeis byte-identical tomainat 10,692 B runtime / 10,974 B initcode..gas-snapshotgains 1 entry. 27 existing entries move by −67 to +162 gas: adding apublicfunction shifts solc's dispatch table for the test contract. 22 entries are untouched.Test Evidence:
forge fmt --check— clean, no diffsforge build --sizes— 10,692 / 10,974, unchanged frommainforge test— 50 passed, 0 failed, 0 skipped; new test at gas 787,995forge snapshot --check— passes, 50 entriesforge coverage— aggregate unchanged, sinceEthTransferFailedwas already reached by test: cover EthTransferFailed path in native ETH settlement #17. The LCOV counts show the new path:WETH.withdraw()executedOne more guard evaluation, zero more withdrawals, revert firing a second time — the failure now happens on the branch that never unwraps.
The
-vvvvtrace contains noMockWETH::withdrawand noMockWETH::balanceOfat all; the&&on line 102 short-circuits, so the WETH balance is never read. #17's trace on the same revert contains both.Deployment and Verification:
PRIVATE_KEY, RPC URL, explorer key)Screenshots/Recordings:
N/A — test-only change.
Additional Notes:
Rebased onto
mainafter #20 merged.The buy order is funded with plain WETH rather than native
{value:}on purpose:MockWETH.mintcreates unbacked supply, so the mock holds no ETH and any straywithdraw()on the failing leg would revert with the mock's own string instead ofEthTransferFailed— making the skipped unwrap provable without relying on cheatcode behaviour.Verified non-vacuous with three negative controls, each reverted after checking: removing the pre-fund fails with
ETH transfer failed != EthTransferFailed(); setting theexpectCallcount to 1 fails withcalled 0 times; removingvm.expectRevertfails withEthTransferFailed().Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: Claude Code (Claude Opus 5, Claude Fable 5).
AI Notice - Important!
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Bug Fixes
Tests