fix: Fix superfluid delegation edge case on jailed validator #8778
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (7)
x/superfluid/keeper/export_test.go (1)
89-93: LGTM. Consider adding documentation for this exported test function.The new
ForceUndelegateAndBurnOsmoTokensfunction is correctly implemented as an exported wrapper for testing purposes, consistent with the existing code style and conventions in this file.Consider adding a brief comment explaining the purpose of this function and its intended use in tests. This would improve the overall documentation of the test helpers.
CHANGELOG.md (5)
Line range hint
69-70: State Breaking: Reset Validator Signing Info Missed Blocks CounterResetting the missed blocks counter for validator signing info is a state-breaking change. This could potentially affect validator performance metrics and slashing calculations.
Carefully consider the implications of resetting this counter. Ensure that validators are informed about this change and its potential impact on their historical performance data.
Line range hint
71-72: State Breaking: Speedup CL Spread Factor CalculationsWhile this change improves performance, it's noted that it slightly changes rounding behavior in the final decimal place. This is a state-breaking change that could affect precise calculations in the system.
Thoroughly test this change to ensure that the performance improvement doesn't introduce any unintended consequences in spread factor calculations or related operations.
Line range hint
73-74: State Breaking: Fee Deduction for Smart AccountsThis change modifies when fees are deducted for smart accounts, which is a significant behavioral change in the system.
Ensure that this change is well-documented and communicated to users and developers working with smart accounts. Consider potential edge cases where this change might affect existing transactions or contracts.
Line range hint
83-84: State Breaking: Consensus Params UpdateChanging consensus parameters to match the unbonding period is a significant state-breaking change that affects the core consensus mechanism of the chain.
This change requires careful consideration and testing. Ensure that all validators are aware of this change and its implications on the consensus process.
Line range hint
102-5589: Overall Changelog Structure and ContentThe changelog provides a comprehensive history of changes across multiple versions of the software. It's well-organized, with clear delineation between major, minor, and patch releases.
The changelog follows good practices by:
- Clearly separating breaking changes, new features, and bug fixes.
- Providing links to relevant pull requests and issues.
- Offering upgrade instructions for node operators when necessary.
Consider the following improvements:
- Consistently use semantic versioning across all releases.
- Provide a brief summary of major changes at the beginning of each release section.
- Group similar changes together within each release for better readability.
As the project evolves:
- Consider maintaining a separate upgrade guide for node operators, linked from the changelog.
- Implement a changelog automation tool to ensure consistent formatting and easier maintenance.
- Regularly review and potentially deprecate old features, documenting these decisions in the changelog.
x/superfluid/keeper/stake.go (1)
513-515: Fix typos and improve clarity in commentsThere are typos and grammatical errors in the comments that could reduce readability:
- Line 514: "greater then what validator has this can be due to..." should be "greater than what the validator has; this can be due to..."
- Line 515: "we brun min(amount_tpo_burn, total_delegation_share)" should be "we burn min(amount_to_burn, total_delegation_shares)"
Apply this diff to correct the typos:
// if ValidateUnbondAmount has failed it indicates that the amount we're trying to unbond is - // greater then what validator has this can be due to different factors (e.g jail) + // greater than what the validator has; this can be due to different factors (e.g., jail) // in this case we brun min(amount_tpo_burn, total_delegation_share) - // in this case we brun min(amount_tpo_burn, total_delegation_share) + // in this case we burn min(amount_to_burn, total_delegation_shares)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- x/superfluid/keeper/export_test.go (1 hunks)
- x/superfluid/keeper/stake.go (2 hunks)
- x/superfluid/keeper/stake_test.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
x/superfluid/keeper/stake.go
[failure] 525-525:
ineffectual assignment to err (ineffassign)
🔇 Additional comments (9)
CHANGELOG.md (9)
67-68: State Compatible: ProtoRev Distribution on EpochEnabling ProtoRev distribution on epoch is a significant feature addition. This change allows for more frequent and automated distribution of ProtoRev profits.
This change improves the automation and efficiency of the ProtoRev system.
Line range hint
75-76: State Breaking: Gauge Creation RestrictionsThis change adds a restriction to gauge creation, preventing the addition of rewards that have no ProtoRev route. This is a state-breaking change that affects the behavior of the incentives system.
This change adds a useful safeguard to ensure that all gauge rewards can be properly distributed and valued.
Line range hint
77-78: State Breaking: IBC Wasm Clients UpdateAllowing IBC Wasm clients to make Stargate queries and support abort is a significant feature addition. This expands the capabilities of IBC integration with Wasm contracts.
This change enhances the functionality and flexibility of IBC Wasm clients, which is beneficial for cross-chain operations and smart contract interactions.
Line range hint
79-80: State Breaking: Unbonding Locks Processing FrequencyChanging the frequency of processing unbonding locks from every block to once per minute is a significant optimization. However, it's important to note that this is a state-breaking change.
This change should significantly reduce the computational load on the system. However, ensure that this doesn't introduce any issues with timing-sensitive operations related to unbonding.
Line range hint
81-82: State Breaking: ProtoRev Performance ImprovementThis change optimizes ProtoRev by not unmarshalling pools in the cost-estimation phase. While it improves performance, it's marked as a state-breaking change.
This optimization should lead to better performance in ProtoRev operations. Ensure that this change doesn't affect the accuracy of cost estimations.
Line range hint
85-86: State Compatible: Smart Account Parameter CachingUsing a local cache for the isSmartAccountActive parameter is a performance improvement that doesn't break state compatibility.
This change should lead to faster checks for smart account activity without affecting the overall state of the system.
Line range hint
87-88: State Breaking: Circuit Breaker Controller for Smart AccountsAdding a circuit breaker controller to smart account params is a state-breaking change that introduces new functionality for managing smart accounts.
This addition provides an important safety mechanism for smart accounts. Ensure that the implementation is thoroughly tested and that the conditions for triggering the circuit breaker are well-defined.
Line range hint
90-98: State Compatible Changes: CometBFT OptimizationsThese changes to CometBFT include various optimizations and improvements to the consensus and networking layers. While they don't break state compatibility, they can have significant impacts on performance and behavior.
These optimizations should lead to improved performance in block processing, consensus, and network communication. However, it's important to:
- Thoroughly test these changes under various network conditions and load scenarios.
- Monitor the system closely after deployment to ensure there are no unexpected side effects.
- Consider providing guidance to node operators on any configuration changes that might be necessary to take full advantage of these optimizations.
Line range hint
99-100: State Compatible: Overwrite timeoutProposeChanging the timeoutPropose from 3s to 2s is a configuration change that could affect the consensus process timing.
This change should lead to faster block proposal times, potentially improving overall network throughput. However:
- Ensure that this change doesn't negatively impact nodes with slower network connections.
- Monitor the network after deployment to verify that it doesn't lead to an increase in timeout events.
| * [#8778](https://github.com/osmosis-labs/osmosis/pull/8778) fix: Fix superfluid delegation edge case on jailed validator | ||
| ## v26.0.1 |
There was a problem hiding this comment.
State Breaking Changes: Superfluid Delegation Edge Case Fix
This change addresses an edge case in superfluid delegation for jailed validators. It's important to note that this is a state-breaking change, which means it could potentially affect existing data or behavior in the system.
Ensure that this fix is thoroughly tested and that its impact on existing superfluid delegations is well understood before deployment.
| func (s *KeeperTestSuite) TestForceUndelegateAndBurnOsmoTokens() { | ||
| testCases := []struct { | ||
| name string | ||
| validatorStats []stakingtypes.BondStatus | ||
| superDelegations []superfluidDelegation | ||
| jailed bool | ||
| jailValWithSmallAmt bool | ||
|
|
||
| expectedShareDiff math.LegacyDec | ||
| }{ | ||
| { | ||
| "with single validator and single superfluid delegation and single undelegation", | ||
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | ||
| []superfluidDelegation{{0, 0, 0, 1000000}}, | ||
| false, | ||
| false, | ||
| math.LegacyMustNewDecFromStr("10"), | ||
| }, | ||
| { | ||
| "jailed validator where superfluid delegation was major delegation", | ||
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | ||
| []superfluidDelegation{{0, 0, 0, 1}}, | ||
| true, | ||
| true, | ||
| math.LegacyDec{}, | ||
| }, | ||
| { | ||
| "jailed validator where superfluid delegation was major delegation", | ||
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | ||
| []superfluidDelegation{{0, 0, 0, 1000000}}, | ||
| true, | ||
| false, | ||
| math.LegacyMustNewDecFromStr("10.526315789473684210"), | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
| s.Run(tc.name, func() { | ||
| s.SetupTest() | ||
|
|
||
| // setup validators | ||
| valAddrs := s.SetupValidators(tc.validatorStats) | ||
|
|
||
| denoms, _ := s.SetupGammPoolsAndSuperfluidAssets([]osmomath.Dec{osmomath.NewDec(20), osmomath.NewDec(20)}) | ||
|
|
||
| // setup superfluid delegations | ||
| _, intermediaryAccs, _ := s.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms) | ||
|
|
||
| delegationBeforeUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| // jail the validator as part of set up | ||
| if tc.jailed { | ||
| validator, err := s.App.StakingKeeper.GetValidator(s.Ctx, valAddrs[0]) | ||
| s.Require().NoError(err) | ||
| s.Ctx = s.Ctx.WithBlockHeight(100) | ||
| consAddr, err := validator.GetConsAddr() | ||
| s.Require().NoError(err) | ||
| // slash by slash factor | ||
| power := sdk.TokensToConsensusPower(validator.Tokens, sdk.DefaultPowerReduction) | ||
|
|
||
| // Note: this calls BeforeValidatorSlashed hook | ||
| s.handleEquivocationEvidence(s.Ctx, &evidencetypes.Equivocation{ | ||
| Height: 80, | ||
| Time: time.Time{}, | ||
| Power: power, | ||
| ConsensusAddress: sdk.ConsAddress(consAddr).String(), | ||
| }) | ||
| val, err := s.App.StakingKeeper.GetValidatorByConsAddr(s.Ctx, consAddr) | ||
| s.Require().NoError(err) | ||
| s.Require().Equal(val.Jailed, true) | ||
| } | ||
|
|
||
| err = s.App.SuperfluidKeeper.ForceUndelegateAndBurnOsmoTokens(s.Ctx, math.NewInt(10), intermediaryAccs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| if !tc.jailValWithSmallAmt { | ||
| delegationAfterUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | ||
| s.Require().NoError(err) | ||
|
|
||
| shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares) | ||
| fmt.Println("share diff: ", shareDiff.String()) | ||
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for multiple validators and delegations
Currently, the test function covers single validator scenarios. Consider adding test cases with multiple validators and multiple superfluid delegations to ensure comprehensive coverage.
Replace math.LegacyDec with osmomath.Dec for consistency
The test function TestForceUndelegateAndBurnOsmoTokens uses math.LegacyDec and math.LegacyMustNewDecFromStr, which are deprecated. Consider updating to osmomath.Dec and osmomath.MustNewDecFromStr to maintain consistency and avoid deprecation issues.
Apply this diff to update the code:
1058,1063c1058,1063
< expectedShareDiff math.LegacyDec
---
> expectedShareDiff osmomath.Dec
1071c1071
< math.LegacyMustNewDecFromStr("10"),
---
> osmomath.MustNewDecFromStr("10"),
1079c1079
< math.LegacyDec{},
---
> osmomath.Dec{},
1087c1087
< math.LegacyMustNewDecFromStr("10.526315789473684210"),
---
> osmomath.MustNewDecFromStr("10.526315789473684210"),📝 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.
| func (s *KeeperTestSuite) TestForceUndelegateAndBurnOsmoTokens() { | |
| testCases := []struct { | |
| name string | |
| validatorStats []stakingtypes.BondStatus | |
| superDelegations []superfluidDelegation | |
| jailed bool | |
| jailValWithSmallAmt bool | |
| expectedShareDiff math.LegacyDec | |
| }{ | |
| { | |
| "with single validator and single superfluid delegation and single undelegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| false, | |
| false, | |
| math.LegacyMustNewDecFromStr("10"), | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1}}, | |
| true, | |
| true, | |
| math.LegacyDec{}, | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| true, | |
| false, | |
| math.LegacyMustNewDecFromStr("10.526315789473684210"), | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| s.Run(tc.name, func() { | |
| s.SetupTest() | |
| // setup validators | |
| valAddrs := s.SetupValidators(tc.validatorStats) | |
| denoms, _ := s.SetupGammPoolsAndSuperfluidAssets([]osmomath.Dec{osmomath.NewDec(20), osmomath.NewDec(20)}) | |
| // setup superfluid delegations | |
| _, intermediaryAccs, _ := s.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms) | |
| delegationBeforeUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| // jail the validator as part of set up | |
| if tc.jailed { | |
| validator, err := s.App.StakingKeeper.GetValidator(s.Ctx, valAddrs[0]) | |
| s.Require().NoError(err) | |
| s.Ctx = s.Ctx.WithBlockHeight(100) | |
| consAddr, err := validator.GetConsAddr() | |
| s.Require().NoError(err) | |
| // slash by slash factor | |
| power := sdk.TokensToConsensusPower(validator.Tokens, sdk.DefaultPowerReduction) | |
| // Note: this calls BeforeValidatorSlashed hook | |
| s.handleEquivocationEvidence(s.Ctx, &evidencetypes.Equivocation{ | |
| Height: 80, | |
| Time: time.Time{}, | |
| Power: power, | |
| ConsensusAddress: sdk.ConsAddress(consAddr).String(), | |
| }) | |
| val, err := s.App.StakingKeeper.GetValidatorByConsAddr(s.Ctx, consAddr) | |
| s.Require().NoError(err) | |
| s.Require().Equal(val.Jailed, true) | |
| } | |
| err = s.App.SuperfluidKeeper.ForceUndelegateAndBurnOsmoTokens(s.Ctx, math.NewInt(10), intermediaryAccs[0]) | |
| s.Require().NoError(err) | |
| if !tc.jailValWithSmallAmt { | |
| delegationAfterUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares) | |
| fmt.Println("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | |
| } | |
| }) | |
| } | |
| } | |
| func (s *KeeperTestSuite) TestForceUndelegateAndBurnOsmoTokens() { | |
| testCases := []struct { | |
| name string | |
| validatorStats []stakingtypes.BondStatus | |
| superDelegations []superfluidDelegation | |
| jailed bool | |
| jailValWithSmallAmt bool | |
| expectedShareDiff osmomath.Dec | |
| }{ | |
| { | |
| "with single validator and single superfluid delegation and single undelegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| false, | |
| false, | |
| osmomath.MustNewDecFromStr("10"), | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1}}, | |
| true, | |
| true, | |
| osmomath.Dec{}, | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| true, | |
| false, | |
| osmomath.MustNewDecFromStr("10.526315789473684210"), | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| s.Run(tc.name, func() { | |
| s.SetupTest() | |
| // setup validators | |
| valAddrs := s.SetupValidators(tc.validatorStats) | |
| denoms, _ := s.SetupGammPoolsAndSuperfluidAssets([]osmomath.Dec{osmomath.NewDec(20), osmomath.NewDec(20)}) | |
| // setup superfluid delegations | |
| _, intermediaryAccs, _ := s.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms) | |
| delegationBeforeUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| // jail the validator as part of set up | |
| if tc.jailed { | |
| validator, err := s.App.StakingKeeper.GetValidator(s.Ctx, valAddrs[0]) | |
| s.Require().NoError(err) | |
| s.Ctx = s.Ctx.WithBlockHeight(100) | |
| consAddr, err := validator.GetConsAddr() | |
| s.Require().NoError(err) | |
| // slash by slash factor | |
| power := sdk.TokensToConsensusPower(validator.Tokens, sdk.DefaultPowerReduction) | |
| // Note: this calls BeforeValidatorSlashed hook | |
| s.handleEquivocationEvidence(s.Ctx, &evidencetypes.Equivocation{ | |
| Height: 80, | |
| Time: time.Time{}, | |
| Power: power, | |
| ConsensusAddress: sdk.ConsAddress(consAddr).String(), | |
| }) | |
| val, err := s.App.StakingKeeper.GetValidatorByConsAddr(s.Ctx, consAddr) | |
| s.Require().NoError(err) | |
| s.Require().Equal(val.Jailed, true) | |
| } | |
| err = s.App.SuperfluidKeeper.ForceUndelegateAndBurnOsmoTokens(s.Ctx, math.NewInt(10), intermediaryAccs[0]) | |
| s.Require().NoError(err) | |
| if !tc.jailValWithSmallAmt { | |
| delegationAfterUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares) | |
| fmt.Println("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | |
| } | |
| }) | |
| } | |
| } |
Ensure error handling when retrieving delegations after undelegation
In the test cases where jailValWithSmallAmt is true, retrieving the delegation after undelegation may result in an error. Ensure that the error is appropriately handled to prevent unexpected test failures.
Apply this diff to handle the error condition:
- s.Require().NoError(err)
- shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares)
- s.Require().True(shareDiff.Equal(tc.expectedShareDiff))
+ if tc.jailValWithSmallAmt {
+ s.Require().Error(err)
+ } else {
+ s.Require().NoError(err)
+ shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares)
+ s.Require().True(shareDiff.Equal(tc.expectedShareDiff))
+ }📝 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.
| func (s *KeeperTestSuite) TestForceUndelegateAndBurnOsmoTokens() { | |
| testCases := []struct { | |
| name string | |
| validatorStats []stakingtypes.BondStatus | |
| superDelegations []superfluidDelegation | |
| jailed bool | |
| jailValWithSmallAmt bool | |
| expectedShareDiff math.LegacyDec | |
| }{ | |
| { | |
| "with single validator and single superfluid delegation and single undelegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| false, | |
| false, | |
| math.LegacyMustNewDecFromStr("10"), | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1}}, | |
| true, | |
| true, | |
| math.LegacyDec{}, | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| true, | |
| false, | |
| math.LegacyMustNewDecFromStr("10.526315789473684210"), | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| s.Run(tc.name, func() { | |
| s.SetupTest() | |
| // setup validators | |
| valAddrs := s.SetupValidators(tc.validatorStats) | |
| denoms, _ := s.SetupGammPoolsAndSuperfluidAssets([]osmomath.Dec{osmomath.NewDec(20), osmomath.NewDec(20)}) | |
| // setup superfluid delegations | |
| _, intermediaryAccs, _ := s.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms) | |
| delegationBeforeUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| // jail the validator as part of set up | |
| if tc.jailed { | |
| validator, err := s.App.StakingKeeper.GetValidator(s.Ctx, valAddrs[0]) | |
| s.Require().NoError(err) | |
| s.Ctx = s.Ctx.WithBlockHeight(100) | |
| consAddr, err := validator.GetConsAddr() | |
| s.Require().NoError(err) | |
| // slash by slash factor | |
| power := sdk.TokensToConsensusPower(validator.Tokens, sdk.DefaultPowerReduction) | |
| // Note: this calls BeforeValidatorSlashed hook | |
| s.handleEquivocationEvidence(s.Ctx, &evidencetypes.Equivocation{ | |
| Height: 80, | |
| Time: time.Time{}, | |
| Power: power, | |
| ConsensusAddress: sdk.ConsAddress(consAddr).String(), | |
| }) | |
| val, err := s.App.StakingKeeper.GetValidatorByConsAddr(s.Ctx, consAddr) | |
| s.Require().NoError(err) | |
| s.Require().Equal(val.Jailed, true) | |
| } | |
| err = s.App.SuperfluidKeeper.ForceUndelegateAndBurnOsmoTokens(s.Ctx, math.NewInt(10), intermediaryAccs[0]) | |
| s.Require().NoError(err) | |
| if !tc.jailValWithSmallAmt { | |
| delegationAfterUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares) | |
| fmt.Println("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | |
| } | |
| }) | |
| } | |
| } | |
| func (s *KeeperTestSuite) TestForceUndelegateAndBurnOsmoTokens() { | |
| testCases := []struct { | |
| name string | |
| validatorStats []stakingtypes.BondStatus | |
| superDelegations []superfluidDelegation | |
| jailed bool | |
| jailValWithSmallAmt bool | |
| expectedShareDiff math.LegacyDec | |
| }{ | |
| { | |
| "with single validator and single superfluid delegation and single undelegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| false, | |
| false, | |
| math.LegacyMustNewDecFromStr("10"), | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1}}, | |
| true, | |
| true, | |
| math.LegacyDec{}, | |
| }, | |
| { | |
| "jailed validator where superfluid delegation was major delegation", | |
| []stakingtypes.BondStatus{stakingtypes.Bonded}, | |
| []superfluidDelegation{{0, 0, 0, 1000000}}, | |
| true, | |
| false, | |
| math.LegacyMustNewDecFromStr("10.526315789473684210"), | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| s.Run(tc.name, func() { | |
| s.SetupTest() | |
| // setup validators | |
| valAddrs := s.SetupValidators(tc.validatorStats) | |
| denoms, _ := s.SetupGammPoolsAndSuperfluidAssets([]osmomath.Dec{osmomath.NewDec(20), osmomath.NewDec(20)}) | |
| // setup superfluid delegations | |
| _, intermediaryAccs, _ := s.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms) | |
| delegationBeforeUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| s.Require().NoError(err) | |
| // jail the validator as part of set up | |
| if tc.jailed { | |
| validator, err := s.App.StakingKeeper.GetValidator(s.Ctx, valAddrs[0]) | |
| s.Require().NoError(err) | |
| s.Ctx = s.Ctx.WithBlockHeight(100) | |
| consAddr, err := validator.GetConsAddr() | |
| s.Require().NoError(err) | |
| // slash by slash factor | |
| power := sdk.TokensToConsensusPower(validator.Tokens, sdk.DefaultPowerReduction) | |
| // Note: this calls BeforeValidatorSlashed hook | |
| s.handleEquivocationEvidence(s.Ctx, &evidencetypes.Equivocation{ | |
| Height: 80, | |
| Time: time.Time{}, | |
| Power: power, | |
| ConsensusAddress: sdk.ConsAddress(consAddr).String(), | |
| }) | |
| val, err := s.App.StakingKeeper.GetValidatorByConsAddr(s.Ctx, consAddr) | |
| s.Require().NoError(err) | |
| s.Require().Equal(val.Jailed, true) | |
| } | |
| err = s.App.SuperfluidKeeper.ForceUndelegateAndBurnOsmoTokens(s.Ctx, math.NewInt(10), intermediaryAccs[0]) | |
| s.Require().NoError(err) | |
| if !tc.jailValWithSmallAmt { | |
| delegationAfterUndelegate, err := s.App.StakingKeeper.GetDelegation(s.Ctx, intermediaryAccs[0].GetAccAddress(), valAddrs[0]) | |
| if tc.jailValWithSmallAmt { | |
| s.Require().Error(err) | |
| } else { | |
| s.Require().NoError(err) | |
| shareDiff := delegationBeforeUndelegate.Shares.Sub(delegationAfterUndelegate.Shares) | |
| fmt.Println("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | |
| } | |
| } | |
| }) | |
| } | |
| } |
| fmt.Println("share diff: ", shareDiff.String()) | ||
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use s.T().Log instead of fmt.Println for test logging
Using s.T().Log integrates better with the testing framework and provides more control over test output. Replace fmt.Println with s.T().Log to adhere to best practices.
Apply this diff to update the logging:
- fmt.Println("share diff: ", shareDiff.String())
+ s.T().Log("share diff: ", shareDiff.String())📝 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.
| fmt.Println("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) | |
| s.T().Log("share diff: ", shareDiff.String()) | |
| s.Require().True(shareDiff.Equal(tc.expectedShareDiff)) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/superfluid/keeper/stake.go (2)
Line range hint
507-554: Improved error handling for ValidateUnbondAmountThe changes to the
forceUndelegateAndBurnOsmoTokensfunction have improved the error handling for theValidateUnbondAmountcall. If the validation fails, the function now calculates the shares to unbond based on the minimum of the delegation's shares and the validator's shares from the osmoAmount. This approach handles edge cases where the requested unbond amount exceeds the available delegation.Consider adding error logging for the
validator.SharesFromTokens(osmoAmount)call at line 525. While the error is being returned, logging it would provide more context for debugging:valShares, err := validator.SharesFromTokens(osmoAmount) if err != nil { + ctx.Logger().Error("Error calculating shares from tokens", "error", err) return err }
Line range hint
1-893: Summary of changes: Improved handling of jailed validator edge caseThe main change in this file is in the
forceUndelegateAndBurnOsmoTokensfunction. This change improves the handling of edge cases, particularly for jailed validators, when undelegating and burning OSMO tokens. The new implementation calculates the shares to unbond based on the minimum of the delegation's shares and the validator's shares from the osmoAmount when theValidateUnbondAmountcall fails.This change aligns well with the PR objective of fixing a superfluid delegation edge case on jailed validators. It should make the system more robust in handling these scenarios.
The rest of the file, including functions like
UnbondConvertAndStake,convertLockToStake,convertUnlockedToStake,convertGammSharesToOsmoAndStake, anddelegateBaseOnValsetPref, remain unchanged.Consider adding unit tests specifically for the jailed validator edge case in the
forceUndelegateAndBurnOsmoTokensfunction to ensure the new logic works as expected in various scenarios.
x/superfluid/keeper/stake.go
Outdated
| if err == stakingtypes.ErrNoDelegation { | ||
| return nil |
There was a problem hiding this comment.
We still want this check right?
There was a problem hiding this comment.
yes, added it back in the latest commit. Thanks for the catch 🙏
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/superfluid/keeper/stake.go (1)
507-534: Improved error handling and delegation logicThe changes in this function enhance its robustness by handling edge cases more effectively. The new logic for dealing with situations where the unbond amount exceeds the validator's delegation is a good addition.
Consider adding a comment explaining the rationale behind using
math.LegacyMinDec(del.Shares, valShares)to improve code readability.
|
|
||
| if err != nil { | ||
| // if ValidateUnbondAmount has failed it indicates that the amount we're trying to unbond is | ||
| // greater then what validator has this can be due to different factors (e.g jail) |
There was a problem hiding this comment.
| // greater then what validator has this can be due to different factors (e.g jail) | |
| // greater then what validator has delegated to it. This can be due to different factors (e.g jail) |
This actually doesn't quite check out to me as an explanation. If a validator is downtime jailed on Osmosis, there is no slash. So why would the unbond amount tracked be greater than expected post jail?
| if err != nil { | ||
| // if ValidateUnbondAmount has failed it indicates that the amount we're trying to unbond is | ||
| // greater then what validator has this can be due to different factors (e.g jail) | ||
| // in this case we brun min(amount_tpo_burn, total_delegation_share) |
There was a problem hiding this comment.
| // in this case we brun min(amount_tpo_burn, total_delegation_share) | |
| // in this case we run min(amount_to_burn, total_delegation_share) |
I think that is what was meant?
There was a problem hiding this comment.
@mattverse still trying to grok everything. As a side note, has this been run on in place testnet? I am curious of the following:
- Run in place testnet without upgrade. See epoch logs. See that the error is occuring
- Probably just reset but run with the trigger upgrade flag so you don't have to manually define the gov prop. Watch the epoch after upgrade. What errors occur, if any.
- Post epoch, manually check values (or add prints to make it easier) to see if there was any unexpected behavior
Its possible in place testnet might cause some oddities due to messing with validators and voting power though, so its possible the above wont be a good testing solution. But I think its worth a go.
|
@czarcas7ic |
|
No sorry for 1 I meant, make sure that running in place testnet with v26 still shows the same error at epoch @mattverse |
|
The reason I want 1 is because, its unclear to me that after in place testnet modifies the validator set that we would still get the same error. If we get the same error, then good, we can test to see if this fix works. But if we no longer get the error after #1, then running in place testnet to test the fix is less helpful. |
|
@czarcas7ic Ah understood why you want 1. I'll start with 1 rn and lyk if we have the same error logs. I am assuming we should tho |
I am too, just didn't want false confidence in the event the error was no longer showing due to testnetify magic |
|
Hey @czarcas7ic I've tried replicating this on in place testnet, but the problem is that we would not be able to get the errors we want (for step 1 and of course in step2 as well), with the following errors. The reasons we are having this is because for in place testnet, we are deleting all the val states, but not the val states in superfluid, thus on epoch we would be hitting these error logs instead of the error logs we were expecting to see. |
|
Damn. Thinking about it, we could probably add some logic to testnetify on the sdk side that special cases the validator in question and does not delete its info.. This would take some time to do though, and require knowledge of the testnetify sytem |
Closes: #XXX
What is the purpose of the change
This PR fixes an edge case for a jailed validator when refreshing intermediary account amounts on epoch.
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)