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

Added genesis functions #6

Merged
merged 6 commits into from
Jan 13, 2020
Merged

Added genesis functions #6

merged 6 commits into from
Jan 13, 2020

Conversation

sunnya97
Copy link
Contributor

@sunnya97 sunnya97 commented Jan 7, 2020

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Reviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge PR #XYZ: [title]" (coding standards)

@sunnya97 sunnya97 requested a review from ethanfrey as a code owner January 7, 2020 00:43
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thank you for your PR contribution. And thank you for wiring everything up. In general it looks good and you capture all relevent data in the genesis file. I added some minor style and bugfix comments, but the main questions are about the GenesisState itself. First that I would group data by code and contract and not have so many independent arrays (see example structs I propose). The other issue is actually containing all this info in the genesis. Maybe it should be in files next to the genesis?

This works fine for a handful of contracts. But let's say we have 100 contracts, even if each only have 10-20kb data, we end up with 10-20MB of genesis state just for them (base64 encoding the wasm code - 100kB+ per pop, then json encoding the state). The cosmoshub2 genesis export was 3.2 MB https://github.com/cosmos/launch/blob/master/genesis.cosmoshub-2.json and I remember hearing that cause problems for a few people to initialize from. If it is 20MB, I assume this would create many problems (but maybe there are known fixes for those issues already in the cosmos-sdk codebase)

x/wasm/alias.go Show resolved Hide resolved
x/wasm/internal/types/genesis.go Show resolved Hide resolved
x/wasm/internal/types/types.go Show resolved Hide resolved
x/wasm/internal/keeper/genesis.go Show resolved Hide resolved
x/wasm/internal/keeper/genesis.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/genesis.go Outdated Show resolved Hide resolved
@sunnya97
Copy link
Contributor Author

sunnya97 commented Jan 8, 2020

Do you mind if I rename the current Contract struct to ContractInfo? That way it's more in line with CodeInfo.

@sunnya97
Copy link
Contributor Author

sunnya97 commented Jan 8, 2020

The cosmoshub2 genesis export was 3.2 MB https://github.com/cosmos/launch/blob/master/genesis.cosmoshub-2.json and I remember hearing that cause problems for a few people to initialize from

If I remember correctly, I think someone thought that it was causing issues for them, but it wasn't, and their issue was something else. A couple megabytes in memory to read a JSON should be fine.

But that being said, you're right, as the state increases into the many gigabytes, to the level of the number of contracts something like Ethereum has, then it's definitely going to have major issues. (the issue is likely going to be from contract state, not contract bytecode).

One option might be to read the json as a stream, rather than loading it all into memory at once. Not completely sure how feasible that is, given the current design for Import/Export Genesis.

Alternatively, I agree it is nice to have all the genesis info in one file, but I guess in the future it's okay if that is a .zip instead of a .json per say. I think in that case, we should have a mechanism where the genesis json refers to a relative file directory, and includes a hash of the file (in this case, we happen to have the code hash already, but for other use cases it might not). I think should be figured out as a common standard across SDK projects.

For now, I think keeping it all in genesis json is fine, but we should start thinking about an ADR for doing this generally in the SDK.

@ethanfrey
Copy link
Member

Do you mind if I rename the current Contract struct to ContractInfo? That way it's more in line with CodeInfo.

Please do. The code works, but could definitely use some stylistic cleanup. And if you find any bugs, please report/fix them.

@ethanfrey
Copy link
Member

For now, I think keeping it all in genesis json is fine, but we should start thinking about an ADR for doing this generally in the SDK.

This is a good point. Let's go the most straight-forward approach now. There are some months til we need to have this scalability, and best if it works with some system supported by the sdk rather than bikeshedding something else.

@sunnya97
Copy link
Contributor Author

sunnya97 commented Jan 9, 2020

I think all comments have been addressed 👍

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. The code looks quite nice now. I just added one minor change request.

The only thing stopping me from merging is a test case (and maybe an implementation for ValidateGenesis, but I'm not sure how important that is). You can use the contract in testdata and look at some keeper tests to Create and Instantiate a contract, then you have state to export in the genesis file.

x/wasm/internal/types/genesis.go Show resolved Hide resolved
@sunnya97 sunnya97 mentioned this pull request Jan 11, 2020
7 tasks
@ethanfrey
Copy link
Member

I'm happy to merge this, but would love a test case... at least a simple one on the happy path. Dump state and restart is a horrible time to find a bug

@ethanfrey
Copy link
Member

Okay, I will merge without tests, as other PRs are incoming and I do not want merge conflicts.

But I left an open issue to finish up tests #20

@ethanfrey ethanfrey merged commit 3fae154 into CosmWasm:master Jan 13, 2020
@sunnya97
Copy link
Contributor Author

Oops sorry, was writing the test, but got busy with something else for a few days. Opened PR for tests in #23

alpe added a commit that referenced this pull request Jan 21, 2021
giansalex pushed a commit to disperze/wasmd that referenced this pull request Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants