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

Make list of opcodes property of VM instance #592

Merged
merged 3 commits into from Sep 10, 2019
Merged

Make list of opcodes property of VM instance #592

merged 3 commits into from Sep 10, 2019

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Sep 9, 2019

Fixes #591, also related to #543

Instead of having a global list of opcodes in the opcodes.ts file, this PR sets this list as a property of the VM instance to avoid problems when two VM instances with different HFs are created.

I also made each opcode to be an object instead of a list. Objects are typed easily and are more flexible.

Note: If the Common instance of a VM instance is modified after construction, the list of opcodes will be out-dated.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Shouldn't _opcode rather be a property of the EVM instead of the VM? In the current implementation this is referencing back and forth between Interpreter and the VM object, introducing a new circular dependency.

lib/evm/interpreter.ts Show resolved Hide resolved
lib/evm/opcodes.ts Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
@s1na
Copy link
Contributor Author

s1na commented Sep 10, 2019

Shouldn't _opcode rather be a property of the EVM instead of the VM? In the current implementation this is referencing back and forth between Interpreter and the VM object, introducing a new circular dependency.

So it's possible to have the list of opcodes as a property of Interpreter, EVM or the VM. I don't have a strong preference here and will do as you think best.

The good thing about VM is that the list of opcodes doesn't need to be instantiated when running every tx (but this shouldn't be a expensive operation, so it's okay), and users could pass a list of modified opcodes to the VM. Between EVM and Interpreter I'm slightly in favor of Interpreter, because if it was stored in EVM we'd again need a circular dependency, whereas Interpreter has directly access to Common and could detect the HF and set the list.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 95.278% when pulling 4ce3b7a on hf-opcodes-in-vm into 9333e02 on master.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Ok, let's leave this "as is" for now - see there are good arguments in both directions. As long as we still have EVM and VM this tightly coupled this will do the job and if we decouple we'll likely have to do some further refactoring anyhow.

Will approve now.

@s1na s1na merged commit cf7601f into master Sep 10, 2019
@s1na s1na deleted the hf-opcodes-in-vm branch September 10, 2019 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching hardfork in VM overwrites the opcodes codes variable
4 participants