Skip to content

Parse decimal and hexadecimal owner token IDs exactly - #16

Merged
GelatoGenesis merged 2 commits into
mainfrom
fix/emma-token-id-parsing
Sep 22, 2026
Merged

GelatoGenesis merged 2 commits into
mainfrom
fix/emma-token-id-parsing

Conversation

@GelatoGenesis

@GelatoGenesis GelatoGenesis commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Issue

Historical Alchemy owner snapshots interpreted every token ID as hexadecimal. A decimal response for token 10 therefore became token 16, producing empty or incorrect allowlists.

Fix

Parse decimal IDs as decimal and use hexadecimal only with an explicit 0x prefix. Preserve BigInt precision and reject malformed or out-of-range uint256 IDs instead of truncating them.

Changes

  • Add regression coverage for decimal/hex formats, precision, uint256 boundaries, malformed values, and paginated REST responses.
  • Run existing CI on pull requests as well as main.
  • Refresh CSV lockfile resolutions to remove the moderate advisory blocking the existing audit gate, and resolve existing lint findings.
  • Document accepted owner ID formats. No public API or storage schema changes.

Validation

  • Full library suite: 41 suites, 428 tests passed, two skipped.
  • Lint, build, dependency audit, release automation tests, and package compatibility checks passed.
  • Package checks cover CommonJS, ESM, legacy imports, TypeScript, and the quick start.
  • Existing network E2E cases are skipped by the repository; live EMMA staging snapshot verification is tracked with the companion API fix.

Risk

  • Level: Low.
  • Why: Hexadecimal input retains its meaning; decimal input now preserves its original token ID. Invalid values fail visibly.
  • Rollback: Revert the parser change in a subsequent package release.

Review Notes

The companion API adapter normalizes IDs to explicit hex, so EMMA can retain its tested dependency while this package is released independently.

Companion API fix and staging deployment evidence: 6529-Collections/allowlist-api#74. The reviewed change is merged into staging and main; 6529bot follow-up reports no new findings and all checks are green.

Published as 0.0.137 by the validated npm release workflow. Registry version and artifact checksum were verified after publication.

Signed-off-by: GelatoGenesis <1407802+GelatoGenesis@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: fab8cdd4-5b26-4522-b4eb-5c381af4d479


Comment @coderabbitai help to get the list of available commands.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot WCAG 2.2 AA analysis - dbdf509

Verdict: No WCAG findings

This PR is a backend/library change (token ID parsing, dependency bumps, test and CI updates) in allowlist-lib. There is no user-facing UI code, component markup, or accessibility surface in the changed files, so WCAG 2.2 AA criteria do not apply here.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot general PR review - dbdf509

Verdict: Good to merge

The core change is a genuine correctness fix. The old code always prepended 0x and truncated to 64 hex chars, so it silently reinterpreted decimal IDs as hex and could corrupt large IDs. The new normalizeTokenId correctly distinguishes decimal from 0x-prefixed hex, rejects malformed/out-of-range input, and preserves uint256 precision via BigInt. Test coverage for both accepted and rejected cases is thorough.

Nice-to-have

  • src/services/alchemy.service.ts:9 — the typeof tokenId !== 'string' guard is unreachable given the declared tokenId: string parameter type, though it usefully hardens against untyped runtime input (which the tests exercise with null/undefined/10). Consider widening the parameter to unknown so the guard's intent matches the signature and the compiler doesn't treat the branch as dead.
  • src/services/alchemy.service.ts:16 — BigInt(2) ** BigInt(256) is recomputed on every token. Hoisting it to a module-level constant (e.g. const UINT256_LIMIT = 2n ** 256n) is a trivial micro-optimization and reads more clearly.

No blocking or correctness concerns. The dependency bumps (csv and friends) and the CI pull_request trigger are reasonable and unrelated to the parsing fix.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot i18n analysis - dbdf509

Verdict: No i18n findings

This PR is a backend/library change in allowlist-lib (token ID parsing logic, dependency bumps, test updates, and CI configuration). The only user-facing strings touched are:

  • README.md:97-100 — developer-facing documentation, not localizable app UI.
  • src/services/alchemy.service.ts:12,17 — 'Invalid Alchemy token ID' is a thrown Error message for library consumers/logs, not end-user React copy on a localizable surface.

There is no React UI, no i18n/messages surface, and no locale-sensitive formatting in the changed files, so the progressive i18n standard does not apply here.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot crypto security analysis - dbdf509

Verdict: No security findings

The core change to normalizeTokenId (src/services/alchemy.service.ts:7-20) is a genuine correctness/robustness improvement for token ID handling, and I found no exploitable crypto/web3 issue in the changed code:

  • The regex ^(?:\d+|0x[0-9a-f]+)$ is fully anchored, so whitespace, signs, decimals, scientific notation, and non-hex garbage are rejected before BigInt sees them — the previous silent decimal→hex reinterpretation and 64-char truncation (which could corrupt or collide token IDs) is gone.
  • The upper-bound check value >= 2n ** 256n correctly rejects out-of-range uint256 values, and BigInt preserves full precision (no float coercion). Tests cover the boundary cases well.
  • No signature, auth, session, or nonce surface is touched. balance still uses +token.balance, but that is unchanged by this PR and out of scope.

One CI note worth flagging for maintainers (not a code-level finding in the reviewed source): adding a pull_request trigger (.github/workflows/main.yml:4-6) while the workflow injects ETHERSCAN_API_KEY/ALCHEMY_API_KEY and DISCORD_WEBHOOK (lines 12-13, 60) means fork PRs could gain a path to those secrets. GitHub does not expose secrets to pull_request runs from forks by default, so this is safe as configured; just avoid switching to pull_request_target or running untrusted PR code with these secrets in scope.

Nothing blocking from a crypto/security standpoint.

Signed-off-by: GelatoGenesis <1407802+GelatoGenesis@users.noreply.github.com>
@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - e4471e8

Verdict: No new findings

The two follow-up commits directly address the prior nice-to-have suggestions and introduce no regressions.

Resolved since last review

  • src/services/alchemy.service.ts:9 — the guard's intent now matches the signature: normalizeTokenId accepts unknown, so the typeof tokenId !== 'string' branch is no longer dead code. Tests exercise null/undefined/10 inputs.
  • src/services/alchemy.service.ts:4,18 — 2 ** 256 is now hoisted to the module-level UINT256_LIMIT constant instead of being recomputed per token.

Both changes preserve correct behavior: the regex remains fully anchored, BigInt precision is intact, and the boundary/out-of-range test cases still pass. The remaining touched files (README doc, dependency bumps, test mock cleanups, whitespace formatting) are cosmetic or unrelated to the parsing logic and carry no correctness concerns.

@sonarqubecloud

Copy link
Copy Markdown

@GelatoGenesis
GelatoGenesis merged commit c287a3e into main Sep 22, 2026
6 checks passed
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.

1 participant