feat(registry)!: remove legacy registry data - #329
Conversation
Align the package with IBC deprecation and the latest upstream chain registry metadata. 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 #329 +/- ##
==========================================
+ Coverage 81.07% 81.10% +0.02%
==========================================
Files 73 72 -1
Lines 3752 3757 +5
==========================================
+ Hits 3042 3047 +5
Misses 710 710
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A clean, well-scoped breaking change: IBC exports and data are removed, the chain-registry submodule is bumped, and WALLETS is added to the package root — all correctly covered by a major changeset, with tsconfig.json and scripts/build-registry.ts updated to match the new import graph. No correctness or security blockers; the notes below are about duplicated filter logic and tests that pin exact upstream submodule values.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty); Codex reported no material issues. This review is therefore effectively single-source on the second-opinion axis. - I could not verify the new chain-registry submodule contents in this environment (submodules are not checked out and network access was unavailable), so the asserted endpoints, gas prices, and wallet list in the updated tests could not be checked against commit
855440d. Per the repo guidelines on hand-maintained/upstream registry data, worth a maintainer confirming the built artifact matches upstream before merge — the PR test plan says this was done. - The IBC-filter predicate (
isIbcDenomination/isIbcAsset) and its metadata interface are now duplicated verbatim inpackages/registry/src/tokens/index.tsandscripts/build-registry.ts. Both copies are genuinely needed (build-time to shrink the bundled JSON, runtime sobun test srcand direct-src consumers see the filtered list), but they must stay in lockstep — a change to one silently divergesdistfromsrc. packages/registry/tsconfig.jsonnow enumerates the chain-registry JSON files by name instead of globbing. That's the right call for excludingibc_info.json, but it becomes a list that has to be updated by hand whenever a new registry JSON is imported. Low risk since the failure is a loud type error, just worth knowing.- The new
excludes IBC assetstest insrc/tokens/__tests__/index.spec.tsre-implements the exact same predicate asisIbcAsset, so it confirms the filter was applied but cannot catch a wrong definition of "IBC asset". Consider asserting against a known-IBC base denom from the asset list instead. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| it('contains specific wallet extension by identifier', () => { | ||
| const identifierToCheck = 'compass'; // Example identifier | ||
| it('contains only the current wallet identifiers', () => { | ||
| expect(WALLETS.map(({ identifier }) => identifier)).toEqual(['metamask', 'keplr', 'coin98']); |
There was a problem hiding this comment.
[suggestion] Exact-array toEqual on upstream submodule data pins both the full set and the ordering of wallets.json. Any wallet added, removed, or reordered upstream fails this test on an otherwise-routine submodule bump, in a package whose whole job is to track upstream. Consider asserting the invariant instead:
const identifiers = WALLETS.map(({ identifier }) => identifier);
expect(identifiers).toEqual(expect.arrayContaining(['metamask', 'keplr', 'coin98']));Same applies to the CHAIN_INFO.supported_wallets toEqual in chain-info/__tests__/index.spec.ts.
|
|
||
| it('contains the current RPC, EVM, and explorer metadata', () => { | ||
| const mainnet = NETWORKS['pacific-1']; | ||
| expect(mainnet.rpc.some(({ provider, url }) => provider === 'Rhino' && url === 'https://rpc.sei-apis.com')).toBeTrue(); |
There was a problem hiding this comment.
[suggestion] These assertions hardcode specific third-party provider names and URLs (Rhino/rpc.sei-apis.com, dRPC/sei.drpc.org, Seistream). If an upstream provider is swapped or an endpoint domain changes, CI breaks on a data refresh rather than on a code defect. A structural check — every rpc/evm_rpc entry has a non-empty provider and a parseable https:// (or wss://) url, and explorers is non-empty — would catch real regressions without coupling the suite to a specific vendor list.
| expect(pacific1.min_gas_price).toBeGreaterThanOrEqual(0.01); | ||
| expect(pacific1.module_adjustments.dex.sudo_gas_price).toBeLessThanOrEqual(0.02); | ||
| expect(pacific1.min_gas_price).toBe(0.02); | ||
| expect(GAS_INFO['atlantic-2'].min_gas_price).toBe(0.08); |
There was a problem hiding this comment.
[nit] Worth a sanity check on the source: atlantic-2 at 0.08 is 4x the pacific-1 value of 0.02, and a testnet minimum gas price above mainnet's is unusual enough to be worth confirming against gas.json at the new submodule commit before this becomes the pinned expectation. I couldn't verify it here (submodules not checked out). If the values are correct, toBeGreaterThan(0) plus a mainnet-specific exact check would survive future upstream tuning.
|
|
||
| const isIbcDenomination = (denomination: string): boolean => denomination.toLowerCase().startsWith('ibc/'); | ||
|
|
||
| const isIbcAsset = (asset: RegistryAssetMetadata): boolean => |
There was a problem hiding this comment.
[suggestion] This is a byte-for-byte copy of isIbcAsset / isIbcDenomination / the asset-metadata interface in packages/registry/src/tokens/index.ts. The script already imports CHAIN_IDS from ../packages/registry/src/supported-networks, so it can import the predicate the same way — export isIbcAsset from a shared module (e.g. alongside supported-networks.ts) and have both call sites use it. Otherwise a future change to what counts as an IBC asset gets applied in one place and the bundled dist diverges from the src the tests run against.
| coingecko_id?: string; | ||
| /** The type of the token, if applicable (e.g., "cw20" for CosmWasm tokens). */ | ||
| type_token?: string; | ||
| type_asset?: string; |
There was a problem hiding this comment.
[nit] The doc comment still describes the old type_token semantics. type_asset in the Cosmos asset-list schema carries a defined set of values (sdk.coin, cw20, erc20, ics20, …), and ics20 in particular is now the value this package filters on — worth saying so here, since it documents the exclusion rule for consumers.
Stop exporting and bundling obsolete chain gas information from the registry package. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview Refreshes bundled chain registry data (RPC/EVM/explorers, supported wallets) and re-exports Reviewed by Cursor Bugbot for commit c37bf61. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
A clean, well-scoped breaking change to @sei-js/registry with a correct major changeset, no orphaned consumers of the removed exports, and a tsconfig include list that still covers every JSON import. No blockers; the notable non-blocking items are a JSDoc block that now documents an internal const instead of TOKEN_LIST, isIbcAsset duplicated between the build script and package source, and new tests that snapshot upstream submodule data by exact value and order.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wallet-list shrinkage is worth confirming: the new tests pin
WALLETSto exactly['metamask', 'keplr', 'coin98']andCHAIN_INFO.supported_walletsto['keplr', 'coin98'], which means the submodule bump drops Compass Wallet (the old test asserted it was present). Compass is a widely used Sei wallet, and any dApp rendering a picker fromWALLETSsilently loses it on upgrade. The changeset covers this only as a generic "refresh ... wallets". Please confirm this matchessei-protocol/chain-registryHEAD and, if intentional, call the removal out explicitly in the changeset so it lands in the release notes. - The
chain-registry/community-assetlistsubmodules are not checked out in the review environment, so I could not independently verify the refreshed data values (RPC/EVM URLs, explorer names, wallet entries) that the new tests assert against. Per the repo guidelines I'm raising these as questions rather than asserting the values are wrong. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only the Codex pass and my own analysis. - The OpenAI Codex pass reported "No material issues found in the pull request diff" — no findings to merge from it.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| * ``` | ||
| */ | ||
| export const TOKEN_LIST: SeiTokens = pickSupportedNetworks(TokenListJSON) as unknown as SeiTokens; | ||
| const supportedTokenList = pickSupportedNetworks(TokenListJSON); |
There was a problem hiding this comment.
[suggestion] The TOKEN_LIST JSDoc block (lines 71-84, with the @remarks community-data warning and the @example) is now attached to the internal supportedTokenList const rather than to the exported TOKEN_LIST on line 87. IDE hover and generated docs for the public export will show nothing, and the "verify and filter tokens yourself" warning — the one piece of documentation consumers most need — disappears from the published API surface.
Move the doc block down so it sits directly above export const TOKEN_LIST, and put a short internal comment on supportedTokenList if one is wanted.
|
|
||
| const isIbcDenomination = (denomination: string): boolean => denomination.toLowerCase().startsWith('ibc/'); | ||
|
|
||
| const isIbcAsset = (asset: RegistryAssetMetadata): boolean => |
There was a problem hiding this comment.
[suggestion] isIbcAsset / isIbcDenomination / the asset-metadata interface are byte-for-byte duplicates of packages/registry/src/tokens/index.ts:12-15. Both copies have to agree or the published bundle (pre-filtered here at build time) and source consumers / the test suite (filtered at runtime in tokens/index.ts) will disagree about what counts as an IBC asset — a drift that nothing in CI would catch, since the tests import the source path only.
This script already imports CHAIN_IDS from ../packages/registry/src/supported-networks; the same trick works here. Export isIbcAsset from the package (or a small shared module) and import it, so there is one definition of the filter.
| it('contains specific wallet extension by identifier', () => { | ||
| const identifierToCheck = 'compass'; // Example identifier | ||
| it('contains only the current wallet identifiers', () => { | ||
| expect(WALLETS.map(({ identifier }) => identifier)).toEqual(['metamask', 'keplr', 'coin98']); |
There was a problem hiding this comment.
[suggestion] This asserts exact array equality and ordering against data vendored from the chain-registry submodule. Same pattern at chain-info/__tests__/index.spec.ts:21 (supported_wallets toEqual ['keplr', 'coin98']) and, more loosely, networks/__tests__/index.spec.ts:32-41 (specific provider names and RPC/WS URLs).
The repo guidelines note the submodule JSON is vendored upstream and that review should target the TypeScript wrappers, not the data. These tests invert that: they turn the suite into an upstream-change detector, so a routine registry refresh that adds a wallet, reorders providers, or rotates an endpoint URL breaks CI on an unrelated PR — with a failure message that points at nothing the author changed.
Suggest asserting the wrapper's contract instead: that WALLETS is non-empty, that every entry has the required fields (already covered by the test above), and expect(identifiers).toContain('keplr') for the specific wallets you care about keeping.
| const isIbcDenomination = (denomination: string): boolean => denomination.toLowerCase().startsWith('ibc/'); | ||
|
|
||
| const isIbcAsset = (asset: AssetMetadata): boolean => | ||
| isIbcDenomination(asset.base) || asset.denom_units.some(({ denom }) => isIbcDenomination(denom)) || asset.type_asset?.toLowerCase() === 'ics20'; |
There was a problem hiding this comment.
[nit] asset.base.toLowerCase() and asset.denom_units.some(...) are unguarded. This runs at module-import time over community-maintained upstream data, so a single asset entry missing base or denom_units throws a TypeError that takes down the whole @sei-js/registry import rather than just skipping that asset. The build script hits the same access first, so it would surface at build time — but as a bare TypeError with no indication of which asset or file is at fault.
asset.base?.toLowerCase() / asset.denom_units?.some(...) (or a ?? []) costs nothing and keeps the failure mode contained.
| coingecko_id?: string; | ||
| /** The type of the token, if applicable (e.g., "cw20" for CosmWasm tokens). */ | ||
| type_token?: string; | ||
| type_asset?: string; |
There was a problem hiding this comment.
[nit] The doc comment above still reads (e.g., "cw20" for CosmWasm tokens), carried over from type_token. Under the Cosmos asset-list schema type_asset takes values like sdk.coin, ics20, erc20, and cw20 — worth listing a couple of those, especially since ics20 is now the value the new filter keys off.
2 participants
Summary
Test plan
bun run checkbun run buildbun run testbun run lint:pack:all