-
Notifications
You must be signed in to change notification settings - Fork 15
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
add DUP17-32 and SWAP17-32 instructions to expand local variable number #40
Conversation
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.
I think there are two things missing:
- For the FastVM, there is no way to run pre-change and post-change modes at the same time. We can't provide two copies of binaries to the kernel. Suggest using the
evm_revision
enum. - There is no tests to verify the changes are valid or not.
New revision number is named AION_V1 to let kernel know the current version of fvm. It should be under discussion for release schedule. Local variable contract for test is uploaded. |
LGTM |
I'm good with this, though I would like to see the "proof" of the test contract you wrote (an actual JUnit test that compiles it and uses it). |
It is tested under Rust Kernel. In Java kernel, I just use run_v1 in FastVM module to lauch the new fvm. Anyone who works on Java kernel interface could have a better solution. |
Gotcha. I will be fixing up some old fvm tests and adding some new ones in the coming weeks for all of this avm integration, so I'll add some for this. |
@@ -449,6 +449,7 @@ enum evm_revision { | |||
EVM_BYZANTIUM = 4, | |||
EVM_AION = 5, | |||
EVM_CONSTANTINOPLE = 6, | |||
EVM_AION_V1 = 7, |
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.
I'd suggest we remove EVM_CONSTANTINOPLE
becauses it's not implemented yet, and assign 6 to EVM_AION_V1.
relax the local variable limit, support up to 32 items in local stack.