feat(gov): allow variable governance proposal durations with dynamic quorum #24576
Conversation
Ironbird - launch a networkTo use Ironbird, you can use the following commands:
Custom Load Test ConfigurationYou can provide a custom load test configuration using the `--load-test-config=` flag:Use |
📝 WalkthroughWalkthroughThis change introduces support for variable governance proposal durations in the Cosmos SDK governance module. The protobuf definitions for proposals and proposal submission messages are extended with a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/User
participant CLI
participant MsgSubmitProposal
participant Keeper
participant Params
CLI/User->>CLI: Submit proposal with duration
CLI->>MsgSubmitProposal: Construct MsgSubmitProposal(duration)
MsgSubmitProposal->>Keeper: SubmitProposal(..., duration)
Keeper->>Params: CalculateThresholdAndMinDeposit(duration)
Params-->>Keeper: threshold, minDeposit
Keeper->>Keeper: Validate initial deposit (uses minDeposit)
Keeper->>Keeper: Set proposal voting period (uses duration)
Keeper-->>CLI/User: Proposal created with custom duration/quorum
Assessment against linked issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
x/gov/types/v1/proposal.go (1)
26-26: Function signature updated with duration parameter, needs documentation.The
NewProposalfunction signature has been updated to include a newdurationparameter, which is part of the variable governance proposal durations feature. However, the function's documentation comment doesn't explain this new parameter or its purpose.Consider updating the function documentation at line 25 to explain the purpose and usage of the duration parameter.
-// NewProposal creates a new Proposal instance +// NewProposal creates a new Proposal instance +// duration is an optional parameter that can be used to set a custom voting period duration, +// which will influence the required quorum for proposal passagex/gov/types/v1/params.go (1)
10-10: Inconsistent import alias for the same package.The import for
github.com/cosmos/cosmos-sdk/typesnow has two different aliases in use -sdkon line 9 andtypeson line 10. This could lead to confusion and inconsistency in the codebase.Choose one alias consistently throughout the file:
-import ( - "fmt" - "time" - - sdkmath "cosmossdk.io/math" - - sdk "github.com/cosmos/cosmos-sdk/types" - types "github.com/cosmos/cosmos-sdk/types" -) +import ( + "fmt" + "time" + + sdkmath "cosmossdk.io/math" + + sdk "github.com/cosmos/cosmos-sdk/types" +)x/gov/keeper/deposit_test.go (2)
56-57: Add test coverage for variable duration proposals.While the test has been updated to support the new function signature, there's no explicit test case for the variable duration functionality. Consider adding test cases that verify the behavior with custom durations to ensure the feature works as expected.
-proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], tc.expedited, nil) + +// Test with nil duration (default behavior) +proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], tc.expedited, nil) +require.NoError(t, err) +proposalID := proposal.Id + +// Test with custom duration +customDuration := 72 * time.Hour +customDurationProposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title with custom duration", "summary", + TestAddrs[0], tc.expedited, &customDuration) +require.NoError(t, err) + +// Verify custom duration is set correctly +if tc.expedited { + require.NotNil(t, customDurationProposal.Duration) + require.Equal(t, customDuration, *customDurationProposal.Duration) +}
146-147: Add a test case with non-nil duration for expedited proposals.The current test only checks the functionality with nil duration. Consider adding a test case where expedited is true and a custom duration is provided to ensure the correct interaction between expedited proposals and custom durations.
x/gov/keeper/proposal.go (2)
19-19: Document the purpose of the new duration parameter in the function comment.The function comment should be updated to explain the purpose and behavior of the new
durationparameter, particularly its interaction with expedited proposals.// SubmitProposal creates a new proposal given an array of messages -func (k Keeper) SubmitProposal(ctx context.Context, messages []sdk.Msg, metadata, title, summary string, proposer sdk.AccAddress, expedited bool, duration *time.Duration) (v1.Proposal, error) { +// SubmitProposal creates a new proposal given an array of messages +// If duration is provided for expedited proposals, it overrides the default expedited voting period. +// For non-expedited proposals, the duration parameter is ignored. +func (k Keeper) SubmitProposal(ctx context.Context, messages []sdk.Msg, metadata, title, summary string, proposer sdk.AccAddress, expedited bool, duration *time.Duration) (v1.Proposal, error) {
252-259: Consider supporting variable durations for non-expedited proposals as well.The current implementation only supports custom durations for expedited proposals. Consider extending this functionality to non-expedited proposals as well for consistency and flexibility.
if proposal.Expedited { if proposal.Duration != nil { votingPeriod = proposal.Duration } else { votingPeriod = params.ExpeditedVotingPeriod } } else { - votingPeriod = params.VotingPeriod + if proposal.Duration != nil { + votingPeriod = proposal.Duration + } else { + votingPeriod = params.VotingPeriod + } }x/gov/client/cli/util.go (2)
92-92: Update the comment to document the Duration field.Add a comment line documenting the purpose and format of the Duration field for better developer experience.
Expedited bool `json:"expedited"` - Duration Duration `json:"duration"` + // Duration is an optional custom voting period for the proposal (e.g. "72h", "10m", "1h30m") + Duration Duration `json:"duration"`
206-209: Add support for JSON marshaling.Implement the
json.Marshalerinterface as well to ensure the Duration type can be properly serialized back to JSON if needed.func (d Duration) ToDuration() *time.Duration { duration := time.Duration(d) return &duration } + +// MarshalJSON implements the json.Marshaler interface +func (d Duration) MarshalJSON() ([]byte, error) { + return json.Marshal(time.Duration(d).String()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/cosmos/gov/v1/gov.pulsar.gois excluded by!**/*.pulsar.goapi/cosmos/gov/v1/tx.pulsar.gois excluded by!**/*.pulsar.gox/gov/types/v1/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/v1/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (29)
proto/cosmos/gov/v1/gov.proto(1 hunks)proto/cosmos/gov/v1/tx.proto(2 hunks)x/bank/app_test.go(1 hunks)x/gov/abci_test.go(10 hunks)x/gov/client/cli/tx.go(1 hunks)x/gov/client/cli/util.go(3 hunks)x/gov/keeper/deposit.go(3 hunks)x/gov/keeper/deposit_test.go(4 hunks)x/gov/keeper/export_test.go(1 hunks)x/gov/keeper/grpc_query_test.go(13 hunks)x/gov/keeper/hooks_test.go(2 hunks)x/gov/keeper/keeper_test.go(2 hunks)x/gov/keeper/msg_server.go(2 hunks)x/gov/keeper/msg_server_test.go(33 hunks)x/gov/keeper/proposal.go(3 hunks)x/gov/keeper/proposal_test.go(6 hunks)x/gov/keeper/tally.go(1 hunks)x/gov/keeper/tally_test.go(2 hunks)x/gov/keeper/vote_test.go(1 hunks)x/gov/migrations/v4/store_test.go(1 hunks)x/gov/simulation/msg_factory.go(1 hunks)x/gov/simulation/operations.go(1 hunks)x/gov/simulation/operations_test.go(4 hunks)x/gov/types/v1/msgs.go(3 hunks)x/gov/types/v1/msgs_test.go(1 hunks)x/gov/types/v1/params.go(2 hunks)x/gov/types/v1/params_test.go(1 hunks)x/gov/types/v1/proposal.go(2 hunks)x/gov/types/v1/proposals_test.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
x/gov/client/cli/tx.go (2)
x/gov/types/v1/msgs.go (1)
NewMsgSubmitProposal(18-43)x/gov/client/cli/util.go (1)
Duration(191-191)
x/gov/types/v1/params.go (4)
x/gov/client/cli/util.go (1)
Duration(191-191)x/gov/simulation/genesis.go (6)
VotingPeriod(22-22)ExpeditedVotingPeriod(23-23)ExpeditedThreshold(26-26)ExpeditedMinDeposit(19-19)Threshold(25-25)MinDeposit(18-18)math/legacy_dec.go (2)
LegacyMustNewDecFromStr(209-215)LegacyNewDecFromInt(130-132)types/coin.go (1)
NewCoin(19-30)
x/gov/types/v1/params_test.go (3)
x/gov/types/v1/params.go (1)
DefaultParams(88-107)types/coin.go (2)
NewCoins(194-201)NewCoin(19-30)math/int.go (1)
NewInt(119-121)
x/gov/keeper/msg_server_test.go (2)
x/gov/types/v1/msgs.go (1)
NewMsgSubmitProposal(18-43)x/gov/migrations/v4/store_test.go (1)
TestProposal(31-31)
x/gov/keeper/deposit.go (1)
x/gov/keeper/keeper.go (1)
Keeper(21-62)
x/gov/keeper/msg_server.go (1)
x/gov/client/cli/util.go (1)
Duration(191-191)
x/gov/types/v1/msgs.go (1)
x/gov/client/cli/util.go (1)
Duration(191-191)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (36)
x/gov/simulation/msg_factory.go (1)
161-170: Added support for new duration parameter in proposal submission.The call to
v1.NewMsgSubmitProposalhas been updated to include a newnilparameter for the optional proposal duration. This is consistent with the PR's goal of implementing variable governance proposal durations.x/gov/keeper/vote_test.go (1)
22-24: Updated test call with duration parameter.The SubmitProposal call has been updated to match the new signature that supports variable durations (passed as nil here to maintain default behavior).
x/bank/app_test.go (1)
382-393: Updated governance proposal construction with duration parameter.The
NewMsgSubmitProposalcall has been updated to include the new optional duration parameter asnil. This change properly implements the variable proposal duration feature in test code.x/gov/migrations/v4/store_test.go (2)
80-82: Updated proposal creation with duration parameter.The
NewProposalcall has been updated to include the new optional duration parameter asnilfor this test proposal.
87-88: Updated second test proposal creation with duration parameter.Similar to the first proposal, this one has also been updated to include the new optional duration parameter, maintaining consistency throughout the test code.
x/gov/keeper/tally_test.go (2)
25-25: Updated SubmitProposal call with nil duration parameterThe function call has been updated to accommodate the new duration parameter in the SubmitProposal signature. Passing nil here maintains backward compatibility, using the default duration behavior for this test case.
69-73: Updated SubmitProposal calls with nil duration parameterBoth SubmitProposal calls have been updated to match the new method signature by adding a nil duration parameter, preserving the existing test behavior while supporting the new variable duration feature.
x/gov/keeper/tally.go (1)
197-202: Added support for dynamic threshold calculation based on proposal durationThe implementation correctly handles both cases:
- When a custom duration is specified, it dynamically calculates the threshold using
CalculateThresholdAndMinDeposit- When no duration is provided, it falls back to
GetExpeditedThresholdThis change integrates well with the existing code structure while adding support for variable governance proposal durations.
proto/cosmos/gov/v1/gov.proto (1)
104-107: Added duration field to Proposal messageThe new field enables variable governance proposal durations with proper type and options:
- Uses the standard Google protobuf Duration type
- Includes appropriate gogoproto options for standard duration handling
- Clearly documented purpose for quorum calculation
- Field is properly optional, maintaining backward compatibility
This protobuf change aligns with the broader implementation of variable proposal durations throughout the governance module.
x/gov/keeper/hooks_test.go (2)
76-76: Updated SubmitProposal call with nil duration parameterThe function call has been updated to match the new method signature by adding the nil duration parameter, maintaining backward compatibility for the test.
88-88: Updated SubmitProposal call with nil duration parameterSimilar to the previous change, this function call was updated to support the new signature while preserving the existing test behavior.
x/gov/types/v1/msgs_test.go (1)
70-70: Duration parameter added to NewMsgSubmitProposal callsThe test now correctly passes the new duration parameter (as nil) to the
v1.NewMsgSubmitProposalfunction, maintaining compatibility with the updated function signature that supports variable governance proposal durations.x/gov/simulation/operations.go (1)
249-249: Duration parameter added to simulation codeAdded support for the new duration parameter (passed as nil) in the simulated proposal submission function. This change properly aligns the simulation code with the updated
NewMsgSubmitProposalsignature.x/gov/client/cli/tx.go (1)
143-143: CLI support for custom proposal durationsThe CLI command now properly handles the custom duration parameter by passing
proposal.Duration.ToDuration()to theNewMsgSubmitProposalfunction. This enables users to specify variable durations when submitting governance proposals through the CLI.x/gov/types/v1/proposals_test.go (1)
40-40: Duration parameter added to test proposal creationUpdated all test cases to include the new duration parameter (as nil) in calls to
v1.NewProposal, maintaining compatibility with the updated constructor signature that supports variable proposal durations.Also applies to: 49-49, 53-53, 74-74
x/gov/types/v1/proposal.go (1)
46-46: LGTM!Correctly assigning the duration parameter to the Proposal struct's Duration field.
x/gov/keeper/keeper_test.go (2)
93-104: LGTM!All calls to
govKeeper.SubmitProposalhave been correctly updated to includenilas the last parameter, aligning with the new method signature that accepts a duration parameter.
119-120: LGTM!The call to
govKeeper.SubmitProposalhas been properly updated to includenilas the last parameter for the duration.proto/cosmos/gov/v1/tx.proto (1)
12-12: LGTM!Properly added import for the Duration protobuf type to support the new duration field.
x/gov/keeper/export_test.go (1)
13-13: LGTM!Correctly updated the call to
k.validateInitialDepositto includenilas the last parameter for the duration, aligning with the updated method signature.x/gov/keeper/proposal_test.go (1)
32-32: Updated function calls to match new signature with duration parameter.All calls to
suite.govKeeper.SubmitProposalhave been updated to include anilduration parameter, matching the new method signature that supports variable governance proposal durations.Also applies to: 59-59, 80-80, 110-110, 171-171, 181-181, 185-185, 190-190, 798-798
x/gov/types/v1/msgs.go (2)
4-5: Added time package import for duration support.This import is needed to support the new
*time.Durationparameter.
23-24: Added duration parameter to support variable governance proposal durations.The
NewMsgSubmitProposalconstructor now accepts an optional duration parameter, which is a key part of the implementation for variable governance proposal durations with dynamic quorum. This parameter allows proposals to have custom voting periods rather than fixed ones.Also applies to: 32-33
x/gov/keeper/msg_server_test.go (1)
55-56: Updated function calls to include nil duration parameter.All test cases calling
v1.NewMsgSubmitProposalhave been updated to include anilduration parameter, maintaining existing test behavior while supporting the new variable duration feature.Also applies to: 71-72, 87-88, 103-104, 119-120, 135-136, 151-152, 167-168, 183-184, 199-200, 215-216, 231-232, 247-248, 263-264, 279-280, 319-320, 369-370, 421-422, 468-469, 513-514, 567-568, 689-690, 734-735, 798-799, 814-815, 1025-1026, 1072-1073, 1161-1162, 1312-1313, 1357-1358, 1415-1416
x/gov/abci_test.go (3)
34-35: Added nil duration parameter to NewProposal calls.Updated
v1.NewProposalconstructor calls to include the new duration parameter asnil, which preserves existing behavior in tests while supporting the variable duration feature.Also applies to: 62-63
108-109: Updated proposal submission calls to include nil duration parameter.All calls to
v1.NewMsgSubmitProposalandsuite.GovKeeper.SubmitProposalnow include anilduration parameter, ensuring consistent function signatures across the codebase while maintaining existing test behavior.Also applies to: 161-162, 185-186, 233-234, 296-297, 396-397, 454-455, 551-552
1-724:✅ Verification successful
Verify integration with dynamic quorum calculations.
The PR aims to implement variable governance proposal durations with dynamic quorum, but the quorum calculation logic isn't visible in this file. Ensure that the duration parameter is properly used to adjust quorum requirements in the relevant parts of the codebase.
🏁 Script executed:
#!/bin/bash # Search for code that uses the duration parameter to calculate quorum echo "Searching for dynamic quorum calculation based on duration..." rg -A 10 "CalculateThresholdAndMinDeposit|interpolate.*quorum|quorum.*duration" --type goLength of output: 3310
Dynamic quorum calculation integration verified
The
CalculateThresholdAndMinDeposithelper is invoked in both the tally and deposit keeper logic to adjust quorum and minimum deposit based on each proposal’sDuration:
- In
x/gov/keeper/tally.go, we call
thresholdStr, _ = params.CalculateThresholdAndMinDeposit(*proposal.Duration)
before converting to aDecfor the vote threshold.- In
x/gov/keeper/deposit.go, we retrieve
_, minDepositCoins = params.CalculateThresholdAndMinDeposit(*duration)
to enforce the correct minimum deposit.Additionally,
x/gov/types/v1/params_test.goincludes comprehensive tests forCalculateThresholdAndMinDeposit.No further changes are needed here.
x/gov/keeper/grpc_query_test.go (1)
61-61: SubmitProposal method signature updated to support variable durations.All calls to
suite.govKeeper.SubmitProposal()have been updated with an additionalnilparameter to match the new method signature that now includes an optional*time.Durationparameter. This change correctly maintains the existing behavior while supporting the new variable governance proposal durations feature.Also applies to: 142-142, 158-158, 219-219, 416-416, 497-497, 611-611, 717-717, 821-821, 1108-1108, 1209-1209, 1299-1299, 1394-1394
x/gov/simulation/operations_test.go (1)
234-234: NewProposal constructor updated to include optional duration parameter.The
v1.NewProposal()constructor calls have been updated with anilduration parameter to support the variable governance proposal durations feature. This correctly adapts the test code to the new constructor signature while maintaining the previous behavior.Also applies to: 282-282, 331-331, 378-378
x/gov/keeper/deposit.go (1)
286-306: Added support for dynamic minimum deposit based on proposal duration.The
validateInitialDepositfunction has been enhanced to consider custom proposal durations when validating initial deposits. For expedited proposals, it now:
- Uses the standard expedited minimum deposit when no duration is specified
- Calculates a dynamic minimum deposit based on the provided duration using the new interpolation method
This implementation correctly supports the variable governance proposal durations feature by allowing minimum deposit requirements to scale with the proposal's timeline.
x/gov/types/v1/params.go (1)
226-263: Core implementation for dynamic governance parameters based on proposal duration.The new
CalculateThresholdAndMinDepositmethod is a well-designed solution for implementing variable governance proposal durations. Key points:
- It properly normalizes the proposal duration between expedited and normal voting periods
- It uses linear interpolation to calculate appropriate threshold and deposit values based on duration
- It handles edge cases appropriately by clamping values and providing fallback behavior
This implementation allows governance parameters to scale proportionally with proposal durations, enabling more flexible governance processes.
Two minor suggestions for future improvement:
- Consider adding explicit validation that
normalCoinsandexpeditedCoinshave the same length and matching denominations- Add comments documenting the behavior when duration is outside the normal range
x/gov/keeper/msg_server.go (1)
325-325: Ensure nil duration propagation through legacy message server.The addition of
nilfor the duration in the legacy message server is appropriate for backward compatibility. This ensures that legacy proposal submissions will continue to work with the new duration parameter architecture.x/gov/keeper/deposit_test.go (2)
223-224: LGTM!Changes appropriately adapt the test to the new function signature.
412-414: LGTM!Changes appropriately adapt the test to the new function signature.
x/gov/keeper/proposal.go (1)
102-102: LGTM!The change correctly passes the duration parameter to the NewProposal constructor.
x/gov/types/v1/params_test.go (1)
14-83: Well-designed test cases for the dynamic threshold and deposit calculationThis test thoroughly validates the
CalculateThresholdAndMinDepositfunctionality, covering a comprehensive range of duration scenarios including edge cases and intermediate values. The linear interpolation between expedited and normal governance parameters is well-tested with precise expected values.The test cases appropriately verify:
- Default duration (48h) parameters
- Expedited duration (24h) parameters
- Durations less than expedited (using expedited values)
- Durations greater than default (using default values)
- Multiple intermediate durations with correctly interpolated values
The calculations for the interpolated thresholds and minimum deposits are mathematically sound, maintaining proper precision for financial values.
| return nil, err | ||
| } | ||
|
|
||
| if err := k.validateDepositDenom(ctx, params, initialDeposit); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| proposal, err := k.Keeper.SubmitProposal(ctx, proposalMsgs, msg.Metadata, msg.Title, msg.Summary, proposer, msg.Expedited) | ||
| proposal, err := k.Keeper.SubmitProposal(ctx, proposalMsgs, msg.Metadata, msg.Title, msg.Summary, proposer, msg.Expedited, msg.Duration) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate the function signature updates are applied consistently across the codebase.
The addition of the duration parameter to the SubmitProposal function is a significant change that must be consistently applied to all callers of this method.
🏁 Script executed:
#!/bin/bash
# Check all SubmitProposal function calls to verify they have been updated
rg "SubmitProposal\([^)]*\)" --type go | grep -v "duration"Length of output: 17121
Ensure the SubmitProposal signature update is applied across all callers
I ran a search for SubmitProposal( calls missing the duration argument and found numerous unupdated usages. All calls to the gov keeper (and its MsgServer) must now pass the new duration parameter.
Key locations needing updates:
- x/gov/abci_test.go
- x/gov/keeper/deposit_test.go
- x/gov/keeper/grpc_query_test.go
- x/gov/keeper/msg_server_test.go
- x/gov/keeper/tally_test.go
- x/gov/keeper/vote_test.go
- x/gov/keeper/proposal_test.go
- x/gov/keeper/keeper_test.go
- x/gov/client/cli/util.go
- x/gov/client/cli/tx.go
- x/gov/simulation/operations.go & operations_test.go
- tests/e2e/gov/tx.go
- tests/integration/gov/keeper/tally_test.go, grpc_query_test.go, genesis_test.go
Each invocation should match:
SubmitProposal(
ctx,
msgs,
metadata,
title,
summary,
proposer,
expedited,
duration,
)After updating, please re-run your grep to confirm no missing instances.
| @@ -76,15 +76,15 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos | |||
| return nil, fmt.Errorf("failed to get governance parameters: %w", err) | |||
| } | |||
|
|
|||
| if err := k.validateInitialDeposit(ctx, params, initialDeposit, msg.Expedited); err != nil { | |||
| if err := k.validateInitialDeposit(ctx, params, initialDeposit, msg.Expedited, msg.Duration); err != nil { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure parameter validation for the new duration parameter.
The Duration parameter is now added to the validateInitialDeposit call, but there seems to be no validation to ensure that if provided, the duration is reasonable (not too short or too long). Consider adding validation logic elsewhere in the code to prevent potential abuse of this feature.
🏁 Script executed:
#!/bin/bash
# Searching for duration validation code in other files
rg -A 5 "func.*validateInitialDeposit.*duration \*time\.Duration" --type goLength of output: 608
🏁 Script executed:
#!/bin/bash
# Print the full body of validateInitialDeposit to check for duration validation
rg -n "func (k Keeper) validateInitialDeposit" -A200 x/gov/keeper/deposit.goLength of output: 78
🏁 Script executed:
#!/bin/bash
# Verify files in the keeper directory
echo "=== x/gov/keeper contents ==="
ls -R x/gov/keeper
# Search for the full definition of validateInitialDeposit across the repo
echo "=== validateInitialDeposit definition ==="
rg -n "func (k Keeper) validateInitialDeposit" -A200 --type goLength of output: 527
🏁 Script executed:
#!/bin/bash
# Locate the definition of validateInitialDeposit, accounting for pointer receiver
rg -n "func \(.*Keeper\) validateInitialDeposit" -A20 -B5 x/gov/keeper/deposit.go
rg -n "validateInitialDeposit" -A5 -B2 x/gov/keeper/deposit.goLength of output: 1870
🏁 Script executed:
#!/bin/bash
# Locate CalculateThresholdAndMinDeposit and its implementation to check for duration validation
rg -nH "func .*CalculateThresholdAndMinDeposit" -A200 -t go
# Search for any Params fields related to deposit periods to identify valid duration bounds
rg -nH "MinDepositPeriod" -A5 -t go
rg -nH "MaxDepositPeriod" -A5 -t goLength of output: 23515
Add explicit duration bounds validation in validateInitialDeposit
Although CalculateThresholdAndMinDeposit clamps out-of-range durations, we should still validate that the user-supplied duration lies within the configured expedited and normal voting periods. Without this, an attacker could pass a zero or negative duration (bypassing expedited deposits) or an absurdly large duration (skewing deposit calculations).
Please add, at the top of validateInitialDeposit (in x/gov/keeper/deposit.go), something like:
func (k Keeper) validateInitialDeposit(
ctx context.Context,
params v1.Params,
initialDeposit sdk.Coins,
expedited bool,
duration *time.Duration,
) error {
+ // Ensure provided duration is within [ExpeditedVotingPeriod, VotingPeriod]
+ if duration != nil {
+ if *duration < params.ExpeditedVotingPeriod || *duration > params.VotingPeriod {
+ return fmt.Errorf(
+ "duration %s must be between expedited voting period (%s) and voting period (%s)",
+ duration.String(), params.ExpeditedVotingPeriod.String(), params.VotingPeriod.String(),
+ )
+ }
+ }
// existing coin validation…• File: x/gov/keeper/deposit.go
• Function: validateInitialDeposit
Committable suggestion skipped: line range outside the PR's diff.
| type Duration time.Duration | ||
|
|
||
| func (d *Duration) UnmarshalJSON(b []byte) error { | ||
| var s string | ||
| if err := json.Unmarshal(b, &s); err != nil { | ||
| return err | ||
| } | ||
| duration, err := time.ParseDuration(s) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| *d = Duration(duration) | ||
| return nil | ||
| } | ||
|
|
||
| func (d Duration) ToDuration() *time.Duration { | ||
| duration := time.Duration(d) | ||
| return &duration | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for reasonable duration values.
The code currently allows any valid duration string to be parsed, but there's no validation to ensure the duration is reasonable for governance proposals (e.g., not too short or too long). Consider adding validation logic to prevent potential misuse.
func (d *Duration) UnmarshalJSON(b []byte) error {
var s string
if err := json.Unmarshal(b, &s); err != nil {
return err
}
duration, err := time.ParseDuration(s)
if err != nil {
return err
}
+ // Add validation to ensure the duration is reasonable
+ minDuration := 1 * time.Hour // Example: 1 hour minimum
+ maxDuration := 30 * 24 * time.Hour // Example: 30 days maximum
+ if duration < minDuration {
+ return fmt.Errorf("duration is too short, minimum allowed is %s", minDuration)
+ }
+ if duration > maxDuration {
+ return fmt.Errorf("duration is too long, maximum allowed is %s", maxDuration)
+ }
*d = Duration(duration)
return nil
}Note: The specific minimum and maximum values should be determined based on your governance requirements. Consider aligning with the default governance parameters or defining these values as constants.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Duration time.Duration | |
| func (d *Duration) UnmarshalJSON(b []byte) error { | |
| var s string | |
| if err := json.Unmarshal(b, &s); err != nil { | |
| return err | |
| } | |
| duration, err := time.ParseDuration(s) | |
| if err != nil { | |
| return err | |
| } | |
| *d = Duration(duration) | |
| return nil | |
| } | |
| func (d Duration) ToDuration() *time.Duration { | |
| duration := time.Duration(d) | |
| return &duration | |
| } | |
| func (d *Duration) UnmarshalJSON(b []byte) error { | |
| var s string | |
| if err := json.Unmarshal(b, &s); err != nil { | |
| return err | |
| } | |
| duration, err := time.ParseDuration(s) | |
| if err != nil { | |
| return err | |
| } | |
| // Add validation to ensure the duration is reasonable | |
| minDuration := 1 * time.Hour // Example: 1 hour minimum | |
| maxDuration := 30 * 24 * time.Hour // Example: 30 days maximum | |
| if duration < minDuration { | |
| return fmt.Errorf("duration is too short, minimum allowed is %s", minDuration) | |
| } | |
| if duration > maxDuration { | |
| return fmt.Errorf("duration is too long, maximum allowed is %s", maxDuration) | |
| } | |
| *d = Duration(duration) | |
| return nil | |
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@aaronc assigning you for review for this:
|
There was a problem hiding this comment.
Thanks for working on this @DuongNguyen62025 !
If I understand correctly, this allows for specifying a duration between the regular and expedited voting periods and scales it linearly. Is that right?
We need to document the algorithm and conditions thoroughly. Initially, I suggest adding this documentation to the new duration field in MsgSubmitProposal in the form of a specification for how this field will be used. Later we will need to add additional user facing docs.
Assuming we are fine with the specified algorithm, there are some issues to address in terms of calculating the target threshold deterministically using LegacyDec instead of float64.
| @@ -185,3 +187,23 @@ func ReadGovPropCmdFlags(proposer string, flagSet *pflag.FlagSet) (*govv1.MsgSub | |||
| func ReadGovPropFlags(clientCtx client.Context, flagSet *pflag.FlagSet) (*govv1.MsgSubmitProposal, error) { | |||
| return ReadGovPropCmdFlags(clientCtx.GetFromAddress().String(), flagSet) | |||
| } | |||
|
|
|||
| type Duration time.Duration | |||
There was a problem hiding this comment.
Proto3 json already has custom formatting for durations. The issue is that we're using encoding/json instead of the proto codec. We should just use proto json instead in parseSubmitProposal IMHO
There was a problem hiding this comment.
I don't quite understand what you mean. protojson.Unmarshal() is used for proto.Message and json.Unmarshal is used for any. So if using protojson I think we will need to change the proposal(add ProtoReflect method)
I have created 3 buttons with simapp for submitting proposal and voting, so tested it after adding duration field
Just seeing this @aljo242. Already left my initial review. Trying to first make sure that we get clarification on the specified behavior. It does appear backwards compatible. |
That's right, this is backwards compatible, a period of time that is not too fast or too slow that the proposer desires |
| // based on a desired proposal duration, interpolating between the normal and expedited values. | ||
| func (p Params) CalculateThresholdAndMinDeposit(duration time.Duration) (threshold string, minDeposit []types.Coin) { | ||
| // Ensure voting period ranges make sense | ||
| rate := (duration.Seconds() - p.ExpeditedVotingPeriod.Seconds()) / (p.VotingPeriod.Seconds() - p.ExpeditedVotingPeriod.Seconds()) |
Check notice
Code scanning / CodeQL
Floating point arithmetic Note
| // based on a desired proposal duration, interpolating between the normal and expedited values. | ||
| func (p Params) CalculateThresholdAndMinDeposit(duration time.Duration) (threshold string, minDeposit []types.Coin) { | ||
| // Ensure voting period ranges make sense | ||
| rate := (duration.Seconds() - p.ExpeditedVotingPeriod.Seconds()) / (p.VotingPeriod.Seconds() - p.ExpeditedVotingPeriod.Seconds()) |
Check notice
Code scanning / CodeQL
Floating point arithmetic Note
| // based on a desired proposal duration, interpolating between the normal and expedited values. | ||
| func (p Params) CalculateThresholdAndMinDeposit(duration time.Duration) (threshold string, minDeposit []types.Coin) { | ||
| // Ensure voting period ranges make sense | ||
| rate := (duration.Seconds() - p.ExpeditedVotingPeriod.Seconds()) / (p.VotingPeriod.Seconds() - p.ExpeditedVotingPeriod.Seconds()) |
Check notice
Code scanning / CodeQL
Floating point arithmetic Note
There was a problem hiding this comment.
Logic mostly looks good in terms of how this will be working. I assume that the dynamic amounts will be hard to reason about when constructing a gov prop, so I think it would be nice to include some client/CLI functions to allow simulation to show you what the calculated values will be before you submit a prop.
I can add a query to preview Threshold and MinDeposit from suggested duration. What do you think? type QueryThresholdAndMinDepositFromDurationRequest struct {
Duration time.Duration `protobuf:"varint,1,opt,name=duration,json=duration,proto3" json:"duration,omitempty"`
}type QueryThresholdAndMinDepositFromDurationResponse struct {
MinDeposit []types.Coin `protobuf:"bytes,1,rep,name=min_deposit,json=minDeposit,proto3" json:"min_deposit"`
Threshold string `protobuf:"bytes,2,opt,name=threshold,proto3" json:"threshold,omitempty"`
} |
Description
Closes: #24535
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. Your PR will not be merged unless you satisfy
all of these items.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores