feat: E2E gas profiling native and erc20#122
Conversation
🟡 Heimdall Review Status
|
|
Addressing this previous comment:
Yes, it does since it calls internal _getUserOp which uses userOpCalldata. |
ilikesymmetry
left a comment
There was a problem hiding this comment.
Good pass, handful of comments on cleaning up the conventions
| function test_transfer_native() public { | ||
| userOpCalldata = abi.encodeCall(CoinbaseSmartWallet.execute, (address(0x1234), 1 ether, "")); |
There was a problem hiding this comment.
Recommend fuzzing in general which can remove hardcoded values.
Also there's an increased cost when updating the storage for a users balance from zero to nonzero so we can cover for that by dusting the address first.
| function test_transfer_native() public { | |
| userOpCalldata = abi.encodeCall(CoinbaseSmartWallet.execute, (address(0x1234), 1 ether, "")); | |
| function test_transfer_native(address recipient, uint256 amount) public { | |
| vm.assume(amount < 1 ether); | |
| // Dust recipient to control for gas increase for first non-zero balance | |
| vm.deal(recipient, 1 wei); | |
| usdc.mint(eoaUser, 1 wei); | |
| // Prepare userOp | |
| userOpCalldata = abi.encodeCall(CoinbaseSmartWallet.execute, (recipient, amount, ""); |
test/gas/EndToEnd.t.sol
Outdated
| bytes memory handleOpsCalldata = abi.encodeCall(entryPoint.handleOps, (_makeOpsArray(op), payable(bundler))); | ||
| console2.log("test_transfer_native ERC-4337 calldata size:", handleOpsCalldata.length); | ||
|
|
||
| vm.startSnapshotGas("e2e_transfer_native_4337"); |
There was a problem hiding this comment.
naming nit
| vm.startSnapshotGas("e2e_transfer_native_4337"); | |
| vm.startSnapshotGas("e2e_transfer_native_baseAccount"); |
|
|
||
| vm.prank(eoaUser); | ||
| vm.startSnapshotGas("e2e_transfer_native_eoa"); | ||
| payable(address(0x1234)).transfer(1 ether); |
There was a problem hiding this comment.
if using fuzzing from earlier comment
| payable(address(0x1234)).transfer(1 ether); | |
| payable(address(recipient)).transfer(1 ether); |
test/gas/EndToEnd.t.sol
Outdated
| UserOperation memory op = _getUserOpWithSignature(); | ||
|
|
||
| bytes memory handleOpsCalldata = abi.encodeCall(entryPoint.handleOps, (_makeOpsArray(op), payable(bundler))); | ||
| console2.log("test_transfer_native ERC-4337 calldata size:", handleOpsCalldata.length); |
There was a problem hiding this comment.
also thinking we should log the calldata size for eoa execution as well for easy diff
| function test_transfer_erc20() public { | ||
| userOpCalldata = abi.encodeCall( | ||
| CoinbaseSmartWallet.execute, | ||
| (address(usdc), 0, abi.encodeCall(usdc.transfer, (address(0x5678), 100e6))) | ||
| ); |
There was a problem hiding this comment.
same thing here as previous function with:
- fuzzing instead of hardcoding values
- dust recipients to remove variability from zero->nonzero storage transition
test/gas/EndToEnd.t.sol
Outdated
| UserOperation memory op = _getUserOpWithSignature(); | ||
|
|
||
| bytes memory handleOpsCalldata = abi.encodeCall(entryPoint.handleOps, (_makeOpsArray(op), payable(bundler))); | ||
| console2.log("test_transfer_erc20 ERC-4337 calldata size:", handleOpsCalldata.length); |
There was a problem hiding this comment.
also add eoa calldata size for comparison
test/gas/EndToEnd.t.sol
Outdated
| bytes memory handleOpsCalldata = abi.encodeCall(entryPoint.handleOps, (_makeOpsArray(op), payable(bundler))); | ||
| console2.log("test_transfer_erc20 ERC-4337 calldata size:", handleOpsCalldata.length); | ||
|
|
||
| vm.startSnapshotGas("e2e_transfer_erc20_4337"); |
There was a problem hiding this comment.
| vm.startSnapshotGas("e2e_transfer_erc20_4337"); | |
| vm.startSnapshotGas("e2e_transfer_erc20_baseAccount"); |
|
|
||
| vm.prank(eoaUser); | ||
| vm.startSnapshotGas("e2e_transfer_erc20_eoa"); | ||
| usdc.transfer(address(0x5678), 100e6); |
There was a problem hiding this comment.
use fuzzing
| usdc.transfer(address(0x5678), 100e6); | |
| usdc.transfer(recipient, amount); |
| return ops; | ||
| } | ||
|
|
||
| function _sign(UserOperation memory userOp) internal view override returns (bytes memory signature) { |
There was a problem hiding this comment.
Not seeing where this is being used. Does the parent test contract require use to override this?
There was a problem hiding this comment.
Think it would be good to separate the lockfile and lib/forge-std diff into its own tiny PR for clarity and merge that first. Might not be worth the effort though, but generally touching build stuff is high enough importance to be isolated.
ilikesymmetry
left a comment
There was a problem hiding this comment.
Thanks for adding benchmarks :)
|
Review Error for ilikesymmetry @ 2025-09-08 20:10:32 UTC |
|
Review Error for ilikesymmetry @ 2025-09-08 22:34:51 UTC |
Gas profiling done for native and erc20 transfers. Results in snapshot folder.
https://docs.google.com/document/d/1G-ReRP_SQ4SvbHSjytj_ubdRwXTTMTdlwPAYSyr7LF4/edit?tab=t.0