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

Fix typo in Hooked.js #398

Closed
wants to merge 2 commits into from
Closed

Fix typo in Hooked.js #398

wants to merge 2 commits into from

Conversation

Kyrrui
Copy link

@Kyrrui Kyrrui commented Nov 28, 2018

Working on a project that was getting an error from cache being undefined, realized in stateManager cache is initialized as such
"self._cache = new Cache(self._trie);" with a preceding underscore.

Working on a project that was getting an error from cache being undefined, realized in stateManager cache is initialized as such 
"self._cache = new Cache(self._trie);"  with a preceding underscore.
@holgerd77
Copy link
Member

Hi Kyle @Kyrrui, thanks for reporting this! The StateManager has been reworked in the v2.5.0 release and we are now on the path towards a stable external StateManger API.

I haven't thought about this hooked.js part along with this (and I haven't been in contact with the code yet), but as far as I read from the comment on top this file should probably also marked as DEPRECATED and removed in a v3.0.0 release, @mattdean-digicatapult do you have a stance on this?

So as an in between note for you (@Kyrrui) and others using hooked.js: you should probably have some look into the StateManager API once time permits and see if you can update your code to factor out the hooked.js usage and use StateManager directly. It would be very valuable feedback if you tell us about your experiences and how well this worked out (or not).

As a short-term path I would suggest that we merge the PR here and do a minor release, so that people currently using hooked.js are not left behind, together with the DEPRECATION note. Than current users get some more time to get their code ready.

Does this make sense?

@Kyrrui
Copy link
Author

Kyrrui commented Nov 29, 2018

@holgerd77 Sounds good to me - although we are not using your library directly, instead we are using Metamask's web3-provider-engine which calls fromWeb3Provider in hooked.js
https://github.com/MetaMask/provider-engine/blob/abd9663419034be70fce8da9f0fbdd6e736b7c1d/subproviders/vm.js#L116
So would have to reach out to them to deprecate or update code.

CC @kumavis

@Kyrrui Kyrrui closed this Nov 29, 2018
@Kyrrui Kyrrui reopened this Nov 29, 2018
@mattdean-digicatapult
Copy link
Contributor

I think that makes sense @holgerd77. hooked.js should be deprecated and a second stateManager implementation created that exposes the required features.

One question though is should we just deprecate it here and an external project can implement the new stateManager impl or should ethereumjs-vm be providing this functionality?

@holgerd77
Copy link
Member

This was re-submitted here #434, have merged the new PR since the one here is out-of-date with the base branch, will close.

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.

4 participants