optimize gas usage with common techniques and refactor code structure#341
optimize gas usage with common techniques and refactor code structure#341
Conversation
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
contracts/abstracts/EIP712.sol
Outdated
| assembly { | ||
| // Compute the digest. | ||
| mstore(0x00, 0x1901000000000000000000000000000000000000000000000000000000000000) // Store "\x19\x01". | ||
| mstore(0x2, digest) // Store the domain separator. |
There was a problem hiding this comment.
Maybe align with other hex values:
| mstore(0x2, digest) // Store the domain separator. | |
| mstore(0x02, digest) // Store the domain separator. |
| digest = _getDomainSeparator(); | ||
|
|
||
| // solhint-disable no-inline-assembly | ||
| assembly { |
There was a problem hiding this comment.
Are those inline assemblies copied from some standard library? How much gas does it save?
There was a problem hiding this comment.
Is it possible to use OZ's 712 implementation?
There was a problem hiding this comment.
Are those inline assemblies copied from some standard library?
The assembly code was referenced from soladay's implementaion and 1inch's implementation (line 1204~1209)
How much gas does it save?
Basically, it saves 135 gas when testing the getEIP712Hash function. However, the main value of using assembly here is that it helps avoid quadratic memory expansion costs, as we reuse the same memory slot for hashing.
reference: https://x.com/optimizoor/status/1825913380244435408?mx=2
Is it possible to use OZ's 712 implementation?
Our implementation declares some variables as public, while the OZ's implementation declares them as private. I'm not sure whether this difference might cause issues in the front-end or back-end. I will switch to OZ's (or slady) implementation after verifying that it does not break anything on either end
There was a problem hiding this comment.
Perhaps add a comment referencing the code copied from
| assertFalse(SignatureValidator.validateSignature(vm.addr(walletAdminPrivateKey), otherDigest, signature)); | ||
| } | ||
|
|
||
| /// forge-config: default.allow_internal_expect_revert = true |
There was a problem hiding this comment.
What is the setting for? Where is it checked?
There was a problem hiding this comment.
Same reason as mentioned above.
Add this config setting to resolve the following error:
[FAIL: call didn't revert at a lower depth than cheatcode call depth]
| { | ||
| bytes memory makerSpecificData = abi.encode(defaultAMMPath); | ||
| bytes memory strategyData = abi.encode(UNISWAP_SWAP_ROUTER_02_ADDRESS, makerSpecificData); | ||
| bytes memory strategyData = abi.encode(UNISWAP_SWAP_ROUTER_02_ADDRESS, order.makerToken, order.makerTokenAmount - fee, makerSpecificData); |
There was a problem hiding this comment.
Why added these two parameters? Is it because the change of executeStrategy interface?
There was a problem hiding this comment.
Yes, the strategyData here is for the mockLimitOrderTaker, which inherits an interface that defines the executeStrategy function. To minimize breaking changes for the mock contract, we simply encode these two parameters in strategyData.
|
Can you still run |
Yes, it works fine after enabling the |
| if (ops.length == 0) revert EmptyOps(); | ||
|
|
||
| // wrap ETH to WETH if inputToken is ETH | ||
| if (Asset.isETH(inputToken)) { |
There was a problem hiding this comment.
This if statement duplicates the logic in the GenericSwap contract: https://github.com/consenlabs/tokenlon-contracts/blob/v6.0.1/contracts/GenericSwap.sol#L68-L75
| if (msg.value != inputAmount) revert InvalidMsgValue(); | ||
| // the coverage report indicates that the following line causes this branch to not be covered by our tests | ||
| // even though we tried all possible success and revert scenarios | ||
| IWETH(weth).deposit{ value: inputAmount }(); |
There was a problem hiding this comment.
We don't wrap ETH directly, as it could prevent SmartOrderStrategy from interacting with functions that require ETH.
| /// @dev This contract provides functions to handle EIP-712 domain separator and hash calculation. | ||
| abstract contract EIP712 { | ||
| // EIP-191 Header | ||
| string public constant EIP191_HEADER = "\x19\x01"; |
There was a problem hiding this comment.
This variable is no longer needed after rewriting the getEIP712Hash function
| if (!success) { | ||
| assembly { | ||
| revert(add(result, 32), mload(result)) | ||
| if (amount > 0) { |
There was a problem hiding this comment.
Using nested if statements is more gas-efficient.
| assertFalse(SignatureValidator.validateSignature(vm.addr(walletAdminPrivateKey), otherDigest, signature)); | ||
| } | ||
|
|
||
| /// forge-config: default.allow_internal_expect_revert = true |
There was a problem hiding this comment.
Add this config setting to resolve the following error:
[FAIL: call didn't revert at a lower depth than cheatcode call depth]
| optimizer = true # enable or disable the solc optimizer | ||
| optimizer_runs = 1000 # the number of optimizer runs | ||
| optimizer_runs = 65536 # the number of optimizer runs | ||
| via_ir = true # enable or disable the compilation pipeline for the new IR optimizer |
There was a problem hiding this comment.
Enabling via-IR: true improves Solidity compilation by introducing an Intermediate Representation (IR) pipeline. This enhances bytecode efficiency, reduces gas costs, and optimizes contract size. It also makes compilation more predictable and future-proof, aligning with Solidity's long-term roadmap
| bytes memory opsData = abi.encode(operations); | ||
|
|
||
| vm.startPrank(genericSwap); | ||
| vm.deal(address(smartOrderStrategy), rfqOffer.takerTokenAmount); |
There was a problem hiding this comment.
The deal cheatcode attempts to mock GenericSwap transferring takerToken to the SmartOrderStrategy contract, but since takerToken is ETH, which is sent during executeStrategy, deal should not be used here.
| assertFalse(SignatureValidator.validateSignature(vm.addr(walletAdminPrivateKey), otherDigest, signature)); | ||
| } | ||
|
|
||
| /// forge-config: default.allow_internal_expect_revert = true |
There was a problem hiding this comment.
Same reason as mentioned above.
Add this config setting to resolve the following error:
[FAIL: call didn't revert at a lower depth than cheatcode call depth]
| { | ||
| bytes memory makerSpecificData = abi.encode(defaultAMMPath); | ||
| bytes memory strategyData = abi.encode(UNISWAP_SWAP_ROUTER_02_ADDRESS, makerSpecificData); | ||
| bytes memory strategyData = abi.encode(UNISWAP_SWAP_ROUTER_02_ADDRESS, order.makerToken, order.makerTokenAmount - fee, makerSpecificData); |
There was a problem hiding this comment.
Yes, the strategyData here is for the mockLimitOrderTaker, which inherits an interface that defines the executeStrategy function. To minimize breaking changes for the mock contract, we simply encode these two parameters in strategyData.
| digest = _getDomainSeparator(); | ||
|
|
||
| // solhint-disable no-inline-assembly | ||
| assembly { |
There was a problem hiding this comment.
Are those inline assemblies copied from some standard library?
The assembly code was referenced from soladay's implementaion and 1inch's implementation (line 1204~1209)
How much gas does it save?
Basically, it saves 135 gas when testing the getEIP712Hash function. However, the main value of using assembly here is that it helps avoid quadratic memory expansion costs, as we reuse the same memory slot for hashing.
reference: https://x.com/optimizoor/status/1825913380244435408?mx=2
Is it possible to use OZ's 712 implementation?
Our implementation declares some variables as public, while the OZ's implementation declares them as private. I'm not sure whether this difference might cause issues in the front-end or back-end. I will switch to OZ's (or slady) implementation after verifying that it does not break anything on either end
2 participants
via-irsetting.optimizer_runsfrom 1,000 to 65,536.executeStrategyfunction.getEIP712Hashfunction of EIP-712 using assembly code.