Skip to content

Fix decimal owner token IDs in EMMA snapshots - #74

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

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

Conversation

@GelatoGenesis

@GelatoGenesis GelatoGenesis commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Issue

Alchemy owner responses can contain decimal token IDs. Published allowlist-lib 0.0.136 treats every ID as hexadecimal, so token 10 becomes 16 and EMMA snapshots can be empty or select the wrong holders.

Fix

Validate owner IDs as uint256 values and normalize decimal or explicitly prefixed hexadecimal input to canonical hexadecimal at the Alchemy adapter boundary. This works with both the current published library and its corrected parser, allowing an independent rollout without a package release.

Changes

  • Cover decimal, hex, large IDs, invalid IDs, pagination, and real token-pool selection through the published library.
  • Add validated staging and production service selectors so each dispatch deploys and smoke-tests exactly one Lambda. Production dispatches require main; default CLI behavior remains unchanged.
  • Document deployment order and snapshot recreation. No database migrations, API schema changes, or frontend changes.

Validation

  • Full API suite: 14 suites, 118 tests passed; lint and build passed.
  • Five deployment-selector tests passed.
  • Production and full dependency audits passed their high/critical gates (one existing moderate csv-parse advisory).
  • Lambda packaging and handler/artifact verification passed.
  • Live read-only integration through this adapter and published library matched every wallet balance at block 26031727: token 10 had 344 wallets / 469 tokens; token 16 had 425 wallets / 569 tokens. This local harness uses in-memory storage, known ERC1155 schema, and no sanctions filtering; deployed configuration is unchanged.
  • Staging deployment and cold-start smoke checks passed sequentially: downloader, worker, API. All three use the same verified artifact checksum.
  • Staging core E2E passed after deployment: 79 passed, 33 skipped, including EMMA entry/wallet gate coverage.
  • Signed-in staging browser verification also passed: created a fresh The Memes token 10 snapshot at block 26031727 with consolidation off. The downloader completed on the fast path with 469 token ownerships; Run analysis displayed 344 wallets and 469 tokens. Left the wizard on step 2 without advancing.
  • Additional signed-in staging verification passed for BAYC (ERC721) token 1148 at the same block with consolidation off: fast-path download prepared one ownership, and analysis displayed one wallet / one token. The Memes snapshot remained in the plan; the wizard stayed on step 2.
  • Production deployment and cold-start checks passed sequentially: downloader, worker, API. All three are Active/Successful and their artifact checksum exactly matches staging. Public API metadata health check passed.
  • Additional read-only verification using production's Alchemy account matched all expected wallet balances for Memes 10 and 16 at block 26031727; BAYC 1148 returned one wallet / one token and its owner matched an on-chain ownerOf call at that block. This check exercised the adapter and pinned library locally against production provider data; the deployed artifact is the one verified above.

Risk

  • Level: Low.
  • Why: Provider-boundary normalization preserves exact token identity and accepts both supported formats. Existing stored snapshots are unchanged; affected snapshots must be recreated.
  • Rollback: Redeploy the previous API revision through the normal workflow. No data migration is required.

Review Notes

Focus on decimal 10 versus hexadecimal 0x10 and compatibility with the pinned library. Deploy downloader, worker, then API, waiting for each selected service to pass. The production workflow uses the same service selection already verified in staging; the application code and pinned dependency are unchanged by that workflow follow-up.

Companion parser correction: 6529-Collections/allowlist-lib#16. 6529bot follow-up review reports no new findings after its robustness suggestions were addressed.

The library correction is published as 0.0.137. EMMA retains the exact dependency used in staging; the compatible adapter delivers the fix independently. Existing stored snapshots are unchanged and affected empty snapshots must be recreated.

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: 0ab867ef-497b-4cb9-98a9-0516ae814b1a


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 - 58db474

Verdict: No WCAG findings

This PR is a backend/CI change: token ID parsing in the Alchemy adapter, Lambda deployment selection scripts, GitHub Actions workflow inputs, tests, and README documentation. There is no user-facing UI code (no markup, components, styling, or interactive controls) in the changed files, so there are no WCAG 2.2 AA success criteria to evaluate here.

No coverage claim is made for UI areas outside this diff.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot i18n analysis - 58db474

Verdict: No i18n findings

This is a backend/infrastructure PR (Alchemy API adapter token ID parsing, Lambda deploy tooling, CI workflows, README). No user-facing React UI copy, locale-sensitive formatting, or accessible-name changes are touched, and there is no frontend i18n path in scope.

The changed user-facing strings that do exist are operational/developer-facing only and outside the i18n standard:

  • UpstreamProviderError message 'Invalid Alchemy owners token ID' (src/alchemy-api/alchemy-api.client.ts:204) is a server-side provider error, not localized UI copy.
  • GitHub Actions workflow_dispatch input description and Discord notification titles (.github/workflows/staging.yml:7, :81, :90) are CI-operator text, not app UI.
  • README additions are documentation.

No action needed from an i18n/localization standpoint.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot general PR review - 58db474

Verdict: Good to merge

The core fix is sound: token IDs are validated as strings against isTokenId, range-checked against uint256, and normalized to canonical 0x hex. This resolves the decimal/hex ambiguity (decimal 10 no longer collides with hex 16) and works with both the current and corrected library.

Nice-to-have

  • src/alchemy-api/alchemy-api.client.ts:197 — The BigInt(value) >= 2n ** 256n check recomputes 2n ** 256n per call. Minor, but consider hoisting it to a module constant for clarity.

  • src/alchemy-api/alchemy-api.client.ts:185-190 — owner.tokenBalances.map(...) assumes tokenBalances is always an array. The response validation at line 181 only checks ownerAddresses is an array; an owner entry missing tokenBalances (or a non-array value) would throw a raw TypeError rather than a controlled UpstreamProviderError. The spec exercises well-formed shapes only. If Alchemy can omit tokenBalances, consider guarding it the same way malformed token IDs are handled.

  • scripts/lambda-services.cjs:2 — selectFunctions accepts undefined for the all-services path, but process.argv[3] yields undefined when absent, so this is correct. Note the test at line 22 covers empty string rejection, but the workflow always passes a required, constrained choice input — the defensive throw is appropriate belt-and-suspenders. No change needed.

The added concurrency group with cancel-in-progress: false on the staging workflow correctly serializes deploys, and the ordering guidance in the README matches the downloader→worker→API dependency direction. Test coverage for the parsing change and deployment selection is thorough.

@6529bot

6529bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

6529bot crypto security analysis - 58db474

Verdict: No security findings

The token ID normalization is the security-relevant change, and it holds up:

  • isTokenId (src/alchemy-api/alchemy-api.client.ts:232) enforces a strict ^(?:\d+|0x[0-9a-f]+)$ shape and rejects non-strings, so the subsequent BigInt(value) can only receive well-formed decimal or 0x hex. No leading/trailing whitespace, signs, exponents, or empty 0x slip through (confirmed by the malformed-input spec at lines 66–100).
  • The uint256 range guard BigInt(value) >= 2n ** 256n (:197) rejects out-of-range IDs before they are canonicalized, preventing overflow/aliasing of distinct on-chain token IDs.
  • Canonicalizing to 0x${BigInt(value).toString(16)} removes the decimal/hex ambiguity that previously let decimal 10 collide with hex 16 — this is the correct fix and closes a real snapshot-integrity bug. It works with both the published and corrected library, so ordering of deploys can't produce a silently wrong ownership set.
  • Failures raise a controlled UpstreamProviderError with a sanitized message rather than leaking upstream diagnostics.

On the CI/deploy path: the workflow_dispatch service input is a constrained choice and is passed via an env var (LAMBDA_SERVICE) quoted in the run command (.github/workflows/staging.yml:67-74), avoiding shell injection. selectFunctions (scripts/lambda-services.cjs) fails closed on any unknown selector, so a bad value never fan-outs to deploying all functions — a sensible guard against unintended production/staging changes.

No wallet, auth, signature, nonce, or chain-id logic is touched in this diff. The tokenBalances non-array edge case was already flagged in the prior general review and is a robustness (not security) concern, so I won't repeat it.

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 - 77de72c

Verdict: No new findings

The follow-up commit (77de72c) addresses the prior review's nice-to-have items cleanly.

Resolved since last review

  • The tokenBalances non-array/undefined-owner edge case flagged in the prior general and security reviews is now handled: getOwnersForContract guards each owner entry with !owner || !Array.isArray(owner.tokenBalances) and raises a controlled UpstreamProviderError instead of a raw TypeError (src/alchemy-api/alchemy-api.client.ts:187-196). The new spec cases (null, {}, { tokenBalances: null }, { tokenBalances: {} }) exercise this path (src/alchemy-api/alchemy-ownership-compatibility.spec.ts:102-113).
  • The repeated 2n ** 256n computation is hoisted to the UINT256_LIMIT module constant (src/alchemy-api/alchemy-api.client.ts:27, :210).

The token?.tokenId optional chaining (:201) correctly funnels missing token entries into normalizeOwnerTokenId, which rejects undefined via isTokenId, so nested malformed tokens still fail closed.

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 - 766a0da

Verdict: No new findings

The newest commit (766a0da) applies the single-service deployment pattern to production.yml, mirroring what was already reviewed for staging.yml. The changes are consistent and correct:

  • The production workflow adds the same constrained choice service input, concurrency group (allowlist-production-deploy, cancel-in-progress: false), and passes it via the quoted LAMBDA_SERVICE env var (.github/workflows/production.yml:68-75) — no shell injection, matching the staging pattern already validated.
  • The new if: github.ref == 'refs/heads/main' guard (.github/workflows/production.yml:29) is a sensible ref restriction for a manually dispatched production deploy, and the README accurately documents that non-main refs are rejected (README.md:105).
  • selectFunctions is already covered by the deployment-selection test that runs in the production job (.github/workflows/production.yml:52-53), so the fail-closed behavior on unknown selectors is exercised on the production path too.
  • README guidance (README.md:103-110) correctly describes the per-service dispatch flow for both environments and preserves the documented downloader→worker→API ordering.

No regressions relative to the prior same-lane review; the alchemy token-ID and tokenBalances handling are unchanged in this commit.

@sonarqubecloud

Copy link
Copy Markdown

@GelatoGenesis
GelatoGenesis merged commit 70a93c4 into main Sep 22, 2026
5 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