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

AA-440 validate using handleOps #243

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
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
6 changes: 4 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@ jobs:
with:
node-version: '18'
- uses: actions/checkout@v4
with:
show-progress: false
- uses: actions/cache@v4
with:
path: node_modules
key: ${{ runner.os }}-${{ hashFiles('yarn.lock') }}
- run: yarn install
- run: yarn run ci

lint:
Expand All @@ -35,11 +36,12 @@ jobs:
with:
node-version: '18'
- uses: actions/checkout@v4
with:
show-progress: false
- uses: actions/cache@v4
with:
path: node_modules
key: ${{ runner.os }}-${{ hashFiles('yarn.lock') }}
- run: yarn install
- run: yarn preprocess
- run: yarn lerna-lint

Expand Down
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[submodule "submodules/account-abstraction"]
path = submodules/account-abstraction
url = https://github.com/eth-infinitism/account-abstraction.git
branch = releases/v0.7
branch = develop
[submodule "submodules/rip7560"]
path = submodules/rip7560
url = https://github.com/eth-infinitism/rip7560_contracts.git
3 changes: 1 addition & 2 deletions lerna.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{
"version": "0.6.0",
"npmClient": "yarn",
"useWorkspaces": true
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we use lerna workspaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lerna/lerna#3695
lerna removed the useWorkspaces, and makes it the default.

"npmClient": "yarn"
}
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"lerna-lint": "lerna run lint --stream --parallel -- ",
"lerna-test": "lerna run hardhat-test --stream --",
"lerna-tsc": "lerna run tsc --scope @account-abstraction/*",
"lerna-watch-tsc": "lerna run --parallel watch-tsc",
"lerna-watch-tsc": "lerna run --stream --parallel watch-tsc",
"lint-fix": "lerna run lint-fix --parallel",
"clear": "lerna run clear",
"hardhat-compile": "lerna run hardhat-compile",
Expand All @@ -37,21 +37,21 @@
"preprocess1": "yarn submodule-update && yarn && cd submodules/account-abstraction && yarn && yarn --cwd contracts prepack",
"submodule-update": "git submodule update --remote --init --recursive",
"runop-self": "ts-node ./packages/bundler/src/runner/runop.ts --deployFactory --selfBundler --unsafe",
"ci": "env && yarn depcheck && yarn preprocess && yarn lerna-test"
"ci": "env && yarn && yarn depcheck && yarn preprocess && yarn lerna-test"
},
"dependencies": {
"@ethereumjs/common": "^5.0.0-alpha.1",
"@ethereumjs/tx": "^6.0.0-alpha.1",
"@ethereumjs/util": "^10.0.0-alpha.1",
"@typescript-eslint/eslint-plugin": "^5.33.0",
"@typescript-eslint/parser": "^5.33.0",
"depcheck": "^1.4.3",
"depcheck": "^1.4.7",
"eslint": "^8.21.0",
"eslint-config-standard-with-typescript": "^22.0.0",
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-n": "^15.2.4",
"eslint-plugin-promise": "^6.0.0",
"lerna": "^5.4.0",
"lerna": "^8.1.9",
"typescript": "^5.6.2",
"webpack": "^5.74.0",
"webpack-cli": "^4.10.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/bundler/contracts/Import.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ pragma solidity ^0.8.0;

// import contracts to get their type info.
import "@account-abstraction/utils/contracts/test/SampleRecipient.sol";
import "@account-abstraction/contracts/samples/SimpleAccountFactory.sol";
import "@account-abstraction/contracts/accounts/SimpleAccountFactory.sol";
4 changes: 2 additions & 2 deletions packages/bundler/contracts/tests/TestRecursionAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ contract TestRecursionAccount is TestRuleAccount {
function runRule(string memory rule) public virtual override returns (uint) {

if (eq(rule, "handleOps")) {
PackedUserOperation[] memory ops = new PackedUserOperation[](0);
ep.handleOps(ops, payable(address (1)));
//handleOps is protected by reentrancy guard. check other blocked calls to EntryPoint
ep.getDepositInfo(address(0));
return 0;
}

Expand Down
24 changes: 21 additions & 3 deletions packages/bundler/src/runBundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ import { MethodHandlerERC4337 } from './MethodHandlerERC4337'

import { initServer } from './modules/initServer'
import { DebugMethodHandler } from './DebugMethodHandler'
import { supportsDebugTraceCall, supportsNativeTracer } from '@account-abstraction/validation-manager'
import {
AA_NONCE_MANAGER,
AA_STAKE_MANAGER,
supportsDebugTraceCall,
supportsNativeTracer
} from '@account-abstraction/validation-manager'
import { resolveConfiguration } from './Config'
import { bundlerConfigDefault } from './BundlerConfig'
import { parseEther } from 'ethers/lib/utils'
Expand Down Expand Up @@ -155,8 +160,14 @@ export async function runBundler (argv: string[], overrideExit = true): Promise<

if (config.rip7560) {
try {
await deployNonceManager(provider, wallet as any)
await deployStakeManager(provider, wallet as any)
const nonceManager = await deployNonceManager(provider, wallet as any)
if (nonceManager.address !== AA_NONCE_MANAGER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a util function or at lest do .toLowerCase() consistently when comparing addresses - the amount of time I wasted on these kinds of bugs is just sad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok for now, but I think its better to make one-pass to return always toChecksum addresses, and then just compare - instead of little the code with huge amount of toLowerCase, which makes it far less readable..

throw new Error(`NonceManager deployed at ${nonceManager.address} does not match constant AA_NONCE_MANAGER=${AA_NONCE_MANAGER}`)
}
const stakeManager = await deployStakeManager(provider, wallet as any)
if (stakeManager.address !== AA_STAKE_MANAGER) {
throw new Error(`StakeManager deployed at ${stakeManager.address} does not match constant AA_STAKE_MANAGER=${AA_NONCE_MANAGER}`)
}
} catch (e: any) {
console.warn(e)
if (!(e.message as string).includes('replacement fee too low') && !(e.message as string).includes('already known')) throw e
Expand All @@ -166,6 +177,13 @@ export async function runBundler (argv: string[], overrideExit = true): Promise<
const {
entryPoint
} = await connectContracts(wallet, !config.rip7560)

if (entryPoint != null && entryPoint?.address !== config.entryPoint && [1337, 31337].includes(chainId)) {
console.log('NOTICE: overriding config entrypoint: ', { entryPoint: entryPoint.address })
Copy link
Contributor

Choose a reason for hiding this comment

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

Please console.warn it - seems like something you don't actually want most of the time.

config.entryPoint = entryPoint.address
config.senderCreator = await entryPoint.senderCreator()
}

// bundleSize=1 replicate current immediate bundling mode
const execManagerConfig = {
...config
Expand Down
8 changes: 6 additions & 2 deletions packages/bundler/test/BundlerServer.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { JsonRpcProvider } from '@ethersproject/providers'
import { ethers } from 'hardhat'
import { expect } from 'chai'
import { parseEther } from 'ethers/lib/utils'

Expand All @@ -24,14 +23,19 @@ import { MethodHandlerERC4337 } from '../src/MethodHandlerERC4337'
import { BundlerConfig } from '../src/BundlerConfig'
import { DepositManager } from '../src/modules/DepositManager'
import { ERC7562Parser } from '@account-abstraction/validation-manager/dist/src/ERC7562Parser'
import { ethers } from 'hardhat'

describe('BundleServer', function () {
let entryPoint: IEntryPoint
let server: BundlerServer
before(async () => {
const provider = ethers.provider
const signer = await createSigner()
entryPoint = await deployEntryPoint(provider)
try {
entryPoint = await deployEntryPoint(provider)
} catch (e) {
throw new Error(`Failed to deploy entry point - no RPC node?\n ${e as string}`)
}

const config: BundlerConfig = {
chainId: 1337,
Expand Down
2 changes: 1 addition & 1 deletion packages/bundler/test/UserOpMethodHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('UserOpMethodHandler', function () {
chainId: 1337,
beneficiary: await signer.getAddress(),
entryPoint: entryPoint.address,
senderCreator: '0xefc2c1444ebcc4db75e7613d20c6a62ff67a167c',
senderCreator: await entryPoint.senderCreator(),
gasFactor: '0.2',
minBalance: '0',
mnemonic: '',
Expand Down
28 changes: 15 additions & 13 deletions packages/bundler/test/ValidateManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe('#ValidationManager', () => {
const unsafe = !await supportsDebugTraceCall(provider, false)
const preVerificationGasCalculator = new PreVerificationGasCalculator(MainnetConfig)

const senderCreator = '0xefc2c1444ebcc4db75e7613d20c6a62ff67a167c'
const senderCreator = await entryPoint.senderCreator()
const erc7562Parser = new ERC7562Parser(entryPoint.address, senderCreator)
vm = new ValidationManager(entryPoint, unsafe, preVerificationGasCalculator, erc7562Parser)

Expand Down Expand Up @@ -261,21 +261,21 @@ describe('#ValidationManager', () => {
})

it('should reject request with past validUntil', async () => {
await expect(
testTimeRangeUserOp(0, Date.now() - 1000)
).to.be.revertedWith('already expired')
expect(
await testTimeRangeUserOp(0, Date.now() - 1000).catch(e => e.message)
).match(/expired/)
})

it('should reject request with short validUntil', async () => {
await expect(
testTimeRangeUserOp(0, Date.now() + 25000)
).to.be.revertedWith('expires too soon')
expect(
await testTimeRangeUserOp(0, Date.now() + 25000).catch(e => e.message)
).to.match(/expires too soon/)
})

it('should reject request with future validAfter', async () => {
await expect(
testTimeRangeUserOp(Date.now() * 2, 0)
).to.be.revertedWith('future ')
expect(
await testTimeRangeUserOp(Date.now() * 2, 0).catch(e => e.message)
).to.match(/not due/)
})
})

Expand Down Expand Up @@ -347,6 +347,7 @@ describe('#ValidationManager', () => {
// await entryPoint.depositTo(pm.address, { value: parseEther('0.1') })
// await pm.addStake(entryPoint.address, { value: parseEther('0.1') })
const acct = await new TestRecursionAccount__factory(ethersSigner).deploy(entryPoint.address)
await acct.deployTransaction.wait()

const userOp: UserOperation = {
...cEmptyUserOp,
Expand All @@ -361,6 +362,7 @@ describe('#ValidationManager', () => {

it('should fail if validation recursively calls handleOps', async () => {
const acct = await new TestRecursionAccount__factory(ethersSigner).deploy(entryPoint.address)
await acct.deployTransaction.wait()
const op: UserOperation = {
...cEmptyUserOp,
sender: acct.address,
Expand Down Expand Up @@ -390,9 +392,9 @@ describe('#ValidationManager', () => {

it('should throw for a transaction that violates the rules', async () => {
const userOp = await createTestUserOp('coinbase')
await expect(
checkRulesViolations(provider, userOp, entryPoint.address)
).to.be.revertedWith('account uses banned opcode: COINBASE')
expect(
await checkRulesViolations(provider, userOp, entryPoint.address).catch(e => e.message)
).to.match(/account uses banned opcode: COINBASE/)
})
})
})
2 changes: 1 addition & 1 deletion packages/sdk/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { HardhatUserConfig } from 'hardhat/config'

const config: HardhatUserConfig = {
solidity: {
version: '0.8.23',
version: '0.8.28',
settings: {
optimizer: { enabled: true }
}
Expand Down
1 change: 1 addition & 0 deletions packages/sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@ethersproject/providers": "^5.7.0",
"@types/debug": "^4.1.7",
"debug": "^4.3.4",
"ethereumjs-util": "^7.1.5",
"ethers": "^5.7.0"
},
"devDependencies": {
Expand Down
20 changes: 13 additions & 7 deletions packages/sdk/src/BaseAccountAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { ethers, BigNumber, BigNumberish, BytesLike } from 'ethers'
import { Provider } from '@ethersproject/providers'

import { TransactionDetailsForUserOp } from './TransactionDetailsForUserOp'
import { defaultAbiCoder } from 'ethers/lib/utils'
import { defaultAbiCoder, hexConcat, hexDataSlice } from 'ethers/lib/utils'
import { PaymasterAPI } from './PaymasterAPI'
import { encodeUserOp, getUserOpHash, IEntryPoint, IEntryPoint__factory, UserOperation } from '@account-abstraction/utils'

Expand Down Expand Up @@ -120,12 +120,13 @@ export abstract class BaseAccountAPI {
if (factory == null) {
throw new Error(('no counter factual address if not factory'))
}
// use entryPoint to query account address (factory can provide a helper method to do the same, but
// this method attempts to be generic
const retAddr = await this.provider.call({
to: factory, data: factoryData
const getSenderAddressData = this.entryPointView.interface.encodeFunctionData('getSenderAddress', [hexConcat([factory, factoryData ?? '0x'])])
Copy link
Contributor

@forshtat forshtat Feb 13, 2025

Choose a reason for hiding this comment

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

Why don't we use to the factory contract object here? Manually encoding and decoding this data is not pretty.

const senderAddressResult = await this.provider.call({
to: this.entryPointAddress,
data: getSenderAddressData
})
const [addr] = defaultAbiCoder.decode(['address'], retAddr)
// the result is "error SenderAddressResult(address)", so remove methodsig first.
const [addr] = defaultAbiCoder.decode(['address'], hexDataSlice(senderAddressResult, 4))
return addr
}

Expand Down Expand Up @@ -205,7 +206,12 @@ export abstract class BaseAccountAPI {
if (factoryParams == null) {
return 0
}
return await this.provider.estimateGas({ to: factoryParams.factory, data: factoryParams.factoryData })
const senderCreator = await this.entryPointView.senderCreator()
return await this.provider.estimateGas({
from: senderCreator,
to: factoryParams.factory,
data: factoryParams.factoryData
})
}

/**
Expand Down
5 changes: 4 additions & 1 deletion packages/sdk/src/SimpleAccountAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { arrayify } from 'ethers/lib/utils'
import { Signer } from '@ethersproject/abstract-signer'
import { BaseApiParams, BaseAccountAPI, FactoryParams } from './BaseAccountAPI'
import { ecsign, toRpcSig } from 'ethereumjs-util'

/**
* constructor params, added no top of base params:
Expand Down Expand Up @@ -100,6 +101,8 @@ export class SimpleAccountAPI extends BaseAccountAPI {
}

async signUserOpHash (userOpHash: string): Promise<string> {
return await this.owner.signMessage(arrayify(userOpHash))
const privateKey = (this.owner as any).privateKey
const sig = ecsign(Buffer.from(arrayify(userOpHash)), Buffer.from(arrayify(privateKey)))
return toRpcSig(sig.v, sig.r, sig.s)
Comment on lines +104 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change made and how is it related to this issue?

}
}
31 changes: 31 additions & 0 deletions packages/utils/contracts/GetStakes.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
pragma solidity >=0.8;
// SPDX-License-Identifier: GPL-3.0

import "@account-abstraction/contracts/interfaces/IEntryPoint.sol";

struct StakeInfo {
address addr;
uint256 stake;
uint256 unstakeDelaySec;
}

error StakesRet(StakeInfo[] stakes);

// helper: get stake info of multiple entities.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use OpenZeppelin's Multicall contract here instead of reinventing the wheel?
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Multicall.sol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

multicall has to be deployed, and only then can be called.
This contract never gets deployed (in fact, it can't, since it reverts in its constructor)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but also this is a bit of reinventing the wheel. Maybe we should just give in and deploy the Multicall the same way we deploy EntryPoint? On real networks with real bundlers, it will just be an address config parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we could use multicall, but:

  1. need to deploy it, at least once
  2. it loses typechecking. I return an array of structs.

// This contract is never deployed: it is called using eth_call, and it reverts with the result...
contract GetStakes {

constructor(IEntryPoint entryPoint, address[] memory addrs) {
StakeInfo[] memory stakes = getStakes(entryPoint, addrs);
revert StakesRet(stakes);
}

function getStakes(IEntryPoint entryPoint, address[] memory addrs) public view returns (StakeInfo[] memory) {
StakeInfo[] memory stakes = new StakeInfo[](addrs.length);
for (uint256 i = 0; i < addrs.length; i++) {
IStakeManager.DepositInfo memory info = entryPoint.getDepositInfo(addrs[i]);
stakes[i] = StakeInfo(addrs[i], info.stake, info.unstakeDelaySec);
}
return stakes;
}
}
7 changes: 4 additions & 3 deletions packages/utils/contracts/Imports.sol
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;
pragma solidity ^0.8;

import "@account-abstraction/contracts/core/EntryPointSimulations.sol";
import "@account-abstraction/contracts/interfaces/IStakeManager.sol";
import "@account-abstraction/contracts/samples/SimpleAccountFactory.sol";
import "@account-abstraction/contracts/samples/TokenPaymaster.sol";
import "@account-abstraction/contracts/accounts/SimpleAccountFactory.sol";
import "@account-abstraction/rip7560/contracts/predeploys/Rip7560StakeManager.sol";
import "@account-abstraction/rip7560/contracts/interfaces/IRip7560Account.sol";
import "@account-abstraction/rip7560/contracts/interfaces/IRip7560Paymaster.sol";

import {NonceManager as NonceManagerRIP7712} from "@account-abstraction/rip7560/contracts/predeploys/NonceManager.sol";
2 changes: 1 addition & 1 deletion packages/utils/contracts/test/SampleRecipient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity ^0.8.0;

// TODO: get hardhat types from '@account-abstraction' package directly
// only to import the file in hardhat compilation
import "@account-abstraction/contracts/samples/SimpleAccount.sol";
import "@account-abstraction/contracts/accounts/SimpleAccount.sol";

contract SampleRecipient {

Expand Down
3 changes: 2 additions & 1 deletion packages/utils/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ const config: HardhatUserConfig = {
target: 'ethers-v5'
},
solidity: {
version: '0.8.23',
version: '0.8.28',
settings: {
evmVersion: 'cancun',
optimizer: { enabled: true }
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"@ethersproject/abstract-signer": "^5.7.0",
"@ethersproject/keccak256": "^5.7.0",
"@ethersproject/providers": "^5.7.0",
"@openzeppelin/contracts": "^5.0.1",
"@openzeppelin/contracts": "^5.2.0",
"debug": "^4.3.4",
"ethereumjs-util": "^7.1.5",
"ethers": "^5.7.0",
Expand Down
Loading
Loading