AA-243 FailedOpWithRevert (#385)

revert with "FailedOpWithRevert" when there is an inner revert, with the
entire, unparsed inner revert.

contracts/core/EntryPoint.sol

@@ -438,13 +438,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
                 }(op, opInfo.userOpHash, missingAccountFunds)
             returns (uint256 _validationData) {
                 validationData = _validationData;
-            } catch Error(string memory revertReason) {
-                revert FailedOp(
-                    opIndex,
-                    string.concat("AA23 reverted: ", revertReason)
-                );
             } catch {
-                revert FailedOp(opIndex, "AA23 reverted (or OOG)");
+                revert FailedOpWithRevert(opIndex, "AA23 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN));
             }
             if (paymaster == address(0)) {
                 DepositInfo storage senderInfo = deposits[sender];
@@ -503,13 +498,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
             returns (bytes memory _context, uint256 _validationData) {
                 context = _context;
                 validationData = _validationData;
-            } catch Error(string memory revertReason) {
-                revert FailedOp(
-                    opIndex,
-                    string.concat("AA33 reverted: ", revertReason)
-                );
             } catch {
-                revert FailedOp(opIndex, "AA33 reverted (or OOG)");
+                revert FailedOpWithRevert(opIndex, "AA33 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN));
             }
         }
     }

contracts/interfaces/IEntryPoint.sol

@@ -102,6 +102,15 @@ interface IEntryPoint is IStakeManager, INonceManager {
      */
     error FailedOp(uint256 opIndex, string reason);
 
+    /**
+     * A custom revert error of handleOps, to report a revert by account or paymaster.
+     * @param opIndex - Index into the array of ops to the failed one (in simulateValidation, this is always zero).
+     * @param reason  - Revert reason. see FailedOp(uint,string), above
+     * @param inner   - data from inner cought revert reason
+     * @dev note that inner is truncated to 2048 bytes
+     */
+    error FailedOpWithRevert(uint256 opIndex, string reason, bytes inner);
+
     error PostOpReverted(bytes returnData);
 
     /**

reports/gas-checker.txt

@@ -4,36 +4,36 @@
 ╔══════════════════════════╤════════╗
 ║ gas estimate "simple"    │  29014 ║
 ╟──────────────────────────┼────────╢
-║ gas estimate "big tx 5k" │ 125248 ║
+║ gas estimate "big tx 5k" │ 125260 ║
 ╚══════════════════════════╧════════╝
 
 ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗
 ║ handleOps description          │ count │ total gasUsed │ per UserOp gas │ per UserOp overhead ║
 ║                                │       │               │     (delta for │        (compared to ║
 ║                                │       │               │    one UserOp) │     account.exec()) ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │     1 │         81934 │                │                     ║
+║ simple                         │     1 │         81930 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │     2 │               │          44179 │               15165 ║
+║ simple - diff from previous    │     2 │               │          44187 │               15173 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple                         │    10 │        479746 │                │                     ║
+║ simple                         │    10 │        479694 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple - diff from previous    │    11 │               │          44203 │               15189 ║
+║ simple - diff from previous    │    11 │               │          44247 │               15233 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │     1 │         88232 │                │                     ║
+║ simple paymaster               │     1 │         88187 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │     2 │               │          43195 │               14181 ║
+║ simple paymaster with diff     │     2 │               │          43138 │               14124 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster               │    10 │        477072 │                │                     ║
+║ simple paymaster               │    10 │        476718 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ simple paymaster with diff     │    11 │               │          43266 │               14252 ║
+║ simple paymaster with diff     │    11 │               │          43185 │               14171 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │     1 │        182967 │                │                     ║
+║ big tx 5k                      │     1 │        182987 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │     2 │               │         144702 │               19454 ║
+║ big tx - diff from previous    │     2 │               │         144698 │               19438 ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx 5k                      │    10 │       1485426 │                │                     ║
+║ big tx 5k                      │    10 │       1485290 │                │                     ║
 ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
-║ big tx - diff from previous    │    11 │               │         144715 │               19467 ║
+║ big tx - diff from previous    │    11 │               │         144699 │               19439 ║
 ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
 

test/entrypoint.test.ts

@@ -287,7 +287,7 @@ describe('EntryPoint', function () {
         // if we get here, it means the userOp passed first sim and reverted second
         expect.fail(null, null, 'should fail on first simulation')
       } catch (e: any) {
-        expect(e.message).to.include('Revert after first validation')
+        expect(decodeRevertReason(e)).to.include('Revert after first validation')
       }
     })
 
@@ -335,7 +335,7 @@ describe('EntryPoint', function () {
             const tx = await entryPoint.handleOps([badOp], beneficiaryAddress, { gasLimit: 1e6 })
             await tx.wait()
           } else {
-            expect(e.message).to.include('AA23 reverted (or OOG)')
+            expect(decodeRevertReason(e)).to.include('AA23 reverted')
           }
         }
       })
@@ -360,7 +360,7 @@ describe('EntryPoint', function () {
             const tx = await entryPoint.handleOps([badOp], beneficiaryAddress, { gasLimit: 1e6 })
             await tx.wait()
           } else {
-            expect(e.message).to.include('AA23 reverted (or OOG)')
+            expect(decodeRevertReason(e)).to.include('AA23 reverted')
           }
         }
       })
@@ -727,7 +727,7 @@ describe('EntryPoint', function () {
           verificationGasLimit: 10000
         }, accountOwner, entryPoint)
         await expect(simulateValidation(op1, entryPoint.address))
-          .to.revertedWith('AA23 reverted (or OOG)')
+          .to.revertedWith('AA23 reverted')
       })
     })
 

test/entrypointsimulations.test.ts

@@ -142,7 +142,7 @@ describe('EntryPointSimulations', function () {
     it('should revert on oog if not enough verificationGas', async () => {
       const op = await fillAndSign({ sender: account.address, verificationGasLimit: 1000 }, accountOwner, entryPoint)
       await expect(simulateValidation(op, entryPoint.address)).to
-        .revertedWith('AA23 reverted (or OOG)')
+        .revertedWith('AA23 reverted')
     })
 
     it('should succeed if validateUserOp succeeds', async () => {

test/gnosis.test.ts

@@ -23,7 +23,7 @@ import {
   deployEntryPoint,
   getBalance,
   HashZero,
-  isDeployed
+  isDeployed, decodeRevertReason
 } from './testutils'
 import { fillAndSign } from './UserOp'
 import { defaultAbiCoder, hexConcat, hexZeroPad, parseEther } from 'ethers/lib/utils'
@@ -107,7 +107,10 @@ describe('Gnosis Proxy', function () {
 
     const anotherEntryPoint = await new EntryPoint__factory(ethersSigner).deploy()
 
-    await expect(anotherEntryPoint.handleOps([op], beneficiary)).to.revertedWith('account: not from entrypoint')
+    // await expect(
+    expect(await anotherEntryPoint.handleOps([op], beneficiary)
+      .catch(e => decodeRevertReason(e)))
+      .to.include('account: not from entrypoint')
   })
 
   it('should fail on invalid userop', async function () {

test/paymaster.test.ts

@@ -22,7 +22,7 @@ import {
   createAddress,
   ONE_ETH,
   createAccount,
-  getAccountAddress
+  getAccountAddress, decodeRevertReason
 } from './testutils'
 import { fillAndSign, simulateValidation } from './UserOp'
 import { hexConcat, parseEther } from 'ethers/lib/utils'
@@ -100,12 +100,14 @@ describe('EntryPoint with paymaster', function () {
           paymasterAndData: paymaster.address,
           callData: calldata
         }, accountOwner, entryPoint)
-        await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
+        expect(await entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
           gasLimit: 1e7
-        })).to.revertedWith('AA33 reverted: TokenPaymaster: no balance')
-        await expect(entryPoint.handleOps([op], beneficiaryAddress, {
+        }).catch(e => decodeRevertReason(e)))
+          .to.include('TokenPaymaster: no balance')
+        expect(await entryPoint.handleOps([op], beneficiaryAddress, {
           gasLimit: 1e7
-        })).to.revertedWith('AA33 reverted: TokenPaymaster: no balance')
+        }).catch(e => decodeRevertReason(e)))
+          .to.include('TokenPaymaster: no balance')
       })
     })
 
@@ -120,9 +122,10 @@ describe('EntryPoint with paymaster', function () {
           verificationGasLimit: 1e7,
           paymasterAndData: paymaster.address
         }, accountOwner, entryPoint)
-        await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
+        expect(await entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
           gasLimit: 1e7
-        }).catch(rethrow())).to.revertedWith('TokenPaymaster: no balance')
+        }).catch(e => decodeRevertReason(e)))
+          .to.include('TokenPaymaster: no balance')
       })
 
       it('should succeed to create account with tokens', async () => {

test/samples/TokenPaymaster.test.ts

@@ -49,19 +49,24 @@ function generatePaymasterAndData (pm: string, tokenPrice?: BigNumberish): strin
 
 const priceDenominator = BigNumber.from(10).pow(26)
 
+function uniq (arr: any[]): any[] {
+  // remove items with duplicate "name" attribute
+  return Object.values(arr.reduce((set, item) => ({ ...set, [item.name]: item }), {}))
+}
+
 describe('TokenPaymaster', function () {
   const minEntryPointBalance = 1e17.toString()
   const initialPriceToken = 100000000 // USD per TOK
   const initialPriceEther = 500000000 // USD per ETH
   const ethersSigner = ethers.provider.getSigner()
   const beneficiaryAddress = '0x'.padEnd(42, '1')
   const testInterface = new Interface(
-    [
+    uniq([
       ...TestUniswap__factory.abi,
       ...TestERC20__factory.abi,
       ...TokenPaymaster__factory.abi,
       ...EntryPoint__factory.abi
-    ]
+    ])
   )
 
   let chainId: number
@@ -148,15 +153,17 @@ describe('TokenPaymaster', function () {
       callData
     }, entryPoint)
     op = signUserOp(op, accountOwner, entryPoint.address, chainId)
-    await expect(
-      entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 })
-    ).to.be.revertedWith('AA33 reverted: ERC20: insufficient allowance')
+    // await expect(
+    expect(await entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 })
+      .catch(e => decodeRevertReason(e)))
+      .to.include('ERC20: insufficient allowance')
 
     await token.sudoApprove(account.address, paymaster.address, ethers.constants.MaxUint256)
 
-    await expect(
-      entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 })
-    ).to.revertedWith('AA33 reverted: ERC20: transfer amount exceeds balance')
+    expect(await entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 })
+      .catch(e => decodeRevertReason(e)))
+      .to.include('ERC20: transfer amount exceeds balance')
+
     await ethers.provider.send('evm_revert', [snapshot])
   })
 

test/testutils.ts

@@ -1,7 +1,7 @@
 import { ethers } from 'hardhat'
 import {
   arrayify,
-  hexConcat,
+  hexConcat, Interface,
   keccak256,
   parseEther
 } from 'ethers/lib/utils'
@@ -15,7 +15,7 @@ import {
   SimpleAccountFactory__factory,
   SimpleAccount__factory,
   SimpleAccountFactory,
-  TestAggregatedAccountFactory
+  TestAggregatedAccountFactory, TestPaymasterRevertCustomError__factory
 } from '../typechain'
 import { BytesLike } from '@ethersproject/bytes'
 import { expect } from 'chai'
@@ -157,33 +157,50 @@ export function rethrow (): (e: Error) => void {
   }
 }
 
-export function decodeRevertReason (data: string, nullIfNoMatch = true): string | null {
+const decodeRevertReasonContracts = new Interface([
+  ...[
+    ...EntryPoint__factory.createInterface().fragments,
+    ...TestPaymasterRevertCustomError__factory.createInterface().fragments
+  ].filter(f => f.type === 'error')
+]) //
+
+export function decodeRevertReason (data: string | Error, nullIfNoMatch = true): string | null {
+  if (typeof data !== 'string') {
+    const err = data as any
+    data = (err.data ?? err.error.data) as string
+  }
   const methodSig = data.slice(0, 10)
   const dataParams = '0x' + data.slice(10)
 
   if (methodSig === '0x08c379a0') {
     const [err] = ethers.utils.defaultAbiCoder.decode(['string'], dataParams)
     // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
     return `Error(${err})`
-  } else if (methodSig === '0x00fa072b') {
-    const [opindex, paymaster, msg] = ethers.utils.defaultAbiCoder.decode(['uint256', 'address', 'string'], dataParams)
-    // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
-    return `FailedOp(${opindex}, ${paymaster !== AddressZero ? paymaster : 'none'}, ${msg})`
   } else if (methodSig === '0x4e487b71') {
     const [code] = ethers.utils.defaultAbiCoder.decode(['uint256'], dataParams)
     return `Panic(${panicCodes[code] ?? code} + ')`
-  } else if (methodSig === '0x8d6ea8be') {
-    const [reason] = ethers.utils.defaultAbiCoder.decode(['string'], dataParams)
-    return `CustomError("${reason as string}")`
-  } else if (methodSig === '0xad7954bc') {
-    const [reasonBytes] = ethers.utils.defaultAbiCoder.decode(['bytes'], dataParams)
-    const reason = decodeRevertReason(reasonBytes)
-    return `PostOpReverted(${reason as string})`
   }
-  if (!nullIfNoMatch) {
-    return data
+
+  try {
+    const err = decodeRevertReasonContracts.parseError(data)
+    // let args: any[] = err.args as any
+
+    // treat any error "bytes" argument as possible error to decode (e.g. FailedOpWithRevert, PostOpReverted)
+    const args = err.args.map((arg: any, index) => {
+      switch (err.errorFragment.inputs[index].type) {
+        case 'bytes' : return decodeRevertReason(arg)
+        case 'string': return `"${(arg as string)}"`
+        default: return arg
+      }
+    })
+    return `${err.name}(${args.join(',')})`
+  } catch (e) {
+    // throw new Error('unsupported errorSig ' + data)
+    if (!nullIfNoMatch) {
+      return data
+    }
+    return null
   }
-  return null
 }
 
 let currentNode: string = ''
@@ -215,12 +232,14 @@ export async function checkForGeth (): Promise<void> {
 // becomes:
 // { first: "a", second: "20" }
 export function objdump (obj: { [key: string]: any }): any {
-  return Object.keys(obj)
-    .filter(key => key.match(/^[\d_]/) == null)
-    .reduce((set, key) => ({
-      ...set,
-      [key]: decodeRevertReason(obj[key].toString(), false)
-    }), {})
+  return obj == null
+    ? null
+    : Object.keys(obj)
+      .filter(key => key.match(/^[\d_]/) == null)
+      .reduce((set, key) => ({
+        ...set,
+        [key]: decodeRevertReason(obj[key].toString(), false)
+      }), {})
 }
 
 export async function checkForBannedOps (txHash: string, checkPaymaster: boolean): Promise<void> {