refactor: Bring back deprecated proto fields to v1beta1
#9534
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9534 +/- ##
==========================================
+ Coverage 60.61% 60.65% +0.03%
==========================================
Files 589 588 -1
Lines 37260 37226 -34
==========================================
- Hits 22587 22579 -8
+ Misses 12730 12703 -27
- Partials 1943 1944 +1
|
|
Let's remember to update buf.yaml too while we're at it and remove the FIELD_NO_DELETE exception |
|
R4R, @aaronc @robert-zaremba lmk if I forgot something. Edit: I also added the |
| @@ -13,7 +13,7 @@ import ( | |||
| // NewVote creates a new Vote instance | |||
| //nolint:interfacer | |||
| func NewVote(proposalID uint64, voter sdk.AccAddress, options WeightedVoteOptions) Vote { | |||
| return Vote{proposalID, voter.String(), options} | |||
| return Vote{ProposalId: proposalID, Voter: voter.String(), Options: options} | |||
There was a problem hiding this comment.
If len(options) == 1 and the weight is 1, let's also set Option to have a graceful fallback
There was a problem hiding this comment.
I don't think that's desired here.
This function is called when setting state. A user can call MsgWeightedVote explicity with len(options) == 1 and weight == 1, in state I think we should simply always skip the deprecated field.
There was a problem hiding this comment.
I added graceful fallback only for queries in d1d03fd
| reserved 2; | ||
| reserved "time"; | ||
| // If this field is not empty, an error will be thrown. | ||
| google.protobuf.Timestamp time = 2 [deprecated = true, (gogoproto.stdtime) = true, (gogoproto.nullable) = false]; |
There was a problem hiding this comment.
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.
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.
x/gov/keeper/vote.go
Outdated
| // vote.Option is a deprecated field, we don't set it in state | ||
| if vote.Option != types.OptionEmpty { //nolint staticcheck // Deprecated field | ||
| panic(fmt.Errorf("expected empty vote.Option, got %s", vote.Option)) //nolint staticcheck // Deprecated field | ||
| } |
There was a problem hiding this comment.
In Keeper.AddVote, shall we check that Vote.Option and Vote.Options are exclusive (on can't set both parameters)?
AddVote takes options as argument, I don't think there's anything to change.
I added this piece of code in keeper.SetVote, so that in state, we always only store the weighted option, does that seem okay to you?
x/gov/keeper/vote.go
Outdated
| // VoteOption. | ||
| if len(vote.Options) == 1 && vote.Options[0].Weight.Equal(sdk.MustNewDecFromStr("1.0")) { | ||
| vote.Option = vote.Options[0].Option //nolint | ||
| } |
There was a problem hiding this comment.
should we remove the entries in Options?
vote.Options = nil
There was a problem hiding this comment.
Also, should we do the same transformation (setting vote.Option in GetVotes?
There was a problem hiding this comment.
Also, should we do the same transformation (setting vote.Option in GetVotes?
Yes, fixed.
should we remove the entries in Options? vote.Options = nil
I'm not sure. We should for sure populate Vote.Option to keep backwards-compatibility, but I would also populate Vote.Options too, so that clients know what to use instead of the deprecated field.
There was a problem hiding this comment.
OK, Vote.Option is marked as deprecated, so Vote.Options should be used in principal.
Co-authored-by: likhita-809 <[email protected]>
amaury1093
added
the
A:automerge
Description
Closes: #9446
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.mdReviewers 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