From 8be756808c15a7b5e5c17da29fd1192f1907764d Mon Sep 17 00:00:00 2001 From: Eric Mastro Date: Wed, 7 Sep 2022 15:25:01 +1000 Subject: [PATCH] [marketplace] address various PR comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove `FundsWithdrawn` event - do not copy request to memory - todo for changing withdraw amount to not include proof payments - test lock expiry border - in tests, move `RequestState` to exported const in marketplace.js - move test for state checks on `fillSlot` to the “filling a slot” section. --- contracts/Marketplace.sol | 17 ++++++++-------- test/AccountLocks.test.js | 2 +- test/Marketplace.test.js | 41 +++++++++++++-------------------------- test/marketplace.js | 10 +++++++++- 4 files changed, 33 insertions(+), 37 deletions(-) diff --git a/contracts/Marketplace.sol b/contracts/Marketplace.sol index bc0d52d..ce4eb47 100644 --- a/contracts/Marketplace.sol +++ b/contracts/Marketplace.sol @@ -102,22 +102,24 @@ contract Marketplace is Collateral, Proofs { /// @dev Request must be expired, must be in RequestState.New, and the transaction must originate from the depositer address. /// @param requestId the id of the request function withdrawFunds(bytes32 requestId) public marketplaceInvariant { - Request memory request = requests[requestId]; + Request storage request = requests[requestId]; require(block.timestamp > request.expiry, "Request not yet timed out"); require(request.client == msg.sender, "Invalid client address"); RequestContext storage context = requestContexts[requestId]; require(context.state == RequestState.New, "Invalid state"); - uint256 amount = _price(request); - funds.sent += amount; - funds.balance -= amount; - token.transfer(msg.sender, amount); - emit FundsWithdrawn(requestId); - // Update request state to Cancelled. Handle in the withdraw transaction // as there needs to be someone to pay for the gas to update the state context.state = RequestState.Cancelled; emit RequestCancelled(requestId); + + // TODO: To be changed once we start paying out hosts for the time they + // fill a slot. The amount that we paid to hosts will then have to be + // deducted from the price. + uint256 amount = _price(request); + funds.sent += amount; + funds.balance -= amount; + require(token.transfer(msg.sender, amount), "Withdraw failed"); } /// @notice Return true if the request state is RequestState.Cancelled or if the request expiry time has elapsed and the request was never started. @@ -262,7 +264,6 @@ contract Marketplace is Collateral, Proofs { bytes32 indexed slotId ); event RequestCancelled(bytes32 requestId); - event FundsWithdrawn(bytes32 requestId); modifier marketplaceInvariant() { MarketplaceFunds memory oldFunds = funds; diff --git a/test/AccountLocks.test.js b/test/AccountLocks.test.js index 35d6a9a..1fbee6a 100644 --- a/test/AccountLocks.test.js +++ b/test/AccountLocks.test.js @@ -193,7 +193,7 @@ describe("Account Locks", function () { }) it("fails when lock is already expired", async function () { - waitUntilExpired(expiry + hours(1)) + waitUntilExpired(expiry) await expect(locks.extendLockExpiry(id, hours(1))).to.be.revertedWith( "Lock already expired" ) diff --git a/test/Marketplace.test.js b/test/Marketplace.test.js index f8f7b16..8191991 100644 --- a/test/Marketplace.test.js +++ b/test/Marketplace.test.js @@ -4,7 +4,7 @@ const { expect } = require("chai") const { exampleRequest } = require("./examples") const { now, hours } = require("./time") const { requestId, slotId, askToArray } = require("./ids") -const { waitUntilExpired } = require("./marketplace") +const { waitUntilExpired, RequestState } = require("./marketplace") const { price, pricePerSlot } = require("./price") const { snapshot, @@ -176,6 +176,16 @@ describe("Marketplace", function () { marketplace.fillSlot(slot.request, invalid, proof) ).to.be.revertedWith("Invalid slot") }) + + it("fails when all slots are already filled", async function () { + const lastSlot = request.ask.slots - 1 + for (let i = 0; i <= lastSlot; i++) { + await marketplace.fillSlot(slot.request, i, proof) + } + await expect( + marketplace.fillSlot(slot.request, lastSlot, proof) + ).to.be.revertedWith("Invalid state") + }) }) describe("paying out a slot", function () { @@ -257,16 +267,9 @@ describe("Marketplace", function () { for (let i = 0; i <= lastSlot; i++) { await marketplace.fillSlot(slot.request, i, proof) } - await expect(await marketplace.state(slot.request)).to.equal(1) - }) - it("fails when all slots are already filled", async function () { - const lastSlot = request.ask.slots - 1 - for (let i = 0; i <= lastSlot; i++) { - await marketplace.fillSlot(slot.request, i, proof) - } - await expect( - marketplace.fillSlot(slot.request, lastSlot, proof) - ).to.be.revertedWith("Invalid state") + await expect(await marketplace.state(slot.request)).to.equal( + RequestState.Started + ) }) }) @@ -307,14 +310,6 @@ describe("Marketplace", function () { ) }) - it("emits event once funds are withdrawn", async function () { - await waitUntilExpired(request.expiry) - switchAccount(client) - await expect(marketplace.withdrawFunds(slot.request)) - .to.emit(marketplace, "FundsWithdrawn") - .withArgs(requestId(request)) - }) - it("emits event once request is cancelled", async function () { await waitUntilExpired(request.expiry) switchAccount(client) @@ -343,14 +338,6 @@ describe("Marketplace", function () { await marketplace.deposit(collateral) }) - const RequestState = { - New: 0, - Started: 1, - Cancelled: 2, - Finished: 3, - Failed: 4, - } - it("state is Cancelled when client withdraws funds", async function () { await expect(await marketplace.state(slot.request)).to.equal( RequestState.New diff --git a/test/marketplace.js b/test/marketplace.js index d4bc229..008be27 100644 --- a/test/marketplace.js +++ b/test/marketplace.js @@ -1,5 +1,13 @@ +const RequestState = { + New: 0, + Started: 1, + Cancelled: 2, + Finished: 3, + Failed: 4, +} + async function waitUntilExpired(expiry) { await ethers.provider.send("hardhat_mine", [ethers.utils.hexValue(expiry)]) } -module.exports = { waitUntilExpired } +module.exports = { waitUntilExpired, RequestState }