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

Use the returndatasize opcode instead of caching the return size in a variable #453

Closed

Conversation

mmv08
Copy link
Contributor

@mmv08 mmv08 commented Feb 22, 2024

Per @nlordell's suggestion, the returndatasize opcode is a single byte opcode and costs just 2 gas. Its use is more efficient than creating a stack variable or pushing a one-byte constant.

The gas report proves it's more efficient.

@drortirosh
Copy link
Contributor

Thanks for the suggestion.
But please take a look at the gas calculations (the failed "gas-checks" test, or you can run it locally with yarn gas-calc)
your fix costs an extra 36 gas for a single UserOp bundle, and an extra 336 gas for a 10-user bundle.
(I can't say why, because looking at your suggestion, it does seem to save gas, not to waste)

@mmv08
Copy link
Contributor Author

mmv08 commented Feb 22, 2024

Thanks for the suggestion. But please take a look at the gas calculations (the failed "gas-checks" test, or you can run it locally with yarn gas-calc) your fix costs an extra 36 gas for a single UserOp bundle, and an extra 336 gas for a 10-user bundle. (I can't say why, because looking at your suggestion, it does seem to save gas, not to waste)

I did run it locally and pushed the updated calculations as well. For some reason, if I run the CI job locally, it passes. I'm not sure why the CI run reports higher gas.

@mmv08
Copy link
Contributor Author

mmv08 commented Feb 22, 2024

Ok, we found it - the command hasn't recompiled the code, and it was benchmarking the older version.

@mmv08 mmv08 closed this Feb 22, 2024
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.

2 participants