refactor(gov): simplify state management #19349
Conversation
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 47 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update enhances the governance module in the Cosmos SDK by simplifying proposal management through the use of an Changes
Related issues
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? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: .coderabbit.yml
Files selected for processing (8)
- x/gov/abci.go (13 hunks)
- x/gov/genesis.go (1 hunks)
- x/gov/keeper/deposit.go (1 hunks)
- x/gov/keeper/keeper.go (3 hunks)
- x/gov/keeper/proposal.go (3 hunks)
- x/gov/migrations/v6/store.go (1 hunks)
- x/gov/types/keys.go (2 hunks)
- x/gov/types/v1/proposal.go (4 hunks)
Additional comments: 12
x/gov/types/keys.go (2)
- 28-29: The introduction of
ProposalIndexKeyaligns with the refactor to useIndexedMapfor proposals, facilitating efficient indexing and access.- 33-34: Adding reserved KVStore keys without specific usage might be for future expansion or compatibility. Ensure their intended use is documented or tracked to avoid confusion.
x/gov/migrations/v6/store.go (1)
- 13-18: The introduction of
ActiveProposalsQueueandInactiveProposalsQueueusingcollections.Mapis a significant change. Ensure that the migration logic correctly handles the transition from the old data structure to these new types.x/gov/genesis.go (1)
- 59-59: Directly setting proposals in the
Proposalscollection duringInitGenesissimplifies the initialization process. This change should be reflected in the module's genesis tests to ensure compatibility and correctness.x/gov/types/v1/proposal.go (3)
- 81-81: The
UnpackInterfacesmethod implementation forProposalis necessary for the correct handling of polymorphic message types. Ensure all message types used in proposals are registered for interface unpacking.Verification successful
The examination of the
RegisterInterfacesfunction withincodec.goreveals that a variety of message types related to governance proposals, including submission, voting, depositing, and administrative actions, are indeed registered. This aligns with the necessity of ensuring all message types used in proposals are properly accounted for interface unpacking, as highlighted in the review comment. Without further evidence of missing message types necessary for proposals, it can be concluded that the implementation addresses the concern raised.* 101-101: The `UnpackInterfaces` method for `Proposals` ensures that the interface unpacking is propagated to each `Proposal` in the slice. This is crucial for the correct deserialization of messages within proposals. * 149-193: The introduction of `ProposalIndexes` with specific indexes for proposal status and end times is a significant improvement for querying and managing proposals. Ensure that these indexes are correctly utilized in proposal queries throughout the module.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify registration of all message types used in proposals grep -r "RegisterInterfaces" ./x/gov/types/v1/Length of output: 245
Script:
#!/bin/bash # Extract and review the contents of the RegisterInterfaces function cat ./x/gov/types/v1/codec.go | grep -A 20 "func RegisterInterfaces"Length of output: 467
Verification successful
The examination of
ProposalIndexesusage within thekeeper.goandmigrations/v6/store.gofiles indicates that these indexes are integrated into the governance module's core data structures and are utilized in migration logic. This suggests thatProposalIndexesare indeed being used to facilitate efficient querying and management of proposals based on their status and end times, aligning with the intended improvements mentioned in the review comment. While the specific query operations leveraging these indexes were not directly observed, the evidence of their integration and usage in migration provides a reasonable basis to conclude thatProposalIndexesare correctly utilized in proposal queries within the observed contexts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct utilization of ProposalIndexes in proposal queries grep -r "ProposalIndexes" ./x/gov/Length of output: 1052
Script:
#!/bin/bash # Examine the usage of ProposalIndexes in keeper.go and migrations/v6/store.go grep -A 10 -B 3 "ProposalIndexes" ./x/gov/keeper/keeper.go grep -A 10 -B 3 "ProposalIndexes" ./x/gov/migrations/v6/store.goLength of output: 2925
x/gov/keeper/proposal.go (2)
- 122-122: Using
k.Proposals.Setdirectly inSubmitProposalreflects the changes in proposal management. Ensure that the proposal ID sequence is correctly managed to avoid ID collisions.Verification successful
The script output confirms the presence of
ProposalID.Nextin./x/gov/keeper/proposal.go, indicating that there is a mechanism in place for managing proposal ID sequences to avoid collisions, as mentioned in the review comment. This suggests that the concern regarding the management of proposal ID sequences has been addressed in the codebase. However, without a deeper analysis, the effectiveness and correctness of this implementation cannot be fully verified. Based on the information available, it appears that the review comment has been addressed.* 267-267: Directly setting the proposal in `ActivateVotingPeriod` using `k.Proposals.Set` is consistent with the new data structure. Confirm that the voting period activation logic correctly updates the proposal's status and timing.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify proposal ID sequence management grep -r "ProposalID.Next" ./x/gov/keeper/Length of output: 111
Verification successful
The provided script output reveals the implementation details of the
ActivateVotingPeriodfunction withinproposal.go. This function performs several key operations as part of activating the voting period for a proposal:
- It unwraps the SDK context to obtain the current blockchain time, which is then set as the proposal's voting start time.
- It retrieves the governance parameters, handling any errors that might occur during this process.
- Based on the proposal type, it determines the voting period duration. For expedited proposals, it uses a different, presumably shorter, voting period than the default.
- It also includes logic to check for custom message parameters if the proposal contains any messages, indicating that further customization of the voting period could be possible based on the proposal's content.
This implementation confirms that the
ActivateVotingPeriodfunction updates the proposal's status by setting the voting start time and determining the voting period's duration based on the proposal type and potentially the content of the proposal messages. This aligns with the review comment's request to verify that the voting period activation logic correctly updates the proposal's status and timing.Given this information, it can be concluded that the review comment is verified as the implementation of
ActivateVotingPeriodindeed handles the activation of the voting period in a manner consistent with the expectations outlined in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure correct voting period activation logic grep -r "ActivateVotingPeriod" ./x/gov/keeper/Length of output: 1421
Script:
#!/bin/bash # View the implementation of ActivateVotingPeriod in proposal.go cat ./x/gov/keeper/proposal.go | grep -A 20 "func (k Keeper) ActivateVotingPeriod"Length of output: 773
x/gov/keeper/keeper.go (1)
- 64-65: Transforming
Proposalsinto anIndexedMapwith additional indexes for status and end times is a key part of the refactor. This change should significantly improve the efficiency of proposal queries.x/gov/keeper/deposit.go (1)
- 144-144: The update to use
k.Proposals.Setfor updating proposals inAddDepositaligns with the new proposal management approach. Ensure that deposit handling correctly updates the proposal's total deposit and activates the voting period when necessary.Verification successful
The examination of the
AddDepositfunction withindeposit.goreveals the following:
Proposal's Total Deposit Update: The proposal's total deposit is updated by adding the new deposit amount to the existing total deposit. This is done with the line
proposal.TotalDeposit = sdk.NewCoins(proposal.TotalDeposit...).Add(depositAmount...).Activation of the Voting Period: The voting period is activated if the proposal's status is in the deposit period and the total deposit meets or exceeds the minimum deposit amount required. This is checked and executed with the following code snippet:
activatedVotingPeriod := false if proposal.Status == v1.StatusDepositPeriod && sdk.NewCoins(proposal.TotalDeposit...).IsAllGTE(minDepositAmount) { err = k.ActivateVotingPeriod(ctx, proposal) if err != nil { return false, err } activatedVotingPeriod = true }If the conditions are met,
k.ActivateVotingPeriodis called to transition the proposal into the voting period, andactivatedVotingPeriodis set totrue.These findings confirm that the
AddDepositfunction correctly handles the updating of the proposal's total deposit and activates the voting period when the necessary conditions are met. Therefore, the review comment's request to ensure that deposit handling correctly updates the proposal's total deposit and activates the voting period when necessary is verified to be correctly implemented in the code.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify correct handling of deposits and voting period activation grep -r "AddDeposit" ./x/gov/keeper/Length of output: 1331
Script:
#!/bin/bash # Inspect the implementation of AddDeposit in deposit.go cat ./x/gov/keeper/deposit.goLength of output: 11216
x/gov/abci.go (1)
- 216-216: Ensure that the updated proposal status and type are correctly persisted before proceeding with further operations.
x/gov/abci.go
Outdated
| proposal, err := keeper.Proposals.Get(ctx, proposalID) | ||
| if err != nil { | ||
| // if the proposal has an encoding error, this means it cannot be processed by x/gov | ||
| // this could be due to some types missing their registration | ||
| // instead of returning an error (i.e, halting the chain), we fail the proposal | ||
| if errors.Is(err, collections.ErrEncoding) { | ||
| proposal.Id = key.K2() | ||
| if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), false); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil { | ||
| if err = keeper.DeleteProposal(ctx, proposalID); err != nil { |
There was a problem hiding this comment.
Ensure that failUnsupportedProposal logs sufficient context about the proposal that failed due to encoding errors, including its ID and the specific error encountered.
x/gov/migrations/v6/store.go
Outdated
| // MigrateStore performs in-place store migrations from v5 (v0.50) to v6 (v0.51). The | ||
| // migration includes: | ||
| // | ||
| // Addition of new field in params to store types of proposals that can be submitted. | ||
| // Addition of gov params for optimistic proposals. | ||
| func MigrateStore(ctx sdk.Context, paramsCollection collections.Item[v1.Params], proposalCollection collections.Map[uint64, v1.Proposal]) error { | ||
| func MigrateStore(ctx sdk.Context, paramsCollection collections.Item[v1.Params], proposalCollection *collections.IndexedMap[uint64, v1.Proposal, v1.ProposalIndexes]) error { |
There was a problem hiding this comment.
Tip
Codebase Verification
The review of store_test.go reveals that the test TestMigrateStore primarily focuses on the migration of parameters (paramsCollection) and does not explicitly address the migration of proposalCollection, especially concerning edge cases in proposal statuses and times. This indicates that the original concern about ensuring comprehensive testing for the migration paths of proposalCollection, as highlighted in the review comment, remains valid. The test coverage for the proposalCollection migration, particularly focusing on edge cases, appears to be insufficient based on the provided context.
Analysis chain
The signature change of MigrateStore to use *collections.IndexedMap for proposalCollection is crucial for the new proposal management system. Confirm that all migration paths are tested, especially for edge cases in proposal statuses and times.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Ensure comprehensive testing of MigrateStore function, focusing on proposalCollection migrations
grep -r "func TestMigrateStore" ./x/gov/migrations/v6/
Length of output: 130
Script:
#!/bin/bash
# Extract and review the contents of TestMigrateStore to assess the comprehensiveness of the testing
cat ./x/gov/migrations/v6/store_test.go
Length of output: 2013
x/gov/migrations/v6/store_test.go
Outdated
| @@ -47,4 +47,7 @@ func TestMigrateStore(t *testing.T) { | |||
| require.Equal(t, v1.DefaultParams().ProposalCancelMaxPeriod, newParams.ProposalCancelMaxPeriod) | |||
| require.Equal(t, v1.DefaultParams().OptimisticAuthorizedAddresses, newParams.OptimisticAuthorizedAddresses) | |||
| require.Equal(t, v1.DefaultParams().OptimisticRejectedThreshold, newParams.OptimisticRejectedThreshold) | |||
|
|
|||
| // Check proposals | |||
| // TODO: add mock proposals and check if they were migrated correctly | |||
There was a problem hiding this comment.
Review Status
Actionable comments generated: 31
Configuration used: .coderabbit.yml
Files selected for processing (18)
- tests/integration/gov/abci_test.go (2 hunks)
- tests/integration/gov/keeper/grpc_query_test.go (1 hunks)
- tests/integration/gov/keeper/tally_test.go (15 hunks)
- tests/sims/gov/operations_test.go (2 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/abci.go (13 hunks)
- x/gov/keeper/grpc_query_test.go (14 hunks)
- x/gov/keeper/keeper.go (3 hunks)
- x/gov/keeper/keeper_test.go (2 hunks)
- x/gov/keeper/migrations.go (1 hunks)
- x/gov/keeper/proposal.go (4 hunks)
- x/gov/keeper/proposal_test.go (4 hunks)
- x/gov/keeper/vote.go (1 hunks)
- x/gov/keeper/vote_test.go (2 hunks)
- x/gov/migrations/v6/store.go (2 hunks)
- x/gov/migrations/v6/store_test.go (3 hunks)
- x/gov/types/keys.go (1 hunks)
- x/gov/types/v1/proposal.go (4 hunks)
Additional comments: 37
x/gov/types/keys.go (1)
- 19-28: Ensure the new key prefixes introduced are correctly utilized throughout the module, especially in relation to proposal indexing.
Verification successful
The search results indicate that the
ProposalEndDepositPeriodIndexKeyandProposalEndVotingPeriodIndexKeyare indeed utilized within the module, specifically inx/gov/types/v1/proposal.go, which suggests that the new key prefixes introduced for proposal indexing are being correctly used as intended. Based on the provided evidence, there are no issues found with the utilization of these key prefixes in the context of proposal indexing.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of new key prefixes for proposal indices rg --type go 'ProposalEndDepositPeriodIndexKey|ProposalEndVotingPeriodIndexKey'Length of output: 509
x/gov/migrations/v6/store_test.go (1)
- 52-52: Skip generating a similar comment as instructed.
x/gov/keeper/vote.go (2)
- 18-28: Validate the proposal status directly using
proposal.Statusinstead of a separate check, ensuring consistency with the new proposal management logic.- 28-28: Ensure metadata length assertion is correctly implemented and aligns with governance module requirements.
x/gov/CHANGELOG.md (1)
- 61-61: Confirm that the removal of specific functions and the introduction of
ProposalTypeparameter are accurately reflected in the module's documentation and implementation.Verification successful
Given the absence of the specified functions in the search results, it appears that the removal of
InactiveProposalsQueue,ActiveProposalsQueue,VotingPeriodProposals,DeleteProposal, andSetProposalhas been verified. To fully address the review comment, we now need to verify the introduction and correct reflection of theProposalTypeparameter in the module's documentation and implementation. Since this aspect was not covered in the initial verification, further investigation is required.
The verification process has confirmed that the specific functions mentioned (
InactiveProposalsQueue,ActiveProposalsQueue,VotingPeriodProposals,DeleteProposal,SetProposal) have been removed, as no occurrences were found in the Go files. Additionally, the introduction and usage of theProposalTypeparameter have been verified across various Go files, indicating its implementation within the module. TheProposalTypeparameter's presence and usage are also reflected in the module's documentation, as seen in the CHANGELOG.md and architecture documentation. This confirms that the removal of specific functions and the introduction of theProposalTypeparameter are accurately reflected in the module's documentation and implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for removed functions and new ProposalType parameter usage rg --type go 'InactiveProposalsQueue|ActiveProposalsQueue|VotingPeriodProposals|DeleteProposal|SetProposal'Length of output: 109
Script:
#!/bin/bash # Search for the usage of ProposalType parameter in Go files rg --type go 'ProposalType' # Additionally, check for mentions of ProposalType in documentation files rg --type md 'ProposalType'Length of output: 40934
x/gov/keeper/keeper_test.go (2)
- 108-108: Rename
TestProposalQueuestoTestProposalIndexesto reflect the new indexing strategy for proposals.- 122-128: Verify the correct usage of proposal indexes for deposit and voting end times, ensuring proposals are accurately tracked.
Verification successful
The verification process has confirmed that the implementation of proposal indexes for deposit and voting end times (
StatusDepositEndTimeandStatusVotingEndTime) is correctly utilized within the tests and other parts of the governance module. The usage inkeeper_test.gofor matching proposals based on their deposit and voting end times aligns with the definitions and usages found inproposal.goandabci.go, ensuring proposals are accurately tracked as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify correct implementation of proposal indexes in tests rg --type go 'StatusDepositEndTime|StatusVotingEndTime'Length of output: 1229
x/gov/types/v1/proposal.go (3)
- 81-81: Update
UnpackInterfacesmethod to usecodectypes.AnyUnpackerfor compatibility with the latest codec changes.- 78-91: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [88-101]
Implement
codectypes.UnpackInterfacesMessageforProposalsto ensure compatibility with interface unpacking.
- 149-193: Introduce
ProposalIndexeswith methods for managing proposal indexes, ensuring efficient proposal management.x/gov/keeper/proposal.go (3)
- 122-122: Directly set proposals using
k.Proposals.Setto streamline proposal submission.- 203-203: Remove proposals directly using
k.Proposals.Removeto simplify proposal cancellation.- 249-249: Activate voting period by directly updating the proposal status and end time using
k.Proposals.Set.x/gov/keeper/keeper.go (2)
- 64-65: The transition to
IndexedMapforProposalsaligns with the PR's goal of simplifying state management. Ensure that all interactions withProposalsthroughout the codebase have been updated to utilize theIndexedMapmethods correctly.- 113-129: The
NewKeeperfunction correctly initializes theIndexedMapforProposalsand other collections. Verify that the removal of specific queues for active and inactive proposals and their replacement withIndexedMapdoes not introduce any regressions in proposal lifecycle management.x/gov/keeper/proposal_test.go (3)
- 43-43: Correct usage of
Proposals.Removeto delete a proposal after testing its activation. This aligns with the new approach to managing proposals.- 75-75: Correctly uses
Proposals.Removeto delete a proposal within the voting period, ensuring the test reflects the updated proposal management logic.- 209-209: Using
Proposals.Setto update a proposal's status before testing cancellation is appropriate and aligns with the new data structure operations.tests/integration/gov/abci_test.go (5)
- 38-38: Ensure that the proposal ID and object passed to
Proposals.Setare correct and that the proposal's status is appropriately managed before and after this operation.- 62-62: Verify that the proposal status is correctly set to
StatusVotingPeriodbefore callingProposals.Set, as this affects the logic inEndBlockerand the final assertion of the proposal's status.- 36-36: Setting the
DepositEndTimedirectly on the proposal object before storing it withProposals.Setis correct. Ensure that the time manipulation aligns with the test's intent and that the proposal's timing fields are consistent with the test scenario.- 38-38: The error handling after
Proposals.Setis correct, expecting no error. This aligns with the expected behavior when storing a proposal in the governance module's keeper.- 62-62: Correct use of
Proposals.Setfor storing the proposal in the voting period status. Ensure that subsequent operations in the test (e.g.,EndBlocker, proposal status checks) are consistent with this setup.x/gov/keeper/grpc_query_test.go (14)
- 300-300: Use of
Proposals.Setmethod correctly replaces the previousSetProposalmethod, aligning with the refactor. Ensure related logic correctly handles the proposal object's state.- 523-523: Correct application of
Proposals.Setfor updating proposal status toStatusVotingPeriod. This change is consistent with the refactor's objectives.- 640-640: The use of
Proposals.Setin the legacy GRPC query test is appropriate and ensures backward compatibility with the updated proposal management approach.- 746-746: The application of
Proposals.Setfor setting the proposal status before adding votes is correctly implemented, ensuring the proposal is in the expected state for the test.- 852-852: Usage of
Proposals.Setin the context of legacy GRPC query tests for votes is correctly done, maintaining consistency with the new proposal management mechanism.- 1558-1558: Setting a proposal with status
StatusPassedusingProposals.Setfor testing tally result query is correctly implemented. This setup is crucial for testing the query's behavior with a finalized proposal.- 1585-1585: The setup of a proposal in
StatusDepositPeriodusingProposals.Setfor testing tally result query correctly simulates a scenario where the proposal is not yet in the voting period.- 1612-1612: Correctly setting a proposal in
StatusVotingPeriodusingProposals.Setfor testing tally result query. This setup is essential for testing the query's behavior during an active voting period.- 1703-1703: The use of
Proposals.Setfor setting a proposal with statusStatusPassedin the context of legacy GRPC query tests for tally results is correctly implemented, ensuring the test accurately reflects the updated proposal management logic.- 1729-1729: Setting up a proposal in
StatusDepositPeriodusingProposals.Setfor legacy GRPC query tests for tally results is done correctly, simulating a scenario relevant for testing the query's behavior before the voting period.- 1755-1755: Correct application of
Proposals.Setfor a proposal inStatusVotingPeriodwithin legacy GRPC query tests for tally results. This ensures the test scenario accurately represents an ongoing voting period.- 1823-1823: The setup for testing non-multiple choice proposal vote options using
Proposals.Setis correctly implemented, providing a valid scenario for testing default vote options.- 1851-1851: The setup for an invalid multiple choice proposal without explicitly set vote options, using
Proposals.Set, correctly falls back to default vote options, demonstrating the handling of incomplete proposal configurations.- 1882-1882: Correctly setting up a multiple choice proposal with specific vote options using
Proposals.SetandProposalVoteOptions.Set. This setup is essential for testing the query's behavior with custom vote options.
x/gov/abci.go
Outdated
| return false, err | ||
| } | ||
| if !params.BurnProposalDepositPrevote { | ||
| err = keeper.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal | ||
| err = keeper.RefundAndDeleteDeposits(ctx, proposalID) // refund deposit if proposal got removed without getting 100% of the proposal | ||
| } else { | ||
| err = keeper.DeleteAndBurnDeposits(ctx, proposal.Id) // burn the deposit if proposal got removed without getting 100% of the proposal | ||
| err = keeper.DeleteAndBurnDeposits(ctx, proposalID) // burn the deposit if proposal got removed without getting 100% of the proposal | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [48-59]
Refactor the conditional block handling deposit refunds and burns into a separate function to improve readability and maintainability.
x/gov/abci.go
Outdated
|
|
||
| // called when proposal become inactive | ||
| cacheCtx, writeCache := ctx.CacheContext() | ||
| err = keeper.Hooks().AfterProposalFailedMinDeposit(cacheCtx, proposal.Id) | ||
| err = keeper.Hooks().AfterProposalFailedMinDeposit(cacheCtx, proposalID) | ||
| if err == nil { // purposely ignoring the error here not to halt the chain if the hook fails | ||
| writeCache() | ||
| } else { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-78]
Ignoring errors from hooks can lead to unnoticed failures in proposal processing. Consider logging the error at a minimum or handling it more explicitly.
x/gov/abci.go
Outdated
| sdk.NewAttribute(types.AttributeKeyProposalResult, types.AttributeValueProposalDropped), | ||
| ), | ||
| ) | ||
|
|
||
| logger.Info( | ||
| "proposal did not meet minimum deposit; deleted", | ||
| "proposal", proposal.Id, | ||
| "proposal", proposalID, |
There was a problem hiding this comment.
The log message for a proposal not meeting the minimum deposit should include the current block time for context.
x/gov/abci.go
Outdated
| rng = collections.NewPrefixUntilPairRange[collections.Pair[int32, time.Time], uint64](collections.Join(int32(v1.StatusVotingPeriod), ctx.HeaderInfo().Time)) | ||
| err = keeper.Proposals.Indexes.StatusVotingEndTime.Walk(ctx, rng, func(key collections.Pair[int32, time.Time], proposalID uint64) (bool, error) { |
There was a problem hiding this comment.
Similar to the earlier comment, pre-calculate collections.Join(int32(v1.StatusVotingPeriod), ctx.HeaderInfo().Time) outside the Walk function call.
| @@ -261,7 +261,7 @@ | |||
| proposal, err := v1.NewProposal([]sdk.Msg{contentMsg}, 1, submitTime, submitTime.Add(*depositPeriod), "", "text proposal", "description", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), v1.ProposalType_PROPOSAL_TYPE_STANDARD) | |||
| require.NoError(t, err) | |||
|
|
|||
| err = suite.GovKeeper.SetProposal(ctx, proposal) | |||
| err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Ensure error handling is consistent and robust for the call to suite.GovKeeper.Proposals.Set.
- err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal)
- require.NoError(t, err)
+ require.NoError(t, err, "setting proposal should not error")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.
| err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |
| require.NoError(t, err, "setting proposal should not error") |
| @@ -38,7 +38,7 @@ func TestLegacyGRPCQueryTally(t *testing.T) { | |||
| proposal, err := f.govKeeper.SubmitProposal(ctx, TestProposal, "", "test", "description", addrs[0], v1.ProposalType_PROPOSAL_TYPE_STANDARD) | |||
| assert.NilError(t, err) | |||
| proposal.Status = v1.StatusVotingPeriod | |||
| err = f.govKeeper.SetProposal(ctx, proposal) | |||
| err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Replace f.govKeeper.SetProposal with f.govKeeper.Proposals.Set to align with the new proposal management approach.
- f.govKeeper.SetProposal(ctx, proposal.Id, proposal)
+ err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal)
+ require.NilError(t, err)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.
| err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |
| err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |
| require.NilError(t, err) |
x/gov/migrations/v6/store.go
Outdated
| // MigrateStore performs in-place store migrations from v5 (v0.50) to v6 (v0.51). The | ||
| // migration includes: | ||
| // | ||
| // Addition of new field in params to store types of proposals that can be submitted. | ||
| // Addition of gov params for optimistic proposals. | ||
| func MigrateStore(ctx sdk.Context, paramsCollection collections.Item[v1.Params], proposalCollection collections.Map[uint64, v1.Proposal]) error { | ||
| // Migrate proposals | ||
| func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, paramsCollection collections.Item[v1.Params], proposalCollection *collections.IndexedMap[uint64, v1.Proposal, v1.ProposalIndexes]) error { |
There was a problem hiding this comment.
Ensure error handling is correctly implemented for the MigrateStore function, especially when dealing with proposal migrations and clearing old proposal queues.
+ if err != nil {
+ return fmt.Errorf("error migrating proposals: %w", err)
+ }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.
| func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, paramsCollection collections.Item[v1.Params], proposalCollection *collections.IndexedMap[uint64, v1.Proposal, v1.ProposalIndexes]) error { | |
| func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, paramsCollection collections.Item[v1.Params], proposalCollection *collections.IndexedMap[uint64, v1.Proposal, v1.ProposalIndexes]) error { | |
| if err != nil { | |
| return fmt.Errorf("error migrating proposals: %w", err) | |
| } |
| @@ -32,7 +32,7 @@ func TestVotes(t *testing.T) { | |||
| require.Error(t, govKeeper.AddVote(ctx, 10, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), ""), "invalid proposal ID") | |||
|
|
|||
| proposal.Status = v1.StatusVotingPeriod | |||
| err = govKeeper.SetProposal(ctx, proposal) | |||
| err = govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Align with the new proposal management approach by using govKeeper.Proposals.Set for setting proposals in tests.
- govKeeper.SetProposal(ctx, proposal.Id, proposal)
+ require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal))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.
| err = govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | |
| require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal)) |
| @@ -123,7 +123,7 @@ | |||
| require.NoError(t, err) | |||
|
|
|||
| proposal.Status = v1.StatusVotingPeriod | |||
| require.NoError(t, govKeeper.SetProposal(ctx, proposal)) | |||
| require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal)) | |||
There was a problem hiding this comment.
Ensure proposals are correctly set using govKeeper.Proposals.Set in the context of multiple choice proposals.
- govKeeper.SetProposal(ctx, proposal.Id, proposal)
+ require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal))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.
| require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal)) | |
| require.NoError(t, govKeeper.Proposals.Set(ctx, proposal.Id, proposal)) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/gov/abci.go (12 hunks)
Additional comments: 7
x/gov/abci.go (7)
- 26-27: Pre-calculate
collections.Join(int32(v1.StatusDepositPeriod), ctx.HeaderInfo().Time)outside theWalkfunction call to avoid repeated calculations.- 69-69: Ignoring errors from hooks can lead to unnoticed failures in proposal processing. Consider logging the error at a minimum or handling it more explicitly.
- 86-86: The log message for a proposal not meeting the minimum deposit should include the current block time for context.
- 131-133: The logic for deciding whether to refund or burn deposits based on proposal outcomes is complex. Consider encapsulating this logic in a dedicated method to improve clarity.
- 236-242: Similar to previous comments on hooks, ignoring errors from
AfterProposalVotingPeriodEndedmight mask important failures. Reconsider this approach.- 248-254: Include the final tally results in the log message for a tallied proposal for complete transparency.
- 296-296: When setting the proposal status to failed in
failUnsupportedProposal, ensure that all necessary cleanup actions are taken, including logging and event emission.
x/gov/abci.go
Outdated
| if err != nil { | ||
| // if the proposal has an encoding error, this means it cannot be processed by x/gov | ||
| // this could be due to some types missing their registration | ||
| // instead of returning an error (i.e, halting the chain), we fail the proposal | ||
| if errors.Is(err, collections.ErrEncoding) { | ||
| proposal.Id = key.K2() | ||
| proposal.Id = proposalID |
There was a problem hiding this comment.
When failing a proposal due to encoding errors, ensure that the failure reason is logged for better traceability.
x/gov/abci.go
Outdated
| if err := failUnsupportedProposal(logger, ctx, keeper, proposal, err.Error(), false); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil { | ||
| if err = keeper.Proposals.Remove(ctx, proposalID); err != nil { |
There was a problem hiding this comment.
Ensure that the removal of a proposal from the store is logged for auditability and debugging purposes.
x/gov/abci.go
Outdated
| @@ -46,18 +46,18 @@ | |||
| return false, err | |||
| } | |||
|
|
|||
| if err = keeper.DeleteProposal(ctx, proposal.Id); err != nil { | |||
| if err = keeper.Proposals.Remove(ctx, proposalID); err != nil { | |||
There was a problem hiding this comment.
Logging the removal of a proposal here as well would enhance traceability.
x/gov/abci.go
Outdated
| err = keeper.DeleteAndBurnDeposits(ctx, proposalID) // burn the deposit if proposal got removed without getting 100% of the proposal | ||
| } else { | ||
| err = keeper.DeleteAndBurnDeposits(ctx, proposal.Id) // burn the deposit if proposal got removed without getting 100% of the proposal | ||
| err = keeper.RefundAndDeleteDeposits(ctx, proposalID) // refund deposit if proposal got removed without getting 100% of the proposal |
There was a problem hiding this comment.
Consider consolidating the logic for deciding whether to burn or refund deposits into a single method to reduce duplication and improve maintainability.
|
Tests are failing because the reference/dereference of the index fails when it isn't able to decode the proposal. |
julienrbrt
changed the title
julienrbrt
force-pushed
the
julien/gov-collections-refactor
branch
from
28dd5b0
to
c75295a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 17
Configuration used: .coderabbit.yml
Files selected for processing (19)
- tests/integration/gov/abci_test.go (2 hunks)
- tests/integration/gov/keeper/grpc_query_test.go (1 hunks)
- tests/integration/gov/keeper/tally_test.go (15 hunks)
- tests/sims/gov/operations_test.go (2 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/abci.go (2 hunks)
- x/gov/genesis.go (1 hunks)
- x/gov/keeper/deposit.go (1 hunks)
- x/gov/keeper/grpc_query_test.go (16 hunks)
- x/gov/keeper/keeper.go (2 hunks)
- x/gov/keeper/migrations.go (1 hunks)
- x/gov/keeper/proposal.go (4 hunks)
- x/gov/keeper/proposal_test.go (3 hunks)
- x/gov/keeper/vote.go (1 hunks)
- x/gov/keeper/vote_test.go (2 hunks)
- x/gov/migrations/v6/store.go (2 hunks)
- x/gov/migrations/v6/store_test.go (1 hunks)
- x/gov/types/keys.go (1 hunks)
- x/gov/types/v1/proposal.go (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- x/gov/abci.go
- x/gov/genesis.go
- x/gov/keeper/deposit.go
- x/gov/keeper/proposal.go
- x/gov/types/keys.go
- x/gov/types/v1/proposal.go
Additional comments: 32
tests/integration/gov/keeper/grpc_query_test.go (1)
- 41-41: Correct usage of
f.govKeeper.Proposals.Setwith proper error handling.x/gov/migrations/v6/store.go (3)
- 12-12: Introduced a new variable
votingPeriodProposalKeyPrefixfor proposal indexing.- 19-21: Added new parameters and cleanup logic in
MigrateStore.- 40-46: Clearing old proposal stores correctly.
x/gov/keeper/vote.go (1)
- 18-25: Retrieving proposal before checking its voting period status.
x/gov/CHANGELOG.md (1)
- 62-62: Documented API breaking changes correctly.
x/gov/keeper/vote_test.go (2)
- 35-35: Correctly setting proposals using
govKeeper.Proposals.Set.- 126-126: Properly setting proposals in the context of multiple choice proposals.
x/gov/keeper/keeper.go (1)
- 71-76: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [62-62]
Removed
VotingPeriodProposalsfield as part of simplification.x/gov/keeper/proposal_test.go (2)
- 42-42: Using
govKeeper.Proposals.Setfor setting proposals in tests.- 249-249: Correctly setting proposal status before testing
CancelProposal.tests/integration/gov/keeper/tally_test.go (15)
- 29-29: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 56-56: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 82-82: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 111-111: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 138-138: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 166-166: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 195-195: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 224-224: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 254-254: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 291-291: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 330-330: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 372-372: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 417-417: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 463-463: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 499-499: Ensure error handling is consistent and robust for the call to
f.govKeeper.Proposals.Set.- err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) - assert.NilError(t, err) + require.NoError(t, err, "setting proposal should not error")tests/sims/gov/operations_test.go (2)
- 221-221: Ensure error handling is consistent and robust for the call to
suite.GovKeeper.Proposals.Set.- err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal) - require.NoError(t, err) + require.NoError(t, err, "setting proposal should not error")
- 264-264: Ensure error handling is consistent and robust for the call to
suite.GovKeeper.Proposals.Set.- err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal) - require.NoError(t, err) + require.NoError(t, err, "setting proposal should not error")tests/integration/gov/abci_test.go (2)
- 37-37: Ensure that the error handling for
Proposals.Setis consistent with the expected behavior for setting proposals in different states. The test assumes successful setting without considering potential failure scenarios that could affect the governance module's integrity.- 65-65: Similar to the previous comment, verify that
Proposals.Setis correctly handling errors and that the governance module's integrity is maintained when setting active proposals. Consider adding tests to cover failure scenarios.x/gov/keeper/grpc_query_test.go (2)
- 1506-1506: The line number annotation seems misplaced or unnecessary as there's no corresponding code change or context provided for this specific line.
- 1667-1667: The line number annotation seems misplaced or unnecessary as there's no corresponding code change or context provided for this specific line.
| }, 1, startTime, startTime, "", "Unsupported proposal", "Unsupported proposal", addrs[0], v1.ProposalType_PROPOSAL_TYPE_STANDARD) | ||
| require.NoError(t, err) | ||
|
|
||
| err = suite.GovKeeper.SetProposal(ctx, proposal) | ||
| err = suite.GovKeeper.Proposals.Set(ctx, proposal.Id, proposal) | ||
| require.NoError(t, err) | ||
|
|
||
| // manually set proposal in inactive proposal queue |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-237]
Consider adding integration tests to specifically cover the new IndexedMap functionality for proposals and the removal of specific queues for active and inactive proposals. This will ensure that the refactor does not introduce regressions and that the new approach integrates well with the existing governance module functionality.
| @@ -41,5 +41,5 @@ func (m Migrator) Migrate4to5(ctx context.Context) error { | |||
|
|
|||
| // Migrate4to5 migrates from version 5 to 6. | |||
| func (m Migrator) Migrate5to6(ctx context.Context) error { | |||
| return v6.MigrateStore(ctx, m.keeper.Params, m.keeper.Proposals) | |||
| return v6.MigrateStore(ctx, m.keeper.storeService, m.keeper.Params, m.keeper.Proposals) | |||
There was a problem hiding this comment.
Missing error handling for v6.MigrateStore call.
- return v6.MigrateStore(ctx, m.keeper.storeService, m.keeper.Params, m.keeper.Proposals)
+ err := v6.MigrateStore(ctx, m.keeper.storeService, m.keeper.Params, m.keeper.Proposals)
+ if err != nil {
+ return err
+ }
+ return nilCommittable 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.
| return v6.MigrateStore(ctx, m.keeper.storeService, m.keeper.Params, m.keeper.Proposals) | |
| err := v6.MigrateStore(ctx, m.keeper.storeService, m.keeper.Params, m.keeper.Proposals) | |
| if err != nil { | |
| return err | |
| } | |
| return nil |
| @@ -37,7 +37,7 @@ func TestMigrateStore(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| // Run migrations. | |||
| err = v6.MigrateStore(ctx, paramsCollection, proposalCollection) | |||
| err = v6.MigrateStore(ctx, storeService, paramsCollection, proposalCollection) | |||
There was a problem hiding this comment.
Missing error handling for v6.MigrateStore call.
- err = v6.MigrateStore(ctx, storeService, paramsCollection, proposalCollection)
+ err := v6.MigrateStore(ctx, storeService, paramsCollection, proposalCollection)
+ require.NoError(t, err)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.
| err = v6.MigrateStore(ctx, storeService, paramsCollection, proposalCollection) | |
| err := v6.MigrateStore(ctx, storeService, paramsCollection, proposalCollection) | |
| require.NoError(t, err) |
| @@ -297,7 +297,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryProposals() { | |||
| "request with filter of deposit address", | |||
| func() { | |||
| testProposals[1].Status = v1.StatusVotingPeriod | |||
| err := suite.govKeeper.SetProposal(ctx, *testProposals[1]) | |||
| err := suite.govKeeper.Proposals.Set(ctx, testProposals[1].Id, *testProposals[1]) | |||
There was a problem hiding this comment.
Missing error handling after changing the proposal status. Ensure that any state changes are followed by error checks.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(ctx, testProposals[1].Id, *testProposals[1]) | |
| err := suite.govKeeper.Proposals.Set(ctx, testProposals[1].Id, *testProposals[1]) | |
| suite.Require().NoError(err) |
| @@ -520,7 +520,7 @@ | |||
| "valid request", | |||
| func() { | |||
| proposal.Status = v1.StatusVotingPeriod | |||
| err := suite.govKeeper.SetProposal(ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal status. Always check for errors after modifying state to ensure consistency.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
| @@ -1742,7 +1742,7 @@ | |||
| VotingEndTime: &propTime, | |||
| Metadata: "proposal metadata", | |||
| } | |||
| err := suite.govKeeper.SetProposal(ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal. Error checks are crucial after state changes to ensure the integrity of the data.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
| @@ -1768,7 +1768,7 @@ | |||
| VotingEndTime: &propTime, | |||
| Metadata: "proposal metadata", | |||
| } | |||
| err := suite.govKeeper.SetProposal(ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal. Always verify the success of operations that alter the state to maintain consistency.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
| @@ -1836,7 +1836,7 @@ | |||
| Metadata: "proposal metadata", | |||
| ProposalType: v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |||
| } | |||
| err := suite.govKeeper.SetProposal(suite.ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal. It's necessary to check for errors after state modifications to catch any issues.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
| @@ -1864,7 +1864,7 @@ | |||
| Metadata: "proposal metadata", | |||
| ProposalType: v1.ProposalType_PROPOSAL_TYPE_MULTIPLE_CHOICE, | |||
| } | |||
| err := suite.govKeeper.SetProposal(suite.ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal. Error checks are essential after modifying state to ensure data integrity.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
| @@ -1895,7 +1895,7 @@ | |||
| Metadata: "proposal metadata", | |||
| ProposalType: v1.ProposalType_PROPOSAL_TYPE_MULTIPLE_CHOICE, | |||
| } | |||
| err := suite.govKeeper.SetProposal(suite.ctx, proposal) | |||
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |||
There was a problem hiding this comment.
Missing error handling after setting the proposal and vote options. Ensure all state changes are validated to maintain data consistency.
+ suite.Require().NoError(err)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.
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| err := suite.govKeeper.Proposals.Set(suite.ctx, proposal.Id, proposal) | |
| suite.Require().NoError(err) |
Description
Closes: #16270
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.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...
Summary by CodeRabbit
New Features
Refactor
IndexedMapfor improved performance.Chores