Retire all versions of IBC precompiles and remove legacy implementations - #3884
Retire all versions of IBC precompiles and remove legacy implementations#3884masih wants to merge 4 commits into
Conversation
Retire the IBC precompile at every version while preserving each historical ABI and the registered address. All valid calls now revert with a clear retirement reason. Remove obsolete legacy IBC implementations and unused keeper dependencies. Teach the version generator to skip retired modules, preventing IBC from being archived or reactivated during future upgrades. Add coverage confirming every registered IBC version reverts and remains non-payable.
|
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 #3884 +/- ##
==========================================
- Coverage 61.61% 60.74% -0.87%
==========================================
Files 2348 2253 -95
Lines 200852 190078 -10774
==========================================
- Hits 123755 115472 -8283
+ Misses 66044 64406 -1638
+ Partials 11053 10200 -853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Removes IBC wiring from the node: Tests now assert every entry from Reviewed by Cursor Bugbot for commit bd7abc8. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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.
Reviewed by Cursor Bugbot for commit add06b2. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: add06b2823
ℹ️ 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".
| versioned := make(utils.VersionedPrecompiles, len(legacyABIByVersion)+1) | ||
| versioned[latestUpgrade] = newRetiredPrecompile(pcommon.MustGetABI(currentABI, "abi.json"), keepers) | ||
| for version, filename := range legacyABIByVersion { | ||
| versioned[version] = newRetiredPrecompile(pcommon.MustGetABI(legacyABIs, filename), keepers) |
There was a problem hiding this comment.
When debug_traceTransaction replays a pre-v6.6 transaction that successfully called the IBC precompile, CustomPrecompiles selects one of these height-specific entries, but every entry now uses the retired executor and reverts rather than reproducing the original call and state transitions. Keeping only each historical ABI is insufficient for replay correctness; retain the legacy executors for historical versions and retire only the active version.
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Retiring the IBC precompile while preserving every historical ABI and the registered address is a clean approach, and the dead-keeper cleanup is thorough with no dangling references. One blocking issue: opting the module out of scripts/bump_version leaves legacyABIByVersion hand-maintained, and it is already missing v6.6, which will resolve to a nil precompile for the IBC address as soon as the next tag is cut.
Findings: 1 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. Codex also reported it could not run tests (Go 1.25.6 toolchain download blocked); I could not build either, so all findings here are from static reading. precompiles/ibc/versionsis now orphaned:discoverModules()skips retired modules, soarchiveModulewill never append future tags to it, yetx/evm/keeper/keeper_test.go:141(TestGetCustomPrecompiles) still reads it. Either delete the file or make it the source of truth forlegacyABIByVersion— the latter also closes the v6.6 gap permanently.- Test coverage exercises only the
transferWithDefaultTimeoutselector. The other ABI methods, an unknown selector, and sub-4-byte calldata all take different paths throughRunAndCalculateGas(revert with no reason data rather than the retirement reason); worth one case each. - Every retired call now reaches
HandlePrecompileErrorplus the unconditionalfmt.Printf("precompile %s encountered error: ...")atprecompiles/common/precompiles.go:161and increments error metrics. A precompile that reverts by design turns that pre-existing log line into a guaranteed per-call stdout write and error-metric bump that any caller can drive cheaply. Consider whether retirement should bypass the error-reporting path. - Confirmation request, not a correctness claim: retirement is applied to every historical version, so tracing/replaying blocks that originally executed a successful IBC transfer will now revert instead of reproducing the committed result. Per REVIEW_GUIDELINES §2 I am not treating this as a bug, but the retroactive scope (as opposed to gating retirement at the next upgrade) is worth an explicit sign-off in the PR description given the
app-hash-breakinglabel. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| moduleName := entry.Name() | ||
| moduleDir := filepath.Join(precompilesDir, moduleName) | ||
| if fileExists(filepath.Join(moduleDir, retiredMarker)) { |
There was a problem hiding this comment.
[suggestion] The retirement guard now exists in two places — here and in discoverModules() (line 534). regenerateAllSetup re-walks precompilesDir itself rather than iterating discoverModules(), so the next person adding a module-wide filter has to remember both.
AGENTS.md "Structural corrections" calls for exactly this: guard at the single choke point every path passes through. Having regenerateAllSetup iterate discoverModules() (and keep only its versions/{name}.go checks) makes the exclusion an invariant instead of a convention.
| evm := &vm.EVM{StateDB: state.NewDBImpl(ctx, &testApp.EvmKeeper, true)} | ||
|
|
||
| versioned := ibc.GetVersioned("v6.6", testApp.GetPrecompileKeepers()) | ||
| require.Len(t, versioned, 15) |
There was a problem hiding this comment.
[suggestion] require.Len(t, versioned, 15) is the assertion that should have caught the missing v6.6 mapping, and it can't: the count is a literal, and "v6.6" is passed in as latestUpgrade on line 24 so it's present regardless.
Read precompiles/ibc/versions and assert that the key set of GetVersioned(<last line>, …) equals the set of lines in that file. That fails loudly when a tag is added without a corresponding ABI entry — which is now a purely manual step for this module.
| const ( | ||
| IBCAddress = "0x0000000000000000000000000000000000001009" | ||
| ) | ||
| const RetiredReason = "ibc precompile is retired; ibc transfers are disabled" |
There was a problem hiding this comment.
[nit] "ibc transfers are disabled" overstates the change. The IBC transfer module is still mounted and routed (app/app.go:624, :636, :867), so native Cosmos IBC transfers keep working — only the EVM precompile is retired. This string is surfaced to contract callers as the revert reason, so it's worth being precise, e.g. "ibc precompile is retired; use native ibc transfers".
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err | ||
| } | ||
| return e.revertData, GetRemainingGas(ctx, e.evmKeeper), e.err |
There was a problem hiding this comment.
[nit] e.revertData is a single slice built once at construction and handed to every caller. Nothing in the current EVM path mutates returned ret, so this isn't a live bug, but the aliasing is invisible from the call site and one in-place write would corrupt the revert reason for every subsequent call on that node. return common.CopyBytes(e.revertData), … removes the question.
There was a problem hiding this comment.
@masih I could imagine some possible issue with OCC here where some weird caching somewhere could lead to a consensus divergence. worth a look maybe
|
|
||
| func (e *retiredExecutor) Execute(ctx sdk.Context, _ *abi.Method, _ common.Address, _ common.Address, _ []interface{}, value *big.Int, _ bool, _ *vm.EVM, _ uint64, _ *tracing.Hooks) ([]byte, uint64, error) { | ||
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err |
There was a problem hiding this comment.
[nit] The non-payable rejection returns nil revert data, so a value-bearing call reverts with no reason at all — TestRetiredIBCPrecompileRemainsNonPayable pins that with require.Empty(t, ret). Returning all-gas-consumed matches the convention of the other executors, but the empty reason is at odds with the stated goal that calls revert with a clear retirement reason; the caller sending value gets less information than the caller who doesn't. Consider returning e.revertData here too.
There was a problem hiding this comment.
Something worth to improve, some precompile functions are payable and are also used as such
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, oracletypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, |
There was a problem hiding this comment.
[nit] Dropping ibchost.StoreKey / ibctransfertypes.StoreKey here (and "ibc" / "transfer" from Modules below) goes further than retiring the precompile: both modules are still mounted in app/app.go, so these lists no longer describe the app's actual stores. tools/utils has no importer in-tree today, so the impact is nil, but if it's ever used for a state dump or size report it will silently omit live IBC state. Retiring the precompile doesn't require this hunk.
There was a problem hiding this comment.
Retiring the IBC precompile while keeping 0x1009 registered and preserving every historical ABI is the right shape, and the dangling keeper wiring is removed cleanly with no leftover references. No blockers; the notes below concern the hand-written GetVersioned diverging from the generator's manifest contract, unasserted gas/revert semantics, a duplicated retired-marker guard, and an out-of-scope change to tools/utils.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Neither new test asserts the returned
remainingGas, yet retirement changes it: previously every IBC failure returned0(all supplied gas burned), now the normal revert path returns the real remaining gas. Gas is consensus-relevant on an app-hash-breaking change — pin the expected gas outcome for both the reverting and the value-bearing path. TestEveryIBCVersionIsRetiredonly exercisestransferWithDefaultTimeout. The other registered method (transfer) is never called for any version, so a selector-decoding regression ontransferwould go unnoticed. Iterating overcontractABI.Methodsinstead of hardcoding one name would cover both and stay correct as ABIs differ across versions (v5.5.2 lacksmemo).precompiles/ibc/IBC.solis unchanged and still presentstransfer/transferWithDefaultTimeoutas functional. Keeping the file is correct (the ABI must be preserved), but a retirement note in the interface doc would stop integrators writing against a permanently-reverting contract.RunAndCalculateGasdoesfmt.Printf("precompile %s encountered error: ...")on every error. Retirement makes that the guaranteed outcome for a publicly callable address, so every call now writes a line to validator stdout. Pre-existing code path, but the cost/benefit changes when it is the only possible outcome — worth considering demoting it for retired precompiles.- Cursor's review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues, matching my read on correctness. - I could not compile or run the test suite in this environment (Go toolchain fetch is network-blocked, same limitation Codex hit). Compile-correctness of the
NewKeepersignature changes and the removedTransferK()/ClientK()/ConnectionK()/ChannelK()interface methods was verified by grepping for dangling references (none found inprecompiles/,x/evm/,giga/,tools/,app/), not by a build. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| "v6.5": check(ibcv65.NewPrecompile(keepers)), | ||
| historicalVersions := getHistoricalVersions() | ||
| versioned := make(utils.VersionedPrecompiles, len(historicalVersions)+1) | ||
| for _, version := range historicalVersions { |
There was a problem hiding this comment.
[suggestion] This loop treats every line of versions as a legacy version, but the generator's contract is that the last manifest entry is the active version, not an archived one — regenerateAllSetup uses legacyCount := len(versions) - 1 and keys that final entry via latestUpgrade (scripts/bump_version/main.go:412). The generated setup.go this replaces stopped at v6.5 for exactly that reason, even though versions ends with v6.6.
Consequences today are benign — app/tags ends at v6.6, so line 26 overwrites the loop's v6.6 entry and both resolve to byte-identical ABIs (legacy/v66/abi.json == abi.json). But two things follow:
- In production the map has
len(versions)entries, notlen(versions)+1, sorequire.Len(t, versioned, len(historicalVersions)+1)inibc_test.go:32only passes because the test injects a synthetic"future-upgrade". It asserts a shape the running node never produces, and will silently start being the real shape once the next tag is cut. historicalVersionsis a misnomer: it includes the current version.
Suggest iterating historicalVersions[:len(historicalVersions)-1] (mirroring the generator) and naming it accordingly, or documenting deliberately why the whole manifest is archived here.
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err | ||
| } | ||
| return e.revertData, GetRemainingGas(ctx, e.evmKeeper), e.err |
There was a problem hiding this comment.
[suggestion] The two exit paths of this executor have inconsistent semantics. A value-bearing call takes the branch above and returns nil, 0, err — all supplied gas burned, and no revert data, so a Solidity caller sees a bare revert() with no reason. Every other call returns the encoded reason plus the real remaining gas, so it reverts cheaply and explains itself. TestRetiredIBCPrecompileRemainsNonPayable pins the empty-ret half of that asymmetry without noting it is intentional.
The PR description says "All valid calls now revert with a clear retirement reason"; a call carrying value is exactly the case where a caller most needs the reason (their funds are the thing being rejected). Consider returning e.revertData for the non-payable rejection too, or add a comment stating why the payable path deliberately keeps the older burn-all-gas behavior.
There was a problem hiding this comment.
same as above with the payable
| func validCallData(t *testing.T, contractABI abi.ABI) []byte { | ||
| t.Helper() | ||
|
|
||
| method := contractABI.Methods["transferWithDefaultTimeout"] |
There was a problem hiding this comment.
[nit] Single-value map index: if a version's ABI ever lacked transferWithDefaultTimeout, this yields the zero abi.Method — nil ID, empty Inputs — so input becomes empty, ExtractMethodID fails with "input too short", and the test fails at abi.UnpackRevert with a message that points nowhere near the actual cause. Use the two-value form with require.True(t, ok, ...) so the failure names the missing method. (All 15 archived ABIs do currently define it, so this is about the next one, not today.)
|
|
||
| moduleName := entry.Name() | ||
| moduleDir := filepath.Join(precompilesDir, moduleName) | ||
| if fileExists(filepath.Join(moduleDir, retiredMarker)) { |
There was a problem hiding this comment.
[suggestion] The retired-marker check now lives in two places — here and in discoverModules at line 534 — because regenerateAllSetup re-walks precompilesDir itself instead of going through discoverModules(). That is the pattern AGENTS.md calls out under "Guard at the choke point, never at each caller": a third module walker added later has to remember the marker, where routing both through one discovery function makes it an invariant. regenerateAllSetup also duplicates the excludeDirs filter for the same reason.
Suggest having regenerateAllSetup iterate discoverModules() so the marker (and excludeDirs) are honoured in exactly one place.
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, oracletypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, |
There was a problem hiding this comment.
[suggestion] Dropping ibchost.StoreKey / ibctransfertypes.StoreKey here (and "ibc" / "transfer" from Modules below) makes this helper disagree with the running app: app/app.go:276-277 still mounts both stores, and the IBC and transfer modules are still registered in the module manager (app/app.go:897, 967-970). Those stores still hold state, so a state-size or dump tool built on ModuleKeys/Modules will now silently omit it and under-report rather than error.
Nothing in-tree imports tools/utils today, so this is not an active break — but it is also unrelated to retiring the precompile, and the precompile retirement does not remove the IBC module. Worth either reverting this hunk from the PR or stating why the tool should stop seeing state the chain still keeps.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Retiring the IBC precompile at every version while preserving the address and each historical ABI is a clean, well-documented approach, and the removal of ~7.4k lines of legacy code and unused keepers checks out (no dangling references, all embedded legacy ABIs present). No blockers; the notes below are a mix of a manifest off-by-one against the bump_version convention, gas/log-amplification asymmetries in the new retired executor, an out-of-scope tools/utils change, and test/doc gaps.
Findings: 0 blocking | 14 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Historical trace fidelity is permanently lost: the versioned precompile map is consumed only under
ctx.IsTracing()(x/evm/keeper/keeper.go:174-182), sodebug_trace*/ replay of any historical block containing a successful IBC precompile call will now report a revert, with different gas accounting. This appears intentional ("Retire the IBC precompile at every version") and per REVIEW_GUIDELINES.md §2 I'm not treating it as a correctness bug — but it is irreversible (the legacy implementations are deleted, not just unwired) and should be stated explicitly in the PR description / upgrade notes for anyone relying on historical tracing. CHANGELOG.md## Unreleasedis not updated. This is both client-breaking (calls to 0x…1009 now revert) and state-machine-breaking, and the section is actively maintained (e.g. the #3818 entry), so it warrants an entry under the appropriate stanza.precompiles/ibc/IBC.solis left unchanged and still advertises workingtransfer/transferWithDefaultTimeout. It is the developer-facing artifact for this address; a deprecation comment there (and in the retainedabi.jsondocs if any) would prevent integrators from writing against a permanently reverting interface.- The new
retiredmarker convention is documented only inscripts/bump_version/README.md. A line in the top-levelAGENTS.md(or aprecompiles/AGENTS.md) would make it discoverable to contributors and agents working underprecompiles/, since it changes how the version generator behaves. - Second-opinion passes: Codex reported "No material issues found in the PR diff." Cursor's
cursor-review.mdis empty — that pass produced no output, so it provides no signal either way. - Verification caveat:
go build ./...andgo test ./precompiles/ibc/...were blocked in this sandbox, so compile/test confirmation is by inspection only. I did grep-verify that no references remain to the removedTransferKeeper/ClientKeeper/ConnectionKeeper/ChannelKeeperinterfaces or the deleted legacy IBC packages (including in the otherprecompiles/common/legacy/*dirs), thatibc.IBCAddressis still consumed byprecompiles/setup.goandgiga/executor/precompiles/failfast.go, and that all 15precompiles/ibc/legacy/*/abi.jsonfiles required by the newgo:embedpattern exist. Please rely on CI for the actual build/test result. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| func (e *retiredExecutor) Execute(ctx sdk.Context, _ *abi.Method, _ common.Address, _ common.Address, _ []interface{}, value *big.Int, _ bool, _ *vm.EVM, _ uint64, _ *tracing.Hooks) ([]byte, uint64, error) { | ||
| if err := ValidateNonPayable(value); err != nil { | ||
| return common.CopyBytes(e.revertData), 0, err |
There was a problem hiding this comment.
[suggestion] Asymmetric gas semantics between two paths that produce the identical outcome. A value-bearing call returns remainingGas = 0 (all supplied gas consumed) while the plain retirement revert on line 37 returns GetRemainingGas(...) (gas refunded). Both return the same Error("ibc precompile is retired; ...") revert data and both surface as vm.ErrExecutionReverted, so a caller sending 1 wei is charged the full gas limit and a caller sending 0 is not, for the same revert.
A precompile that unconditionally reverts has no payability semantics left to enforce, so the simplest correction is to drop the ValidateNonPayable branch entirely and always return common.CopyBytes(e.revertData), GetRemainingGas(ctx, e.evmKeeper), e.err. That also makes the retirement reason the single recorded error for SetPrecompileError/metrics, rather than sometimes "sending funds to a non-payable function". If you prefer to keep the branch, at least align its gas return with line 37.
Worth noting: several historical ABIs declare both methods "stateMutability": "payable" (e.g. legacy/v552/abi.json), so TestRetiredIBCPrecompileRemainsNonPayable is not quite describing what it pins — the retired executor imposes non-payability uniformly, including on versions where the method was payable.
| if err := ValidateNonPayable(value); err != nil { | ||
| return common.CopyBytes(e.revertData), 0, err | ||
| } | ||
| return common.CopyBytes(e.revertData), GetRemainingGas(ctx, e.evmKeeper), e.err |
There was a problem hiding this comment.
[suggestion] Because this executor errors on every call, each call to 0x…1009 now unconditionally triggers three logging/metric side effects that previously only fired on genuine failures:
- the unconditional
fmt.Printf("precompile %s encountered error: %v\n", ...)inDynamicGasPrecompile.RunAndCalculateGas(precompiles/common/precompiles.go:161) — raw stdout, no rate limiting; metrics.IncrementErrorMetrics(operation, err)viaHandlePrecompileError, which will now light up error dashboards/alerts for what is expected behaviour;- if a contract try/catches the revert and the transaction succeeds,
logger.Error("Transaction succeeded in execution but has precompile error ", ...)inx/evm/keeper/receipt.go:283.
A reverting precompile call is cheap, so this is unbounded stdout/error-log and error-metric amplification on every node, triggerable by anyone. None of these lines are introduced here, but this change is what makes them fire on the happy path. Removing (or gating) that fmt.Printf and considering whether a retirement revert should count toward error metrics would be worth doing in this PR.
| filename := "legacy/" + strings.ReplaceAll(version, ".", "") + "/abi.json" | ||
| versioned[version] = newRetiredPrecompile(pcommon.MustGetABI(retiredAssets, filename), keepers) | ||
| } | ||
| versioned[latestUpgrade] = newRetiredPrecompile(pcommon.MustGetABI(currentABI, "abi.json"), keepers) |
There was a problem hiding this comment.
[suggestion] Off-by-one against the versions manifest convention this file is now hand-maintaining. scripts/bump_version/main.go:411 computes legacyCount := len(versions) - 1 — the last line of versions corresponds to latestUpgrade and is deliberately not emitted as a legacy entry (compare precompiles/bank/versions, which ends in v6.6, against the generated precompiles/bank/setup.go, whose legacy list stops at v6.5).
precompiles/ibc/versions also ends in v6.6, and app.LatestUpgrade is currently v6.6 (last line of app/tags). So the loop on lines 22-25 sets versioned["v6.6"] from legacy/v66/abi.json, and this line silently overwrites it — the returned map has 15 entries, not the len(historicalVersions)+1 the make on line 21 assumes.
This is benign today (abi.json and legacy/v66/abi.json are byte-identical and every version reverts anyway) and self-resolves once a later tag is cut, but the file's own doc comment calls the manifest "the source of truth" while diverging from how every generated setup.go reads it. Either iterate historicalVersions[:len(historicalVersions)-1] to match the generator, or drop the trailing v6.6 from the frozen manifest — and either way the duplicate-assignment shadowing is worth removing before the next maintainer reads this as the reference implementation for retiring a module.
|
|
||
| const futureUpgrade = "future-upgrade" | ||
| versioned := ibc.GetVersioned(futureUpgrade, testApp.GetPrecompileKeepers()) | ||
| require.Len(t, versioned, len(historicalVersions)+1) |
There was a problem hiding this comment.
[suggestion] This assertion only holds because the test injects the synthetic futureUpgrade = "future-upgrade" name. The production call site is precompiles/setup.go:64, ibc.GetVersioned(latestUpgrade, keepers) with latestUpgrade == app.LatestUpgrade == "v6.6", which is also the last line of versions — so in production the map has len(historicalVersions) entries and this require.Len would fail (see the related note on setup.go:26).
As written the test pins an invariant that is false for the only real caller, and specifically hides the duplicate-key collision. Adding a case that calls ibc.GetVersioned(app.LatestUpgrade, ...) would both cover the production path and surface the off-by-one.
| func validCallData(t *testing.T, contractABI abi.ABI) []byte { | ||
| t.Helper() | ||
|
|
||
| method := contractABI.Methods["transferWithDefaultTimeout"] |
There was a problem hiding this comment.
[suggestion] Two test-coverage gaps here:
- Only
transferWithDefaultTimeoutis exercised, but every ABI version also exposestransfer(with differing signatures across versions). The PR description claims "All valid calls now revert"; iteratingcontractABI.Methodsand asserting the retirement revert for each would actually prove that, and would automatically cover any method a historical ABI has that the current one doesn't. contractABI.Methods["transferWithDefaultTimeout"]is not existence-checked. On a miss this yields a zeroabi.Method, somethod.IDis nil andvalidCallDatareturns empty calldata — the failure then surfaces fromabi.UnpackRevertrather than from the missing method, which is a confusing signal. Arequire.Contains(t, contractABI.Methods, ...)(orrequire.True(t, ok)on the two-value lookup) makes it fail where the problem is.
Minor, on line 112: append(method.ID, encoded...) appends to a slice the ABI's Method still holds (geth builds it as crypto.Keccak256(sig)[:4], so cap is 32). The transferWithDefaultTimeout payload always exceeds 28 bytes so it reallocates in practice, but append(append([]byte{}, method.ID...), encoded...) removes the aliasing question entirely.
|
|
||
| moduleName := entry.Name() | ||
| moduleDir := filepath.Join(precompilesDir, moduleName) | ||
| if fileExists(filepath.Join(moduleDir, retiredMarker)) { |
There was a problem hiding this comment.
[suggestion] The retired-module guard is now duplicated: here, and again inside discoverModules at line 534. AGENTS.md ("Structural corrections") calls this out directly — "Guard at the choke point, never at each caller. A guard repeated at every call site is a convention the next caller can forget, where a guard at the single function every path passes through is an invariant they cannot."
There is a choke point available: regenerateAllSetup re-implements discoverModules's filter (entry.IsDir() + excludeDirs) by walking os.ReadDir(precompilesDir) itself. Iterating discoverModules() instead gives you the identical module set plus the retired check for free, and means a third loop over precompiles/ added later cannot forget the marker. fileExists then has a single caller and the guard becomes an invariant rather than a convention.
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, oracletypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, |
There was a problem hiding this comment.
[suggestion] This goes beyond retiring the precompile and looks inconsistent with the rest of the tree. ibchost.StoreKey and ibctransfertypes.StoreKey are dropped from ModuleKeys (and "ibc" / "transfer" from Modules below), but the IBC and transfer modules are still fully wired and their stores still mounted: app/app.go:276-277 mounts both store keys, app/app.go:620-637 constructs IBCKeeper/TransferKeeper and registers the transfer IBC module, app/app.go:967-970 keeps both in the module ordering, and app/ante.go:99 still installs ibcante.NewAnteDecorator. Native MsgTransfer IBC transfers remain functional; only the EVM precompile path is retired.
So the s/k:ibc/ and s/k:transfer/ subtrees still exist on chain, and any tooling driving off these lists (dump / state-size / prefix iteration, via BuildRawPrefix/BuildTreePrefix) will now silently skip them rather than report them as empty. I found no in-repo consumer of ModuleKeys/Modules, so nothing breaks at compile time, which is exactly why a silent coverage hole in external tooling is the risk. Unless the IBC Cosmos modules are also being removed (they aren't in this PR), I'd revert this file and keep it in sync with app/app.go.
| const ( | ||
| IBCAddress = "0x0000000000000000000000000000000000001009" | ||
| ) | ||
| const RetiredReason = "ibc precompile is retired; ibc transfers are disabled" |
There was a problem hiding this comment.
[suggestion] "ibc transfers are disabled" overstates the change and will be inaccurate for as long as this string exists. Native Cosmos IBC transfers remain fully enabled — app/app.go:624-637 still builds TransferKeeper and registers the transfer module/IBC route, and app/ante.go:99 still installs the IBC ante decorator. What is retired is the EVM precompile path only.
This reason is ABI-encoded into on-chain revert data at a fixed address and is what dapp developers will see in their error strings, so it's worth getting precise — e.g. "ibc precompile is retired; use native ibc transfers". Same wording appears in the NewPrecompile doc comment on line 20 ("IBC transfers are permanently disabled").
4 participants
Retire the IBC precompile at every version while preserving each historical ABI and the registered address. All valid calls now revert with a clear retirement reason.
Remove obsolete legacy IBC implementations and unused keeper dependencies. Teach the version generator to skip retired modules, preventing IBC from being archived or reactivated during future upgrades.
Add coverage confirming every registered IBC version reverts and remains non-payable.