-
Notifications
You must be signed in to change notification settings - Fork 33
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
refactor: sloads and increments in assembly for gas optimization. #179 #180
Conversation
Hey @0xneves ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting the following changes:
contracts/Swaplace.sol
Outdated
unchecked { | ||
assembly { | ||
sstore(_totalSwaps.slot, add(sload(_totalSwaps.slot), 1)) | ||
|
||
swapId := sload(_totalSwaps.slot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't change gas usage, we can use the previous way for better legibility
contracts/Swaplace.sol
Outdated
i++; | ||
assembly { | ||
i := mload(0x40) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 72
contracts/Swaplace.sol
Outdated
i := mload(0x40) | ||
|
||
i := add(i, 1) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 74
contracts/Swaplace.sol
Outdated
i++; | ||
assembly { | ||
i := mload(0x40) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 91
contracts/Swaplace.sol
Outdated
i := mload(0x40) | ||
|
||
i := add(i, 1) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 93
contracts/Swaplace.sol
Outdated
@@ -126,6 +140,14 @@ contract Swaplace is SwapFactory, ISwaplace, IERC165 { | |||
* @dev Getter function for _totalSwaps. | |||
*/ | |||
function totalSwaps() public view returns (uint256) { | |||
return _totalSwaps; | |||
uint256 swapId; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totalSwaps is never called on-chain, therefore there is no gas optm., we can keep the original way to preserve legibility
Hey @0xneves, done as requested. Ready for review. |
closes #179