diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol index ddc44baf6..3f880b795 100644 --- a/contracts/core/EntryPoint.sol +++ b/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)); } } } diff --git a/contracts/interfaces/IEntryPoint.sol b/contracts/interfaces/IEntryPoint.sol index 2bde4c1df..2b48ce7e8 100644 --- a/contracts/interfaces/IEntryPoint.sol +++ b/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); /** diff --git a/contracts/samples/TokenPaymaster.sol b/contracts/samples/TokenPaymaster.sol index 632f1e374..48c085597 100644 --- a/contracts/samples/TokenPaymaster.sol +++ b/contracts/samples/TokenPaymaster.sol @@ -42,8 +42,6 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper { event UserOperationSponsored(address indexed user, uint256 actualTokenCharge, uint256 actualGasCost, uint256 actualTokenPrice); - event PostOpReverted(address indexed user, uint256 preCharge); - event Received(address indexed sender, uint256 value); /// @notice All 'price' variables are multiplied by this value to avoid rounding up diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index 9249f5389..0be941e8a 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -4,7 +4,7 @@ ╔══════════════════════════╤════════╗ ║ gas estimate "simple" │ 29002 ║ ╟──────────────────────────┼────────╢ -║ gas estimate "big tx 5k" │ 125260 ║ +║ gas estimate "big tx 5k" │ 125248 ║ ╚══════════════════════════╧════════╝ ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗ @@ -12,28 +12,28 @@ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 81934 │ │ ║ +║ simple │ 1 │ 81930 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 2 │ │ 44191 │ 15189 ║ +║ simple - diff from previous │ 2 │ │ 44187 │ 15185 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 479794 │ │ ║ +║ simple │ 10 │ 479718 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 11 │ │ 44203 │ 15201 ║ +║ simple - diff from previous │ 11 │ │ 44223 │ 15221 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 88232 │ │ ║ +║ simple paymaster │ 1 │ 88187 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 43183 │ 14181 ║ +║ simple paymaster with diff │ 2 │ │ 43126 │ 14124 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 477132 │ │ ║ +║ simple paymaster │ 10 │ 476670 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 43254 │ 14252 ║ +║ simple paymaster with diff │ 11 │ │ 43173 │ 14171 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 182991 │ │ ║ +║ big tx 5k │ 1 │ 182963 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 2 │ │ 144702 │ 19442 ║ +║ big tx - diff from previous │ 2 │ │ 144686 │ 19438 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1485426 │ │ ║ +║ big tx 5k │ 10 │ 1485374 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 11 │ │ 144739 │ 19479 ║ +║ big tx - diff from previous │ 11 │ │ 144759 │ 19511 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts index 24a0bd878..dc235517d 100644 --- a/test/entrypoint.test.ts +++ b/test/entrypoint.test.ts @@ -286,7 +286,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') } }) @@ -334,7 +334,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') } } }) @@ -359,7 +359,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') } } }) @@ -726,7 +726,7 @@ describe('EntryPoint', function () { verificationGasLimit: 10000 }, accountOwner, entryPoint) await expect(simulateValidation(op1, entryPoint.address)) - .to.revertedWith('AA23 reverted (or OOG)') + .to.revertedWith('AA23 reverted') }) }) diff --git a/test/entrypointsimulations.test.ts b/test/entrypointsimulations.test.ts index 8920057ab..2ef719d73 100644 --- a/test/entrypointsimulations.test.ts +++ b/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 () => { diff --git a/test/gnosis.test.ts b/test/gnosis.test.ts index 24819edf0..b763e1abb 100644 --- a/test/gnosis.test.ts +++ b/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 () { diff --git a/test/paymaster.test.ts b/test/paymaster.test.ts index 5fcb410e6..d58063974 100644 --- a/test/paymaster.test.ts +++ b/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 () => { diff --git a/test/samples/TokenPaymaster.test.ts b/test/samples/TokenPaymaster.test.ts index 4a1e94117..007528882 100644 --- a/test/samples/TokenPaymaster.test.ts +++ b/test/samples/TokenPaymaster.test.ts @@ -49,6 +49,11 @@ 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 @@ -56,12 +61,12 @@ describe('TokenPaymaster', function () { 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]) }) diff --git a/test/testutils.ts b/test/testutils.ts index 8307ce9af..d8c79a416 100644 --- a/test/testutils.ts +++ b/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,7 +157,18 @@ 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) @@ -165,25 +176,31 @@ export function decodeRevertReason (data: string, nullIfNoMatch = true): string 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 { // 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 { diff --git a/test/verifying_paymaster.test.ts b/test/verifying_paymaster.test.ts index 5103f2cb6..6c8ee7294 100644 --- a/test/verifying_paymaster.test.ts +++ b/test/verifying_paymaster.test.ts @@ -9,7 +9,7 @@ import { } from '../typechain' import { createAccount, - createAccountOwner, createAddress, + createAccountOwner, createAddress, decodeRevertReason, deployEntryPoint } from './testutils' import { fillAndSign, simulateValidation } from './UserOp' @@ -58,7 +58,9 @@ describe('EntryPoint with VerifyingPaymaster', function () { sender: account.address, paymasterAndData: hexConcat([paymaster.address, defaultAbiCoder.encode(['uint48', 'uint48'], [MOCK_VALID_UNTIL, MOCK_VALID_AFTER]), '0x1234']) }, accountOwner, entryPoint) - await expect(simulateValidation(userOp, entryPoint.address)).to.be.revertedWith('invalid signature length in paymasterAndData') + expect(await simulateValidation(userOp, entryPoint.address) + .catch(e => decodeRevertReason(e))) + .to.include('invalid signature length in paymasterAndData') }) it('should reject on invalid signature', async () => { @@ -66,7 +68,9 @@ describe('EntryPoint with VerifyingPaymaster', function () { sender: account.address, paymasterAndData: hexConcat([paymaster.address, defaultAbiCoder.encode(['uint48', 'uint48'], [MOCK_VALID_UNTIL, MOCK_VALID_AFTER]), '0x' + '00'.repeat(65)]) }, accountOwner, entryPoint) - await expect(simulateValidation(userOp, entryPoint.address)).to.be.revertedWith('ECDSA: invalid signature') + expect(await simulateValidation(userOp, entryPoint.address) + .catch(e => decodeRevertReason(e))) + .to.include('ECDSA: invalid signature') }) describe('with wrong signature', () => {