trigger distribution on slash event #261
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
x/feedistribution/keeper/allocation.go (1)
154-154: Improve clarity of error messageThe error message
"Skipping distribution for due to fail to generate avsAddr from chainID"is grammatically incorrect and may be confusing. Please rephrase it for clarity.Apply this diff to improve the error message:
- logger.Error("Skipping distribution for due to fail to generate avsAddr from chainID", "chainID", ctx.ChainID()) + logger.Error("Skipping distribution due to failure in generating avsAddress from chainID", "chainID", ctx.ChainID())x/feedistribution/keeper/impl_epoch_hooks.go (2)
33-33: Remove commented-out code or provide justificationThe line
// previousTotalPower := wrapper.keeper.StakingKeeper.GetLastTotalPower(ctx)is commented out. If this code is no longer needed, consider removing it to keep the codebase clean. If you plan to use it in the future, please add a comment explaining why it is retained.
38-41: Fix typos and improve comment clarityThere are typos in the comments that may reduce readability. For example, "minited tokens" should be "minted tokens". Please correct these typos and consider rephrasing the comments for better clarity.
Apply this diff to fix typos and improve the comments:
- // at the end of an epoch we distribute all minted tokens for one epoch and left fees - // here we have the temporary approach that we allocate minited tokens for next epoch then we can correctly - // distribute those tokens based on potential slash event. And that should be ok since the amount of minited - // token for each epoch is the same, so we just missed the very first epoch's minting. + // At the end of an epoch, we distribute all minted tokens and any remaining fees for that epoch. + // Currently, we have a temporary approach where we allocate minted tokens for the next epoch, + // allowing us to correctly distribute tokens based on potential slashing events. + // This is acceptable since the minted token amount per epoch is the same, so we only miss the very first epoch's minting.x/feedistribution/keeper/impl_operator_hooks.go (1)
19-25: Consider implementing or documenting empty hook methodsThe methods
AfterOperatorKeySet,AfterOperatorKeyReplaced, andAfterOperatorKeyRemovalInitiatedare currently empty. If they are intentionally left unimplemented, consider adding comments to explain why or if implementation is planned for the future.x/feedistribution/keeper/keeper.go (1)
45-46: LGTM! Consider updating module documentation.The new parameters are properly added to the constructor. Since these represent significant new dependencies, ensure that the module's documentation (especially godocs) is updated to reflect these changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/app.go(1 hunks)x/feedistribution/keeper/allocation.go(2 hunks)x/feedistribution/keeper/impl_epoch_hooks.go(1 hunks)x/feedistribution/keeper/impl_operator_hooks.go(1 hunks)x/feedistribution/keeper/keeper.go(3 hunks)x/feedistribution/types/expected_keepers.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/feedistribution/types/expected_keepers.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: x/avs/types/expected_keepers.go:47-48
Timestamp: 2024-11-12T10:03:10.791Z
Learning: Adding new methods to the `OperatorKeeper` interface in `x/avs/types/expected_keepers.go` is acceptable, even if there are other unmodifiable `OperatorKeeper` interfaces elsewhere, because interfaces can be extended without causing conflicts.
🔇 Additional comments (6)
x/feedistribution/keeper/allocation.go (1)
14-21:
Verify all calls to AllocateTokens are updated to match the new signature
The AllocateTokens function now accepts a boolean parameter isSlash instead of totalPreviousPower int64. Please ensure that all invocations of this function throughout the codebase are updated to match the new signature to prevent compile-time errors.
Run the following script to locate all calls to AllocateTokens that may need updating:
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax. Let's try with ripgrep to find all occurrences of AllocateTokens calls and its function definition to compare the signatures.
All calls to AllocateTokens are already updated with the new signature
The verification shows that all calls to AllocateTokens are correctly using the new boolean parameter:
- In
impl_operator_hooks.go:AllocateTokens(ctx, true) - In
impl_epoch_hooks.go:AllocateTokens(ctx, false)
These match the new function signature defined in allocation.go: AllocateTokens(ctx sdk.Context, isSlash bool) error
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of AllocateTokens calls that need updating.
ast-grep --pattern 'AllocateTokens($_)'
Length of output: 41
Script:
#!/bin/bash
# Search for AllocateTokens function calls with ripgrep for better coverage
rg "AllocateTokens\(" -A 2
# Also search for the function definition to understand the old signature
rg "func.*AllocateTokens"
Length of output: 1492
app/app.go (1)
Line range hint 780-784: Verify the order of hooks in OperatorKeeper.SetHooks
Ensure that the order of hooks set in OperatorKeeper is correct, as it may affect the application's behavior. The sequence can be crucial if multiple hooks interact with the same events. Please confirm that app.DistrKeeper.OperatorHooks() should be set before app.StakingKeeper.OperatorHooks().
🧰 Tools
🪛 GitHub Check: build
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
🪛 GitHub Check: test-unit-cover
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
🪛 GitHub Check: Run golangci-lint
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
x/feedistribution/keeper/impl_epoch_hooks.go (1)
42-42: Ensure all calls to AllocateTokens are updated
The AllocateTokens function is now called with a boolean parameter false to indicate that it is not triggered by a slashing event. Please verify that this change aligns with the updated function signature and logic.
x/feedistribution/keeper/impl_operator_hooks.go (1)
43-54: Implementation of AfterSlash hook
The AfterSlash method logs the event and calls AllocateTokens(ctx, true) to allocate tokens after a slashing event. The error handling is appropriate. Ensure that this implementation aligns with the intended business logic for handling slashing events.
x/feedistribution/types/expected_keepers.go (1)
59-65: Addition of MintKeeper and AVSKeeper interfaces
The new interfaces MintKeeper and AVSKeeper are appropriately defined to extend the expected keepers' functionality. This aligns with the practice of extending interfaces without causing conflicts, as per previous learnings.
x/feedistribution/keeper/keeper.go (1)
Line range hint 27-68: Verify slash event handling implementation.
While these changes add the necessary dependencies for handling slash events, the actual implementation of slash event handling is not visible in this file. Let's verify the implementation exists in related files.
✅ Verification successful
Slash event handling is implemented through fee allocation mechanism
The verification reveals that slash event handling is properly implemented in the fee distribution module through the following mechanisms:
- The
AllocateTokensfunction inallocation.goexplicitly handles slash events through itsisSlashparameter - The module integrates with the epoch system for regular distribution, while also supporting slash-triggered distributions
- The implementation correctly manages both collected fees and minted coins during slash events
The changes in the keeper's dependencies are necessary to support this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for slash event handling implementation
# Search for slash event handling
echo "Checking for slash event handling..."
rg -l "slash|Slash" --type go
# Search for specific hook implementations
echo "Checking for hook implementations..."
ast-grep --pattern 'func $FUNC($CTX, $ADDR) {
$$$
}'
Length of output: 11531
Script:
#!/bin/bash
# Let's check the implementation of slash event handling in feedistribution module
# Check for slash-related hooks in feedistribution
echo "Checking feedistribution hooks..."
rg -A 10 "AfterValidatorSlashed|BeforeValidatorSlashed" x/feedistribution/
# Check for slash event handling in operator hooks implementation
echo "Checking operator hooks implementation..."
rg -A 10 "AfterValidatorSlashed" x/feedistribution/keeper/impl_operator_hooks.go
# Look for any slash-related functions in allocation
echo "Checking allocation handling..."
rg -A 10 "slash|Slash" x/feedistribution/keeper/allocation.go
Length of output: 1883
Script:
#!/bin/bash
# Let's check the epoch hooks implementation since slash events seem to be handled through epochs
# Check epoch hooks implementation
echo "Checking epoch hooks implementation..."
rg -A 10 "AfterEpochEnd|BeforeEpochStart" x/feedistribution/keeper/impl_epoch_hooks.go
# Check for any slash-related state management
echo "Checking for slash state management..."
fd -e go . x/feedistribution/keeper/ -x rg -l "slash|Slash"
# Look for any tests related to slash handling
echo "Checking for slash-related tests..."
fd -e go . x/feedistribution/ -x rg -l "TestSlash|test.*Slash"
Length of output: 1845
| } else { | ||
| passedDuration := sdkmath.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime))) | ||
| epochDuration := sdkmath.LegacyNewDec(int64(epochInfo.Duration)) | ||
| mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using mutable operations on decimals
Using MulMut modifies the decimal value in place, which can lead to unintended side effects. It's safer to use immutable methods like Mul, which returns a new decimal. Please replace MulMut with Mul.
Apply this diff to use immutable decimal operations:
- mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))
+ mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) | |
| mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) |
| passedDuration := sdkmath.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime))) | ||
| epochDuration := sdkmath.LegacyNewDec(int64(epochInfo.Duration)) | ||
| mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) | ||
| feesCollected.Sub(sdk.DecCoins{mintedCoinDec}) | ||
| } |
There was a problem hiding this comment.
Correct the calculation of passedDuration and epochDuration
Currently, passedDuration and epochDuration are calculated by converting a time.Duration to int64, which represents the duration in nanoseconds. This may lead to incorrect calculations, as the minting logic likely expects durations in seconds. Please update the calculation to use the correct time units.
Apply this diff to correct the duration calculations:
- passedDuration := sdkmath.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime)))
- epochDuration := sdkmath.LegacyNewDec(int64(epochInfo.Duration))
+ passedDuration := sdkmath.LegacyNewDec(ctx.BlockTime().Sub(epochInfo.StartTime).Seconds())
+ epochDuration := sdkmath.LegacyNewDec(epochInfo.Duration.Seconds())📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| passedDuration := sdkmath.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime))) | |
| epochDuration := sdkmath.LegacyNewDec(int64(epochInfo.Duration)) | |
| mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) | |
| feesCollected.Sub(sdk.DecCoins{mintedCoinDec}) | |
| } | |
| passedDuration := sdkmath.LegacyNewDec(ctx.BlockTime().Sub(epochInfo.StartTime).Seconds()) | |
| epochDuration := sdkmath.LegacyNewDec(epochInfo.Duration.Seconds()) | |
| mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration))) | |
| feesCollected.Sub(sdk.DecCoins{mintedCoinDec}) | |
| } |
Handle potential division by zero in epoch duration calculation
If epochDuration is zero, passedDuration.QuoTruncate(epochDuration) will result in a division by zero error, causing a panic. Please ensure that epochDuration is not zero before performing the division, and handle the potential error appropriately.
Apply this diff to add a check for zero epochDuration:
+ if epochDuration.IsZero() {
+ logger.Error("Epoch duration is zero, cannot calculate minted coin portion")
+ feesCollected = feesCollected.Sub(sdk.DecCoins{mintedCoinDec})
+ } else {
mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))
feesCollected = feesCollected.Sub(sdk.DecCoins{mintedCoinDec})
+ }Committable suggestion skipped: line range outside the PR's diff.
| mintKeeper types.MintKeeper | ||
| avsKeeper types.AVSKeeper |
There was a problem hiding this comment.
💡 Codebase verification
Interface mismatch detected in AVSKeeper implementations
The verification reveals inconsistencies in AVSKeeper interface definitions across modules:
x/feedistribution/types: Only definesIsAVSByChainID(ctx, chainID) (bool, string)x/appchain/coordinator/types: Has different return type forIsAVSByChainID-(bool, common.Address)x/dogfood/types: Has additional methods and matches feedistribution's return typex/operator/types: Has completely different method set
This needs to be addressed to ensure type safety and consistent behavior across modules.
🔗 Analysis chain
LGTM! Verify interface implementations.
The new keeper dependencies are properly declared. To ensure correctness, let's verify the interface implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface implementations for MintKeeper and AVSKeeper
# Check for MintKeeper interface definition and implementations
echo "Checking MintKeeper interface and implementations..."
rg -A 5 "type MintKeeper interface"
# Check for AVSKeeper interface definition and implementations
echo "Checking AVSKeeper interface and implementations..."
rg -A 5 "type AVSKeeper interface"
Length of output: 2747
| mintKeeper: mintKeeper, | ||
| avsKeeper: avsKeeper, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for new keepers.
Consider adding nil checks for mintKeeper and avsKeeper similar to the accountKeeper validation above. This follows Cosmos SDK best practices and prevents potential runtime panics.
func NewKeeper(...) Keeper {
// ensure distribution module account is set
if addr := accountKeeper.GetModuleAddress(types.ModuleName); addr == nil {
panic(fmt.Sprintf("%s module account has not been set", types.ModuleName))
}
+ // ensure mint keeper is set
+ if mintKeeper == nil {
+ panic("mint keeper is nil")
+ }
+
+ // ensure avs keeper is set
+ if avsKeeper == nil {
+ panic("avs keeper is nil")
+ }
if _, err := sdk.AccAddressFromBech32(authority); err != nil {
panic(fmt.Sprintf("invalid authority address: %s", authority))
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mintKeeper: mintKeeper, | |
| avsKeeper: avsKeeper, | |
| mintKeeper: mintKeeper, | |
| avsKeeper: avsKeeper, | |
| // ensure mint keeper is set | |
| if mintKeeper == nil { | |
| panic("mint keeper is nil") | |
| } | |
| // ensure avs keeper is set | |
| if avsKeeper == nil { | |
| panic("avs keeper is nil") | |
| } |
leonz789
force-pushed
the
develop-distribution-stakers
branch
from
21bab9d to
ec92007
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/feedistribution/keeper/allocation.go (1)
154-154: Fix grammatical error in log messageThe error message contains a grammatical error: "Skipping distribution for due to fail to generate avsAddr from chainID". Please revise it for clarity.
Consider changing it to:
- logger.Error("Skipping distribution for due to fail to generate avsAddr from chainID", "chainID", ctx.ChainID()) + logger.Error("Skipping distribution due to failure to generate avsAddr from chainID", "chainID", ctx.ChainID())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/app.go(1 hunks)x/feedistribution/keeper/allocation.go(2 hunks)x/feedistribution/keeper/impl_epoch_hooks.go(1 hunks)x/feedistribution/keeper/impl_operator_hooks.go(1 hunks)x/feedistribution/keeper/keeper.go(3 hunks)x/feedistribution/types/expected_keepers.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/feedistribution/types/expected_keepers.go
- x/feedistribution/keeper/impl_epoch_hooks.go
- x/feedistribution/keeper/keeper.go
- x/feedistribution/keeper/impl_operator_hooks.go
🔇 Additional comments (4)
app/app.go (1)
780-782: Verify compatibility of added operator hook
The addition of app.DistrKeeper.OperatorHooks() to OperatorKeeper.SetHooks() introduces a new interaction between the Operator module and the Distribution module. Please verify that OperatorKeeper is designed to handle hooks from DistrKeeper and that this integration does not introduce unintended side effects or cyclic dependencies.
🧰 Tools
🪛 GitHub Check: test-unit-cover
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
🪛 GitHub Check: build
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
🪛 GitHub Check: Run golangci-lint
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
[failure] 781-781:
too many arguments in call to (&app.OperatorKeeper).SetHooks
x/feedistribution/keeper/allocation.go (3)
44-45:
[Duplicate Comment] Correct the calculation units of passedDuration and epochDuration
Currently, passedDuration and epochDuration are calculated by converting a time.Duration to int64, which represents nanoseconds. This may lead to incorrect calculations due to unit mismatch. Consider using the .Seconds() method to obtain the duration in seconds for accurate calculations.
Apply this diff to correct the duration calculations:
- passedDuration := sdkmath.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime)))
- epochDuration := sdkmath.LegacyNewDec(int64(epochInfo.Duration))
+ passedDuration := sdkmath.LegacyNewDec(ctx.BlockTime().Sub(epochInfo.StartTime).Seconds())
+ epochDuration := sdkmath.LegacyNewDec(epochInfo.Duration.Seconds())46-46: 🛠️ Refactor suggestion
[Duplicate Comment] Avoid mutable operations on decimals
Using MulMut modifies the decimal value in place, which can lead to unintended side effects. It's safer to use immutable methods like Mul, which returns a new decimal value. Please replace MulMut with Mul.
Apply this diff to use immutable decimal operations:
- mintedCoinDec.Amount.MulMut(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))
+ mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))45-46:
[Duplicate Comment] Handle potential division by zero
If epochDuration is zero, the division passedDuration.QuoTruncate(epochDuration) will result in a division by zero error, causing a panic. Please add a check to ensure epochDuration is not zero before performing the division.
Apply this diff to add a check for zero epochDuration:
+ if epochDuration.IsZero() {
+ logger.Error("Epoch duration is zero, cannot calculate minted coin portion")
+ feesCollected = feesCollected.Sub(sdk.DecCoins{mintedCoinDec})
+ } else {
mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(sdkmath.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))
+ }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
testutil/keeper/feedistribute.go (2)
62-63: Consider adding test setup for distribution triggers.While the keeper initialization is correct, consider adding test setup for:
- Slash event handling
- Epoch-end distribution
- Asset calculation based on dogfood
This will ensure the new functionality is properly tested.
Example test setup:
// Setup slash event test slashEvent := types.NewSlashEvent(...) ctx = ctx.WithEventManager(sdk.NewEventManager()) k.HandleSlashEvent(ctx, slashEvent) // Setup epoch-end test epochInfo := epochstypes.NewGenesisEpochInfo(...) k.OnEpochEnd(ctx, epochInfo)
Line range hint
51-70: Consider testing keeper interactions.The new distribution trigger functionality involves interaction between multiple keepers (mint, AVS, epochs). Consider adding integration tests that verify:
- The flow of events between keepers
- The order of operations during distribution
- State consistency across keeper boundaries
This will help ensure the robustness of the distribution trigger implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
app/app.go(2 hunks)proto/exocore/delegation/v1/query.proto(1 hunks)testutil/keeper/feedistribute.go(4 hunks)x/feedistribution/testutil/expected_account_mock.go(1 hunks)x/feedistribution/testutil/expected_avs_mock.go(1 hunks)x/feedistribution/testutil/expected_bank_mock.go(1 hunks)x/feedistribution/testutil/expected_epochs_mock.go(1 hunks)x/feedistribution/testutil/expected_mint_mock.go(1 hunks)x/feedistribution/testutil/mock_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- x/feedistribution/testutil/mock_test.go
- proto/exocore/delegation/v1/query.proto
- x/feedistribution/testutil/expected_epochs_mock.go
- x/feedistribution/testutil/expected_mint_mock.go
- x/feedistribution/testutil/expected_avs_mock.go
- x/feedistribution/testutil/expected_account_mock.go
- x/feedistribution/testutil/expected_bank_mock.go
🔇 Additional comments (4)
testutil/keeper/feedistribute.go (2)
11-11: LGTM! Good practice using local test utilities.
The switch to local test utilities provides better control over test implementations specific to the project's needs.
47-48: Verify mock implementations for new keepers.
The mock keepers are correctly initialized, but let's verify they implement all required methods for testing the distribution trigger functionality.
✅ Verification successful
Let me gather more information about the mock implementations.
Mock implementations are complete and correctly implemented
The mock keepers implement all required methods from their respective interfaces:
- MockMintKeeper implements
GetParams(ctx sdk.Context) minttypes.Params - MockAVSKeeper implements
IsAVSByChainID(ctx sdk.Context, chainID string) (bool, string)
These match exactly with the interface definitions in x/feedistribution/types/expected_keepers.go, making the mocks suitable for testing the distribution trigger functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that mock implementations include all required methods
# Check the interface definitions and mock implementations
rg -A 5 "type (Mint|AVS)Keeper interface"
ast-grep --pattern 'type Mock$_Keeper struct {
$$$
}'
Length of output: 2367
Script:
#!/bin/bash
# Check mock implementations in the test utilities
rg -A 10 "func NewMock(Mint|AVS)Keeper"
rg "func \(m \*Mock(Mint|AVS)Keeper\)"
Length of output: 2293
app/app.go (2)
715-716: LGTM: DistrKeeper initialization with required dependencies
The initialization of DistrKeeper with ExomintKeeper and AVSManagerKeeper is correctly implemented to support fee distribution based on validator set changes.
782-785: Verify hook execution order for operator events
The setup of operator hooks with DistrKeeper and StakingKeeper looks correct. However, let's verify the hook execution order to ensure proper handling of slash events.
✅ Verification successful
Let me try a different approach to find the hook implementations.
Let me search for the operator hooks interface and its implementations.
Based on the findings, I can now generate the final response.
Hook execution order is correctly implemented
The MultiOperatorHooks implementation in x/operator/types/hooks.go ensures sequential execution of hooks in the order they are provided. The current setup with DistrKeeper followed by StakingKeeper hooks is appropriate as:
- DistrKeeper's hooks (implemented in feedistribution) handle fee distribution updates
- Both implementations are lightweight and don't interfere with each other's functionality
- The order matches the dependency flow where distribution updates should precede staking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hook execution order for operator events
# Test: Search for hook implementations to confirm execution order
# Check DistrKeeper operator hooks implementation
echo "Checking DistrKeeper operator hooks..."
ast-grep --pattern 'func ($x *Keeper) OperatorHooks() $type {
$$$
}'
# Check StakingKeeper operator hooks implementation
echo "Checking StakingKeeper operator hooks..."
ast-grep --pattern 'func ($x *Keeper) OperatorHooks() $type {
$$$
}'
Length of output: 323
Script:
#!/bin/bash
# Search for operator hooks in distribution and staking modules
echo "Searching for operator hooks in distribution module..."
rg -A 5 "OperatorHooks" x/distribution/
echo "Searching for operator hooks in staking module..."
rg -A 5 "OperatorHooks" x/staking/
# Search for MultiOperatorHooks implementation
echo "Searching for MultiOperatorHooks implementation..."
rg -A 10 "type MultiOperatorHooks"
Length of output: 1116
Script:
#!/bin/bash
# Search for OperatorHooks interface definition
echo "Searching for OperatorHooks interface..."
rg -A 10 "type OperatorHooks interface"
# Search for implementations in keeper files
echo "Searching for keeper implementations..."
fd -e go keeper.go -x rg -A 5 "OperatorHooks()" {} \;
# Search for specific hook methods to understand the execution flow
echo "Searching for hook method implementations..."
rg -A 5 "AfterOperatorKeySet|AfterOperatorBonded|AfterOperatorUnbonded"
Length of output: 5084
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/e2e/oracle/create_price.go(1 hunks)testutil/network/genesis_data.go(3 hunks)testutil/network/util.go(2 hunks)x/feedistribution/keeper/allocation.go(5 hunks)
🔇 Additional comments (5)
testutil/network/genesis_data.go (1)
88-88: Verify the impact of reduced jail duration.
The OracleMissJailDuration has been reduced from 30s to 10s. This shorter duration might not provide sufficient time for validators to recover from temporary network issues.
x/feedistribution/keeper/allocation.go (2)
49-50:
Avoid mutable decimal operations.
Using MulMut modifies the decimal value in place, which can lead to unintended side effects.
Apply this diff to use immutable operations:
-mintedCoinDec.Amount.MulMut(math.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))
+mintedCoinDec.Amount = mintedCoinDec.Amount.Mul(math.LegacyOneDec().Sub(passedDuration.QuoTruncate(epochDuration)))Likely invalid or redundant comment.
47-49:
Fix potential precision loss in duration calculations.
The conversion of time.Duration to int64 could lead to precision loss and incorrect calculations:
- Duration is converted to nanoseconds which might overflow int64
- The decimal conversion might not preserve the intended precision
Apply this diff to fix the precision issues:
-passedDuration := math.LegacyNewDec(int64(ctx.BlockTime().Sub(epochInfo.StartTime)))
-epochDuration := math.LegacyNewDec(int64(epochInfo.Duration))
+passedDuration := math.LegacyNewDec(ctx.BlockTime().Sub(epochInfo.StartTime).Seconds())
+epochDuration := math.LegacyNewDec(epochInfo.Duration.Seconds())Likely invalid or redundant comment.
testutil/network/util.go (2)
24-24: LGTM!
The import statement follows the project's conventions and is properly grouped with other module imports.
250-252: Consider adding error handling and verify DefaultGenStateExoMint
While the initialization follows the pattern used for other modules, consider:
- Adding error handling similar to other module initializations
- Verifying that
DefaultGenStateExoMintcontains appropriate configuration for fee distribution triggers (slash events and epoch end)
Consider adding error handling similar to other module initializations:
// set exomint genesis state
-cfg.GenesisState[exominttypes.ModuleName] = cfg.Codec.MustMarshalJSON(&DefaultGenStateExoMint)
+exomintGenState, err := NewGenStateExoMint()
+if err != nil {
+ return err
+}
+cfg.GenesisState[exominttypes.ModuleName] = cfg.Codec.MustMarshalJSON(&exomintGenState)| DefaultGenStateOracle.Params.Slashing.OracleMissJailDuration = 30 * time.Second | ||
| DefaultGenStateOracle.Params.Slashing.OracleMissJailDuration = 10 * time.Second | ||
| // set mint duration to minute | ||
| DefaultGenStateExoMint.Params.EpochIdentifier = epochtypes.MinuteEpochID |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found in epoch identifier configuration
The verification reveals an inconsistency between the test utility's epoch identifier and the actual module configuration. The DefaultGenStateExoMint in test utilities uses MinuteEpochID, while the exomint module's default configuration in x/exomint/types/params.go uses DefaultEpochIdentifier = epochstypes.DayEpochID. This mismatch could lead to unexpected behavior in tests.
testutil/network/genesis_data.go: UsesMinuteEpochIDx/exomint/types/params.go: UsesDayEpochIDas default
🔗 Analysis chain
Verify epoch identifier consistency.
Ensure that the MinuteEpochID is consistently used across the codebase for minting operations.
Let me gather more specific information about epoch identifiers in the minting context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check epoch identifier usage
# Search for MinuteEpochID references
rg "MinuteEpochID"
# Search for other epoch identifiers in minting context
ast-grep --pattern 'EpochIdentifier = $_'
Length of output: 1137
Script:
#!/bin/bash
# Search for epoch identifier configurations in minting and fee distribution contexts
rg -A 2 "EpochIdentifier.*=.*" x/feedistribution/
rg -A 2 "EpochIdentifier.*=.*" x/exomint/
# Check for any other epoch-related configurations in minting
rg -i "epoch" x/exomint/
Length of output: 14763
| chainIDWithoutRevision := avstypes.ChainIDWithoutRevision(ctx.ChainID()) | ||
| isAVS, avsAddress := k.avsKeeper.IsAVSByChainID(ctx, chainIDWithoutRevision) | ||
| if !isAVS { | ||
| logger.Error("Skipping distribution for due to fail to generate avsAddr from chainID", "chainID", ctx.ChainID()) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in AVS status check.
The AVS status check returns early with just an error log. This could lead to silent failures in fee distribution.
Apply this diff to improve error handling:
if !isAVS {
- logger.Error("Skipping distribution for due to fail to generate avsAddr from chainID", "chainID", ctx.ChainID())
- return
+ err := fmt.Errorf("failed to generate AVS address from chainID %s", ctx.ChainID())
+ logger.Error(err.Error())
+ return err
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -323,15 +323,14 @@ func (s *E2ETestSuite) testSlashing() { | |||
| s.Require().NoError(err) | |||
| s.Require().True(resOperator.Jailed) | |||
| // wait for validator3 to pass jail duration | |||
| time.Sleep(35 * time.Second) | |||
| time.Sleep(12 * time.Second) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential test flakiness.
The sleep duration has been reduced to 12s to match the new jail duration. However, this tight timing might make tests flaky, especially in CI environments with varying performance characteristics.
Consider these alternatives:
- Use a multiplier of the jail duration instead of hardcoded seconds
- Add retry logic with appropriate timeout
- Wait for a specific chain state instead of using sleep
Example implementation:
-time.Sleep(12 * time.Second)
+jailDuration := s.network.QueryOracle().Params(context.Background(), &oracletypes.QueryParamsRequest{}).Params.Slashing.OracleMissJailDuration
+time.Sleep(jailDuration + 2*time.Second) // Add buffer for chain processingCommittable suggestion skipped: line range outside the PR's diff.
Description
Trigger fee(including minted coins) distribution on validator set change, which caused by:
Notable Changes
Closes #XXX
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor