feat(precompiles)!: remove IBC and Oracle precompiles - #324
feat(precompiles)!: remove IBC and Oracle precompiles#324alexander-sei wants to merge 2 commits into
Conversation
The IBC and Oracle precompiles are no longer supported on Sei, so the package was pointing integrators at endpoints that fail. Drops the addresses, ABIs, ethers contract factories, and viem ABIs across all three entrypoints, following the confidential transfers removal in #279. BREAKING CHANGE: IBC_PRECOMPILE_ADDRESS, IBC_PRECOMPILE_ABI, ETHERS_IBC_PRECOMPILE_ABI, getIbcPrecompileEthersV6Contract, VIEM_IBC_PRECOMPILE_ABI and the Oracle equivalents are no longer exported. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A clean, well-scoped deletion of the unsupported IBC and Oracle precompiles: all six source files, three barrels, and both spec files are pruned consistently, no dangling references remain anywhere in the repo, and the changeset is present. The only open items are non-blocking: an explicit call on minor vs major for removing ten public exports, and changelog wording that offers inlining as a migration path for precompiles that no longer work on-chain.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this synthesis reflects only my pass plus Codex's. - Independently verified the removal is complete: repo-wide grep for
IBC_PRECOMPILE*/ORACLE_PRECOMPILE*/getIbcPrecompileEthersV6Contract/getOraclePrecompileEthersV6Contract/oraclefinds no remaining references outside the new changeset and the historicalpackages/precompiles/CHANGELOG.md.src/index.tsre-exports the three barrels withexport *, so the removal propagates to the package root entrypoint as well, and neither spec file has a hardcoded precompile count that would silently drift. - The PR description states the minor bump matches "how #279 shipped the equivalent removal", but
packages/precompiles/CHANGELOG.md:13-17shows that removal shipped as a patch (2.1.2: "Fixed oracle precompile address ... and removed confidential transfers"). The direction (not a major) matches precedent; the level does not. Worth correcting the description so the versioning rationale rests on the actual precedent. - Merge-order hazard: the PR base is #323's branch, not
main. Merging in the wrong order (or squashing #323 without retargeting) would pull #323's barrel-parity test in as part of this PR's history or lose it. The author flagged this — just confirming a human needs to sequence it. - I disagree with Codex's prompt-injection finding on the PR description. "Review #323 first; retarget this to
mainonce it merges" is ordinary author-to-reviewer prose about stacked-branch logistics, not an attempt to redirect a reviewer agent's instructions or verdict. Flagging it risks desensitizing reviewers to real injection attempts. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@sei-js/precompiles': minor | |||
There was a problem hiding this comment.
[suggestion] Semver: this deletes ten public exports, so a minor lets any consumer on ^2.1.x pick it up on a routine pnpm update and fail to compile. Strict semver would call for major.
Two things temper this, which is why I'm raising it as a suggestion rather than a blocker:
- The repo has precedent for shipping exactly this shape of removal below
major. Note that the precedent is looser than the PR description claims: Fixed Oracle Precompile Address & Removed Confidential Transfers Precompile #279 shipped as a patch (packages/precompiles/CHANGELOG.md:13-17, 2.1.2), not a minor. Worth fixing that sentence in the description either way. - The failure mode is a compile error at the import site, not silent misbehaviour — and since these precompiles are already unsupported on-chain, the pre-removal runtime behaviour was a failing call anyway.
So the substantive question is just whether the team wants 2.2.0 or 3.0.0 here. Flagging for an explicit call rather than asserting the bump is wrong.
| '@sei-js/precompiles': minor | ||
| --- | ||
|
|
||
| **Breaking:** remove the IBC and Oracle precompiles. `IBC_PRECOMPILE_ADDRESS`, `IBC_PRECOMPILE_ABI`, `ETHERS_IBC_PRECOMPILE_ABI`, `getIbcPrecompileEthersV6Contract`, `VIEM_IBC_PRECOMPILE_ABI`, `ORACLE_PRECOMPILE_ADDRESS`, `ORACLE_PRECOMPILE_ABI`, `ETHERS_ORACLE_PRECOMPILE_ABI`, `getOraclePrecompileEthersV6Contract`, and `VIEM_ORACLE_PRECOMPILE_ABI` are no longer exported. Consumers still calling these precompiles must inline the address and ABI themselves. |
There was a problem hiding this comment.
[suggestion] "Consumers still calling these precompiles must inline the address and ABI themselves" sits awkwardly against the PR's own rationale that IBC and Oracle are no longer supported on Sei (CON-283/CON-205). As written it reads as a working migration path — inline 0x...1009 / 0x...1008 and carry on — when the calls will fail regardless of where the ABI lives.
Since the changeset body is what lands in the published changelog, it's the one place downstream integrators will look. Suggest saying the precompiles are no longer supported on-chain and that calls to them fail, then pointing at the replacement path (or stating there isn't one) instead of offering inlining as the remedy.
Review feedback on #324. Telling consumers to inline the address and ABI reads as a working migration, but these precompiles are unsupported on-chain so the calls fail wherever the ABI comes from. Say that instead. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both review notes addressed in 29e24a9 and the updated description.
Thanks for catching the precedent error — #279 shipped as a patch (2.1.2), not a minor. The description said this PR "matched" #279, which overstated the precedent in my favour. Corrected: the precedent is looser than what is proposed here, and minor sits between it and strict semver. Still happy to move to major if you would rather hold the strict line. "inline the address and ABI themselves" — agreed, that read as a working migration path when it is not one. The changeset now says the precompiles are unsupported on-chain, that calls fail wherever the ABI comes from, and that there is no drop-in replacement. |
There was a problem hiding this comment.
Clean, complete removal of the IBC and Oracle precompiles — all six source files deleted, all three barrels and both spec fixture tables pruned consistently, and a changeset is present with a clear migration note. The only substantive discussion point is the deliberate choice to ship a public-export removal as a minor rather than a major; nothing here is a correctness or security defect.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verified the blast-radius claim independently: after this diff there are zero remaining
IBC_PRECOMPILE_*/ORACLE_PRECOMPILE_*/getIbcPrecompile*/getOraclePrecompile*references anywhere outside the changeset itself and the historical CHANGELOG.create-sei's extension and next-template only importBANK_PRECOMPILE_ADDRESS/VIEM_BANK_PRECOMPILE_ABI, andpackages/registry's IBC chain registry is unrelated. There is also no typedoc config or docs manifest inpackages/precompilesthat enumerated the removed files, so nothing is left dangling. - The
barrelParity.spec.tsassertion added in #323 does hold up here as claimed — it derives names via regex over each barrel's exports rather than a hardcoded count, so it tracks the set down from 12 to 10 and would fail if any one barrel were pruned inconsistently. No new test is needed for a deletion of this shape. - Per the PR note, this targets the #323 branch rather than
main, socoverage.ymldoes not runrun-testson this PR. Worth confirming the full suite is green after retargeting, since the ethers/viem spec fixture tables changed here and onlybuildhas exercised them so far. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review is not corroborated by it. The Codex pass reported "No material issues found in the reviewed diff," which matches my read. - Docs outside this repo (docs.sei.io, any published typedoc output) may still document the removed exports. Out of scope for this PR, but worth a follow-up so integrators aren't pointed at endpoints that no longer exist in the package or on-chain.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@sei-js/precompiles': minor | |||
There was a problem hiding this comment.
[suggestion] Non-blocking, and I see you explicitly invited this call in the description — but I lean toward major here.
The concrete cost of minor is that caret ranges don't protect anyone. This repo's own packages/create-sei/extensions/precompiles/package.json and templates/next-template/package.json both depend on "@sei-js/precompiles": "^2.1.2", so 2.2.0 is picked up automatically on any fresh install or lockfile refresh. Downstream dApps on ^2.x are in the same position: they receive the removal without an intentional upgrade step. A major is exactly the mechanism that makes the removal opt-in, and it's cheap here — there is no coordinated bump to worry about, since .changeset/config.json sets fixed: [] and linked: [].
On the two arguments for minor: the #279 precedent (patch) is real, but a prior looser call is weak justification for a second one rather than a reason to tighten. And "compile error, not silent misbehaviour" is the right read of the failure mode — it's a loud break — but semver's major signal is about whether consumers opted in, not about how loudly the break surfaces. The **Breaking:** marker in the body does keep the changelog honest, which mitigates but doesn't replace the range semantics.
Happy to defer if the team prefers holding to the #279 precedent; flagging so it's a recorded decision rather than an assumed one.
|
Recording the decision: staying with Your caret-range point is the strongest argument against and it's correct — I verified both in-repo consumers ( Weighed against that: these precompiles are already dead on-chain, so consumers "protected" by a major would be pinned to a version whose IBC/Oracle calls fail anyway — the opt-in buys a working build, not working functionality. Combined with the repo's existing tolerance for sub-major removals, that's where the team landed for this release. The |
Closes PLT-977.
Note
Stacked on #323 — both touch the ethers barrel and the ethers spec, so this is based on that branch to avoid a conflict. Review #323 first; retarget this to
mainonce it merges.Why
The IBC and Oracle precompiles are no longer supported on Sei (see CON-283 and CON-205), but the package still shipped a full public surface for both, pointing integrators at endpoints that will fail. Same shape of change as #279, which removed the confidential transfers precompile.
What
Deletes the six source files (
precompiles/,ethers/,viem/for each) and prunes all three barrels and both spec files. Ten exports are removed:precompiles/IBC_PRECOMPILE_ADDRESS,IBC_PRECOMPILE_ABIORACLE_PRECOMPILE_ADDRESS,ORACLE_PRECOMPILE_ABIethers/ETHERS_IBC_PRECOMPILE_ABI,getIbcPrecompileEthersV6ContractETHERS_ORACLE_PRECOMPILE_ABI,getOraclePrecompileEthersV6Contractviem/VIEM_IBC_PRECOMPILE_ABIVIEM_ORACLE_PRECOMPILE_ABIVersioning
This removes ten public exports, so strict semver would call for a major. Shipping it as a minor (2.2.0) is a deliberate call, not an oversight:
The changeset body leads with an explicit
**Breaking:**marker so it still reads as breaking in the changelog under the "Minor Changes" heading. Happy to move it to major if reviewers would rather hold the strict line.Blast radius
Nothing else in the monorepo referenced these —
create-sei's extension and next-template only use the bank precompile, and they pin^2.1.2. The only remainingibc/oraclematches in the repo are@sei-js/registry's IBC chain registry, which is unrelated and stays, and the historical changelog.Verification
tsc --noEmitclean, 35/35 jest tests pass, biome clean,changeset statusresolves@sei-js/precompilesto minor.run-testsjob does not run on this PR —coverage.ymlonly triggers for PRs targetingmainorfeature/**. It will run once this is retargeted tomain.builddoes run and passes.