-
Notifications
You must be signed in to change notification settings - Fork 995
Implement EIP-5920 - PAY opcode #8498
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
Conversation
jochem-brouwer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw this on discord, did a quick skim-through of this :)
| final boolean accountIsWarm = frame.warmUpAddress(to); | ||
|
|
||
| if (frame.isStatic() && hasValue && !Objects.equals(frame.getSenderAddress(), to)) { | ||
| return new OperationResult(cost(to, true, recipient, true), ExceptionalHaltReason.ILLEGAL_STATE_CHANGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EIP will need clarification for this, but if you use the CALL opcode and you send value (also to self) in a static context, this is not allowed and will throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but isn't it what StaticCall actually is? A Call with no value that does not change any state. But EIP should be clarified for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can argue that static calls don't do logs though so you be correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but isn't it what StaticCall actually is? A Call with no value that does not change any state. But EIP should be clarified for sure
Note that I'm talking about a PAY with value, but the PAY is actually to the current address. So the state does not change. The EIP will need clarification for this. But, note for the CALL opcode that if value is being sent in a STATICCALL context, the opcode will exceptionally abort (so even though no state is changed if it would be executed).
I will open a PR for this against the EIP.
|
|
||
| final Bytes toAddressBytes = frame.getStackItem(1); | ||
| if (toAddressBytes.size() > 20 | ||
| && toAddressBytes.numberOfLeadingZeroBytes() < 12) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the stack trim zeros by default? If not, what if I push 21 bytes to the stack with a leading 0 and an address of 20 nonzero bytes? This throws, but should be valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PUSH0 PUSH21 00<20 nonzero bytes> PAY maybe?
| } | ||
|
|
||
| final Address to = Words.toAddress(toAddressBytes); | ||
| final Wei value = Wei.wrap(frame.getStackItem(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| if (accountIsWarm | ||
| //TODO: what about precompile accounts? | ||
| || gasCalculator().isPrecompile(to)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks good to know. This is just the way Besu does it not sure why but in the end it's the same thing.
cd7bd61 to
8a9e838
Compare
Thanks for the review, fixes a few things based on that |
|
This pr is stale because it has been open for 30 days with no activity. |
macfarla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lu-pinto what do we have to change to make this independent of EOF?
Just adding it at the right hardfork configuration and aligning the return value with current EVM opcodes (*CALL). |
76adc11 to
7c9b02b
Compare
d26b61b to
42876ba
Compare
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
Signed-off-by: Luis Pinto <luis.pinto@consensys.net>
91e61a3 to
8056897
Compare


PR description
Implement new PAY opcode
Fixed Issue(s)
fixes #8438
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests