From 1267a38a9904340da38f187d925ae67522cb4c98 Mon Sep 17 00:00:00 2001 From: Matej Poklemba Date: Wed, 3 Apr 2024 15:11:10 +0200 Subject: [PATCH] Remove unnecessary `cancelled` attribute --- .../contracts/orderbook/IOrderbookDex.sol | 5 +---- .../contracts/orderbook/OrderbookDex.sol | 13 +++++-------- .../contracts/evm-contracts/test/OrderbookDex.t.sol | 3 +-- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol b/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol index fa5741483..c946fe6ca 100644 --- a/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol +++ b/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol @@ -11,7 +11,6 @@ interface IOrderbookDex is IERC1155Receiver { uint256 assetId; uint256 assetAmount; uint256 pricePerAsset; - bool cancelled; } event OrderCreated( @@ -48,7 +47,6 @@ interface IOrderbookDex is IERC1155Receiver { /// @dev Transfers portions of msg.value to the orders' sellers according to the price. /// The sum of asset amounts of filled orders MUST be at least `minimumAsset`. /// If msg.value is more than the sum of orders' prices, it MUST refund the excess back to msg.sender. - /// An order whose `cancelled` parameter has value `true` MUST NOT be filled. /// MUST change the `assetAmount` parameter for the specified order according to how much of it was filled. /// MUST emit `OrderFilled` event for each order accordingly. function fillOrdersExactEth( @@ -61,7 +59,6 @@ interface IOrderbookDex is IERC1155Receiver { /// by providing a possibly surplus amount of ETH and requesting an exact amount of asset to receive. /// @dev Transfers portions of msg.value to the orders' sellers according to the price. /// The sum of asset amounts of filled orders MUST be exactly `assetAmount`. Excess ETH MUST be returned back to `msg.sender`. - /// An order whose `cancelled` parameter has value `true` MUST NOT be filled. /// MUST change the `assetAmount` parameter for the specified order according to how much of it was filled. /// MUST emit `OrderFilled` event for each order accordingly. /// If msg.value is more than the sum of orders' prices, it MUST refund the difference back to msg.sender. @@ -72,7 +69,7 @@ interface IOrderbookDex is IERC1155Receiver { ) external payable; /// @notice Cancels the sell order identified by combination ``, making it unfillable. - /// @dev MUST change the `cancelled` parameter for the specified order to `true`. + /// @dev MUST change the `assetAmount` parameter for the specified order to `0`. /// MUST emit `OrderCancelled` event. function cancelSellOrder(uint256 orderId) external; } diff --git a/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol b/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol index f09c2102d..810edb55d 100644 --- a/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol +++ b/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol @@ -57,8 +57,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { Order memory newOrder = Order({ assetId: assetId, assetAmount: assetAmount, - pricePerAsset: pricePerAsset, - cancelled: false + pricePerAsset: pricePerAsset }); uint256 orderId = sellersOrderId[msg.sender]; orders[msg.sender][orderId] = newOrder; @@ -71,7 +70,6 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { /// @dev Transfers portions of msg.value to the orders' sellers according to the price. /// The sum of asset amounts of filled orders MUST be at least `minimumAsset`. /// If msg.value is more than the sum of orders' prices, it MUST refund the excess back to msg.sender. - /// An order whose `cancelled` parameter has value `true` MUST NOT be filled. /// MUST change the `assetAmount` parameter for the specified order according to how much of it was filled. /// MUST emit `OrderFilled` event for each order accordingly. function fillOrdersExactEth( @@ -89,7 +87,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { address payable seller = sellers[i]; uint256 orderId = orderIds[i]; Order storage order = orders[seller][orderId]; - if (order.cancelled || order.assetAmount == 0 || order.pricePerAsset == 0) { + if (order.assetAmount == 0) { continue; } uint256 assetsToBuy = remainingEth / order.pricePerAsset; @@ -127,7 +125,6 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { /// by providing a possibly surplus amount of ETH and requesting an exact amount of asset to receive. /// @dev Transfers portions of msg.value to the orders' sellers according to the price. /// The sum of asset amounts of filled orders MUST be exactly `assetAmount`. Excess ETH MUST be returned back to `msg.sender`. - /// An order whose `cancelled` parameter has value `true` MUST NOT be filled. /// MUST change the `assetAmount` parameter for the specified order according to how much of it was filled. /// MUST emit `OrderFilled` event for each order accordingly. /// If msg.value is more than the sum of orders' prices, it MUST refund the difference back to msg.sender. @@ -146,7 +143,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { address payable seller = sellers[i]; uint256 orderId = orderIds[i]; Order storage order = orders[seller][orderId]; - if (order.cancelled || order.assetAmount == 0 || order.pricePerAsset == 0) { + if (order.assetAmount == 0) { continue; } uint256 assetsToBuy = order.assetAmount; @@ -181,14 +178,14 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { } /// @notice Cancels the sell order identified by combination ``, making it unfillable. - /// @dev MUST change the `cancelled` parameter for the specified order to `true`. + /// @dev MUST change the `assetAmount` parameter for the specified order to `0`. /// MUST emit `OrderCancelled` event. function cancelSellOrder(uint256 orderId) public virtual { Order memory order = orders[msg.sender][orderId]; if (order.assetAmount == 0) { revert OrderDoesNotExist(orderId); } - orders[msg.sender][orderId].cancelled = true; + orders[msg.sender][orderId].assetAmount = 0; asset.safeTransferFrom( address(this), msg.sender, diff --git a/packages/contracts/evm-contracts/test/OrderbookDex.t.sol b/packages/contracts/evm-contracts/test/OrderbookDex.t.sol index 62cd716b3..14969dfb1 100644 --- a/packages/contracts/evm-contracts/test/OrderbookDex.t.sol +++ b/packages/contracts/evm-contracts/test/OrderbookDex.t.sol @@ -52,7 +52,6 @@ contract OrderbookDexTest is CTest, ERC1155Holder { assertEq(order.assetId, assetId); assertEq(order.assetAmount, assetAmount); assertEq(order.pricePerAsset, pricePerAsset); - assertTrue(!order.cancelled); assertEq(asset.balanceOf(address(dex), assetId), assetAmount); } @@ -66,7 +65,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { emit IOrderbookDex.OrderCancelled(address(this), orderId); dex.cancelSellOrder(orderId); IOrderbookDex.Order memory order = dex.getOrder(address(this), orderId); - assertTrue(order.cancelled); + assertEq(order.assetAmount, 0); assertEq(asset.balanceOf(address(dex), assetId), 0); assertEq(asset.balanceOf(address(this), assetId), assetAmount); }