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

add DUP17-32 and SWAP17-32 instructions to expand local variable number #40

Merged
merged 9 commits into from
Mar 25, 2019

Conversation

aion-camus
Copy link
Contributor

relax the local variable limit, support up to 32 items in local stack.

Copy link
Contributor

@iamyulong iamyulong left a 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.

@aion-camus
Copy link
Contributor Author

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.

@iamyulong
Copy link
Contributor

LGTM

@aionick
Copy link
Contributor

aionick commented Jan 22, 2019

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).

@AionJayT AionJayT changed the base branch from master to master-latestRelease January 22, 2019 20:32
@aion-camus
Copy link
Contributor Author

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.

@aionick
Copy link
Contributor

aionick commented Jan 23, 2019

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,
Copy link
Contributor

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.

@AionJayT AionJayT changed the base branch from master-latestRelease to master March 19, 2019 14:42
@AionJayT AionJayT merged commit 890964a into master Mar 25, 2019
@AionJayT AionJayT deleted the solidity_enhancement branch April 8, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants