Remove unused interchain accounts implementation - #3875
Conversation
Remove the unwired ICS-27 Interchain Accounts implementation and tests. Drop ICA keeper, store, capability, module, and router wiring from the IBC simapp and wasmd reference app.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3875 +/- ##
==========================================
- Coverage 61.73% 60.85% -0.88%
==========================================
Files 2381 2287 -94
Lines 201682 191126 -10556
==========================================
- Hits 124502 116319 -8183
+ Misses 66084 64549 -1535
+ Partials 11096 10258 -838
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
PR SummaryHigh Risk Overview The deleted surface area covers IBC channel middleware ( This aligns the fork with the PR goal of dropping unwired ICA code; the description also notes removal of ICA keeper, store, capability, module, and router wiring from the IBC simapp and wasmd reference app (not all of that wiring may appear in the excerpted diff). Reviewed by Cursor Bugbot for commit 999a475. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Clean, mechanical removal of the never-wired ICS-27 module: I verified no Go references to ICA symbols remain anywhere in the tree, the production app/app.go never wired it, and the two touched apps (sei-ibc-go/testing/simapp, sei-wasmd/app) are consistently de-registered across store keys, maccPerms, param subspaces, IBC routes, module manager, and init-genesis order. Remaining items are leftovers outside the diff — the ICA .proto sources still feed codegen, and several docs/comments now describe removed code.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- ICA proto sources survive the deletion and will resurrect it:
sei-ibc-go/proto/ibc/applications/interchain_accounts/**(8 files) is still covered by thesei-ibc-go/protoinput in the rootbuf.gen.yaml, and theirgo_packageoptions point straight back at the deletedsei-ibc-go/modules/apps/27-interchain-accounts/{types,controller/types,host/types}directories. The nextscripts/protoc.shrun silently recreatesaccount.pb.go,genesis.pb.go,metadata.pb.go,packet.pb.go,controller.pb.go,host.pb.go,query.pb.go, and the twoquery.pb.gw.gofiles as orphan packages. No CI job regenerates-and-diffs, so this won't be caught until someone runs codegen. Delete the proto directory too (or exclude it from the buf input). Agreed with Codex's P1. Note the buf registry push (.github/workflows/proto-registry.yml) only publishesproto/, so deleting these is registry-safe. sei-ibc-go/testing/simapp/test_helpers.go:77-80andsei-wasmd/app/test_helpers.go:83-86pinscConfig.WriteMode = sctypes.MemiavlOnlywith the justification "This simapp mounts non-canonical store names (e.g. icacontroller, icahost) that are not in keys.MemIAVLStoreKeys". After this PR every store key both apps mount is inkeys.MemIAVLStoreKeys(sei-db/common/keys/store_keys.go:39), so the stated reason no longer holds. Either update the comments or drop the pin and let auto mode apply.- Docs still advertise the removed module:
sei-ibc-go/README.md:54lists ICS-27 as an included application, andsei-ibc-go/testing/README.md:314shows aGetSimApp().ICAAuthModule.IBCApp.OnChanOpenInitexample that no longer compiles (the field is deleted in this PR). Same as Codex's P2 — thetesting/README.mdexample is the worse one since it's copy-paste guidance. docs/swagger-ui/swagger.ymlstill documents/ibc/apps/interchain_accounts/host/v1/paramsand theibc.applications.interchain_accounts.host.v1.*schemas (lines ~23908, ~56716). Dead endpoint documentation; falls out naturally if the protos are removed and swagger is regenerated.- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this synthesis merges only my findings with Codex's.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 999a47532c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,377 +0,0 @@ | |||
| // Code generated by protoc-gen-gogo. DO NOT EDIT. | |||
There was a problem hiding this comment.
Exclude retained ICA schemas from code generation
When scripts/protoc.sh is run, the root buf.gen.yaml still includes all of sei-ibc-go/proto, and the retained interchain_accounts schemas still set go_package to these deleted directories; the script then copies that generated tree into the repository. Consequently this file and the other deleted *.pb.go/*.pb.gw.go files are recreated, leaving code generation dirty and effectively undoing part of this removal. Remove or exclude the ICA schemas from generation, or retain their generated outputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yup this is valid and deliverately left behind to reduce the scope since buf will complain of breaking changes. Follow up IBC cleanup PRs will take a stab at things in revertable units.
alexander-sei
left a comment
There was a problem hiding this comment.
Went digging on whether the "unwired" premise actually holds on-chain, and it does — no ICA account has ever existed on any Sei network. Evidence below, since I think it belongs in the PR description. Then four leftovers, one of which I'd call blocking.
The premise checks out
No archive node needed: both records below are cumulative, so current state answers the historical question.
1. The module was never registered. x/upgrade's module version map is the on-chain record of registered modules, and SetModuleVersionMap only writes — it never deletes. So entries survive module removal:
curl -s https://rest.sei-apis.com/cosmos/upgrade/v1beta1/module_versions | jq -r '.module_versions[].name'pacific-1 returns 25 modules, no interchainaccounts. The retention property is what makes this conclusive, and it's demonstrable on the same output: dex (v18) and accesscontrol (v2) are still listed even though neither module exists in the tree anymore (atlantic-2 also still lists nitro). Had ICA ever been registered — for one block, even if later removed — it would still appear. It doesn't, on pacific-1, atlantic-2, or arctic-1.
2. No ICA channel ever opened. Channels are never deleted from state (closed ones persist as CLOSED), so the current set is the complete history:
curl -s "https://rest.sei-apis.com/ibc/core/channel/v1/channels?pagination.limit=500" | jq -r '.channels[].port_id' | sort -uAll 93 mainnet channels are transfer or wasm.*. Zero icahost, zero icacontroller-*, and zero counterparty ICA ports. Same on atlantic-2 (106) and arctic-1 (35). Since an ICA is only ever created during an ICA channel handshake, this independently rules it out for Sei as host and as controller.
3. /ibc/apps/interchain_accounts/{host,controller}/v1/params returns code 12 Not Implemented on all three networks.
Code side agrees: interchainaccounts was never in kvStoreKeyNames (app/app.go:273) or ModuleBasics (app/app.go:221), no upgrade handler ever added the store, and git log --all -S over full history (excluding the vendored trees) finds zero hits for interchainaccounts/icatypes.
1. The .proto sources are still there — codegen will resurrect the module
This is the one I'd fix before merge. The PR deletes the eight ICA .pb.go files but leaves all eight .proto sources under sei-ibc-go/proto/ibc/applications/interchain_accounts/. sei-ibc-go/scripts/protocgen.sh:16 walks unconditionally:
proto_dirs=$(find ./proto -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)So the next person to run codegen regenerates every file this PR just deleted. The removal isn't idempotent. git rm -r sei-ibc-go/proto/ibc/applications/interchain_accounts fixes it and leaves proto/ibc/applications/{transfer/v1,transfer/v2} intact.
2. Two comments now assert something the PR made false
sei-ibc-go/testing/simapp/test_helpers.go:77 and sei-wasmd/app/test_helpers.go:83 both read:
This simapp mounts non-canonical store names (e.g. icacontroller, icahost) that are not in keys.MemIAVLStoreKeys, so it cannot use the default auto mode …
After this PR neither app mounts a non-canonical store name — the NewKVStoreKeys calls at sei-ibc-go/testing/simapp/app.go:218 and sei-wasmd/app/app.go:279 now use only names present in keys.MemIAVLStoreKeys (sei-db/common/keys/store_keys.go:39). So the whole rationale is void, not just the parenthetical example, and it's load-bearing: it's the stated reason for scConfig.WriteMode = sctypes.MemiavlOnly. Suggested rewording that keeps behavior identical and only claims what's still true:
// Pin memiavl_only: this simapp never migrates, so it has no need for the
// default auto mode, which routes writes for the memiavl->flatkv
// migration.
scConfig.WriteMode = sctypes.MemiavlOnlyWorth noting the pin itself is now unjustified too — sei-db/state_db/sc/composite/store.go:1148 describes that MemiavlOnly branch as existing "to support legacy test apps that use non-standard store names", which these no longer are. But dropping it changes what those harnesses exercise (they'd start going through auto-mode routing), so that feels like a separate change rather than something to fold in here.
3. docs/swagger-ui/swagger.yml still documents the ICA endpoint
Path /ibc/apps/interchain_accounts/host/v1/params plus its two orphaned definitions entries — 81 lines documenting something that returns 501 on every network. Not generated by anything in-tree as far as I can tell, so it needs deleting by hand.
4. Nit — sei-ibc-go/README.md:54
Contents index still lists ICS 27. Genuinely a judgment call: every link in that list points at upstream cosmos/ibc-go, so it reads partly as a copy of upstream's README rather than an inventory of this fork, and removing one entry adds divergence. Take it or leave it — I lean remove, since a reader would otherwise reasonably conclude ICS-27 is available here.
Deliberately not flagged
The icahost/icacontroller strings in sei-db/state_db/sc/composite/store_test.go and sei-db/state_db/sc/migration/passthrough_router_test.go should stay. They're synthetic arbitrary store names exercising non-canonical-name handling — a generic capability that still matters for downstream apps, borrowing the names incidentally.
I have 1–4 as a patch locally, verified with go build ./... clean and go test ./sei-ibc-go/testing/... ./sei-wasmd/app/... fully passing, gofmt -s/goimports clean. Happy to hand it over or open it as a follow-up — didn't want to push into your branch.
* main: test(config): complete the GetConfig read-site coverage (PLT-893) (#3870) Remove interchain swagger API and protos (#3881) fix(flatkv): preserve empty misc values and reject malformed empty node imports (#3869) fix(evm): count post-admission apply failures in dynamic base-fee gas (CON-359) (#3871) scripts: load generator for arctic-1 and atlantic-2 (#3850) Update go-releaser heading with experimental notice (#3879) fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780) (#3836) Remove unused interchain accounts implementation (#3875) test(config): extend golden value test coverage (PLT-893) (#3861) Update v6.6 changelog in prep to cut patch release (#3876) Close temporary rootmulti store in connection types setup (#3872) Restore LCD pagination while preserving v6.6 precompile semantics (#3867)
Remove the unwired ICS-27 Interchain Accounts implementation and tests. Drop ICA keeper, store, capability, module, and router wiring from the IBC simapp and wasmd reference app.