refactor(x/staking): migrate to use env #19414
Conversation
|
Warning Rate Limit Exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 52 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. WalkthroughThe changes focus on updating various components to leverage the new Changes
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -271,7 +274,7 @@ func TestSlashWithUnbondingDelegation(t *testing.T) { | |||
| assert.NilError(t, f.stakingKeeper.SetUnbondingDelegation(f.sdkCtx, ubd)) | |||
|
|
|||
| // slash validator for the first time | |||
| f.sdkCtx = f.sdkCtx.WithBlockHeight(12) | |||
| f.sdkCtx = f.sdkCtx.WithHeaderInfo(header.Info{Height: 12}) | |||
There was a problem hiding this comment.
Consider adding assertions after each slashing operation to verify the expected state changes, not just at the end of the test. This will help ensure each step of the test behaves as expected and makes the test more robust.
Repeated slashing operations in the test are intended to simulate different scenarios. However, ensure there's clear documentation or comments explaining the rationale behind each operation and what specific condition or behavior it's testing. This will improve the maintainability and readability of the test.
The final assertions in the test verify the expected outcomes of the slashing operations, including the validator's power and the balances of the bonded and not bonded pools. These assertions are crucial for ensuring the slashing logic is correctly implemented. However, consider adding more granular assertions throughout the test to verify intermediate states after each significant operation.
The test function TestSlashWithUnbondingDelegation effectively covers the scenario it's designed to test. However, consider adding tests for edge cases, such as slashing with a fraction of zero or more than one, and cases where the unbonding delegation or validator does not exist. Expanding test coverage in this way can help ensure the robustness of the slashing logic under various conditions.
x/staking/keeper/genesis.go
Outdated
| @@ -28,7 +28,7 @@ func (k Keeper) InitGenesis(ctx context.Context, data *types.GenesisState) (res | |||
| // first TM block is at height 1, so state updates applied from | |||
| // genesis.json are in block 0. | |||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |||
| sdkCtx = sdkCtx.WithBlockHeight(1 - sdk.ValidatorUpdateDelay) | |||
| sdkCtx = sdkCtx.WithBlockHeight(1 - sdk.ValidatorUpdateDelay) // TODO: replace this | |||
There was a problem hiding this comment.
Address the TODO comment regarding the replacement of block height setup in the context to ensure it aligns with the module's initialization requirements and does not lead to future technical debt.
Would you like me to open a GitHub issue to track the resolution of this TODO?
tac0turtle
force-pushed
the
marko/staking_env
branch
from
6e13b19
to
66f2128
Compare
| sdk.NewAttribute(types.AttributeKeyCompletionTime, completionTime.Format(time.RFC3339)), | ||
| ), | ||
| }) | ||
| if err := k.environment.EventService.EventManager(ctx).EmitKV( |
There was a problem hiding this comment.
Would this be only be an EmitKV or could it also potentially also be an Emit too?
There was a problem hiding this comment.
it could be emit but since we dont have the types defined i went with EmitKV
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- runtime/module.go (1 hunks)
- tests/integration/slashing/abci_test.go (5 hunks)
- x/staking/CHANGELOG.md (1 hunks)
- x/staking/keeper/genesis.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- runtime/module.go
- x/staking/keeper/genesis.go
Additional comments: 5
tests/integration/slashing/abci_test.go (4)
- 10-10: The import of
coreheaderis correctly added to support the newctx.WithHeaderInfomethod usage.- 87-87: The update from
ctx.BlockHeight()toctx.HeaderInfo().Heightaligns with the migration to usecoreheader.Info. Ensure all instances where block height is needed are updated accordingly.- 98-98: Using
ctx.WithHeaderInfo(coreheader.Info{Height: height})for setting the block height is correct and improves the test's clarity by explicitly setting the header information.- 108-108: The combination of
ctx.WithHeaderInfoandctx.WithCometInfoin a single call is a good practice for setting multiple context properties simultaneously, ensuring the test context is correctly configured for each iteration.x/staking/CHANGELOG.md (1)
- 84-84: The changelog entry accurately reflects the significant change introduced by the PR, which is the modification of
NewStakingKeeperto accept an environment variable. This entry is clear, follows the changelog format, and is placed correctly under "API Breaking Changes".
julienrbrt
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (3)
- tests/integration/slashing/abci_test.go (5 hunks)
- tests/integration/slashing/keeper/keeper_test.go (11 hunks)
- x/staking/keeper/validator_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/integration/slashing/abci_test.go
- tests/integration/slashing/keeper/keeper_test.go
Additional comments: 2
x/staking/keeper/validator_test.go (2)
- 438-438: Using
ctx.HeaderInfo().Height + 10forendHeightcalculation is a good practice as it relies on the current block height from the context, ensuring the test reflects more realistic scenarios.- 465-465: Creating a new context with updated header information using
ctx.WithHeaderInfo(header.Info{Height: endHeight, Time: endTime})is correctly implemented for testing unbonding at a specific block height and time.
tac0turtle
force-pushed
the
marko/staking_env
branch
from
bed0b68
to
f5a7068
Compare
Description
Migrate staking module to use env variable
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
CHANGELOG.mdto reflect the new initialization approach forNewStakingKeeper.