MVP for Windmill-EVM-Contracts Single file architecture#3
Closed
aniket866 wants to merge 3 commits into
Closed
Conversation
Contributor
WalkthroughRemoves the Counter contract, its deployment script, and tests; adds a new WindmillExchange Solidity contract with order lifecycle, dynamic time-based price curves, matching/pruning logic, safe token transfers, a Foundry deploy script, and comprehensive Forge tests. Changes
Sequence DiagramsequenceDiagram
participant MakerA as Maker (Buy)
participant MakerB as Maker (Sell)
participant WE as WindmillExchange
participant TokenA as TokenA
participant TokenB as TokenB
participant Keeper as Keeper
MakerA->>WE: createOrder(tokenA→tokenB, buy, amountIn, startPrice,...)
WE->>TokenA: transferFrom(MakerA, WE, amountIn)
TokenA-->>WE: transfer success
WE-->>MakerA: orderId
MakerB->>WE: createOrder(tokenB→tokenA, sell, amountIn, startPrice,...)
WE->>TokenB: transferFrom(MakerB, WE, amountIn)
TokenB-->>WE: transfer success
WE-->>MakerB: orderId
Keeper->>WE: matchOrders(buyId, sellId, deadline)
WE->>WE: validate, compute settlement price, compute fills
WE->>TokenA: safeTransfer(MakerA, outAmountA)
TokenA-->>MakerA: transfer success
WE->>TokenB: safeTransfer(MakerB, outAmountB)
TokenB-->>MakerB: transfer success
WE-->>Keeper: emit OrderMatched (settlementPrice, filledAmount)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
changed the title
aniket866
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/WindmillExchange.sol`:
- Around line 291-303: Partial fills currently only update remainingIn on the
buy/sell orders (in the branch where buyFilled/sellFilled is false) but leave
the original createdAt so the residual keeps the original price anchor; modify
the non-filled branches for buyOrderId and sellOrderId to re-anchor the residual
by resetting _orders[buyOrderId].createdAt = block.timestamp (and similarly for
_orders[sellOrderId].createdAt) whenever you set remainingIn to
newBuyRemaining/newSellRemaining so the leftover order reprices from the current
time; keep the existing _pairRemove and active=false logic for fully filled
orders.
- Around line 141-156: safeTransfer and safeTransferFrom currently only assert
the ERC-20 call returned true and therefore allow fee-on-transfer/deflationary
tokens to under- or over-transfer; update both helpers to enforce exact balance
deltas or reject such tokens: for safeTransferFrom(address token, address from,
address to, uint256 amount) read preBalance =
IERC20(token).balanceOf(address(this)) (or balanceOf(to) if pulling directly to
recipient), perform the call, then read postBalance and require(postBalance -
preBalance == amount) else revert TransferFailed(); for safeTransfer(address
token, address to, uint256 amount) read preBalanceTo =
IERC20(token).balanceOf(to), perform the call, then read postBalanceTo and
require(postBalanceTo - preBalanceTo == amount) else revert TransferFailed();
keep the existing checks for token.code.length and call success but add these
balance checks so createOrder/cancelOrder/settlement cannot be cheated by
transfer-tax tokens or explicitly reject tokens that fail the exact-delta
requirement.
- Around line 268-270: The current matching path rejects expired orders but
never deactivates them or removes their ids from _pairOrders, causing stale
entries; update the execution logic (where buy/sell are validated and
OrderExpired is thrown) to mark the expired order(s) inactive and remove their
id(s) from _pairOrders before reverting or continue, and emit an event (e.g.,
OrderPruned) for state transparency; additionally add a permissionless prune
function (e.g., pruneExpiredOrders(pairId, limit)) that scans _pairOrders,
checks buy.expiry/sell.expiry against block.timestamp, sets active=false,
removes ids from _pairOrders and updates any indices/counts to prevent
accumulation of abandoned orders so paginated reads no longer encounter dead
entries.
In `@test/WindmillExchange.t.sol`:
- Around line 203-267: The tests assert only that balances changed, so expand
test_matchOrders_fullFill and test_matchOrders_partialFill to assert exact
post-trade balances and order fields: check exchange.getOrder(...).remainingIn
equals 0 for full fill and equals expected remaining for partial fill, verify
both orders' active flags, assert exact tokenA/tokenB balances for maker/taker
and the exact fee split recipient/amount (keeper/feeRecipient) and paymentOwed
updates after calling exchange.matchOrders; add a new test that exercises the
public deadline expiry path by creating orders, advancing time past the order
deadline with vm.warp and calling exchange.matchOrders expecting
OrderExpired.selector to be reverted. Ensure you reference the same identifiers
used in the diff (test_matchOrders_fullFill, test_matchOrders_partialFill,
exchange.matchOrders, exchange.getOrder, remainingIn, OrderExpired) when adding
the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8df01de-68d3-4f66-847e-16f48d6607cb
📒 Files selected for processing (6)
script/Counter.s.solscript/DeployWindmill.s.solsrc/Counter.solsrc/WindmillExchange.soltest/Counter.t.soltest/WindmillExchange.t.sol
💤 Files with no reviewable changes (3)
- src/Counter.sol
- test/Counter.t.sol
- script/Counter.s.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/WindmillExchange.t.sol (1)
201-327:⚠️ Potential issue | 🟠 MajorAdd explicit deadline-expiry coverage for
matchOrders.The suite does not directly test the
deadline expiredguard, so one public revert path is still uncovered.🧪 Suggested test case
+function test_matchOrders_revert_deadlineExpired() public { + uint256 buyId = _buy(alice, 100 ether, RAY, 0, 0); + uint256 sellId = _sell(bob, 100 ether, RAY, 0, 0); + vm.warp(block.timestamp + 2); + vm.expectRevert(bytes("deadline expired")); + exchange.matchOrders(buyId, sellId, block.timestamp - 1); +}As per coding guidelines, "Ensure failure paths and revert scenarios are explicitly handled and validated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/WindmillExchange.t.sol` around lines 201 - 327, Add a unit test that explicitly covers the function-level deadline check in matchOrders: create a valid buy and sell (use helper _buy/_sell or createOrder), then call exchange.matchOrders(buyId, sellId, block.timestamp - 1) and vm.expectRevert(DeadlineExpired.selector) before the call to assert the deadline guard reverts; name it e.g., test_matchOrders_revert_deadlineExpired and use matchOrders, _buy/_sell, and DeadlineExpired.selector to locate the relevant checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/WindmillExchange.sol`:
- Around line 363-384: pruneExpiredOrders currently deactivates and removes
orders but never returns o.remainingIn to o.maker, locking escrow; update
pruneExpiredOrders to follow checks-effects-interactions: record local values
(id, o.maker, o.remainingIn, token address field), set o.active = false and
o.remainingIn = 0 and update pair index/list state before any external call,
then perform the external token transfer/refund of the saved remaining amount to
o.maker (use the same safe transfer routine used by cancelOrder), emit
OrderPruned, and add a reentrancy guard (or apply the contract's existing
nonReentrant modifier) to pruneExpiredOrders to prevent reentrancy during the
refund flow.
In `@test/WindmillExchange.t.sol`:
- Around line 340-356: The test_pruneExpiredOrders currently validates order
deactivation and index cleanup but omits asserting that the maker's escrowed
funds are returned; after calling exchange.pruneExpiredOrders(...) and
before/after checking active flags, capture the maker's tokenA (or relevant
ERC20) balance delta (using tokenA.balanceOf(alice) or the appropriate
escrow/balance-view helper) and assert it increased by the original escrowed
amount from the _buy call for id1 (100 ether in this case); reference the
existing symbols _buy, exchange.pruneExpiredOrders, exchange.getOrder, tokenA,
and alice to locate where to add the balance-before/balance-after assertions so
the test ensures escrow refunds on prune.
---
Duplicate comments:
In `@test/WindmillExchange.t.sol`:
- Around line 201-327: Add a unit test that explicitly covers the function-level
deadline check in matchOrders: create a valid buy and sell (use helper
_buy/_sell or createOrder), then call exchange.matchOrders(buyId, sellId,
block.timestamp - 1) and vm.expectRevert(DeadlineExpired.selector) before the
call to assert the deadline guard reverts; name it e.g.,
test_matchOrders_revert_deadlineExpired and use matchOrders, _buy/_sell, and
DeadlineExpired.selector to locate the relevant checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd419579-64a8-41af-98af-a54d865793e5
📒 Files selected for processing (2)
src/WindmillExchange.soltest/WindmillExchange.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/WindmillExchange.sol`:
- Around line 281-285: When marking an order fully filled in the settlement
logic, also clear its remainingIn to zero to enable the storage refund: after
setting _orders[buyOrderId].active = false (and before/after calling
_pairRemove) assign _orders[buyOrderId].remainingIn = 0; do the same for
_orders[sellOrderId].remainingIn = 0 when sellFilled is true; reference the
_orders mapping, buyOrderId/sellOrderId, remainingIn, active, and the
_pairRemove(...) call to locate and update the two places in the code.
In `@test/WindmillExchange.t.sol`:
- Around line 249-295: Add a test that covers the contract-level deadline revert
by creating a buy and sell order (use _buy and _sell as in other tests), then
call exchange.matchOrders with a deadline strictly less than block.timestamp and
assert the revert message "deadline expired" via vm.expectRevert("deadline
expired"); name the test test_matchOrders_revert_deadlineExpired and mirror the
setup style used in existing tests (e.g., create buyId/sellId then expectRevert
before calling exchange.matchOrders).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c0bcb8b-e553-4a01-b66d-581d3fd5ede3
📒 Files selected for processing (2)
src/WindmillExchange.soltest/WindmillExchange.t.sol
Contributor
Author
|
Closing |
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
1 participant
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.
Addressed Issues:
Closes #1
Summary:
Introduces a clean 3-file MVP implementation of WindmillExchange on top of the base template.
Files Introduced
Contract Scope:
N/A
Security Considerations:
Storage/Layout Impact:
No storage layout changes this is a fresh deployment with no prior on-chain version, so there is nothing to be compatible with.
Gas and Performance:
this is a fresh deployment with no prior on-chain version, so there is nothing to be compatible with.
Test Evidence:
forge fmt --check:forge build --sizes:forge test -vvv:forge coverage:forge snapshot:Deployment and Verification:
PRIVATE_KEY, RPC URL, explorer key)Screenshots/Recordings:
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
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