chore: force undelegate and clean up superfluid state #9552
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a v31 upgrade cleanup that undelegates intermediary accounts, force-unlocks synthetic locks, and deletes all superfluid-related KV state during the v31 upgrade. Adds end-to-end tests for the cleanup and a CHANGELOG entry referencing PR 9552. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Node
participant UpgradeHandler as Upgrade Handler (v31)
participant Cleanup as Superfluid Cleanup
participant Staking as StakingKeeper
participant Lockup as LockupKeeper
participant Bank as BankKeeper
participant Store as KV Store
Node->>UpgradeHandler: Trigger v31 upgrade
activate UpgradeHandler
Note over UpgradeHandler: Run existing migrations...
UpgradeHandler->>Cleanup: cleanupSuperfluid()
activate Cleanup
Cleanup->>Staking: undelegateAllIntermediaryAccounts() (per intermediary)
loop per intermediary
Staking-->>Cleanup: delegation/shares info
Cleanup->>Staking: instant undelegate
Cleanup->>Bank: transfer undelegated tokens to module
Cleanup->>Bank: burn tokens & adjust supply offset
Note right of Cleanup: Log errors and continue
end
Cleanup->>Lockup: unlockAllSyntheticLocks() (per connection)
loop per lock-intermediary connection
Lockup->>Lockup: force-unlock underlying lock
Note right of Lockup: Log errors and continue
end
Cleanup->>Store: deleteAllSuperfluidStorage() (remove superfluid key prefixes)
Store-->>Cleanup: iteration complete
deactivate Cleanup
UpgradeHandler-->>Node: cleanup completed (duration logged)
deactivate UpgradeHandler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)app/upgrades/v31/upgrades.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)app/upgrades/v31/upgrades.go(4 hunks)app/upgrades/v31/upgrades_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/upgrades/v31/upgrades.go (3)
app/keepers/keepers.go (1)
AppKeepers(128-198)x/protorev/types/expected_keepers.go (1)
BankKeeper(22-28)x/txfees/types/expected_keepers.go (1)
BankKeeper(76-84)
app/upgrades/v31/upgrades_test.go (1)
x/protorev/types/expected_keepers.go (2)
BankKeeper(22-28)AccountKeeper(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: go (01)
- GitHub Check: go (02)
- GitHub Check: go (00)
- GitHub Check: go (03)
- GitHub Check: e2e
- GitHub Check: test
- GitHub Check: Run golangci-lint
- GitHub Check: Summary
| valAddr := sdk.ValAddress([]byte("testvaraddr1")) | ||
| intermediaryAcc := superfuidtypes.NewSuperfluidIntermediaryAccount(denom, valAddr.String(), 1) | ||
| s.App.SuperfluidKeeper.SetIntermediaryAccount(s.Ctx, intermediaryAcc) | ||
|
|
||
| // Create a lock for testing | ||
| lockOwner := s.TestAccs[0] | ||
| coins := sdk.NewCoins(sdk.NewCoin(denom, osmomath.NewInt(1000000))) |
There was a problem hiding this comment.
Fix the validator address setup to avoid panic.
sdk.ValAddress([]byte("testvaraddr1")) is only 12 bytes, so valAddr.String() panics when it runs through the bech32 length check. The test suite will crash before any assertions execute. Please switch to a 20-byte address (for example, reuse one of the pre-funded test accounts) before calling String() so the helper stays deterministic.
- valAddr := sdk.ValAddress([]byte("testvaraddr1"))
+ valAddr := sdk.ValAddress(s.TestAccs[1])📝 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.
| valAddr := sdk.ValAddress([]byte("testvaraddr1")) | |
| intermediaryAcc := superfuidtypes.NewSuperfluidIntermediaryAccount(denom, valAddr.String(), 1) | |
| s.App.SuperfluidKeeper.SetIntermediaryAccount(s.Ctx, intermediaryAcc) | |
| // Create a lock for testing | |
| lockOwner := s.TestAccs[0] | |
| coins := sdk.NewCoins(sdk.NewCoin(denom, osmomath.NewInt(1000000))) | |
| // Use a valid 20-byte validator address from pre-funded test accounts | |
| valAddr := sdk.ValAddress(s.TestAccs[1]) | |
| intermediaryAcc := superfuidtypes.NewSuperfluidIntermediaryAccount(denom, valAddr.String(), 1) | |
| s.App.SuperfluidKeeper.SetIntermediaryAccount(s.Ctx, intermediaryAcc) | |
| // Create a lock for testing | |
| lockOwner := s.TestAccs[0] | |
| coins := sdk.NewCoins(sdk.NewCoin(denom, osmomath.NewInt(1000000))) |
🤖 Prompt for AI Agents
In app/upgrades/v31/upgrades_test.go around lines 142 to 148, the test
constructs a validator address from a 12-byte literal which causes a bech32
length panic when calling String(); replace that construction with a proper
20-byte validator address (for example reuse one of the pre-funded test accounts
in s.TestAccs by converting its address to a sdk.ValAddress) before calling
String(), then continue to create and set the SuperfluidIntermediaryAccount and
rest of the test as before.
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/upgrades/v31/constants.go (1)
22-26: Improve the comment grammar and consider staleness risk.The comment has a grammatical issue: "All superfluid delegation will not increase there is no asset allowed" should read "All superfluid delegations will no longer increase as there are no assets allowed...".
Additionally, the hardcoded value (385298452) is sourced from a point-in-time LCD query. If the actual on-chain total at upgrade time differs from this constant (due to ongoing chain activity between now and the upgrade), the validation at line 149-151 of upgrades.go could fail the entire upgrade.
Apply this diff to improve the comment:
- // All superfluid delegation will not increase there is no asset allowed to perform superfluid delegation - // So it is ok to set the check for total undelegated amount to the current total amount + // All superfluid delegations will no longer increase as there are no assets allowed to perform superfluid delegation. + // This constant represents the maximum expected undelegation amount based on the snapshot at: // https://lcd.osmosis.zone/osmosis/superfluid/v1beta1/all_superfluid_delegations totalSuperfluidDelegationAmount = osmomath.NewInt(385298452)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/upgrades/v31/constants.go(2 hunks)app/upgrades/v31/upgrades.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/upgrades/v31/constants.go (1)
osmomath/sdk_math_alias.go (1)
NewInt(38-38)
app/upgrades/v31/upgrades.go (3)
app/keepers/keepers.go (1)
AppKeepers(128-198)x/protorev/types/expected_keepers.go (1)
BankKeeper(22-28)x/txfees/types/expected_keepers.go (1)
BankKeeper(76-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: e2e
- GitHub Check: Cursor Bugbot
- GitHub Check: test
- GitHub Check: Run golangci-lint
- GitHub Check: Summary
🔇 Additional comments (4)
app/upgrades/v31/constants.go (1)
4-4: LGTM!The osmomath import is correctly added to support the NewInt function used in the totalSuperfluidDelegationAmount initialization.
app/upgrades/v31/upgrades.go (3)
44-48: LGTM!The cleanupSuperfluid integration is correctly placed after RunMigrations and updateTakerFeeDistribution, with proper error propagation to halt the upgrade on failure.
79-110: LGTM! Well-structured orchestration with clear phases.The three-phase cleanup (undelegate → unlock → delete storage) is logically ordered, has comprehensive logging with timing, and properly propagates errors from each phase.
265-288: The implementation deletes the entire superfluid store, not specific prefixes.The comment at lines 266-271 lists specific key prefixes that should be deleted, but the implementation uses a full store iterator (
store.Iterator(nil, nil)) and deletes every key. This is correct if the entire superfluid module is being removed and the store should be completely empty afterward, but it's worth confirming this is intentional.If only specific prefixes should be deleted, use prefix iterators instead:
prefixes := [][]byte{ superfuidtypes.KeyPrefixLockIntermediaryAccAddr, superfuidtypes.KeyPrefixIntermediaryAccount, superfuidtypes.KeyPrefixSuperfluidAsset, superfuidtypes.KeyPrefixTokenMultiplier, superfuidtypes.KeyUnpoolAllowedPools, } for _, prefix := range prefixes { iterator := sdk.KVStorePrefixIterator(store, prefix) defer iterator.Close() keysToDelete := [][]byte{} for ; iterator.Valid(); iterator.Next() { keysToDelete = append(keysToDelete, iterator.Key()) } for _, key := range keysToDelete { store.Delete(key) } }Can you confirm whether the entire superfluid store should be wiped (current implementation) or only specific prefixes (per the comment)?
| if err != nil { | ||
| // Log error but continue with other accounts | ||
| ctx.Logger().Error(fmt.Sprintf("Failed to undelegate intermediary account %s: %v", | ||
| intermediaryAcc.GetAccAddress().String(), err)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The "log and continue" error handling could mask failures and allow incomplete cleanup.
When undelegateSingleIntermediaryAccount fails, the error is logged but processing continues. This means:
- Some intermediary accounts may remain delegated
- The total undelegated amount will be less than expected
- However, the validation at line 149-151 uses
GT(greater than), so a lower total would pass - The upgrade could succeed with orphaned state still present
Consider either:
- Failing fast on the first error to ensure complete cleanup, or
- Collecting all errors and validating that the success count matches the total count before proceeding
🤖 Prompt for AI Agents
In app/upgrades/v31/upgrades.go around lines 128–133, instead of logging and
continuing when undelegateSingleIntermediaryAccount fails, accumulate errors and
track success count; after the loop, if any errors occurred or the number of
successful undelegations does not equal the number of intermediary accounts,
return an error to abort the upgrade (do not proceed), and update the later
validation (lines ~149–151) to require the expected undelegated amount/success
count matches exactly (or fail) rather than using a permissive GT check.
| denoms := totalUndelegatedCoins.Denoms() | ||
| if len(denoms) > 1 || (len(denoms) == 1 && denoms[0] != bondDenom) { | ||
| return fmt.Errorf("expected only %s denom, but got: %v", bondDenom, denoms) | ||
| } | ||
|
|
||
| totalAmount := totalUndelegatedCoins.AmountOf(bondDenom) | ||
| if totalAmount.GT(totalSuperfluidDelegationAmount) { | ||
| return fmt.Errorf("total undelegated amount %s is greater than expected %s", totalAmount.String(), totalSuperfluidDelegationAmount.String()) | ||
| } |
There was a problem hiding this comment.
The validation logic could pass with zero undelegations or incomplete cleanup.
Two concerns with this validation:
-
Empty denoms check (line 144): If
totalUndelegatedCoinsis empty (no undelegations occurred),denomswill be an empty slice, which passes the conditionlen(denoms) > 1 || (len(denoms) == 1 && denoms[0] != bondDenom)as false. ThenAmountOf(bondDenom)returns zero, which passes theGTcheck at line 149. This would allow the upgrade to succeed with no undelegations performed. -
GT vs GTE (line 149): The check
totalAmount.GT(totalSuperfluidDelegationAmount)only fails if total is strictly greater. Combined with the "log and continue" error handling at lines 128-133, if some accounts fail to undelegate, the total will be less than expected but still pass this check. The comment in constants.go line 24 suggests the check should validate against the expected amount, but the current implementation allows any amount ≤ expected.
Consider this diff to add explicit validation:
denoms := totalUndelegatedCoins.Denoms()
+ if len(denoms) == 0 {
+ return fmt.Errorf("no coins were undelegated, expected %s %s", totalSuperfluidDelegationAmount.String(), bondDenom)
+ }
if len(denoms) > 1 || (len(denoms) == 1 && denoms[0] != bondDenom) {
return fmt.Errorf("expected only %s denom, but got: %v", bondDenom, denoms)
}
totalAmount := totalUndelegatedCoins.AmountOf(bondDenom)
+ if totalAmount.IsZero() {
+ return fmt.Errorf("zero amount undelegated, expected %s %s", totalSuperfluidDelegationAmount.String(), bondDenom)
+ }
if totalAmount.GT(totalSuperfluidDelegationAmount) {
return fmt.Errorf("total undelegated amount %s is greater than expected %s", totalAmount.String(), totalSuperfluidDelegationAmount.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.
| denoms := totalUndelegatedCoins.Denoms() | |
| if len(denoms) > 1 || (len(denoms) == 1 && denoms[0] != bondDenom) { | |
| return fmt.Errorf("expected only %s denom, but got: %v", bondDenom, denoms) | |
| } | |
| totalAmount := totalUndelegatedCoins.AmountOf(bondDenom) | |
| if totalAmount.GT(totalSuperfluidDelegationAmount) { | |
| return fmt.Errorf("total undelegated amount %s is greater than expected %s", totalAmount.String(), totalSuperfluidDelegationAmount.String()) | |
| } | |
| denoms := totalUndelegatedCoins.Denoms() | |
| if len(denoms) == 0 { | |
| return fmt.Errorf("no coins were undelegated, expected %s %s", totalSuperfluidDelegationAmount.String(), bondDenom) | |
| } | |
| if len(denoms) > 1 || (len(denoms) == 1 && denoms[0] != bondDenom) { | |
| return fmt.Errorf("expected only %s denom, but got: %v", bondDenom, denoms) | |
| } | |
| totalAmount := totalUndelegatedCoins.AmountOf(bondDenom) | |
| if totalAmount.IsZero() { | |
| return fmt.Errorf("zero amount undelegated, expected %s %s", totalSuperfluidDelegationAmount.String(), bondDenom) | |
| } | |
| if totalAmount.GT(totalSuperfluidDelegationAmount) { | |
| return fmt.Errorf("total undelegated amount %s is greater than expected %s", totalAmount.String(), totalSuperfluidDelegationAmount.String()) | |
| } |
🤖 Prompt for AI Agents
In app/upgrades/v31/upgrades.go around lines 143 to 151, the current validation
lets an empty undelegation result or a shortfall pass; change it to first
explicitly fail if denoms is empty or contains any denom other than bondDenom,
and then require the aggregated totalAmount to exactly match the expected
totalSuperfluidDelegationAmount (i.e. use an equality check instead of only GT),
returning a clear fmt.Errorf when either the denom set is empty/invalid or the
totalAmount does not equal the expected amount.
| delegation, err := keepers.StakingKeeper.GetDelegation(ctx, intermediaryAcc.GetAccAddress(), valAddr) | ||
| if err != nil { | ||
| // No delegation found, skip | ||
| return sdk.Coins{}, nil | ||
| } |
There was a problem hiding this comment.
The error handling conflates "delegation not found" with actual errors.
When GetDelegation returns an error, the code returns (sdk.Coins{}, nil), treating all errors as "no delegation found". However, GetDelegation can fail for reasons other than missing delegation (e.g., store access errors, corruption). These actual errors should be propagated rather than silently ignored.
Apply this diff to distinguish error cases:
// Check if there's a delegation from this intermediary account
delegation, err := keepers.StakingKeeper.GetDelegation(ctx, intermediaryAcc.GetAccAddress(), valAddr)
if err != nil {
- // No delegation found, skip
- return sdk.Coins{}, nil
+ // Check if it's a "not found" error vs a real error
+ if errors.Is(err, stakingtypes.ErrNoDelegation) {
+ // No delegation found, skip
+ return sdk.Coins{}, nil
+ }
+ return sdk.Coins{}, fmt.Errorf("failed to get delegation: %w", err)
}Then add the necessary import:
import (
"context"
+ "errors"
"fmt"
"time"📝 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.
| delegation, err := keepers.StakingKeeper.GetDelegation(ctx, intermediaryAcc.GetAccAddress(), valAddr) | |
| if err != nil { | |
| // No delegation found, skip | |
| return sdk.Coins{}, nil | |
| } | |
| // Check if there's a delegation from this intermediary account | |
| delegation, err := keepers.StakingKeeper.GetDelegation(ctx, intermediaryAcc.GetAccAddress(), valAddr) | |
| if err != nil { | |
| // Check if it's a "not found" error vs a real error | |
| if errors.Is(err, stakingtypes.ErrNoDelegation) { | |
| // No delegation found, skip | |
| return sdk.Coins{}, nil | |
| } | |
| return sdk.Coins{}, fmt.Errorf("failed to get delegation: %w", err) | |
| } |
| delegation, err := keepers.StakingKeeper.GetDelegation(ctx, intermediaryAcc.GetAccAddress(), valAddr) | |
| if err != nil { | |
| // No delegation found, skip | |
| return sdk.Coins{}, nil | |
| } | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "time" | |
| // …other imports… | |
| ) |
🤖 Prompt for AI Agents
In app/upgrades/v31/upgrades.go around lines 168-172, the current error handling
treats any error from GetDelegation as "no delegation found"; change it to
distinguish the not-found case from real errors by checking errors.Is(err,
stakingtypes.ErrNoDelegation) and only return (sdk.Coins{}, nil) for that
specific case, otherwise propagate the error (return sdk.Coins{}, err); add
imports for the standard "errors" package and staking types
("github.com/cosmos/cosmos-sdk/x/staking/types") so you can use errors.Is with
stakingtypes.ErrNoDelegation.
| if err := deleteForceUnlockSyntheticLock(ctx, keepers, connection); err != nil { | ||
| // Log error but continue with other locks | ||
| ctx.Logger().Error(fmt.Sprintf("Failed to delete synthetic locks for lock %d: %v", connection.LockId, err)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Similar "log and continue" issue as in undelegateAllIntermediaryAccounts.
When deleteForceUnlockSyntheticLock fails, the error is logged but processing continues. This could leave some locks in an inconsistent state (underlying lock force-unlocked but synthetic lock not deleted, or vice versa). Since there's no validation of success rate afterward, the upgrade could complete with partial cleanup.
Consider the same remediation as suggested for undelegateAllIntermediaryAccounts: either fail fast or validate success count.
🤖 Prompt for AI Agents
In app/upgrades/v31/upgrades.go around lines 236 to 240, the code logs errors
from deleteForceUnlockSyntheticLock and continues, which can leave
partial/inconsistent state; change behavior to either (a) fail fast by returning
the error (or wrapping it with context) so the upgrade aborts on any deletion
failure, or (b) track success/failure counts for all processed connections and
after the loop validate that all deletions succeeded, returning an error if any
failed; implement one of these strategies and ensure the returned error includes
the connection ID and original error for debugging.
1 participant
Closes: CHAIN-1040
What is the purpose of the change
Force undelegate and remove superfluid staking state to prepare for the full module removal.
Testing and Verifying
Unit tested and manually observing in-place testnet.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)