AA-440 validate using handleOps #243

drortirosh · 2025-02-02T18:39:00Z

using handleOps (not EntryPointSimulations) for UserOp validation.

shahafn · 2025-02-02T19:10:54Z

.gitmodules

@@ -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 = callValidateUserOp-catch-revert


This is temporary, no?

yes, though the new value will be "develop", until we complete releases/v0.8

shahafn · 2025-02-02T19:18:35Z

packages/bundler/test/BundlerServer.test.ts

@@ -29,9 +28,14 @@ describe('BundleServer', function () {
  let entryPoint: IEntryPoint
  let server: BundlerServer
  before(async () => {
-    const provider = ethers.provider
+    // const provider = ethers.provider


shahafn · 2025-02-02T19:19:18Z

packages/bundler/test/BundlerServer.test.ts

+    try {
+      entryPoint = await deployEntryPoint(provider)
+    } catch (e) {
+      throw new Error('Failed to deploy entry point - no geth?\n' + e)


Geth or just a node?

shahafn · 2025-02-02T19:42:26Z

packages/validation-manager/src/ValidationManager.ts

  // standard eth_call to simulateValidation
-  async _callSimulateValidation (userOp: UserOperation): Promise<ValidationResult> {


Remove the comment above and rename

shahafn · 2025-02-02T19:44:10Z

packages/validation-manager/src/ValidationManager.ts

-      [this.entryPoint.address]: {
-        code: EntryPointSimulationsJson.deployedBytecode
-      },
+      // [this.entryPoint.address]: {


shahafn · 2025-02-02T19:44:32Z

packages/validation-manager/src/ValidationManager.ts

@@ -194,9 +252,10 @@ export class ValidationManager implements IValidationManager {
    //   return [data as any, tracerResult]
    // }
    try {
-      const [decodedSimulations] = entryPointSimulations.decodeFunctionResult('simulateValidation', data)
-      const validationResult = this.parseValidationResult(userOp, decodedSimulations)
+      // const [decodedSimulations] = entryPointSimulations.decodeFunctionResult('simulateValidation', data)


shahafn · 2025-02-02T19:48:14Z

packages/validation-manager/src/ValidationManager.ts

@@ -110,27 +148,40 @@ export class ValidationManager implements IValidationManager {
    }
  }

+  allOnes = '0x'.padEnd(66, 'f')
+
+  // a "flag" UserOperation that triggers "AA94" revert error (not even FailedOp)


Can you explain the rationale? That we inject a reverting userOp to avoid simulating the execution, and explicitly mention that revert on the eolUserOp means success?

the handleOps validate the userop before calling it.
Thus FailedOp(index=1, "AA94") means it is an invalid UserOp
is it better to have a revert inside validateUserOp ?

Okay, I need more context to understand what is going on here.

forshtat · 2025-02-13T12:44:13Z

packages/sdk/src/BaseAccountAPI.ts

-    // 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'])])


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

forshtat · 2025-02-13T12:45:29Z

packages/sdk/src/SimpleAccountAPI.ts

+    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)


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

forshtat · 2025-02-13T12:47:51Z

packages/utils/contracts/GetStakes.sol

+
+    error StakesRet(StakeInfo[] stakes);
+
+// helper: get stake info of multiple entities.


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

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)

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.

yes, we could use multicall, but:

need to deploy it, at least once

it loses typechecking. I return an array of structs.

forshtat · 2025-02-13T12:48:00Z

packages/utils/contracts/Imports.sol


 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/samples/TokenPaymaster.sol";


forshtat · 2025-02-13T12:52:33Z

packages/validation-manager/src/ERC7562Parser.ts

+  private mapAddrToName (mapAddrs: {[name: string]: string}, addr: string): string {
+    if (addr == null) {
+      return addr
+    }
+    for (const name of Object.keys(mapAddrs)) {
+      if (mapAddrs[name]?.toString().toLowerCase() === addr.toLowerCase()) {
+        return name
+      }
+    }
+    return addr
+  }
+
+  // recursively dump call tree, and storage accesses
+  dumpCallTree (call: ERC7562Call, mapAddrs = {}, indent = ''): void {
+    const map = (addr: string): string => this.mapAddrToName(mapAddrs, addr)
+    debug(`${indent} ${map(call.from)} => ${call.type} ${call.to} ${map(call.to)}.${this._tryDetectKnownMethod(call)}`)
+    for (const access of ['reads', 'writes']) {
+      const arr = (call.accessedSlots as any)[access]
+      if (arr != null) {
+        for (const [idx, val] of Object.entries(arr)) {
+          debug(`${indent}   - ${access}  ${idx}: ${val as string}`)
+        }
+      }
+    }
+    for (const innerCall of call.calls ?? []) {
+      this.dumpCallTree(innerCall, mapAddrs, indent + '  ')
+    }
+  }
+


This code is basically a utility function and should not be added to the ERC7562Parser class itself.

I suggest extracting these two functions, as well as tryDetectKnownMethod, into Utils.
Also, how is this related to AA-440 ?

forshtat · 2025-02-13T13:03:15Z

packages/validation-manager/src/ValidationManager.ts

+
+    const ret: ValidationResult = {
+      returnInfo: {
+        sigFailed: false, // can't fail here, since handleOps didn't revert.


since handleOps didn't revert

Is it for sure? I was under impression that the debug_traceCall will return correctly with the trace results even if everything was reverted in the end. I may be wrong.

simulateValidation returns the validation result, even on failure.
since we use handleOps, it doesn't continue after a signature error - it reverts...

forshtat · 2025-02-13T13:03:44Z

packages/validation-manager/src/ValidationManager.ts

+    const paymasterAddress = op.paymaster ?? AddressZero
+    const factoryAddress = op.factory ?? AddressZero
+    const addrs = [op.sender, paymasterAddress, factoryAddress, aggregator]
+    const retStakes = (await runContractScript(this.provider, new GetStakes__factory(), [this.entryPoint.address, addrs]))[0]


Sorry, what??

this is the usage of GetStake. will refactor.

forshtat · 2025-02-13T13:04:35Z

packages/validation-manager/src/ValidationManager.ts

-      paymasterInfo: fillEntity(userOp.paymaster, res.paymasterInfo),
-      factoryInfo: fillEntity(userOp.factory, res.factoryInfo),
-      aggregatorInfo: fillEntity(res.aggregatorInfo.aggregator, res.aggregatorInfo.stakeInfo)
+    if (op.paymaster !== null) {


Use != instead of !== maybe, to catch undefined and false ?

fixed (though linter will report it when we're done.

forshtat · 2025-02-13T13:05:29Z

packages/validation-manager/src/ValidationManager.ts

@@ -110,27 +148,40 @@ export class ValidationManager implements IValidationManager {
    }
  }

+  allOnes = '0x'.padEnd(66, 'f')
+
+  // a "flag" UserOperation that triggers "AA94" revert error (not even FailedOp)


Okay, I need more context to understand what is going on here.

forshtat · 2025-02-13T13:06:20Z

packages/validation-manager/src/ValidationManager.ts

+        }
+
+        if (decodedError.startsWith('FailedOp(1,"AA94 gas values overflow')) {
+          // this is not an error.. it is a marker the UserOp-under-test passed successfully


using different compiler settings. also, sanity-check addresses

forshtat · 2025-02-20T11:26:52Z

lerna.json

@@ -1,5 +1,4 @@
 {
  "version": "0.6.0",
-  "npmClient": "yarn",
-  "useWorkspaces": true


Don't we use lerna workspaces?

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

forshtat · 2025-02-20T11:29:39Z

packages/bundler/src/runBundler.ts

-      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) {


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.

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..

forshtat · 2025-02-20T11:30:23Z

packages/bundler/src/runBundler.ts

@@ -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 })


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

forshtat · 2025-02-20T11:33:53Z

packages/utils/contracts/GetStakes.sol

+
+    error StakesRet(StakeInfo[] stakes);
+
+// helper: get stake info of multiple entities.


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.

forshtat · 2025-02-20T11:39:54Z

packages/validation-manager/src/ValidationManager.ts

+  // generate validation result from trace: by decoding inner calls.
+  async generateValidationResult (op: UserOperation, tracerResult: ERC7562Call): Promise<ValidationResult> {
+    // const validationData = tracerResult.calls[0].output
+    const validateUserOpCallIndex = op.factory == null ? 0 : 1


I would like to recommend that you add another utility function that turns the tracerResult.calls array to a named object like:

{ senderCall: {}, paymasterCall: {}, factoryCall: null }

It is taxing for the brain to deal with the code like const validateUserOpCallIndex = op.factory == null ? 0 : 1; const validatePaymasterCallIndex = validateUserOpCallIndex + 1, what do you think?

forshtat · 2025-02-20T11:42:39Z

packages/validation-manager/src/ValidationManager.ts

-  async _callSimulateValidation (userOp: UserOperation): Promise<ValidationResult> {
-    // Promise<IEntryPointSimulations.ValidationResultStructOutput> {
-    const data = entryPointSimulations.encodeFunctionData('simulateValidation', [packUserOp(userOp)])
+  async _simulateHandleOps (userOp: UserOperation): Promise<void> {


Maybe don't call it "simulate" if there is still a contract called "simulation" but this contract is not used in this function?

this is part of the code that will get away when removing unsafe.

forshtat · 2025-02-20T11:46:42Z

packages/validation-manager/src/ValidationManager.ts

@@ -288,7 +340,23 @@ export class ValidationManager implements IValidationManager {
      }
    } else {
      // NOTE: this mode doesn't do any opcode checking and no stake checking!
-      res = await this._callSimulateValidation(userOp)
+      // can't decode validationResult at all. we only have "pass-or-not" result


Do we still need the "unsafe" mode? The entire reason we keep this bundler repo maintained is as a test stub for the validation rules, isn't that so?

unsafe was for internal testing. and actually, now with "handleOps" validation, unsafe can't be fully implemented (one of the reason to use simulateValidation was to expose inner fields that are only accessible via tracing).
I suggest removing it in a separte PR.

forshtat · 2025-02-20T11:48:33Z

packages/validation-manager/src/decodeHelper.ts

+
+const debug = Debug('aa.dump')
+
+export function base64Tohex (input: string): string {


Are we the ones who encoded the data in base64 in native 7562 tracer in the first place, or does it come from go-ethereum developers?

drortirosh added 2 commits February 2, 2025 00:43

initial code, not fully compiles.

695cb26

changes

64e60bb

shahafn reviewed Feb 2, 2025

View reviewed changes

most #ValidationManager tests

ae02267

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from 9271c17 to ae02267 Compare February 3, 2025 15:29

drortirosh added 4 commits February 3, 2025 19:46

fix tests

ee39c5e

pass spec-test

0314372

checkdep

f9c37f2

update build.yml

2055b72

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from b7d163f to 2055b72 Compare February 4, 2025 16:24

drortirosh added 3 commits February 4, 2025 18:48

lean comments

6f2972a

update lerna

3545105

updated depcheck

9df986b

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from 82261b7 to 9df986b Compare February 4, 2025 17:44

update submodules

c5eebfa

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from 23017c9 to c5eebfa Compare February 4, 2025 18:43

drortirosh added 2 commits February 5, 2025 10:55

restore provider

4ed0ca0

lints

56482d1

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from 51b8e19 to 56482d1 Compare February 5, 2025 11:13

drortirosh added 3 commits February 10, 2025 03:18

update submodule (aa)

8df579a

lints

114fef8

Merge branch 'master' into AA-440-validate-without-simulateValidation

1dab49f

forshtat requested changes Feb 13, 2025

View reviewed changes

drortirosh added 7 commits February 13, 2025 17:06

Merge branch 'master' into AA-440-validate-without-simulateValidation

f2c4a86

update pkg

be7b10f

7702 overrides in simulation

b1f2042

updated comment.

cc66bcc

Merge branch 'master' into AA-440-validate-without-simulateValidation

7ef33d3

reference account-abstraction#develop

8ffdff8

pr fixes

dc8320c

drortirosh added 3 commits February 19, 2025 15:05

new account-abstraction contracts

acf965d

ts type

f616408

dumpCalls

c85351a

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from a84afcc to c85351a Compare February 19, 2025 15:56

fix 7560stakeManager, nonceManager addresses

1e3b92e

using different compiler settings. also, sanity-check addresses

drortirosh force-pushed the AA-440-validate-without-simulateValidation branch from 4d022d8 to 1e3b92e Compare February 19, 2025 18:20

forshtat reviewed Feb 20, 2025

View reviewed changes

		// standard eth_call to simulateValidation
		async _callSimulateValidation (userOp: UserOperation): Promise<ValidationResult> {


		error StakesRet(StakeInfo[] stakes);

		// helper: get stake info of multiple entities.


		const debug = Debug('aa.dump')

		export function base64Tohex (input: string): string {

AA-440 validate using handleOps #243

Are you sure you want to change the base?

AA-440 validate using handleOps #243

Conversation

drortirosh commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat Feb 13, 2025 •

edited

Loading