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.

Sign up for GitHub

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

Jump to bottom

Add a new logUserOperationGas function and improve user operation gas estimation #446

Merged
merged 9 commits into from
Jun 26, 2024
Merged

Add a new logUserOperationGas function and improve user operation gas estimation #446

merged 9 commits into from
Jun 26, 2024

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Jun 19, 2024 •
edited
Loading

This PR fixes #425 by introducing a new estimateUserOperationGas function, which runs the simulation using AA's reference EntryPointSimulations contract to ensure we use correct estimations for our test user operations. Previously, we set them to arbitrarily high numbers to make the tests pass. I also improved a couple of tests where we set maxFeePerGas to 0 by actually obtaining the prefund value the account paid for the operation.

The code was mostly borrowed from the account-abstraction reference bundler and is not suitable for use in a production environment.

It also includes the logUserOperationGas function, which fixes the core issue of displaying the actual gas used by the user operation.

Example log:

  Gas Metering
    Safe Deployment + Enabling 4337 Module
           Used 418349 gas (Account or Paymaster) for >Safe with 4337 Module Deployment<
           Used 415210 gas (Transaction) for >Safe with 4337 Module Deployment<
      ✔ Safe with 4337 Module Deployment (137ms)
    Safe Deployment + Enabling 4337 Module + Native Transfers
           Used 449725 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer<
           Used 447665 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer<
      ✔ Safe with 4337 Module Deployment + Native Transfer
           Used 191691 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer<
           Used 182078 gas (Transaction) for >Safe with 4337 Module Native Transfer<
      ✔ Safe with 4337 Module Native Transfer
    Safe Deployment + Enabling 4337 Module + Token Operations
           Used 435081 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer<
           Used 426148 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer<
      ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer
           Used 469349 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer<
           Used 467959 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer<
      ✔ Safe with 4337 Module Deployment + ERC721 Token Minting
    Token Operations Only
           Used 176295 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer<
           Used 160584 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer<
      ✔ Safe with 4337 Module ERC20 Token Transfer
           Used 211369 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Transfer<
           Used 202383 gas (Transaction) for >Safe with 4337 Module ERC721 Transfer<
      ✔ Safe with 4337 Module ERC721 Token Minting

I ran into some types issues on CI and just working on the issue. I'll add a GitHub comment to each one.

@mmv08 mmv08 requested a review from a team as a code owner June 19, 2024 13:33
@mmv08 mmv08 requested review from nlordell, akshay-ap and remedcu and removed request for a team June 19, 2024 13:33
@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from 0b045e9 to 4305341 Compare June 19, 2024 13:37
@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9583033693

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9578863041: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 19, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9583999402

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9578863041: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 marked this pull request as draft June 19, 2024 16:47
@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch 2 times, most recently from 732165b to 65352d4 Compare June 21, 2024 16:32
@coveralls
Copy link

coveralls commented Jun 21, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9616845351

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-37.3%) to 62.712%
Totals Coverage Status
Change from base Build 9615652927: -37.3%
Covered Lines: 29
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from 65352d4 to 4f04ba5 Compare June 21, 2024 17:13
@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch 2 times, most recently from c05a47c to 4e16f66 Compare June 24, 2024 14:02
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9646722637

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9646759500

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from 4e16f66 to 10f95ec Compare June 24, 2024 14:16
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9646992241

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647336964

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647423005

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from f803ec3 to e88d3d4 Compare June 24, 2024 14:46
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647468294

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch 2 times, most recently from 12304ad to 9e23b01 Compare June 24, 2024 14:53
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647573606

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from 9e23b01 to ab61a97 Compare June 24, 2024 14:58
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647650551

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 marked this pull request as ready for review June 24, 2024 15:08
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9647772115

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

mmv08
mmv08 commented Jun 24, 2024
View reviewed changes
examples/4337-passkeys/package.json Show resolved Hide resolved
modules/4337/certora/conf/TransactionExecutionMethods.conf
@@ -18,6 +18,6 @@
"verify": "Safe4337Module:certora/specs/TransactionExecutionMethods.spec",
"packages": [
"@account-abstraction=../../node_modules/.pnpm/@account-abstraction+contracts@0.7.0/node_modules/@account-abstraction",
"@safe-global=../../node_modules/.pnpm/@safe-global+safe-contracts@1.4.1-build.0_ethers@6.12.1_bufferutil@4.0.8_utf-8-validate@5.0.10_/node_modules/@safe-global"
"@safe-global=../../node_modules/.pnpm/@safe-global+safe-contracts@1.4.1-build.0_ethers@6.13.1_bufferutil@4.0.8_utf-8-validate@5.0.10_/node_modules/@safe-global"
Copy link
Member Author

Choose a reason for hiding this comment

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

this didn't survive the lockfile regeneration

modules/4337/contracts/test/SafeMock.sol
Comment on lines +219 to +227

bytes32 dataHash = keccak256(operationData);
uint8 v;
bytes32 r;
bytes32 s;
(v, r, s) = _signatureSplit(signatures);
bool validSignature = owner == ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);

validationData = _packValidationData(!validSignature, validUntil, validAfter);
Copy link
Member Author

Choose a reason for hiding this comment

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

Attention

I had to inline the signature validation to Safe4337Mock because the checkSignatures function always reverted, which doesn't comply with the ERC4337 standard. It also made the simulations fail because if you use a placeholder signature, it'd just revert. Try/catch is not available for internal function calls.

I did not find any security implications, but I didn't spend much time on it since it's a mock contract.

modules/4337/package.json
@@ -50,27 +50,27 @@
"@account-abstraction/contracts": "^0.7.0",
"@noble/curves": "^1.4.0",
"@nomicfoundation/hardhat-ethers": "^3.0.6",
"@nomicfoundation/hardhat-network-helpers": "^1.0.10",
"@nomicfoundation/hardhat-network-helpers": "^1.0.11",
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into some bizzare typescript error again and did a dependency bump. It wasn't it in the end, but I decided to keep it - no breaking changes here.

modules/4337/src/utils/execution.ts
Comment on lines +188 to +189
actualGasUsed: parsedUserOperationEvent.args.actualGasUsed,
actualGasCost: parsedUserOperationEvent.args.actualGasCost,
Copy link
Member Author

Choose a reason for hiding this comment

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

initially, I wanted to use those values in tests, but then I realised that the actual gas cost and paid prefund vary, so in the end, they were not needed, but IMO, it's handy to have them available, so I kept them. LMK if you disagree

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to include it.

@mmv08 mmv08 changed the title Add a new logUserOperationGas function Add a new logUserOperationGas function and improve user operation gas estimation Jun 24, 2024
nlordell
nlordell reviewed Jun 25, 2024
View reviewed changes
modules/4337/test/erc4337/Safe4337Mock.spec.ts
} from '../../src/utils/userOp'
import { chainId, timestamp } from '../utils/encoding'
import { estimateUserOperationGas } from '../utils/simulations'

describe('Safe4337Mock', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

rewriting tests is always annoying 😅

nlordell
nlordell approved these changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM. Just some smaller nits, but nothing blocking the merge.

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from 3f9ef43 to 105c00f Compare June 25, 2024 17:58
@coveralls
Copy link

coveralls commented Jun 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9667332437

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9615652927: 0.0%
Covered Lines: 39
Relevant Lines: 39
💛 - Coveralls

@mmv08 mmv08 force-pushed the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch from eb6bc94 to 87b998a Compare June 25, 2024 18:24
nlordell
nlordell approved these changes Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Awesomesauce.

@mmv08 mmv08 merged commit b4a0b60 into main Jun 26, 2024
6 checks passed
@mmv08 mmv08 deleted the 425-gas-benchmarking-in-4337-tests-outputs-gas-used-for-the-transaction-instead-of-useroperation-gas branch June 26, 2024 10:01
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nlordell nlordell nlordell approved these changes

@akshay-ap akshay-ap Awaiting requested review from akshay-ap akshay-ap is a code owner automatically assigned from safe-global/safe-protocol

@remedcu remedcu Awaiting requested review from remedcu remedcu is a code owner automatically assigned from safe-global/safe-protocol

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gas benchmarking in 4337 tests outputs gas used for the transaction instead of user operation gas
3 participants
@mmv08 @coveralls @nlordell