-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
store: Remove Amino #6984
Conversation
Codecov Report
@@ 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
There was a problem hiding this 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
Description
Remove Amino usage from the
store
pkg and sub-pkgs:Pairs
Proto schemaPairs
Pairs
store/rootmulti/internal
pkg tostore
rootBefore 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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes