feat(x/auth/vesting): change TrackDelegation and TrackUndelegation to return errors instead of panicking #25295
Conversation
📝 WalkthroughWalkthroughAdds error returns to VestingAccount.TrackDelegation and TrackUndelegation interfaces. Updates all vesting account implementations to validate inputs and return errors instead of panicking. Propagates and handles these errors in x/bank keeper. Updates tests accordingly. Interfaces adjusted in both x/auth/vesting and x/bank/types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Msg/Module as Caller (Bank)
participant Keeper as BankKeeper
participant VA as VestingAccount
Note over Keeper,VA: Delegation flow with error returns
Msg/Module->>Keeper: trackDelegation(addr, amt)
Keeper->>VA: TrackDelegation(blockTime, balance, amt)
alt ok
VA-->>Keeper: nil
Keeper->>Keeper: SetAccount(updated)
Keeper-->>Msg/Module: nil
else error
VA-->>Keeper: error
Keeper-->>Msg/Module: wrapped error
end
sequenceDiagram
autonumber
participant Msg/Module as Caller (Bank)
participant Keeper as BankKeeper
participant VA as VestingAccount
Note over Keeper,VA: Undelegation flow with error returns
Msg/Module->>Keeper: trackUndelegation(addr, amt)
Keeper->>VA: TrackUndelegation(amt)
alt ok
VA-->>Keeper: nil
Keeper->>Keeper: SetAccount(updated)
Keeper-->>Msg/Module: nil
else error
VA-->>Keeper: error
Keeper-->>Msg/Module: wrapped error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
x/bank/keeper/keeper.go (2)
130-151: Prevent partial state updates: reject non-positive amounts before any balance mutations.If TrackDelegation returns an error (e.g., non-positive amounts), we’ve already deducted balances in the loop above, leading to inconsistent state. Add an explicit positivity check so TrackDelegation shouldn’t error after we mutate balances.
if !amt.IsValid() { return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) } + // Reject zero/negative amounts up-front to avoid TrackDelegation errors after deductions. + if !amt.IsAllPositive() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, "amount must be > 0") + } balances := sdk.NewCoins()Follow-up (optional, safer): call k.trackDelegation before setBalance and perform a best-effort rollback (TrackUndelegation) if a later write fails. Can provide a patch if you want to go that route.
179-189: Same issue on undelegation: reject non-positive amounts before mutating the module account balance.Avoid subtracting from the module account and then returning on TrackUndelegation error.
if !amt.IsValid() { return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String()) } + if !amt.IsAllPositive() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, "amount must be > 0") + }
🧹 Nitpick comments (7)
x/auth/vesting/exported/exported.go (1)
25-31: Doc phrasing: prefer “non-positive amount” and avoid duplicating specifics across packages.Nit: use “non-positive amount” instead of “zero amount” to match typical SDK validation language and mirror the same wording in x/bank/types. Also consider keeping the comment generic (“returns an error if the operation is invalid”) to reduce drift between this interface and the mirror in x/bank/types.
Apply:
-// Returns an error if the delegation is invalid (e.g., zero amount or insufficient funds). +// Returns an error if the delegation is invalid (e.g., non-positive amount or insufficient funds). ... -// Returns an error if the undelegation is invalid (e.g., zero amount). +// Returns an error if the undelegation is invalid (e.g., non-positive amount).x/bank/types/vesting.go (1)
23-29: Keep wording consistent with exported interface (use “non-positive amount”).Mirror the phrasing used in x/auth/vesting/exported to avoid doc drift.
-// Returns an error if the delegation is invalid (e.g., zero amount or insufficient funds). +// Returns an error if the delegation is invalid (e.g., non-positive amount or insufficient funds). ... -// Returns an error if the undelegation is invalid (e.g., zero amount). +// Returns an error if the undelegation is invalid (e.g., non-positive amount).x/bank/keeper/keeper.go (2)
151-153: Avoid double-wrapping errors; keep one contextual layer.trackDelegation/trackUndelegation already return an error wrapped with account context. Wrapping again here duplicates “failed to track …” verbiage.
- if err := k.trackDelegation(ctx, delegatorAddr, balances, amt); err != nil { - return errorsmod.Wrap(err, "failed to track delegation") - } + if err := k.trackDelegation(ctx, delegatorAddr, balances, amt); err != nil { + return err + } ... - if err := k.trackUndelegation(ctx, delegatorAddr, amt); err != nil { - return errorsmod.Wrap(err, "failed to track undelegation") - } + if err := k.trackUndelegation(ctx, delegatorAddr, amt); err != nil { + return err + }Also applies to: 188-190
441-451: Remove “TODO: COMPLETED” blocks.These are noisy commit notes in production code. The surrounding code and commit history already convey this change.
- // TODO: ✅ COMPLETED - Changed VestingAccount interface to return errors instead of panicking - // - Updated TrackDelegation(blockTime time.Time, balance, amount sdk.Coins) to return error - // - Updated TrackUndelegation(amount sdk.Coins) to return error - // - Added error handling here instead of letting panics occur - // - This was a breaking change that required updating all vesting account implementationsAlso applies to: 466-474
x/auth/vesting/types/vesting_account.go (2)
60-74: Use typed SDK errors for consistency and clearer upstream handling; fix comment grammar.Return sdkerrors (wrapped) instead of fmt.Errorf and tweak the comment wording.
-func (bva *BaseVestingAccount) TrackDelegation(balance, vestingCoins, amount sdk.Coins) error { +func (bva *BaseVestingAccount) TrackDelegation(balance, vestingCoins, amount sdk.Coins) error { for _, coin := range amount { baseAmt := balance.AmountOf(coin.Denom) vestingAmt := vestingCoins.AmountOf(coin.Denom) delVestingAmt := bva.DelegatedVesting.AmountOf(coin.Denom) - // Return error if the delegation amount is zero or if the base coins does not - // exceed the desired delegation amount. - if coin.Amount.IsZero() { - return fmt.Errorf("delegation attempt with zero coins") - } - if baseAmt.LT(coin.Amount) { - return fmt.Errorf("delegation attempt with insufficient funds: balance %s, requested %s", baseAmt, coin.Amount) - } + // Return error if the delegation amount is non-positive or if the base balance is insufficient. + if coin.Amount.IsZero() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, "delegation amount must be > 0") + } + if baseAmt.LT(coin.Amount) { + return errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, "balance %s, requested %s", baseAmt, coin.Amount) + }Additional imports required at the top of this file:
import ( // ... errorsmod "cosmossdk.io/errors" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" )
103-109: Typed error for undelegation zero-amount case.Match the delegation path and SDK error conventions.
- if coin.Amount.IsZero() { - return fmt.Errorf("undelegation attempt with zero coins") - } + if coin.Amount.IsZero() { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, "undelegation amount must be > 0") + }x/auth/vesting/types/vesting_account_test.go (1)
159-166: Tests updated to assert errors: LGTM; consider asserting NoError everywhere the new API returns error.Most sites now use require.NoError/require.Error—good. There are still a few calls that ignore the error (e.g., Lines 145–148, 152–155, 565–576). For consistency with the new contract, assert errors at those sites too.
Example patch for one site:
- cva.TrackDelegation(now, origCoins, origCoins) + err := cva.TrackDelegation(now, origCoins, origCoins) + require.NoError(t, err)If you want, I can sweep the file and send a patch normalizing all TrackDelegation/TrackUndelegation calls.
Also applies to: 164-168, 172-176, 187-201, 197-201, 207-211, 215-229, 221-231, 313-324, 330-334, 345-359, 365-369, 373-381, 386-390, 582-593, 600-609, 613-617, 633-647, 653-667, 671-675, 677-687, 743-747, 758-768, 766-770, 774-778, 789-793, 799-805, 809-813, 817-821, 823-833, 830-834
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
x/auth/vesting/exported/exported.go(1 hunks)x/auth/vesting/types/vesting_account.go(8 hunks)x/auth/vesting/types/vesting_account_test.go(9 hunks)x/bank/keeper/keeper.go(2 hunks)x/bank/types/vesting.go(1 hunks)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/auth/vesting/types/vesting_account.go (1)
243-245: Wrappers delegating to BaseVestingAccount look good.Thin adapters returning error are correct and keep behavior centralized.
Also applies to: 348-350, 447-449, 503-505
… return errors instead of panicking - Update TrackDelegation and TrackUndelegation methods to return errors - Modify all vesting account implementations accordingly - Add proper error handling in bank keeper - Update tests to expect errors instead of panics Signed-off-by: Hwangjae Lee <[email protected]>
meetrick
force-pushed
the
20250831_vesting
branch
from
529f9cf to
0c07186
Compare
Description
Closes: #25144
Summary by CodeRabbit
Bug Fixes
Refactor