-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(group)!: Register types with Amino #13592
Conversation
out, err := cdc.Amino.MarshalJSON(group.MsgSubmitProposal{Proposers: []string{member1.String()}}) | ||
require.NoError(t, err) | ||
require.Equal(t, | ||
`{"type":"cosmos-sdk/group/MsgSubmitProposal","value":{"proposers":["cosmos1d4jk6cn9wgcsj540xq"]}}`, |
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.
Whereas on v0.46, we have:
{"proposers":["cosmos1d4jk6cn9wgcsj540xq"]}
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13592 +/- ##
==========================================
- Coverage 55.78% 54.70% -1.08%
==========================================
Files 692 645 -47
Lines 59931 55692 -4239
==========================================
- Hits 33431 30465 -2966
+ Misses 23666 22739 -927
+ Partials 2834 2488 -346
|
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.
its unclear to me if we miss these or choose not to include them? does it make sense to do some ast tool in order to verify things are right?
I think we simply missed them. It's better to have the Re AST tool: We now add amino annoations on proto Msgs. We could add validations based on those. One simple validation I'm thinking is that 1/ all proto Msg has an amino annotation and 2/ the annotation value is equal to the one we registered inside go-amino (and in a later phase 3/ the output of go-amino is the same as our new proto-based amino encoder) |
@Mergifyio backport release/v0.46.x |
* fix!(group): Register types with Amino * Chagenlog (cherry picked from commit 9a9e5b5) # Conflicts: # CHANGELOG.md # x/group/msgs_test.go
✅ Backports have been created
|
* fix!(group): Register types with Amino * Chagenlog
* fix!(group): Register types with Amino * Chagenlog
Description
In v0.46, we forgot to register amino types on the group's AppModule interface.
This results in group Msgs not rendering correctly for amino-json: #13592 (comment)
This is a state-machine breaking change if the chain uses group with amino. We should decide if we backport it with clear instructions, of just wait for 0.47. I prefer the latter.
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change