fix(precompiles): export solo precompile from ethers entrypoint - #323
fix(precompiles): export solo precompile from ethers entrypoint#323alexander-sei wants to merge 4 commits into
Conversation
ETHERS_SOLO_PRECOMPILE_ABI and getSoloPrecompileEthersV6Contract were implemented but never re-exported from src/ethers/index.ts, leaving them unreachable from @sei-js/precompiles and @sei-js/precompiles/ethers. The ethers spec worked around this by importing from '../soloPrecompile' directly; it now resolves through the barrel so the test covers the export path it is meant to. Co-authored-by: Cursor <cursoragent@cursor.com>
The solo export could go missing because nothing asserted the ethers, viem, and precompiles barrels expose the same set. Derives the precompile names from each barrel's ABI constants and compares them, so the next precompile that is added to one barrel and forgotten in another fails the suite instead of shipping. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 82.59% 82.60% +0.01%
==========================================
Files 79 79
Lines 1333 1334 +1
Branches 163 224 +61
==========================================
+ Hits 1101 1102 +1
+ Misses 232 226 -6
- Partials 0 6 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Correct, minimal fix for a real gap — solo was exported from the precompiles and viem barrels but not ethers — backed by a well-designed parity test that would have caught the class of bug rather than just this instance. No blockers; two non-blocking notes, the more substantive being that the changeset text implies the @sei-js/precompiles/ethers subpath is fixed when the author's own verification says it still doesn't resolve.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty, so that pass produced no output. Codex's pass reported no material findings, which matches my own read of the diff. - Confirmed the new
src/__tests__/barrelParity.spec.tsis picked up by the package's jest config — it uses a barepreset: 'ts-jest'with no customtestMatch, so the default**/__tests__/**/*.[jt]spattern collects it. Worth knowing the guard actually runs in CI, since a test that silently isn't collected is the usual failure mode for a regression guard like this. - The parity test correctly scopes the factory assertion to the ethers barrel only — viem exports ABIs (plus
seiLocalfromchain.ts) and no contract factories, so there is no viem equivalent to check. Noting this so a future reader doesn't 'fix' the asymmetry. - Semver bump (
minor) is right for a new reachable public export on a 2.1.x package, and the single-package changeset is appropriate here — this doesn't touch shared deps, so the repo's cross-package patch-changeset precedent doesn't apply. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| '@sei-js/precompiles': minor | ||
| --- | ||
|
|
||
| Export the Solo precompile from the `ethers` entrypoint. `ETHERS_SOLO_PRECOMPILE_ABI` and `getSoloPrecompileEthersV6Contract` were implemented but never re-exported from `src/ethers/index.ts`, so they were unreachable from both `@sei-js/precompiles` and `@sei-js/precompiles/ethers`. |
There was a problem hiding this comment.
[suggestion] This wording will mislead users in the published release notes. Saying the symbols "were unreachable from both @sei-js/precompiles and @sei-js/precompiles/ethers" reads as though this PR restores both paths, but per your own note in the PR description the /ethers subpath still fails with MODULE_NOT_FOUND from a packed tarball — the package has no exports map and no ethers/ directory at the root. A reader of the changelog will reasonably try the subpath import and hit an error.
Suggest scoping the claim to what actually changed, e.g.: "…so they were unreachable from the @sei-js/precompiles package root." Optionally add a line noting the @sei-js/precompiles/ethers subpath remains unresolvable and is tracked separately — that turns a misleading note into a useful one.
| it('exposes one ethers contract factory per precompile', () => { | ||
| const factories = Object.keys(ethersBarrel).filter((key) => /^get[A-Za-z]+PrecompileEthersV6Contract$/.test(key)); | ||
|
|
||
| expect(factories).toHaveLength(precompileNames.length); |
There was a problem hiding this comment.
[nit] This asserts a count rather than a correspondence, which leaves a gap the other three assertions don't have: adding a second factory for one precompile while dropping another keeps the length equal and the test green — exactly the shape of the bug this file exists to catch.
The factory names map cleanly onto the ABI names if you capture and uppercase the middle segment (Solo→SOLO, Ibc→IBC, Pointerview→POINTERVIEW, JSON→JSON), so a set comparison is available at no extra cost and matches the style of the assertions above:
const factories = Object.keys(ethersBarrel)
.map((key) => /^get([A-Za-z]+)PrecompileEthersV6Contract$/.exec(key)?.[1]?.toUpperCase())
.filter((name): name is string => name !== undefined)
.sort();
expect(factories).toEqual(precompileNames);Non-blocking — the count check does catch the plain missing-export case this PR fixes.
Review feedback on #323. The count assertion passed whenever a factory was added for one precompile and dropped for another, which is the shape of bug this file exists to catch. Factory names map onto the ABI names once the captured segment is uppercased, so compare the sets directly. Also scopes the changeset to the package root: the /ethers subpath is still unresolvable (PLT-841) and the old wording implied this restored it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both review notes addressed in 20a5829. Factory count vs. correspondence — agreed, and the gap was real. Switched to the set comparison; the uppercased capture maps cleanly across all twelve ( I checked it catches the exact case you described rather than just the plain missing-export one. Aliasing solo's factory to a different name while leaving its ABI export in place keeps the count at 12 on both sides — the old assertion passed, the new one fails. Changeset wording — correct, and the misleading part would have landed in the published changelog. Scoped the claim to the package root and added a line noting the |
There was a problem hiding this comment.
A correct, well-scoped fix: the missing ./soloPrecompile re-export is added to the ethers barrel, the existing spec is redirected through the barrel so the gap can't hide again, and a new parity test guards the whole class of bug. No blockers — only a regex-consistency nit and a note that the Cursor pass produced no output.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty, so that pass contributed nothing to this synthesis. Codex reported no material issues, which matches my own read of the diff. - The parity test covers
*_PRECOMPILE_ABIsymbols and ethers contract factories, but not*_PRECOMPILE_ADDRESSconstants from the base barrel. Extending the same derivation to addresses would close the last symbol class where a barrel could silently drift — optional, since addresses are only exported from one barrel today. - Per REVIEW_GUIDELINES §3 a changeset is required, and
.changeset/export-solo-precompile-ethers.mdis present with an accurate description;minoris the right bump for newly-reachable public API. The unresolvable@sei-js/precompiles/etherssubpath is correctly scoped out and disclosed in both the PR body and the changeset — worth confirming a tracking ticket exists, since every ethers JSDoc example insrc/ethers/*.tsstill shows that import path. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| it('exposes an ethers contract factory for each precompile', () => { | ||
| const factories = Object.keys(ethersBarrel) | ||
| .map((key) => /^get([A-Za-z]+)PrecompileEthersV6Contract$/.exec(key)?.[1]?.toUpperCase()) |
There was a problem hiding this comment.
[nit] The factory capture is [A-Za-z]+ while the ABI capture on line 6 is [A-Z0-9]+. Verified against the current 12 precompiles the two agree (including the irregular getIbc…/getJSON…/getPointerview… cases), but a future precompile with a digit in its name — say ERC721_PRECOMPILE_ABI / getErc721PrecompileEthersV6Contract — would be picked up by the ABI pattern and dropped by the factory pattern, failing this test with a message that points at a missing factory rather than at the regex. Widening to [A-Za-z0-9]+ keeps the two derivations symmetric.
Review nit on #323. The ABI capture allows digits but the factory capture did not, so a future precompile like ERC721 would be found by one derivation and dropped by the other, failing as a missing factory rather than as a pattern mismatch. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed in 7045788 — good catch, the asymmetry would have surfaced as a misleading "missing factory" failure rather than pointing at the pattern. Both captures now allow digits. |
There was a problem hiding this comment.
Correct, well-scoped fix: soloPrecompile was genuinely the only precompile missing from the ethers barrel, the one-line export makes it reachable from the package root via src/index.ts, and the new parity test is a real regression guard (verified to resolve to the same 12-name set across all four assertions). No blockers; one robustness gap in the new test's name-matching regex and a couple of minor notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The parity test guarantees consistency between barrels, not completeness against the filesystem: a new
fooPrecompile.tsadded undersrc/precompiles/,src/ethers/andsrc/viem/but never wired into any of the threeindex.tsfiles would leave all four assertions green. Deriving the expected name list from thesrc/precompiles/*.tsfile listing (or asserting the count against it) would close that remaining hole. Not required for this PR — the fix at hand is the missing-from-one-barrel case, which the test does catch. - The Cursor second-opinion pass (
cursor-review.md) is empty — that review produced no output, so this synthesis reflects only the Codex pass (no material findings) and my own review. - The JSDoc example at
packages/precompiles/src/ethers/soloPrecompile.ts:16still imports from@sei-js/precompiles/ethers, which does not resolve from a packed tarball (noexportsmap, no rootethers/directory). Pre-existing, unchanged by this PR, and explicitly disclosed in both the PR body and the changeset — flagging only so the follow-up isn't lost, since every precompile's JSDoc carries the same broken path. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| import * as viemBarrel from '../viem'; | ||
|
|
||
| const precompileNamesFrom = (barrel: Record<string, unknown>, prefix: '' | 'ETHERS_' | 'VIEM_'): string[] => { | ||
| const pattern = new RegExp(`^${prefix}([A-Z0-9]+)_PRECOMPILE_ABI$`); |
There was a problem hiding this comment.
[suggestion] [A-Z0-9]+ excludes _, so a precompile whose name spans multiple words is silently invisible to this guard. A future CONFIDENTIAL_TRANSFERS_PRECOMPILE_ABI fails ^([A-Z0-9]+)_PRECOMPILE_ABI$ (the class can't span the interior underscore, and ^ blocks a partial match on the trailing segment), and ETHERS_CONFIDENTIAL_TRANSFERS_PRECOMPILE_ABI fails the prefixed form identically — so the name drops out of all three lists symmetrically and the comparisons still pass. That's exactly the bug this PR is fixing: solo missing from just the ethers barrel would go undetected for such a precompile. The toBeGreaterThan(0) guard above doesn't help either, since the other 12 names keep the base list non-empty.
[A-Z0-9_]+ fixes it. Note the prefixed patterns then need the anchor to stay meaningful — ^ETHERS_([A-Z0-9_]+)_PRECOMPILE_ABI$ is fine, but the unprefixed ^([A-Z0-9_]+)_PRECOMPILE_ABI$ applied to a barrel that also held ETHERS_* keys would capture ETHERS_FOO. It's safe here because src/precompiles re-exports nothing prefixed, though a short comment recording that assumption would help the next editor.
Fixes PLT-848.
Problem
src/ethers/soloPrecompile.tsimplementsETHERS_SOLO_PRECOMPILE_ABIandgetSoloPrecompileEthersV6Contract, butsrc/ethers/index.tsnever re-exported it. Solo is exported from theprecompiles/andviem/barrels, so the omission was ethers-only and unintentional.This is present in published
2.1.2, sogetSoloPrecompileEthersV6Contracthas never been reachable from the package root for ethers consumers — the file ships indist/*/ethers/soloPrecompile.jsbut nothing re-exported it.The existing suite masked it:
ethersPrecompiles.spec.tsimported solo via a direct module path (from '../soloPrecompile') rather than through the barrel, so it passed.Changes
export * from './soloPrecompile';tosrc/ethers/index.ts.src/__tests__/barrelParity.spec.ts, asserting theprecompiles,ethers, andviembarrels expose the same set of precompiles. This is the actual root cause and it would have recurred on the next precompile.The parity test derives names from each barrel's
*_PRECOMPILE_ABIconstants rather than hardcoding a count, so it stays correct as precompiles are added or removed. It also asserts the base barrel is non-empty, otherwise the three comparisons would pass vacuously on three empty sets if the naming convention ever changed.Verification
barrelParity.spec.tsfail (2 of 4 assertions), confirming the guard actually catches this bug rather than just passing alongside the fix.import { getSoloPrecompileEthersV6Contract } from '@sei-js/precompiles'verified against a realnpm packtarball: resolves, and the returned contract targets0x…100C.tsc --noEmitclean, 41/41 jest tests pass, biome clean, full build clean.Note, not fixed here
@sei-js/precompiles/ethers— the subpath used throughout our JSDoc examples — does not resolve at all from a packed tarball (MODULE_NOT_FOUND). The package has noexportsmap and noethers/directory at the package root. That is a separate packaging bug with wider blast radius, so it is out of scope for this PR.Made with Cursor