-
Notifications
You must be signed in to change notification settings - Fork 161
feat: E2E gas profiling native and erc20 #122
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| { | ||
| "lib/account-abstraction": { | ||
| "rev": "abff2aca61a8f0934e533d0d352978055fddbd96" | ||
| }, | ||
| "lib/forge-std": { | ||
| "tag": { | ||
| "name": "v1.10.0", | ||
| "rev": "8bbcf6e3f8f62f419e5429a0bd89331c85c37824" | ||
| } | ||
| }, | ||
| "lib/openzeppelin-contracts": { | ||
| "rev": "5705e8208bc92cd82c7bcdfeac8dbc7377767d96" | ||
| }, | ||
| "lib/p256-verifier": { | ||
| "rev": "29475ae300ec95d98d5c7cc34c094846f0aa2dcd" | ||
| }, | ||
| "lib/safe-singleton-deployer-sol": { | ||
| "rev": "cf2b89c33fed536c4dd6fef2fb84f39053068868" | ||
| }, | ||
| "lib/solady": { | ||
| "rev": "c4c96607cb3aa3807b14c81ae2015bcba061f8fc" | ||
| }, | ||
| "lib/webauthn-sol": { | ||
| "rev": "619f20ab0f074fef41066ee4ab24849a913263b2" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "e2e_transfer_erc20_4337": "159402", | ||
| "e2e_transfer_erc20_eoa": "50910", | ||
| "e2e_transfer_native_4337": "159635", | ||
| "e2e_transfer_native_eoa": "17652" | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,103 @@ | ||||||||||||||||||||||||
| // SPDX-License-Identifier: MIT | ||||||||||||||||||||||||
| pragma solidity ^0.8.4; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import {console2} from "forge-std/Test.sol"; | ||||||||||||||||||||||||
| import {SmartWalletTestBase} from "../CoinbaseSmartWallet/SmartWalletTestBase.sol"; | ||||||||||||||||||||||||
| import {CoinbaseSmartWallet} from "../../src/CoinbaseSmartWallet.sol"; | ||||||||||||||||||||||||
| import {CoinbaseSmartWalletFactory} from "../../src/CoinbaseSmartWalletFactory.sol"; | ||||||||||||||||||||||||
| import {MockTarget} from "../mocks/MockTarget.sol"; | ||||||||||||||||||||||||
| import {UserOperation} from "account-abstraction/interfaces/UserOperation.sol"; | ||||||||||||||||||||||||
| import {MockERC20} from "../../lib/solady/test/utils/mocks/MockERC20.sol"; | ||||||||||||||||||||||||
| import {Static} from "../CoinbaseSmartWallet/Static.sol"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// forge-config: default.isolate = true | ||||||||||||||||||||||||
| contract EndToEndTest is SmartWalletTestBase { | ||||||||||||||||||||||||
| address eoaUser = address(0xe0a); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| MockERC20 usdc; | ||||||||||||||||||||||||
| MockTarget target; | ||||||||||||||||||||||||
| CoinbaseSmartWalletFactory factory; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function setUp() public override { | ||||||||||||||||||||||||
| vm.etch(0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, Static.ENTRY_POINT_BYTES); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| CoinbaseSmartWallet implementation = new CoinbaseSmartWallet(); | ||||||||||||||||||||||||
| factory = new CoinbaseSmartWalletFactory(address(implementation)); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| signerPrivateKey = 0xa11ce; | ||||||||||||||||||||||||
| signer = vm.addr(signerPrivateKey); | ||||||||||||||||||||||||
| owners.push(abi.encode(signer)); | ||||||||||||||||||||||||
| account = factory.createAccount(owners, 0); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| vm.deal(address(account), 100 ether); | ||||||||||||||||||||||||
| vm.deal(eoaUser, 100 ether); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| usdc = new MockERC20("USD Coin", "USDC", 6); | ||||||||||||||||||||||||
| usdc.mint(address(account), 10000e6); | ||||||||||||||||||||||||
| usdc.mint(eoaUser, 10000e6); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| target = new MockTarget(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Native ETH transfer comparison between ERC-4337 and EOA | ||||||||||||||||||||||||
| function test_transfer_native() public { | ||||||||||||||||||||||||
| userOpCalldata = abi.encodeCall(CoinbaseSmartWallet.execute, (address(0x1234), 1 ether, "")); | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| vm.startSnapshotGas("e2e_transfer_native_4337"); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| vm.startSnapshotGas("e2e_transfer_native_4337"); | |
| vm.startSnapshotGas("e2e_transfer_native_baseAccount"); |
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.
if using fuzzing from earlier comment
| payable(address(0x1234)).transfer(1 ether); | |
| payable(address(recipient)).transfer(1 ether); |
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.
same thing here as previous function with:
- fuzzing instead of hardcoding values
- dust recipients to remove variability from zero->nonzero storage transition
Outdated
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.
also add eoa calldata size for comparison
Outdated
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.
| vm.startSnapshotGas("e2e_transfer_erc20_4337"); | |
| vm.startSnapshotGas("e2e_transfer_erc20_baseAccount"); |
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.
use fuzzing
| usdc.transfer(address(0x5678), 100e6); | |
| usdc.transfer(recipient, amount); |
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.
Not seeing where this is being used. Does the parent test contract require use to override this?
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.
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.