introduce a whitelist of emergency proposal submitters #404
Conversation
📝 WalkthroughWalkthroughThis update introduces the concept of "emergency submitters" to the governance module. It adds a repeated string field for emergency submitters in the parameters, new protobuf messages and RPCs for managing them, full reflection and serialization support, server-side logic for add/remove operations, and comprehensive tests. Emergency proposal activation now requires the proposer to be an authorized emergency submitter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MsgServer
participant Keeper
participant ParamStore
Client->>MsgServer: AddEmergencyProposalSubmitters(authority, submitters)
MsgServer->>Keeper: GetParams()
Keeper->>ParamStore: Fetch Params
ParamStore-->>Keeper: Params
MsgServer->>MsgServer: Validate authority, check/merge submitters
MsgServer->>Keeper: SetParams(updated Params)
Keeper->>ParamStore: Save Params
MsgServer-->>Client: MsgAddEmergencyProposalSubmittersResponse
Client->>MsgServer: RemoveEmergencyProposalSubmitters(authority, submitters)
MsgServer->>Keeper: GetParams()
Keeper->>ParamStore: Fetch Params
ParamStore-->>Keeper: Params
MsgServer->>MsgServer: Validate authority, remove submitters
MsgServer->>Keeper: SetParams(updated Params)
Keeper->>ParamStore: Save Params
MsgServer-->>Client: MsgRemoveEmergencyProposalSubmittersResponse
sequenceDiagram
participant User
participant Keeper
participant Proposal
participant Params
User->>Keeper: AddDeposit(proposalID, depositor, amount)
Keeper->>Proposal: Get proposal
Keeper->>Params: Get Params
alt Proposal is in voting period AND deposit >= emergency min
alt Proposer is in EmergencySubmitters
Keeper->>Proposal: ActivateEmergencyProposal
else Proposer not in EmergencySubmitters
Keeper->>Proposal: No emergency activation
end
end
Keeper-->>User: Result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
api/initia/gov/v1/gov.pulsar.go (1)
492-503: 🛠️ Refactor suggestionRange order looks correct, but double-check
LowThresholdFunctions/EmergencySubmittersserializationThe newly inserted branches in
Rangecorrectly iterate overEmergencySubmitters(93) beforeLowThresholdFunctions(94), preserving ascending tag order.
However, because both fields share the same element type ([]string), any accidental mismatch elsewhere (e.g. in keeper logic or migrations) will be very hard to spot at runtime.Consider adding a sanity check in the keeper when loading params:
if params.LowThresholdFunctions == nil { params.LowThresholdFunctions = []string{} } if params.EmergencySubmitters == nil { params.EmergencySubmitters = []string{} }and unit tests that round-trip encode/decode params to ensure both slices stay distinct.
🧹 Nitpick comments (4)
x/gov/keeper/custom_msg_server_test.go (2)
72-75: Avoid misleading variable naming –submittersis actually the fullParamsstruct
submitters, err := input.GovKeeper.Params.Get(ctx)retrieves the entire parameter object, not just the slice of submitters.
Renaming the variable (e.g.params) keeps the meaning clear and prevents accidental misuse later in the test.- submitters, err := input.GovKeeper.Params.Get(ctx) + params, err := input.GovKeeper.Params.Get(ctx) ... - require.Equal(t, []string{...}, submitters.EmergencySubmitters) + require.Equal(t, []string{...}, params.EmergencySubmitters)
86-88: Preferrequire.ElementsMatchfor order-agnostic list comparison
If the keeper ever changes the order in which addresses are appended, this test will fail even though the list is semantically identical. UsingElementsMatchexpresses intent while making the test less brittle.- require.Equal(t, [...], params.EmergencySubmitters) + require.ElementsMatch(t, [...], params.EmergencySubmitters)x/gov/keeper/custom_msg_server.go (2)
42-60: Quadratic behaviour & duplicate handling inAddEmergencyProposalSubmitters
for+slices.Containsyields O(n²) in the worst case. With a long whitelist (e.g. hundreds of validators) this becomes noticeable.
A small set/hash-map gives O(n) and eliminates intra-request duplicates:+seen := make(map[string]struct{}, len(req.EmergencySubmitters)) for _, submitter := range req.EmergencySubmitters { if _, err := k.authKeeper.AddressCodec().StringToBytes(submitter); err != nil { return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid submitter: %s", submitter) } - if !slices.Contains(params.EmergencySubmitters, submitter) { + if _, dup := seen[submitter]; dup { + continue // skip duplicate in the same request + } + seen[submitter] = struct{}{} + if !slices.Contains(params.EmergencySubmitters, submitter) { params.EmergencySubmitters = append(params.EmergencySubmitters, submitter) } }
52-56: Validation side-effect only – consider extracting to helper
StringToBytesis invoked purely for validation and its result discarded.
Encapsulating this in a small helper (isValidAddress(string) bool) makes the intent explicit and avoids repeated codec plumbing if more callers appear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/initia/gov/v1/tx_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (9)
api/initia/gov/v1/gov.pulsar.go(20 hunks)api/initia/gov/v1/tx.pulsar.go(5 hunks)proto/initia/gov/v1/gov.proto(1 hunks)proto/initia/gov/v1/tx.proto(2 hunks)x/gov/common_test.go(1 hunks)x/gov/keeper/custom_msg_server.go(2 hunks)x/gov/keeper/custom_msg_server_test.go(1 hunks)x/gov/keeper/deposit.go(2 hunks)x/gov/keeper/deposit_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
x/gov/common_test.go (1)
98-98: The change appropriately initializes the EmergencySubmitters parameter.This addition properly initializes the
EmergencySubmittersparameter with the first test address, ensuring that tests using this setup function have a valid emergency submitter configured. This change supports the new whitelist mechanism for emergency proposals.x/gov/keeper/deposit.go (2)
6-6: Appropriate import of the slices package.The import of the
slicespackage is correctly added to support the new emergency submitter whitelist check.
156-159:❓ Verification inconclusive
Emergency proposal activation now checks for authorized submitters.
The implementation correctly ensures that only whitelisted addresses can activate emergency proposals by adding the condition
slices.Contains(params.EmergencySubmitters, proposal.Proposer). This enhances security by restricting emergency proposal activation to trusted accounts.Consider what happens when
EmergencySubmittersis empty - this would effectively disable emergency proposals entirely. Let's verify this is the intended behavior:
🏁 Script executed:
#!/bin/bash # Check if there's any logic that handles the case when EmergencySubmitters is empty # Looking for checks on EmergencySubmitters emptiness rg -A 2 -B 2 "EmergencySubmitters\s*=\s*\[\]" --type go rg -A 2 -B 2 "len\(.*EmergencySubmitters\)" --type goLength of output: 7959
Confirm behavior when EmergencySubmitters is empty
The new check in
x/gov/keeper/deposit.go(lines 156–159) correctly gates emergency proposal activation to addresses inparams.EmergencySubmitters. However, if that slice is empty,slices.Containswill always returnfalse, effectively disabling all emergency proposals by default. Please verify that this “disable when unset” behavior is intentional and, if so, ensure it’s clearly documented in the module’s configuration defaults.• File:
x/gov/keeper/deposit.go, lines 156–159
• Behavior: emptyEmergencySubmitters⇒ no one can activate emergency proposalsproto/initia/gov/v1/gov.proto (2)
117-119: Well-defined emergency submitters field added to Params message.The
emergency_submittersfield is correctly defined as a repeated string with the appropriate Cosmos address scalar annotation. This enables storing the whitelist of addresses that can submit emergency proposals.
123-126: Appropriate renumbering of existing fields.The field numbers for
low_threshold_functionsandvestinghave been correctly incremented to accommodate the newemergency_submittersfield. This maintains backward compatibility while adding the new functionality.x/gov/keeper/deposit_test.go (3)
284-284: Test now properly configures emergency submitters.The test initialization correctly sets up the emergency submitters whitelist with the first test address, ensuring proper testing of the emergency proposal activation conditions.
332-332: Test appropriately sets up emergency submitters parameter.Similar to the previous test, this test correctly initializes the emergency submitters list with the first test address.
353-365: Great test coverage for the negative case.This test addition is excellent as it verifies both the positive case (authorized proposer's proposal becomes emergency) and the negative case (unauthorized proposer's proposal doesn't become emergency). It confirms that:
- A proposal from an authorized submitter (
addrs[0]) gets the emergency flag set when it meets the deposit requirement- A proposal from an unauthorized submitter (
addrs[1]) enters voting period but doesn't get the emergency flag set, even with sufficient depositThis comprehensive test coverage ensures the whitelist is properly enforced.
proto/initia/gov/v1/tx.proto (2)
21-26: Great addition – RPCs are clearly documented
The new RPCs are self-descriptive and follow existing naming conventions. Nice work!
54-57: Consider adding amin_items: 1validation note
Both add/remove messages allow emptyemergency_submitters; this becomes a no-op but still consumes gas/consensus resources.
If that behaviour isn’t desirable, document it here or enforce length > 0 in keeper methods.api/initia/gov/v1/tx.pulsar.go (1)
874-905: Skip reviewing generated code
This section is auto-generated; manual edits will be overwritten byprotoc. No review necessary.
api/initia/gov/v1/gov.pulsar.go
Outdated
| fd_Params_emergency_min_deposit protoreflect.FieldDescriptor | ||
| fd_Params_emergency_tally_interval protoreflect.FieldDescriptor | ||
| fd_Params_emergency_submitters protoreflect.FieldDescriptor | ||
| fd_Params_low_threshold_functions protoreflect.FieldDescriptor | ||
| fd_Params_vesting protoreflect.FieldDescriptor | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Renumbered protobuf tags can corrupt on-chain state – please add a migration step
low_threshold_functions moved from tag 93 → 94 and you inserted the new emergency_submitters at tag 93 (see the new fd_Params_* variables).
Any Params binary already persisted (or encoded in genesis) with field 93 will now be decoded into EmergencySubmitters, leaving LowThresholdFunctions empty and potentially authorising unintended addresses.
Unless main-net is being started from scratch, you must:
- Provide an x/gov store migration that rewrites existing
Paramsbytes, renumbering the field to 94. - Bump the module version in
InitGenesis/Upgradehandlers so the migration is executed automatically during the upgrade. - Add a regression test that encodes old-format params and ensures the migrator produces the expected new struct.
Failing to do so risks disabling low-threshold logic and permitting everyone on that list to submit emergency proposals.
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Verify protobuf tag numbers for Params fields in gov.pulsar.go
echo "=== Emergency Min Deposit Descriptor ==="
rg -n "fd_Params_emergency_min_deposit" -C2 api/initia/gov/v1/gov.pulsar.go
echo
echo "=== Emergency Submitters Descriptor ==="
rg -n "fd_Params_emergency_submitters" -C2 api/initia/gov/v1/gov.pulsar.go
echo
echo "=== Low Threshold Functions Descriptor ==="
rg -n "fd_Params_low_threshold_functions" -C2 api/initia/gov/v1/gov.pulsar.go
echo
echo "=== All FieldDescriptor .ByNumber Assignments ==="
rg -n "ByNumber" api/initia/gov/v1/gov.pulsar.goLength of output: 3585
Add a Params migration for renumbered protobuf fields
We’ve confirmed that in api/initia/gov/v1/gov.pulsar.go the new field
EmergencySubmitters now uses tag 93 (_Params_93_list) and
LowThresholdFunctions has been bumped to tag 94 (_Params_94_list).
Any existing on-chain or genesis‐encoded Params with field 93 will decode into EmergencySubmitters, leaving LowThresholdFunctions empty.
You must:
- Implement an x/gov store migration that rewrites any old
Paramsbytes, renumbering field 93→94. - Bump the gov module version in your
InitGenesisand upgrade handler so this migration runs automatically. - Add a regression test that encodes the old‐format
Paramsand verifies the migrator produces the correct new struct.
Failing to do so risks disabling low‐threshold logic and granting unintended addresses emergency‐proposal rights.
🤖 Prompt for AI Agents
In api/initia/gov/v1/gov.pulsar.go around lines 286 to 291, the protobuf tag for
Params field EmergencySubmitters was assigned tag 93, pushing
LowThresholdFunctions from tag 93 to 94. This causes existing on-chain or
genesis Params data with tag 93 to decode incorrectly. To fix this, implement an
x/gov store migration that rewrites stored Params bytes, renumbering field 93 to
94 for LowThresholdFunctions. Then, bump the gov module version in InitGenesis
and upgrade handlers to trigger this migration automatically during upgrades.
Finally, add a regression test encoding old-format Params and verify the
migration produces the correct new struct, ensuring no loss of low-threshold
logic or unintended permissions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
+ Coverage 40.10% 40.14% +0.04%
==========================================
Files 295 295
Lines 27679 27718 +39
==========================================
+ Hits 11100 11127 +27
- Misses 14873 14881 +8
- Partials 1706 1710 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/initia/gov/v1/gov.pulsar.go (1)
222-266: Good job addressing prior review comments on protobuf tag assignmentsI see the new EmergencySubmitters field is assigned protobuf tag 95, while LowThresholdFunctions remains at tag 93 and Vesting at tag 94. This properly addresses the previous review concern about renumbering field tags potentially corrupting on-chain state. Since you're adding a new field with a new tag number rather than renumbering existing fields, no migration is needed.
Also applies to: 290-290, 316-316, 504-509, 565-566, 623-624, 713-718, 787-790, 860-865, 950-952, 1107-1112, 1142-1152, 2055-2086, 5073-5074, 5237-5242
🧹 Nitpick comments (2)
x/gov/keeper/custom_msg_server_test.go (2)
90-118: Consider adding a test for removing a non-existent submitter from an empty list.While you're testing removal from a populated list, it would be good to also verify the behavior when attempting to remove submitters from an initially empty list.
76-88: Verify the expected behavior for duplicate submitters.The test adds addrs[2] twice, but doesn't explicitly assert that duplicates are handled correctly. Consider adding a comment or specific assertion to clarify the expected behavior when adding duplicate submitters.
+// Adding duplicate submitter (addrs[2]) should not result in duplicates in the list _, err = ms.AddEmergencyProposalSubmitters(ctx, &types.MsgAddEmergencyProposalSubmitters{ Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(), EmergencySubmitters: []string{ addrs[2].String(), addrs[3].String(), addrs[4].String(), }, }) require.NoError(t, err) submitters, err = input.GovKeeper.Params.Get(ctx) require.NoError(t, err) require.Equal(t, []string{addrs[0].String(), addrs[1].String(), addrs[2].String(), addrs[3].String(), addrs[4].String()}, submitters.EmergencySubmitters) +// Verify no duplicates exist in the final list +require.Equal(t, len(submitters.EmergencySubmitters), 5, "No duplicates should exist in the list")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (2)
x/gov/types/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/gov/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (7)
api/initia/gov/v1/gov.pulsar.go(17 hunks)api/initia/gov/v1/query.pulsar.go(1 hunks)proto/initia/gov/v1/gov.proto(1 hunks)proto/initia/gov/v1/query.proto(2 hunks)proto/initia/gov/v1/tx.proto(2 hunks)x/gov/keeper/custom_msg_server.go(2 hunks)x/gov/keeper/custom_msg_server_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- proto/initia/gov/v1/query.proto
- api/initia/gov/v1/query.pulsar.go
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/initia/gov/v1/gov.proto
- x/gov/keeper/custom_msg_server.go
- proto/initia/gov/v1/tx.proto
🔇 Additional comments (6)
x/gov/keeper/custom_msg_server_test.go (1)
41-139: Comprehensive test coverage for emergency proposal submitters functionality.The test suite covers critical paths including authority validation, address validation, duplicate handling, error cases, and successful operations. The assertions verify the expected state after each operation.
api/initia/gov/v1/gov.pulsar.go (5)
222-266: Implementation of emergency submitters protobuf list wrapper looks goodThe added code properly implements the necessary protoreflect.List interface for handling the repeated string field for emergency submitters, consistent with existing patterns in the codebase.
290-290: Field descriptor declaration and initialization is well implementedThe new field descriptor for emergency_submitters is correctly declared and initialized, following the established pattern in the code.
Also applies to: 316-316
504-509: Reflection methods properly updated to support emergency submittersAll necessary reflection methods (Range, Has, Clear, Get, Set, Mutable, NewField) have been correctly updated to handle the new EmergencySubmitters field, maintaining consistency with existing field patterns.
Also applies to: 565-566, 623-624, 713-718, 787-790, 860-865, 950-952
1107-1112: Serialization and deserialization implementation is correctThe ProtoMethods functions for size calculation, marshaling, and unmarshaling are properly updated to include the EmergencySubmitters field with the correct field number (95) and wire encoding.
Also applies to: 1142-1152, 2055-2086
5073-5074: Parameter field definition and accessor method are well definedThe EmergencySubmitters field is correctly added to the Params struct with appropriate documentation, and the getter method is properly implemented. The cosmos.AddressString validation annotation ensures these will be validated as proper blockchain addresses.
Also applies to: 5237-5242
close #402