Skip to content

fix(registry): refresh reviewed asset data - #342

Open
alexander-sei wants to merge 2 commits into
mainfrom
fix/registry-release-data
Open

fix(registry): refresh reviewed asset data#342
alexander-sei wants to merge 2 commits into
mainfrom
fix/registry-release-data

Conversation

@alexander-sei

Copy link
Copy Markdown
Collaborator

Summary

  • repin the community asset list to reviewed commit 964ca87f7cff8d8791ad1e994628fa410faae61e
  • validate and expose current non-IBC asset and pointer metadata through one shared source/build filter
  • enforce exact gitlinks, generated-artifact parity, canonical assets, and explicit retained-image release checks

Test plan

  • bun run check
  • bun run build
  • bun run test
  • bun run lint:pack:all
  • bun run --cwd packages/registry check:artifact
  • bun run --cwd packages/registry check:images — 48/48 URLs passed
  • exact staged gitlink and checkout verification
  • focused review loop completed with no remaining findings

Made with Cursor

Ship current non-IBC metadata from an exact reviewed gitlink and make schema, artifact, submodule, and image verification reproducible.

Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov-commenter

codecov-commenter commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.70%. Comparing base (6fade5f) to head (96fe0f2).

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #342    +/-   ##
========================================
  Coverage   99.69%   99.70%            
========================================
  Files          64       66     +2     
  Lines        4293     4412   +119     
========================================
+ Hits         4280     4399   +119     
  Misses         13       13            
Flag Coverage Δ
mcp-server 99.57% <ø> (ø)
precompiles 100.00% <ø> (ø)
registry 100.00% <100.00%> (ø)
sei-global-wallet 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-executed registry repin: the IBC/ICS-20 filter is now genuinely shared between src/tokens/index.ts and the esbuild plugin (removing the previous duplicate-logic drift risk), the gitlink/remote checks match .gitmodules exactly, and a minor changeset is present. No blockers; the notes below are about validator strictness that will bite at the next repin, and about coupling live git state and a full build into the unit-test scripts.

Findings: 0 blocking | 12 non-blocking | 6 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Cursor's second-opinion pass (cursor-review.md) produced no output — the file is blank. Codex reported no material issues. Only this pass has substantive findings, so treat the multi-tool coverage as thinner than usual.
  • TOKEN_LIST entries are now typed RegistryToken with type_asset: AssetType (a closed union), narrowed from the previous type_asset?: string. This is source-breaking for TS consumers who compare type_asset against a literal outside the union (TS2367) or who assign hand-built Token values into a SeiTokenList. The changeset is minor, which is defensible given the README migration notes, but the narrowing is worth calling out in the release notes explicitly.
  • verify:release-data runs check:submodules and then test, which runs check:submodules again. Harmless, but the duplication suggests the gate belongs in one place.
  • check:images has no retry, so a single transient 429/503 from a CDN fails the whole release gate for 48 URLs. A one-shot retry on 5xx/429 would make the gate less flaky without weakening it.
  • Hard-coded counts are duplicated across three places (README 53/9/46/7/48, RUNBOOK, and index.spec.ts/registry-release.test.ts). The tests enforce them, but the two docs will silently drift at the next repin — the RUNBOOK's repin checklist should list the doc numbers as required edits alongside REVIEWED_ASSETLIST_REVISION.
  • Guideline §2 (hand-maintained addresses are source of truth) applies to the canonical-asset fixtures in index.spec.ts — WSEI 0xE30f…e8C7, USDC 0xe15f…2392, USDT0 0x9151…EcC5, WETH 0x1603…42d8, fastUSD 0x37a4…5269. I could not verify these from the diff since the submodule isn't checked out in this environment; they now act as pinned assertions, so please confirm each against Seiscan/docs.sei.io before merge.
  • 6 suggestion(s)/nit(s) flagged inline on specific lines.

};
});

const images = parseImages(value.images, `${path}.images`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] parseImages starts with assert(isRecord(value), ...), so an upstream asset with no images key now throws and fails the entire build/import rather than yielding {}.

That's a tolerance regression: the old code did no validation at all, and the test this PR replaces guarded with if (asset.images) — evidence that upstream has historically shipped assets without the key. images is also absent from the Draft-07 required set in most assetlist schemas.

Suggest making it tolerant, matching how coingecko_id and pointer_contract are handled:

const images = value.images === undefined ? {} : parseImages(value.images, `${path}.images`);

Same question for description and display via requiredString — if upstream ever omits either on a new asset, the repin fails wholesale instead of surfacing one bad entry.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retained intentionally: the pinned reviewed schema and existing public Token contract require images/description/display. Unknown or missing required metadata fails loudly at repin instead of silently weakening published data; RUNBOOK now documents this policy.

Comment thread packages/registry/src/tokens/filter.ts Outdated
const denomUnits = value.denom_units.map((unit, index) => {
const unitPath = `${path}.denom_units[${index}]`;
assert(isRecord(unit), `${unitPath} must be an object`);
assertOnlyKeys(unit, new Set(['denom', 'exponent']), unitPath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This restricts denom units to exactly denom and exponent, but DenomUnit in types.ts:29 declares aliases?: string[] with the comment "Optional aliases retained for compatibility with registry consumers."

Those two contradict: aliases can never be populated (the parser strips it and there's no path that sets it), and if upstream adds aliases to any denom unit the build throws contains unsupported property "aliases". Either allow and parse it, or drop the field from the type so the declared shape matches what's actually produced.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 96fe0f2: denomination aliases are validated as string arrays, preserved in output, and covered by valid/invalid tests.

const ASSET_TYPE_SET: ReadonlySet<string> = new Set(ASSET_TYPES);
const POINTER_CONTRACT_TYPE_SET: ReadonlySet<string> = new Set(POINTER_CONTRACT_TYPES);
const NON_FUNGIBLE_ASSET_TYPES: ReadonlySet<AssetType> = new Set(['cw721', 'erc721', 'erc1155']);
const TOKEN_KEYS = new Set(['name', 'description', 'symbol', 'base', 'display', 'denom_units', 'images', 'coingecko_id', 'type_asset', 'pointer_contract']);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The combination of assertOnlyKeys(value, TOKEN_KEYS, path) and a closed ASSET_TYPES union means any additive upstream change — a new metadata field like traces/deprecated, or a new asset type such as erc4626 — fails the whole TOKEN_LIST rather than one asset. Because parseTokenList iterates every key in the source, that's true even for a network the package never exports.

Given the submodule is pinned and the RUNBOOK makes repinning a deliberate reviewed step, fail-loud-at-repin is a reasonable choice — just flagging that the blast radius is the entire build, and that reviewers will need the RUNBOOK's schema-diff step to distinguish "upstream added a field" from "upstream broke".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed intentional: the source is commit-pinned and reviewed, so additive unknown fields/types fail loudly until public types, validation, tests, and docs are updated together. The runbook now states this explicitly.

Comment thread packages/registry/package.json Outdated
"check:artifact": "bun ../../scripts/check-registry-artifact.ts",
"check:images": "bun ../../scripts/check-registry-images.ts",
"check:submodules": "bun ../../scripts/check-registry-submodules.ts",
"test": "bun run check:submodules && bun test --isolate src",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Folding check:submodules into test makes the unit-test suite depend on live git state: the exact REVIEWED_ASSETLIST_REVISION, a clean submodule worktree under --untracked-files=all, and matching origin URLs.

Two consequences: (1) the next legitimate repin breaks bun test until the script constant is edited in the same commit — intentional per the RUNBOOK, but it turns a data-provenance gate into a test failure; (2) a stray untracked file inside either submodule fails the entire registry suite with a git error rather than a test result.

The gate is already reachable via verify:release-data, and coverage.yml invokes bun test packages/registry/src directly and therefore bypasses it anyway. Consider keeping test as bun test --isolate src and enforcing check:submodules as its own step in checks.yml/release.yml.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 96fe0f2: package tests are hermetic again; Checks and Release run explicit submodule identity gates where recursive checkouts are guaranteed.

Comment thread scripts/registry-release.test.ts Outdated
expect(urls.every((url) => url.startsWith('https://'))).toBeTrue();
});

test('generates a bundle identical to the filtered submodule source', async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This test spawns a full bun run --cwd packages/registry build (tsc + esbuild), which now runs as part of the root bun run test via test:scripts. Two side effects worth avoiding in a unit suite: it writes to packages/registry/dist (the build does rm -rf dist first, so a concurrent bun run build in CI or locally will race), and it makes test:scripts require the community-assetlist submodule, which it previously did not.

The artifact-parity check is a release concern and verify:release-data already covers it. Suggest excluding this one case from test:scripts (separate file/target) and keeping the pure-fixture submodule tests in the default suite.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 96fe0f2: default script tests no longer build or mutate dist; artifact parity runs explicitly after build in Checks/Release and the manual release gate.

export const TOKEN_LIST: SeiTokens = Object.fromEntries(
Object.entries(supportedTokenList).map(([network, assets]) => [network, assets.filter((asset) => !isIbcAsset(asset))])
) as unknown as SeiTokens;
export const TOKEN_LIST: SeiTokenList = filterTokenList(TokenListJSON, 'community-assetlist/assetlist.json');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] In the published bundle this re-runs the full validator over data the esbuild plugin already produced with the same filterTokenList call, so every consumer pays a redundant parse of ~53 assets at import and ships the validator + ASSET_TYPES/assertOnlyKeys machinery in the bundle.

It does buy the source/artifact symmetry that check:artifact asserts, so this may be deliberate — but if bundle size or import cost matters, the plugin could emit the already-validated JSON and the runtime could cast, with check-registry-artifact.ts remaining the guarantee that the two agree.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retained deliberately for this small pinned data set: runtime source and generated bundle enforce the same schema, and artifact parity tests guard that contract.

Keep unit tests hermetic while making exact gitlinks, artifact parity, aliases, and transient image failures explicit release checks.

Co-authored-by: Cursor <cursoragent@cursor.com>

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured registry refresh: the IBC/ICS-20 filter is now shared by the source module and the esbuild plugin, the asset-list gitlink is pinned to a reviewed SHA and enforced in CI, and a changeset is present. No blockers; the notes below are about validator strictness scope, retry semantics, and the fact that the actual data change isn't visible in this diff.

Findings: 0 blocking | 9 non-blocking | 4 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Second-opinion passes: cursor-review.md is empty (that pass produced no output). codex-review.md reports no material issues.
  • The substance of this PR — the community asset-list content going from 19 to 46 retained mainnet assets — is a gitlink bump (831406ba964ca87f) and is not reviewable from the diff. The review rests on the pinned SHA plus the deterministic count/canonical-asset assertions. I spot-checked the canonical addresses asserted in the test (WSEI 0xE30feDd1…, native USDC 0xe15fC38F…, USDT0 0x9151434b…, Stargate WETH 0x160345fC…) against known Sei mainnet deployments and they match; the remaining ~40 addresses were not independently verified, which is worth a human eye given @sei-js/registry feeds contract addresses to downstream dApps.
  • Every repin now requires a coordinated edit across REVIEWED_ASSETLIST_REVISION, the hard-coded counts in both test files (53/9, 46/7, 25/18 pointer contracts, 48 image URLs), the canonical-asset fixtures, README, and RUNBOOK. That is deliberate and documented in RUNBOOK.md, but it is a real maintenance cost and a source of future CI failures that look like bugs — worth keeping the RUNBOOK checklist authoritative.
  • check:images is intentionally kept out of CI (network-dependent) and only run manually via verify:release-data. Reasonable, but it means retained image links can rot silently between releases; the RUNBOOK instruction to re-run it close to publication is the only guard.
  • The changeset is minor. TOKEN_LIST element type narrows from Token (type_asset?: string) to RegistryToken (type_asset: AssetType). Reads stay source-compatible and arrays remain assignable to Token[], so minor is defensible — flagging only so it's a conscious call rather than an oversight.
  • 4 suggestion(s)/nit(s) flagged inline on specific lines.

* runtime and generated-artifact policies cannot drift.
*/
export function filterTokenList(source: unknown, sourceName = 'asset list'): SeiTokenList {
const parsed = parseTokenList(source, sourceName);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] filterTokenList parses every key in the upstream file before selecting the two supported networks, so validation strictness applies to data the package discards. Combined with assertOnlyKeys (unknown properties are a hard error) and the Array.isArray(assets) assertion in parseTokenList, an upstream change that only touches an unsupported network — a new devnet key, a new metadata field on a devnet asset, or a non-array top-level key such as $schema — will fail bun run build and throw at package import, even though none of that data is retained.

Consider narrowing the parse to the supported networks (select first, then parseToken the retained entries) so the strict schema contract applies only to data actually shipped. If validating everything is the intent (the RUNBOOK reads that way), a one-line comment here saying so would save the next person diagnosing an unrelated-looking build break.

assertOnlyKeys(unit, new Set(['denom', 'exponent', 'aliases']), unitPath);
assert(typeof unit.denom === 'string', `${unitPath}.denom must be a string`);
assert(Number.isInteger(unit.exponent) && Number(unit.exponent) >= 0, `${unitPath}.exponent must be a non-negative integer`);
if (index === 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This requires the first denom_units entry to be the exponent-0 base unit. That holds at the current pin and matches convention, but it's an ordering constraint the upstream Draft-07 schema doesn't guarantee — a repin that emits the display unit first would hard-fail both the build and consumer import. If the intent is "exactly one unit has exponent 0", asserting that over the array rather than on index === 0 would be equally strict without depending on ordering.


return `HTTP ${response.status}${response.statusText ? ` ${response.statusText}` : ''}: ${url}`;
}
} catch (error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The try wraps the whole retry loop, so a thrown error — DNS failure, connection reset, or the AbortSignal.timeout firing — returns immediately with no retry, while a 429/5xx response gets one. Timeouts are at least as transient as a 503 on a per-release link check across 48 external hosts. Moving the try inside the loop and retrying on a thrown error for attempt === 0 would make the two paths consistent. (Related: the final return after the loop on line 79 is unreachable — every path inside the loop returns or continues into the second iteration, which always returns.)

export const TOKEN_LIST: SeiTokens = Object.fromEntries(
Object.entries(supportedTokenList).map(([network, assets]) => [network, assets.filter((asset) => !isIbcAsset(asset))])
) as unknown as SeiTokens;
export const TOKEN_LIST: SeiTokenList = filterTokenList(TokenListJSON, 'community-assetlist/assetlist.json');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The esbuild plugin already replaces assetlist.json with the output of filterTokenList, so this call re-validates data that was validated and filtered at build time and cannot change afterwards. Net effect for consumers: the full validator (~180 lines of filter.ts) is bundled into dist/index.js and runs on every import for a guaranteed no-op. The RUNBOOK states this is deliberate so source execution and the bundle enforce the same schema — worth it for source runs, but if published bundle size or cold-import cost matters you could alias the runtime filter to identity in the esbuild build while keeping the source path unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants