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.

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

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/4337-passkeys/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,22 @@
"@account-abstraction/contracts": "0.7.0",
"@safe-global/safe-4337": "0.3.0",
"@safe-global/safe-contracts": "^1.4.1-build.0",
"@safe-global/safe-deployments": "^1.36.0",
"@safe-global/safe-deployments": "^1.37.0",
"@safe-global/safe-modules-deployments": "^2.2.0",
"@safe-global/safe-passkey": "0.2.0",
"@web3modal/ethers": "^4.1.11",
"ethers": "^6.12.1",
"ethers": "^6.13.1",
"react": "^18.3.1",
"react-dom": "^18.3.1",
"react-router-dom": "^6.23.1"
},
"devDependencies": {
"@types/react": "^18.3.2",
"@types/react": "^18.3.3",
"@types/react-dom": "^18.3.0",
"@vitejs/plugin-react-swc": "^3.6.0",
"@vitejs/plugin-react-swc": "^3.7.0",
"react-router": "^6.23.1",
"typescript": "^5.4.5",
"vite": "^5.2.11",
"typescript": "^5.5.2",
"vite": "^5.3.1",
nlordell marked this conversation as resolved.
Show resolved Hide resolved
"vite-plugin-commonjs": "^0.10.1"
}
}
27 changes: 27 additions & 0 deletions examples/4337-passkeys/tsconfig.app.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"compilerOptions": {
"composite": true,
"tsBuildInfoFile": "./node_modules/.tmp/tsconfig.app.tsbuildinfo",
"target": "ES2020",
"useDefineForClassFields": true,
"lib": ["ES2020", "DOM", "DOM.Iterable"],
"module": "ESNext",
"skipLibCheck": true,

/* Bundler mode */
"moduleResolution": "bundler",
"allowImportingTsExtensions": true,
"resolveJsonModule": true,
"isolatedModules": true,
"moduleDetection": "force",
"noEmit": true,
"jsx": "react-jsx",

/* Linting */
"strict": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"noFallthroughCasesInSwitch": true
},
"include": ["src"]
}
32 changes: 9 additions & 23 deletions examples/4337-passkeys/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,25 +1,11 @@
{
"compilerOptions": {
"target": "ES2020",
"useDefineForClassFields": true,
"lib": ["ES2020", "DOM", "DOM.Iterable"],
"module": "ESNext",
"skipLibCheck": true,

/* Bundler mode */
"moduleResolution": "bundler",
"allowImportingTsExtensions": true,
"resolveJsonModule": true,
"isolatedModules": true,
"noEmit": true,
"jsx": "react-jsx",

/* Linting */
"strict": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"noFallthroughCasesInSwitch": true
},
"include": ["src"],
"references": [{ "path": "./tsconfig.node.json" }]
"files": [],
"references": [
{
"path": "./tsconfig.app.json"
},
{
"path": "./tsconfig.node.json"
}
]
}
5 changes: 4 additions & 1 deletion examples/4337-passkeys/tsconfig.node.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
{
"compilerOptions": {
"composite": true,
"tsBuildInfoFile": "./node_modules/.tmp/tsconfig.node.tsbuildinfo",
"skipLibCheck": true,
"module": "ESNext",
"moduleResolution": "bundler",
"allowSyntheticDefaultImports": true
"allowSyntheticDefaultImports": true,
"strict": true,
"noEmit": true
},
"include": ["vite.config.ts"]
}
2 changes: 1 addition & 1 deletion modules/4337/certora/conf/Safe4337Module.conf
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
"verify": "Safe4337Module:certora/specs/Safe4337Module.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"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

]
}
2 changes: 1 addition & 1 deletion modules/4337/certora/conf/ValidationDataLastBitOne.conf
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
"verify": "Safe4337Module:certora/specs/ValidationDataLastBitOne.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"
]
}
11 changes: 9 additions & 2 deletions modules/4337/contracts/test/SafeMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,15 @@ contract Safe4337Mock is SafeMock, IAccount {
*/
function _validateSignatures(PackedUserOperation calldata userOp) internal view returns (uint256 validationData) {
(bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp);
checkSignatures(keccak256(operationData), operationData, signatures);
validationData = _packValidationData(false, validUntil, validAfter);

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);
Comment on lines +219 to +227
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.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@ import "@safe-global/safe-contracts/contracts/libraries/MultiSend.sol";
import "@safe-global/safe-contracts/contracts/libraries/SafeStorage.sol";
import "@safe-global/safe-contracts/contracts/proxies/SafeProxyFactory.sol";
import "@safe-global/safe-contracts/contracts/SafeL2.sol";
// Named import for EntryPointSimulations needed because it also defines an interface for IERC165, which
// conflicts with the same interface defined in the Safe contracts.
/* solhint-disable-next-line no-unused-import */
import {EntryPointSimulations} from "@account-abstraction/contracts/core/EntryPointSimulations.sol";
14 changes: 7 additions & 7 deletions modules/4337/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"@nomicfoundation/hardhat-toolbox": "^5.0.0",
"@openzeppelin/contracts": "^5.0.2",
"@safe-global/safe-4337-local-bundler": "workspace:^0.0.0",
"@safe-global/safe-4337-provider": "workspace:^0.0.0",
"@simplewebauthn/server": "10.0.0",
"@types/chai": "^4.3.16",
"@types/mocha": "^10.0.6",
"@types/node": "^20.14.0",
"@types/mocha": "^10.0.7",
"@types/node": "^20.14.8",
"@types/yargs": "^17.0.32",
"cbor": "^9.0.2",
"debug": "^4.3.4",
"debug": "^4.3.5",
"dotenv": "^16.4.5",
"ethers": "^6.12.1",
"hardhat": "^2.22.3",
"ethers": "^6.13.1",
"hardhat": "^2.22.5",
"hardhat-deploy": "^0.12.4",
"husky": "^9.0.11",
"solc": "^0.8.25",
"solhint": "^5.0.1",
"ts-node": "^10.9.2",
"typescript": "^5.4.5",
"typescript": "^5.5.2",
"yargs": "^17.7.2"
},
"dependencies": {
Expand Down
19 changes: 19 additions & 0 deletions modules/4337/src/deploy/entrypointHelpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { DeployFunction } from 'hardhat-deploy/types'

const deploy: DeployFunction = async ({ deployments, getNamedAccounts, network }) => {
if (!network.tags.test) {
return
}

const { deployer } = await getNamedAccounts()
const { deploy } = deployments

await deploy('EntryPointSimulations', {
from: deployer,
args: [],
log: true,
deterministicDeployment: true,
})
}

export default deploy
48 changes: 48 additions & 0 deletions modules/4337/src/utils/execution.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { BigNumberish, Signer, TransactionResponse, ethers } from 'ethers'
import { IEntryPoint } from '../../typechain-types'

export const EIP_DOMAIN = {
EIP712Domain: [
Expand Down Expand Up @@ -137,9 +138,56 @@ export const buildSignatureBytes = (signatures: SafeSignature[]): string => {
export const logGas = async (message: string, tx: Promise<TransactionResponse>, skip?: boolean): Promise<TransactionResponse> => {
return tx.then(async (result) => {
const receipt = await result.wait()

if (!receipt?.gasUsed) throw new Error('No gas used in receipt')

if (!skip) console.log(` Used ${receipt.gasUsed} gas for >${message}<`)
return result
})
}

type UserOperationGasLog = {
actualGasUsed: bigint
actualGasCost: bigint
transactionResponse: TransactionResponse
}

/**
* Logs the gas used by a user operation and returns the gas log.
*
* @param message - The message associated with the user operation.
* @param entryPoint - The entry point object.
* @param tx - The transaction promise.
* @param skip - Optional flag to skip logging.
* @returns {UserOperationGasLog} A promise that resolves to the user operation gas log.
* @throws An error if the receipt is not available, gas used is not available in the receipt,
* gas used or gas cost is not available in the UserOperationEvent, or UserOperationEvent is not emitted.
*/
export const logUserOperationGas = async (
message: string,
entryPoint: IEntryPoint,
tx: Promise<TransactionResponse>,
skip?: boolean,
): Promise<UserOperationGasLog> => {
return tx.then(async (transactionResponse) => {
const receipt = await transactionResponse.wait()
if (!receipt) throw new Error('No receipt')

const userOperationEvent = await entryPoint.queryFilter(entryPoint.filters.UserOperationEvent(), receipt.blockNumber)
const parsedUserOperationEvent = entryPoint.interface.parseLog(userOperationEvent[0])

if (!receipt?.gasUsed) throw new Error('No gas used in receipt')
if (!parsedUserOperationEvent?.args.actualGasUsed || !parsedUserOperationEvent?.args.actualGasCost)
throw new Error('No gas used or gas cost in UserOperationEvent or UserOperationEvent not emitted')

if (!skip) {
console.log(` Used ${parsedUserOperationEvent.args.actualGasUsed} gas (Account or Paymaster) for >${message}<`)
console.log(` Used ${receipt.gasUsed} gas (Transaction) for >${message}<`)
}
return {
actualGasUsed: parsedUserOperationEvent.args.actualGasUsed,
actualGasCost: parsedUserOperationEvent.args.actualGasCost,
Comment on lines +188 to +189
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.

transactionResponse,
}
})
}
8 changes: 5 additions & 3 deletions modules/4337/src/utils/userOp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ export { PackedUserOperation, UserOperation }
type OptionalExceptFor<T, TRequired extends keyof T = keyof T> = Partial<Pick<T, Exclude<keyof T, TRequired>>> &
Required<Pick<T, TRequired>>

export const PLACEHOLDER_SIGNATURE =
'0x9c8ecb7ad80d2dd4411c8827079cda17095236ee3cba1c9b81153d52af17bc9d0701228dc95a75136a3e3a0130988ba4053cc15d3805db49e2cc08d9c99562191b'

export type SafeUserOperation = {
safe: string
entryPoint: string
Expand Down Expand Up @@ -172,11 +175,10 @@ export const getRequiredGas = (userOp: PackedUserOperation): string => {
return (BigInt(callGasLimit) + BigInt(verificationGasLimit) * multiplier + BigInt(userOp.preVerificationGas)).toString()
}

export const getRequiredPrefund = (userOp: PackedUserOperation): string => {
export const getRequiredPrefund = (userOp: PackedUserOperation): bigint => {
const requiredGas = getRequiredGas(userOp)
const { maxFeePerGas } = unpackGasParameters(userOp)
const requiredPrefund = (BigInt(requiredGas) * BigInt(maxFeePerGas)).toString()
console.log({ requiredGas, requiredPrefund })
const requiredPrefund = BigInt(requiredGas) * BigInt(maxFeePerGas)

return requiredPrefund
}
Expand Down
Loading
Loading