-
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
refactor: Bring back deprecated proto fields to v1beta1
#9534
Changes from 12 commits
88ecb48
dc13f9d
ce49abe
7ab4aef
5c386c5
1ec356c
b2c277a
362d1c7
dfcab08
d1d03fd
e89c869
4574bfc
f68526b
387774a
c992428
38a1756
ea0e5ce
1a0dcd4
429ed40
4d960dd
b08a3ff
0ce625e
d279dc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,8 +26,6 @@ lint: | |
breaking: | ||
use: | ||
- FILE | ||
except: | ||
- FIELD_NO_DELETE | ||
ignore: | ||
- tendermint | ||
- gogoproto | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,13 @@ func (keeper Keeper) GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.A | |
} | ||
|
||
keeper.cdc.MustUnmarshal(bz, &vote) | ||
|
||
// Graceful fallback of deprecated `Option` field, in case there's only 1 | ||
// VoteOption. | ||
if len(vote.Options) == 1 && vote.Options[0].Weight.Equal(sdk.MustNewDecFromStr("1.0")) { | ||
vote.Option = vote.Options[0].Option //nolint staticcheck | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we remove the entries in Options?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, should we do the same transformation (setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
I'm not sure. We should for sure populate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, |
||
|
||
return vote, true | ||
} | ||
|
||
|
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.
in fact, this field is problematic. User simply can't use it. We need to be clear in the changelog about it - because in other circumstance this should require a proto version update in my opinion.
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.
Yeah I agree, I updated the existing changelog entry, do you think it's enough? A mention in the release notes would be useful too.