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

Modify ReentrancyGuard to reduce contract size #3515

Merged
merged 6 commits into from Jul 11, 2022

Conversation

abhi3700
Copy link
Contributor

@abhi3700 abhi3700 commented Jun 29, 2022

Refer issue #3514

I wrapped the code present in function modifier with 2 functions (private) outside & just call the function(s) inside the modifier.

@Amxx
Copy link
Collaborator

Amxx commented Jun 29, 2022

Hello @abhi3700 and thank you for this PR.

I think we would want better naming. Maybe something like _nonReentrantBefore, _nonReentrantAfter ?

@abhi3700
Copy link
Contributor Author

abhi3700 commented Jul 1, 2022

Yeah great!

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

@abhi3700 Can you implement the changes suggested by @Amxx?

@frangio frangio changed the title Modify ReentrancyGuard.sol to reduce the contract size by 0.028 KB per call Modify ReentrancyGuard to reduce contract size Jul 4, 2022
@frangio
Copy link
Contributor

frangio commented Jul 4, 2022

Did you measure the additional runtime cost?

@abhi3700
Copy link
Contributor Author

abhi3700 commented Jul 6, 2022

@abhi3700 Can you implement the changes suggested by @Amxx?

Done! ✅

@abhi3700
Copy link
Contributor Author

abhi3700 commented Jul 6, 2022

Did you measure the additional runtime cost?

In my repo,
the compile result with standard "ReentrancyGuard" is 9.72s
image

And the compile result with standard "ReentrancyGuard" is 9.29s
image

The compile time differs because of the machine running concurrent applications at this time.

So, it can be seen that the contract size is highly reduced by 0.308 KB

@abhi3700 abhi3700 requested a review from frangio July 11, 2022 05:05
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks!

@frangio frangio enabled auto-merge (squash) July 11, 2022 20:53
@frangio frangio merged commit 3210a86 into OpenZeppelin:master Jul 11, 2022
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
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.

None yet

3 participants