MVP to the WindMill-Exchange Based on Modular structure using components and libs , storage, Math libraries#2
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves example Counter contract, script, and tests; adds WindmillExchange MVP with order lifecycle, storage and pair indexing, price-curve, math and token transfer libraries, ERC20 and exchange interfaces, deployment script, and comprehensive Forge tests for matching and edge cases. Changes
Sequence DiagramsequenceDiagram
actor Maker as Maker
participant WX as WindmillExchange
participant OS as OrderStorage
participant PS as PairStorage
participant TT as TokenTransfer
participant PC as PriceCurve
participant Token as ERC20
rect rgba(100,150,200,0.5)
note over Maker,WX: Order Creation
Maker->>WX: createOrder(tokenIn, tokenOut, amountIn, ...)
WX->>WX: validate inputs & expiry
WX->>OS: _storeOrder(order)
OS-->>WX: orderId
WX->>PS: _addOrderToPair(tokenIn, tokenOut, orderId)
WX->>TT: safeTransferFrom(tokenIn, maker, WX, amountIn)
TT->>Token: transferFrom(maker, WX, amountIn)
Token-->>TT: success
WX-->>Maker: returns orderId + emit OrderCreated
end
rect rgba(150,200,150,0.5)
note over Maker,WX: Order Matching
Maker->>WX: matchOrders(buyOrderId, sellOrderId, deadline)
WX->>OS: _getOrder(buyOrderId), _getOrder(sellOrderId)
OS-->>WX: buyOrder, sellOrder
WX->>PC: currentPrice(buyOrder, ts)
PC-->>WX: buyPrice
WX->>PC: currentPrice(sellOrder, ts)
PC-->>WX: sellPrice
WX->>PC: hasCrossed(buy, sell, ts)
PC-->>WX: crossed?
WX->>WX: _computeSettlement(...)
WX->>OS: _updateRemainingIn(...)
WX->>TT: safeTransfer(tokenOut, buyer, amountOut)
WX->>TT: safeTransfer(tokenIn, seller, amountIn)
WX-->>Maker: emit OrderMatched / OrderFilled / OrderPartiallyFilled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
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.
Actionable comments posted: 8
🤖 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/core/WindmillExchange.sol`:
- Around line 239-243: Compute paymentOwed from the asset amount only after
ensuring the asset can be fully paid at the rounded price: keep the existing
maxAssetFromBuy and initial filledAsset calculation (using MathUtils.mulDiv,
sell.remainingIn, buy.remainingIn, settlementPx), then compute paymentOwed =
MathUtils.mulDiv(filledAsset, MathUtils.RAY, settlementPx) and immediately
recompute filledAsset = MathUtils.mulDiv(paymentOwed, settlementPx,
MathUtils.RAY) so filledAsset is the asset amount implied by the floored
payment; if this reduces filledAsset to zero or otherwise makes the fill invalid
handle it (revert or skip) instead of sending more tokenIn than paymentOwed
supports. Ensure you reference filledAsset, paymentOwed, MathUtils.mulDiv,
settlementPx, buy.remainingIn and sell.remainingIn when making the change.
- Around line 64-109: The createOrder function registers the order (via
_storeOrder and _addOrderToPair) before performing the external
TokenTransfer.safeTransferFrom, allowing a reentrant token callback to call
matchOrders against an order without funds; fix by preventing reentrancy: either
add the nonReentrant modifier to createOrder (preferred) or move
TokenTransfer.safeTransferFrom before calling _storeOrder/_addOrderToPair so
external calls cannot reenter into matchOrders using the un-funded order;
reference functions: createOrder, _storeOrder, _addOrderToPair,
TokenTransfer.safeTransferFrom, and matchOrders.
In `@src/libraries/MathUtils.sol`:
- Around line 76-79: The clamp function can return a value even when lo > hi;
add an explicit validation at the start of MathUtils.clamp to revert when lo >
hi (e.g., require or revert with a clear error) so callers like
PriceCurve._compute never receive a nonsensical value, and keep the existing
behavior when lo <= hi so the later checks in clamp (value < lo / value > hi)
remain unchanged.
In `@src/libraries/PriceCurve.sol`:
- Around line 49-50: The negation of slope can overflow when slope ==
type(int256).min; update MathUtils.abs(int256) to explicitly check for slope ==
type(int256).min and revert with SlopeOverflow() (or return a safe handling)
instead of performing uint256(-x), and mirror that same check in
PriceCurve._compute() (replace uint256(-slope) usage with a branch that checks
for type(int256).min and reverts SlopeOverflow() or handles it safely before
converting to uint256). Ensure both the validation path (where MathUtils.abs is
used) and price computation path explicitly handle this edge case, and add
regression tests that call MathUtils.abs(type(int256).min) and
PriceCurve._compute() (or the public compute wrapper) with slope ==
type(int256).min to assert the SlopeOverflow() behavior.
In `@src/libraries/TokenTransfer.sol`:
- Around line 9-16: The helper currently calls token.call which returns
(true,"") for EOAs/no-code addresses, so add a pre-call code-existence check: in
safeTransfer (and the analogous safeTransferFrom) verify token.code.length > 0
(or address(token).code.length > 0) and revert (e.g., TransferFailed() or a new
descriptive error) if code length is zero before performing the low-level call;
then keep the existing ok/data decoding checks. Also add a regression test that
passes a no-code address as token to both safeTransfer and safeTransferFrom to
assert they revert rather than succeed silently.
In `@src/storage/PairStorage.sol`:
- Around line 7-8: The _pairIndex mapping is currently global and must be
namespaced by pair to avoid cross-pair collisions; change the declaration from
mapping(uint256 => uint256) _pairIndex to mapping(bytes32 => mapping(uint256 =>
uint256)) _pairIndex (keyed by the same pair key used for _pairOrders) and
update every place that reads/writes _pairIndex (e.g., in the functions that
push, remove, swap and update order indexes) to pass the pair key first (e.g.,
_pairIndex[pair][orderId]) so insertions, deletions and index updates operate on
the correct pair bucket and the pop/update logic uses the per-pair index rather
than a global one.
In `@test/WindmillExchange.t.sol`:
- Around line 265-345: Update the three tests
(test_matchOrders_success_fullFill, test_matchOrders_partialFill_buySmaller,
test_matchOrders_partialFill_sellSmaller) to assert exact post-trade balances
instead of only asserting positive deltas: compute expected token transfers
using the RAY-scaled price and the filled amounts (use the same math as
WindmillExchange.matchOrders: paymentOwed = filledIn * price / RAY and
filledAsset = filledOut) and replace assertGt checks with assertEq against those
computed expected balances; additionally add a 1-wei seller-limited match case
(create a scenario where seller remainingIn is limited by 1 wei) and assert
exact balances there to catch rounding edge-cases. Ensure you reference the
order IDs (buyId, sellId) and read Order.remainingIn from exchange.getOrder to
compute the actual filled amounts when asserting expected balances.
- Around line 179-220: Add a new unit test named
test_createOrder_revert_invalidPriceBounds that vm.prank(alice),
vm.expectRevert(InvalidPriceBounds.selector) and then calls exchange.createOrder
with tokenA/tokenB and parameters that violate the price-bounds check (e.g.,
provide startPrice and endPrice in the wrong order or out-of-range so the
contract's InvalidPriceBounds branch is hit); mirror the style of the existing
tests (use alice, tokenA/tokenB, amount like 100 ether, RAY constant, and other
zeros) so the new test validates the createOrder InvalidPriceBounds revert path.
🪄 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: 25722536-e6ac-469c-a567-e52eea99c840
📒 Files selected for processing (14)
script/Counter.s.solscript/DeployWindmill.s.solsrc/Counter.solsrc/core/WindmillExchange.solsrc/interfaces/IERC20.solsrc/interfaces/IWindmillExchange.solsrc/libraries/MathUtils.solsrc/libraries/PriceCurve.solsrc/libraries/TokenTransfer.solsrc/storage/OrderStorage.solsrc/storage/PairStorage.solsrc/types/OrderTypes.soltest/Counter.t.soltest/WindmillExchange.t.sol
💤 Files with no reviewable changes (3)
- test/Counter.t.sol
- script/Counter.s.sol
- src/Counter.sol
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/WindmillExchange.t.sol`:
- Around line 191-195: The test asserts ZeroAddress.selector for same-token
orders but the PR requires replacing that with a new SameToken error; add a
custom error SameToken() to WindmillExchange and update the createOrder logic
(the branch that currently reverts when tokenA == tokenB) to revert with
SameToken instead of ZeroAddress, and then update the test
test_createOrder_revert_sameToken to expect SameToken.selector; locate checks in
createOrder and the test function by the identifiers createOrder and
test_createOrder_revert_sameToken.
- Around line 437-483: Add a test that verifies currentPrice increases over time
with a positive slope: create a buy order via _createBuyOrder (like in
test_currentPrice_descendingSlope_decreasesOverTime) with a positive int256
slope (e.g., slope = int256(RAY / 1000)), capture t0 = block.timestamp, call
exchange.currentPrice(id, t0) and exchange.currentPrice(id, t0 + delta), assert
the initial price equals startPrice and that priceLater > priceNow (using
assertGt) to confirm ascending behavior; keep the test name consistent (e.g.,
test_currentPrice_ascendingSlope_increasesOverTime) and mirror the structure of
the descending slope test.
- Around line 358-431: Add a test that asserts matchOrders reverts with
OrderInactive.selector when either order is inactive: create a buy and sell
order via _createBuyOrder/_createSellOrder, then in one variant cancel the buy
via exchange.cancelOrder (or fully fill it via matchOrders with another
counterparty) and in another variant cancel the sell, and call
exchange.matchOrders(buyId, sellId) expecting
vm.expectRevert(OrderInactive.selector); ensure you reference matchOrders,
OrderInactive.selector, _createBuyOrder/_createSellOrder and cancelOrder (or the
helper used to fully fill) so the test targets the inactive-order path.
🪄 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: de4fce50-9247-4610-a870-2492980f6cc7
📒 Files selected for processing (1)
test/WindmillExchange.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
test/WindmillExchange.t.sol (2)
207-211:⚠️ Potential issue | 🟠 MajorSame-token revert expectation is still targeting the wrong error.
Line 209 expects
ZeroAddress.selector; this should validate the explicit same-token error path (SameToken) defined by the MVP objective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/WindmillExchange.t.sol` around lines 207 - 211, The test test_createOrder_revert_sameToken is asserting the wrong revert: replace the current vm.expectRevert(ZeroAddress.selector) with vm.expectRevert(SameToken.selector) so the call to exchange.createOrder(address(tokenA), address(tokenA), ...) asserts the explicit same-token failure path; ensure the SameToken selector symbol is imported/visible to the test so the expectation references the contract error type used by createOrder.
307-310:⚠️ Potential issue | 🟠 MajorSettlement tests are still too weak to catch rounding/payment bugs.
These checks only assert positive deltas, not exact settlement amounts. Please assert exact post-trade balances (including a 1-wei edge case) so price/rounding regressions fail deterministically.
As per coding guidelines, "Ensure security-sensitive logic changes are not introduced without adequate test coverage."Also applies to: 343-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/WindmillExchange.t.sol` around lines 307 - 310, Replace the loose assertGt checks on token balances (tokenB.balanceOf(alice), tokenA.balanceOf(bob) vs aliceTokenBBefore, bobTokenABefore) with exact equality assertions that compute and compare against the expected post-trade balances (e.g., aliceExpected = aliceTokenBBefore + expectedFilledAmount, bobExpected = bobTokenABefore + expectedPaymentOwed), using the same calculation logic used by the matcher/settlement code (price, amounts, rounding mode) so rounding regressions fail deterministically; also add an additional test case exercising a 1-wei remainder (edge case) and assert the exact expected balances for that case too, referencing the same variables and functions (tokenA.balanceOf, tokenB.balanceOf, expectedFilled/paymentOwed calculation) so the assertions fail if rounding/payment changes.src/storage/PairStorage.sol (1)
7-8:⚠️ Potential issue | 🔴 CriticalCompilation blocker: duplicate
_pairIndexdeclaration and missing_pairOrdersstate variable.Line 8 redeclares
_pairIndexwith an incompatible type, and_pairOrdersis referenced at lines 13, 14, 22, and 40 but is never declared. The file cannot compile in its current state.Recommended fix
abstract contract PairStorage { mapping(bytes32 => mapping(uint256 => uint256)) private _pairIndex; - mapping(uint256 => uint256) private _pairIndex; + mapping(bytes32 => uint256[]) private _pairOrders;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/PairStorage.sol` around lines 7 - 8, Remove the duplicate mapping declaration "mapping(uint256 => uint256) private _pairIndex" and instead declare the missing _pairOrders state variable with the correct type and visibility used throughout the contract (e.g., "mapping(bytes32 => uint256[]) private _pairOrders" if the code treats _pairOrders as an array of order IDs keyed by pairHash); keep the original mapping(bytes32 => mapping(uint256 => uint256)) private _pairIndex as-is, and ensure all references to _pairOrders match that newly added mapping type and visibility.
🤖 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/core/WindmillExchange.sol`:
- Around line 63-73: The createOrder function currently accepts and stores
caller-supplied startPrice directly; add an on-chain oracle resolution path
instead: add an optional oracle identifier parameter (e.g., oracleId or
priceFeed) to createOrder, branch on its presence to fetch the current price via
the contract's oracle interface (e.g., call getPrice/peek on the
PriceFeed/Oracle contract), validate the fetched price (non-zero, within
bounds), and persist the resolved startPrice and the oracle identifier in the
order record (instead of trusting the caller value). Also keep a clear
fallback/require path if no oracle is provided (or make oracle mandatory), and
ensure any oracle calls are gas-safe and use the same symbols present in this
contract (createOrder, startPrice, orders mapping/Order struct, and oracle
interface) so storage and event emission remain consistent.
- Around line 26-35: Add an admin/pauser governance surface to WindmillExchange:
introduce an owner/admin state (or inherit OpenZeppelin Ownable) and a paused
boolean plus modifiers like onlyOwner (or onlyAdmin) and whenNotPaused to guard
state-changing functions (e.g., deposit/withdraw/trade functions and any
functions in PairStorage/OrderStorage that modify state), implement pause() and
unpause() setter functions callable only by the owner/admin, and ensure
nonReentrant remains intact by keeping the existing _reentrancyStatus and
nonReentrant modifier; update function signatures to include whenNotPaused where
appropriate so the exchange can be frozen and admin settings adjusted securely.
- Around line 12-17: Declare and use a dedicated SameToken() error instead of
reusing ZeroAddress() for same-asset orders: add error SameToken(); to the
top-level errors list and update the order validation logic (the place that
currently checks makerToken == takerToken and reverts with ZeroAddress()) to
instead revert with SameToken() when makerToken == takerToken; ensure any tests
or callers expecting ZeroAddress() for this condition are updated to expect
SameToken().
- Around line 188-195: The current getOrdersByPair(tokenA, tokenB) materializes
the entire order id array and must be converted to a paginated API to avoid
unbounded gas/copy; change the external signature to accept pagination params
(e.g. getOrdersByPair(address tokenA, address tokenB, uint256 cursor, uint256
limit) external view returns (uint256[] memory)), update the override and any
interface accordingly, and implement a paginated iterator in _getOrdersByPair
(or create _getOrdersByPairPaginated) that precomputes a bounded size =
min(limit, remaining) and loops from cursor to cursor+size (no unbounded loops),
copies only that window into the result array, and enforce a sane max limit
guard to prevent abuse. Ensure callers are updated to use the new paginated
form.
In `@src/interfaces/IWindmillExchange.sol`:
- Line 21: The IWindmillExchange.matchOrders API lacks the promised deadline
parameter; update the interface function signature for matchOrders in
IWindmillExchange to accept a uint256 deadline (e.g., change matchOrders(uint256
buyOrderId, uint256 sellOrderId) to include a deadline parameter), keep external
visibility, and then propagate this signature change to all implementations and
callers (contracts and tests) so they pass and enforce the per-call deadline
when executing matchOrders.
---
Duplicate comments:
In `@src/storage/PairStorage.sol`:
- Around line 7-8: Remove the duplicate mapping declaration "mapping(uint256 =>
uint256) private _pairIndex" and instead declare the missing _pairOrders state
variable with the correct type and visibility used throughout the contract
(e.g., "mapping(bytes32 => uint256[]) private _pairOrders" if the code treats
_pairOrders as an array of order IDs keyed by pairHash); keep the original
mapping(bytes32 => mapping(uint256 => uint256)) private _pairIndex as-is, and
ensure all references to _pairOrders match that newly added mapping type and
visibility.
In `@test/WindmillExchange.t.sol`:
- Around line 207-211: The test test_createOrder_revert_sameToken is asserting
the wrong revert: replace the current vm.expectRevert(ZeroAddress.selector) with
vm.expectRevert(SameToken.selector) so the call to
exchange.createOrder(address(tokenA), address(tokenA), ...) asserts the explicit
same-token failure path; ensure the SameToken selector symbol is
imported/visible to the test so the expectation references the contract error
type used by createOrder.
- Around line 307-310: Replace the loose assertGt checks on token balances
(tokenB.balanceOf(alice), tokenA.balanceOf(bob) vs aliceTokenBBefore,
bobTokenABefore) with exact equality assertions that compute and compare against
the expected post-trade balances (e.g., aliceExpected = aliceTokenBBefore +
expectedFilledAmount, bobExpected = bobTokenABefore + expectedPaymentOwed),
using the same calculation logic used by the matcher/settlement code (price,
amounts, rounding mode) so rounding regressions fail deterministically; also add
an additional test case exercising a 1-wei remainder (edge case) and assert the
exact expected balances for that case too, referencing the same variables and
functions (tokenA.balanceOf, tokenB.balanceOf, expectedFilled/paymentOwed
calculation) so the assertions fail if rounding/payment changes.
🪄 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: 93b363b9-960b-4561-8351-d4dc8915f84d
📒 Files selected for processing (10)
script/DeployWindmill.s.solsrc/core/WindmillExchange.solsrc/interfaces/IWindmillExchange.solsrc/libraries/MathUtils.solsrc/libraries/PriceCurve.solsrc/libraries/TokenTransfer.solsrc/storage/OrderStorage.solsrc/storage/PairStorage.solsrc/types/OrderTypes.soltest/WindmillExchange.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
test/WindmillExchange.t.sol (3)
497-509: 🧹 Nitpick | 🔵 TrivialConsider adding ascending slope test for completeness.
The
currentPricetests validate flat and descending slope behavior. An ascending slope test would complete the price curve coverage.🧪 Optional: ascending slope test
+ function test_currentPrice_ascendingSlope_increasesOverTime() public { + uint256 startPrice = RAY; + int256 slope = int256(RAY / 1000); // price increases + + uint256 id = _createBuyOrder(alice, 100 ether, startPrice, slope, 0); + uint256 t0 = block.timestamp; + + uint256 priceNow = exchange.currentPrice(id, t0); + uint256 priceLater = exchange.currentPrice(id, t0 + 1000); + + assertEq(priceNow, startPrice); + assertGt(priceLater, priceNow, "price should increase over time with positive slope"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/WindmillExchange.t.sol` around lines 497 - 509, Add a complementary test that verifies a positive (ascending) slope increases price over time: create a new test function (e.g., test_currentPrice_ascendingSlope_increasesOverTime) that mirrors test_currentPrice_descendingSlope_decreasesOverTime but sets slope to a positive value (e.g., int256(RAY / 1000)), calls _createBuyOrder(alice, ... , startPrice, slope, 0), uses exchange.currentPrice(id, t0) and exchange.currentPrice(id, t0 + delta) and assert that priceLater > priceNow (and assertEq for initial price if desired) to cover the ascending slope case.
410-486: 🧹 Nitpick | 🔵 TrivialConsider adding
test_matchOrders_revert_orderInactivetest.The match revert tests cover expired, self-match, no-cross, and pair-mismatch scenarios, but there's no test verifying that
matchOrdersreverts withOrderInactivewhen attempting to match a cancelled order. This was noted in a previous review as addressed, but the test is not present in the current file.🧪 Suggested test case
+ function test_matchOrders_revert_orderInactive() public { + uint256 buyId = _createBuyOrder(alice, 100 ether, RAY, 0, 0); + uint256 sellId = _createSellOrder(bob, 100 ether, RAY, 0, 0); + + // Cancel the buy order to make it inactive + vm.prank(alice); + exchange.cancelOrder(buyId); + + vm.expectRevert(OrderInactive.selector); + 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 410 - 486, Add a new unit test named test_matchOrders_revert_orderInactive that creates a buy and a sell order using the existing helpers (_createBuyOrder/_createSellOrder), cancels one of them via exchange.cancelOrder (using vm.prank as the order owner), then calls exchange.matchOrders(...) and asserts it reverts with OrderInactive.selector; reference _createBuyOrder, _createSellOrder, exchange.cancelOrder, OrderInactive, and matchOrders so the test mirrors the other revert tests (use vm.expectRevert(OrderInactive.selector) and pass block.timestamp + 1 to matchOrders).
326-353:⚠️ Potential issue | 🟡 MinorSettlement assertions should verify exact amounts, not just positive deltas.
Lines 344-346 use
assertGtto verify balance increases, but this doesn't catch settlement math bugs. A rounding error in the exchange could still pass these assertions while paying incorrect amounts.💡 Suggested exact assertion approach
function test_matchOrders_success_fullFill() public { uint256 price = RAY; // 1 tokenA per tokenB uint256 amount = 100 ether; uint256 buyId = _createBuyOrder(alice, amount, price, 0, 0); uint256 sellId = _createSellOrder(bob, amount, price, 0, 0); uint256 aliceTokenBBefore = tokenB.balanceOf(alice); uint256 bobTokenABefore = tokenA.balanceOf(bob); exchange.matchOrders(buyId, sellId, block.timestamp + 1); // Both orders should be deactivated (fully filled) assertFalse(exchange.getOrder(buyId).active, "buy should be inactive"); assertFalse(exchange.getOrder(sellId).active, "sell should be inactive"); - // Alice should have received tokenB (filledAsset from sell order) - assertGt(tokenB.balanceOf(alice), aliceTokenBBefore, "alice should receive tokenB"); - // Bob should have received tokenA (paymentOwed from buy order) - assertGt(tokenA.balanceOf(bob), bobTokenABefore, "bob should receive tokenA"); + // At price = 1 RAY, filledAsset = amount, paymentOwed = amount + // Keeper fee = paymentOwed / 1000 = amount / 1000 + uint256 keeperFee = amount / 1000; + + // Alice receives full filledAsset (sell.tokenIn = tokenB) + assertEq(tokenB.balanceOf(alice), aliceTokenBBefore + amount, "alice should receive exact tokenB"); + // Bob receives paymentOwed minus keeper fee + assertEq(tokenA.balanceOf(bob), bobTokenABefore + amount - keeperFee, "bob should receive exact tokenA");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/WindmillExchange.t.sol` around lines 326 - 353, The test_matchOrders_success_fullFill uses assertGt to only check positive balance deltas, which can mask settlement math/rounding bugs; replace those loose checks by computing the exact expected token transfers from the created orders (use the price, amount, and the created buyId/sellId via _createBuyOrder/_createSellOrder and the exchange.matchOrders call) and assert equality: verify tokenB.balanceOf(alice) - aliceTokenBBefore == expectedFilledAssetFromSellOrder and tokenA.balanceOf(bob) - bobTokenABefore == expectedPaymentOwedFromBuyOrder (use exchange.getOrder or your internal calc to derive expected amounts) and keep the other assertions (orders inactive, getOrdersByPair empty) intact.
🤖 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/core/WindmillExchange.sol`:
- Around line 4-6: Extract the inline interface IPriceOracle into its own
Solidity interface file (create IPriceOracle.sol containing interface
IPriceOracle { function getPrice() external view returns (uint256); }) and
remove the inline declaration from WindmillExchange.sol; then add an import of
the new interface into WindmillExchange.sol and ensure any references to
IPriceOracle (e.g., in functions or state variables) compile against the
imported interface name.
- Around line 30-53: The contract sets owner in the constructor but provides no
way to change it; add an ownership transfer mechanism by implementing a
transferOwnership(address newOwner) function guarded by onlyOwner that requires
newOwner != address(0), updates the owner state variable, and emits an
OwnershipTransferred(address indexed previousOwner, address indexed newOwner)
event; also consider adding renounceOwnership() (onlyOwner => owner = address(0)
+ emit event) and update any access checks that rely on owner accordingly.
- Around line 224-242: getOrdersByPair currently pulls the entire array into
memory via _getOrdersByPair and will underflow if cursor > all.length; change to
read the orders directly from storage by adding an internal view that returns
uint256[] storage (e.g. _getOrdersByPairStorage using
_pairOrders[pairKeyOf(tokenA, tokenB)]), perform a bounds check that reverts
with a clear message if cursor >= storageArray.length, compute remaining/size
from storageArray.length, then allocate the result array and copy only the
requested window (storageArray[cursor + i]) into memory to avoid loading the
full list.
- Around line 101-103: The call to IPriceOracle(priceFeed).getPrice() in
WindmillExchange (setting startPrice when priceFeed != address(0)) needs
defensive handling: wrap the external call in a try/catch around
IPriceOracle(priceFeed).getPrice() to avoid reverts bubbling up, validate the
returned price (startPrice) against reasonable bounds (e.g., non-zero, within
configured min/max or a change threshold) and, if the oracle exposes timestamps,
perform a staleness check against block.timestamp; on failure or invalid values,
fall back to a safe behavior (revert with clear error, use a cached price, or
skip oracle-dependent logic) and emit/log an event so callers can detect oracle
issues. Ensure references to priceFeed, startPrice, and IPriceOracle.getPrice
are the focal points of the patch.
---
Duplicate comments:
In `@test/WindmillExchange.t.sol`:
- Around line 497-509: Add a complementary test that verifies a positive
(ascending) slope increases price over time: create a new test function (e.g.,
test_currentPrice_ascendingSlope_increasesOverTime) that mirrors
test_currentPrice_descendingSlope_decreasesOverTime but sets slope to a positive
value (e.g., int256(RAY / 1000)), calls _createBuyOrder(alice, ... , startPrice,
slope, 0), uses exchange.currentPrice(id, t0) and exchange.currentPrice(id, t0 +
delta) and assert that priceLater > priceNow (and assertEq for initial price if
desired) to cover the ascending slope case.
- Around line 410-486: Add a new unit test named
test_matchOrders_revert_orderInactive that creates a buy and a sell order using
the existing helpers (_createBuyOrder/_createSellOrder), cancels one of them via
exchange.cancelOrder (using vm.prank as the order owner), then calls
exchange.matchOrders(...) and asserts it reverts with OrderInactive.selector;
reference _createBuyOrder, _createSellOrder, exchange.cancelOrder,
OrderInactive, and matchOrders so the test mirrors the other revert tests (use
vm.expectRevert(OrderInactive.selector) and pass block.timestamp + 1 to
matchOrders).
- Around line 326-353: The test_matchOrders_success_fullFill uses assertGt to
only check positive balance deltas, which can mask settlement math/rounding
bugs; replace those loose checks by computing the exact expected token transfers
from the created orders (use the price, amount, and the created buyId/sellId via
_createBuyOrder/_createSellOrder and the exchange.matchOrders call) and assert
equality: verify tokenB.balanceOf(alice) - aliceTokenBBefore ==
expectedFilledAssetFromSellOrder and tokenA.balanceOf(bob) - bobTokenABefore ==
expectedPaymentOwedFromBuyOrder (use exchange.getOrder or your internal calc to
derive expected amounts) and keep the other assertions (orders inactive,
getOrdersByPair empty) intact.
🪄 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: ca2f2696-2ee8-41b7-b6c7-01b9f762bf83
📒 Files selected for processing (4)
src/core/WindmillExchange.solsrc/interfaces/IWindmillExchange.solsrc/storage/PairStorage.soltest/WindmillExchange.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/WindmillExchange.sol`:
- Around line 55-61: The pause/unpause functions change protocol state but don't
emit events; declare and use events (e.g., event Paused(address account); event
Unpaused(address account);) and emit them inside pause() and unpause()
respectively (emit Paused(msg.sender); and emit Unpaused(msg.sender);) so
callers monitoring the WindmillExchange contract (functions pause and unpause)
can observe state transitions and the actor who triggered them.
- Around line 224-243: The getOrdersByPair function currently accepts an
unbounded limit which can allow very large loops; introduce a hard cap constant
(e.g., uint256 constant MAX_PAGE_SIZE) and enforce size = min(remaining,
min(limit, MAX_PAGE_SIZE)) before allocating result to guarantee bounded gas
usage; update getOrdersByPair to apply this cap and validate limit (treat zero
as zero-page or revert per project convention), and mention/_use the existing
_getOrdersByPairStorage(tokenA, tokenB) storage lookup in your change so callers
still receive the same paginated items but never more than MAX_PAGE_SIZE per
call.
- Around line 110-133: The order is stored using the requested amountIn before
the external transfer (TokenTransfer.safeTransferFrom), which allows
fee-on-transfer/non-standard ERC20s to cause the contract to receive less than
order.remainingIn; update the logic in the order creation flow to verify and
enforce escrow invariants immediately after the transfer: perform the
safeTransferFrom, read the token balance delta (e.g.,
beforeBalance/afterBalance) and then either (a) revert if the received amount <
amountIn, or (b) set order.remainingIn (and amountIn) to the actual received
amount and persist via _storeOrder/_addOrderToPair so cancelOrder and
matchOrders use the true escrowed amount; ensure the change references
TokenTransfer.safeTransferFrom, _storeOrder, _addOrderToPair, order.remainingIn,
cancelOrder and matchOrders so all later flows rely on the verified received
amount.
🪄 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: a0525d7a-4139-4ec3-8b4d-4b85f9db11f2
📒 Files selected for processing (1)
src/core/WindmillExchange.sol
aniket866
changed the title
aniket866
changed the title
aniket866
changed the title
20 tasks
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
Contributor
Author
|
hi @kaneki003 thankyou for review , I will correct all of them acutally this pr was created a way before I started making proposal, at that time I had no indepth knowledge about this project later I studied the project in detail during making proposal so I will now correct this PR you can point out the wrong/irrelevent codes |
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
kaneki003
reviewed
May 30, 2026
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.
Closes #1
PROject status after this PR
WindmillExchange.sol— create, cancel, matchWindmillExchange.solWindmillExchange.solIWindmillExchange,IERC20MathUtils,PriceCurve,TokenTransferOrderStorage,PairStorageOrderstruct,pairKeyOfhelperDeployWindmill.s.solWindmillExchange.t.sol— unit tests + 1 fuzz testcreateOrder(...)cancelOrder(uint256)matchOrders(uint256, uint256)currentPrice(uint256, uint256)getOrder(uint256)getOrdersByPair(address, address)WindmillExchange — MVP
Adds a fully self-contained on-chain DEX with dynamic linear price curves and keeper-triggered matching.
Feature and its Use Cases
1 — Reward for matching orders
People who call
matchOrders()should get a small fee for doing the work.2 — Limit order list size per pair
Right now the order list can grow forever. Add a way to fetch orders in chunks so it stays cheap to read.
3 — Reset price after partial fill
When an order is only partly filled, its price should restart from that point — not keep counting from when it was first created.
4 — Better error for same token
If someone tries to trade a token for itself, show a
SameTokenerror instead of the currentZeroAddresswhich is confusing.5 — Expiry on match call
Let keepers set a deadline when calling
matchOrders()so the trade can't be held and submitted late.6 — Use a price feed for start price
Let orders pull the starting price from an on-chain oracle instead of always entering it manually.
7 — Admin controls
Add a basic owner role to pause the exchange and change settings when needed.
Files Added
File | PurposeKey Design Choices
Out of Scope
Fees, oracle integration, and governance are deferred to the next milestone.
Storage/Layout Impact:
Gas and Performance:
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:
Additional Notes:
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
New Features
Tests
Chores
@Zahnentferner , @kaneki003 Please review