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

State manager API changes #309

Conversation

mattdean-digicatapult
Copy link
Contributor

This is to inform discussion of #268 as requested in that thread, but is not intended to be merged in it's current state.

@coveralls
Copy link

coveralls commented Jul 12, 2018

Coverage Status

Coverage increased (+0.2%) to 85.945% when pulling 0711147 on mattdean-digicatapult:stateManagerCleanup-hacks into 4ecae8a on ethereumjs:master.

@holgerd77
Copy link
Member

Oh, the state tests are actually passing on this? That’s already impressive, cool!

@mattdean-digicatapult
Copy link
Contributor Author

I think all the necessary tests should be passing, with the cache tests being commented out as the exception. Honestly I had to make my changes that way in order to make sure what I was doing was even viable. Not sure what's going on with the Travis build, but I'm pretty sure my changes aren't at fault

@axic
Copy link
Member

axic commented Jul 21, 2018

Sorry for that but since #266 was merged, this needs to be rebased.

mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Jul 23, 2018
Squashed changes as discussed in ethereumjs#309
mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Jul 23, 2018
Squashed changes as discussed in ethereumjs#309
@mattdean-digicatapult
Copy link
Contributor Author

Thanks @axic, I've rebased and everything still seems to work. A few of the changes from #266 seemed to conflict in direct opposition to my changes, notably in the moving of the blockchain instance onto stateManager from the vm. I've stuck with my way for reasons discussed in #268, but I think it would be good to work out where the blockchain should sit so that we're not trampling on each others changes.

@axic
Copy link
Member

axic commented Jul 25, 2018

I think to speed up the process of merging this we need to split it into smaller chunks. I can see one good first step is prefixing the soon-to-be-private variables with underscore (_cache, _trie) in a separate PR.

@mattdean-digicatapult
Copy link
Contributor Author

Unquestionably this needs splitting up before it can be merged. My intention with this PR was really just to facilitate the discussion in #268. If others are happy with the logic I presented there I'm more than happy to start breaking this up into separate PRs.

mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Aug 3, 2018
Squashed changes as discussed in ethereumjs#309
mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Aug 3, 2018
Squashed changes as discussed in ethereumjs#309
mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Aug 3, 2018
Squashed changes as discussed in ethereumjs#309
mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Aug 6, 2018
Squashed changes as discussed in ethereumjs#309
@holgerd77
Copy link
Member

Hi @mattdean-digicatapult, have sent you a message on Gitter which is time-critical, hijacking this thread to notify you on a second communcation means! 😄

mattdean-digicatapult added a commit to mattdean-digicatapult/ethereumjs-vm that referenced this pull request Sep 3, 2018
Squashed changes as discussed in ethereumjs#309
@mattdean-digicatapult mattdean-digicatapult force-pushed the stateManagerCleanup-hacks branch 3 times, most recently from 7d6b00d to d3f9552 Compare September 20, 2018 10:32
@holgerd77 holgerd77 changed the title State manager API changes [DO NOT MERGE] State manager API changes Sep 20, 2018
@mattdean-digicatapult
Copy link
Contributor Author

These changes have all now been merged to master. Closing

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.

5 participants