-
Notifications
You must be signed in to change notification settings - Fork 672
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
Update EntryPoint.sol: Optimized Gas #345
Conversation
Did you run any gas checks to see that it's indeed saving gas? |
Oh yeah it obviously saves gas.. I run some kind of tests earlier on remix, like creating some random function and checking whether this thing real works.. And yea that did And yea, it doesn't matters whatever contract it is, these changes works everywhere! You might find this helpful -> https://gist.github.com/grGred/9bab8b9bad0cd42fc23d4e31e7347144#for-loops-improvement |
@shahafn Hope this makes us more sure about it This one is without any changes, like what most contracts still nail at this..and some even don't use Required changes made here, Results proves the show! Sorry, But I was kind of a bit lazy to have a gas-check for real contract, and need up to setup the foundry thing for it...But yea I am pretty sure these changes, works for every contract. Thanks! |
You don't need to setup foundry for gas checks. just run The CI also tests the gas report, so you can see the effect of code changes in the git history.. |
Looks like @drortirosh need to really make me understand what's going on..like you said, I cloned the repository in my local machine, ran the command |
you should either have docker and docker-compose installed on your machine.
if you don't have docker installed either, you can run geth locally on your machine, with the above parameters.. the "gas-calc" script will use docker-compose only if it doesn't find geth running locally on port 8545. |
@drortirosh did exactly what you said..installed docker desktop and ran the provided code after running geth Really don't know what's going on here, Can't you run the gas-checks? |
Did you manage to run other tests ? |
Which other tests? Maybe you can use this pull request as a reference -> allo-protocol/allo-v2#288 |
You know, |
But there's a catch, I guess you are thinking that I made changes in this repository after cloning it down in my PC, but that isn't the case. For making these changes, I really don't need any cloning part to do cuz I know they don't make any errors. Just forked down this repository and made changes online..That's it. |
Moreover both
|
ok, great, so the yarn system works. I just can't understand why "yarn gas-calc" doesn't as it is built as just another "mocha test". |
hmm what next? |
I checked out your code, and ran the test.
explanation:
bottom line: This is not an optimization at all. +/- 30 gas is observable on irrelevant minor changes such as adding/removing external functions (since they modify the "jump table") Thank you for the effort. |
will look at it! Can make some more changes in two more files..Should I? |
you may suggest more optimization. but please find a way to run the gas checks too (e.g. by starting a docker or cloud image, or find how to make it run on Windows) |
Looks like I got something useful here!
|
please don't copy the gas report text into the comment - add it as a commit |
Actually, that was just to show you. Moreover, it was just a test run for the original codebase, it doesn't include any of my changes. |
|
You need three things for that, docker desktop installed, command prompt and git bash
|
the overall I see from the commited gas results is that it causes slight increase in gas - insignificant (10s of gas on 10 userops) - but of course, there's no reason to include. |
Got it! Just went through Kind of wondering why is that, cuz this requirement statement stresses the visibility of this function as |
We have to open a separate context. There is no way in solidity (or rather evm) to revert 2 method calls together, unless they are in an "external call" |
Re: docker
|
Nope, in my case "yarn gas-calc" just checks whether the geth is running or not. If not, it doesn't try to run any docker for geth itself..just shows an error msg |
Got it! |
1st change ->
uint256 i = 0;
touint256 i;
: Usually default values of all unsigned integers are 0, thus it's kind of a waste of gas when we initialize it with 0 itself2nd change ->
i++
to++i
: '++i' gonna be more helpful here and costs less gas, But it can mess up with code's logic if not used carefully. Although, I don't find any issues with it in this solidity file but do make a check related to this changeUse of
unchecked
is well appreciated, otherwise that would be my third change..Thanks!