Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review of #335: simplify fulfillAndPay by extracting callback #364

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Review of #335: simplify fulfillAndPay by extracting callback #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ae0f70
simplify fulfillAndPay function by extracting callbacks
nategraf Mar 5, 2025
944a8e6
update gas snapshots
nategraf Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions contracts/snapshots/BoundlessMarketBasicTest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
{
"ERC20 approve: required for depositStake": "45966",
"bytecode size implementation": "23314",
"bytecode size implementation": "22914",
"bytecode size proxy": "89",
"deposit: first ever deposit": "50823",
"deposit: second deposit": "33723",
"depositStake: 1 HP (tops up market account)": "60150",
"depositStake: full (drains testProver account)": "50550",
"depositStakeWithPermit: 1 HP (tops up market account)": "72964",
"depositStakeWithPermit: full (drains testProver account)": "72954",
"fulfill: a locked request": "79498",
"fulfill: a locked request (locked via prover signature)": "79498",
"fulfill: another prover fulfills without payment": "74397",
"fulfill: fulfilled by the locked prover for payment (request already fulfilled by another prover)": "74826",
"fulfillBatch: a batch of 8": "323176",
"fulfill: a locked request": "79478",
"fulfill: a locked request (locked via prover signature)": "79478",
"fulfill: another prover fulfills without payment": "74377",
"fulfill: fulfilled by the locked prover for payment (request already fulfilled by another prover)": "74806",
"fulfillBatch: a batch of 8": "322906",
"lockinRequest: base case": "117183",
"lockinRequest: with prover signature": "122873",
"priceAndFulfillBatch: a single request": "107140",
"priceAndFulfillBatch: a single request (with selector)": "109264",
"priceAndFulfillBatch: a single request that was not locked": "107128",
"priceAndFulfillBatch: a single request that was not locked fulfilled by prover not in allow-list": "107128",
"priceAndFulfillBatch: a single request": "107094",
"priceAndFulfillBatch: a single request (with selector)": "109218",
"priceAndFulfillBatch: a single request that was not locked": "107082",
"priceAndFulfillBatch: a single request that was not locked fulfilled by prover not in allow-list": "107082",
"slash: base case": "87250",
"slash: fulfilled request after lock deadline": "61891",
"submitRequest: with maxPrice ether": "51990",
"submitRequest: without ether": "45105",
"submitRootAndFulfillBatch: a batch of 2 requests": "149603",
"submitRootAndFulfillBatch: a batch of 2 requests": "149525",
"unfreezeAccount": "31578",
"withdraw: 1 ether": "40539",
"withdraw: full balance": "40551",
Expand Down
40 changes: 20 additions & 20 deletions contracts/snapshots/BoundlessMarketBench.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
{
"fulfillBatch (with callback): batch of 001": "123654",
"fulfillBatch (with callback): batch of 002": "201437",
"fulfillBatch (with callback): batch of 004": "357322",
"fulfillBatch (with callback): batch of 008": "668169",
"fulfillBatch (with callback): batch of 016": "1123526",
"fulfillBatch (with callback): batch of 032": "2064560",
"fulfillBatch (with selector): batch of 001": "83164",
"fulfillBatch (with selector): batch of 002": "120530",
"fulfillBatch (with selector): batch of 004": "197125",
"fulfillBatch (with selector): batch of 008": "339836",
"fulfillBatch (with selector): batch of 016": "624747",
"fulfillBatch (with selector): batch of 032": "1214402",
"fulfillBatch: batch of 001": "81068",
"fulfillBatch: batch of 002": "116335",
"fulfillBatch: batch of 004": "188726",
"fulfillBatch: batch of 008": "323104",
"fulfillBatch: batch of 016": "590875",
"fulfillBatch: batch of 032": "1145686",
"fulfillBatch: batch of 064": "2300625",
"fulfillBatch: batch of 128": "4721343"
"fulfillBatch (with callback): batch of 001": "123480",
"fulfillBatch (with callback): batch of 002": "201100",
"fulfillBatch (with callback): batch of 004": "356660",
"fulfillBatch (with callback): batch of 008": "666859",
"fulfillBatch (with callback): batch of 016": "1120918",
"fulfillBatch (with callback): batch of 032": "2059356",
"fulfillBatch (with selector): batch of 001": "83118",
"fulfillBatch (with selector): batch of 002": "120452",
"fulfillBatch (with selector): batch of 004": "196983",
"fulfillBatch (with selector): batch of 008": "339566",
"fulfillBatch (with selector): batch of 016": "624221",
"fulfillBatch (with selector): batch of 032": "1213364",
"fulfillBatch: batch of 001": "81022",
"fulfillBatch: batch of 002": "116257",
"fulfillBatch: batch of 004": "188584",
"fulfillBatch: batch of 008": "322834",
"fulfillBatch: batch of 016": "590349",
"fulfillBatch: batch of 032": "1144648",
"fulfillBatch: batch of 064": "2298563",
"fulfillBatch: batch of 128": "4717233"
}
62 changes: 29 additions & 33 deletions contracts/src/BoundlessMarket.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,48 +327,52 @@ contract BoundlessMarket is
function fulfill(Fulfillment calldata fill, AssessorReceipt calldata assessorReceipt) public {
verifyDelivery(fill, assessorReceipt);

// Execute the callback with the associated fulfillment information.
// Callbacks are called exactly once, on the first fulfillment. Checking that the request is
// not fulfilled at this point ensures this. Note that by the end of the transaction, the
// fulfilled flag for the provided fulfillment will be set, or this transaction will
// revert (and revery any effects from the callback along with it).
if (assessorReceipt.callbacks.length > 0) {
AssessorCallback memory callback = assessorReceipt.callbacks[0];
_fulfillAndPay(fill, assessorReceipt.prover, callback.addr, callback.gasLimit);
} else {
_fulfillAndPay(fill, assessorReceipt.prover, address(0), 0);
if (!requestIsFulfilled(fill.id)) {
_executeCallback(fill.id, callback.addr, callback.gasLimit, fill.imageId, fill.journal, fill.seal);
}
}

_fulfillAndPay(fill, assessorReceipt.prover);

emit ProofDelivered(fill.id, fill.journal, fill.seal);
}

/// @inheritdoc IBoundlessMarket
function fulfillBatch(Fulfillment[] calldata fills, AssessorReceipt calldata assessorReceipt) public {
verifyBatchDelivery(fills, assessorReceipt);

// Execute the callback with the associated fulfillment information.
// Callbacks are called exactly once, on the first fulfillment. Checking that the request is
// not fulfilled at this point ensures this. Note that by the end of the transaction, the
// fulfilled flag for every provided fulfillment will be set, or this transaction will
// revert (and revery any effects from the callbacks along with it).
uint256 callbacksLength = assessorReceipt.callbacks.length;
for (uint256 i = 0; i < callbacksLength; i++) {
AssessorCallback memory callback = assessorReceipt.callbacks[i];
Fulfillment calldata fill = fills[callback.index];
if (!requestIsFulfilled(fill.id)) {
_executeCallback(fill.id, callback.addr, callback.gasLimit, fill.imageId, fill.journal, fill.seal);
}
}

// NOTE: It would be slightly more efficient to keep balances and request flags in memory until a single
// batch update to storage. However, updating the same storage slot twice only costs 100 gas, so
// this savings is marginal, and will be outweighed by complicated memory management if not careful.
uint256 callbacksLength = assessorReceipt.callbacks.length;
if (callbacksLength > 0) {
uint256 callbackIdx = 0;
for (uint256 i = 0; i < fills.length; i++) {
if (callbackIdx < callbacksLength && assessorReceipt.callbacks[callbackIdx].index == i) {
AssessorCallback memory callback = assessorReceipt.callbacks[callbackIdx];
_fulfillAndPay(fills[i], assessorReceipt.prover, callback.addr, callback.gasLimit);
callbackIdx++;
} else {
_fulfillAndPay(fills[i], assessorReceipt.prover, address(0), 0);
}
emit ProofDelivered(fills[i].id, fills[i].journal, fills[i].seal);
}
} else {
for (uint256 i = 0; i < fills.length; i++) {
_fulfillAndPay(fills[i], assessorReceipt.prover, address(0), 0);
emit ProofDelivered(fills[i].id, fills[i].journal, fills[i].seal);
}
for (uint256 i = 0; i < fills.length; i++) {
_fulfillAndPay(fills[i], assessorReceipt.prover);
emit ProofDelivered(fills[i].id, fills[i].journal, fills[i].seal);
}
}

/// Complete the fulfillment logic after having verified the app and assessor receipts.
function _fulfillAndPay(Fulfillment calldata fill, address prover, address callback, uint96 callbackGasLimit)
internal
{
function _fulfillAndPay(Fulfillment calldata fill, address prover) internal {
RequestId id = fill.id;
(address client, uint32 idx) = id.clientAndIndex();
Account storage clientAccount = accounts[client];
Expand All @@ -386,14 +390,6 @@ contract BoundlessMarket is
paymentError = _fulfillAndPayNeverLocked(id, client, idx, fill.requestDigest, fulfilled, prover);
}

// Callbacks are only executed the first time a request is marked as fulfilled.
if (callback != address(0) && !fulfilled) {
(, bool fulfilledAfter) = clientAccount.requestFlags(idx);
if (fulfilledAfter) {
_executeCallback(id, callback, callbackGasLimit, fill.imageId, fill.journal, fill.seal);
}
}

if (paymentError.length > 0) {
if (fill.requirePayment) {
revertWith(paymentError);
Expand Down Expand Up @@ -726,7 +722,7 @@ contract BoundlessMarket is
}

/// @inheritdoc IBoundlessMarket
function requestIsFulfilled(RequestId id) external view returns (bool) {
function requestIsFulfilled(RequestId id) public view returns (bool) {
(address client, uint32 idx) = id.clientAndIndex();
(, bool fulfilled) = accounts[client].requestFlags(idx);
return fulfilled;
Expand Down
24 changes: 12 additions & 12 deletions contracts/test/BoundlessMarket.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1966,10 +1966,10 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
(Fulfillment memory fill, AssessorReceipt memory assessorReceipt) =
createFillAndSubmitRoot(request, APP_JOURNAL, address(testProver));

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit MockCallback.MockCallbackCalled(request.requirements.imageId, APP_JOURNAL, fill.seal);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, false);
emit IBoundlessMarket.ProofDelivered(request.id, APP_JOURNAL, fill.seal);
boundlessMarket.fulfill(fill, assessorReceipt);
Expand Down Expand Up @@ -2002,10 +2002,10 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
(Fulfillment memory fill, AssessorReceipt memory assessorReceipt) =
createFillAndSubmitRoot(request, APP_JOURNAL, address(testProver));

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.CallbackFailed(request.id, address(mockHighGasCallback), "");
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, false);
emit IBoundlessMarket.ProofDelivered(request.id, APP_JOURNAL, fill.seal);
boundlessMarket.fulfill(fill, assessorReceipt);
Expand Down Expand Up @@ -2039,11 +2039,11 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
createFillAndSubmitRoot(request, APP_JOURNAL, otherProverAddress);
fill.requirePayment = false;

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit MockCallback.MockCallbackCalled(request.requirements.imageId, APP_JOURNAL, fill.seal);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.PaymentRequirementsFailed(
abi.encodeWithSelector(IBoundlessMarket.RequestIsLocked.selector, request.id)
);
Expand Down Expand Up @@ -2082,11 +2082,11 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
createFillAndSubmitRoot(request, APP_JOURNAL, otherProverAddress);
fill.requirePayment = false;

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit MockCallback.MockCallbackCalled(request.requirements.imageId, APP_JOURNAL, fill.seal);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.PaymentRequirementsFailed(
abi.encodeWithSelector(IBoundlessMarket.RequestIsLocked.selector, request.id)
);
Expand Down Expand Up @@ -2149,10 +2149,10 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
(Fulfillment memory fill, AssessorReceipt memory assessorReceipt) =
createFillAndSubmitRoot(request, APP_JOURNAL, address(otherProver));

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, true);
emit MockCallback.MockCallbackCalled(request.requirements.imageId, APP_JOURNAL, fill.seal);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(request.id);
vm.expectEmit(true, true, true, false);
emit IBoundlessMarket.ProofDelivered(request.id, APP_JOURNAL, fill.seal);
boundlessMarket.priceAndFulfill(request, clientSignature, fill, assessorReceipt);
Expand Down Expand Up @@ -2213,10 +2213,10 @@ contract BoundlessMarketBasicTest is BoundlessMarketTest {
(Fulfillment memory fill, AssessorReceipt memory assessorReceipt) =
createFillAndSubmitRoot(requestB, APP_JOURNAL, address(testProver));

vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(requestB.id);
vm.expectEmit(true, true, true, true);
emit MockCallback.MockCallbackCalled(requestB.requirements.imageId, APP_JOURNAL, fill.seal);
vm.expectEmit(true, true, true, true);
emit IBoundlessMarket.RequestFulfilled(requestB.id);
vm.expectEmit(true, true, true, false);
emit IBoundlessMarket.ProofDelivered(requestB.id, APP_JOURNAL, fill.seal);
boundlessMarket.priceAndFulfill(requestB, clientSignatureB, fill, assessorReceipt);
Expand Down