-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(x/gov): add max cancel voting period param #18856
Conversation
WalkthroughThe Cosmos Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
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.
changes are looking good.
Just some cosmetic changes are needed.
And also could you fix the failing gov genesis test?
func (x *_Params_18_list) Set(i int, value protoreflect.Value) { | ||
valueUnwrapped := value.String() | ||
concreteValue := valueUnwrapped | ||
(*x.list)[i] = concreteValue | ||
} |
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.
The Set
method directly assigns a string value to an element in the list without any validation. Consider adding validation to ensure the value being set is appropriate for the list.
func (x *_Params_18_list) Truncate(n int) { | ||
*x.list = (*x.list)[:n] | ||
} |
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.
The Truncate
method modifies the underlying slice to the specified length. Ensure that there is proper handling for cases where n
is out of bounds.
v := "" | ||
return protoreflect.ValueOfString(v) | ||
} | ||
|
||
func (x *_Params_17_list) IsValid() bool { | ||
func (x *_Params_18_list) IsValid() bool { |
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.
The IsValid
method checks if the list is not nil. This is a good null check, but consider if there are other conditions that should be checked to determine the validity of the list.
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.
LGTM
if proposal.VotingEndTime != nil { | ||
currentTime := sdkCtx.HeaderInfo().Time | ||
|
||
maxCancelPeriodRate := sdkmath.LegacyMustNewDecFromStr(params.ProposalCancelMaxPeriod) |
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.
nit: what does rate mean in maxCancelPeriodRate
? Maybe maxCancelPeriod
or maxCancelPeriodDuration
and rename maxCancelPeriod
to maxCancelTime
?
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.
nvm, it's not a duration, my bad
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.
lgtm
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.
lgtm, just a doubt.
Description
This PR adds maximum cancellation time for cancelling a proposal.
We as well set the SDK default to 1/2 the voting period.
ref: Slack, suggested by dydx.
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...
!
in 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...