nomoixyz / vulcan

Development framework for Foundry projects
https://nomoixyz.github.io/vulcan/
MIT License
286 stars 18 forks source link

Update Request.sol: Optimized 'for' loops #177

Closed 0xScratch closed 1 year ago

0xScratch commented 1 year ago

Change made in this PR is -> uint256 i = 0; to uint256 i;

Usually default value of any uint is 0, so it is kind of wastage of some gas in initializing some 'uint' variable to 0

Well, two more changes can be made in these for loops, which will really optimize the gas usage in this smart contract:

  1. Let's take a look at this code snippet:

    for (uint256 i; i < len; i++) {
    req.headers[i] = req.headers[i];
    }

    Here, as far I am concerned, 'len' will never be part of any 'integer overflow', and 'i++' used to perform two operations in this code snippet which are -> first it iterates the value of i, and also it checks whether i get 'overflowed' or not (and this check do costs some gas) So it is kind of unfair when we know that this 'i' will never ever get overflowed because it will always be less than 'len' (which is always limited to a value less than 2**256 - 1) Thus it be better, if we change this snippet to:

    for (uint256 i; i < len;) {
    req.headers[i] = req.headers[i];
    unchecked{
        i++;
    }
    }
  2. Another change that can be made in this PR is: changing i++ to ++i, Actually '++i' takes costs less gas as compared to 'i++', and this change can easily be implemented here, if it doesn't mess up with the code's logic

After making these two changes, we can get our for loop as:

for (uint256 i; i < len;) {
    req.headers[i] = req.headers[i];
    unchecked{
        ++i;
    }
}

I haven't included these two changes in this PR, just need some approvals if you find these changes worth it..

Thanks!

gnkz commented 1 year ago

Hey @Aryan9592 . Thanks for contributing to Vulcan. Since Vulcan is meant to be used for development purposes I feel ok adding the other optimizations. Will wait for @vdrg opinion on this

0xScratch commented 1 year ago

Sure! 👍

vdrg commented 1 year ago

Hi @Aryan9592 thanks for checking out and contributing to Vulcan!

We can merge this PR, but in general we don't want to do micro optimizations like those unchecked blocks you mention, as Vulcan is not meant to be used on-chain (so gas is not a concern) and those changes can decrease readability. We are aware of these optimizations and if there are major optimizations we can consider it, but in most cases it doesn't make sense to include them.

Thanks!

0xScratch commented 1 year ago

I do agree with that. Thanks @vdrg