Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Add petersburg aka constantinople fix support #44

Merged
merged 4 commits into from
Jan 31, 2019

Conversation

holgerd77
Copy link
Member

Addresses #41.

This PR adds a new HF file for the petersburg HF (aka constantinopleFix), adds HF numbers for mainnet and ropsten and fixes a bug on HF comparison along the way (see commit messages).

@holgerd77
Copy link
Member Author

//cc @davidmurdoch

Copy link

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Only one suggested addition (in the hardforks tests) in my comments. All the rest looks good to me.

src/chains/mainnet.json Show resolved Hide resolved
src/chains/ropsten.json Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
2,
'should return 2 active HFs when restricted to supported HFs',
3,
'should return 3 active HFs when restricted to supported HFs',
Copy link

Choose a reason for hiding this comment

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

We could add another test here that checks that activeHardforks behaves correctly when Common is instantiated with at least one non-supported hardfork and activeHardforks is called with the onlySupported option. This would fulfill the original purpose of the test. Could use dao as done below with one of the hardforkGteHardfork tests.

example:

    c = new Common('ropsten', null, ['spuriousDragon', 'byzantium', 'dao'])
    st.equal(
      c.activeHardforks(null, { onlySupported: true }).length,
      3,
      'should return 3 active HFs when restricted to supported HFs',
    )

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added the additional test (result of this should be 2 though, to match the two supported from the three restricted to).

Thanks for the review! 😄 Will directly merge on tests passing since this was the only change requested and changes are not production-code touching.

src/hardforks/petersburg.json Show resolved Hide resolved
@danjm
Copy link

danjm commented Jan 30, 2019

I am going to further validate these changes by running the tests on ethereumjs-vm once I get the proper changes applied. I will work on that next.

@holgerd77 holgerd77 force-pushed the add-petersburg-aka-constantinople-fix-support branch from b49a7da to 6ea794b Compare January 31, 2019 12:39
@holgerd77 holgerd77 merged commit ce35b08 into master Jan 31, 2019
@holgerd77 holgerd77 deleted the add-petersburg-aka-constantinople-fix-support branch January 31, 2019 12:45
@holgerd77 holgerd77 restored the add-petersburg-aka-constantinople-fix-support branch January 31, 2019 12:47
@axic axic deleted the add-petersburg-aka-constantinople-fix-support branch January 31, 2019 12:49
@axic axic restored the add-petersburg-aka-constantinople-fix-support branch January 31, 2019 12:49
@holgerd77
Copy link
Member Author

First thought of directly do a release on this, but maybe let's wait for the core devs call from tomorrow, maybe there will be some late adjustments to fork naming or something.

@holgerd77
Copy link
Member Author

(should not prevent from working on it on the VM side I suppose)

@danjm
Copy link

danjm commented Jan 31, 2019

First thought of directly do a release on this, but maybe let's wait for the core devs call from tomorrow, maybe there will be some late adjustments to fork naming or something.
(should not prevent from working on it on the VM side I suppose)

That makes sense. I am currently trying to get this branch installed in the vm... running into trouble, but hopefully will have it sorted soon.

@holgerd77
Copy link
Member Author

I had once deleted and then recreated the branch after thinking that you'll need it for development (sorry). Not sure if this can be the cause for your problems.

You can for the moment also alternatively develop against the master branch.

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

Successfully merging this pull request may close these issues.

2 participants