Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/distribution)!: remove PreviousProposer and return early if there are no fees to distribute #20735

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Jun 20, 2024

Description

PreviousProposer was being used back when we had proposer rewards (correct me if I'm wrong). We were storing it on every block, which is unnecessary as this value is not being used anywhere else.

Also we are returning early if there are no collected fees in AllocateTokens. Now with epoched minting this is possible, as there will be blocks with no minting (#19761).

These 2 things allow x/distribution to not make any changes to the store when there is nothing to be distributed.


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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Enhanced community pool funds management with the introduction of FeePool.DecimalPool.
  • Refactor

    • Removed PreviousProposer functionality from the distribution module, including related state management and initialization processes.
  • Bug Fixes

    • Improved AllocateTokens function to handle scenarios with no fees collected more efficiently.
  • Tests

    • Updated and streamlined test cases to reflect the removal of PreviousProposer and related checks.

@facundomedica facundomedica requested a review from a team as a code owner June 20, 2024 09:26
@facundomedica facundomedica changed the title refactor(x/distribution): remove PreviousProposer and return early if there are no fees to distribute refactor(x/distribution)!: remove PreviousProposer and return early if there are no fees to distribute Jun 20, 2024
Copy link
Contributor

coderabbitai bot commented Jun 20, 2024

Walkthrough

Walkthrough

The changes focus on removing the PreviousProposer from the Cosmos SDK's x/distribution module. This involves eliminating references to PreviousProposer in various components, like the state machine, migration functions, and tests. Additionally, changes are made to handle cases where the PreviousProposer was referenced, such as updating the GenesisState and altering key definitions.

Changes

File Path Change Summary
x/distribution/CHANGELOG.md Documented removal of PreviousProposer from the x/distribution module and related migrations.
x/distribution/keeper/abci.go Removed import of sdk "github.com/cosmos/cosmos-sdk/types" and refactored BeginBlocker by removing proposer recording.
x/distribution/keeper/allocation.go Added a check for empty fees in AllocateTokens.
x/distribution/keeper/genesis.go Removed logic related to PreviousProposer from InitGenesis and ExportGenesis.
x/distribution/keeper/keeper.go Removed PreviousProposer field from Keeper struct and its initialization in NewKeeper.
x/distribution/proto/cosmos/distribution/v1beta1/genesis.proto Removed previous_proposer field and added reserved field in GenesisState.
x/distribution/types/genesis.go Removed pp parameter and initialization of PreviousProposer in NewGenesisState.
x/distribution/types/keys.go Removed ProposerKey and added DeprecatedProposerKey.
x/distribution/migrations/v4/migrate.go Refactored MigrateStore to delete the last proposer directly.
x/distribution/migrations/v4/migrate_test.go Removed proposer-related imports and checks.
x/distribution/simulation/decoder.go Removed case handling for types.ProposerKey in NewDecodeStore.
x/distribution/simulation/decoder_test.go Removed consAddr1 initialization and usage in TestDecodeDistributionStore.
tests/integration/distribution/keeper/msg_server_test.go Removed references and checks related to valConsAddr in TestMsgWithdrawDelegatorReward.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the C:x/distribution distribution module related label Jun 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (5)
x/distribution/types/keys.go (1)

65-65: The renaming of ProposerKey to DeprecatedProposerKey is appropriate given the deprecation of the PreviousProposer logic. Ensure that deprecated items are clearly documented to guide developers during the transition period.

x/distribution/CHANGELOG.md (1)

Line range hint 34-34: Correct the Markdown formatting for the unordered list indentation to improve the readability of the changelog.

- * [#20588](https://github.com/cosmos/cosmos-sdk/pull/20588) `x/distribution` now takes cometService in order to get consensus related information.
- * [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method
+ * [#20588](https://github.com/cosmos/cosmos-sdk/pull/20588) `x/distribution` now takes cometService to get consensus related information.
+ * [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method

Also applies to: 37-37, 38-38, 41-41, 43-43, 45-45, 47-47, 49-49, 52-52, 54-54

api/cosmos/distribution/v1beta1/genesis.pulsar.go (3)

Line range hint 50-56: Review the use of panic in the default case to ensure it aligns with error handling best practices.

Consider replacing panic with a more controlled error handling mechanism if applicable.


Line range hint 85-91: Correct the comments to accurately describe each field rather than incorrectly labeling multiple as fee_pool.

-	// fee_pool defines the delegator withdraw infos at genesis.
-	// fee_pool defines the outstanding rewards of all validators at genesis.
-	// fee_pool defines the accumulated commissions of all validators at genesis.
+	// DelegatorWithdrawInfos defines the delegator withdraw infos at genesis.
+	// OutstandingRewards defines the outstanding rewards of all validators at genesis.
+	// ValidatorAccumulatedCommissions defines the accumulated commissions of all validators at genesis.

[!TIP]
Codebase Verification

The provided unit tests do not explicitly check the handling of empty OutstandingRewards. The TestQueryValidatorOutstandingRewards function is marked as a TODO, and TestGRPCValidatorOutstandingRewards sets up a validator with commission but does not cover the empty case.

  • x/distribution/keeper/grpc_query_test.go: TestQueryValidatorOutstandingRewards is marked as a TODO.
  • tests/integration/distribution/keeper/grpc_query_test.go: TestGRPCValidatorOutstandingRewards sets up a validator with commission but does not verify the empty list scenario.

Please add unit tests to explicitly verify the handling of both empty and non-empty OutstandingRewards.

Analysis chain

Line range hint 36-42: Ensure the list conversion and handling of empty OutstandingRewards are correctly implemented.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify correct handling of empty and non-empty `OutstandingRewards`.
# Test: Implement unit tests to check both scenarios.
echo "Unit testing required for correct list handling."

Length of output: 106



Script:

#!/bin/bash
# Search for unit tests related to `OutstandingRewards`.
rg 'OutstandingRewards' --type go --glob '*_test.go'

Length of output: 5161



Script:

#!/bin/bash
# Extract relevant test implementations to verify handling of empty and non-empty `OutstandingRewards`.
rg -A 10 'func TestQueryValidatorOutstandingRewards' x/distribution/keeper/grpc_query_test.go
rg -A 10 'func TestGRPCValidatorOutstandingRewards' tests/integration/distribution/keeper/grpc_query_test.go

Length of output: 1034

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d0e0b4c and b2ba274.

Files ignored due to path filters (1)
  • x/distribution/types/genesis.pb.go is excluded by !**/*.pb.go
Files selected for processing (9)
  • api/cosmos/distribution/v1beta1/genesis.pulsar.go (16 hunks)
  • x/distribution/CHANGELOG.md (1 hunks)
  • x/distribution/keeper/abci.go (2 hunks)
  • x/distribution/keeper/allocation.go (1 hunks)
  • x/distribution/keeper/genesis.go (3 hunks)
  • x/distribution/keeper/keeper.go (2 hunks)
  • x/distribution/proto/cosmos/distribution/v1beta1/genesis.proto (2 hunks)
  • x/distribution/types/genesis.go (2 hunks)
  • x/distribution/types/keys.go (3 hunks)
Files not summarized due to errors (1)
  • api/cosmos/distribution/v1beta1/genesis.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (4)
  • x/distribution/keeper/abci.go
  • x/distribution/keeper/genesis.go
  • x/distribution/keeper/keeper.go
  • x/distribution/proto/cosmos/distribution/v1beta1/genesis.proto
Additional context used
Path-based instructions (5)
x/distribution/types/genesis.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/distribution/types/keys.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/distribution/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/distribution/keeper/allocation.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

api/cosmos/distribution/v1beta1/genesis.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Learnings (1)
x/distribution/keeper/allocation.go (1)
User: julienrbrt
PR: cosmos/cosmos-sdk#18539
File: x/distribution/keeper/allocation.go:26-42
Timestamp: 2023-11-22T14:50:39.724Z
Learning: User clarified that the addition of fees to the DecimalPool is handled in a different part of the code when `totalPreviousPower` is zero.
LanguageTool
x/distribution/CHANGELOG.md

[style] ~35-~35: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...x/distribution now takes cometService in order to get consensus related information. * [#...

Markdownlint
x/distribution/CHANGELOG.md

37-37: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


38-38: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


41-41: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


43-43: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


45-45: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


47-47: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


49-49: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


52-52: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


54-54: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


34-34: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines

Additional comments not posted (14)
x/distribution/types/genesis.go (1)

4-4: LGTM! Simplification of the NewGenesisState function by removing the PreviousProposer parameter aligns well with the PR objectives and reduces complexity.

x/distribution/CHANGELOG.md (1)

69-69: The changelog entry accurately reflects the changes made in this PR regarding the removal of PreviousProposer. Good documentation practice!

api/cosmos/distribution/v1beta1/genesis.pulsar.go (12)

Line range hint 1-7: No notable changes detected in this hunk.


Line range hint 8-14: Correct assignment of field descriptors in GenesisState.


Line range hint 22-28: Correct checks for non-emptiness of fields in GenesisState.


Line range hint 57-63: Initialization of empty lists for specific fields appears correct. Ensure this aligns with the data handling requirements of the application.


Line range hint 92-98: Getter for OutstandingRewards appears correctly implemented.


Line range hint 99-105: Binary data for serialization appears correctly formatted. No action needed unless there are specific serialization issues reported.


Line range hint 15-21: Ensure that the condition and list handling for OutstandingRewards are consistent with expected behavior.

Verification successful

To verify the review comment, we need to check the usage of the reflect and sync packages in the api/cosmos/distribution/v1beta1/genesis.pulsar.go file. This will help us confirm if the imports are necessary and used correctly. Additionally, we should verify the consistency of the variable declarations for md_DelegatorWithdrawInfo and fd_DelegatorWithdrawInfo_delegator_address.


The results indicate that the reflect and sync packages are indeed used within the api/cosmos/distribution/v1beta1/genesis.pulsar.go file. Additionally, the variable declarations for md_DelegatorWithdrawInfo and fd_DelegatorWithdrawInfo_delegator_address are consistent with the rest of the codebase.

  • The reflect package is used for reflect.TypeOf(x{}).PkgPath().
  • The sync package is used for sync.Once.

Given this context, the imports and variable declarations in the provided snippet are correct and necessary.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the behavior when `OutstandingRewards` is empty or non-empty.
# Test: Manually test or write unit tests to ensure correct handling.
echo "Manual testing or unit test verification required."

Length of output: 110



Script:

#!/bin/bash
# Search for the usage of `reflect` and `sync` in the specified file
rg 'reflect|sync' api/cosmos/distribution/v1beta1/genesis.pulsar.go

# Verify the variable declarations for `md_DelegatorWithdrawInfo` and `fd_DelegatorWithdrawInfo_delegator_address`
rg 'md_DelegatorWithdrawInfo|fd_DelegatorWithdrawInfo_delegator_address' api/cosmos/distribution/v1beta1/genesis.pulsar.go

Length of output: 31616


Line range hint 43-49: Check for correct type handling in list value conversions to ensure data integrity.


Line range hint 106-112: Review of binary data for protobuf serialization. Ensure alignment with protobuf specifications.


Line range hint 71-77: Ensure marshaling of DelegatorWithdrawInfos is handled correctly and that errors are properly managed.


Line range hint 64-70: Review the performance implications of this loop, especially if OutstandingRewards can become large.


Line range hint 78-84: Correct handling of wire type errors during unmarshaling. Ensure comprehensive testing is in place to cover these scenarios.

Verification successful

Given the repeated lack of search results, it would be prudent to manually inspect the file api/cosmos/distribution/v1beta1/genesis.pulsar.go for unmarshaling and error handling logic related to wire types. This will help ensure a thorough verification process.


The manual inspection of the file api/cosmos/distribution/v1beta1/genesis.pulsar.go has revealed the presence of unmarshaling logic, including error handling for incorrect wire types. This confirms that the code appropriately handles wire type mismatches during unmarshaling.

  • The unmarshaling logic is located in the unmarshal function, which includes checks for wire type mismatches and other related errors.

Based on this evidence, the original review comment's concern about wire type error handling is addressed and verified.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify error handling for wire type mismatches.
# Test: Review or create tests that intentionally pass incorrect wire types.
echo "Testing for wire type error handling required."

Length of output: 102






Script:

#!/bin/bash
# Display the contents of the file to manually inspect for unmarshaling and error handling logic.
cat api/cosmos/distribution/v1beta1/genesis.pulsar.go

Length of output: 256142

Comment on lines +24 to +27
// return early if no fees to distribute
if feesCollectedInt.Empty() {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The early return when no fees are collected is an efficient addition, preventing unnecessary computations and state changes. Consider adding a comment explaining the reasoning behind this early return for future maintainability.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b2ba274 and e4435d0.

Files selected for processing (4)
  • x/distribution/migrations/v4/migrate.go (1 hunks)
  • x/distribution/migrations/v4/migrate_test.go (3 hunks)
  • x/distribution/simulation/decoder.go (1 hunks)
  • x/distribution/simulation/decoder_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
  • x/distribution/migrations/v4/migrate_test.go
Additional context used
Path-based instructions (3)
x/distribution/migrations/v4/migrate.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/distribution/simulation/decoder_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

x/distribution/simulation/decoder.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (3)
x/distribution/migrations/v4/migrate.go (1)

18-21: Well-implemented migration logic.

The direct deletion of OldProposerKey aligns with the PR's objective to clean up deprecated state. The implementation is simple and effective.

x/distribution/simulation/decoder_test.go (1)

22-23: Tests updated correctly.

The removal of consAddr1 is consistent with the broader changes of removing PreviousProposer. Ensure that all related functionality is covered by existing or new tests.

x/distribution/simulation/decoder.go (1)

Line range hint 1-39: Decoder functionality remains intact.

No changes are necessary here as the decoding logic should not be affected by the removal of PreviousProposer. The function continues to handle various types of distribution data correctly.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e4435d0 and a13d686.

Files selected for processing (1)
  • tests/integration/distribution/keeper/msg_server_test.go (3 hunks)
Additional context used
Path-based instructions (1)
tests/integration/distribution/keeper/msg_server_test.go (3)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (6)
tests/integration/distribution/keeper/msg_server_test.go (6)

Line range hint 1-441: Review of TestMsgWithdrawDelegatorReward

This test function appears to be well-structured and covers a variety of cases including error scenarios and valid operations. However, given the removal of PreviousProposer, ensure that any logic or assertions related to this are also removed if they are no longer relevant.


Line range hint 442-609: Review of TestMsgSetWithdrawAddress

The test scenarios here are comprehensive, covering both enabled and disabled states of withdraw address functionality, and also handle various error conditions. Ensure all references and logic are up-to-date with the latest changes in the distribution module, especially regarding parameter settings and error messages.


Line range hint 610-758: Review of TestMsgWithdrawValidatorCommission

The test cases here adequately cover the functionality of withdrawing validator commissions, including error handling for non-existent validators and validators without commissions. Given the changes in the PR, ensure that all logic and assertions are consistent with the current state of the distribution module.


Line range hint 759-887: Review of TestMsgFundCommunityPool

The test scenarios here are comprehensive, covering various error conditions and a valid operation scenario. Ensure that the logic and assertions are up-to-date with the latest changes in the distribution module, particularly in terms of error messages and the handling of coin amounts.


Line range hint 888-1066: Review of TestMsgUpdateParams

The test cases here adequately cover the functionality of updating distribution parameters, including error handling for invalid authorities and deprecated fields. Given the changes in the PR, ensure that all logic and assertions are consistent with the current state of the distribution module.


Line range hint 1067-1275: Review of TestMsgCommunityPoolSpend

The test scenarios here are comprehensive, covering various error conditions and a valid operation scenario. Ensure that the logic and assertions are up-to-date with the latest changes in the distribution module, particularly in terms of error messages and the handling of recipient addresses.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@facundomedica facundomedica added this pull request to the merge queue Jul 4, 2024
Merged via the queue into main with commit 01312df Jul 4, 2024
70 checks passed
@facundomedica facundomedica deleted the facu/refactors branch July 4, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Simulations C:x/distribution distribution module related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants