Skip to content

BM-462: Drop requirePayment #370

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.

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

Merged
merged 13 commits into from
Mar 8, 2025

Conversation

capossele
Copy link
Contributor

@capossele capossele commented Mar 7, 2025

Here we drop the requirePayment bool. Valid fulfillments should never revert, but only return an error message (or an array of errors if batching).
As for simulating whether a prover will get paid or not would be sufficient to just call the transaction and check if returns any error.

Copy link

linear bot commented Mar 7, 2025

@github-actions github-actions bot changed the title Drop requirePayment BM-462: Drop requirePayment Mar 7, 2025
Copy link

github-actions bot commented Mar 7, 2025

🚀 Documentation Preview

Deployment URL: https://boundless-documentation-iufh5k5p8-risczero.vercel.app

Updated at: 2025-03-07 00:52:13 UTC

} else {
emit PaymentRequirementsFailed(paymentError);
}
revertWith(paymentError);
Copy link
Member

@nategraf nategraf Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this was not explained well in the issue. We actually want to always apply requirePayment = false rather than requirePayment = true 🙈. The idea is that someone can always wrap this contract to add the behavior of reverting on payment failure, but the opposite is not true.

Suggested change
revertWith(paymentError);
emit PaymentRequirementsFailed(paymentError);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you simulate a call you only have read permissions so any event would not be available. As such I dropped the event and instead returned an error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad (I wrote the issue) Sorry this wasn't clear!

Comment on lines 476 to 472
return abi.encodeWithSelector(RequestIsLocked.selector, RequestId.unwrap(id));
return "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment above, we want to leave this line as is.

@willpote willpote merged commit 44606f8 into main Mar 8, 2025
16 checks passed
@willpote willpote deleted the angelo/bm-462-removing-requirepayment-from-fulfillment branch March 8, 2025 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants