Implement EIP-5920 #1225
Implement EIP-5920 #1225
Conversation
gurukamath
force-pushed
the
eips/osaka/eip-5920
branch
from
d1113c3 to
6dd18e4
Compare
gurukamath
force-pushed
the
eips/osaka/eip-5920
branch
from
6dd18e4 to
aa0bb3b
Compare
| @@ -22,6 +22,9 @@ | |||
| from ethereum.utils.byte import left_pad_zero_bytes | |||
|
|
|||
| from ..fork_types import Address | |||
| from ..vm.exceptions import InvalidParameter | |||
|
|
|||
| MAX_ADDRESS = Address(b"\xff" * 20) | |||
There was a problem hiding this comment.
As optimization you could add MAX_ADDRESS_UINT256 here as a constant
| address : `Address` | ||
| The obtained address. | ||
| """ | ||
| if data > U256.from_be_bytes(MAX_ADDRESS): |
There was a problem hiding this comment.
And then query the MAX_ADDRESS_UINT256 here 😄 👍
|
|
||
| # OPERATION | ||
| if evm.message.is_static: | ||
| raise WriteInStaticContext("Cannot PAY in static context") |
There was a problem hiding this comment.
In the spec, this check is done before any other operation (so also before popping any value from stack). I don't think moving this around in this method would yield a different outcome, but it would match the spec better to do this check prior to anything else. So I would suggest moving this check up.
There was a problem hiding this comment.
Yes. This does not make a difference to the outcome since all the gas in consumed and warmed addresses are discarded upon error. However, I implemented it in this order since that makes it consistent with the implementation of CALL* and other methods where you check if the frame is static.
| if to in evm.accessed_addresses: | ||
| access_gas_cost = GAS_WARM_ACCESS | ||
| else: | ||
| evm.accessed_addresses.add(to) |
There was a problem hiding this comment.
If the method encounters an error later, is the to address removed from warm addresses if it got added here?
There was a problem hiding this comment.
It will continue to be a part of the warm addresses in the current execution frame but will not be added to the warm addresses of the parent frame (contrast incorporate_child_on_success vs incorporate_child_on_error here). This means the warmed up address play no further role in the execution.
If we are in the initial frame, the warmed addresses are not explicitly removed. However, since an error is basically the end of the execution in that case, the warmed addresses play not further role here either.
| if to in evm.accessed_addresses: | ||
| access_gas_cost = GAS_WARM_ACCESS | ||
| else: | ||
| evm.accessed_addresses.add(to) | ||
| access_gas_cost = GAS_COLD_ACCOUNT_ACCESS |
There was a problem hiding this comment.
| if to in evm.accessed_addresses: | |
| access_gas_cost = GAS_WARM_ACCESS | |
| else: | |
| evm.accessed_addresses.add(to) | |
| access_gas_cost = GAS_COLD_ACCOUNT_ACCESS | |
| access_gas_cost = GAS_WARM_ACCESS | |
| if to not in evm.accessed_addresses: | |
| access_gas_cost = GAS_COLD_ACCOUNT_ACCESS | |
| evm.accessed_addresses.add(to) |
Personally I find this more readable, but I'm weird.
There was a problem hiding this comment.
This is just the way we define access_gas_cost in other opcodes. I do not see a big difference one way or another, so am ok with either representation
| try: | ||
| move_ether( | ||
| evm.message.block_env.state, | ||
| evm.message.current_target, | ||
| to, | ||
| value, | ||
| ) | ||
| push(evm.stack, U256(1)) | ||
| except AssertionError: | ||
| push(evm.stack, U256(0)) |
There was a problem hiding this comment.
We should not be using exception handling.
| try: | |
| move_ether( | |
| evm.message.block_env.state, | |
| evm.message.current_target, | |
| to, | |
| value, | |
| ) | |
| push(evm.stack, U256(1)) | |
| except AssertionError: | |
| push(evm.stack, U256(0)) | |
| sender_balance = get_account( | |
| evm.message.block_env.state, evm.message.current_target | |
| ).balance | |
| if sender_balance < value: | |
| push(evm.stack, U256(0)) | |
| else: | |
| move_ether( | |
| evm.message.block_env.state, | |
| evm.message.current_target, | |
| to, | |
| value, | |
| ) | |
| push(evm.stack, U256(1)) |
There was a problem hiding this comment.
I would have also prefered to not catch a general AssertionError but since the check is already performed in move_ether, I think we should just take advantage of that. Perhaps, raising a more specific exception in move_ether like InsufficientBalance and trying to catch that is a better way to go.
(closes #1181 )
What was wrong?
The current Osaka branch does not have the
PAYopcode implementation which has been CFI'd for OsakaRelated to Issue #1181
How was it fixed?
Implement EIP-5920
Cute Animal Picture