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

Add gas benchmarks for BatchLockup #359

Merged
merged 4 commits into from
Jun 29, 2024
Merged

Add gas benchmarks for BatchLockup #359

merged 4 commits into from
Jun 29, 2024

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Jun 26, 2024

Changelog

Question: Do we also want benchmark tables for MerkleLockupFactory and MerkleLL/LT contracts? IMO that should not be necessary since Airstream campaign creators are usually the companies which have very few incentive to care about transaction cost.

@smol-ninja smol-ninja marked this pull request as draft June 26, 2024 19:37
@smol-ninja smol-ninja marked this pull request as ready for review June 27, 2024 12:01
test: misc enhancements
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

Good job!

Reminder to update Ward Analytics (see Telegram) about this once we merge this PR.

Do we also want benchmark tables for MerkleLockupFactory and MerkleLL/LT contracts?

No, as per your rationale.

benchmark/BatchLockup.Gas.t.sol Outdated Show resolved Hide resolved
benchmark/results/SablierV2BatchLockup.md Outdated Show resolved Hide resolved
@smol-ninja
Copy link
Member Author

@andreivladbrg can you please review and approve it?

@andreivladbrg
Copy link
Member

andreivladbrg commented Jun 28, 2024

Before giving a full review, I have a question.

Adds gas benchmarks for batchsize: [5, 10, 20, 30, 50] and segments/tranches: [24, 24, 24, 24, 12] respectively

I guess the reason for adding 12 count for a batch of 50 size (instead of 24) is because it's close to the block gas limit. But what's the reason for not using the 12 everywhere instead?

Also, would it be useful if we add more counts for each batch size? e.g. 2,12,24 etc

@smol-ninja
Copy link
Member Author

I guess the reason for adding 12 count for a batch of 50 size (instead of 24) is because it's close to the block gas limit. But what's the reason for not using the 12 everywhere instead?
Also, would it be useful if we add more counts for each batch size? e.g. 2,12,24 etc

Please see the discussion here.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Should we also add a benchmark test to see the size for the value close to 20 million gas for the linear batch function? The current gas usage for a size of 50 is far from exceeding the limit.

@smol-ninja
Copy link
Member Author

Should we also add a benchmark test to see the size for the value close to 20 million gas for the linear batch function? The current gas usage for a size of 50 is far from exceeding the limit.

The batch size depends on number of segments/tranches as well. So for what values of segments/tranches, would we do it for?

@andreivladbrg
Copy link
Member

The batch size depends on number of segments/tranches as well. So for what values of segments/tranches, would we do it for?

I am talking about LockupLinear, not Dynamic or Tranched

@smol-ninja
Copy link
Member Author

smol-ninja commented Jun 28, 2024

For LockupLinear, the gas consumption should increase linearly with the batch size. So based on (50, 6.87M), it should be $50*(30M/6.87M) = 218$. I ran the test, and its indeed ~218.

| `createWithDurationsLL`  | Lockup Linear   | N/A               | 218        | 30,037,705  |
| `createWithTimestampsLL` | Lockup Linear   | N/A               | 218        | 30,050,659  |

I can add another test contract to generate this but do we need it? If you want to add to this table, then thats very easy. But if you want to write a script similar to the calculation of MAX_SEGMENT_COUNT in v2-core, then it is a bit of work.

PS: we can also add this ~218 number in the Sablier docs of BatchLockup.

PS: my opinion is there is no need to know the value for max batch size in LL as it can easily be calculated from the data in the gas table we are providing.

@smol-ninja smol-ninja merged commit 37164ae into staging Jun 29, 2024
7 checks passed
@smol-ninja smol-ninja deleted the test/benchmarks branch June 29, 2024 10:38
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