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

Skipper-go Solidity Contract Gas Optimization #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jim380
Copy link

@jim380 jim380 commented Apr 10, 2023

A few gas optimizations that I nitpicked while checking out the codebase:

Test Gas Cost Before Gas Cost After
testFailIfArbitrageIsntProfitable 1054705 1023194
testFailWithdrawNativeBalanceWhenCalledByNonOwner 654259 622197
testFailWithdrawWhenCalledByNonOwner 842590 810528
testItWorks 1085981 1054530
testWithdrawNativeBalanceWorksWhenCalledByOwner 661190 629148
testWithdrawWorksWhenCalledByOwner 769274 737212

@@ -20,6 +20,10 @@ contract Multihop is Ownable {
uint256 fee;
}

// --------------------- Errors -------------------- //
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use custom errors to save gas

@@ -35,7 +39,7 @@ contract Multihop is Ownable {
function swapMultihop(
address fromToken,
uint256 fromAmount,
DexHop[] memory route
DexHop[] calldata route
Copy link
Author

@jim380 jim380 Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

route is not mutated in the function body so using calldata would save some gas.

@@ -48,24 +52,27 @@ contract Multihop is Ownable {
IERC20(fromToken).transfer(route[0].pairAddress, fromAmount);

// Loop through the route and execute the trades
for (uint256 i = 0; i < route.length; i++) {
DexHop memory hop = route[i];
uint256 length = route.length;
Copy link
Author

@jim380 jim380 Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching array length to avoid repeatedly loading from the state in each iteration.

Comment on lines +126 to +127
(bool success, ) = owner().call{value: balance}("");
if (!success) revert WithdrawNativeBalanceFailed();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.transfer has a hardcoded 2300 gas limit which prevents smart contracts from doing things like sending ether to a gnosis safe due to out of gas. .call instead forwards all gas and is now the recommended way to send ether from a contract. It's also reentrancy safe here since the function is only accessible by the owner.

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.

None yet

1 participant