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.

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

Important adjustments to the batch function #1124

Closed
3 tasks done
PaulRBerg opened this issue Dec 21, 2024 · 0 comments
Closed
3 tasks done

Important adjustments to the batch function #1124

PaulRBerg opened this issue Dec 21, 2024 · 0 comments
Assignees
Labels
effort: medium Default level of effort. priority: 1 This is important. It should be dealt with shortly. type: feature New feature or request. type: refactor Change that neither fixes a bug nor adds a feature. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.

Comments

@PaulRBerg
Copy link
Member

PaulRBerg commented Dec 21, 2024

Context

I've recently tweeted about our batch function, and it turned out to be a productive exercise, as we have received a lot of great feedback in the replies.

Changes

I've parsed through all replies, and I would like us to make the following changes to batch:

  • Bubble up the revert (for better specificity when debugging errors, and not needing a Tenderly account to debug batch-related reverts in Sablier)
  • Explain in the NatSpec comments that it is NOT safe to use msg.value; quote this article
  • Return results in an array

Rationale

Why return the results?

  1. They provide critical information for onchain integrators. Imagine needing to know the stream IDs that have just been created. It's impossible to do that now when batch is used.
  2. The additional gas cost isn't much.

I've made a gas comparison:

So it's only 4422 extra gas for creating two streams, i.e., extra ~2.2k gas per created streams. Small price to pay.

Resources

Check out PRBProxy for inspiration on how to bubble up the revert, as well as this StackExchange Q&A.

Note: unlike in PRBProxy, let's NOT revert with a custom error when the revert response is empty. Let's just bubble it up empty, as is.

@PaulRBerg PaulRBerg added effort: medium Default level of effort. priority: 1 This is important. It should be dealt with shortly. type: feature New feature or request. type: refactor Change that neither fixes a bug nor adds a feature. work: clear Sense-categorize-respond. The relationship between cause and effect is clear. labels Dec 21, 2024
@PaulRBerg PaulRBerg marked this as a duplicate and then as not a duplicate of #1122 Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. priority: 1 This is important. It should be dealt with shortly. type: feature New feature or request. type: refactor Change that neither fixes a bug nor adds a feature. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.
Projects
None yet
Development

No branches or pull requests

3 participants