Skip to content

Conversation

@transphorm
Copy link
Member

@transphorm transphorm commented Aug 11, 2025

Summary

  • add tsup config to build core SDK into esm and cjs outputs
  • generate per-export package.json files and Metro shims
  • expand package exports and scripts for new build pipeline

Testing

  • corepack yarn workspace @selfxyz/core nice
  • corepack yarn lint (fails: exit code 0 after warnings)
  • corepack yarn workspace @selfxyz/core build
  • corepack yarn types (fails: interrupt, exit code 129)
  • corepack yarn workspace @selfxyz/core test (fails: missing script)

https://chatgpt.com/codex/tasks/task_b_68993d835a1c832d8a4bb944c54c5f1b

Summary by CodeRabbit

  • New Features

    • V2 identity verification hub support plus migration-aware hub adapters and a V2-focused hub client; expanded ABI and proof utilities exposed.
  • Improvements

    • Dual ESM/CJS builds, explicit exports map, Metro compatibility shims, and cleaner type-only imports to reduce runtime surface.
  • Documentation

    • Added a detailed migration guide for V1 → V2.
  • Chores

    • New SDK Core CI workflow with caching/quality checks, lint/build tooling, and safer network account handling.

@transphorm transphorm marked this pull request as draft August 11, 2025 02:24
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 11, 2025

Walkthrough

Adds a new SDK Core CI workflow; restructures sdk/core build to produce ESM and CJS artifacts with tsup and shims; migrates core verifier to Hub V2 (ABIs, adapters, client); converts several runtime imports to type-only; adds migration/post-build/shim/ABI scripts; updates package exports and CI/hardhat config.

Changes

Cohort / File(s) Summary
CI workflows
.github/workflows/sdk-core-ci.yml, .../.github/workflows/common-ci.yml
New "SDK Core CI" workflow scoped to sdk/core/** with build + quality-checks; common CI job adjusted to use strict shell flags and CI=true for yarn types.
Lint & Prettier
sdk/core/.eslintrc.cjs, sdk/core/.prettierrc
Adds TypeScript-focused ESLint config (import sorting, sort-exports) and removes explicit semi from Prettier.
Build config & packaging
sdk/core/tsup.config.ts, sdk/core/tsconfig.cjs.json, sdk/core/package.json
Introduces tsup configs for ESM/CJS outputs, CJS tsconfig, expanded exports map with granular subpaths (types/import/require), new build/typechain scripts, and devDependencies for lint/type tooling.
Post-build & shim scripts
sdk/core/scripts/postBuild.mjs, sdk/core/scripts/shimConfigs.mjs, sdk/core/scripts/copyAbi.sh, sdk/core/scripts/migrate.mjs
Adds postBuild that emits dist package.jsons and Metro shims, central shim config registry, ABI copy script updated for Hub V2, and a migration helper that installs deps and updates TS import extensions.
Public API / entry
sdk/core/index.ts, sdk/core/src/index.ts
Reworks public exports to relative/dist paths, adds AllIds, re-exports hub adapters/clients and migration utilities/types, and converts several exports to type-only where appropriate.
Core verifier & hub clients/adapters
sdk/core/src/SelfBackendVerifier.ts, sdk/core/src/clients/HubClient.ts, sdk/core/src/adapters/HubAdapter.ts
Migrates verifier to use IdentityVerificationHubImplV2 (V2 factories/types), introduces IdentityVerificationHubClient, unified HubAdapter with V1/V2 adapters, migration utilities, and related errors/types.
Stores & types
sdk/core/src/store/*, sdk/core/src/types/types.ts
Convert runtime imports to type-only imports, relocate/normalize AttestationId and GenericDiscloseOutput declarations, and adjust import paths.
Utilities
sdk/core/src/utils/constants.ts, sdk/core/src/utils/id.ts, sdk/core/src/utils/proof.ts
Add AllIds constant, convert imports to type-only, introduce bytesCount and getRevealedDataPublicSignalsLength, and expose revealed-data helper functions.
ABI artifacts
sdk/core/src/abi/*
Add/update ABI TS modules exported as consts: IdentityVerificationHubImplV2, IdentityVerificationHubImplV1, VerifyAll, IdentityRegistryImplV1, plus generated ABI outputs.
Contracts config
contracts/hardhat.config.ts
Make network accounts conditional on PRIVATE_KEYS/PRIVATE_KEY env vars (derived accounts array).
Docs
sdk/core/MIGRATION_GUIDE.md
New migration guide documenting V1→V2 migration, adapters, detection, validation, breaking changes, and examples.

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant SDK as SelfBackendVerifier
  participant Adapter as HubAdapter (auto)
  participant HubV2 as IdentityVerificationHubImplV2
  participant Registry as IdentityRegistry
  participant Verifier as Verifier

  App->>SDK: verify(baseInput, ctx)
  SDK->>Adapter: resolve/create adapter (auto)
  Adapter->>HubV2: verificationConfigV2Exists / getVerificationConfigV2
  alt config exists
    HubV2-->>Adapter: VerificationConfigV2
    Adapter->>HubV2: verify(baseInput, ctx)
    HubV2->>Registry: getIdentityCommitmentMerkleRoot(attestationId)
    HubV2->>Verifier: verify proofs
    Verifier-->>HubV2: proof result
    HubV2-->>Adapter: verification outcome + disclosed data
    Adapter-->>SDK: normalized result
    SDK-->>App: VerificationResult (+ disclosed data)
  else config missing
    HubV2-->>Adapter: not found / error
    Adapter-->>SDK: throws Hub/Config error
    SDK-->>App: error
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • aaronmgdr
  • remicolin
  • motemotech

Poem

Hubs upgraded, shims in place, CI sings,
Types realign and contracts spread their wings.
ABIs doubled, adapters learn to speak,
Exports mapped and build scripts find their streak.
Merge the core — a tidy, tested spring 🚀


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca53484 and 96a156a.

📒 Files selected for processing (1)
  • sdk/core/src/clients/HubClient.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • sdk/core/src/clients/HubClient.ts
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/set-up-core-sdk-build-configuration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@transphorm transphorm changed the title feat(core): add dual build config and postbuild shims feat: add dual build config and postbuild shims to sdk/core Aug 11, 2025
@transphorm transphorm changed the title feat: add dual build config and postbuild shims to sdk/core feat: modernize sdk/core build pipeline Aug 11, 2025
@transphorm transphorm marked this pull request as ready for review August 11, 2025 03:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🔭 Outside diff range comments (5)
sdk/core/src/utils/proof.ts (1)

20-36: Add bounds validation for publicSignals to prevent runtime errors

If publicSignals is shorter than expected, BigInt(publicSignals[idx]) will throw. Validate length before processing.

 export function getRevealedDataBytes(
   attestationId: AttestationId,
   publicSignals: PublicSignals
 ): number[] {
   const bytes: number[] = [];
+  const base = discloseIndices[attestationId].revealedDataPackedIndex;
+  const expected = getRevealedDataPublicSignalsLength(attestationId);
+  if (publicSignals.length < base + expected) {
+    throw new Error(
+      `Insufficient publicSignals: need ${base + expected}, got ${publicSignals.length}`
+    );
+  }
   for (let i = 0; i < getRevealedDataPublicSignalsLength(attestationId); i++) {
     let publicSignal = BigInt(
       publicSignals[discloseIndices[attestationId].revealedDataPackedIndex + i]
     );
     for (let j = 0; j < bytesCount[attestationId][i]; j++) {
       bytes.push(Number(publicSignal & 0xffn));
       publicSignal = publicSignal >> 8n;
     }
   }
sdk/core/src/SelfBackendVerifier.ts (4)

137-144: Attestation ID comparison may be format-mismatched (decimal vs hex/string normalization)

attestationId.toString() compares to a publicSignal that may be zero-prefixed hex or decimal. Normalize both to BigInt to avoid false negatives.

-const isValidAttestationId =
-  attestationId.toString() === publicSignals[discloseIndices[attestationId].attestationIdIndex];
+const attestationIdInCircuit = BigInt(
+  publicSignals[discloseIndices[attestationId].attestationIdIndex]
+);
+const isValidAttestationId = BigInt(attestationId) === attestationIdInCircuit;
 if (!isValidAttestationId) {
   issues.push({
     type: ConfigMismatch.InvalidAttestationId,
     message: 'Attestation ID does not match with the one in the circuit',
   });
 }

179-193: excludedCountries is optional; current usage can throw and comparison is one-sided

  • If verificationConfig.excludedCountries is undefined, .every will throw.
  • Current check only ensures config ⊆ circuit. If circuit contains extra countries, it won’t be flagged.
-const forbiddenCountriesListVerificationConfig = verificationConfig.excludedCountries;
-
-const isForbiddenCountryListValid = forbiddenCountriesListVerificationConfig.every((country) =>
-  forbiddenCountriesList.includes(country as Country3LetterCode)
-);
+const configCountries: Country3LetterCode[] = verificationConfig.excludedCountries ?? [];
+// Compare as sets for equality: same length and all members present
+const isForbiddenCountryListValid =
+  configCountries.length === forbiddenCountriesList.length &&
+  configCountries.every((country) =>
+    forbiddenCountriesList.includes(country as Country3LetterCode)
+  );
 if (!isForbiddenCountryListValid) {
   issues.push({
     type: ConfigMismatch.InvalidForbiddenCountriesList,
     message:
       'Forbidden countries list in config does not match with the one in the circuit\nCircuit: ' +
-      forbiddenCountriesList.join(', ') +
+      forbiddenCountriesList.join(', ') +
       '\nConfig: ' +
-      forbiddenCountriesListVerificationConfig.join(', '),
+      configCountries.join(', '),
   });
 }

252-271: Simplify and harden OFAC checks

You currently have three separate checks that only trigger when ofac is disabled; the return’s isOfacValid is also incorrect (see next comment). Collapse these conditions and ensure the code aligns with the returned validity.

- if (!verificationConfig.ofac && genericDiscloseOutput.ofac[0]) {
-   issues.push({
-     type: ConfigMismatch.InvalidOfac,
-     message: 'Passport number OFAC check is not allowed',
-   });
- }
-
- if (!verificationConfig.ofac && genericDiscloseOutput.ofac[1]) {
-   issues.push({
-     type: ConfigMismatch.InvalidOfac,
-     message: 'Name and DOB OFAC check is not allowed',
-   });
- }
-
- if (!verificationConfig.ofac && genericDiscloseOutput.ofac[2]) {
-   issues.push({
-     type: ConfigMismatch.InvalidOfac,
-     message: 'Name and YOB OFAC check is not allowed',
-   });
- }
+ if (verificationConfig.ofac === false && genericDiscloseOutput.ofac.some(Boolean)) {
+   issues.push({
+     type: ConfigMismatch.InvalidOfac,
+     message: 'OFAC checks are not allowed by config but were enabled in the circuit',
+   });
+ }

313-318: isOfacValid is a tautology; it always evaluates to true when ofac is enabled

enabled ? genericDiscloseOutput.ofac[index] : true is equivalent to x ? x : true, which is always true. Reflect the actual policy: valid if OFAC checks are allowed, else valid only if no OFAC checks were enabled.

-        isOfacValid:
-          verificationConfig.ofac !== undefined && verificationConfig.ofac
-            ? genericDiscloseOutput.ofac.every((enabled: boolean, index: number) =>
-                enabled ? genericDiscloseOutput.ofac[index] : true
-              )
-            : true,
+        isOfacValid:
+          verificationConfig.ofac === true ||
+          (verificationConfig.ofac === false
+            ? !genericDiscloseOutput.ofac.some(Boolean)
+            : true),
🧹 Nitpick comments (10)
sdk/core/tsup.config.ts (1)

3-28: Excellent dual build configuration with one consideration.

The tsup setup properly handles both ESM and CJS outputs with consistent entry points and appropriate module-specific settings. The clean: false for CJS prevents wiping the ESM build - good attention to detail.

Consider whether src/**/* as entry points is intentional - this creates individual output files for each source file, enabling granular imports but increasing build complexity. This aligns with your expanded exports map, so it appears deliberate.

sdk/core/package.json (1)

18-89: Enable Corepack and re-run export verification

The export-path check failed because yarn build didn’t run under Corepack. Please:

  • Run corepack enable to activate the correct Yarn v4.6.0 runtime
  • Rebuild the package:
    cd sdk/core
    yarn build
  • Re-execute your export-resolution script and confirm no missing files under dist/

Once all paths resolve correctly, the expanded exports map is safe to merge.

sdk/core/src/utils/id.ts (1)

1-1: Type-only import + relative path: good; ensure Buffer polyfill on RN

Type-only import is correct. Since this module uses Buffer, ensure the React Native environment includes a Buffer polyfill (common via rn-nodeify or Metro config) to avoid runtime errors.

sdk/core/scripts/postBuild.mjs (2)

14-18: Ensure dist subdirs exist before writing package.json files

Guard against edge-cases where dist/esm or dist/cjs are missing (failed/partial build).

+mkdirSync(path.join(DIST, 'esm'), { recursive: true });
+mkdirSync(path.join(DIST, 'cjs'), { recursive: true });
 writeFileSync(path.join(DIST, 'esm', 'package.json'), JSON.stringify({ type: 'module' }, null, 4));
 writeFileSync(
   path.join(DIST, 'cjs', 'package.json'),
   JSON.stringify({ type: 'commonjs' }, null, 4)
 );

21-41: Consider adding types metadata and conditional exports for Node

Dist-level package.json only points exports to ESM; you’re generating CJS shims, but Node consumers may also rely on types. Consider:

  • Add "types": "./index.d.ts" at dist root or "typesVersions" if needed.
  • If you intend Node to consume dist directly (beyond Metro), use conditional exports with import/require targets.
sdk/core/src/utils/proof.ts (1)

6-11: bytesCount: document source-of-truth and keep aligned with circuits

These constants are security-critical. Add a brief comment referencing the circuit version or generation script to reduce risk of silent drift when circuits change.

sdk/core/src/SelfBackendVerifier.ts (4)

112-134: Catching and rethrowing with a generic message hides the root cause (hurts debuggability)

Both registry and verifier lookups swallow the original error and rethrow a generic one. For ops/debugging, consider including the original message as a cause or attaching it to the error.

-} catch (error) {
-  throw new Error('Registry contract not found');
-}
+} catch (error) {
+  const reason = error instanceof Error ? `: ${error.message}` : '';
+  throw new Error(`Registry contract lookup failed${reason}`);
+}

Do similarly for the verifier block below. If you must surface a generic error to callers, at least log the inner error at debug level in the surrounding infrastructure.


159-171: Config fetch: silent catch may hide actionable errors; fold into issues or logs

You ignore errors from getConfig and then throw a ConfigMismatchError anyway. Consider capturing the error message in the issues for context, or log it, while still throwing ConfigMismatchError for callers.

-let verificationConfig: VerificationConfig | null = null;
+let verificationConfig: VerificationConfig | null = null;
 try {
   verificationConfig = await this.configStorage.getConfig(configId);
-} catch {
-  /* ignore */
+} catch (err) {
+  issues.push({
+    type: ConfigMismatch.ConfigNotFound,
+    message: `Config lookup error for ${configId}: ${
+      err instanceof Error ? err.message : String(err)
+    }`,
+  });
 }
 if (!verificationConfig) {
   issues.push({
     type: ConfigMismatch.ConfigNotFound,
     message: `Config not found for ${configId}`,
   });
   throw new ConfigMismatchError(issues);
 }

99-110: Scope comparison assumes identical string formatting; consider normalizing

If the circuit value is hex without 0x, or padded differently, the direct string comparison may fail. Consider normalizing both sides (lowercase, ensure 0x prefix) before comparing.

-const isValidScope = this.scope === publicSignals[discloseIndices[attestationId].scopeIndex];
+const circuitScope = publicSignals[discloseIndices[attestationId].scopeIndex].toLowerCase();
+const expectedScope = (this.scope.startsWith('0x') ? this.scope : `0x${this.scope}`)
+  .toLowerCase();
+const isValidScope = expectedScope === (circuitScope.startsWith('0x') ? circuitScope : `0x${circuitScope}`);

77-87: Public signals normalization: be explicit about base and zero-padding to avoid mismatches

Transforming inputs by adding 0x when [a-f] is present is heuristic and may mis-handle decimal-encoded bigints. Prefer explicit normalization to BigInt and re-encoding as hex with 0x prefix for values that represent field elements.

If acceptable in your verifier ABI, convert to BigInt across the board:

const publicSignals = pubSignals.map((x) => BigInt(x).toString());

Or, if hex is required:

const publicSignals = pubSignals.map((x) => '0x' + BigInt(x).toString(16));

This ensures consistent comparison and contract input formatting.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ed8bcf and f89ad33.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (18)
  • .github/workflows/sdk-core-ci.yml (1 hunks)
  • sdk/core/.eslintrc.cjs (1 hunks)
  • sdk/core/.prettierrc (0 hunks)
  • sdk/core/index.ts (1 hunks)
  • sdk/core/package.json (2 hunks)
  • sdk/core/scripts/migrate.mjs (1 hunks)
  • sdk/core/scripts/postBuild.mjs (1 hunks)
  • sdk/core/scripts/shimConfigs.js (1 hunks)
  • sdk/core/src/SelfBackendVerifier.ts (5 hunks)
  • sdk/core/src/store/DefaultConfigStore.ts (1 hunks)
  • sdk/core/src/store/InMemoryConfigStore.ts (1 hunks)
  • sdk/core/src/store/interface.ts (1 hunks)
  • sdk/core/src/types/types.ts (1 hunks)
  • sdk/core/src/utils/constants.ts (1 hunks)
  • sdk/core/src/utils/id.ts (1 hunks)
  • sdk/core/src/utils/proof.ts (3 hunks)
  • sdk/core/tsconfig.cjs.json (1 hunks)
  • sdk/core/tsup.config.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • sdk/core/.prettierrc
🧰 Additional context used
🧬 Code Graph Analysis (5)
sdk/core/scripts/postBuild.mjs (2)
sdk/core/scripts/shimConfigs.js (2)
  • shimConfigs (2-39)
  • shimConfigs (2-39)
app/metro.config.cjs (1)
  • config (176-196)
sdk/core/src/utils/constants.ts (1)
sdk/core/src/types/types.ts (1)
  • AttestationId (7-7)
sdk/core/src/types/types.ts (1)
sdk/core/src/utils/constants.ts (1)
  • discloseIndices (8-35)
sdk/core/src/SelfBackendVerifier.ts (4)
contracts/test/utils/types.ts (1)
  • IdentityVerificationHubImplV2 (93-93)
sdk/core/index.ts (1)
  • VerificationConfig (18-18)
sdk/core/src/types/types.ts (1)
  • VerificationConfig (29-33)
sdk/core/src/errors.ts (1)
  • ConfigMismatchError (14-29)
sdk/core/src/utils/proof.ts (1)
sdk/core/src/types/types.ts (1)
  • AttestationId (7-7)
🪛 GitHub Check: lint
sdk/core/src/SelfBackendVerifier.ts

[failure] 16-16:
Unable to resolve path to module './typechain-types/index.js'

🪛 GitHub Actions: Common CI
sdk/core/src/SelfBackendVerifier.ts

[error] 16-16: yarn types: ESM build failed: Could not resolve './typechain-types/index.js' (src/SelfBackendVerifier.ts:16).

⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (12)
sdk/core/tsconfig.cjs.json (1)

1-10: LGTM! Solid CommonJS build configuration.

The configuration properly extends the base tsconfig and sets appropriate CJS-specific options. The esModuleInterop setting is particularly important for ensuring compatibility between module systems in your dual build setup.

.github/workflows/sdk-core-ci.yml (3)

1-12: Well-designed CI workflow with proper path filtering.

The workflow correctly targets SDK core changes and includes the workflow file itself in the trigger paths. This ensures the CI runs when relevant code or the workflow configuration changes.


31-43: Comprehensive build artifact caching strategy.

The cache includes all relevant build outputs and intermediate files. Using the git SHA as the cache key ensures reproducibility and prevents cache poisoning between different commits.


85-102: Excellent module system compatibility testing.

Testing both ESM import and CJS require ensures your dual build actually works in practice. The export verification step adds valuable confidence that the package.json exports map correctly.

sdk/core/scripts/migrate.mjs (1)

8-12: Ensure ESLint dependencies are audited & pinned

The migration script currently adds ESLint tooling without a lockfile or version constraints, which means you could accidentally pull in vulnerable or unexpected versions. To mitigate this:

  • Generate and commit a lockfile before running an audit:
    • If you use Yarn:
    1. Run yarn install to update or create yarn.lock
    2. Run yarn audit --level=moderate
      • If you use npm:
    3. Run npm i --package-lock-only to create package-lock.json
    4. Run npm audit --audit-level=moderate
  • Review any reported vulnerabilities and bump to the latest patched releases.
  • Pin your ESLint dependencies in the migrate script (either with --exact or explicit version specifiers) to ensure reproducible, secure installs.
sdk/core/.eslintrc.cjs (3)

40-48: Excellent import organization rules.

The import sorting configuration creates a logical hierarchy: Node.js built-ins → external packages → organization packages → local files. This will significantly improve code readability and consistency across the codebase.


54-60: Smart tree-shaking optimization rule.

Prohibiting export * statements is excellent for bundle size optimization. Explicit exports make it easier for bundlers to perform dead code elimination and improve the developer experience with better IDE support.


70-87: Proper CommonJS file handling.

The override for .cjs files correctly adjusts parser options and disables TypeScript-specific rules that don't apply to CommonJS modules. This ensures the linter works correctly with your dual build output.

sdk/core/src/store/InMemoryConfigStore.ts (1)

1-2: Excellent use of TypeScript type-only imports.

Converting to type-only imports is the correct approach here since these types are only used in type annotations. This improves build performance, enables better tree-shaking, and aligns with your ESLint configuration's consistent-type-imports rule.

sdk/core/src/store/interface.ts (1)

1-1: Consistent type-only import pattern.

This change maintains consistency with the other store modules and correctly uses type-only imports since VerificationConfig is only referenced in method signatures. This supports better tree-shaking and follows TypeScript best practices.

sdk/core/src/store/DefaultConfigStore.ts (1)

1-2: Good move to type-only imports

Reduces runtime deps and avoids circular runtime imports. LGTM.

sdk/core/scripts/shimConfigs.js (1)

1-39: Centralized shim config looks solid

Consistent paths and naming; matches postBuild.mjs expectations. LGTM.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🔭 Outside diff range comments (2)
sdk/core/scripts/copyAbi.sh (1)

2-2: Use strict bash modes to fail fast and surface issues clearly

Harden the script to catch unset vars and pipeline errors.

- set -e
+ set -Eeuo pipefail
sdk/core/src/abi/IdentityVerificationHubImpl.json (1)

5-1018: ABI parity: keep TS and JSON in sync

The TS export and this JSON’s abi should be generated from the same source to prevent drift. If this JSON is kept, wire scripts/copyAbi.sh (or a single source-of-truth step) to generate both.

I can generate a single script that outputs both the TS “as const” file and a minimal abi-only JSON from the artifact—want me to open a follow-up?

🧹 Nitpick comments (7)
contracts/hardhat.config.ts (1)

6-10: Duplicate plugin import: hardhat-contract-sizer imported twice

Line 6 and 10 both import hardhat-contract-sizer. Remove one import.

-import "hardhat-contract-sizer";
 import "@nomicfoundation/hardhat-ignition-ethers";
 import "solidity-coverage";
 import "hardhat-gas-reporter";
 import "hardhat-contract-sizer";
sdk/core/scripts/postBuild.mjs (1)

14-18: Ensure dist/esm and dist/cjs directories exist before writing package.json

Guard against race conditions when this script runs standalone or in parallel CI steps.

- writeFileSync(path.join(DIST, 'esm', 'package.json'), JSON.stringify({ type: 'module' }, null, 4));
- writeFileSync(
+ mkdirSync(path.join(DIST, 'esm'), { recursive: true });
+ mkdirSync(path.join(DIST, 'cjs'), { recursive: true });
+ writeFileSync(path.join(DIST, 'esm', 'package.json'), JSON.stringify({ type: 'module' }, null, 4));
+ writeFileSync(
   path.join(DIST, 'cjs', 'package.json'),
   JSON.stringify({ type: 'commonjs' }, null, 4)
 );
sdk/core/scripts/copyAbi.sh (3)

28-31: Minify ABI JSON to reduce file size and CI noise

jq’s compact output keeps generated TS files smaller, speeding up builds/lints.

-ABI_JSON_REGISTRY=$(jq '.abi' "$SOURCE_REGISTRY")
-ABI_JSON_VERIFYALL=$(jq '.abi' "$SOURCE_VERIFYALL")
-ABI_JSON_HUB_V2=$(jq '.abi' "$SOURCE_HUB_V2")
+ABI_JSON_REGISTRY=$(jq -c '.abi' "$SOURCE_REGISTRY")
+ABI_JSON_VERIFYALL=$(jq -c '.abi' "$SOURCE_VERIFYALL")
+ABI_JSON_HUB_V2=$(jq -c '.abi' "$SOURCE_HUB_V2")

32-35: Make generated ABIs immutable for safer TS typing

Append “as const” so consumers get literal types and can’t accidentally mutate the ABI.

-OUTPUT_REGISTRY="export const registryAbi = ${ABI_JSON_REGISTRY};"
-OUTPUT_VERIFYALL="export const verifyAllAbi = ${ABI_JSON_VERIFYALL};"
-OUTPUT_HUB_V2="export const hubV2Abi = ${ABI_JSON_HUB_V2};"
+OUTPUT_REGISTRY="export const registryAbi = ${ABI_JSON_REGISTRY} as const;"
+OUTPUT_VERIFYALL="export const verifyAllAbi = ${ABI_JSON_VERIFYALL} as const;"
+OUTPUT_HUB_V2="export const hubV2Abi = ${ABI_JSON_HUB_V2} as const;"

40-47: Add an autogenerated header and disable linting in generated files

Prepend a small header to clarify provenance and avoid lint warnings for JSON-style keys.

-echo "$OUTPUT_REGISTRY" > "$DEST_REGISTRY_TS"
-echo "Written ABI for IdentityRegistryImplV1 to: $DEST_REGISTRY_TS"
-
-echo "$OUTPUT_VERIFYALL" > "$DEST_VERIFYALL_TS"
-echo "Written ABI for VerifyAll to: $DEST_VERIFYALL_TS"
-
-echo "$OUTPUT_HUB_V2" > "$DEST_HUB_V2_TS"
-echo "Written ABI for IdentityVerificationHubImplV2 to: $DEST_HUB_V2_TS"
+{
+  echo "/* Autogenerated file. Do not edit manually. */"
+  echo "/* eslint-disable */"
+  echo "$OUTPUT_REGISTRY"
+} > "$DEST_REGISTRY_TS"
+echo "Written ABI for IdentityRegistryImplV1 to: $DEST_REGISTRY_TS"
+
+{
+  echo "/* Autogenerated file. Do not edit manually. */"
+  echo "/* eslint-disable */"
+  echo "$OUTPUT_VERIFYALL"
+} > "$DEST_VERIFYALL_TS"
+echo "Written ABI for VerifyAll to: $DEST_VERIFYALL_TS"
+
+{
+  echo "/* Autogenerated file. Do not edit manually. */"
+  echo "/* eslint-disable */"
+  echo "$OUTPUT_HUB_V2"
+} > "$DEST_HUB_V2_TS"
+echo "Written ABI for IdentityVerificationHubImplV2 to: $DEST_HUB_V2_TS"
sdk/core/src/abi/IdentityVerificationHubImplV2.ts (2)

1-1: Freeze ABI for better type safety

Appending “as const” locks string literals and shapes, improving developer experience with ethers/viem.

-export const hubV2Abi = [
+export const hubV2Abi = [
   // …unchanged…
-];
+ ] as const;

1-1: Mark as generated to avoid drive-by edits and lints

If this file is generated by scripts/copyAbi.sh, add a header. This also helps Metro/tsup consumers understand the source of truth.

+/* Autogenerated file. Do not edit manually. */
+/* eslint-disable */
 export const hubV2Abi = [
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f89ad33 and 3deab93.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (11)
  • .github/workflows/common-ci.yml (1 hunks)
  • contracts/hardhat.config.ts (1 hunks)
  • sdk/core/index.ts (1 hunks)
  • sdk/core/scripts/copyAbi.sh (2 hunks)
  • sdk/core/scripts/postBuild.mjs (1 hunks)
  • sdk/core/scripts/shimConfigs.mjs (1 hunks)
  • sdk/core/src/abi/IdentityRegistryImplV1.ts (1 hunks)
  • sdk/core/src/abi/IdentityVerificationHubImpl.json (3 hunks)
  • sdk/core/src/abi/IdentityVerificationHubImplV2.ts (1 hunks)
  • sdk/core/src/abi/VerifyAll.ts (1 hunks)
  • sdk/core/src/utils/constants.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • sdk/core/src/abi/IdentityRegistryImplV1.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • sdk/core/src/utils/constants.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
sdk/core/scripts/postBuild.mjs (1)
sdk/core/scripts/shimConfigs.mjs (2)
  • shimConfigs (2-39)
  • shimConfigs (2-39)
🔇 Additional comments (4)
sdk/core/index.ts (1)

1-11: Type-only exports and ESM/CJS-friendly paths look solid

Type-only exports prevent runtime re-exports under NodeNext/tsup. Explicit .js extensions align with ESM output. Public surface reads clean.

Also applies to: 14-21

sdk/core/scripts/copyAbi.sh (1)

11-11: Confirm artifact path stability in copyAbi.sh

The hard-coded SOURCE_HUB_V2 relative path points to a generated artifact location that may shift if your Hardhat paths.artifacts setting or contracts folder structure changes. Please double-check that your hardhat.config.(js|ts) still emits JSON files under contracts/artifacts/contracts/..., or consider deriving the artifact directory dynamically.

• File: sdk/core/scripts/copyAbi.sh
Line 11:

SOURCE_HUB_V2="$SCRIPT_DIR/../../../contracts/artifacts/contracts/IdentityVerificationHubImplV2.sol/IdentityVerificationHubImplV2.json"

• Verify your Hardhat config’s paths.artifacts matches this path.
• Optionally, refactor the script to read the artifact output directory from your config or an env var to guard against future changes.

sdk/core/src/abi/IdentityVerificationHubImplV2.ts (1)

1-1015: Bundle hygiene verified: ABI is correctly isolated under “./abi/*”
The package’s exports field maps "./abi/*" exclusively to ./src/abi/*, and there are no re-exports of any ABI entries in the root/index entrypoints. React Native bundles will only include this large constant when you explicitly import from the abi path. No changes required.

sdk/core/src/abi/IdentityVerificationHubImpl.json (1)

1020-1022: ✅ No Bytecode-heavy JSON in Published Bundle

I confirmed that only dist is published (per package.json “files” array) and running fd -e json sdk/core/dist yields no JSON artifacts. The Hardhat .json files in src/abi won’t be bundled or shipped—so there’s no bloat and no change is needed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

♻️ Duplicate comments (1)
sdk/core/package.json (1)

158-165: Pin ESLint dependencies to exact versions.

As previously noted, ESLint dependencies should be pinned to specific versions to ensure consistent behavior across environments.

🧹 Nitpick comments (1)
sdk/core/MIGRATION_GUIDE.md (1)

1-313: Excellent migration documentation with comprehensive guidance.

The guide provides clear, actionable steps for migrating from V1 to V2 with well-structured sections, helpful code examples, and practical checklists. The breaking changes mapping and troubleshooting sections are particularly valuable for developers.

Minor suggestion: Consider adding version compatibility matrix or a table showing which networks currently support V2 to help developers plan their migration timeline.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3deab93 and ca53484.

📒 Files selected for processing (13)
  • .github/workflows/common-ci.yml (1 hunks)
  • contracts/hardhat.config.ts (2 hunks)
  • sdk/core/MIGRATION_GUIDE.md (1 hunks)
  • sdk/core/package.json (2 hunks)
  • sdk/core/scripts/postBuild.mjs (1 hunks)
  • sdk/core/scripts/shimConfigs.mjs (1 hunks)
  • sdk/core/src/abi/IdentityRegistryImplV1.ts (1 hunks)
  • sdk/core/src/abi/IdentityVerificationHubImplV1.ts (1 hunks)
  • sdk/core/src/abi/IdentityVerificationHubImplV2.ts (1 hunks)
  • sdk/core/src/abi/VerifyAll.ts (1 hunks)
  • sdk/core/src/adapters/HubAdapter.ts (1 hunks)
  • sdk/core/src/clients/HubClient.ts (1 hunks)
  • sdk/core/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • sdk/core/src/abi/IdentityVerificationHubImplV2.ts
  • .github/workflows/common-ci.yml
  • sdk/core/scripts/shimConfigs.mjs
  • contracts/hardhat.config.ts
  • sdk/core/src/abi/IdentityRegistryImplV1.ts
  • sdk/core/scripts/postBuild.mjs
  • sdk/core/src/abi/VerifyAll.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
sdk/core/src/clients/HubClient.ts (3)
sdk/core/src/index.ts (9)
  • IdentityVerificationHubClient (6-6)
  • createHubAdapter (7-7)
  • IdentityVerificationHubAdapter (2-2)
  • createHubAdapterWithValidation (8-8)
  • createHubClient (9-9)
  • getMigrationReport (10-10)
  • MigrationInfo (2-2)
  • HubMigrationUtils (3-3)
  • supportsV2 (11-11)
sdk/core/src/abi/IdentityVerificationHubImplV2.ts (1)
  • hubV2Abi (1-1015)
sdk/core/src/adapters/HubAdapter.ts (8)
  • createHubAdapter (421-438)
  • IdentityVerificationHubAdapter (5-38)
  • createHubAdapterAuto (441-458)
  • createHubAdapterWithMigration (461-491)
  • getMigrationReport (128-164)
  • MigrationInfo (41-47)
  • HubMigrationUtils (107-195)
  • supportsV2 (111-123)
sdk/core/src/adapters/HubAdapter.ts (4)
sdk/core/src/index.ts (6)
  • IdentityVerificationHubAdapter (2-2)
  • MigrationInfo (2-2)
  • HubMigrationError (3-3)
  • HubMigrationUtils (3-3)
  • HubVersionError (3-3)
  • createHubAdapter (7-7)
sdk/core/src/abi/IdentityVerificationHubImplV2.ts (1)
  • hubV2Abi (1-1015)
sdk/core/src/abi/IdentityVerificationHubImplV1.ts (1)
  • hubV1Abi (1-500)
sdk/core/src/clients/HubClient.ts (1)
  • createHubAdapter (176-182)
🪛 LanguageTool
sdk/core/MIGRATION_GUIDE.md

[style] ~124-~124: Consider using a different adverb to strengthen your wording.
Context: ...context) | Completely new interface | |registerPassportComm...

(COMPLETELY_ENTIRELY)

🪛 GitHub Check: lint
sdk/core/src/clients/HubClient.ts

[warning] 219-219:
Unexpected any. Specify a different type


[warning] 208-208:
Unexpected any. Specify a different type


[warning] 207-207:
Unexpected any. Specify a different type


[warning] 191-191:
Unexpected any. Specify a different type


[warning] 190-190:
Unexpected any. Specify a different type


[warning] 179-179:
Unexpected any. Specify a different type


[warning] 178-178:
Unexpected any. Specify a different type


[warning] 21-21:
Unexpected any. Specify a different type


[warning] 20-20:
Unexpected any. Specify a different type


[warning] 16-16:
Unexpected any. Specify a different type

🪛 GitHub Actions: Common CI
sdk/core/package.json

[warning] 22-22: The condition 'types' here will never be used as it comes after both 'import' and 'require'.


[warning] 27-27: The condition 'types' here will never be used as it comes after both 'import' and 'require'.


[warning] 32-32: The condition 'types' here will never be used as it comes after both 'import' and 'require'.


[warning] 37-37: The condition 'types' here will never be used as it comes after both 'import' and 'require'.


[warning] 42-42: The condition 'types' here will never be used as it comes after both 'import' and 'require'.

🪛 Biome (2.1.2)
sdk/core/src/adapters/HubAdapter.ts

[error] 89-91: An empty interface is equivalent to {}.

Safe fix: Use a type alias instead.

(lint/suspicious/noEmptyInterface)

🔇 Additional comments (5)
sdk/core/src/abi/IdentityVerificationHubImplV1.ts (1)

1-500: LGTM! Well-structured ABI definition for V1 hub.

The ABI is properly formatted with comprehensive error definitions, events, and function signatures. The use of as const assertion ensures type safety and immutability, which is excellent for contract interaction reliability.

sdk/core/src/index.ts (1)

1-14: Clean and well-organized public API exports.

Good separation of type exports from value exports. The hub client and adapter exports provide a comprehensive API surface for migration scenarios while maintaining backward compatibility with the existing SelfBackendVerifier.

sdk/core/package.json (1)

19-23: Fix export condition ordering for proper TypeScript resolution.

The 'types' condition must come before 'import' and 'require' to ensure TypeScript correctly resolves type definitions. This ordering issue affects lines 20-22, 25-27, 30-32, 35-37, 40-42, and other export entries.

Apply this pattern throughout all exports:

    ".": {
-      "types": "./dist/esm/index.d.ts",
-      "import": "./dist/esm/index.js",
-      "require": "./dist/cjs/index.cjs"
+      "types": "./dist/esm/index.d.ts",
+      "import": "./dist/esm/index.js",
+      "require": "./dist/cjs/index.cjs"
    },

Wait, the ordering is already correct with 'types' first. Let me reconsider...

sdk/core/src/adapters/HubAdapter.ts (2)

111-123: Solid V2 detection logic with proper error handling.

The supportsV2 method correctly attempts to call a V2-specific function and handles the error gracefully to determine version. The use of a zero address for the check is appropriate.


198-307: Well-designed V1 adapter with excellent migration guidance.

The adapter properly handles V1 limitations, provides helpful migration warnings when V2 features are attempted, and maintains backward compatibility. The migration info is comprehensive and actionable.

"import": "./dist/esm/src/abi/IdentityRegistryImplV1.js",
"require": "./dist/cjs/src/abi/IdentityRegistryImplV1.cjs"
},
"./abi/*": "./src/abi/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the implications of exposing source ABI files directly.

The alias "./abi/*": "./src/abi/*" maps directly to source files rather than bundled outputs. This could expose unbundled TypeScript files to consumers. Consider mapping to the dist folder instead to ensure consumers receive properly transpiled JavaScript.

-    "./abi/*": "./src/abi/*"
+    "./abi/*": "./dist/esm/src/abi/*"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"./abi/*": "./src/abi/*"
"./abi/*": "./dist/esm/src/abi/*"
🤖 Prompt for AI Agents
In sdk/core/package.json around line 109, the export alias "./abi/*":
"./src/abi/*" points consumers to source files (untranspiled TypeScript) which
can leak implementation and break downstream builds; change the alias to point
to the compiled output (e.g., "./dist/abi/*" or wherever your build emits
transpiled JS) and update your build to copy/emit ABI files into that dist path
so consumers import the transpiled artifacts instead of raw source.

Comment on lines +89 to +91
export interface V2VerificationConfig {
// Add V2 config structure as needed
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace empty interface with type alias.

An empty interface is equivalent to {}. Since this appears to be a placeholder for future configuration, a type alias would be more appropriate.

-// V2 Types
-export interface V2VerificationConfig {
-  // Add V2 config structure as needed
-}
+// V2 Types
+export type V2VerificationConfig = {
+  // TODO: Add V2 config structure when available
+  olderThanEnabled?: boolean;
+  olderThan?: bigint;
+  forbiddenCountriesEnabled?: boolean;
+  forbiddenCountriesListPacked?: readonly [bigint, bigint, bigint, bigint];
+  ofacEnabled?: readonly [boolean, boolean, boolean];
+};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export interface V2VerificationConfig {
// Add V2 config structure as needed
}
// V2 Types
export type V2VerificationConfig = {
// TODO: Add V2 config structure when available
olderThanEnabled?: boolean;
olderThan?: bigint;
forbiddenCountriesEnabled?: boolean;
forbiddenCountriesListPacked?: readonly [bigint, bigint, bigint, bigint];
ofacEnabled?: readonly [boolean, boolean, boolean];
};
🧰 Tools
🪛 Biome (2.1.2)

[error] 89-91: An empty interface is equivalent to {}.

Safe fix: Use a type alias instead.

(lint/suspicious/noEmptyInterface)

🤖 Prompt for AI Agents
In sdk/core/src/adapters/HubAdapter.ts around lines 89 to 91, replace the empty
interface declaration for V2VerificationConfig with a type alias (e.g., type
V2VerificationConfig = {};), keeping it a placeholder for future config. Update
any usages if they rely on interface-specific features (none expected) and
ensure exports remain unchanged.

Comment on lines +420 to +437
// Factory function to create the appropriate adapter
export function createHubAdapter(
contractAddress: string,
version: 'v1' | 'v2',
publicClient: unknown // Replace with proper client type
): IdentityVerificationHubAdapter {
const abi = version === 'v1' ? hubV1Abi : hubV2Abi;

// Create contract instance (replace with proper contract creation)
const contract = {
read: {},
write: {},
abi,
};

return version === 'v1'
? new HubV1Adapter(contract, publicClient, contractAddress)
: new HubV2Adapter(contract, publicClient, contractAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address placeholder contract implementation.

Similar to HubClient, the factory creates placeholder contract objects with empty read/write properties that won't function at runtime.

The current placeholder implementation will fail at runtime. Would you like me to help implement a proper contract factory using ethers or create a mock implementation for testing purposes?

🤖 Prompt for AI Agents
In sdk/core/src/adapters/HubAdapter.ts around lines 420 to 437, the factory
currently returns a placeholder contract with empty read/write fields which will
break at runtime; replace the placeholder with a real ethers Contract instance
created from the chosen ABI and contractAddress using the provided publicClient
(add proper typing for publicClient as Provider | Signer | JsonRpcProvider),
import { Contract } from 'ethers', and instantiate: new
Contract(contractAddress, abi, publicClient). If you need separate read and
write behaviors, create a read-only contract using a Provider and a
write-enabled contract by connecting a Signer (contract.connect(signer)), then
pass the appropriate contract instance(s) into HubV1Adapter/HubV2Adapter; ensure
imports and types are updated and add a short runtime check to throw a helpful
error if publicClient is not a compatible provider/signer.

Comment on lines 16 to 31
private contract: any; // Will be properly typed when ethers integration is added

constructor(
contractAddress: string,
_provider: any, // Replace with proper provider type
_signer?: any // Replace with proper signer type
) {
// TODO: Implement proper ethers contract creation
this.contract = {
address: contractAddress,
abi: hubV2Abi,
// Placeholder for ethers contract methods
read: {},
write: {},
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Address placeholder implementation and type safety issues.

The contract initialization uses a placeholder that won't work at runtime. The empty read and write objects will cause all method calls to fail. Additionally, multiple any types compromise type safety.

Consider either:

  1. Completing the ethers integration before merging
  2. Throwing a clear "not implemented" error in the constructor
  3. Adding proper type definitions for provider and signer

🏁 Script executed:

#!/bin/bash
# Check if ethers types are available in the codebase
fd -e ts -e tsx | xargs rg "import.*ethers" -l | head -10
rg "Contract.*ethers" --type ts -A 2

Length of output: 1848


Implement proper Ethers integration or guard against runtime failures

The current placeholder in sdk/core/src/clients/HubClient.ts will break at runtime (empty read/write) and the untyped any parameters undermine type safety.

Please update lines 16–31 as follows:

• Replace the placeholder object with a real ethers.Contract instance or throw an explicit “not implemented” error to prevent silent failures.
• Import and use the correct Ethers types:
Contract for this.contract
providers.Provider for _provider
Signer for _signer

Suggested diff:

-import { hubV2Abi } from '../abis/hubV2.json';
+import { hubV2Abi } from '../abis/hubV2.json';
+import { Contract, providers, Signer } from 'ethers';

-  private contract: any; // Will be properly typed when ethers integration is added
+  private contract: Contract;

  constructor(
    contractAddress: string,
-    _provider: any, // Replace with proper provider type
-    _signer?: any // Replace with proper signer type
+    provider: providers.Provider,
+    signer?: Signer
  ) {
-    // TODO: Implement proper ethers contract creation
-    this.contract = {
-      address: contractAddress,
-      abi: hubV2Abi,
-      // Placeholder for ethers contract methods
-      read: {},
-      write: {},
-    };
+    // Initialize real Ethers contract to enable on-chain calls
+    this.contract = new Contract(contractAddress, hubV2Abi, signer ?? provider);
  }

If full integration isn’t ready, at minimum throw in the constructor:

throw new Error('HubClient: not implemented – ethers integration required');
🧰 Tools
🪛 GitHub Check: lint

[warning] 21-21:
Unexpected any. Specify a different type


[warning] 20-20:
Unexpected any. Specify a different type


[warning] 16-16:
Unexpected any. Specify a different type

Comment on lines 176 to 238
export function createHubAdapter(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any // Replace with proper signer type
): Promise<IdentityVerificationHubAdapter> {
return createHubAdapterAuto(contractAddress, provider);
}

/**
* Factory function to create an adapter with migration validation
* Use this when you want detailed migration guidance and validation.
*/
export function createHubAdapterWithValidation(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any, // Replace with proper signer type
options?: {
forceVersion?: 'v1' | 'v2';
validateMigration?: boolean;
showWarnings?: boolean;
}
): Promise<IdentityVerificationHubAdapter> {
return createHubAdapterWithMigration(contractAddress, provider, options);
}

/**
* Factory function to create a IdentityVerificationHubClient for V2
* This is the preferred way to create a hub client for new code.
*/
export function createHubClient(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any // Replace with proper signer type
): IdentityVerificationHubClient {
return new IdentityVerificationHubClient(contractAddress, provider, _signer);
}

/**
* Get detailed migration report for a contract
* Use this to understand migration requirements and get step-by-step guidance.
*/
export async function getMigrationReport(
contractAddress: string,
provider: any // Replace with proper provider type
): Promise<{
currentVersion: 'v1' | 'v2';
canMigrate: boolean;
migrationInfo: MigrationInfo;
recommendations: string[];
}> {
return await HubMigrationUtils.getMigrationReport(contractAddress, provider);
}

/**
* Check if a contract supports V2 features
* Use this to determine if V2 is available before attempting migration.
*/
export async function supportsV2(
contractAddress: string,
provider: any // Replace with proper provider type
): Promise<boolean> {
return await HubMigrationUtils.supportsV2(contractAddress, provider);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace any types with proper type definitions or unknowns.

Multiple functions use any for provider and signer parameters, reducing type safety. Since these are public API functions, proper typing is essential.

Consider creating placeholder types until ethers integration is complete:

+type Provider = unknown; // TODO: Replace with ethers.Provider
+type Signer = unknown; // TODO: Replace with ethers.Signer

export function createHubAdapter(
  contractAddress: string,
-  provider: any, // Replace with proper provider type
-  _signer?: any // Replace with proper signer type
+  provider: Provider,
+  _signer?: Signer
): Promise<IdentityVerificationHubAdapter> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function createHubAdapter(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any // Replace with proper signer type
): Promise<IdentityVerificationHubAdapter> {
return createHubAdapterAuto(contractAddress, provider);
}
/**
* Factory function to create an adapter with migration validation
* Use this when you want detailed migration guidance and validation.
*/
export function createHubAdapterWithValidation(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any, // Replace with proper signer type
options?: {
forceVersion?: 'v1' | 'v2';
validateMigration?: boolean;
showWarnings?: boolean;
}
): Promise<IdentityVerificationHubAdapter> {
return createHubAdapterWithMigration(contractAddress, provider, options);
}
/**
* Factory function to create a IdentityVerificationHubClient for V2
* This is the preferred way to create a hub client for new code.
*/
export function createHubClient(
contractAddress: string,
provider: any, // Replace with proper provider type
_signer?: any // Replace with proper signer type
): IdentityVerificationHubClient {
return new IdentityVerificationHubClient(contractAddress, provider, _signer);
}
/**
* Get detailed migration report for a contract
* Use this to understand migration requirements and get step-by-step guidance.
*/
export async function getMigrationReport(
contractAddress: string,
provider: any // Replace with proper provider type
): Promise<{
currentVersion: 'v1' | 'v2';
canMigrate: boolean;
migrationInfo: MigrationInfo;
recommendations: string[];
}> {
return await HubMigrationUtils.getMigrationReport(contractAddress, provider);
}
/**
* Check if a contract supports V2 features
* Use this to determine if V2 is available before attempting migration.
*/
export async function supportsV2(
contractAddress: string,
provider: any // Replace with proper provider type
): Promise<boolean> {
return await HubMigrationUtils.supportsV2(contractAddress, provider);
}
// Add placeholder types for now
type Provider = unknown; // TODO: Replace with ethers.Provider
type Signer = unknown; // TODO: Replace with ethers.Signer
export function createHubAdapter(
contractAddress: string,
provider: Provider,
_signer?: Signer
): Promise<IdentityVerificationHubAdapter> {
return createHubAdapterAuto(contractAddress, provider);
}
🧰 Tools
🪛 GitHub Check: lint

[warning] 219-219:
Unexpected any. Specify a different type


[warning] 208-208:
Unexpected any. Specify a different type


[warning] 207-207:
Unexpected any. Specify a different type


[warning] 191-191:
Unexpected any. Specify a different type


[warning] 190-190:
Unexpected any. Specify a different type


[warning] 179-179:
Unexpected any. Specify a different type


[warning] 178-178:
Unexpected any. Specify a different type

🤖 Prompt for AI Agents
In sdk/core/src/clients/HubClient.ts around lines 176 to 238, several public
factory and helper functions use the any type for provider and signer parameters
which reduces type safety; replace those anys with concrete types (or safe
placeholders) — for example import and use the appropriate ethers provider and
signer interfaces (ethers.providers.Provider and ethers.Signer) or define local
alias types like ProviderType and SignerType and use them across functions, or
if you prefer a temporary safe option use unknown instead of any and narrow/cast
internally; update the function signatures and any downstream usages so
TypeScript enforces correct provider/signer shapes and export the placeholder
types so the public API is strongly typed.

@transphorm transphorm marked this pull request as draft August 12, 2025 20:49
@transphorm
Copy link
Member Author

this is not working as expected. we need to figure out how to pre-build and cache the circuit artifacts before testing

@transphorm
Copy link
Member Author

going to circle back on this

@transphorm transphorm closed this Aug 18, 2025
@transphorm transphorm deleted the codex/set-up-core-sdk-build-configuration branch August 20, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants