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

Update FeeBankCharger.sol: Refactoring the Modifier #123

Closed
wants to merge 1 commit into from

Conversation

0xScratch
Copy link

Well, this PR is solely based on optimizing some gas by refactoring the modifier to save bytecode size and development cost..

Let's see how
Usually, modifier code is copied in all instances where it is used, thus increasing the bytecode size. By doing a refactor like this and introducing the internal function we can reduce the bytecode size at the cost of one JUMP opcode. Thus, this refactoring might only be used in the scenarios when our modifier is being used more than one time!!

Moreover, I kind of tested this refactor technique and the results were quite appealing, Here they are:

First I created a contract that is same as the one we use commonly (without using any internal function for the modifiers)
and this is the result I get:

Screenshot (20)

quite a basic contract, Ain't it? And then I refactored that modifier with the help of an internal function and gas usage dropped, but with a catch i.e gas usage of each function increased quite a bit (see at the right side of those functions and compare them to the previous one)

Screenshot (21)

That's it. If you found it not worth a change then please let me know. It will be a nice learning process for me then!! 😄

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
contracts/FeeBankCharger.sol 100.00%

📢 Thoughts on this report? Let us know!.

@ZumZoom
Copy link
Member

ZumZoom commented Aug 30, 2023

Summary:

It saves 800-900 gas on Settlement deploy, however execution cost for FeeBank::deposit* and FeeBank::withdraw* increases by 19. We mostly favour execution gas savings, so we'll leave the code as it is.

@0xScratch
Copy link
Author

Yea, that makes sense. Thanks!

@0xScratch 0xScratch closed this Aug 30, 2023
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