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

store: Remove Amino #6984

Merged
merged 9 commits into from
Aug 11, 2020
Merged

store: Remove Amino #6984

merged 9 commits into from
Aug 11, 2020

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Aug 7, 2020

Description

Remove Amino usage from the store pkg and sub-pkgs:

  • Define new Pairs Proto schema
  • Update IAVL proof value to be a Proto-encoded Pairs
  • Update simulations and store to use new Pairs
  • Move store/rootmulti/internal pkg to store root

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@alexanderbez alexanderbez changed the title Update kv pair to proto store: Remove Amino Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

Merging #6984 into master will decrease coverage by 0.08%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #6984      +/-   ##
==========================================
- Coverage   61.94%   61.86%   -0.09%     
==========================================
  Files         519      520       +1     
  Lines       32089    32108      +19     
==========================================
- Hits        19879    19865      -14     
- Misses      10596    10628      +32     
- Partials     1614     1615       +1     

@@ -0,0 +1,29 @@
syntax = "proto3";
package cosmos.store;
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather this not be a top-level cosmos package. Isn't this really something internal to the SDK that clients or even people trying to re-implement the SDK in say rust would never use? We should have something like cosmos_sdk.internal for stuff like this. Or am I wrong and is this an essential part of consensus state and the app hash?

Copy link
Contributor

@amaury1093 amaury1093 Aug 10, 2020

Choose a reason for hiding this comment

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

Note that after #6905, we'll have a cosmos.base parent, so this can go as cosmos.base.store.v1beta1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @aaronc, it's used to compute the AppHash. I don't really care where this goes. If you have a preference on location, just let me know and I'll move it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's put it where @amaurymartiny suggested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Happy to merge this, I'll take care of #6984 (comment) in #6905

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 11, 2020
@mergify mergify bot merged commit 0f44d1a into master Aug 11, 2020
@mergify mergify bot deleted the bez/store-amino-removal branch August 11, 2020 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Encoding C:Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants