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

Deprecate createHookedVm #435

Closed
s1na opened this issue Feb 5, 2019 · 10 comments
Closed

Deprecate createHookedVm #435

s1na opened this issue Feb 5, 2019 · 10 comments

Comments

@s1na
Copy link
Contributor

s1na commented Feb 5, 2019

As per the jsdoc, createHookedVm is "a helper for creating a vm with modified state lookups, this should be made obsolete by a better public API for StateManager". Given the recent changes in StateManager, I'm wondering if the use-cases that prompted this helper are already covered by the new API, and if not, what should be added to allow deprecating createHookedVm?

@holgerd77
Copy link
Member

I did searched on GitHub ethereumjs, ethereum (e.g. for remix) and trufflesuite organizations and as well as a google search for createHookedVm and found no references whatsoever. Not sure if we are running some phantom debate here. 😄 Eventually we can really just drop?

@s1na
Copy link
Contributor Author

s1na commented Feb 25, 2019

😆 Will create a PR

@holgerd77
Copy link
Member

Addressed in #451, will close.

@axic
Copy link
Member

axic commented Feb 26, 2019

It is used in Metamask's provider-engine: https://github.com/MetaMask/provider-engine/blob/master/subproviders/vm.js

Which was used to run gas estimation and calls on the client. Not sure if Metamask still does that or delegates it to Infura.

@axic
Copy link
Member

axic commented Feb 26, 2019

What his means is ethereumjs-vm should have a version bump to 3.x.

@holgerd77
Copy link
Member

holgerd77 commented Feb 26, 2019

//cc @whymarrh

@holgerd77
Copy link
Member

//cc @danfinlay

@holgerd77
Copy link
Member

@axic Yes, I was thinking about doing some minor version bump release integrating the latest refactoring work, which is all not really breaking but might nevertheless cause the one or the other challenges depending on how deep people integrate with the VM code.

@axic
Copy link
Member

axic commented Feb 26, 2019

Well removing the hooked vm I would say is a major bump since it removes functionality in a breaking way.

@holgerd77
Copy link
Member

holgerd77 commented Feb 26, 2019

Ah sorry, I thought you were referring to the x in 3.x, didn't have a close-enough look, yes, probably a major version bump is justified (necessary).

Think we'll generally wait a bit with the release so that there is some more time for discussion, also @s1na can eventually do some more of the integrations of #424 he is planning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants