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.

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

Refactor executeCall to use executeUnsignedEOACall #65

Merged
merged 18 commits into from
Apr 2, 2020
Merged

Refactor executeCall to use executeUnsignedEOACall #65

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9a594cb
Attempt to use unsignedEOACall instead of executeCall
karlfloersch Mar 24, 2020
7091ec7
Remove double encoding of `methodIds.makeCall`
masonforest Mar 31, 2020
c6a2bd4
rename executeUnsignedEOACall to executeTransaction
masonforest Mar 31, 2020
9f05d1a
Refactor `executeCall` test helper method
masonforest Apr 1, 2020
4209f92
Remove assembly used for debugging
masonforest Apr 1, 2020
2aec960
Run a successful ovmCALL
masonforest Apr 1, 2020
a34a630
Pass all call tests with executeCall using executeTransaction
masonforest Apr 1, 2020
9b76077
Merge executeTransaction an executeTransaction2
masonforest Apr 2, 2020
cbbdcb3
Remove unused components of executeCall
masonforest Apr 2, 2020
68a21f2
Rename executeCall to executeTransactionRaw
masonforest Apr 2, 2020
dd97da2
Revert changes to returndatacopy
masonforest Apr 2, 2020
cf0f73a
Rebase and pass tests
masonforest Apr 2, 2020
62eee97
Revert newline changes
masonforest Apr 2, 2020
0dafb58
Lint
masonforest Apr 2, 2020
1b5ee7a
Rename `executeUnsignedEOACall` to `executeTransaction`
masonforest Apr 2, 2020
362532a
Use hex in assembly
masonforest Apr 2, 2020
f5b346f
Rename callExecutionManagerExecuteUnsignedEOACall
masonforest Apr 2, 2020
eb219d8
Update @notice to reflect changes to the function name
masonforest Apr 2, 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
62 changes: 23 additions & 39 deletions packages/ovm/src/contracts/ExecutionManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ contract ExecutionManager is FullStateManager {
// bitwise right shift 28 * 8 bits so the 4 method ID bytes are in the right-most bytes
bytes32 constant ovmCallMethodId = keccak256("ovmCALL()") >> 224;
bytes32 constant ovmCreateMethodId = keccak256("ovmCREATE()") >> 224;
bytes32 constant executeCallMethodId = keccak256("executeCall()") >> 224;

// Precompile addresses
address constant l2ToL1MessagePasserOvmAddress = 0x4200000000000000000000000000000000000000;
Expand Down Expand Up @@ -118,52 +117,37 @@ contract ExecutionManager is FullStateManager {
* [callBytes (bytes (variable length))]
* returndata: [variable-length bytes returned from call]
*/
function executeCall() external {
function executeTransactionRaw() external {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would consider massaging the @notice above here for clarity as it currently says "Execute a call" and we are renaming the function from executeCall

uint _timestamp;
uint _queueOrigin;
uint callSize;
bytes memory callBytes;
bytes32 methodId = ovmCallMethodId;
uint _callSize;
bytes memory _callBytes;
address _ovmEntrypoint;
Copy link

Choose a reason for hiding this comment

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

I had been going with the convention of _expectedCalldataParameters and regularVariables to go along with regular solidity convention. callBytes is certainly a hybrid. Just a thought -- I'm open to whatever though.

assembly {
// Revert if we don't have methodId, timestamp, queueOrigin, and ovmEntrypointAddress.
if lt(calldatasize, 100) {
revert(0,0)
}

// populate timestamp and queue origin from calldata
_timestamp := calldataload(4)
// skip method ID (bytes4) and timestamp (bytes32)
_queueOrigin := calldataload(0x24)
_queueOrigin := calldataload(add(4, 32))
Copy link

Choose a reason for hiding this comment

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

I think we should either be all hex or all decimal. Also, any reason for adding the extra add instruction?


callBytes := mload(0x40)
_callBytes := mload(0x40)
// set callsize: total param size minus 2 uints (methodId bytes are repurposed)
callSize := sub(calldatasize, 0x40)
mstore(0x40, add(callBytes, callSize))

// leave room for method ID, skip ahead in calldata methodID(4), timestamp(32), queueOrigin(32)
calldatacopy(add(callBytes, 4), 0x44, sub(callSize, 4))

mstore8(callBytes, shr(24, methodId))
mstore8(add(callBytes, 1), shr(16, methodId))
mstore8(add(callBytes, 2), shr(8, methodId))
mstore8(add(callBytes, 3), methodId)
}

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

address addr = address(this);
assembly {
let success := call(gas, addr, 0, callBytes, callSize, 0, 0)
let result := mload(0x40)
returndatacopy(result, 0, returndatasize)

if eq(success, 0) {
revert(result, returndatasize)
}
_callSize := sub(calldatasize, 0x40)
mstore(0x40, add(_callBytes, _callSize))

return(result, returndatasize)
_ovmEntrypoint := calldataload(68)
calldatacopy(add(_callBytes, 32), 100, sub(_callSize, 4))
mstore(_callBytes, sub(_callSize, 36))
Copy link

Choose a reason for hiding this comment

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

Same with all hex or all decimal. I'm cool whichever route, though.

}

return executeTransaction(
_timestamp,
_queueOrigin,
_ovmEntrypoint,
_callBytes,
ZERO_ADDRESS,
ZERO_ADDRESS,
true
);
}

/********************
Expand Down Expand Up @@ -201,7 +185,7 @@ contract ExecutionManager is FullStateManager {
require(_nonce == getOvmContractNonce(eoaAddress), "Incorrect nonce!");
emit CallingWithEOA(eoaAddress);
// Make the EOA call for the account
executeUnsignedEOACall(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress, ZERO_ADDRESS, false);
executeTransaction(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress, ZERO_ADDRESS, false);
}

/**
Expand All @@ -214,7 +198,7 @@ contract ExecutionManager is FullStateManager {
* @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(
function executeTransaction(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above WRT @notice

uint _timestamp,
uint _queueOrigin,
address _ovmEntrypoint,
Expand Down
Loading