From d645633ff40a8fd1545537e22d1f603dcf3a269d Mon Sep 17 00:00:00 2001 From: Matej Poklemba Date: Thu, 11 Apr 2024 16:17:14 +0200 Subject: [PATCH] Simplify getters and use `@inheritdoc` --- .../contracts/orderbook/IOrderbookDex.sol | 8 +- .../contracts/orderbook/OrderbookDex.sol | 81 +++++++------------ .../evm-contracts/test/OrderbookDex.t.sol | 34 ++++---- .../test/OrderbookDexInvariant.t.sol | 10 +-- 4 files changed, 54 insertions(+), 79 deletions(-) diff --git a/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol b/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol index 6d1357ae..5b126fe8 100644 --- a/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol +++ b/packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol @@ -48,11 +48,11 @@ interface IOrderbookDex is IERC1155Receiver { /// @param id The order's unique identifier. event OrderCancelled(address indexed seller, uint256 indexed id); - /// @notice Returns the address of the asset that is being traded in this DEX contract. - function getAsset() external view returns (address); + /// @notice The address of the asset that is being traded in this DEX contract. + function asset() external view returns (address); - /// @notice Returns the `orderId` of the next sell order. - function getCurrentOrderId() external view returns (uint256); + /// @notice The `orderId` of the next sell order. + function currentOrderId() external view returns (uint256); /// @notice Returns the Order struct information about an order identified by the `orderId`. function getOrder(uint256 orderId) external view returns (Order memory); diff --git a/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol b/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol index d66a495b..35b68720 100644 --- a/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol +++ b/packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol @@ -16,9 +16,11 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { using Address for address payable; using Arrays for uint256[]; - IInverseProjected1155 internal immutable asset; + /// @inheritdoc IOrderbookDex + address public immutable override asset; + /// @inheritdoc IOrderbookDex + uint256 public override currentOrderId; mapping(uint256 orderId => Order) internal orders; - uint256 internal currentOrderId; error OrderDoesNotExist(uint256 orderId); error InsufficientEndAmount(uint256 expectedAmount, uint256 actualAmount); @@ -26,30 +28,16 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { error InvalidInput(uint256 input); error Unauthorized(address sender); - constructor(IInverseProjected1155 _asset) { + constructor(address _asset) { asset = _asset; } - /// @notice Returns the address of the asset that is being traded in this DEX contract. - function getAsset() public view virtual returns (address) { - return address(asset); - } - - /// @notice Returns the `orderId` of the next sell order. - function getCurrentOrderId() public view virtual returns (uint256) { - return currentOrderId; - } - - /// @notice Returns the Order struct information about an order identified by the `orderId`. + /// @inheritdoc IOrderbookDex function getOrder(uint256 orderId) public view virtual returns (Order memory) { return orders[orderId]; } - /// @notice Creates a sell order for the `assetAmount` of `assetId` at `pricePerAsset`. - /// @dev The order information is saved in a mapping `orderId -> Order`, with `orderId` being a unique incremental identifier. - /// MUST transfer the `assetAmount` of `assetId` from the seller to the contract. - /// MUST emit `OrderCreated` event. - /// @return The unique identifier of the created order. + /// @inheritdoc IOrderbookDex function createSellOrder( uint256 assetId, uint256 assetAmount, @@ -58,7 +46,13 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { if (assetAmount == 0 || pricePerAsset == 0) { revert InvalidInput(0); } - asset.safeTransferFrom(msg.sender, address(this), assetId, assetAmount, bytes("")); + IInverseProjected1155(asset).safeTransferFrom( + msg.sender, + address(this), + assetId, + assetAmount, + bytes("") + ); Order memory newOrder = Order({ assetId: assetId, assetAmount: assetAmount, @@ -72,9 +66,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { return orderId; } - /// @notice Creates a batch of sell orders for the `assetAmount` of `assetId` at `pricePerAsset`. - /// @dev This is a batched version of `createSellOrder` that simply iterates through the arrays to call said function. - /// @return The unique identifiers of the created orders. + /// @inheritdoc IOrderbookDex function createBatchSellOrder( uint256[] memory assetIds, uint256[] memory assetAmounts, @@ -97,14 +89,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { return orderIds; } - /// @notice Consecutively fills an array of orders identified by the `orderId` of each order, - /// by providing an exact amount of ETH and requesting a specific minimum 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 at least `minimumAsset`. - /// If msg.value is more than the sum of orders' prices, it MUST refund the excess back to `msg.sender`. - /// MUST decrease the `assetAmount` parameter for the specified order according to how much of it was filled, - /// and transfer that amount of the order's `assetId` to the buyer. - /// MUST emit `OrderFilled` event for each order accordingly. + /// @inheritdoc IOrderbookDex function fillOrdersExactEth( uint256 minimumAsset, uint256[] memory orderIds @@ -128,7 +113,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { order.assetAmount -= assetsToBuy; remainingEth -= assetsToBuy * order.pricePerAsset; totalAssetReceived += assetsToBuy; - asset.safeTransferFrom( + IInverseProjected1155(asset).safeTransferFrom( address(this), msg.sender, order.assetId, @@ -149,14 +134,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { } } - /// @notice Consecutively fills an array of orders identified by the `orderId` of each order, - /// 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`. - /// MUST decrease the `assetAmount` parameter for the specified order according to how much of it was filled, - /// and transfer that amount of the order's `assetId` to the buyer. - /// 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`. + /// @inheritdoc IOrderbookDex function fillOrdersExactAsset( uint256 assetAmount, uint256[] memory orderIds @@ -180,7 +158,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { order.assetAmount -= assetsToBuy; remainingEth -= assetsToBuy * order.pricePerAsset; remainingAsset -= assetsToBuy; - asset.safeTransferFrom( + IInverseProjected1155(asset).safeTransferFrom( address(this), msg.sender, order.assetId, @@ -201,11 +179,7 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { } } - /// @notice Cancels the sell order identified by the `orderId`, transferring the order's assets back to the seller. - /// @dev MUST revert if the order's seller is not `msg.sender`. - /// MUST change the `assetAmount` parameter for the specified order to `0`. - /// MUST emit `OrderCancelled` event. - /// MUST transfer the `assetAmount` of `assetId` back to the seller. + /// @inheritdoc IOrderbookDex function cancelSellOrder(uint256 orderId) public virtual { Order storage order = orders[orderId]; if (msg.sender != order.seller) { @@ -213,12 +187,17 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { } uint256 assetAmount = order.assetAmount; delete order.assetAmount; - asset.safeTransferFrom(address(this), msg.sender, order.assetId, assetAmount, bytes("")); + IInverseProjected1155(asset).safeTransferFrom( + address(this), + msg.sender, + order.assetId, + assetAmount, + bytes("") + ); emit OrderCancelled(msg.sender, orderId); } - /// @notice Cancels a batch of sell orders identified by the `orderIds`, transferring the orders' assets back to the seller. - /// @dev This is a batched version of `cancelSellOrder` that simply iterates through the array to call said function. + /// @inheritdoc IOrderbookDex function cancelBatchSellOrder(uint256[] memory orderIds) public virtual { for (uint256 i; i < orderIds.length; ) { cancelSellOrder(orderIds.unsafeMemoryAccess(i)); @@ -235,8 +214,4 @@ contract OrderbookDex is IOrderbookDex, ERC1155Holder, ReentrancyGuard { return interfaceId == type(IOrderbookDex).interfaceId || super.supportsInterface(interfaceId); } - - function _getOrderId(address seller, uint96 orderId) internal view virtual returns (uint256) { - return (uint256(uint160(seller)) << 96) ^ orderId; - } } diff --git a/packages/contracts/evm-contracts/test/OrderbookDex.t.sol b/packages/contracts/evm-contracts/test/OrderbookDex.t.sol index fc6c0be6..3d78006e 100644 --- a/packages/contracts/evm-contracts/test/OrderbookDex.t.sol +++ b/packages/contracts/evm-contracts/test/OrderbookDex.t.sol @@ -26,7 +26,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function setUp() public { asset = new InverseAppProjected1155("Gold", "GOLD", address(this)); - dex = new OrderbookDex(asset); + dex = new OrderbookDex(address(asset)); asset.setApprovalForAll(address(dex), true); vm.deal(alice, 1_000 ether); vm.deal(boris, 1_000 ether); @@ -38,7 +38,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { ) public { vm.assume(assetAmount > 0 && pricePerAsset > 0); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); vm.expectEmit(true, true, true, true); @@ -70,7 +70,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { assetIds[i] = asset.mint(assetAmounts[i], ""); } - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); for (uint256 i = 0; i < orderCount; ++i) { vm.expectEmit(true, true, true, true); emit IOrderbookDex.OrderCreated( @@ -133,7 +133,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { address payable seller = payable(vm.addr(uint256(keccak256(abi.encodePacked(i))))); uint256 assetAmount = price / (ordersCount - i); uint256 pricePerAsset = price / assetAmount; - orderIds[i] = dex.getCurrentOrderId(); + orderIds[i] = dex.currentOrderId(); sellers[i] = seller; vm.startPrank(seller); uint256 assetId = asset.mint(assetAmount, ""); @@ -190,7 +190,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { address payable seller = payable(vm.addr(uint256(keccak256(abi.encodePacked(i))))); uint256 assetAmount = price / (ordersCount - i); uint256 pricePerAsset = price / assetAmount; - orderIds[i] = dex.getCurrentOrderId(); + orderIds[i] = dex.currentOrderId(); sellers[i] = seller; vm.startPrank(seller); uint256 assetId = asset.mint(assetAmount, ""); @@ -248,7 +248,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { address buyer = alice; address payable seller = payable(address(this)); vm.deal(buyer, assetAmount * pricePerAsset); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); vm.startPrank(seller); uint256 assetId = asset.mint(assetAmount, ""); asset.setApprovalForAll(address(dex), true); @@ -282,7 +282,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { address buyer = alice; address payable seller = payable(address(this)); vm.deal(buyer, assetAmount * pricePerAsset); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); vm.startPrank(seller); uint256 assetId = asset.mint(assetAmount, ""); asset.setApprovalForAll(address(dex), true); @@ -313,7 +313,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { vm.assume(pricePerAsset < type(uint256).max / assetAmount / multiplier); vm.deal(alice, assetAmount * pricePerAsset * multiplier); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); @@ -335,7 +335,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { vm.assume(pricePerAsset < type(uint256).max / assetAmount / multiplier); vm.deal(alice, assetAmount * pricePerAsset * multiplier); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); @@ -350,7 +350,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function test_fillOrdersExactEth_wontFillOrderIfOrderWasCancelled() public { uint256 assetAmount = 100; uint256 pricePerAsset = 200; - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); dex.cancelSellOrder(orderId); @@ -367,7 +367,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function test_fillOrdersExactAsset_wontFillOrderIfOrderWasCancelled() public { uint256 assetAmount = 100; uint256 pricePerAsset = 200; - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); dex.cancelSellOrder(orderId); @@ -384,7 +384,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function test_fillOrdersExactEth_reverts_ifInsufficientEndAmount() public { uint256 assetAmount = 10; uint256 pricePerAsset = 100; - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); @@ -406,7 +406,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function test_fillOrdersExactAsset_reverts_ifInsufficientEndAmount() public { uint256 assetAmount = 10; uint256 pricePerAsset = 100; - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(assetAmount, ""); dex.createSellOrder(assetId, assetAmount, pricePerAsset); @@ -434,7 +434,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { function testFuzz_cancelSellOrder_satisfiesRequirements(uint256 assetAmount) public { vm.assume(assetAmount > 0); uint256 assetId = asset.mint(assetAmount, ""); - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); dex.createSellOrder(assetId, assetAmount, 200); vm.expectEmit(true, true, true, true); @@ -447,7 +447,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { } function test_cancelSellOrder_reverts_ifUnauthorized() public { - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(100, ""); dex.createSellOrder(assetId, 100, 200); @@ -468,7 +468,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { assetIds[i] = asset.mint(assetAmounts[i], ""); } - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256[] memory orderIds = dex.createBatchSellOrder( assetIds, @@ -492,7 +492,7 @@ contract OrderbookDexTest is CTest, ERC1155Holder { } function test_cancelBatchSellOrder_reverts_ifUnauthorized() public { - uint256 orderId = dex.getCurrentOrderId(); + uint256 orderId = dex.currentOrderId(); uint256 assetId = asset.mint(100, ""); dex.createSellOrder(assetId, 100, 200); diff --git a/packages/contracts/evm-contracts/test/OrderbookDexInvariant.t.sol b/packages/contracts/evm-contracts/test/OrderbookDexInvariant.t.sol index 1d1dc08c..ddfed6dc 100644 --- a/packages/contracts/evm-contracts/test/OrderbookDexInvariant.t.sol +++ b/packages/contracts/evm-contracts/test/OrderbookDexInvariant.t.sol @@ -109,7 +109,7 @@ contract OrderbookDexHandler is CTest, ERC1155Holder { asset.setApprovalForAll(address(dex), true); // Take note of the previous order id - previousOrderId = dex.getCurrentOrderId(); + previousOrderId = dex.currentOrderId(); // Execute the sell order creation return dex.createSellOrder(assetId, assetAmount, price); @@ -291,7 +291,7 @@ contract OrderbookDexHandler is CTest, ERC1155Holder { ) public useActor(actorIndexSeed) { // If the order does not exist, get the last order id if (dex.getOrder(orderId).assetAmount == 0) { - orderId = dex.getCurrentOrderId(); + orderId = dex.currentOrderId(); // If there are no orders, return if (orderId == 0) { return; @@ -358,7 +358,7 @@ contract OrderbookDexInvariantTest is CTest, ERC1155Holder { function setUp() public { asset = new InverseAppProjected1155("Gold", "GOLD", address(this)); - dex = new OrderbookDex(asset); + dex = new OrderbookDex(address(asset)); dexHandler = new OrderbookDexHandler(asset, dex); assetHandler = new AssetHandler(asset, dex); targetContract(address(assetHandler)); @@ -366,7 +366,7 @@ contract OrderbookDexInvariantTest is CTest, ERC1155Holder { } function invariant_ordersAssetAmountEqualsContractTokenBalance() public { - for (uint256 i; i < dex.getCurrentOrderId(); ++i) { + for (uint256 i; i < dex.currentOrderId(); ++i) { IOrderbookDex.Order memory order = dex.getOrder(i); assertEq(order.assetAmount, asset.balanceOf(address(dex), order.assetId)); } @@ -377,7 +377,7 @@ contract OrderbookDexInvariantTest is CTest, ERC1155Holder { } function invariant_orderIdIsIncremental() public { - uint256 currentId = dex.getCurrentOrderId(); + uint256 currentId = dex.currentOrderId(); uint256 previousId = dexHandler.previousOrderId(); if (currentId == previousId) { assertEq(currentId, 0);