From 576fefe46e9d5a27bbba10bd257bd9957f504ddc Mon Sep 17 00:00:00 2001 From: r4bbit <445106+0x-r4bbit@users.noreply.github.com> Date: Tue, 6 Aug 2024 11:07:03 +0200 Subject: [PATCH] refactor: Use custom errors instead of string messages This reduces the gas cost as custom errors always use 4 bytes. --- contracts/Marketplace.sol | 128 +++++++++++++++++++++++++------------- contracts/Proofs.sol | 49 ++++++++++++--- test/Marketplace.test.js | 60 ++++++++++-------- test/Proofs.test.js | 16 ++--- 4 files changed, 168 insertions(+), 85 deletions(-) diff --git a/contracts/Marketplace.sol b/contracts/Marketplace.sol index 4ea1b6f..3e44a61 100644 --- a/contracts/Marketplace.sol +++ b/contracts/Marketplace.sol @@ -12,6 +12,25 @@ import "./Endian.sol"; import "./Groth16.sol"; contract Marketplace is Proofs, StateRetrieval, Endian { + error Marketplace_RepairRewardPercentageTooHigh(); + error Marketplace_SlashPercentageTooHigh(); + error Marketplace_MaximumSlashingTooHigh(); + error Marketplace_InvalidExpiry(); + error Marketplace_InvalidMaxSlotLoss(); + error Marketplace_InvalidClientAddress(); + error Marketplace_RequestAlreadyExists(); + error Marketplace_InvalidSlot(); + error Marketplace_SlotNotFree(); + error Marketplace_InvalidSlotHost(); + error Marketplace_AlreadyPaid(); + error Marketplace_TransferFailed(); + error Marketplace_UnknownRequest(); + error Marketplace_RequestNotYetTimedOut(); + error Marketplace_InvalidState(); + error Marketplace_StartNotBeforeExpiry(); + error Marketplace_SlotNotAcceptingProofs(); + error Marketplace_SlotIsFree(); + using EnumerableSet for EnumerableSet.Bytes32Set; using Requests for Request; @@ -61,20 +80,21 @@ contract Marketplace is Proofs, StateRetrieval, Endian { ) Proofs(configuration.proofs, verifier) { _token = token_; - require( - configuration.collateral.repairRewardPercentage <= 100, - "Must be less than 100" - ); - require( - configuration.collateral.slashPercentage <= 100, - "Must be less than 100" - ); - require( + if (configuration.collateral.repairRewardPercentage > 100) { + revert Marketplace_RepairRewardPercentageTooHigh(); + } + + if (configuration.collateral.slashPercentage > 100) { + revert Marketplace_SlashPercentageTooHigh(); + } + + if ( configuration.collateral.maxNumberOfSlashes * - configuration.collateral.slashPercentage <= - 100, - "Maximum slashing exceeds 100%" - ); + configuration.collateral.slashPercentage > + 100 + ) { + revert Marketplace_MaximumSlashingTooHigh(); + } _config = configuration; } @@ -87,18 +107,20 @@ contract Marketplace is Proofs, StateRetrieval, Endian { } function requestStorage(Request calldata request) public { - require(request.client == msg.sender, "Invalid client address"); + if (request.client != msg.sender) { + revert Marketplace_InvalidClientAddress(); + } RequestId id = request.id(); - require(_requests[id].client == address(0), "Request already exists"); - require( - request.expiry > 0 && request.expiry < request.ask.duration, - "Expiry not in range" - ); - require( - request.ask.maxSlotLoss <= request.ask.slots, - "maxSlotLoss exceeds slots" - ); + if (_requests[id].client != address(0)) { + revert Marketplace_RequestAlreadyExists(); + } + if (request.expiry == 0 || request.expiry >= request.ask.duration) { + revert Marketplace_InvalidExpiry(); + } + if (request.ask.maxSlotLoss > request.ask.slots) { + revert Marketplace_InvalidMaxSlotLoss(); + } _requests[id] = request; _requestContexts[id].endsAt = block.timestamp + request.ask.duration; @@ -120,14 +142,18 @@ contract Marketplace is Proofs, StateRetrieval, Endian { Groth16Proof calldata proof ) public requestIsKnown(requestId) { Request storage request = _requests[requestId]; - require(slotIndex < request.ask.slots, "Invalid slot"); + if (slotIndex >= request.ask.slots) { + revert Marketplace_InvalidSlot(); + } SlotId slotId = Requests.slotId(requestId, slotIndex); Slot storage slot = _slots[slotId]; slot.requestId = requestId; slot.slotIndex = slotIndex; - require(slotState(slotId) == SlotState.Free, "Slot is not free"); + if (slotState(slotId) != SlotState.Free) { + revert Marketplace_SlotNotFree(); + } _startRequiringProofs(slotId, request.ask.proofProbability); submitProof(slotId, proof); @@ -160,9 +186,14 @@ contract Marketplace is Proofs, StateRetrieval, Endian { function freeSlot(SlotId slotId) public slotIsNotFree(slotId) { Slot storage slot = _slots[slotId]; - require(slot.host == msg.sender, "Slot filled by other host"); + if (slot.host != msg.sender) { + revert Marketplace_InvalidSlotHost(); + } + SlotState state = slotState(slotId); - require(state != SlotState.Paid, "Already paid"); + if (state == SlotState.Paid) { + revert Marketplace_AlreadyPaid(); + } if (state == SlotState.Finished) { _payoutSlot(slot.requestId, slotId); @@ -209,7 +240,10 @@ contract Marketplace is Proofs, StateRetrieval, Endian { } function markProofAsMissing(SlotId slotId, Period period) public { - require(slotState(slotId) == SlotState.Filled, "Slot not accepting proofs"); + if (slotState(slotId) != SlotState.Filled) { + revert Marketplace_SlotNotAcceptingProofs(); + } + _markProofAsMissing(slotId, period); Slot storage slot = _slots[slotId]; Request storage request = _requests[slot.requestId]; @@ -296,13 +330,18 @@ contract Marketplace is Proofs, StateRetrieval, Endian { /// @param requestId the id of the request function withdrawFunds(RequestId requestId) public { Request storage request = _requests[requestId]; - require( - block.timestamp > requestExpiry(requestId), - "Request not yet timed out" - ); - require(request.client == msg.sender, "Invalid client address"); + if (block.timestamp <= requestExpiry(requestId)) { + revert Marketplace_RequestNotYetTimedOut(); + } + + if (request.client != msg.sender) { + revert Marketplace_InvalidClientAddress(); + } + RequestContext storage context = _requestContexts[requestId]; - require(context.state == RequestState.New, "Invalid state"); + if (context.state != RequestState.New) { + revert Marketplace_InvalidState(); + } // 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 @@ -327,7 +366,10 @@ contract Marketplace is Proofs, StateRetrieval, Endian { } modifier requestIsKnown(RequestId requestId) { - require(_requests[requestId].client != address(0), "Unknown request"); + if (_requests[requestId].client == address(0)) { + revert Marketplace_UnknownRequest(); + } + _; } @@ -338,7 +380,9 @@ contract Marketplace is Proofs, StateRetrieval, Endian { } modifier slotIsNotFree(SlotId slotId) { - require(_slots[slotId].state != SlotState.Free, "Slot is free"); + if (_slots[slotId].state == SlotState.Free) { + revert Marketplace_SlotIsFree(); + } _; } @@ -362,11 +406,9 @@ contract Marketplace is Proofs, StateRetrieval, Endian { uint256 startingTimestamp ) private view returns (uint256) { Request storage request = _requests[requestId]; - require( - startingTimestamp < requestExpiry(requestId), - "Start not before expiry" - ); - + if (startingTimestamp >= requestExpiry(requestId)) { + revert Marketplace_StartNotBeforeExpiry(); + } return (requestExpiry(requestId) - startingTimestamp) * request.ask.reward; } @@ -415,7 +457,9 @@ contract Marketplace is Proofs, StateRetrieval, Endian { function _transferFrom(address sender, uint256 amount) internal { address receiver = address(this); - require(_token.transferFrom(sender, receiver, amount), "Transfer failed"); + if (!_token.transferFrom(sender, receiver, amount)) { + revert Marketplace_TransferFailed(); + } } event StorageRequested(RequestId requestId, Ask ask, uint256 expiry); diff --git a/contracts/Proofs.sol b/contracts/Proofs.sol index 5dfb02f..fc059eb 100644 --- a/contracts/Proofs.sol +++ b/contracts/Proofs.sol @@ -7,6 +7,15 @@ import "./Periods.sol"; import "./Groth16.sol"; abstract contract Proofs is Periods { + error Proofs_InsufficientBlockHeight(); + error Proofs_InvalidProof(); + error Proofs_ProofAlreadySubmitted(); + error Proofs_PeriodNotEnded(); + error Proofs_ValidationTimedOut(); + error Proofs_ProofNotMissing(); + error Proofs_ProofNotRequired(); + error Proofs_ProofAlreadyMarkedMissing(); + ProofConfig private _config; IGroth16Verifier private _verifier; @@ -14,7 +23,10 @@ abstract contract Proofs is Periods { ProofConfig memory config, IGroth16Verifier verifier ) Periods(config.period) { - require(block.number > 256, "Insufficient block height"); + if (block.number <= 256) { + revert Proofs_InsufficientBlockHeight(); + } + _config = config; _verifier = verifier; } @@ -113,19 +125,40 @@ abstract contract Proofs is Periods { Groth16Proof calldata proof, uint[] memory pubSignals ) internal { - require(!_received[id][_blockPeriod()], "Proof already submitted"); - require(_verifier.verify(proof, pubSignals), "Invalid proof"); + if (_received[id][_blockPeriod()]) { + revert Proofs_ProofAlreadySubmitted(); + } + + if (!_verifier.verify(proof, pubSignals)) { + revert Proofs_InvalidProof(); + } + _received[id][_blockPeriod()] = true; emit ProofSubmitted(id); } function _markProofAsMissing(SlotId id, Period missedPeriod) internal { uint256 end = _periodEnd(missedPeriod); - require(end < block.timestamp, "Period has not ended yet"); - require(block.timestamp < end + _config.timeout, "Validation timed out"); - require(!_received[id][missedPeriod], "Proof was submitted, not missing"); - require(_isProofRequired(id, missedPeriod), "Proof was not required"); - require(!_missing[id][missedPeriod], "Proof already marked as missing"); + if (end >= block.timestamp) { + revert Proofs_PeriodNotEnded(); + } + + if (block.timestamp >= end + _config.timeout) { + revert Proofs_ValidationTimedOut(); + } + + if (_received[id][missedPeriod]) { + revert Proofs_ProofNotMissing(); + } + + if (!_isProofRequired(id, missedPeriod)) { + revert Proofs_ProofNotRequired(); + } + + if (_missing[id][missedPeriod]) { + revert Proofs_ProofAlreadyMarkedMissing(); + } + _missing[id][missedPeriod] = true; _missed[id] += 1; } diff --git a/test/Marketplace.test.js b/test/Marketplace.test.js index 413ce68..dc2af29 100644 --- a/test/Marketplace.test.js +++ b/test/Marketplace.test.js @@ -57,18 +57,24 @@ describe("Marketplace constructor", function () { await revert() }) - function testPercentageOverflow(property) { + function testPercentageOverflow(property, expectedError) { it(`should reject for ${property} overflowing percentage values`, async () => { config.collateral[property] = 101 await expect( Marketplace.deploy(config, token.address, verifier.address) - ).to.be.revertedWith("Must be less than 100") + ).to.be.revertedWith(expectedError) }) } - testPercentageOverflow("repairRewardPercentage") - testPercentageOverflow("slashPercentage") + testPercentageOverflow( + "repairRewardPercentage", + "Marketplace_RepairRewardPercentageTooHigh" + ) + testPercentageOverflow( + "slashPercentage", + "Marketplace_SlashPercentageTooHigh" + ) it("should reject when total slash percentage exceeds 100%", async () => { config.collateral.slashPercentage = 1 @@ -76,7 +82,7 @@ describe("Marketplace constructor", function () { await expect( Marketplace.deploy(config, token.address, verifier.address) - ).to.be.revertedWith("Maximum slashing exceeds 100%") + ).to.be.revertedWith("Marketplace_MaximumSlashingTooHigh") }) }) @@ -159,7 +165,7 @@ describe("Marketplace", function () { let invalid = { ...request, client: host.address } await token.approve(marketplace.address, price(invalid)) await expect(marketplace.requestStorage(invalid)).to.be.revertedWith( - "Invalid client address" + "Marketplace_InvalidClientAddress" ) }) @@ -176,19 +182,19 @@ describe("Marketplace", function () { request.expiry = request.ask.duration + 1 await expect(marketplace.requestStorage(request)).to.be.revertedWith( - "Expiry not in range" + "Marketplace_InvalidExpiry" ) request.expiry = 0 await expect(marketplace.requestStorage(request)).to.be.revertedWith( - "Expiry not in range" + "Marketplace_InvalidExpiry" ) }) it("is rejected when maxSlotLoss exceeds slots", async function () { request.ask.maxSlotLoss = request.ask.slots + 1 await expect(marketplace.requestStorage(request)).to.be.revertedWith( - "maxSlotLoss exceeds slots" + "Marketplace_InvalidMaxSlotLoss" ) }) @@ -196,7 +202,7 @@ describe("Marketplace", function () { await token.approve(marketplace.address, price(request) * 2) await marketplace.requestStorage(request) await expect(marketplace.requestStorage(request)).to.be.revertedWith( - "Request already exists" + "Marketplace_RequestAlreadyExists" ) }) }) @@ -224,7 +230,7 @@ describe("Marketplace", function () { it("fails to retrieve a request of an empty slot", async function () { expect(marketplace.getActiveSlot(slotId(slot))).to.be.revertedWith( - "Slot is free" + "Marketplace_SlotIsFree" ) }) @@ -238,21 +244,21 @@ describe("Marketplace", function () { it("is rejected when proof is incorrect", async function () { await expect( marketplace.fillSlot(slot.request, slot.index, invalidProof()) - ).to.be.revertedWith("Invalid proof") + ).to.be.revertedWith("Proofs_InvalidProof") }) it("is rejected when slot already filled", async function () { await marketplace.fillSlot(slot.request, slot.index, proof) await expect( marketplace.fillSlot(slot.request, slot.index, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) it("is rejected when request is unknown", async function () { let unknown = await exampleRequest() await expect( marketplace.fillSlot(requestId(unknown), 0, proof) - ).to.be.revertedWith("Unknown request") + ).to.be.revertedWith("Marketplace_UnknownRequest") }) it("is rejected when request is cancelled", async function () { @@ -264,7 +270,7 @@ describe("Marketplace", function () { switchAccount(host) await expect( marketplace.fillSlot(requestId(expired), slot.index, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) it("is rejected when request is finished", async function () { @@ -272,7 +278,7 @@ describe("Marketplace", function () { await waitUntilFinished(marketplace, slot.request) await expect( marketplace.fillSlot(slot.request, slot.index, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) it("is rejected when request is failed", async function () { @@ -280,14 +286,14 @@ describe("Marketplace", function () { await waitUntilFailed(marketplace, request) await expect( marketplace.fillSlot(slot.request, slot.index, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) it("is rejected when slot index not in range", async function () { const invalid = request.ask.slots await expect( marketplace.fillSlot(slot.request, invalid, proof) - ).to.be.revertedWith("Invalid slot") + ).to.be.revertedWith("Marketplace_InvalidSlot") }) it("fails when all slots are already filled", async function () { @@ -302,7 +308,7 @@ describe("Marketplace", function () { } await expect( marketplace.fillSlot(slot.request, lastSlot, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) }) @@ -436,7 +442,7 @@ describe("Marketplace", function () { slot.index = 5 let nonExistentId = slotId(slot) await expect(marketplace.freeSlot(nonExistentId)).to.be.revertedWith( - "Slot is free" + "Marketplace_SlotIsFree" ) }) @@ -444,7 +450,7 @@ describe("Marketplace", function () { await waitUntilStarted(marketplace, request, proof, token) switchAccount(client) await expect(marketplace.freeSlot(id)).to.be.revertedWith( - "Slot filled by other host" + "Marketplace_InvalidSlotHost" ) }) @@ -513,7 +519,7 @@ describe("Marketplace", function () { await waitUntilFinished(marketplace, requestId(request)) await marketplace.freeSlot(slotId(slot)) await expect(marketplace.freeSlot(slotId(slot))).to.be.revertedWith( - "Already paid" + "Marketplace_AlreadyPaid" ) }) @@ -571,7 +577,7 @@ describe("Marketplace", function () { } await expect( marketplace.fillSlot(slot.request, lastSlot, proof) - ).to.be.revertedWith("Slot is not free") + ).to.be.revertedWith("Marketplace_SlotNotFree") }) }) @@ -587,14 +593,14 @@ describe("Marketplace", function () { it("rejects withdraw when request not yet timed out", async function () { switchAccount(client) await expect(marketplace.withdrawFunds(slot.request)).to.be.revertedWith( - "Request not yet timed out" + "Marketplace_RequestNotYetTimedOut" ) }) it("rejects withdraw when wrong account used", async function () { await waitUntilCancelled(request) await expect(marketplace.withdrawFunds(slot.request)).to.be.revertedWith( - "Invalid client address" + "Marketplace_InvalidClientAddress" ) }) @@ -611,7 +617,7 @@ describe("Marketplace", function () { await waitUntilCancelled(request) switchAccount(client) await expect(marketplace.withdrawFunds(slot.request)).to.be.revertedWith( - "Invalid state" + "Marketplace_InvalidState" ) }) @@ -930,7 +936,7 @@ describe("Marketplace", function () { let missedPeriod = periodOf(await currentTime()) await expect( marketplace.markProofAsMissing(slotId(slot), missedPeriod) - ).to.be.revertedWith("Slot not accepting proofs") + ).to.be.revertedWith("Marketplace_SlotNotAcceptingProofs") }) describe("slashing when missing proofs", function () { diff --git a/test/Proofs.test.js b/test/Proofs.test.js index 7ebe87c..a75f9dc 100644 --- a/test/Proofs.test.js +++ b/test/Proofs.test.js @@ -207,14 +207,14 @@ describe("Proofs", function () { let invalid = exampleProof() await expect( proofs.proofReceived(slotId, invalid, pubSignals) - ).to.be.revertedWith("Invalid proof") + ).to.be.revertedWith("Proofs_InvalidProof") }) it("fails proof submission when public input is incorrect", async function () { let invalid = [1, 2, 3] await expect( proofs.proofReceived(slotId, proof, invalid) - ).to.be.revertedWith("Invalid proof") + ).to.be.revertedWith("Proofs_InvalidProof") }) it("emits an event when proof was submitted", async function () { @@ -228,7 +228,7 @@ describe("Proofs", function () { await proofs.proofReceived(slotId, proof, pubSignals) await expect( proofs.proofReceived(slotId, proof, pubSignals) - ).to.be.revertedWith("Proof already submitted") + ).to.be.revertedWith("Proofs_ProofAlreadySubmitted") }) it("marks a proof as missing", async function () { @@ -246,7 +246,7 @@ describe("Proofs", function () { let currentPeriod = periodOf(await currentTime()) await expect( proofs.markProofAsMissing(slotId, currentPeriod) - ).to.be.revertedWith("Period has not ended yet") + ).to.be.revertedWith("Proofs_PeriodNotEnded") }) it("does not mark a proof as missing after timeout", async function () { @@ -255,7 +255,7 @@ describe("Proofs", function () { await advanceTimeToForNextBlock(periodEnd(currentPeriod) + timeout) await expect( proofs.markProofAsMissing(slotId, currentPeriod) - ).to.be.revertedWith("Validation timed out") + ).to.be.revertedWith("Proofs_ValidationTimedOut") }) it("does not mark a received proof as missing", async function () { @@ -266,7 +266,7 @@ describe("Proofs", function () { await mine() await expect( proofs.markProofAsMissing(slotId, receivedPeriod) - ).to.be.revertedWith("Proof was submitted, not missing") + ).to.be.revertedWith("Proofs_ProofNotMissing") }) it("does not mark proof as missing when not required", async function () { @@ -279,7 +279,7 @@ describe("Proofs", function () { await mine() await expect( proofs.markProofAsMissing(slotId, currentPeriod) - ).to.be.revertedWith("Proof was not required") + ).to.be.revertedWith("Proofs_ProofNotRequired") }) it("does not mark proof as missing twice", async function () { @@ -290,7 +290,7 @@ describe("Proofs", function () { await proofs.markProofAsMissing(slotId, missedPeriod) await expect( proofs.markProofAsMissing(slotId, missedPeriod) - ).to.be.revertedWith("Proof already marked as missing") + ).to.be.revertedWith("Proofs_ProofAlreadyMarkedMissing") }) it("requires no proofs when slot is finished", async function () {