Gas Optimizations #47
Comments
|
Okay this is possibly the best gas report I have ever seen. Huge props to Dravee! |
|
Implementing a whole lot of this in a new PR, here are some notes:
Will write more, I'm currently at the unchecked increment section, which is valid. Anyway, C4 give this gigachad a medal. |
|
Included unchecked increments, and cached array lengths (where possible) too! Unchecked increments: lens-protocol/core@37ab8ce The SHR sacrifices readability too much, though it's still valid. Lastly the custom errors I would say are not valid since that's just how ERC721 is built, we don't want to stray away from the standard, even in revert messages as much as possible. Overall this is a fantastic report! |
|
PSA: The happy path short-circuiting is only partly valid in that it saves a minor amount of gas (2-3 opcodes) since even the "sad path" is evaluated lazily, if the first condition evaluates to false, the second condition is not evaluated. |
Gas Report
Table of Contents:
calldatainstead ofmemoryforstring contentURIcalldatainstead ofmemoryforbytes collectModuleDatacalldatainstead ofmemoryforbytes referenceModuleDatacalldatainstead ofmemoryforstring followNFTURIcalldatainstead ofmemorycalldatainstead ofmemoryForeword
@audittagsFile: FollowNFT.sol
function getPowerByBlockNumber()
Unchecked block
As it's impossible for line 121 to underflow (see condition line 116), it should be wrapped inside an
uncheckedblock.function getDelegatedSupplyByBlockNumber()
Unchecked block
As it's impossible for line 163 to underflow (see condition line 158), it should be wrapped inside an
uncheckedblock.File: LensHub.sol
modifier onlyWhitelistedProfileCreator() {
A modifier used only once can get inline to save gas
The
modifier onlyWhitelistedProfileCreatoris used only once:Therefore, it can get inlined in
createProfileto save gasfunction setState()
Short-circuiting can provide a happy path
The current implementation of function
setStatefavors a sad path which will always cost 2 SLOADs to check the condition.It's possible to short-circuit this condition to provide a happy path which may cost only 1 SLOAD instead.
I suggest rewriting the function to:
Another alternative:
function getProfileIdByHandle()
A variable used only once shouldn't get cached
I suggest inlining
handleHashL796.function _createPost()
Use
calldatainstead ofmemoryforstring contentURIUse
calldatainstead ofmemoryforbytes collectModuleDataUse
calldatainstead ofmemoryforbytes referenceModuleDataFor the 3
memoryvariables declared in_createPost(), the parent functions are actually passing acalldatavariable:Therefore, the function declaration should use
calldatainstead ofmemoryfor these 3 parameters.This optimization is similar to the one for the
internalfunction_createMirrorinLensNFTBase.solwhich usesbytes calldata referenceModuleData:function _setFollowNFTURI()
Use
calldatainstead ofmemoryforstring followNFTURIThe parent functions are actually passing a
calldatavariable:Therefore, the function declaration should use
calldatainstead ofmemoryforstring followNFTURIThis optimization is similar to the one for the
internalfunction_createMirrorinLensNFTBase.solwhich usesbytes calldata referenceModuleData.function _validateCallerIsProfileOwnerOrDispatcher()
Short-circuiting can provide a happy path
The current implementation of function
_validateCallerIsProfileOwnerOrDispatcherfavors a sad path which will always cost 2 SLOADs to check the condition.It's possible to short-circuit this condition to provide a happy path which may cost only 1 SLOAD instead.
I suggest rewriting the function to:
File: LensNFTBase.sol
function _validateRecoveredAddress()
Use
calldatainstead ofmemoryThe calls to the
internalfunction_validateRecoveredAddresspass acalldatavariable:Therefore, the function declaration should use
calldatainstead ofmemoryThis optimization is similar to the one for the
internalfunction_createMirrorinLensNFTBase.solwhich usesbytes calldata referenceModuleData.File: PublishingLogic.sol
function _initFollowModule()
Use
calldatainstead ofmemoryThe calls to the
privatefunction_initFollowModulepass acalldatavariable:Therefore, the function declaration should use
calldatainstead ofmemoryThis optimization is similar to the one for the
internalfunction_createMirrorinLensNFTBase.solwhich usesbytes calldata referenceModuleData.General recommendations
Variables
No need to explicitly initialize variables with default values
If a variable is not set/initialized, it is assumed to have the default value (
0foruint,falseforbool,address(0)for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.As an example:
for (uint256 i = 0; i < numIterations; ++i) {should be replaced withfor (uint256 i; i < numIterations; ++i) {Instances include:
I suggest removing explicit initializations for default values.
Pre-increments cost less gas compared to post-increments
As the solution employs pre-increments for all of its for-loops, I'm sure the sponsor is aware of the fact that pre-increments cost less gas compared to post-increments (about 5 gas)
However, some places outside loops were missed.
Instances include:
I suggest only replacing these, as some other places in the solution are actually using post-increments the right way. The logic would break if those other places (not mentioned here) are changed too.
For-Loops
Increments can be unchecked
In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
ethereum/solidity#10695
Instances include:
The code would go from:
to:
The risk of overflow is inexistant for a
uint256here.An array's length should be cached to save gas in for-loops
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:
Arithmetics
Shift Right instead of Dividing by 2
A division by 2 can be calculated by shifting one to the right.
While the
DIVopcode uses 5 gas, theSHRopcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.I suggest replacing
/ 2with>> 1here:Errors
Use Custom Errors instead of Revert Strings to save Gas
I'm quite certain that the sponsor is aware of this optimization, as the library
Errorscontains a lot of custom errors. Therefore, I won't explain it (suffice to say, they are cheaper than require statements + revert strings).The finding here is that the contracts
ERC721EnumerableandERC721Timedon't benefit from this practice:I suggest using custom errors in
ERC721EnumerableandERC721Time.The text was updated successfully, but these errors were encountered: