Skip to content

Commit

Permalink
Simplify getters and use @inheritdoc
Browse files Browse the repository at this point in the history
  • Loading branch information
@matejos
matejos committed Apr 11, 2024
1 parent 6e9e6a3 commit fda626c
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 79 deletions.
8 changes: 4 additions & 4 deletions packages/contracts/evm-contracts/contracts/orderbook/IOrderbookDex.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
81 changes: 28 additions & 53 deletions packages/contracts/evm-contracts/contracts/orderbook/OrderbookDex.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,40 +16,28 @@ 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);
error InvalidArrayLength();
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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -201,24 +179,25 @@ 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) {
revert Unauthorized(msg.sender);
}
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));
Expand All @@ -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;
}
}
34 changes: 17 additions & 17 deletions packages/contracts/evm-contracts/test/OrderbookDex.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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, "");
Expand Down Expand Up @@ -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, "");
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

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

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

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

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

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

Expand Down
10 changes: 5 additions & 5 deletions packages/contracts/evm-contracts/test/OrderbookDexInvariant.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -358,15 +358,15 @@ 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));
targetContract(address(dexHandler));
}

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

0 comments on commit fda626c

Please sign in to comment.