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 AttestationRegistry.sol: Optimized 'For' loops #272

Closed
wants to merge 2 commits into from

Conversation

0xScratch
Copy link
Contributor

What does this PR do?

This PR is solely aimed at reducing gas costs by enhancing the 'for' loops.

Fixes #
Changed the structure of 'for' loops. This will help -> https://gist.github.com/grGred/9bab8b9bad0cd42fc23d4e31e7347144#for-loops-improvement

Also, make a double-check whether that unchecked block doesn't create any unwanted bugs or errors.
However, i++ to ++i might won't be a problem.

Type of change

  • Chore
  • Bug fix
  • New feature
  • Documentation update

Check list

  • Unit tests for any smart contract change
  • Contracts and functions are documented

@codecov-commenter
Copy link

Codecov Report

Merging #272 (023a9fb) into dev (c9283aa) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #272      +/-   ##
==========================================
+ Coverage   93.40%   93.53%   +0.13%     
==========================================
  Files          13       13              
  Lines         197      201       +4     
  Branches       46       46              
==========================================
+ Hits          184      188       +4     
  Misses          8        8              
  Partials        5        5              
Files Coverage Δ
contracts/src/AttestationRegistry.sol 95.00% <100.00%> (+0.35%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@alainncls
Copy link
Collaborator

Hey @Aryan9592 👋
Thanks for your contribution to Verax (once again)!

I was running some comparative tests, and if I'm not mistaken, when running this new version of the massImport method, the difference is 136 gas.
I feel like this is not a huge change for a degraded code readability, what do you think?

In any case, feel free to join us on Discord if you want to learn more about Verax or wish to contribute!

@0xScratch
Copy link
Contributor Author

0xScratch commented Oct 12, 2023

Actually, solidity introduced these unchecked blocks right after the 0.8 version, and these types of changes are being used in various repositories and are now familiar to many devs out there. Still, if you find them not that good as compared to the readability factor, we can remove that unchecked block inside the massImport function. Won't be a big deal to revert some changes back. and changes like uint256 i = 0; -> uint256 i; and i++ -> ++i are quite common..
Talking about the gas difference, when these changes sum up they become really valuable sometimes..Maybe I be digging even more into this codebase

@0xScratch
Copy link
Contributor Author

In any case, feel free to join us on Discord if you want to learn more about Verax or wish to contribute!

Send me the invite!

@orbmis
Copy link
Collaborator

orbmis commented Oct 12, 2023

@alainncls
Copy link
Collaborator

Hey @Aryan9592 I didn't forget about this PR!
Instead of introducing unchecked blocks in the contracts, I created an external uncheckedInc function to call. in the loops.

The corresponding PR is there if you're interested: #350
For example, when running the massImport function with 100 payloads, we decrease the gas cost by 0.04%.
I'm closing this PR in favor of #350 if that's OK with you.

@alainncls alainncls closed this Nov 2, 2023
@0xScratch
Copy link
Contributor Author

Yep that's great @alainncls

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.

4 participants