feat!: Rewire dependencies on the staking module #2056

p-offtermatt · 2024-07-17T13:28:31Z

Description

Closes: #1913

See https://informalsystems.notion.site/Staking-dependencies-in-Cosmos-SDK-v0-50-5-and-ibc-go-v8-3-2-669889523e1840f68996f69946bf388e?pvs=25 for some context

This wires some modules (mint, governance) to use the provider keeper as a replacement of the staking keeper.
This is because these modules need to be wired to take into account the consensus validators, instead of the whole bonded validator set.

This PR:

Rewires the govKeeper and mintKeeper to reference the ProviderKeeper instead of the StakingKeeper
Implements the interfaces these modules expect from the StakingKeeper

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

Reviewers Checklist

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

I have...

confirmed the correct type prefix in the PR title
confirmed ! the type prefix if the change is state-machine breaking
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

Summary by CodeRabbit

New Features
- Introduced scenarios for managing inactive validators, including governance considerations, rewards distribution, slashing/jailing, and minting.
Refactor
- Reorganized initialization of key components (MintKeeper, GovKeeper, ProviderKeeper) for improved dependency management and setup sequence.
Tests
- Added new test cases for validating IterateBondedValidatorsByPower, TotalBondedTokens, and BondedRatio.
- Enhanced mock testing capabilities for StakingKeeper to include additional methods and context handling.
Documentation
- Updated ADR documentation to include testing scenarios for inactive validators in different contexts.

…ngKeeper

coderabbitai · 2024-07-17T15:22:06Z

Walkthrough

The update restructures the initialization of keepers in the application, focusing on MintKeeper, GovKeeper, and ProviderKeeper. It introduces new methods for MockStakingKeeper in testing, and adds functionality for managing validators outside the active consensus set. The new SetGovKeeper method and additional testing scenarios in the ADR enhance the system's flexibility and robustness.

Changes

File Path	Change Summary
app/provider/app.go	Reorganized keeper initializations, adding dependencies between `MintKeeper`, `GovKeeper`, and `ProviderKeeper`.
testutil/keeper/mocks.go	Added new methods and mock recorders to `MockStakingKeeper` for extensive testing.
x/ccv/provider/keeper/keeper.go	Added `SetGovKeeper` method to `Keeper` struct.
x/ccv/provider/keeper/staking_keeper_interface.go	Introduced functions facilitating interaction with staking keeper for validator management.
x/ccv/provider/keeper/staking_keeper_interface_test.go	Added tests for staking keeper interface, ensuring correct iteration and calculation functionality.
docs/docs/adrs/adr-017-allowing-inactive-validators.md	Added testing scenarios for inactive validators, addressing governance, rewards, and penalties.

Sequence Diagram(s)

N/A

Assessment against linked issues

Objective	Addressed	Explanation
Allow validators that are not participating in consensus on the provider to validate consumer chains (#1913)	✅
Validate the expected powers of validators against the actual powers returned by the iteration process (#1913)	✅

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 2008840 and 1a1a1a9.

Files selected for processing (2)

testutil/keeper/mocks.go (5 hunks)
x/ccv/types/expected_keepers.go (2 hunks)

Files skipped from review as they are similar to previous changes (2)

testutil/keeper/mocks.go
x/ccv/types/expected_keepers.go

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

app/provider/app.go

x/ccv/provider/keeper/staking_keeper_interface.go

insumity · 2024-07-18T07:53:16Z

x/ccv/provider/keeper/staking_keeper_interface.go

+// tokens of the first MaxProviderConsensusValidators validators.
+// This is used to implement the interface of the staking keeper to interface with
+// modules that need to reference the consensus-active validators.
+func (k Keeper) TotalBondedTokens(ctx context.Context) (math.Int, error) {


Looking at TotalBondedTokens it's not fully clear to me that bonded tokens does not include the tokens of non-consensus-active validators.

I changed the docstring, does that help?
Or are you talking about the code?

Here are some references for how the original TotalBondedTokens function behaves:

It just gets the tokens in the bonded pool here https://github.com/cosmos/cosmos-sdk/blob/a32186608aab0bd436049377ddb34f90006fcbf7/x/staking/keeper/pool.go#L80

the bonded pool contains the tokens from bonded validators. e.g. see here that tokens are sent to the bonded pool when someone delegates to an active validator https://github.com/cosmos/cosmos-sdk/blob/a32186608aab0bd436049377ddb34f90006fcbf7/x/staking/keeper/delegation.go#L913,
and how tokens are transferred between bonded/not-bonded pool on valstatechanges https://github.com/cosmos/cosmos-sdk/blob/a32186608aab0bd436049377ddb34f90006fcbf7/x/staking/keeper/val_state_change.go#L251

x/ccv/provider/keeper/staking_keeper_interface_test.go

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

docs/docs/adrs/adr-017-allowing-inactive-validators.md (1)
78-115: Testing Scenarios section looks good with a minor grammatical correction.

The testing scenarios are detailed and cover all necessary aspects.

However, consider the following grammatical correction:
- Afterwards, we create another proposal and vote on it with active validators, too.
+ Afterward, we create another proposal and vote on it with active validators, too.
Tools

LanguageTool

[locale-violation] ~92-~92: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...lidators. The proposal should not pass. Afterwards, we create another proposal and vote on...

(AFTERWARDS_US)

Markdownlint

81-81: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

82-82: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

83-83: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

80-80: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)

81-81: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)

81-81: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

* Change wiring for mint and gov to use ProviderKeeper instead of StakingKeeper * Add test for IterateBondedValidatorsByPower * Rewire GovKeeper * Fix docstrings * Test other modified functions * Start writing some testing scenarios * Add TotalBondedTokens to expected staking keeper interface

…er chains (#2079) * refactor!: Refactor the validator set storage and add provider consensus validator storage (#1990) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add key to key test * Add unit test for LastTotalProviderConsensusPower * Address comments * feat!: Introduce the MaxProviderConsensusValidators param (#1992) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add function to get MaxProviderConsensusValidators param * refactor!: Refactor the validator set storage and add provider consensus validator storage (#1990) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add key to key test * Add unit test for LastTotalProviderConsensusPower * Address comments * feat!: Wire the provider module to return ValidatorUpdates, instead of the staking module (#1993) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add no_valupdates_staking module * Add function to get MaxProviderConsensusValidators param * Start returning validators in EndBlock * Fix tests * Revert cosmetic change * Revert cosmetic changes * Revert formatting * Add genutil replacer module * Revert formatting * Revert formatting in tests/integration * Revert minor formatting * Fix type * Change wrapped staking to conform to EndBlocker interface * Fix typo * Revert "Fix typo" This reverts commit 62dfd1e. * Add e2e test for inactive vals * Start fixing e2e test * Revert formatting changes * Remove more formatting * Revert extra formatting * Re-wire provider/app.go to use wrapped modules * Remove consumer rewards check * Add inactive provider vals testcase to nightly * Adjust comment * Address comments * Fix nightly test name * feat: Initialize the max validators parameter for existing consumers (#2012) * Add initialization for validator cap * Remove migration test * Fix inconsistent naming * test: enable the simulator for the provider module (#2005) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add no_valupdates_staking module * Add function to get MaxProviderConsensusValidators param * Start returning validators in EndBlock * Fix tests * Revert cosmetic change * Revert cosmetic changes * Revert formatting * Add genutil replacer module * Revert formatting * Revert formatting in tests/integration * Revert minor formatting * Fix type * Change wrapped staking to conform to EndBlocker interface * Fix typo * Revert "Fix typo" This reverts commit 62dfd1e. * Add e2e test for inactive vals * Start fixing e2e test * Revert formatting changes * Remove more formatting * Revert extra formatting * Re-wire provider/app.go to use wrapped modules * Remove consumer rewards check * Add simulator test * Add randomly generated parameters for provider in sim * Add invariant * Add simulation to Makefile and github workflow * Use simcli instead of just passing true * feat!: Let consumer chains choose a minimum stake and validator rank (#2035) * Add minimum stake key * Add MinValidatorRank prefix * Add keeper and tests for new parameters * Utilize MinStake and MaxRank parameters in computing next validators * Mention MinStake and MaxRank in adr * Add test for FulfillsMinStake * Handle multiple validators with same power * Add min stake and max rank to docs * Add minStake and maxRank to proposals * Check for untyped equality * Handle minStake and maxRank in Msgs * Add integration test for min stake and max rank * Rename test and testfile * Update docs/docs/adrs/adr-017-allowing-inactive-validators.md * Add changelog entries for maxrank and minstake * Address comments * Clarify which feature is disabled by setting maxrank * Test validator powers cap and validator set cap into int param testing function * feat!: Rewire dependencies on the staking module (#2056) * Change wiring for mint and gov to use ProviderKeeper instead of StakingKeeper * Add test for IterateBondedValidatorsByPower * Rewire GovKeeper * Fix docstrings * Test other modified functions * Start writing some testing scenarios * Add TotalBondedTokens to expected staking keeper interface * feat: Calculate Top N based on active validators only (#2070) * Add test for inactive vals with top N * Add test case to predefined tests * Fix bonded/active validator distinction * Fix relay test to set max provider consensus vals correctly * feat!: Add a parameter that determines whether consumer chains allow inactive validators to validate them (#2066) * Introduce new AllowInactiveValidators param for consumer chains * Add AllowInactiveValidators param to tests * Set MaxProviderConsensusValidators in tests * Add migration to initialize inactive vals * Add changelog entries for inactive vals param * Add property-based test for inactive vals * Add docs for inactive vals param * Set AllowInactiveVals parameter in e2e test * test: Add e2e tests for inactive vals (#2064) * Start adding e2e test for governance * Debug gov with inactive vals test * Outline for test scenarios where they are tested * Add MaxRank steps * Add e2e tests for min stake and max rank * Revert formatting change * Refactor stepsOptIn * Use adjusted config for e2e tests * Write for more scenarios where they are tested * Add test for mint * Add docstrings for e2e steps * Delete hanging changelog entry * Address comments * Address more comments * Add migration for param * Fix allow inactive validators param test * Fix tests * Add LastProviderConsensusValidatorKey to fully defined keys * Fix key for validator set updates * Add info about genesis/endblock ordering * Add unit test for ProviderValidatorUpdates * Add example to proto definition of max_rank * Remove max rank * Remove references to max rank * Start adding an extension to the simulator * Make invariant fail early when param is 0 * Reorder InitGenesis to put Crisis last * Remove canary * Swap equals for not equals * Disable invariant check when max validators != max provider consensus validators * Make the simulator use a random seed * Remove TODO * Remove decoder * Run go mod tidy * Add migration in UPGRADING.md * Fix tests * Put random seed generation into golang code * Rename simulation jobs * Update UPGRADING.md Co-authored-by: Marius Poke <marius.poke@posteo.de> * Update UPGRADING.md Co-authored-by: Marius Poke <marius.poke@posteo.de> * Update x/ccv/provider/keeper/genesis.go Co-authored-by: Marius Poke <marius.poke@posteo.de> * Mention simulation tests in testing.md * Address some comments * Remake protos * Panic when LastActiveBondedValidators fails * Address some comments * Address comments * Reorder tests * Adjust stake_multiplier to stakeMultiplier * Address comments * Add error logging * Fix reference to bank blocked addrs in simulation * Change hasToValidate to only take into account active validators * Update docs/docs/adrs/adr-017-allowing-inactive-validators.md Co-authored-by: insumity <karolos@informal.systems> * Clarify: Slash happens on provider Co-authored-by: insumity <karolos@informal.systems> --------- Co-authored-by: Marius Poke <marius.poke@posteo.de> Co-authored-by: insumity <karolos@informal.systems>

github-actions bot added C:Testing Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:x/types Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler C:ADR Assigned automatically by the PR labeler labels Jul 17, 2024

Change wiring for mint and gov to use ProviderKeeper instead of Staki…

6e79bfc

…ngKeeper

p-offtermatt force-pushed the ph/rewire-staking-dependencies branch from a0cedbe to 6e79bfc Compare July 17, 2024 13:31

p-offtermatt changed the title ~~feat: Rewire dependencies on the staking module~~ feat!: Rewire dependencies on the staking module Jul 17, 2024

Add test for IterateBondedValidatorsByPower

900a54f

p-offtermatt marked this pull request as ready for review July 17, 2024 15:21

p-offtermatt requested a review from a team as a code owner July 17, 2024 15:21

insumity reviewed Jul 18, 2024

View reviewed changes

app/provider/app.go Outdated Show resolved Hide resolved

insumity reviewed Jul 18, 2024

View reviewed changes

x/ccv/provider/keeper/staking_keeper_interface.go Show resolved Hide resolved

Rewire GovKeeper

a7a8c96

insumity reviewed Jul 18, 2024

View reviewed changes

x/ccv/provider/keeper/staking_keeper_interface_test.go Outdated Show resolved Hide resolved

insumity reviewed Jul 18, 2024

View reviewed changes

x/ccv/provider/keeper/staking_keeper_interface_test.go Show resolved Hide resolved

p-offtermatt added 2 commits July 18, 2024 10:21

Fix docstrings

d6bcfe0

Test other modified functions

e77d510

insumity approved these changes Jul 18, 2024

View reviewed changes

Start writing some testing scenarios

2008840

coderabbitai bot reviewed Jul 18, 2024

View reviewed changes

Add TotalBondedTokens to expected staking keeper interface

1a1a1a9

p-offtermatt merged commit 4caa577 into feat/inactive-vals-v50 Jul 18, 2024
11 of 12 checks passed

p-offtermatt deleted the ph/rewire-staking-dependencies branch July 18, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Rewire dependencies on the staking module #2056

feat!: Rewire dependencies on the staking module #2056

p-offtermatt commented Jul 17, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 17, 2024 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

insumity Jul 18, 2024

p-offtermatt Jul 18, 2024

p-offtermatt Jul 18, 2024

coderabbitai bot left a comment

feat!: Rewire dependencies on the staking module #2056

feat!: Rewire dependencies on the staking module #2056

Conversation

p-offtermatt commented Jul 17, 2024 • edited by coderabbitai bot Loading

Description

Author Checklist

Reviewers Checklist

Summary by CodeRabbit

coderabbitai bot commented Jul 17, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

insumity Jul 18, 2024

Choose a reason for hiding this comment

p-offtermatt Jul 18, 2024

Choose a reason for hiding this comment

p-offtermatt Jul 18, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

p-offtermatt commented Jul 17, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 17, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)