feat: simplify SelfAppBuilder integration API#1925
Open
Tranquil-Flow wants to merge 9 commits intodevfrom
Open
feat: simplify SelfAppBuilder integration API#1925Tranquil-Flow wants to merge 9 commits intodevfrom
Tranquil-Flow wants to merge 9 commits intodevfrom
Conversation
Add SelfAppBuilder.forContract() and .forBackend() static factory methods with clear parameter names (contractAddress/scopeSeed vs endpoint/scope). Auto-detect endpointType from endpoint prefix (0x → celo, https:// → https) and userIdType from userId prefix (0x → hex, else → uuid). Make userId optional with auto-generation via uuid v4. Add disclosure presets (basic-kyc, age-verification, full-passport, ofac-only) that resolve to SelfAppDisclosureConfig objects. Improve all validation errors to be actionable with specific suggestions. Add frontendConfig() helper to SelfBackendVerifier for generating matching frontend config. Replace duplicate SelfAppBuilder in sdk-common with re-export from new-common. Export presets and new types through sdk-common and qrcode packages. Update READMEs with simplified integration examples. Add 37 tests covering all new and existing behavior. All changes are additive and backwards compatible.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates SelfApp builder into new-common with disclosure presets, inference and factory helpers; re-exports builder/presets/types from sdk-common; updates backend frontendConfig API; adds tests; adjusts mobile/webview handling and country code constants; and updates docs and package dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Tranquil-Flow
had a problem deploying
to
development
GitHub Actions
Error
Tranquil-Flow
temporarily deployed
to
development
GitHub Actions
Inactive
Factory methods (forContract/forBackend) were spreading undefined values for optional params like logoBase64 and header, overriding the '' defaults in the constructor. Now only spread defined values. Fix Complete Example in qrcode README to build selfApp inside useMemo so each mount gets a unique auto-generated userId instead of sharing one across all users.
Tranquil-Flow
had a problem deploying
to
development
GitHub Actions
Failure
Tranquil-Flow
had a problem deploying
to
development
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/webview-app/src/screens/onboarding/ProviderLaunchScreen.tsx (1)
55-88:⚠️ Potential issue | 🟠 MajorCatch post-completion failures inside
handleComplete.
waitForAttestation(...),buildKycDocument(...), andstorePassportData(...)all run after the provider reports success, but none of them is wrapped in a localtry/catch. A failure there bypasseshandleErrorand the outer launch effect, so this path can leave the screen stuck inwaitinginstead of returning a retryable provider result. As per coding guidelines, "Always use try-catch for async operations with graceful degradation when native modules fail and comprehensive error boundaries."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8cc0ffb-182a-43bd-be0e-514009309310
⛔ Files ignored due to path filters (50)
packages/native-shell-android/src/main/assets/self-wallet/assets/booklet-moire-left-Bh6MkDzP.pngis excluded by!**/*.pngpackages/native-shell-android/src/main/assets/self-wallet/assets/booklet-moire-right-CY2PQJOz.pngis excluded by!**/*.pngpackages/native-shell-android/src/main/assets/self-wallet/assets/index-YX6AnLbA.js.mapis excluded by!**/*.mappackages/native-shell-android/src/main/assets/self-wallet/assets/index.es-CVNUZmbC.js.mapis excluded by!**/*.mappackages/native-shell-android/src/main/assets/self-wallet/backgrounds/dialogue-background-proof-result.jpgis excluded by!**/*.jpgpackages/native-shell-android/src/main/assets/self-wallet/backgrounds/dialogue-background-simple.jpgis excluded by!**/*.jpgpackages/native-shell-android/src/main/assets/self-wallet/backgrounds/dialogue-background.jpgis excluded by!**/*.jpgpackages/native-shell-android/src/main/assets/self-wallet/backgrounds/dialogue-background.pngis excluded by!**/*.pngpackages/native-shell-android/src/main/assets/self-wallet/backgrounds/restore.pngis excluded by!**/*.pngpackages/native-shell-android/src/main/assets/self-wallet/logos/self.svgis excluded by!**/*.svgpackages/native-shell-ios/Resources/self-sdk-web/assets/aadhaar-registration-background-CyX4r5V8.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/affirmative-BixXd3iG.wavis excluded by!**/*.wavpackages/native-shell-ios/Resources/self-sdk-web/assets/booklet-moire-left-Bh6MkDzP.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/booklet-moire-right-CY2PQJOz.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/dev-mode-B7OFUXG_.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/didit-sdk.esm-D5Sol1U1.js.mapis excluded by!**/*.mappackages/native-shell-ios/Resources/self-sdk-web/assets/eu-id-guilloche-DpaaokAE.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/eu-id-portrait-v2-Dk8VGCuw.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/eu-id-selfie-rqiwWB-T.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-1-BqIVu7Uh.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-2-xfWeRop8.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-3-BYL8eCsL.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-4-MuEVVNxA.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-6-DvIg7_wA.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-7-DCXaflzB.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-8-CAet-elZ.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/human-bg-9-rwM-BoKb.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/index-YX6AnLbA.js.mapis excluded by!**/*.mappackages/native-shell-ios/Resources/self-sdk-web/assets/index.es-CVNUZmbC.js.mapis excluded by!**/*.mappackages/native-shell-ios/Resources/self-sdk-web/assets/moire-light-OZs3YXh_.svgis excluded by!**/*.svgpackages/native-shell-ios/Resources/self-sdk-web/assets/negative-B7ZRPokk.wavis excluded by!**/*.wavpackages/native-shell-ios/Resources/self-sdk-web/assets/nfc-tap-phone-1-CKYQoX9l.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/passport-front-cover-f0fc8EoB.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/passport-inside-left-CLt089eW.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/passport-inside-right-CR1oMMID.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/passport-open-picture-page-DO1lGFjA.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/progress-DQ3165CA.wavis excluded by!**/*.wavpackages/native-shell-ios/Resources/self-sdk-web/assets/proof-fail-BwRY0Xme.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/assets/utilize-CDvw9gnR.wavis excluded by!**/*.wavpackages/native-shell-ios/Resources/self-sdk-web/backgrounds/dialogue-background-proof-result.jpgis excluded by!**/*.jpgpackages/native-shell-ios/Resources/self-sdk-web/backgrounds/dialogue-background-simple.jpgis excluded by!**/*.jpgpackages/native-shell-ios/Resources/self-sdk-web/backgrounds/dialogue-background.jpgis excluded by!**/*.jpgpackages/native-shell-ios/Resources/self-sdk-web/backgrounds/dialogue-background.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/backgrounds/restore.pngis excluded by!**/*.pngpackages/native-shell-ios/Resources/self-sdk-web/fonts/Advercase-Regular.otfis excluded by!**/*.otfpackages/native-shell-ios/Resources/self-sdk-web/fonts/DINOT-Bold.otfis excluded by!**/*.otfpackages/native-shell-ios/Resources/self-sdk-web/fonts/DINOT-Medium.otfis excluded by!**/*.otfpackages/native-shell-ios/Resources/self-sdk-web/fonts/IBMPlexMono-Regular.otfis excluded by!**/*.otfpackages/native-shell-ios/Resources/self-sdk-web/logos/self.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (135)
.claude/skills/self-skills.github/workflows/contracts.yml.github/workflows/mobile-e2e.yml.github/workflows/rn-sdk-test-app-ci.yml.gitignoreapp/.eslintrc.cjscommon/index.tscommon/src/constants/constants.tscommon/src/constants/index.tscommon/src/utils/kyc/constants.tscommon/src/utils/kyc/ecdsa/ecdsa.tscommon/src/utils/kyc/ecdsa/utils.tscommon/src/utils/kyc/generateInputs.tscommon/src/utils/kyc/types.tscommon/src/utils/trees.tscontracts/ignition/deployments/chain-42220/deployed_addresses.jsoncontracts/ignition/modules/upgrade/upgradeKycRegistry.tsdocs/reviews/PR-1901-review-findings.mdnew-common/src/app/builder.test.tsnew-common/src/app/builder.tsnew-common/src/app/index.tsnew-common/src/app/presets.tsnew-common/src/circuits/inputs/disclose-kyc.tsnew-common/src/foundation/types/app.tspackages/mobile-sdk-alpha/.eslintrc.cjspackages/mobile-sdk-alpha/src/browser.tspackages/mobile-sdk-alpha/src/documents/utils.tspackages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/src/proving/internal/statusHandlers.tspackages/mobile-sdk-alpha/src/proving/provingMachine.tspackages/mobile-sdk-alpha/src/proving/recoveryValidation.tspackages/mobile-sdk-alpha/tests/proving/internal/statusHandlers.test.tspackages/mobile-sdk-alpha/tests/proving/recoveryValidation.test.tspackages/mobile-sdk-alpha/tsconfig.eslint.jsonpackages/native-shell-android/build.gradle.ktspackages/native-shell-android/src/main/assets/self-wallet/animations/app-tour-generate.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/app-tour-get-started.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/app-tour-proof.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/app-tour-welcome.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/cloud-backup.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/proof-progress.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/proof-success-check.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/proof-success.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/push-notification-prompt.jsonpackages/native-shell-android/src/main/assets/self-wallet/animations/scan-bar.jsonpackages/native-shell-android/src/main/assets/self-wallet/assets/index-LqDWjDzu.csspackages/native-shell-android/src/main/assets/self-wallet/assets/index-YX6AnLbA.jspackages/native-shell-android/src/main/assets/self-wallet/assets/index.es-CVNUZmbC.jspackages/native-shell-android/src/main/kotlin/xyz/self/sdk/api/SecureStorageProvider.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/api/SelfSdk.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/api/SelfSdkConfig.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/api/SelfSdkLaunchConfig.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/handlers/LifecycleHandler.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/handlers/SecureStorageHandler.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/webview/AndroidWebViewHost.ktpackages/native-shell-android/src/main/kotlin/xyz/self/sdk/webview/SelfVerificationActivity.ktpackages/native-shell-ios/Resources/self-sdk-web/animations/app-tour-generate.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/app-tour-get-started.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/app-tour-proof.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/app-tour-welcome.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/cloud-backup.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/proof-progress.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/proof-success-check.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/proof-success.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/push-notification-prompt.jsonpackages/native-shell-ios/Resources/self-sdk-web/animations/scan-bar.jsonpackages/native-shell-ios/Resources/self-sdk-web/assets/didit-sdk.esm-D5Sol1U1.jspackages/native-shell-ios/Resources/self-sdk-web/assets/index-LqDWjDzu.csspackages/native-shell-ios/Resources/self-sdk-web/assets/index-YX6AnLbA.jspackages/native-shell-ios/Resources/self-sdk-web/assets/index.es-CVNUZmbC.jspackages/native-shell-ios/Resources/self-sdk-web/index.htmlpackages/native-shell-ios/Sources/SelfNativeShell/API/SecureStorageProvider.swiftpackages/native-shell-ios/Sources/SelfNativeShell/API/SelfSdk.swiftpackages/native-shell-ios/Sources/SelfNativeShell/API/SelfSdkConfig.swiftpackages/native-shell-ios/Sources/SelfNativeShell/Handlers/LifecycleHandler.swiftpackages/native-shell-ios/Sources/SelfNativeShell/Handlers/SecureStorageHandler.swiftpackages/native-shell-ios/Sources/SelfNativeShell/WebView/SelfWebViewHost.swiftpackages/rn-sdk-test-app/android/app/debug.keystorepackages/rn-sdk-test-app/scripts/postinstall.cjspackages/sdk-test-app/android/app/build.gradle.ktspackages/sdk-test-app/android/app/src/main/AndroidManifest.xmlpackages/sdk-test-app/android/app/src/main/kotlin/xyz/self/testapp/EncryptedPrefsStorageProvider.ktpackages/sdk-test-app/android/app/src/main/kotlin/xyz/self/testapp/MainActivity.ktpackages/sdk-test-app/android/app/src/main/res/xml/network_security_config.xmlpackages/sdk-test-app/ios/SelfTestApp/ContentView.swiftpackages/sdk-test-app/ios/SelfTestApp/KeychainStorageProvider.swiftpackages/webview-app/package.jsonpackages/webview-app/src/App.tsxpackages/webview-app/src/providers/SelfClientProvider.tsxpackages/webview-app/src/screens/debug/KeychainDebugScreen.tsxpackages/webview-app/src/screens/onboarding/ConfirmIdentificationScreen.tsxpackages/webview-app/src/screens/onboarding/ProviderLaunchScreen.tsxpackages/webview-app/src/screens/proving/DiscloseResultScreen.tsxpackages/webview-app/src/screens/proving/ProofGenerationRouteScreen.tsxpackages/webview-app/src/screens/proving/ProvingScreen.tsxpackages/webview-app/src/screens/proving/VerificationResultScreen.tsxpackages/webview-app/src/screens/recovery/RecoveryFailureScreen.tsxpackages/webview-app/src/screens/recovery/RecoverySuccessScreen.tsxpackages/webview-app/src/screens/recovery/SecretPhraseInputScreen.tsxpackages/webview-app/src/screens/tunnel/KycMockScreen.tsxpackages/webview-app/src/screens/tunnel/TourScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelDiscloseScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelKycSuccessScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelKycWrapper.tsxpackages/webview-app/src/screens/tunnel/TunnelProofReceiptScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelProvingScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelRecoveryRequiredScreen.tsxpackages/webview-app/src/screens/tunnel/TunnelResultScreen.tsxpackages/webview-app/src/utils/buildKycDocument.tspackages/webview-app/src/utils/diditProvider.tspackages/webview-app/src/utils/insets.tspackages/webview-app/src/utils/provingUtils.test.tspackages/webview-app/src/utils/provingUtils.tspackages/webview-app/src/utils/secretManager.tspackages/webview-app/src/utils/selfAppContext.tspackages/webview-app/src/utils/verificationRequest.test.tspackages/webview-app/src/utils/verificationRequest.tspackages/webview-app/tests/screens/recovery/recoverySupportScreens.test.tsxpackages/webview-app/tests/screens/tunnel/tunnelFlowScreens.test.tsxpackages/webview-app/tests/utils/secretManager.test.tssdk/core/README.mdsdk/core/src/SelfBackendVerifier.tssdk/qrcode/README.mdsdk/qrcode/index.tssdk/sdk-common/index.tssdk/sdk-common/package.jsonspecs/projects/sdk/OVERVIEW.mdspecs/projects/sdk/workstreams/kmp-revival/SPEC.mdspecs/projects/sdk/workstreams/kmp-revival/plans/KR-01-android-parity.mdspecs/projects/sdk/workstreams/kmp-revival/plans/KR-02-ios-parity.mdspecs/projects/sdk/workstreams/kmp-revival/plans/KR-03-validate-and-publish.mdspecs/projects/sdk/workstreams/native-shells-lite/SPEC.mdspecs/projects/sdk/workstreams/native-shells-lite/plans/NSL-04-delegate-keychain.mdspecs/projects/sdk/workstreams/webview/SPEC.mdspecs/projects/sdk/workstreams/webview/plans/WV-17-recovery-phrase-restore-flow.md
💤 Files with no reviewable changes (2)
- packages/native-shell-android/build.gradle.kts
- packages/webview-app/src/screens/tunnel/KycMockScreen.tsx
Comment on lines
72
to
94
| - name: Install Dependencies | ||
| uses: ./.github/actions/yarn-install | ||
| - name: Cache Hardhat compilers | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/hardhat-nodejs/compilers-v2 | ||
| key: ${{ runner.os }}-hardhat-compilers-${{ env.GH_HARDHAT_CACHE_VERSION }}-${{ hashFiles('contracts/hardhat.config.ts', 'contracts/package.json', 'yarn.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-hardhat-compilers-${{ env.GH_HARDHAT_CACHE_VERSION }}- | ||
| - name: Prettier Check - Code Formatting | ||
| run: yarn prettier:check | ||
| working-directory: ./contracts | ||
| - name: Build Common Dependencies | ||
| run: yarn workspace @selfxyz/common build | ||
| - name: Build Contracts | ||
| run: yarn build | ||
| working-directory: ./contracts | ||
| uses: nick-fields/retry@v3 | ||
| with: | ||
| timeout_minutes: 15 | ||
| max_attempts: 3 | ||
| retry_wait_seconds: 15 | ||
| command: yarn build | ||
| working_directory: ./contracts | ||
| - name: Run Tests (Contracts) |
Contributor
There was a problem hiding this comment.
Missing required fast-fail guard for nested require() patterns.
Before build/test execution, test_contracts has no step that runs the nested require() validation script, so known OOM-inducing patterns won’t fail early.
As per coding guidelines ".github/workflows/*.yml: Add a CI fast-fail check step that runs the validation script to detect nested require() patterns before test execution to prevent out-of-memory pipeline failures".
Comment on lines
+17
to
+22
| export const generateRandomsg = (): number[] => { | ||
| const randomNumbers: number[] = Array.from({ length: 298 }, () => | ||
| Math.floor(Math.random() * 128) | ||
| ); | ||
| return randomNumbers; | ||
| }; |
Contributor
There was a problem hiding this comment.
Math.random() is not cryptographically secure.
This function lives in ECDSA utilities where randomness quality directly impacts security. Math.random() is a PRNG that can be predicted or brute-forced. If these values serve any cryptographic purpose (blinding factors, nonces, padding), an attacker could potentially exploit the weak randomness.
Use crypto.getRandomValues() (or Node's crypto.randomBytes) for cryptographic contexts:
Suggested fix
-export const generateRandomsg = (): number[] => {
- const randomNumbers: number[] = Array.from({ length: 298 }, () =>
- Math.floor(Math.random() * 128)
- );
- return randomNumbers;
-};
+export const generateRandomsg = (): number[] => {
+ const bytes = new Uint8Array(298);
+ crypto.getRandomValues(bytes);
+ return Array.from(bytes, (b) => b % 128);
+};
Comment on lines
+178
to
+183
| // Use raw bytes directly — .toString('utf-8') corrupts bytes >= 128 | ||
| const raw = Buffer.from(serializedApplicantInfo, 'base64'); | ||
| const msgPadded = [ | ||
| ...Array.from(raw, (b) => Number(b)), | ||
| ...new Array(Math.max(0, KYC_MAX_LENGTH - raw.length)).fill(0), | ||
| ]; |
Contributor
There was a problem hiding this comment.
Prevent oversized data_padded by truncating before padding.
At Line [179], the new byte-preserving path still allows raw.length > KYC_MAX_LENGTH; this can produce oversized circuit inputs and break proving on large payloads.
🔧 Proposed fix
- const raw = Buffer.from(serializedApplicantInfo, 'base64');
- const msgPadded = [
- ...Array.from(raw, (b) => Number(b)),
- ...new Array(Math.max(0, KYC_MAX_LENGTH - raw.length)).fill(0),
- ];
+ const raw = Buffer.from(serializedApplicantInfo, 'base64');
+ const truncated = raw.subarray(0, KYC_MAX_LENGTH);
+ const msgPadded = [
+ ...Array.from(truncated, (b) => Number(b)),
+ ...new Array(KYC_MAX_LENGTH - truncated.length).fill(0),
+ ];
Comment on lines
+339
to
+352
| if (leafs[0] == BigInt(0) || tree.createProof(leafs[0]).membership) { | ||
| console.log('This entry already exists in the tree, skipping...'); | ||
| continue; | ||
| } else if (leafs[1] == BigInt(0) || tree.createProof(leafs[1]).membership) { | ||
| console.log('This entry already exists in the tree, skipping...'); | ||
| continue; | ||
| } | ||
|
|
||
| tree.add(leafs[0], BigInt(1)); | ||
| count += 1; | ||
| if (leafs[0] != leafs[1]) { | ||
| tree.add(leafs[1], BigInt(1)); | ||
| count += 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Data loss: valid entries silently skipped when reverse leaf has issues
If leafs[0] is valid but leafs[1] is zero or already exists, both entries are skipped entirely. Compare with buildAadhaarSMT (lines 70-82) which adds the primary leaf regardless of the reverse leaf's status.
This means valid KYC entries get dropped from the tree if their reverse form happens to collide or fail processing.
Proposed fix to match Aadhaar behavior
- if (leafs[0] == BigInt(0) || tree.createProof(leafs[0]).membership) {
- console.log('This entry already exists in the tree, skipping...');
- continue;
- } else if (leafs[1] == BigInt(0) || tree.createProof(leafs[1]).membership) {
- console.log('This entry already exists in the tree, skipping...');
- continue;
- }
-
- tree.add(leafs[0], BigInt(1));
- count += 1;
- if (leafs[0] != leafs[1]) {
- tree.add(leafs[1], BigInt(1));
- count += 1;
- }
+ if (leafs[0] == BigInt(0) || tree.createProof(leafs[0]).membership) {
+ console.log('This entry already exists in the tree, skipping...');
+ continue;
+ }
+
+ tree.add(leafs[0], BigInt(1));
+ count += 1;
+
+ if (leafs[0] != leafs[1]) {
+ if (leafs[1] == BigInt(0) || tree.createProof(leafs[1]).membership) {
+ console.log('Reverse entry already exists in the tree, skipping...');
+ continue;
+ }
+ tree.add(leafs[1], BigInt(1));
+ count += 1;
+ }
Comment on lines
+1
to
+130
| # PR #1901 Review Findings | ||
|
|
||
| **PR:** Stitch tunnel flow screens with proving machine, recovery, and disclose | ||
| **Branch:** `justin/debug-stitch-screens-build-issues` | ||
| **Reviewed:** 2026-04-01 | ||
|
|
||
| This document reflects the substantive GitHub PR comments on PR #1901. | ||
| It intentionally excludes low-signal review noise such as PR title, PR description, docstring coverage, and bot/meta comments. | ||
|
|
||
| --- | ||
|
|
||
| ## Critical | ||
|
|
||
| ### 1. Remote WebView loading in production without integrity checks | ||
|
|
||
| **Files:** `AndroidWebViewHost.kt:96,116,180` · `SelfWebViewHost.swift:49-55` | ||
|
|
||
| Non-debug builds now load from `https://self-app-alpha.vercel.app/` by default. The WebView still exposes the native bridge (`SelfNativeAndroid`), so a remote alpha deployment becomes privileged app code. A compromise or XSS on that host can drive the native bridge, camera/mic prompts inside host apps. iOS has no local/bundled fallback — offline devices fail at startup. | ||
|
|
||
| **Fix:** Keep bundled `appassets` as the default entrypoint. Gate remote content behind an explicit environment flag and integrity check. Add offline fallback on iOS. | ||
|
|
||
| --- | ||
|
|
||
| ### 2. `null` selected document takes the success path in recovery | ||
|
|
||
| **File:** `SecretPhraseInputScreen.tsx:183-197` | ||
|
|
||
| `loadSelectedDocument()` returning `null` collapses into the same branch as a mock document. That branch calls `restoreSecretFromMnemonic()` and routes to `/tunnel/kyc`, so a missing/unreadable document context overwrites `self_mnemonic` and `self_private_key` without any identity match check. | ||
|
|
||
| **Fix:** Explicitly handle `null` as an error state. Do not proceed with secret restoration when the document cannot be loaded. | ||
|
|
||
| --- | ||
|
|
||
| ### 3. Register phase can be skipped on stale `"completed"` state | ||
|
|
||
| **File:** `TunnelProvingScreen.tsx:107-117` | ||
|
|
||
| Lines 107-114 switch `phase` to `'register'` but keep `initDone` true. On next render, `currentState` can still be `"completed"`, so lines 115-117 immediately route to `/tunnel/proof/disclose` before `init(client, 'register', true)` has produced a fresh register state. | ||
|
|
||
| **Fix:** Reset `initDone` (or the relevant gate) when switching phase to `'register'`, so the completed-state redirect doesn't fire before the new init resolves. | ||
|
|
||
| --- | ||
|
|
||
| ### 4. `setResult` lifecycle handler breaks backwards compatibility | ||
|
|
||
| **File:** `LifecycleHandler.kt:44-47` · `LifecycleHandler.swift:33-38` | ||
|
|
||
| Android now passes the full lifecycle envelope to `EXTRA_RESULT_DATA` instead of the verification payload. Missing `success` field returns `RESULT_OK`. A legacy or malformed bridge payload comes back as `RESULT_OK` with a JSON shape the host app can't parse. iOS similarly passes the full envelope to `onResult` instead of the verification payload. | ||
|
|
||
| **Fix:** Extract `result` from the envelope before passing to `EXTRA_RESULT_DATA` / `onResult`. Treat missing `success` as `false` (fail closed). Support both payload shapes for backwards compatibility. | ||
|
|
||
| --- | ||
|
|
||
| ## Major | ||
|
|
||
| ### 5. Document finalization is half-transactional | ||
|
|
||
| **File:** `recoveryValidation.ts:26-35` | ||
|
|
||
| If `reStorePassportDataWithRightCSCA()` succeeds and `markCurrentDocumentAsRegistered()` throws, the caller only rolls back secret storage. The selected document has already been mutated, so retries run against a partially finalized document. | ||
|
|
||
| **Fix:** Either make the two operations atomic, or roll back the document state on failure. | ||
|
|
||
| --- | ||
|
|
||
| ### 6. In-flight recovery navigates after unmount | ||
|
|
||
| **File:** `SecretPhraseInputScreen.tsx:197,249-268` | ||
|
|
||
| `isMountedRef` only protects `setState` calls. If the user backs out during validation/finalization, `navigate(...)` still fires from the stale request, yanking them onto another route. | ||
|
|
||
| **Fix:** Gate `navigate()` calls behind the `isMountedRef` check as well. | ||
|
|
||
| --- | ||
|
|
||
| ### 7. No error handling on TourScreen final step | ||
|
|
||
| **File:** `TourScreen.tsx:37` | ||
|
|
||
| If `loadSelectedDocument(client)` rejects, `onNext` aborts and the user is stuck on the last tour screen with no recovery path. | ||
|
|
||
| **Fix:** Wrap in try/catch, fall through to `/tunnel/kyc` on failure. | ||
|
|
||
| --- | ||
|
|
||
| ### 8. PII logged in TourScreen | ||
|
|
||
| **File:** `TourScreen.tsx:29` | ||
|
|
||
| `console.log('selected Doc', selectedDoc)` logs the full document payload, which can include passport/KYC PII. | ||
|
|
||
| **Fix:** Remove the log statement. | ||
|
|
||
| --- | ||
|
|
||
| ### 9. Secret snapshot reads are not atomic | ||
|
|
||
| **File:** `secretManager.ts:81` | ||
|
|
||
| `readStoredSecretSnapshot()` runs two `get()` calls outside `secretLock`. A concurrent write can interleave, producing a mnemonic/private-key pair that never existed together. | ||
|
|
||
| **Fix:** Run the read under `withSecretLock()`. | ||
|
|
||
| --- | ||
|
|
||
| ### 10. Snapshot restore is not failure-atomic | ||
|
|
||
| **File:** `secretManager.ts:108-122` | ||
|
|
||
| Two sequential writes with no rollback. If the mnemonic write succeeds and the private-key write fails, storage is left half-restored and mismatched. | ||
|
|
||
| **Fix:** Capture the previous snapshot before writing. On failure, restore the previous values. | ||
|
|
||
| --- | ||
|
|
||
| ### 11. Hardcoded zero insets instead of `WEB_SAFE_AREA` | ||
|
|
||
| **File:** `ProviderLaunchScreen.tsx:297` | ||
|
|
||
| `KycPendingScreen` receives `{ top: 0, bottom: 0 }` instead of `WEB_SAFE_AREA`, causing potential safe-area overlap/clipping on some devices. | ||
|
|
||
| **Fix:** Import and use `WEB_SAFE_AREA` from `src/utils/insets.ts`. | ||
|
|
||
| --- | ||
|
|
||
| ## Additional PR Comment Review | ||
|
|
||
| - Reviewed the current PR comments on `selfxyz/self#1901`. | ||
| - No additional non-pedantic findings need to be added beyond the items above. | ||
| - The inline CodeRabbit comments about iOS legacy `setResult` compatibility and secret snapshot restore atomicity are already covered by findings 4 and 10. |
Contributor
There was a problem hiding this comment.
Unrelated security review document appears committed in this PR
This file documents findings for PR #1901 (different scope/branch), while this PR is for SelfAppBuilder API simplification (#1925). Merging it here creates a real risk of mis-triaging security/incident work and shipping with inaccurate review records.
Please either remove this file from this PR or replace it with review findings that match #1925.
🧰 Tools
🪛 LanguageTool
[style] ~130-~130: Try using a synonym here to strengthen your wording.
Context: ...he items above. - The inline CodeRabbit comments about iOS legacy setResult compatibil...
(COMMENT_REMARK)
Comment on lines
+182
to
+197
| const selectedDocument = await loadSelectedDocument(client); | ||
| const hasRealDocument = selectedDocument && !selectedDocument.metadata.mock; | ||
|
|
||
| if (!hasRealDocument) { | ||
| await restoreSecretFromMnemonic(storage, mnemonic); | ||
|
|
||
| if (isMountedRef.current) { | ||
| setErrorIndices([]); | ||
| setErrorMessage(null); | ||
| setWords([...EMPTY_WORDS]); | ||
| } | ||
|
|
||
| haptic.trigger('success'); | ||
| analytics.trackEvent('recovery_phrase_recovered', { documentCategory: 'none' }); | ||
| navigate('/tunnel/kyc', { replace: true }); | ||
| return; |
Contributor
There was a problem hiding this comment.
Do not restore secrets when recovery has no real document to validate against.
When loadSelectedDocument() returns null or a mock document, this branch immediately writes the mnemonic/private key and routes to /tunnel/kyc. That turns /recovery/phrase-input into an unvalidated secret-replacement path and can overwrite the current identity without proving the phrase matches any selected document. Route to /recovery/failure here without touching storage.
Comment on lines
+39
to
+41
| const onResore = useCallback(() => { | ||
| navigate('/recovery'); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Pass a returnTo target when launching recovery from the tunnel.
RecoverySuccessScreen now resumes callers via location.state.returnTo/?returnTo=. Navigating to bare /recovery here means a successful restore falls back to /, dropping users out of the tunnel verification flow instead of returning them to the current step.
Also applies to: 45-51
Comment on lines
+23
to
+47
| useEffect(() => { | ||
| if (!providerResult) return; | ||
|
|
||
| if (providerResult.status === 'cancel') { | ||
| navigate(-1); | ||
| return; | ||
| } | ||
|
|
||
| if (providerResult.status === 'error') { | ||
| if (providerResult.error?.retryable === false) { | ||
| navigate('/onboarding/failure', { replace: true }); | ||
| } else { | ||
| navigate('/tunnel/kyc', { replace: true }); | ||
| } | ||
| return; | ||
| } | ||
| }, [providerResult, navigate]); | ||
|
|
||
| const onGenerateProof = useCallback(() => { | ||
| haptic.trigger('success'); | ||
| analytics.trackEvent('tunnel_kyc_success_generate_proof'); | ||
| navigate('/tunnel/proof/generating'); | ||
| }, [navigate, haptic, analytics]); | ||
|
|
||
| return <KycVerificationSuccessScreen insets={WEB_SAFE_AREA.insets} onGenerateProof={onGenerateProof} />; |
Contributor
There was a problem hiding this comment.
Gate this screen on an actual successful provider result.
cancel, error, and missing location.state are only handled after the first render, so this component can still paint the success UI — and expose the proof CTA — without a successful KYC completion.
Suggested fix
useEffect(() => {
- if (!providerResult) return;
+ if (!providerResult) {
+ navigate('/tunnel/kyc', { replace: true });
+ return;
+ }
if (providerResult.status === 'cancel') {
navigate(-1);
return;
}
@@
const onGenerateProof = useCallback(() => {
haptic.trigger('success');
analytics.trackEvent('tunnel_kyc_success_generate_proof');
navigate('/tunnel/proof/generating');
}, [navigate, haptic, analytics]);
+ if (providerResult?.status !== 'success') {
+ return null;
+ }
+
return <KycVerificationSuccessScreen insets={WEB_SAFE_AREA.insets} onGenerateProof={onGenerateProof} />;
Comment on lines
+30
to
+41
| export function buildKycDocument(attestation: KycProviderAttestation) { | ||
| const raw = Uint8Array.from(atob(attestation.serializedApplicantInfo), c => c.charCodeAt(0)); | ||
| const idType = parseIdType(raw); | ||
|
|
||
| return { | ||
| documentType: idType, | ||
| documentCategory: 'kyc' as const, | ||
| mock: false, | ||
| signature: attestation.signature, | ||
| pubkey: attestation.pubkey, | ||
| serializedApplicantInfo: attestation.serializedApplicantInfo, | ||
| }; |
Contributor
There was a problem hiding this comment.
Reject malformed applicant payloads before storing them.
This parser assumes the fixed Didit layout, but short valid base64 input currently yields documentType: '' (or truncated garbage) and still returns a 'kyc' document. That writes a corrupted record to storage and breaks later flows far away from the root cause.
🛠️ Fail fast on malformed payloads
+const DIDIT_TEE_PAYLOAD_LENGTH = 295;
+
export function buildKycDocument(attestation: KycProviderAttestation) {
const raw = Uint8Array.from(atob(attestation.serializedApplicantInfo), c => c.charCodeAt(0));
+ if (raw.length !== DIDIT_TEE_PAYLOAD_LENGTH) {
+ throw new Error(`Invalid KYC applicant info payload length: ${raw.length}`);
+ }
const idType = parseIdType(raw);
+ if (!idType) {
+ throw new Error('Invalid KYC applicant info payload');
+ }
return {📝 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 buildKycDocument(attestation: KycProviderAttestation) { | |
| const raw = Uint8Array.from(atob(attestation.serializedApplicantInfo), c => c.charCodeAt(0)); | |
| const idType = parseIdType(raw); | |
| return { | |
| documentType: idType, | |
| documentCategory: 'kyc' as const, | |
| mock: false, | |
| signature: attestation.signature, | |
| pubkey: attestation.pubkey, | |
| serializedApplicantInfo: attestation.serializedApplicantInfo, | |
| }; | |
| const DIDIT_TEE_PAYLOAD_LENGTH = 295; | |
| export function buildKycDocument(attestation: KycProviderAttestation) { | |
| const raw = Uint8Array.from(atob(attestation.serializedApplicantInfo), c => c.charCodeAt(0)); | |
| if (raw.length !== DIDIT_TEE_PAYLOAD_LENGTH) { | |
| throw new Error(`Invalid KYC applicant info payload length: ${raw.length}`); | |
| } | |
| const idType = parseIdType(raw); | |
| if (!idType) { | |
| throw new Error('Invalid KYC applicant info payload'); | |
| } | |
| return { | |
| documentType: idType, | |
| documentCategory: 'kyc' as const, | |
| mock: false, | |
| signature: attestation.signature, | |
| pubkey: attestation.pubkey, | |
| serializedApplicantInfo: attestation.serializedApplicantInfo, | |
| }; |
Re-export SelfApp and SelfAppDisclosureConfig from new-common in sdk-common instead of duplicating the interfaces, fixing the TS2345 type error that broke CI (selfDefinedData missing, Country3LetterCode mismatch). Add staging_celo endpoint validation in SelfAppBuilder alongside celo. Fix empty string bypassing preset validation (use == null instead of falsy check). Stabilize fallback verificationId with useRef to prevent re-renders re-firing the launch effect. Default DiscloseResultScreen success to false when location.state is missing, preventing false-positive verification results. Clear socketConnection on REGISTERED_COMMITMENT status to prevent stale socket references. Remove PII logging of full selectedDoc in TourScreen. Log actual error object in ensureSecret catch handler for debuggability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f74cf60-bae3-4d79-acf6-d8a99159f1c7
📒 Files selected for processing (10)
new-common/src/app/builder.test.tsnew-common/src/app/builder.tsnew-common/src/app/presets.tspackages/mobile-sdk-alpha/src/proving/internal/statusHandlers.tspackages/mobile-sdk-alpha/tests/proving/internal/statusHandlers.test.tspackages/webview-app/src/providers/SelfClientProvider.tsxpackages/webview-app/src/screens/onboarding/ProviderLaunchScreen.tsxpackages/webview-app/src/screens/proving/DiscloseResultScreen.tsxpackages/webview-app/src/screens/tunnel/TourScreen.tsxsdk/sdk-common/index.ts
💤 Files with no reviewable changes (1)
- packages/webview-app/src/screens/tunnel/TourScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/webview-app/src/screens/proving/DiscloseResultScreen.tsx
- packages/mobile-sdk-alpha/src/proving/internal/statusHandlers.ts
- packages/mobile-sdk-alpha/tests/proving/internal/statusHandlers.test.ts
- sdk/sdk-common/index.ts
Comment on lines
+86
to
+87
| ensureSecret(storage).catch(err => { | ||
| console.error('Failed to ensure secret:', err); |
Contributor
There was a problem hiding this comment.
Avoid raw error-object logging in secret initialization path
At Line 87, logging err directly can leak sensitive values from secret/storage failures into logs. Log only a sanitized classification (or masked fields), not the raw object.
Suggested fix
- ensureSecret(storage).catch(err => {
- console.error('Failed to ensure secret:', err);
+ ensureSecret(storage).catch(err => {
+ const safeErrorType = err instanceof Error ? err.name : 'UnknownError';
+ console.error('Failed to ensure secret:', safeErrorType);
});As per coding guidelines: "NEVER log sensitive data including ... credentials, tokens, API keys, private keys, or session identifiers." and "ALWAYS redact/mask sensitive fields in logs..."
📝 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
| ensureSecret(storage).catch(err => { | |
| console.error('Failed to ensure secret:', err); | |
| ensureSecret(storage).catch(err => { | |
| const safeErrorType = err instanceof Error ? err.name : 'UnknownError'; | |
| console.error('Failed to ensure secret:', safeErrorType); | |
| }); |
…egration # Conflicts: # packages/webview-app/src/screens/onboarding/ProviderLaunchScreen.tsx # packages/webview-app/src/screens/tunnel/TourScreen.tsx
Re-exporting types from new-common caused DTS resolution failures in CI because @selfxyz/new-common deep path imports aren't resolvable during declaration generation. Keep inline interfaces in sdk-common but add the missing selfDefinedData field. Also add EUE/UNO/XCE/XPO/XOM country codes to new-common to align Country3LetterCode types across packages.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c6ac65e-4de3-4edc-9270-e070d10d2eda
📒 Files selected for processing (2)
new-common/src/foundation/constants/countries.tssdk/sdk-common/index.ts
| version: number; | ||
| chainID: 42220 | 11142220; | ||
| userDefinedData: string; | ||
| selfDefinedData: string; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the SelfApp interface definition
fd -t f "index.ts" sdk/sdk-common/ | head -20Repository: selfxyz/self
Length of output: 79
🏁 Script executed:
# Check the contents of sdk/sdk-common/index.ts around line 298
wc -l sdk/sdk-common/index.tsRepository: selfxyz/self
Length of output: 83
🏁 Script executed:
# Search for SelfApp interface definition
rg "interface SelfApp" --context 30Repository: selfxyz/self
Length of output: 20337
🏁 Script executed:
# Check if selfDefinedData is new or was previously optional
rg "selfDefinedData" --context 5Repository: selfxyz/self
Length of output: 43257
🏁 Script executed:
# Look for usage patterns - who constructs/mocks SelfApp
rg "SelfApp\s*[{:]" --context 3 | head -50Repository: selfxyz/self
Length of output: 3172
selfDefinedData must remain optional for backward compatibility.
SelfApp is public API in @selfxyz/sdk-common. Making selfDefinedData required breaks any code that constructs SelfApp objects directly. The builder pattern elsewhere in the codebase already provides a default empty string, so keeping the interface field optional preserves compatibility while allowing the builder to populate it by default.
Suggested fix
export interface SelfApp {
appName: string;
logoBase64: string;
endpointType: EndpointType;
endpoint: string;
deeplinkCallback: string;
header: string;
scope: string;
sessionId: string;
userId: string;
userIdType: UserIdType;
devMode: boolean;
disclosures: SelfAppDisclosureConfig;
version: number;
chainID: 42220 | 11142220;
userDefinedData: string;
- selfDefinedData: string;
+ selfDefinedData?: string;
}📝 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
| selfDefinedData: string; | |
| export interface SelfApp { | |
| appName: string; | |
| logoBase64: string; | |
| endpointType: EndpointType; | |
| endpoint: string; | |
| deeplinkCallback: string; | |
| header: string; | |
| scope: string; | |
| sessionId: string; | |
| userId: string; | |
| userIdType: UserIdType; | |
| devMode: boolean; | |
| disclosures: SelfAppDisclosureConfig; | |
| version: number; | |
| chainID: 42220 | 11142220; | |
| userDefinedData: string; | |
| selfDefinedData?: string; | |
| } |
Contributor
Author
|
@greptileai review |
sdk-common now imports from new-common, so new-common must be built before sdk-common in the QRCode SDK CI pipeline. Also fix prettier formatting in sdk/qrcode/README.md that was failing workspace-lint.
Contributor
Greptile SummaryThis PR simplifies the
Confidence Score: 4/5Safe to merge; two minor P2 gaps (staging_https validation and CI trigger path) but neither blocks production use The builder changes are well-tested (37 tests), backwards-compatible, and the auto-detection logic is straightforward. The staging_https validation gap only affects an internal/testing endpoint type. The missing CI trigger path is a coverage gap but not a runtime risk. The sdk-common re-export path resolves correctly via new-common's package.json exports field. new-common/src/app/builder.ts (staging_https validation), .github/workflows/qrcode-sdk-ci.yml (trigger paths) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+72
to
82
| if (endpointType === 'https' && !config.endpoint.startsWith('https://')) { | ||
| const suggestion = config.endpoint.startsWith('http://') | ||
| ? ` Did you mean '${config.endpoint.replace('http://', 'https://')}'?` | ||
| : ''; | ||
| throw new Error(`endpoint must start with https://.${suggestion}`); | ||
| } | ||
| if (config.endpointType === 'celo' && !config.endpoint.startsWith('0x')) { | ||
| throw new Error('endpoint must be a valid address'); | ||
| if ((endpointType === 'celo' || endpointType === 'staging_celo') && !config.endpoint.startsWith('0x')) { | ||
| throw new Error( | ||
| `Endpoint must be a valid contract address (starting with 0x) for endpointType '${endpointType}'. Got: '${config.endpoint}'`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
staging_https skips endpoint format validation
The two endpoint format checks cover 'https' (requires https://) and 'celo'/'staging_celo' (requires 0x), but 'staging_https' is a valid EndpointType that falls through both conditions with no validation. A caller can pass endpointType: 'staging_https' with a bare hostname or an http:// URL and the builder will silently accept it.
Suggested change
| if (endpointType === 'https' && !config.endpoint.startsWith('https://')) { | |
| const suggestion = config.endpoint.startsWith('http://') | |
| ? ` Did you mean '${config.endpoint.replace('http://', 'https://')}'?` | |
| : ''; | |
| throw new Error(`endpoint must start with https://.${suggestion}`); | |
| } | |
| if (config.endpointType === 'celo' && !config.endpoint.startsWith('0x')) { | |
| throw new Error('endpoint must be a valid address'); | |
| if ((endpointType === 'celo' || endpointType === 'staging_celo') && !config.endpoint.startsWith('0x')) { | |
| throw new Error( | |
| `Endpoint must be a valid contract address (starting with 0x) for endpointType '${endpointType}'. Got: '${config.endpoint}'`, | |
| ); | |
| } | |
| if (endpointType === 'https' || endpointType === 'staging_https') { | |
| if (!config.endpoint.startsWith('https://')) { | |
| const suggestion = config.endpoint.startsWith('http://') | |
| ? ` Did you mean '${config.endpoint.replace('http://', 'https://')}'?` | |
| : ''; | |
| throw new Error(`endpoint must start with https://.${suggestion}`); | |
| } | |
| } | |
| if ((endpointType === 'celo' || endpointType === 'staging_celo') && !config.endpoint.startsWith('0x')) { | |
| throw new Error( | |
| `Endpoint must be a valid contract address (starting with 0x) for endpointType '${endpointType}'. Got: '${config.endpoint}'`, | |
| ); | |
| } |
Route sdk-common's DISCLOSURE_PRESETS and resolveDisclosures export through the new-common app barrel so runtime module resolution finds a built output. The direct src/app/presets subpath was not a tsup entry, so dist/cjs/src/app/presets.cjs was never emitted and require resolution failed. Emit a package.json into sdk-common/dist/cjs with type commonjs so node treats the tsc CJS output as CJS despite the parent package using type module. Without this, node loads dist/cjs/index.js as ESM and transitive requires into new-common subpaths fail with MODULE_NOT_FOUND. Reorder sdk/qrcode/index.ts exports so types are grouped first in ASCII order and value exports follow in ASCII order, satisfying the sort-exports lint rule.
quality-checks and integration-test restore the sdk-build cache and only rebuild dependencies on cache miss. The cache paths covered common, sdk-common, and qrcode but not new-common. sdk-common now imports DISCLOSURE_PRESETS and resolveDisclosures from the new-common app barrel, so on a cache hit those jobs failed with MODULE_NOT_FOUND when requiring the built module, and TypeScript type-checking failed to resolve the subpath types. Add new-common/dist to the cache save and restore paths so the dependencies built in the build job are actually visible to downstream jobs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reduces the minimum Self SDK integration from 4+ required fields down to 3 fields with zero config, while keeping full backwards compatibility. Linked to SELF-1367.
New APIs
Factory methods with clear parameter names for each integration type:
Disclosure presets —
'basic-kyc','age-verification','full-passport','ofac-only'instead of manual boolean config.Auto-detection —
endpointTypeinferred from0x/https://prefix,userIdTypeinferred from0xprefix,userIdauto-generated if omitted.Actionable validation errors — errors now include what was received and suggestions (e.g. "Did you mean 'https://...'?").
frontendConfig()onSelfBackendVerifier— generates matching frontend config snippet.Changes
new-common/src/app/builder.ts— Core builder: auto-detection, optional userId, factory methods, improved errorsnew-common/src/app/presets.ts— Disclosure preset definitions and resolvernew-common/src/foundation/types/app.ts—SelfAppBuilderConfig,DisclosurePresetNametypessdk/sdk-common/index.ts— Replace duplicate builder with re-export from new-commonsdk/core/src/SelfBackendVerifier.ts— AddfrontendConfig()helpersdk/qrcode/index.ts— Re-export presets and new typesBackwards Compatibility
All changes are additive. Existing
new SelfAppBuilder({ ... })calls work unchanged. The old duplicate builder in sdk-common is replaced with a re-export from new-common (same API, now with additional features).Test plan
cd new-common && yarn test— 37 tests passingcd new-common && yarn types— cleancd new-common && yarn format:check— cleancd sdk/sdk-common && yarn build— passesSummary by CodeRabbit
New Features
basic-kyc,age-verification,full-passport,ofac-only).Bug Fixes
Documentation