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

Functional replacement for genesis state access #39

Merged
merged 2 commits into from
Jan 9, 2019

Conversation

holgerd77
Copy link
Member

I did some manual testing of the TypeScript-updated library by injecting a packed library into the VM node_modules folder and realized that the current static genesis state access method is broken along with the distribution reorg.

After tinkering around a bit I came to the conclusion that this cannot be properly recreated with the switch to ES6 import syntax and we have to do a breaking change here and then release the updated library as v1.0.0 (which should be ok).

This PR introduces two new methods genesisStateById, genesisStateByName which can be directly imported and used for access the genesis state dictionaries. It also updates README and docs and adds two new test cases.

@holgerd77 holgerd77 requested a review from s1na January 9, 2019 11:27
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 96.327% when pulling 832dfb8 on fix-genesis-states-import into 41a5413 on master.

@s1na
Copy link
Contributor

s1na commented Jan 9, 2019

@holgerd77 Great that this was found out! The PR looks good. I just didn't quite understand what the issue with ES6 imports was? Is the reason a major version bump required the fact that the path to genesisStates is different (has additional dist)?

@holgerd77
Copy link
Member Author

Yes, it has additional dist in the path. On top it is not possible to define constants with default export, so on import this gets wrapped in another genesisStates dict key and access would be something like:

const genesisStates = require('./dist/genesisStates')
genesisStates['genesisStates']['mainnet']

If you are ok with it can you approve or alternatively request additional changes, but directly through the review functionality?

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Thanks for explaining, using function to get the data is probably better anyway, as even if the underlying data changes it'd be possible to keep the same interface.

In that case, the other data types (chains and hardforks) should be similar, no? or is this PR only for genesisStates?

@holgerd77
Copy link
Member Author

No, this PR is only for genesisStates which are accessed separately due to performance and memory reasons. Access to hardfork and chain parameters is done exclusively through the API as stated in the documentation.

@holgerd77 holgerd77 merged commit ed00b44 into master Jan 9, 2019
@holgerd77 holgerd77 deleted the fix-genesis-states-import branch January 9, 2019 13:11
@s1na
Copy link
Contributor

s1na commented Jan 9, 2019

@holgerd77 Ah, sorry I wasn't aware of the difference. I think I should get to know the libraries better before reviews 😅

@holgerd77
Copy link
Member Author

Perfectly fine, better to ask some questions too much than too few, especially on reviews.

And beyond, a) how things are currently done and b) if things make sense are two totally different things. 😄

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.

3 participants