From 92537a51203c59dc21240ad5d1fba42019ffff9d Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Thu, 15 May 2025 11:40:14 +1000 Subject: [PATCH] fix(slot reservations): clear AddressSet instead of delete (#235) * fix(slot-reservations): Allows slot to be reserved when in repair Previous to when SlotState.Repair was implemented, slots in repair would be considered free and the slots could be reserved in this state. Now that SlotState.Repair has been implemented, the `canReserveSlot` needs to check that the SlotState is in Repair or is Free before allowing reservation. * fix(slot reservations): clear AddressSet instead of delete Deleting an AddressSet causes corrupted memory. Each address must be removed individually, which is OK to do since there is a maxReservations parameter that keeps this number small. https://docs.openzeppelin.com/contracts/5.x/api/utils#EnumerableSet * Switch to EnumerableSet clear function provided by openzeppelin --------- Co-authored-by: Arnaud --- contracts/Marketplace.sol | 2 +- package-lock.json | 17 +++++++++-------- package.json | 2 +- test/Marketplace.test.js | 31 +++++++++++++++++++++++++++++-- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/contracts/Marketplace.sol b/contracts/Marketplace.sol index 60d8aef..ac6938d 100644 --- a/contracts/Marketplace.sol +++ b/contracts/Marketplace.sol @@ -376,7 +376,7 @@ contract Marketplace is SlotReservations, Proofs, StateRetrieval, Endian { context.fundsToReturnToClient += _slotPayout(requestId, slot.filledAt); _removeFromMySlots(slot.host, slotId); - delete _reservations[slotId]; // We purge all the reservations for the slot + _reservations[slotId].clear(); // We purge all the reservations for the slot slot.state = SlotState.Repair; slot.filledAt = 0; slot.currentCollateral = 0; diff --git a/package-lock.json b/package-lock.json index 1b83a12..fd499a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "devDependencies": { "@nomiclabs/hardhat-ethers": "^2.2.1", "@nomiclabs/hardhat-waffle": "^2.0.3", - "@openzeppelin/contracts": "^5.2.0", + "@openzeppelin/contracts": "^5.3.0", "@stdlib/stats-binomial-test": "^0.0.7", "chai": "^4.3.7", "ethereum-waffle": "^3.4.4", @@ -1764,10 +1764,11 @@ } }, "node_modules/@openzeppelin/contracts": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.2.0.tgz", - "integrity": "sha512-bxjNie5z89W1Ea0NZLZluFh8PrFNn9DH8DQlujEok2yjsOlraUPKID5p1Wk3qdNbf6XkQ1Os2RvfiHrrXLHWKA==", - "dev": true + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.3.0.tgz", + "integrity": "sha512-zj/KGoW7zxWUE8qOI++rUM18v+VeLTTzKs/DJFkSzHpQFPD/jKKF0TrMxBfGLl3kpdELCNccvB3zmofSzm4nlA==", + "dev": true, + "license": "MIT" }, "node_modules/@pnpm/config.env-replace": { "version": "1.1.0", @@ -24578,9 +24579,9 @@ } }, "@openzeppelin/contracts": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.2.0.tgz", - "integrity": "sha512-bxjNie5z89W1Ea0NZLZluFh8PrFNn9DH8DQlujEok2yjsOlraUPKID5p1Wk3qdNbf6XkQ1Os2RvfiHrrXLHWKA==", + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.3.0.tgz", + "integrity": "sha512-zj/KGoW7zxWUE8qOI++rUM18v+VeLTTzKs/DJFkSzHpQFPD/jKKF0TrMxBfGLl3kpdELCNccvB3zmofSzm4nlA==", "dev": true }, "@pnpm/config.env-replace": { diff --git a/package.json b/package.json index 301f338..8848549 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "devDependencies": { "@nomiclabs/hardhat-ethers": "^2.2.1", "@nomiclabs/hardhat-waffle": "^2.0.3", - "@openzeppelin/contracts": "^5.2.0", + "@openzeppelin/contracts": "^5.3.0", "@stdlib/stats-binomial-test": "^0.0.7", "chai": "^4.3.7", "ethereum-waffle": "^3.4.4", diff --git a/test/Marketplace.test.js b/test/Marketplace.test.js index a6ab152..9904a40 100644 --- a/test/Marketplace.test.js +++ b/test/Marketplace.test.js @@ -338,6 +338,7 @@ describe("Marketplace", function () { (collateral * config.collateral.repairRewardPercentage) / 100 ) await token.approve(marketplace.address, discountedCollateral) + await marketplace.reserveSlot(slot.request, slot.index) await marketplace.fillSlot(slot.request, slot.index, proof) const endBalance = await token.balanceOf(host.address) expect(startBalance - endBalance).to.equal(discountedCollateral) @@ -408,7 +409,7 @@ describe("Marketplace", function () { await waitUntilFailed(marketplace, request) await expect( marketplace.fillSlot(slot.request, slot.index, proof) - ).to.be.revertedWith("Marketplace_SlotNotFree") + ).to.be.revertedWith("Marketplace_ReservationRequired") }) it("is rejected when slot index not in range", async function () { @@ -563,16 +564,19 @@ describe("Marketplace", function () { describe("freeing a slot", function () { let id + let collateral beforeEach(async function () { slot.index = 0 id = slotId(slot) + period = config.proofs.period + ;({ periodOf, periodEnd } = periodic(period)) switchAccount(client) await token.approve(marketplace.address, maxPrice(request)) await marketplace.requestStorage(request) switchAccount(host) - const collateral = collateralPerSlot(request) + collateral = collateralPerSlot(request) await token.approve(marketplace.address, collateral) }) @@ -603,6 +607,29 @@ describe("Marketplace", function () { .to.emit(marketplace, "SlotFreed") .withArgs(slot.request, slot.index) }) + + it("can reserve and fill a freed slot", async function () { + // Make a reservation from another host + switchAccount(host2) + collateral = collateralPerSlot(request) + await token.approve(marketplace.address, collateral) + await marketplace.reserveSlot(slot.request, slot.index) + + // Switch host and free the slot + switchAccount(host) + await waitUntilStarted(marketplace, request, proof, token) + await marketplace.freeSlot(id) + + // At this point, the slot should be freed and in a repair state. + // Another host should be able to make a reservation for this + // slot and fill it. + switchAccount(host2) + await marketplace.reserveSlot(slot.request, slot.index) + let currPeriod = periodOf(await currentTime()) + await advanceTimeTo(periodEnd(currPeriod) + 1) + await token.approve(marketplace.address, collateral) + await marketplace.fillSlot(slot.request, slot.index, proof) + }) }) describe("paying out a slot", function () {