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

Remove duplicate Solidity sig check in fullnode proxy #23

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 11 additions & 10 deletions packages/ovm/src/contracts/ExecutionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ contract ExecutionManager is FullStateManager {
}

// Initialize our context
initializeContext(_timestamp, _queueOrigin);
initializeContext(_timestamp, _queueOrigin, ZERO_ADDRESS);

address addr = address(this);
assembly {
Expand Down Expand Up @@ -194,10 +194,8 @@ contract ExecutionManager is FullStateManager {
// Require nonce to be correct
require(_nonce == getOvmContractNonce(eoaAddress), "Incorrect nonce!");
emit CallingWithEOA(eoaAddress);
executionContext.ovmTxOrigin = eoaAddress;
// Make the EOA call for the account
executeUnsignedEOACall(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress);
executionContext.ovmTxOrigin = ZERO_ADDRESS;
executeUnsignedEOACall(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress, false);
}

/**
Expand All @@ -208,17 +206,19 @@ contract ExecutionManager is FullStateManager {
* @param _ovmEntrypoint The contract which this transaction should be executed against.
* @param _callBytes The calldata for this ovm transaction.
* @param _fromAddress The address which this call should originate from--the msg.sender.
* @param _allowRevert Flag which controls whether or not to revert in the case of failure.
*/
function executeUnsignedEOACall(
uint _timestamp,
uint _queueOrigin,
address _ovmEntrypoint,
bytes memory _callBytes,
address _fromAddress
address _fromAddress,
bool _allowRevert
) public {
uint _nonce = getOvmContractNonce(_fromAddress);
// Initialize our context
initializeContext(_timestamp, _queueOrigin);
initializeContext(_timestamp, _queueOrigin, _fromAddress);

// Set the active contract to be our EOA address
switchActiveContract(_fromAddress);
Expand Down Expand Up @@ -256,7 +256,6 @@ contract ExecutionManager is FullStateManager {
mstore8(add(_callBytes, 3), methodId)
}

bool isCall = executionContext.ovmTxOrigin == ZERO_ADDRESS;
bool success = false;
address addr = address(this);
bytes memory result;
Expand All @@ -269,7 +268,7 @@ contract ExecutionManager is FullStateManager {
if eq(success, 1) {
return(resultData, returndatasize)
}
if eq(isCall, 1) {
if eq(_allowRevert, 1) {
revert(resultData, returndatasize)
}
mstore(result, returndatasize)
Expand Down Expand Up @@ -503,13 +502,15 @@ contract ExecutionManager is FullStateManager {
* NOTE: this zeroing may not technically be needed as the context should always end up as zero at the end of each execution.
* @param _timestamp The timestamp which should be used for this context.
* @param _queueOrigin The queue which this context's transaction was sent from.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an @param comment for _ovmTxOrigin

* @param _ovmTxOrigin The tx.origin for the currently executing transaction. It will be ZERO_ADDRESS if it's not an EOA call.
*/
function initializeContext(uint _timestamp, uint _queueOrigin) internal {
function initializeContext(uint _timestamp, uint _queueOrigin, address _ovmTxOrigin) internal {
// First zero out the context for good measure (Note ZERO_ADDRESS is reserved for the genesis contract & initial msgSender)
restoreContractContext(ZERO_ADDRESS, ZERO_ADDRESS);
// And finally set the timestamp & queue origin
// And finally set the timestamp, queue origin, & tx origin
executionContext.timestamp = _timestamp;
executionContext.queueOrigin = _queueOrigin;
executionContext.ovmTxOrigin = _ovmTxOrigin;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ describe('Execution Manager -- Call opcodes', () => {
0,
transaction.to,
transaction.data,
ZERO_ADDRESS
ZERO_ADDRESS,
true
)
await provider.waitForTransaction(tx.hash)
})
Expand Down Expand Up @@ -236,6 +237,7 @@ describe('Execution Manager -- Call opcodes', () => {
dummyContractAddress,
internalCalldata,
wallet.address,
true,
]
)
const nonce = await wallet.getTransactionCount()
Expand Down Expand Up @@ -276,6 +278,7 @@ describe('Execution Manager -- Call opcodes', () => {
dummyContractAddress,
internalCalldata,
wallet.address,
true,
]
)
const nonce = await wallet.getTransactionCount()
Expand Down
3 changes: 2 additions & 1 deletion packages/ovm/test/contracts/simple-storage.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ describe('SimpleStorage', () => {
executionManager,
wallet,
simpleStorageOvmAddress,
innerCallData
innerCallData,
true
)
}

Expand Down
3 changes: 2 additions & 1 deletion packages/ovm/test/govm/simple-storage.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ describe('SimpleStorage', () => {
executionManager,
wallet,
simpleStorageOvmAddress,
innerCallData
innerCallData,
true
)
}

Expand Down
9 changes: 6 additions & 3 deletions packages/ovm/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ export const manuallyDeployOvmContractReturnReceipt = async (
executionManager,
wallet,
undefined,
initCode
initCode,
false
)

return internalTxReceiptToOvmTxReceipt(receipt)
Expand Down Expand Up @@ -105,7 +106,8 @@ export const executeUnsignedEOACall = async (
executionManager: Contract,
wallet: Wallet,
to: Address,
data: string
data: string,
allowRevert: boolean
): Promise<TransactionReceipt> => {
Comment on lines 106 to 111
Copy link
Contributor

Choose a reason for hiding this comment

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

A function comment for executeUnsignedEOACall describing what each of the parameters is would be helpful! (Specifically about allowRevert)

// Verify that the transaction is not accidentally sending to the ZERO_ADDRESS
if (to === ZERO_ADDRESS) {
Expand All @@ -121,7 +123,8 @@ export const executeUnsignedEOACall = async (
0,
ovmTo,
data,
wallet.address
wallet.address,
allowRevert
)
// Return the parsed transaction values
return executionManager.provider.waitForTransaction(tx.hash)
Expand Down
69 changes: 29 additions & 40 deletions packages/rollup-full-node/src/app/web3-rpc-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,13 @@ export class DefaultWeb3Handler implements Web3Handler, FullnodeHandler {
)}], defaultBlock: [${defaultBlock}]`
)
// First generate the internalTx calldata
const internalCalldata = this.generateUnsignedCallCalldata(
const internalCalldata = this.getTransactionCalldata(
this.getTimestamp(),
0,
txObject['to'],
txObject['data'],
txObject['from']
txObject['from'],
true
)

log.debug(`calldata: ${internalCalldata}`)
Expand All @@ -188,7 +189,7 @@ export class DefaultWeb3Handler implements Web3Handler, FullnodeHandler {
// Then actually make the call and get the response
response = await this.provider.send(Web3RpcMethods.call, [
{
from: ZERO_ADDRESS,
from: this.wallet.address,
to: this.executionManager.address,
data: internalCalldata,
},
Expand Down Expand Up @@ -222,19 +223,20 @@ export class DefaultWeb3Handler implements Web3Handler, FullnodeHandler {
)}], defaultBlock: [${defaultBlock}]`
)
// First generate the internalTx calldata
const internalCalldata = this.generateUnsignedCallCalldata(
const internalCalldata = this.getTransactionCalldata(
this.getTimestamp(),
0,
txObject['to'],
txObject['data'],
txObject['from']
txObject['from'],
true
)

log.debug(internalCalldata)
// Then estimate the gas
const response = await this.provider.send(Web3RpcMethods.estimateGas, [
{
from: ZERO_ADDRESS,
from: this.wallet.address,
to: this.executionManager.address,
data: internalCalldata,
},
Expand Down Expand Up @@ -480,16 +482,23 @@ export class DefaultWeb3Handler implements Web3Handler, FullnodeHandler {
// Generate the calldata which we'll use to call our internal execution manager
// First pull out the `to` field (we just need to check if it's null & if so set ovmTo to the zero address as that's how we deploy contracts)
const ovmTo = ovmTx.to === null ? ZERO_ADDRESS : ovmTx.to
// Check the nonce
const expectedNonce = (
await this.executionManager.getOvmContractNonce(ovmTx.from)
).toNumber()
if (expectedNonce !== ovmTx.nonce) {
throw new Error(
`Incorrect nonce! Expected nonce: ${expectedNonce} but received nonce: ${ovmTx.nonce}`
)
}
// Construct the raw transaction calldata
const internalCalldata = this.generateEOACallCalldata(
const internalCalldata = this.getTransactionCalldata(
this.getTimestamp(),
0,
ovmTx.nonce,
ovmTo,
ovmTx.data,
ovmTx.v,
ovmTx.r,
ovmTx.s
ovmTx.from,
false
)

log.debug(`EOA calldata: [${internalCalldata}]`)
Expand Down Expand Up @@ -524,50 +533,30 @@ export class DefaultWeb3Handler implements Web3Handler, FullnodeHandler {
return executionManager
}

private generateUnsignedCallCalldata(
/**
* Get the calldata for an EVM transaction to the ExecutionManager.
*/
private getTransactionCalldata(
Copy link
Contributor

Choose a reason for hiding this comment

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

A short function comment would be helpful!

timestamp: number,
queueOrigin: number,
ovmEntrypoint: string,
callBytes: string,
fromAddress: string
fromAddress: string,
allowRevert: boolean
): string {
// Update the ovmEntrypoint to be the ZERO_ADDRESS if this is a contract creation
if (ovmEntrypoint === null || ovmEntrypoint === undefined) {
ovmEntrypoint = ZERO_ADDRESS
}
return this.executionManager.interface.functions[
'executeUnsignedEOACall'
].encode([timestamp, queueOrigin, ovmEntrypoint, callBytes, fromAddress])
}

private generateEOACallCalldata(
timestamp: number,
queueOrigin: number,
nonce: number,
ovmEntrypoint: string,
callBytes: string,
v: number,
r: string,
s: string
): string {
// Update the ovmEntrypoint to be the ZERO_ADDRESS if this is a contract creation
if (ovmEntrypoint === null || ovmEntrypoint === undefined) {
ovmEntrypoint = ZERO_ADDRESS
}

log.debug(
`Generating EOA Calldata: v: [${v}], r: [${r}], s: [${s}], nonce: [${nonce}] entrypoint: [${ovmEntrypoint}], callBytes: [${callBytes}]`
)

return this.executionManager.interface.functions['executeEOACall'].encode([
].encode([
timestamp,
queueOrigin,
nonce,
ovmEntrypoint,
callBytes,
v,
r,
s,
fromAddress,
allowRevert,
])
}

Expand Down