[marketplace] address various PR comments

- 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

@@ -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;

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"
       )

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

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 }