Skip to content

Commit

Permalink
Remove unnecessary cancelled attribute
Browse files Browse the repository at this point in the history
  • Loading branch information
matejos committed Apr 3, 2024
1 parent 5e7aeb4 commit 1267a38
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ interface IOrderbookDex is IERC1155Receiver {
uint256 assetId;
uint256 assetAmount;
uint256 pricePerAsset;
bool cancelled;
}

event OrderCreated(
Expand Down Expand Up @@ -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(
Expand All @@ -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.
Expand All @@ -72,7 +69,7 @@ interface IOrderbookDex is IERC1155Receiver {
) external payable;

/// @notice Cancels the sell order identified by combination `<msg.sender, orderId>`, 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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -181,14 +178,14 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard {
}

/// @notice Cancels the sell order identified by combination `<msg.sender, orderId>`, 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,
Expand Down
3 changes: 1 addition & 2 deletions packages/contracts/evm-contracts/test/OrderbookDex.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
}
Expand Down

0 comments on commit 1267a38

Please sign in to comment.