Deprecate IBC write handlers - #3947
Conversation
Now that IBC in/outbound is disabled as part of SIP-3: - reject all IBC client, connection, and channel messages - reject IBC transfer messages - reject IBC client governance proposals - return stable module deprecation errors - add coverage for every deprecated write handler
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3947 +/- ##
==========================================
- Coverage 59.60% 58.62% -0.98%
==========================================
Files 2331 2233 -98
Lines 200124 188194 -11930
==========================================
- Hits 119277 110329 -8948
+ Misses 69413 67401 -2012
+ Partials 11434 10464 -970
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview Removes governance and CLI paths for IBC client update/upgrade proposals from the main and wasmd apps, and deletes the associated client proposal handlers, keeper methods ( Cleans up tests by dropping the wasmd Reviewed by Cursor Bugbot for commit 8d9d9b5. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped deprecation of the IBC write paths with good test coverage of the rejecting handlers, but two issues need attention: dropping the ibc gov route can panic the gov EndBlocker on an in-flight legacy IBC client proposal, and rejecting Timeout/TimeoutOnClose/Acknowledgement (which base deliberately left ungated) makes any outstanding packet commitment unresolvable and its escrow unrecoverable.
Findings: 2 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Deleting
sei-wasmd/x/wasm/ibctesting/*,ibc_reflect_test.go,relay_test.go, andrelay_pingpong_test.goremoves all integration coverage of the wasm IBC application callbacks insei-wasmd/x/wasm/ibc.go, which remain in the tree. The new tests only assert the deprecation error, so nothing exercises those callbacks anymore. Worth a note in the PR description that this coverage is intentionally retired along with IBC. - The tx CLI still advertises commands that can now only produce rejected transactions:
sei-ibc-go/modules/core/02-client/client/cli/tx.gokeepscreate/update/misbehaviour/upgrade, and the transfer module keeps itstransfercommand. Since the gov-proposal CLI commands were removed in this PR, removing (or marking deprecated inShort) these too would keep the surface consistent. sei-ibc-go/modules/core/keeper/params.gostill exposesGetParams/SetParamsovertypes.Params{InboundEnabled, OutboundEnabled}, but the core keeper no longer reads either field (only the 03-connection and 04-channel keepers, which read their own copies from the shared subspace). Not a defect, just dead surface on the core keeper now.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)). | ||
| AddRoute(distrtypes.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.DistrKeeper)). | ||
| AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper)). | ||
| AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper)). |
There was a problem hiding this comment.
[blocker] Dropping the ibc route entirely can halt the chain on an in-flight proposal.
New submissions are safe — sei-cosmos/x/gov/keeper/proposal.go:20 rejects with ErrNoProposalHandlerExists when HasRoute is false — but a ClientUpdateProposal/UpgradeProposal already sitting in the active queue at the upgrade height is not. sei-cosmos/x/gov/abci.go:74 calls keeper.Router().GetRoute(proposal.ProposalRoute()) for any proposal that passes, and sei-cosmos/x/gov/types/router.go:67 panics on an unregistered path. That panic is in the EndBlocker, outside the cache context and outside any recover, so it halts block processing rather than failing the proposal.
Cheap fix: keep the route registered with a handler that returns coretypes.ErrIBCDeprecated, so a passing legacy proposal is recorded as StatusFailed instead of panicking. Same change needed in sei-wasmd/app/app.go:399, and worth a test that executing a legacy IBC proposal through the router yields the deprecation error.
|
|
||
| return &channeltypes.MsgTimeoutResponse{Result: channeltypes.SUCCESS}, nil | ||
| // Timeout defines an RPC handler for MsgTimeout. | ||
| func (Keeper) Timeout(context.Context, *channeltypes.MsgTimeout) (*channeltypes.MsgTimeoutResponse, error) { |
There was a problem hiding this comment.
[blocker] Timeout, TimeoutOnClose, and Acknowledgement were the three write handlers base deliberately left ungated while everything else was disabled: RecvPacket checked IsInboundEnabled, ConnectionOpenInit/ChannelOpenInit checked IsOutboundEnabled, and at the channel-keeper layer only SendPacket (04-channel/keeper/packet.go:33) and RecvPacket (:172) are gated — AcknowledgePacket and TimeoutPacket/TimeoutOnClose have no gate at all. That carve-out is what lets packets committed before outbound was disabled still be acknowledged or timed out.
Rejecting them here closes that path permanently: any outstanding packet commitment can never reach TimeoutExecuted, so transfer's refundPacketToken never runs and the escrowed tokens for those packets are locked forever, with stale commitments left in state.
Please either confirm on-chain that there are no unresolved packet commitments / non-zero escrow balances left (and say so in the PR description), or keep these three handlers executable — or land an escrow-refund migration alongside this change.
| types.RegisterMsgServer(cfg.MsgServer(), am.keeper) | ||
| // Transactions are rejected as deprecated; Keeper.Transfer stays executable | ||
| // for the versioned EVM precompiles that replay historical blocks. | ||
| types.RegisterMsgServer(cfg.MsgServer(), keeper.DeprecatedMsgServer{}) |
There was a problem hiding this comment.
[suggestion] The comment says Keeper.Transfer stays executable only for versioned precompiles replaying historical blocks, but the current precompile also calls it directly: precompiles/ibc/ibc.go:169 and :255 invoke p.transferKeeper.Transfer(...), bypassing the msg-service router and therefore DeprecatedMsgServer. So a live EVM IBC.transfer call still attempts a real transfer and fails deeper down with ibc-channel: ibc outbound disabled rather than ErrTransferDeprecated.
Given the PR's goal of a stable deprecation error for transfer writes, consider gating the live precompile too (and adjusting this comment, which currently reads as if only legacy versions reach the keeper).
3 participants
Now that IBC in/outbound is disabled as part of SIP-3: