feat(sequencer, rollapp): liveness slashing and jailing #1009
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumx/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- go.mod (5 hunks)
- proto/dymensionxyz/dymension/rollapp/rollapp.proto (1 hunks)
- x/rollapp/keeper/invariants.go (3 hunks)
- x/rollapp/keeper/liveness_test.go (1 hunks)
- x/rollapp/types/genesis_test.go (6 hunks)
- x/rollapp/types/params.go (4 hunks)
- x/rollapp/types/rollapp.go (1 hunks)
- x/sequencer/keeper/msg_server_create_sequencer_test.go (1 hunks)
- x/sequencer/types/params.go (7 hunks)
Files skipped from review due to trivial changes (1)
- x/sequencer/keeper/msg_server_create_sequencer_test.go
Files skipped from review as they are similar to previous changes (6)
- go.mod
- proto/dymensionxyz/dymension/rollapp/rollapp.proto
- x/rollapp/keeper/liveness_test.go
- x/rollapp/types/params.go
- x/rollapp/types/rollapp.go
- x/sequencer/types/params.go
Additional comments not posted (9)
x/rollapp/types/genesis_test.go (6)
26-26: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithRegFeemethod enhances clarity and maintainability by encapsulating the parameter initialization logic.
71-71: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithRegFeemethod enhances clarity and maintainability by encapsulating the parameter initialization logic.
82-87: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithDisputePeriodInBlocksandWithRegFeemethods enhances clarity and maintainability by encapsulating the parameter initialization logic.
93-98: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithLivenessSlashBlocksmethod enhances clarity and maintainability by encapsulating the parameter initialization logic.
104-109: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithLivenessSlashIntervalmethod enhances clarity and maintainability by encapsulating the parameter initialization logic.
Line range hint
115-120: LGTM! Improved parameter initialization.The use of
DefaultParams()andWithLivenessJailBlocksmethod enhances clarity and maintainability by encapsulating the parameter initialization logic.x/rollapp/keeper/invariants.go (3)
18-18: LGTM! Registered new invariant.The
RegisterInvariantsfunction is updated to register theLivenessEventInvariant, ensuring it is included in the invariant checks.
44-47: LGTM! Integrated new invariant.The
AllInvariantsfunction is updated to include theLivenessEventInvariantcheck, ensuring it is part of the overall invariant validation process.
265-312: LGTM! Added new invariant function.The
LivenessEventInvariantfunction validates the integrity of liveness events associated with rollapps, ensuring accurate tracking and robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumx/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- go.mod (5 hunks)
- proto/dymensionxyz/dymension/rollapp/rollapp.proto (1 hunks)
- x/rollapp/keeper/invariants.go (3 hunks)
- x/rollapp/keeper/liveness_test.go (1 hunks)
- x/rollapp/keeper/rollapp_suite_test.go (2 hunks)
- x/rollapp/types/rollapp.go (1 hunks)
- x/sequencer/keeper/msg_server_create_sequencer_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- go.mod
- proto/dymensionxyz/dymension/rollapp/rollapp.proto
- x/rollapp/keeper/liveness_test.go
- x/rollapp/types/rollapp.go
- x/sequencer/keeper/msg_server_create_sequencer_test.go
Additional comments not posted (6)
x/rollapp/keeper/rollapp_suite_test.go (3)
47-47: LGTM! Improved parameter management.The change to retrieve default parameters and modify specific fields enhances clarity and maintainability.
63-65: LGTM! Useful helper method.The
keepermethod is a valuable addition that provides cleaner access to theRollappKeeperinstance.
67-70: LGTM! Enhances block simulation.The
nextBlockmethod improves the test suite's ability to simulate block progression, which is crucial for scenarios dependent on block height.x/rollapp/keeper/invariants.go (3)
19-19: LGTM! New invariant registered.The addition of
LivenessEventInvarianttoRegisterInvariantsensures that the new invariant will be checked during validation.
45-48: LGTM! Integrated new invariant.The inclusion of
LivenessEventInvariantinAllInvariantsenhances the robustness of the validation process.
261-308: LGTM! Comprehensive liveness event check.The
LivenessEventInvariantfunction is a well-structured addition that ensures the integrity of liveness events associated with rollapps, improving the logical flow and control mechanisms within the invariants.
|
@mtsitrin not sure why I can't reply to this one
There is a comment and docstring already? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/rollapp/types/liveness.go (1 hunks)
- x/rollapp/types/params.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/rollapp/types/liveness.go
- x/rollapp/types/params.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- x/rollapp/keeper/keeper.go (4 hunks)
- x/rollapp/keeper/liveness.go (1 hunks)
- x/rollapp/types/liveness.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/rollapp/keeper/keeper.go
- x/rollapp/keeper/liveness.go
- x/rollapp/types/liveness.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/rollapp/keeper/keeper.go (1)
35-35: EnsureNewKeeperCalls IncludesequencerKeeperParameterThe
NewKeeperfunction inx/rollapp/keeper/keeper.gois not consistently called with thesequencerKeeperparameter. Specifically, intestutil/keeper/rollapp.go, the call toNewKeeperdoes not include this parameter. Please review and update the test setup to includesequencerKeeperwhere necessary.
- File:
testutil/keeper/rollapp.go
- Line:
k := keeper.NewKeeper(cdc, storeKey, paramsSubspace, nil, nil, nil)Analysis chain
Update:
NewKeeperfunction now includessequencerKeeper.The
NewKeeperfunction has been updated to includesequencerKeeper, ensuring proper initialization ofKeeperinstances with this new dependency. Verify that all calls toNewKeeperare updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `NewKeeper` include the `sequencerKeeper` parameter. # Test: Search for `NewKeeper` function calls. Expect: Inclusion of `sequencerKeeper` parameter. rg --type go -A 5 $'NewKeeper'Length of output: 14145
Script:
#!/bin/bash # Description: Verify that all calls to `NewKeeper` in `x/rollapp/keeper/keeper.go` include the `sequencerKeeper` parameter. # Test: Search for `NewKeeper` function calls in `x/rollapp/keeper/keeper.go`. Expect: Inclusion of `sequencerKeeper` parameter. rg --type go -A 5 'NewKeeper' x/rollapp/keeper/keeper.goLength of output: 228
Script:
#!/bin/bash # Description: Verify that all calls to `NewKeeper` in the codebase include the `sequencerKeeper` parameter. # Test: Search for `NewKeeper` function calls across the codebase. Expect: Inclusion of `sequencerKeeper` parameter. rg --type go -A 5 'NewKeeper'Length of output: 14145
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
x/rollapp/types/params.pb.gois excluded by!**/*.pb.gox/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.go
Files selected for processing (15)
- app/keepers/keepers.go (2 hunks)
- go.mod (3 hunks)
- proto/dymensionxyz/dymension/rollapp/params.proto (1 hunks)
- proto/dymensionxyz/dymension/rollapp/rollapp.proto (1 hunks)
- testutil/keeper/rollapp.go (1 hunks)
- x/rollapp/keeper/keeper.go (4 hunks)
- x/rollapp/keeper/liveness_test.go (1 hunks)
- x/rollapp/keeper/params.go (2 hunks)
- x/rollapp/keeper/rollapp_suite_test.go (2 hunks)
- x/rollapp/types/expected_keepers.go (1 hunks)
- x/rollapp/types/genesis_test.go (6 hunks)
- x/rollapp/types/params.go (3 hunks)
- x/rollapp/types/rollapp.go (1 hunks)
- x/sequencer/keeper/hook_listener.go (1 hunks)
- x/sequencer/keeper/msg_server_create_sequencer_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/sequencer/keeper/msg_server_create_sequencer_test.go
Files skipped from review as they are similar to previous changes (12)
- app/keepers/keepers.go
- go.mod
- proto/dymensionxyz/dymension/rollapp/params.proto
- proto/dymensionxyz/dymension/rollapp/rollapp.proto
- testutil/keeper/rollapp.go
- x/rollapp/keeper/liveness_test.go
- x/rollapp/keeper/params.go
- x/rollapp/keeper/rollapp_suite_test.go
- x/rollapp/types/expected_keepers.go
- x/rollapp/types/genesis_test.go
- x/rollapp/types/params.go
- x/rollapp/types/rollapp.go
Additional comments not posted (3)
x/sequencer/keeper/hook_listener.go (1)
48-49: Enhancement in fraud handling: VerifySlashAndJailFraudimplementation.The replacement of
SlashingwithSlashAndJailFraudenhances the fraud handling by adding jailing to the slashing process. Ensure that the new method is correctly implemented and covers all necessary fraud scenarios.Verification successful
Fraud Handling Enhancement Verified:
SlashAndJailFraudImplementationThe
SlashAndJailFraudfunction is correctly implemented. It handles fraud scenarios by checking if the sequencer is known and not unbonded, slashing all tokens, and jailing the sequencer. The function also handles errors appropriately.
- Located in
x/sequencer/keeper/slashing.go, lines 13-38.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `SlashAndJailFraud` to ensure it handles all fraud scenarios. # Test: Search for the `SlashAndJailFraud` function definition. Expect: Proper handling of fraud scenarios. ast-grep --lang go --pattern $'func (k Keeper) SlashAndJailFraud($_, $_) error { $$$ }'Length of output: 1525
x/rollapp/keeper/keeper.go (2)
63-65: New method:SetSequencerKeeperadded for flexibility.The
SetSequencerKeepermethod provides flexibility for testing and runtime adjustments, enhancing the modularity of theKeeper. Ensure that this method is utilized where necessary.
24-24: Enhancement: AddedsequencerKeepertoKeeperstruct.The addition of the
sequencerKeeperfield enhances theKeeper's capabilities to manage sequencing operations. Ensure its usage is consistent and correctly integrated within the codebase.Verification successful
Integration of
sequencerKeeperis consistent and correct.The
sequencerKeeperfield is properly integrated within theKeeperstruct. It is initialized in theNewKeeperfunction, used inliveness.gofor managing sequencing operations, and has a setter method. No inconsistencies were found in its usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `sequencerKeeper` field to ensure consistent integration. # Test: Search for the usage of `sequencerKeeper`. Expect: Consistent and correct integration. rg --type go -A 5 $'sequencerKeeper'Length of output: 1968
Description
Closes #927
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.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores