Skip to content

Surfacing revert messages for eth_call and eth_sendRawTransaction #107

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.

Sign up for GitHub

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

Merged
ben-chain merged 9 commits into from
Apr 26, 2020
Merged

Surfacing revert messages for eth_call and eth_sendRawTransaction #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0e4fb48
pass RevertErrors up in full node
ben-chain Apr 24, 2020
413a98f
revert messages working, receipts not
ben-chain Apr 24, 2020
291a4d5
receipts working for failed txs
ben-chain Apr 24, 2020
f5cb2a0
test receipt generation for reverting txns'
ben-chain Apr 24, 2020
60afc74
bugfix, linting
ben-chain Apr 24, 2020
038c2ff
fix eth_call reversion errors
ben-chain Apr 25, 2020
9ce3e33
linting
ben-chain Apr 25, 2020
56da002
Add nonce incrementing test
karlfloersch Apr 26, 2020
cbc0ba9
Clean up unused code / whitespace
Apr 26, 2020
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
25 changes: 16 additions & 9 deletions packages/ovm/src/app/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export const convertInternalLogsToOvmLogs = (logs: Log[]): Log[] => {
* @param internalTxReceipt the internal transaction receipt
* @return ovm transaction metadata
*/
export const getOvmTransactionMetadata = (
export const getSuccessfulOvmTransactionMetadata = (
internalTxReceipt: TransactionReceipt
): OvmTransactionMetadata => {
let ovmTo
Expand All @@ -113,9 +113,6 @@ export const getOvmTransactionMetadata = (
.map((log) => executionManagerInterface.parseLog(log))
.filter((log) => log != null)
const callingWithEoaLog = logs.find((log) => log.name === 'CallingWithEOA')
const eoaContractCreatedLog = logs.find(
(log) => log.name === 'EOACreatedContract'
)

const revertEvents: LogDescription[] = logs.filter(
(x) => x.name === 'EOACallRevert'
Expand All @@ -124,7 +121,12 @@ export const getOvmTransactionMetadata = (

if (callingWithEoaLog) {
ovmFrom = callingWithEoaLog.values._ovmFromAddress
ovmTo = callingWithEoaLog.values._ovmToAddress
}

const eoaContractCreatedLog = logs.find(
(log) => log.name === 'EOACreatedContract'
)
if (eoaContractCreatedLog) {
ovmCreatedContractAddress = eoaContractCreatedLog.values._ovmContractAddress
ovmTo = ovmCreatedContractAddress
Expand Down Expand Up @@ -174,20 +176,25 @@ export const internalTxReceiptToOvmTxReceipt = async (
internalTxReceipt: TransactionReceipt,
ovmTxHash?: string
): Promise<OvmTransactionReceipt> => {
const ovmTransactionMetadata = getOvmTransactionMetadata(internalTxReceipt)
const ovmTransactionMetadata = getSuccessfulOvmTransactionMetadata(
internalTxReceipt
)
// Construct a new receipt
//
// Start off with the internalTxReceipt
const ovmTxReceipt: OvmTransactionReceipt = internalTxReceipt
// Add the converted logs
ovmTxReceipt.logs = convertInternalLogsToOvmLogs(internalTxReceipt.logs)
// Update the to and from fields
ovmTxReceipt.to = ovmTransactionMetadata.ovmTo
// Update the to and from fields if necessary
if (ovmTransactionMetadata.ovmTo) {
ovmTxReceipt.to = ovmTransactionMetadata.ovmTo
}
// TODO: Update this to use some default account abstraction library potentially.
ovmTxReceipt.from = ovmTransactionMetadata.ovmFrom
// Also update the contractAddress in case we deployed a new contract
ovmTxReceipt.contractAddress =
ovmTransactionMetadata.ovmCreatedContractAddress
ovmTxReceipt.contractAddress = !!ovmTransactionMetadata.ovmCreatedContractAddress
? ovmTransactionMetadata.ovmCreatedContractAddress
: null

ovmTxReceipt.status = ovmTransactionMetadata.ovmTxSucceeded ? 1 : 0

Expand Down
8 changes: 6 additions & 2 deletions packages/ovm/src/contracts/ExecutionManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ contract ExecutionManager is FullStateManager {
bytes32 _codeContractHash
);
event CallingWithEOA(
address _ovmFromAddress
address _ovmFromAddress,
address _ovmToAddress
);
event EOACreatedContract(
address _ovmContractAddress
Expand Down Expand Up @@ -137,7 +138,10 @@ contract ExecutionManager is FullStateManager {
require(eoaAddress != ZERO_ADDRESS, "Failed to recover signature");
// Require nonce to be correct
require(_nonce == getOvmContractNonce(eoaAddress), "Incorrect nonce!");
emit CallingWithEOA(eoaAddress);
emit CallingWithEOA(
eoaAddress,
_ovmEntrypoint
);
// Make the EOA call for the account
executeTransaction(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress, ZERO_ADDRESS, false);
}
Expand Down
34 changes: 8 additions & 26 deletions packages/ovm/test/app/utils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
import { TransactionReceipt } from 'ethers/providers'
import {
convertInternalLogsToOvmLogs,
getOvmTransactionMetadata,
getSuccessfulOvmTransactionMetadata,
OvmTransactionMetadata,
revertMessagePrefix,
} from '../../src/app'
Expand Down Expand Up @@ -43,13 +43,17 @@ describe('convertInternalLogsToOvmLogs', () => {
})
})

describe('getOvmTransactionMetadata', () => {
describe('getSuccessfulOvmTransactionMetadata', () => {
it('should return transaction metadata from calls from externally owned accounts', async () => {
const transactionReceipt: TransactionReceipt = {
byzantium: true,
logs: [
[EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]],
[EXECUTION_MANAGER_ADDRESS, 'CallingWithEOA(address)', [ALICE]],
[
EXECUTION_MANAGER_ADDRESS,
'CallingWithEOA(address,address)',
[ALICE, CONTRACT],
],
[EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]],
[EXECUTION_MANAGER_ADDRESS, 'EOACreatedContract(address)', [CONTRACT]],
[EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [CONTRACT]],
Expand All @@ -61,33 +65,11 @@ describe('getOvmTransactionMetadata', () => {
].map((args) => buildLog.apply(null, args)),
}

getOvmTransactionMetadata(transactionReceipt).should.deep.eq({
getSuccessfulOvmTransactionMetadata(transactionReceipt).should.deep.eq({
ovmCreatedContractAddress: CONTRACT,
ovmFrom: ALICE,
ovmTo: CONTRACT,
ovmTxSucceeded: true,
})
})

it('should return with ovmTxSucceeded equal to false if the transaction reverted', async () => {
const revertMessage: string = 'The tx reverted!'
const msgHex: string = add0x(
Buffer.from(revertMessage, 'utf8').toString('hex')
)
const encodedMessage: string = abi.encode(['bytes'], [msgHex])
// needs 4 bytes of sighash
const eventData: string = add0x('ab'.repeat(4) + remove0x(encodedMessage))
const transactionReceipt: TransactionReceipt = {
byzantium: true,
logs: [
[EXECUTION_MANAGER_ADDRESS, 'EOACallRevert(bytes)', [eventData]],
].map((args) => buildLog.apply(null, args)),
}

const metadata: OvmTransactionMetadata = getOvmTransactionMetadata(
transactionReceipt
)
metadata.ovmTxSucceeded.should.eq(false)
metadata.revertMessage.should.eq(revertMessagePrefix + revertMessage)
})
})
14 changes: 14 additions & 0 deletions packages/rollup-full-node/src/app/utils/l2-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,3 +212,17 @@ function getL2ToL1MessagePasserContract(wallet: Wallet): Contract {
wallet
)
}

/**
* Detects whether an internal L2 node error is due to an EVM revert or some other error
*
* @param e The error message reterned from either eth_sendRawTransaction or eth_call on the internal L2 node
* @returns Whether the error is an EVM revert error or some other issue.
*/
export function isErrorEVMRevert(e: any): boolean {
return (
!!e.results &&
!!Object.keys(e.results)[0] &&
e.results[Object.keys(e.results)[0]].error === 'revert'
)
}
73 changes: 57 additions & 16 deletions packages/rollup-full-node/src/app/web3-rpc-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ import {
UnsupportedMethodError,
Web3Handler,
Web3RpcMethods,
RevertError,
} from '../types'
import { initializeL2Node, getCurrentTime } from './utils'
import { initializeL2Node, getCurrentTime, isErrorEVMRevert } from './utils'
import { NoOpL2ToL1MessageSubmitter } from './message-submitter'

const log = getLogger('web3-handler')
Expand Down Expand Up @@ -241,10 +242,16 @@ export class DefaultWeb3Handler
])
} catch (e) {
log.debug(
`Error executing call: ${JSON.stringify(
`Internal error executing call: ${JSON.stringify(
txObject
)}, default block: ${defaultBlock}, error: ${JSON.stringify(e)}`
)
if (isErrorEVMRevert(e)) {
log.debug(
`Internal error appears to be an EVM revert, surfacing revert message up...`
)
throw new RevertError(e.message as string)
}
throw e
}

Expand Down Expand Up @@ -375,6 +382,9 @@ export class DefaultWeb3Handler
? ovmTx[key].toNumber()
: ovmTx[key]
}
if (typeof transaction[key] === 'number') {
transaction[key] = numberToHexString(transaction[key])
}
})

return transaction
Expand Down Expand Up @@ -536,18 +546,45 @@ export class DefaultWeb3Handler
return null
}

// Now let's parse the internal transaction reciept
const ovmTxReceipt: OvmTransactionReceipt = await internalTxReceiptToOvmTxReceipt(
internalTxReceipt,
ovmTxHash
log.debug(
`Converting internal tx receipt to ovm receipt, internal receipt is:`,
internalTxReceipt
)

// if there are no logs, the tx must have failed, as the Execution Mgr always logs stuff
const txSucceeded: boolean = internalTxReceipt.logs.length !== 0
let ovmTxReceipt
if (txSucceeded) {
log.debug(
`The internal tx previously succeeded for this OVM tx, converting internal receipt to OVM receipt...`
)
ovmTxReceipt = await internalTxReceiptToOvmTxReceipt(
internalTxReceipt,
ovmTxHash
)
} else {
log.debug(
`Internal tx previously failed for this OVM tx, creating receipt from the OVM tx itself.`
)
const rawOvmTx = await this.getOvmTransactionByHash(ovmTxHash)
const ovmTx = utils.parseTransaction(rawOvmTx)
// for a failing tx, everything is identical between the internal and external receipts, except to and from
ovmTxReceipt = internalTxReceipt
ovmTxReceipt.from = ovmTx.from
ovmTxReceipt.to = ovmTx.to
}

if (ovmTxReceipt.revertMessage !== undefined && !includeRevertMessage) {
delete ovmTxReceipt.revertMessage
}

if (typeof ovmTxReceipt.status === 'number') {
ovmTxReceipt.status = numberToHexString(ovmTxReceipt.status)
}

log.debug(
`Returning tx receipt for ovm tx hash [${ovmTxHash}]: [${JSON.stringify(
internalTxReceipt
ovmTxReceipt
)}]`
)
return ovmTxReceipt
Expand Down Expand Up @@ -614,16 +651,20 @@ export class DefaultWeb3Handler
[internalTx]
)
} catch (e) {
if (isErrorEVMRevert(e)) {
log.debug(
`Internal EVM revert for Ovm tx hash: ${ovmTxHash} and internal hash: ${internalTxHash}. Incrementing nonce Incrementing nonce for sender (${ovmTx.from}) and surfacing revert message up...`
)
await this.context.executionManager.incrementNonce(add0x(ovmTx.from))
log.debug(`Nonce incremented successfully for ${ovmTx.from}.`)
throw new RevertError(e.message as string)
}
logError(
log,
`Error executing transaction!\n\nIncrementing nonce for sender (${ovmTx.from} and returning failed tx hash. Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}.`,
`Non-revert error executing internal transaction! Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}. Returning generic internal error.`,
e
)

await this.context.executionManager.incrementNonce(add0x(ovmTx.from))
log.debug(`Nonce incremented successfully for ${ovmTx.from}.`)

return ovmTxHash
throw e
}

if (remove0x(internalTxHash) !== remove0x(returnedInternalTxHash)) {
Expand Down Expand Up @@ -704,7 +745,7 @@ export class DefaultWeb3Handler
)
throw e
}
log.debug(`L1 to L2 Transaction mined. Tx hash: ${receipt.hash}`)
log.debug(`L1 to L2 Transaction applied to L2. Tx hash: ${receipt.hash}`)

try {
const ovmTxReceipt: OvmTransactionReceipt = await internalTxReceiptToOvmTxReceipt(
Expand Down Expand Up @@ -804,7 +845,7 @@ export class DefaultWeb3Handler
.waitForTransaction(res.hash)
.then((receipt) => {
log.debug(
`Got receipt mapping ${ovmTxHash} to ${internalTxHash}: ${JSON.stringify(
`Got receipt mapping ovm tx hash ${ovmTxHash} to internal tx hash ${internalTxHash}: ${JSON.stringify(
receipt
)}`
)
Expand Down Expand Up @@ -898,7 +939,7 @@ export class DefaultWeb3Handler
ovmTx.data,
ovmFrom,
ZERO_ADDRESS,
false
true
)

log.debug(`EOA calldata: [${internalCalldata}]`)
Expand Down
1 change: 0 additions & 1 deletion packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
import * as SimpleStorage from '../contracts/build/untranspiled/SimpleStorage.json'
import * as EmptyContract from '../contracts/build/untranspiled/EmptyContract.json'
import * as CallerStorer from '../contracts/build/transpiled/CallerStorer.json'
import { getOvmTransactionMetadata } from '@eth-optimism/ovm'

const log = getLogger('test-web3-handler', true)

Expand Down
Loading