feat: zone (omniflix) offboarding logic#2018
Conversation
- Add MsgGovSetZoneOffboarding to enable/disable offboarding mode - Add MsgGovCancelAllPendingRedemptions to refund pending redemptions - Add MsgGovForceUnbondAllDelegations to force unbond all delegations - Skip redemption rate updates for offboarding zones - Add is_offboarding field to Zone proto - Add upgrade handler for v1.10.0 - Add ADR-003 documentation Closes: zone offboarding for Omniflix
📝 WalkthroughWalkthroughAdds a zone offboarding feature: new governance messages and proto types, zone is_offboarding flag, gov handlers to toggle offboarding/cancel redemptions/force unbond, ICA-driven offboarding undelegate flows, new upgrade entry for v1.10.0, tests, ADR, and related codec/event/ibc memo additions. Changes
Sequence Diagram(s)sequenceDiagram
actor Gov
participant Keeper as InterchainKeeper
participant ICA as ICA (Tx Submission)
participant HostChain as HostZone (Validators/Stake)
participant IBCHandler as IBC Ack Handler
Note over Gov,Keeper: Gov proposal execution
Gov->>Keeper: GovForceUnbondAllDelegations(chain_id)
Keeper->>Keeper: validate authority, ensure zone.is_offboarding
Keeper->>ICA: submit undelegate ICA txs (offboarding memo)
ICA->>HostChain: broadcast undelegate txs to validators
HostChain-->>ICA: ack/tx hash / completion info
ICA->>IBCHandler: relay ack packet (undelegate ack)
IBCHandler->>IBCHandler: detect offboarding memo -> HandleOffboardingUndelegate
IBCHandler->>Keeper: decrement waitgroup, update delegation state via ICQ, emit OffboardingUnbondAck event
Keeper->>Gov: return unbonding_count, total_unbonded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
ajansari95
changed the title
![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
🧹 Nitpick comments (5)
app/upgrades/v1_10.go (1)
11-30: Upgrade handler is standard and safe; minor logging nitThe handler pattern (log → optional work →
mm.RunMigrations) matches other upgrades and is safe given there are no state migrations for the new offboarding functionality.If you care about log precision when the same handler is used for
"v1.10.0-rc.0"and"v1.10.0", you might consider logging the actual plan name or a more generic label (e.g., “v1.10.x upgrade”) instead of hard‑coding"v1.10.0".architecture/adr-003-zone-offboarding.md (1)
26-89: Add a language to the fenced block for the flow diagram
markdownlint(MD040) is complaining about the fenced block that contains the ASCII “ZONE OFFBOARDING FLOW” diagram because it has no language specifier. Consider declaring it explicitly, e.g.:-``` +```text ┌─────────────────────────────────────────────────────────────────────────────┐ │ ZONE OFFBOARDING FLOW │ ... -``` +```This should clear the linter without changing the rendered output.
x/interchainstaking/keeper/ibc_packet_handlers.go (1)
891-949: Consider adding delegation record update before ICQ.The function decrements the waitgroup, then issues an ICQ query and increments the waitgroup, but unlike
HandleUndelegate, it doesn't update the local delegation record synchronously. While the ICQ callback will eventually update the delegation, there may be a window where the local state is inconsistent.Additionally, ensure the zone state is correctly persisted. The zone is passed by pointer, and
SetZoneis called at line 936, which should persist the waitgroup changes.Consider adding local delegation update for consistency:
+ // Update local delegation record optimistically + delegation, found := k.GetDelegation(ctx, zone.ChainId, undelegateMsg.DelegatorAddress, undelegateMsg.ValidatorAddress) + if found { + delegation.Amount = delegation.Amount.Sub(undelegateMsg.Amount) + if delegation.Amount.IsPositive() { + k.SetDelegation(ctx, zone.ChainId, delegation) + } else { + k.DeleteDelegation(ctx, zone.ChainId, delegation) + } + } + // Update the delegation record via ICQ delAddr, err := addressutils.AccAddressFromBech32(undelegateMsg.DelegatorAddress, "")x/interchainstaking/keeper/msg_server_test.go (1)
1829-1871: Consider adding a test case for cancellation with both QUEUED and UNBOND records.The handler iterates both
WithdrawStatusQueuedandWithdrawStatusUnbondrecords. The current "with records" test only creates aWithdrawStatusQueuedrecord. Consider adding a test that verifies both types are cancelled.x/interchainstaking/keeper/msg_server.go (1)
562-568: Consider documenting behavior when disabling offboarding.When
IsOffboardingis set tofalse, the zone state is updated butDepositsEnabledandUnbondingEnabledare not restored. This appears intentional (manual re-enablement via separate governance actions), but documenting this asymmetry would help operators understand the workflow.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.work.sumis excluded by!**/*.sumx/interchainstaking/types/interchainstaking.pb.gois excluded by!**/*.pb.gox/interchainstaking/types/messages.pb.gois excluded by!**/*.pb.gox/interchainstaking/types/messages.pb.gw.gois excluded by!**/*.pb.gw.gox/interchainstaking/types/proposals.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
app/upgrades/types.go(1 hunks)app/upgrades/upgrades.go(1 hunks)app/upgrades/v1_10.go(1 hunks)architecture/adr-003-zone-offboarding.md(1 hunks)proto/quicksilver/interchainstaking/v1/interchainstaking.proto(1 hunks)proto/quicksilver/interchainstaking/v1/messages.proto(1 hunks)proto/quicksilver/interchainstaking/v1/proposals.proto(2 hunks)x/interchainstaking/keeper/ibc_packet_handlers.go(8 hunks)x/interchainstaking/keeper/ibc_packet_handlers_test.go(10 hunks)x/interchainstaking/keeper/msg_server.go(2 hunks)x/interchainstaking/keeper/msg_server_test.go(1 hunks)x/interchainstaking/types/codec.go(2 hunks)x/interchainstaking/types/events.go(2 hunks)x/interchainstaking/types/ibc_packet.go(2 hunks)x/interchainstaking/types/msgs.go(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1620
File: x/participationrewards/keeper/hooks.go:153-176
Timestamp: 2024-07-29T07:24:15.377Z
Learning: When computing IBC denoms, use `channel.PortId` and `counterpartychannel.PortId` instead of hardcoded strings for better accuracy.
Applied to files:
x/interchainstaking/types/ibc_packet.go
📚 Learning: 2025-01-28T19:03:57.668Z
Learnt from: arhamchordia
Repo: quicksilver-zone/quicksilver PR: 1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
Applied to files:
x/interchainstaking/keeper/ibc_packet_handlers_test.gox/interchainstaking/keeper/msg_server_test.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1447-1447
Timestamp: 2024-07-29T07:24:15.377Z
Learning: The user has clarified that in the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/ibc_packet_handlers_test.gox/interchainstaking/keeper/ibc_packet_handlers.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1437-1437
Timestamp: 2024-10-15T13:03:53.675Z
Learning: In the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/ibc_packet_handlers_test.gox/interchainstaking/keeper/ibc_packet_handlers.go
📚 Learning: 2024-12-03T11:18:49.168Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1767
File: app/upgrades/v1_7.go:141-161
Timestamp: 2024-12-03T11:18:49.168Z
Learning: In the `V010705UpgradeHandler` function in `app/upgrades/v1_7.go`, the `OverrideRedemptionRateNoCap` method computes and sets `zone.RedemptionRate` at runtime, making it unnecessary to set it explicitly.
Applied to files:
app/upgrades/v1_10.gox/interchainstaking/keeper/ibc_packet_handlers.goapp/upgrades/upgrades.goapp/upgrades/types.go
📚 Learning: 2024-11-19T14:54:21.046Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1747
File: app/upgrades/v1_7.go:44-49
Timestamp: 2024-11-19T14:54:21.046Z
Learning: In the Quicksilver codebase, panicking in an upgrade handler (e.g., using `panic(err)` within `V010702UpgradeHandler` in `app/upgrades/v1_7.go`) is acceptable to prevent the state from being committed when an error occurs during an upgrade.
Applied to files:
app/upgrades/v1_10.goapp/upgrades/upgrades.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1132
File: x/interchainstaking/keeper/callbacks.go:662-664
Timestamp: 2024-07-29T07:24:15.377Z
Learning: The `DecrementWithdrawalWaitgroup` method in `x/interchainstaking/keeper/callbacks.go` includes underflow protection, ensuring safe decrement operations.
Applied to files:
x/interchainstaking/keeper/ibc_packet_handlers.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1132
File: x/interchainstaking/keeper/callbacks.go:127-127
Timestamp: 2024-10-15T13:03:53.675Z
Learning: The `DecrementWithdrawalWaitgroup` method in `x/interchainstaking/keeper/callbacks.go` internally handles underflow protection, ensuring that the waitgroup count does not become negative.
Applied to files:
x/interchainstaking/keeper/ibc_packet_handlers.go
📚 Learning: 2025-04-04T13:21:50.558Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1833
File: proto/osmosis/gamm/poolmodels/balancer/v1beta1/tx.proto:4-6
Timestamp: 2025-04-04T13:21:50.558Z
Learning: In the Quicksilver project, some proto files like "amino/amino.proto" are not directly included in the repository but are imported during compilation through buf's dependency resolution mechanism, which pulls them from external dependencies specified in the buf.yaml configuration.
Applied to files:
proto/quicksilver/interchainstaking/v1/proposals.proto
📚 Learning: 2025-04-04T13:22:01.808Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1833
File: proto/osmosis/lockup/tx.proto:4-4
Timestamp: 2025-04-04T13:22:01.808Z
Learning: In Cosmos SDK projects using Protocol Buffers, imports like "amino/amino.proto" may be provided through buf's dependency resolution system rather than existing as local files. Static analysis tools may incorrectly flag these as errors, but they are valid imports that will be resolved during compilation.
Applied to files:
proto/quicksilver/interchainstaking/v1/proposals.proto
🧬 Code graph analysis (6)
x/interchainstaking/keeper/ibc_packet_handlers_test.go (3)
utils/randomutils/random.go (1)
GenerateRandomHashAsHex(23-25)x/interchainstaking/types/interchainstaking.pb.go (6)
UnbondingRecord(653-660)UnbondingRecord(664-664)UnbondingRecord(665-667)Validator(816-830)Validator(834-834)Validator(835-837)x/interchainstaking/types/ibc_packet.go (1)
EpochWithdrawalMemo(57-59)
x/interchainstaking/keeper/ibc_packet_handlers.go (8)
x/interchainstaking/types/withdrawal_record.go (3)
WithdrawStatusSend(16-16)WithdrawStatusTokenize(13-13)WithdrawStatusUnbond(15-15)x/interchainstaking/types/ibc_packet.go (3)
ParseEpochMsgMemo(26-38)MsgTypeOffboardingUnbond(16-16)MsgTypeWithdrawal(11-11)x/interchainstaking/keeper/keeper.go (2)
Keeper(45-62)Keeper(138-140)x/interchainstaking/keeper/intent.go (1)
Keeper(20-25)x/interchainstaking/types/interchainstaking.pb.go (3)
Zone(34-67)Zone(71-71)Zone(72-74)x/interchainstaking/types/zones.go (3)
Zone(110-112)Zone(114-135)Zone(205-221)utils/addressutils/address.go (2)
AccAddressFromBech32(45-51)ValAddressFromBech32(67-73)x/interchainstaking/types/events.go (1)
EventTypeOffboardingUnbondAck(18-18)
app/upgrades/upgrades.go (2)
app/upgrades/types.go (2)
V0101000rc0UpgradeName(33-33)V0101000UpgradeName(34-34)app/upgrades/v1_10.go (1)
V0101000UpgradeHandler(16-31)
x/interchainstaking/keeper/msg_server_test.go (2)
x/interchainstaking/types/proposals.pb.go (15)
MsgGovSetZoneOffboarding(658-662)MsgGovSetZoneOffboarding(666-666)MsgGovSetZoneOffboarding(667-669)MsgGovCancelAllPendingRedemptions(735-738)MsgGovCancelAllPendingRedemptions(742-742)MsgGovCancelAllPendingRedemptions(743-745)MsgGovCancelAllPendingRedemptionsResponse(773-776)MsgGovCancelAllPendingRedemptionsResponse(784-784)MsgGovCancelAllPendingRedemptionsResponse(785-787)MsgGovForceUnbondAllDelegations(831-834)MsgGovForceUnbondAllDelegations(838-838)MsgGovForceUnbondAllDelegations(839-841)MsgGovForceUnbondAllDelegationsResponse(869-872)MsgGovForceUnbondAllDelegationsResponse(878-878)MsgGovForceUnbondAllDelegationsResponse(879-881)x/interchainstaking/keeper/msg_server.go (1)
NewMsgServerImpl(27-29)
x/interchainstaking/types/msgs.go (1)
x/interchainstaking/types/proposals.pb.go (9)
MsgGovSetZoneOffboarding(658-662)MsgGovSetZoneOffboarding(666-666)MsgGovSetZoneOffboarding(667-669)MsgGovCancelAllPendingRedemptions(735-738)MsgGovCancelAllPendingRedemptions(742-742)MsgGovCancelAllPendingRedemptions(743-745)MsgGovForceUnbondAllDelegations(831-834)MsgGovForceUnbondAllDelegations(838-838)MsgGovForceUnbondAllDelegations(839-841)
x/interchainstaking/types/codec.go (1)
x/interchainstaking/types/proposals.pb.go (9)
MsgGovSetZoneOffboarding(658-662)MsgGovSetZoneOffboarding(666-666)MsgGovSetZoneOffboarding(667-669)MsgGovCancelAllPendingRedemptions(735-738)MsgGovCancelAllPendingRedemptions(742-742)MsgGovCancelAllPendingRedemptions(743-745)MsgGovForceUnbondAllDelegations(831-834)MsgGovForceUnbondAllDelegations(838-838)MsgGovForceUnbondAllDelegations(839-841)
🪛 Buf (1.61.0)
proto/quicksilver/interchainstaking/v1/proposals.proto
4-4: import "cosmos/base/v1beta1/coin.proto": file does not exist
(COMPILE)
🪛 markdownlint-cli2 (0.18.1)
architecture/adr-003-zone-offboarding.md
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
- GitHub Check: lint
🔇 Additional comments (22)
proto/quicksilver/interchainstaking/v1/interchainstaking.proto (1)
59-59: Zone.is_offboarding field wiring looks correctAdding a new
bool is_offboarding = 32;with a fresh field number is backward‑compatible in proto3 and matches the ADR semantics. No issues from a schema/upgrade perspective.x/interchainstaking/types/ibc_packet.go (1)
11-16: Offboarding memo type is consistent with existing memo patterns
MsgTypeOffboardingUnbond = "offboard"andOffboardingUnbondMemofollow the existingmsgType/valuememo convention (rebalance,withdrawal,unbondSend), so they should integrate cleanly with the existing parsing helpers and IBC ack handling.Also applies to: 65-67
app/upgrades/types.go (1)
33-34: v1.10.0 upgrade name constants are wired correctlyThe new
V0101000rc0UpgradeName/V0101000UpgradeNameconstants cleanly extend the existing upgrade-name set and match the plan names used inupgrades.go. No behavioral concerns.app/upgrades/upgrades.go (1)
27-29: Upgrade entries for v1.10.0 are correctly registeredRegistering both
"v1.10.0-rc.0"and"v1.10.0"withV0101000UpgradeHandleris consistent with the existing pattern and will correctly route both plans through the same upgrade logic.x/interchainstaking/types/codec.go (1)
28-30: Codec registrations for new governance messages are completeThe three new governance messages are properly registered in both the legacy Amino codec and the interface registry as
sdk.Msgimplementations, which is necessary for gov‑v1 Msg proposals to work. No additionalgovv1beta1.Contentregistrations are needed since these are Msg‑style, not legacy content‑style proposals.Also applies to: 48-50
x/interchainstaking/keeper/ibc_packet_handlers_test.go (2)
585-603: Updated tests correctly reflect non-fatal missing-record semanticsRenaming cases and flipping expectations to
err == falsefor “no matching record / returns nil” paths in:
TestHandleWithdrawForUser(“no matching record - returns nil”),TestHandleFailedUnbondSend(“no matching record - returns nil”, “second returns nil”),TestHandleCompleteSend(“From DelegationAddress - missing record returns nil”),TestHandleFailedBankSend(“no matching record - returns nil”),accurately encode the new behavior where missing withdrawal records are treated as benign and simply skipped. Assertions still ensure that no unintended record status transitions occur.
This aligns with the ADR’s goal of making offboarding and replay scenarios more robust without surfacing spurious errors.
Also applies to: 1619-1637, 1641-1669, 4400-4410, 4513-4537
5120-5285: *New _MissingRecord tests give good coverage of nil-return pathsThe added tests:
TestHandleWithdrawForUser_MissingRecordTestHandleTokenizedShares_MissingRecordTestHandleUndelegate_MissingWithdrawalRecordTestHandleFailedUnbondSend_MissingRecordTestHandleFailedUndelegate_MissingWithdrawalRecordall cleanly exercise the “record not found → no error” branches, and where appropriate they also assert that unbonding records are still updated/cleaned up as intended. The setups are minimal and focused on the keeper behavior, not on msg_server logic, which fits prior guidance to avoid duplicating msg_server tests. Based on learnings, this separation between missing-record handling and real failure/requeue semantics looks solid.
proto/quicksilver/interchainstaking/v1/messages.proto (1)
100-123: LGTM!The three new governance RPC methods follow the existing patterns in the file. HTTP annotations are consistent with the naming conventions used for other governance endpoints.
x/interchainstaking/types/events.go (2)
15-18: LGTM!The new event type constants for the offboarding workflow are well-named and follow existing conventions.
40-47: LGTM!The new attribute keys comprehensively cover the offboarding workflow telemetry needs and are consistent with existing attribute naming patterns.
x/interchainstaking/keeper/ibc_packet_handlers.go (3)
461-464: Verify this behavioral change is intentional.Changing from returning an error to returning
nilwhen a withdrawal record is not found makes this a non-fatal condition. This could mask issues where records are unexpectedly missing. The log message indicates this is expected ("may have already been processed"), but ensure this doesn't silently drop legitimate error cases.
801-811: LGTM - Offboarding memo check precedes standard withdrawal flow.The logic correctly routes offboarding unbonds to the dedicated handler while maintaining the existing withdrawal flow for standard unbonds.
1542-1547: LGTM - Redemption rate updates correctly skipped for offboarding zones.This prevents the redemption rate from being updated during offboarding, which aligns with the PR's intent to freeze the rate while winding down the zone.
x/interchainstaking/keeper/msg_server_test.go (2)
1666-1763: LGTM - Comprehensive test coverage for GovSetZoneOffboarding.The test covers key scenarios including invalid authority, zone not found, enabling offboarding, and disabling offboarding. The post-checks verify the expected zone state changes (IsOffboarding, DepositsEnabled, UnbondingEnabled).
1957-1989: Good use of TxKeeper mock to verify ICA transaction submission.The test properly verifies that the ICA transaction was submitted by checking
len(txk.Txs) == 1and validates the response fields.x/interchainstaking/keeper/msg_server.go (3)
712-730: LGTM - Delegation aggregation and message construction.The logic correctly skips zero-amount delegations and aggregates the total unbonded amount. The use of
sdk.ZeroInt()for initialization is appropriate.
745-748: The code is safe. TheIncrementWithdrawalWaitgroupmethod includes built-in overflow protection that detects when the uint32 counter would exceed its maximum value and returns an error, which is properly checked at the call site. No additional bounds checking is needed.Likely an incorrect or invalid review comment.
740-742: Batching implementation is correct.The
SubmitTxfunction properly batches messages usingzone.MessagesPerTx(default 5) as the chunk size inProdSubmitTx. Each batch is submitted as a separate ICA transaction viaSendTx, which naturally constrains gas and message size per transaction. The withdrawal waitgroup increment (by total message count) correctly balances with the decrement-by-1 pattern in acknowledgement handlers, ensuring accurate tracking of in-flight messages.proto/quicksilver/interchainstaking/v1/proposals.proto (2)
4-4: Static analysis false positive: import resolved via buf.The import is flagged by static analysis but will be resolved during compilation through buf's dependency resolution mechanism specified in buf.yaml. This is standard for Cosmos SDK proto dependencies.
Based on learnings, this is a known pattern in Cosmos SDK projects.
165-211: LGTM! Well-structured governance message definitions.The three new governance messages follow Cosmos SDK conventions:
- Proper field types and numbering
- Appropriate gogoproto annotations for SDK type generation
- Clear documentation comments describing purpose and constraints
- Response types correctly use
nullable=falseto prevent nil pointer issuesThe definitions align well with the multi-step offboarding flow described in the PR objectives.
x/interchainstaking/types/msgs.go (2)
35-37: LGTM! Proper constant definitions and interface assertions.The new message type constants follow the established naming convention, and interface assertions ensure compile-time verification that the messages correctly implement both
sdk.Msgandlegacytx.LegacyMsginterfaces.Also applies to: 51-53, 66-68
715-807: LGTM! Implementations follow established patterns.All three governance message implementations correctly:
- Validate authority address and chain_id constraints in
ValidateBasic(consistent with existing governance messages likeMsgGovSetLsmCaps)- Use standard Cosmos SDK patterns for
GetSignBytes,GetSigners,Route, andType- Implement both
sdk.Msgandlegacytx.LegacyMsginterfacesThe validation logic is appropriately minimal at this layer, with business logic validation (e.g., checking if zone exists, verifying offboarding state) properly deferred to the keeper/handler layer.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2018 +/- ##
==========================================
- Coverage 58.81% 58.70% -0.11%
==========================================
Files 171 172 +1
Lines 12251 12509 +258
==========================================
+ Hits 7205 7343 +138
- Misses 4275 4388 +113
- Partials 771 778 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ire zone to be offboarded to cancel withdrawals
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @x/interchainstaking/keeper/msg_server.go:
- Around line 625-629: The code risks a panic by indexing amountsToRefund[0] and
only checks the first denom; update the logic in the cancel withdrawal flow to
first handle an empty amountsToRefund slice (return a clear error or no-op) and
then iterate over all coins in amountsToRefund, calling
k.BankKeeper.GetBalance(ctx,
k.AccountKeeper.GetModuleAddress(types.EscrowModuleAccount), coin.Denom) and
comparing with coin using IsLT; for any insufficient balance return a formatted
error that includes the specific denom and amounts (use escrowBalance.String()
and coin.String()) so every denomination is validated rather than only the
first.
- Line 717: Replace the deprecated sdk.ZeroInt() call used when initializing
totalUnbonded in msg_server.go with the math package used elsewhere: import
cosmossdk.io/math as sdkmath (match existing alias pattern) and change
totalUnbonded := sdk.NewCoin(zone.BaseDenom, sdk.ZeroInt()) to use
sdkmath.ZeroInt() so it becomes sdk.NewCoin(zone.BaseDenom, sdkmath.ZeroInt()).
🧹 Nitpick comments (2)
x/interchainstaking/keeper/msg_server.go (2)
632-643: All-or-nothing cancellation semantics.The function now fails immediately on address parsing or transfer errors, ensuring all-or-nothing semantics (either all records are cancelled and refunded, or the entire operation is rolled back). This is consistent with the commit message's intent to "be defensive around refunds; barf if there are errors."
While this prevents partial state, consider adding a comment to document this behavior explicitly, as operators may expect progress even if some records fail.
749-749: Optional: Add overflow protection for message count.The
nolint:goseccomment indicates awareness of the potential overflow when castinglen(msgs)touint32. While unlikely in practice (would require over 4 billion delegations), consider adding an explicit check for defensive programming.🔎 Proposed overflow check
+ // Ensure message count fits in uint32 + if len(msgs) > 0xFFFFFFFF { + return nil, fmt.Errorf("too many unbonding messages: %d exceeds uint32 max", len(msgs)) + } + // Increment withdrawal waitgroup for the unbonding messages - if err := zone.IncrementWithdrawalWaitgroup(k.Logger(ctx), uint32(len(msgs)), "offboarding unbond messages"); err != nil { //nolint:gosec + if err := zone.IncrementWithdrawalWaitgroup(k.Logger(ctx), uint32(len(msgs)), "offboarding unbond messages"); err != nil { return nil, err }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/interchainstaking/keeper/msg_server.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-03T11:18:49.168Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1767
File: app/upgrades/v1_7.go:141-161
Timestamp: 2024-12-03T11:18:49.168Z
Learning: In the `V010705UpgradeHandler` function in `app/upgrades/v1_7.go`, the `OverrideRedemptionRateNoCap` method computes and sets `zone.RedemptionRate` at runtime, making it unnecessary to set it explicitly.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1447-1447
Timestamp: 2024-10-15T13:03:53.675Z
Learning: The user has clarified that in the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1437-1437
Timestamp: 2024-07-29T07:24:15.377Z
Learning: In the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/msg_server.go
⏰ 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: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
- GitHub Check: lint
🔇 Additional comments (2)
x/interchainstaking/keeper/msg_server.go (2)
15-15: LGTM!The
stakingtypesimport is correctly added to support theMsgUndelegatemessage creation in theGovForceUnbondAllDelegationsfunction.
562-568: Verify intended behavior when disabling offboarding.When
IsOffboardingis set tofalse, the code does not re-enableDepositsEnabledorUnbondingEnabled. The test case for disabling offboarding (lines 1701-1738 in msg_server_test.go) only verifies thatIsOffboardingbecomesfalse, but does not assert the expected state of these flags. Confirm whether deposits and unbonding should remain disabled until explicitly re-enabled via separate governance actions, or whether they should be restored when offboarding is disabled. If the former is intended, update the function comment and test postCheck to document this behavior explicitly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @x/interchainstaking/keeper/msg_server.go:
- Around line 626-631: The escrow balance check currently only verifies the
first coin in amountsToRefund; update the logic in the message handler (where
amountsToRefund is used) to iterate over every coin in amountsToRefund and for
each coin call k.BankKeeper.GetBalance(ctx,
k.AccountKeeper.GetModuleAddress(types.EscrowModuleAccount), coin.Denom) and
ensure escrowBalance.IsLT(coin) is handled (returning the same formatted error
including escrowBalance and coin) before performing any refunds so all
denominations are defensively validated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/interchainstaking/keeper/msg_server.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1767
File: app/upgrades/v1_7.go:141-161
Timestamp: 2024-12-03T11:18:49.168Z
Learning: In the `V010705UpgradeHandler` function in `app/upgrades/v1_7.go`, the `OverrideRedemptionRateNoCap` method computes and sets `zone.RedemptionRate` at runtime, making it unnecessary to set it explicitly.
📚 Learning: 2024-12-03T11:18:49.168Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1767
File: app/upgrades/v1_7.go:141-161
Timestamp: 2024-12-03T11:18:49.168Z
Learning: In the `V010705UpgradeHandler` function in `app/upgrades/v1_7.go`, the `OverrideRedemptionRateNoCap` method computes and sets `zone.RedemptionRate` at runtime, making it unnecessary to set it explicitly.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1447-1447
Timestamp: 2024-10-15T13:03:53.675Z
Learning: The user has clarified that in the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1099
File: x/interchainstaking/keeper/ibc_packet_handlers_test.go:1437-1437
Timestamp: 2024-07-29T07:24:15.377Z
Learning: In the context of testing failure cases for unbondings, it is expected that withdrawal records will be marked as `requeued`.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1132
File: x/interchainstaking/keeper/callbacks.go:662-664
Timestamp: 2024-07-29T07:24:15.377Z
Learning: The `DecrementWithdrawalWaitgroup` method in `x/interchainstaking/keeper/callbacks.go` includes underflow protection, ensuring safe decrement operations.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2025-01-28T19:03:57.668Z
Learnt from: arhamchordia
Repo: quicksilver-zone/quicksilver PR: 1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1132
File: x/interchainstaking/keeper/callbacks.go:625-625
Timestamp: 2024-10-15T13:03:53.675Z
Learning: The `DecrementWithdrawalWaitgroup` method in `x/interchainstaking/keeper/callbacks.go` internally handles underflow protection, ensuring that the waitgroup count does not become negative.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-10-15T13:03:53.675Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1183
File: x/interchainstaking/keeper/ibc_packet_handlers.go:713-717
Timestamp: 2024-10-15T13:03:53.675Z
Learning: The error handling strategy of logging errors without halting execution or propagating them further in the context of decrementing the withdrawal waitgroup is intentional and considered appropriate by the developers.
Applied to files:
x/interchainstaking/keeper/msg_server.go
📚 Learning: 2024-07-29T07:24:15.377Z
Learnt from: joe-bowman
Repo: quicksilver-zone/quicksilver PR: 1620
File: x/participationrewards/keeper/hooks.go:153-176
Timestamp: 2024-07-29T07:24:15.377Z
Learning: When computing IBC denoms, use `channel.PortId` and `counterpartychannel.PortId` instead of hardcoded strings for better accuracy.
Applied to files:
x/interchainstaking/keeper/msg_server.go
⏰ 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: lint
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: Analyze
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
🔇 Additional comments (6)
x/interchainstaking/keeper/msg_server.go (6)
13-13: LGTM! Appropriate imports for offboarding functionality.The
cosmossdk.io/mathimport is correctly used formath.ZeroInt()on line 720, andstakingtypesis used forMsgUndelegateon line 729. Both are necessary for the new governance handlers.Also applies to: 16-16
563-569: Verify: Should disabling offboarding re-enable deposits and unbonding?When
msg.IsOffboardingis set tofalse(disabling offboarding), the code does not restorezone.DepositsEnabledorzone.UnbondingEnabled. Currently, these flags remain in their previous state. If the intended behavior is to automatically re-enable these features when offboarding is disabled, add anelsebranch:if msg.IsOffboarding { zone.DepositsEnabled = false zone.UnbondingEnabled = false } else { zone.DepositsEnabled = true zone.UnbondingEnabled = true }If the current behavior (requiring separate governance actions to re-enable) is intentional, this is fine as-is.
634-645: Error handling prevents partial state correctly.The handler returns errors immediately when address parsing (line 638) or coin transfer (line 644) fails, which prevents partial cancellations and ensures transactional integrity. This is the correct approach for governance operations that should be atomic.
752-755: LGTM! Waitgroup increment follows established patterns.The waitgroup increment correctly accounts for the number of unbonding messages submitted via ICA. The
gosecsuppression is appropriate sincelen(msgs)is bounded by the number of delegations (which should be reasonable for any zone). Error handling ensures that failures are caught before the zone state is saved.
704-717: Excellent validation chain prevents invalid operations.The handler validates all critical preconditions before submitting ICA transactions:
- Zone must be in offboarding mode (line 704)
- Delegation address must exist (line 709)
- Delegations must exist (lines 714-717)
- Non-zero delegations must exist after filtering (lines 739-741)
This defensive approach ensures governance operations fail early with clear error messages rather than submitting invalid ICA transactions.
Also applies to: 739-741
744-749: Offboarding memo handling is correctly implemented in IBC packet handlers.The
HandleUndelegatefunction inibc_packet_handlers.go(lines 801-806) properly detects the offboarding memo pattern usingParseEpochMsgMemo(memo, types.MsgTypeOffboardingUnbond)and correctly routes to the dedicatedHandleOffboardingUndelegatehandler. The handler properly decrements the withdrawal waitgroup and manages state for offboarding unbonds. The memo format matches the constant definition.
2 participants
Implements zone offboarding functionality to safely wind down liquid staking zones (e.g., when a chain is sunsetting like FLIX).
This PR introduces a governance-controlled multi-step process for offboarding zones:
New Governance Messages:
MsgGovSetZoneOffboarding- Enables/disables offboarding mode for a zone. When enabled:zone.IsOffboarding = truezone.DepositsEnabled = false(blocks new deposits)zone.UnbondingEnabled = false(blocks user redemptions)MsgGovCancelAllPendingRedemptions- Cancels all pending redemptions (QUEUED and UNBOND status) and refunds qAssets from escrow to users. Requires zone to be in offboarding mode.MsgGovForceUnbondAllDelegations- Creates MsgUndelegate for all delegations and submits via ICA. Requires zone to be in offboarding mode.Additional Changes:
TriggerRedemptionRate skips redemption rate updates for offboarding zones
HandleUndelegate detects and routes offboarding unbonds to new HandleOffboardingUndelegate handler
New memo type offboard/<block_height> for offboarding unbond ICA transactions
Files Changed:
Proto: Added is_offboarding field to Zone, 3 new message types with responses
Keeper: 3 new msg_server handlers, modified ibc_packet_handlers
Types: New events, attributes, memo functions, codec registration
Upgrades: v1.10.0 upgrade handler
Unit Tests:
go test -v -run "TestKeeperTestSuite/TestGovSetZoneOffboarding" ./x/interchainstaking/keeper/...
go test -v -run "TestKeeperTestSuite/TestGovCancelAllPendingRedemptions" ./x/interchainstaking/keeper/...
go test -v -run "TestKeeperTestSuite/TestGovForceUnbondAllDelegations" ./x/interchainstaking/keeper/...
Offboarding Flow:
See architecture/adr-003-zone-offboarding.md for detailed documentation and example proposals.
Summary by CodeRabbit
New Features
Documentation
Reliability / Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.