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

feat: file base genesis source/target #13724

Closed
wants to merge 38 commits into from
Closed

Conversation

JayT106
Copy link
Contributor

@JayT106 JayT106 commented Nov 1, 2022

ADR-061 defined the genesis source/target interface as part of the core APIs. This PR implements the file-based genesis source/target suggested by #12808 (comment) and is able to support reworking the genesis import/export in PR #13032 .


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@JayT106 JayT106 requested a review from a team as a code owner November 1, 2022 21:34
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #13724 (d62bb9c) into main (2739f83) will increase coverage by 0.56%.
The diff coverage is 58.10%.

❗ Current head d62bb9c differs from pull request most recent head fafd034. Consider uploading reports for the commit fafd034 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #13724      +/-   ##
==========================================
+ Coverage   56.25%   56.82%   +0.56%     
==========================================
  Files         667      640      -27     
  Lines       56576    55047    -1529     
==========================================
- Hits        31829    31280     -549     
+ Misses      22165    21217     -948     
+ Partials     2582     2550      -32     
Impacted Files Coverage Δ
core/genesis/file.go 58.10% <58.10%> (ø)
x/auth/keeper/genesis.go 0.00% <0.00%> (-50.00%) ⬇️
x/auth/keeper/account.go 53.33% <0.00%> (-18.34%) ⬇️
x/group/internal/math/dec.go 41.53% <0.00%> (-7.00%) ⬇️
x/gov/abci.go 90.29% <0.00%> (-6.86%) ⬇️
server/config/config.go 69.87% <0.00%> (-2.65%) ⬇️
x/nft/keeper/grpc_query.go 69.38% <0.00%> (-1.45%) ⬇️
x/nft/client/cli/query.go 61.72% <0.00%> (-0.72%) ⬇️
x/group/msgs.go 57.35% <0.00%> (-0.25%) ⬇️
server/mock/tx.go 53.33% <0.00%> (ø)
... and 62 more

core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
core/genesis/file.go Fixed Show fixed Hide fixed
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm, just few nits! I'd like to still have @aaronc review this as he's the one who described this feature.

core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member

aaronc commented Nov 8, 2022

Thanks @JayT106 🙏. Looks good so far. Will try to do a more thorough review later today.

@JayT106
Copy link
Contributor Author

JayT106 commented Nov 8, 2022

Thanks @JayT106 🙏. Looks good so far. Will try to do a more thorough review later today.

Thanks for your suggestion and proposal @aaronc

core/genesis/file.go Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This looks like a great start thank you @JayT106 !

We need to handle the spec correctly which I tried to outline in my comments. Apologies if this wasn't clear before. Let me know if the comments make sense.

core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
@JayT106 JayT106 force-pushed the core-genesis branch 3 times, most recently from d62bb9c to 7db3988 Compare November 14, 2022 17:53
@JayT106
Copy link
Contributor Author

JayT106 commented Nov 14, 2022

not sure how to pass the dependency review with btcd@0.22.3, The SDK also uses this module version. I tried using btcd@0.23.3 has another module dependency issue.

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Nice work and thank you for this change @JayT106! I've added a review, please take a look.

core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
@julienrbrt
Copy link
Member

julienrbrt commented Nov 28, 2022

not sure how to pass the dependency review with btcd@0.22.3, The SDK also uses this module version. I tried using btcd@0.23.3 has another module dependency issue.

Yep, I don't know either, but we should investigate given that it is starting to block multiple PRs: #13701 (comment)

@JayT106
Copy link
Contributor Author

JayT106 commented Nov 29, 2022

not sure how to pass the dependency review with btcd@0.22.3, The SDK also uses this module version. I tried using btcd@0.23.3 has another module dependency issue.

Yep, I don't know either, but we should investigate given that it is starting to block multiple PRs: #13701 (comment)

submit an issue to tendermint
tendermint/tendermint#9783

Copy link
Collaborator

@odeke-em odeke-em 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 this change @JayT106 and for your patience! I've sent my second round of review but the highlight here is that:

  • No need to create a bytes.Buffer then do buf.ReadFrom(reader), then do buf.Bytes() or buf.String(); there is io.ReadAll(reader) per https://pkg.go.dev/io#ReadAll which will sort things out
  • Anytime you find yourself doing .Write([]byte(str)) then later a comparison in which you have to convert bytes to a string just to compare with str, that means you could have rather kept the value of str as a byteslice

Thank you!

core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/files_test.go Outdated Show resolved Hide resolved
core/genesis/files_test.go Outdated Show resolved Hide resolved
core/genesis/files_test.go Outdated Show resolved Hide resolved
core/genesis/files_test.go Outdated Show resolved Hide resolved
core/genesis/files_test.go Outdated Show resolved Hide resolved
@JayT106
Copy link
Contributor Author

JayT106 commented Dec 2, 2022

Thank you for this change @JayT106 and for your patience! I've sent my second round of review but the highlight here is that:

@odeke-em, very much appreciate your time in reviewing the code. I've updated all the places.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This seems like it's going conceptually in the right direction now @JayT106. But I think the logic is not quite correct yet and could be simplified.

Basically when a FileGenesisSource is created we have three things:

  1. sourceDir
  2. moduleName
  3. the RawJSON that came from app_state_bytes from RequestInitChain

I don't think we should be trying to open <sourceDir>/genesis.json, instead the function creating FileGenesisSource should have already parsed app_state_bytes from RequestInitChain and passes the json.RawMessage for that module into FileGenesisSource.

Then we try first reading <module>/<field>.json, then <module>.json, and finally the json.RawMessage that got passed into NewFileGenesisSource

core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
core/genesis/file.go Outdated Show resolved Hide resolved
@JayT106
Copy link
Contributor Author

JayT106 commented Jan 3, 2023

@aaronc to align with #14223, I've added 2 functions to fulfill the appmodule requirement. See
ad0193b (will add tests in the following commit).

@tac0turtle
Copy link
Member

@aaronc could we get a review from you on this pr

@yihuang
Copy link
Collaborator

yihuang commented Feb 23, 2023

I guess with the new collections feature solving the genesis streaming problem, there's less motivation to pursue this PR? Or can this PR work together with collections? @JayT106

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

The genesis API that we ended up adopting in core (and that collections and ORM use) is significantly simpler than the one originally proposed. There is no need to support the Read/WriteMessage and Read/WriteRawJSON methods anymore so these can be deleted from this PR. That should make the implementation significantly simpler. Do you intend to move this forward with those changes @JayT106? If not someone on the SDK team can probably push it over the finish line.

@JayT106
Copy link
Contributor Author

JayT106 commented Feb 23, 2023

@aaronc @yihuang yes, the collections feature could solve the genesis issues better. Close this PR for now.

@JayT106 JayT106 closed this Feb 23, 2023
@aaronc
Copy link
Member

aaronc commented Feb 23, 2023

I would note that file based genesis is not supported yet in the SDK or collections. So at some point we'll need to pick up the remaining pieces to get it to work (some of which can be pulled from this PR)

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.

6 participants