Detach applications from retired IBC core - #4014
Conversation
Remove IBC keeper, module, genesis, simulation, params, and begin-block wiring from the production and standalone wasm applications. Delete dormant CosmWasm IBC lifecycle callbacks and state-backed queries, returning deterministic unsupported errors for retired contract IBC operations. Keep the ibc, transfer, capability, and feegrant stores mounted, preserve the retired transfer module account, and retain the ibc/103 raw-store query tombstone. Leave the unreferenced sei-ibc-go tree for a follow-up mechanical deletion.
PR SummaryHigh Risk Overview Retired IBC state stays on disk— CosmWasm IBC is removed end-to-end: the IBC module handler, keeper relay callbacks, channel mocks, IBC query plugins, raw-packet handler, and related metrics/tests are deleted. Contract IBC messages and queries now fail with deterministic unsupported errors (
Reviewed by Cursor Bugbot for commit 15f90a7. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 77850f9. Configure here.
| } | ||
| if request.IBC != nil { | ||
| return e.IBC(ctx, caller, request.IBC) | ||
| return nil, wasmvmtypes.UnsupportedRequest{Kind: "IBC"} |
There was a problem hiding this comment.
Dropped CosmWasm IBC queries
Medium Severity
CosmWasm IBC query support was removed: IBCQuerier is gone and every request.IBC path returns UnsupportedRequest. Contract-path reads such as PortID, ListChannels, and Channel no longer work, even though retired IBC stores remain mounted. This violates the CosmWasm IBC rule to keep wasm IBC queries while only dropping writes and the callback router.
Additional Locations (1)
Triggered by learned rule: module removal: streaming genesis and StoreUpgrades guards
Reviewed by Cursor Bugbot for commit 77850f9. Configure here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4014 +/- ##
==========================================
- Coverage 59.73% 58.65% -1.08%
==========================================
Files 2265 2167 -98
Lines 194306 182651 -11655
==========================================
- Hits 116063 107143 -8920
+ Misses 67513 65718 -1795
+ Partials 10730 9790 -940
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@seidroid review |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped removal of the retired IBC core from both applications: no dangling references to the deleted symbols remain, the store/module-account tombstones are preserved, and the new tests pin the "not wired" invariant. A few non-blocking items: the tombstone guard is now duplicated verbatim in a second package, WasmApp newly mounts a capability store it never had, one contract-facing IBC query that used to succeed now errors, and one inlined side-effect-only call diverges from the named-step form the main app already uses.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Observability removal is silent:
ibc_begin_blocker_durationand the sixwasm_contract_ibc_*histograms (plus theirtelemetry.ModuleMeasureSince/MeasureSincetwins) disappear with no note in the PR description. Any dashboard or alert referencing them will go blank rather than to zero — worth calling out to whoever owns those panels before this lands. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-wasmd/app/test_helpers.go:79-82justifies pinningsctypes.MemiavlOnlywith "this test app mounts non-canonical store names (e.g. icacontroller, icahost)", butNewWasmAppno longer mounts either of those keys. The comment's stated reason is stale independently of this PR.
| txDecoder sdk.TxDecoder | ||
| } | ||
|
|
||
| var retiredIBCStoreNames = map[string]struct{}{ |
There was a problem hiding this comment.
[suggestion] retiredIBCStoreNames, errIBCDeprecated, and isRetiredIBCStoreQuery are copied verbatim from app/app.go:469-492, and the copy re-declares "ibc"/"transfer"/"capability" as fresh literals (lines 95-97) at the same time this PR migrates the main app onto storekeys.IBCStoreKey. That leaves two independent lists of retired store names that can silently diverge: add a fourth retired store to sei-db/common/keys and only one of the two apps starts refusing queries for it.
This is the "guard at each caller" shape AGENTS.md calls out — the tombstone would be better as one shared helper (or at minimum both copies keyed off storekeys.IBCStoreKey / IBCTransferStoreKey / CapabilityStoreKey) so the raw-store refusal is an invariant rather than a convention the next app has to remember.
|
|
||
| func (q QueryHandler) Query(request wasmvmtypes.QueryRequest, gasLimit uint64) ([]byte, error) { | ||
| if request.IBC != nil && q.Ctx.IsABCIQuery() { | ||
| if request.IBC != nil { |
There was a problem hiding this comment.
[suggestion] Dropping the q.Ctx.IsABCIQuery() condition rejects more than the "state-backed queries" the PR description scopes to. IBCQuery::PortId was not state-backed — the old IBCQuerier served it purely from wasm.GetContractInfo(ctx, caller).IBCPortID, with no channel-keeper read — so it succeeded for every contract, including today with IBC already retired and no channels open.
After this change, a contract whose execute/query path issues IBCQuery::PortId (a common way for a contract to learn its own port) gets unsupported request: IBC and the whole call fails, where it previously returned {"port_id":"wasm.<addr>"}. Worth confirming against deployed code that no live contract takes that path, since keeper.go:360/:455 still populate ContractInfo.IBCPortID for IBC-entry-point contracts.
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, | ||
| evidencetypes.StoreKey, retiredTransferName, | ||
| govtypes.StoreKey, paramstypes.StoreKey, retiredIBCStoreName, upgradetypes.StoreKey, | ||
| evidencetypes.StoreKey, retiredTransferName, capabilityStoreName, |
There was a problem hiding this comment.
[suggestion] capabilityStoreName is a newly mounted store here — the previous key list was evidencetypes.StoreKey, retiredTransferName, feegrantStoreKeyName, ... with no capability store. So unlike the main app, this isn't "keep the capability store mounted" (per the PR description); it creates an empty store and adds an entry to WasmApp's commit info that was never there.
It looks driven by TestRetiredIBCStateRemainsMountedWithoutModuleWiring asserting testApp.keys[capabilityStoreName] != nil. If preserving historical state is the goal, there is none to preserve for this app and the mount could be dropped along with that assertion; if the intent is store-set parity with the main app, that's worth stating so the asymmetry doesn't read as accidental.
|
|
||
| return app.mm.InitGenesis(ctx, app.appCodec, genesisState, genesis.GenesisImportConfig{}) | ||
| response := app.mm.InitGenesis(ctx, app.appCodec, genesisState, genesis.GenesisImportConfig{}) | ||
| app.accountKeeper.GetModuleAccount(ctx, retiredTransferName) |
There was a problem hiding this comment.
[suggestion] This is a bare call kept only for its side effect — GetModuleAccount creates and persists the retired transfer module account — with nothing naming that intent, so it reads as a discarded result a later cleanup would delete.
The main app already has the form to copy: app/app.go:1180 calls app.initializeRetiredTransferModuleAccount(ctx), a named step whose doc comment carries the why. Mirroring that here keeps InitChainer readable as a sequence of named steps and makes the call self-defending.
| ctx = DecoratePriority(ctx, priority) | ||
|
|
||
| return ctx, ibcante.RejectMessages(tx) | ||
| return ctx, nil |
There was a problem hiding this comment.
i think the error was introduced previously - are we moving it back to nil because ibc is completely removed?
|
@seidroid review |
2 participants
Remove IBC keeper, module, genesis, simulation, params, and begin-block
wiring from the production and standalone wasm applications.
Delete dormant CosmWasm IBC lifecycle callbacks and state-backed queries,
returning deterministic unsupported errors for retired contract IBC
operations.
Keep the ibc, transfer, capability, and feegrant stores mounted, preserve
the retired transfer module account, and retain the ibc/103 raw-store
query tombstone. Leave the unreferenced sei-ibc-go tree for a follow-up
mechanical deletion.