Skip to content

feat: E2E gas profiling native and erc20#123

Closed
shamit05 wants to merge 7 commits intocoinbase:mainfrom
shamit05:shamit/e2e-gas-profiling-native-erc20
Closed

feat: E2E gas profiling native and erc20#123
shamit05 wants to merge 7 commits intocoinbase:mainfrom
shamit05:shamit/e2e-gas-profiling-native-erc20

Conversation

@shamit05
Copy link
Copy Markdown
Contributor

@shamit05 shamit05 commented Sep 8, 2025

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

@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented Sep 8, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@@ -0,0 +1,111 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make at parity with codebase so ^0.8.23

import {UserOperation} from "account-abstraction/interfaces/UserOperation.sol";
import {console2} from "forge-std/Test.sol";

/// forge-config: default.isolate = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what does this do?

}

// Native ETH transfer comparison between ERC-4337 and EOA
function test_transfer_native() public {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would benefit from comments to annotate the code sections here for easier skimming

"rev": "cf2b89c33fed536c4dd6fef2fb84f39053068868"
},
"lib/solady": {
"rev": "c4c96607cb3aa3807b14c81ae2015bcba061f8fc"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if this is a commit, we actually want this to be 5ea5d9f57ed6d24a27d00934f4a3448def931415

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@amiecorso did we update .gas-snapshot when pushing v1.1? Feels weird for the diff to be so big unless its fuzzing variance?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we did actually... See the file on the v1.1 commit here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

differences could have to do with how things are compiled before snapshotting though... since control of that stuff is a mess

@cb-heimdall
Copy link
Copy Markdown
Collaborator

Review Error for ilikesymmetry @ 2025-09-10 00:06:54 UTC
User failed mfa authentication, see go/mfa-help

}

// ERC20 transfer comparison between ERC-4337 and EOA
function test_transfer_erc20() public {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in the https://github.com/coinbase/solidity-style-guide there's a preference for tests only testing one thing. not necessarily a blocker here especially since these are just benchmarks, but if I were to write this I would split it out into the 4 explicit cases of native/erc20, scw/eoa for better org and readability.

return ops;
}

function _sign(UserOperation memory userOp) internal view override returns (bytes memory signature) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should already be available on the SmartWalletTestBase parent contract so I don't think we need to re-write here?

Copy link
Copy Markdown
Contributor

@amiecorso amiecorso left a comment

Choose a reason for hiding this comment

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

nice! love to see you in solidity repos @shamit05

@cb-heimdall
Copy link
Copy Markdown
Collaborator

Review Error for amiecorso @ 2025-09-10 00:24:00 UTC
User failed mfa authentication, either user does not exist or public email is not set on your github profile. \ see go/mfa-help

@amiecorso amiecorso self-requested a review September 10, 2025 00:24
@shamit05 shamit05 closed this Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants