fix: standardize SelfAppDisclosureConfig to all snake_case - #1910
fix: standardize SelfAppDisclosureConfig to all snake_case#1910Tranquil-Flow wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request standardizes disclosure configuration key names from camelCase to snake_case across the codebase ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/native-shell-ios/Sources/SelfNativeShell/Handlers/LifecycleHandler.swift (1)
33-38:⚠️ Potential issue | 🟠 MajorVerify the
setResultpayload contract before shipping this.Line 34 changes
onResultfrom receivingparams["result"]to receiving the entireparamsmap. If the bridge still sends{ result: ... }, iOS callbacks will now receive a differently shaped payload and existing consumers will parse the wrong object.Run this read-only check to confirm the web bridge and both native handlers agree on the same payload shape:
#!/bin/bash set -euo pipefail printf '\n# JS/TS call sites for lifecycle.setResult\n' rg -n -C3 '\bsetResult\s*\(' --type=ts --type=tsx --type=js --type=jsx printf '\n# Places where "setResult" payloads are constructed\n' rg -n -C4 '"setResult"|'\''setResult'\''' packages --type=ts --type=tsx --type=js --type=jsx printf '\n# Native handlers on both platforms\n' rg -n -C4 'setResult' packages/native-shell-ios packages/native-shell-androidExpected result: all producers and consumers use the same contract—either the raw result object or a
{ result: ... }envelope—but not a mix of both.common/src/utils/trees.ts (1)
856-858:⚠️ Potential issue | 🟠 Major
poseidon4will throw ifyearis not exactly 4 characters.
processYearKycpassesyeardirectly toposeidon4, which requires exactly 4 inputs. Called fromgetNameYobLeafKyc(line 772) with no length validation, this will crash if the caller passes a 2-digit year or any non-4-char string.Proposed fix
const processYearKyc = (year: string, i: number): bigint => { + if (year.length !== 4) { + console.error('Invalid year length for processYearKyc:', i, year); + return BigInt(0); + } const yearArr = stringToAsciiBigIntArray(year); return BigInt(poseidon4(yearArr)); };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7f9be31-c0ef-4551-9f6d-65da613b19d6
⛔ 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.cjsapp/src/navigation/deeplinks.tsapp/src/screens/home/ProofHistoryDetailScreen.tsxapp/src/utils/disclosureUtils.tscommon/index.tscommon/src/constants/constants.tscommon/src/constants/index.tscommon/src/utils/appType.tscommon/src/utils/circuits/registerInputs.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/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/qrcode-angular/README.mdsdk/qrcode-angular/src/lib/common.tssdk/qrcode/README.mdsdk/sdk-common/index.tsspecs/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
| export const getNameDobLeafKyc = (name: string, dob: string) => { | ||
| const namePaddingLength = 64; | ||
| const paddedName = name | ||
| .padEnd(namePaddingLength, '\0') | ||
| .split('') | ||
| .map((char) => char.charCodeAt(0)); | ||
| const nameHash = BigInt(packBytesAndPoseidon(paddedName)); | ||
| const dobHash = BigInt(poseidon8(stringToAsciiBigIntArray(dob))); | ||
| return generateSmallKey(poseidon2([dobHash, nameHash])); | ||
| }; |
There was a problem hiding this comment.
poseidon8 will throw if dob is not exactly 8 characters.
Line 618 passes dob through stringToAsciiBigIntArray to poseidon8, which requires exactly 8 inputs. If a caller provides a date in any format other than 8 characters (e.g., "2000-12-31" or "12/31/2000"), this will crash at runtime.
Proposed fix
export const getNameDobLeafKyc = (name: string, dob: string) => {
const namePaddingLength = 64;
const paddedName = name
.padEnd(namePaddingLength, '\0')
.split('')
.map((char) => char.charCodeAt(0));
const nameHash = BigInt(packBytesAndPoseidon(paddedName));
+ const dobArr = stringToAsciiBigIntArray(dob);
+ if (dobArr.length !== 8) {
+ throw new Error(`dob must be exactly 8 characters (YYYYMMDD), got ${dob.length}`);
+ }
- const dobHash = BigInt(poseidon8(stringToAsciiBigIntArray(dob)));
+ const dobHash = BigInt(poseidon8(dobArr));
return generateSmallKey(poseidon2([dobHash, nameHash]));
};| // Status 5 with REGISTERED_COMMITMENT → route to recovery flow | ||
| if (data.status === 5 && data.error_code === 'REGISTERED_COMMITMENT') { | ||
| return { | ||
| shouldDisconnect: true, | ||
| actorEvent: { type: 'PROVE_ALREADY_REGISTERED' }, | ||
| analytics: [ | ||
| { | ||
| event: 'SOCKETIO_PROOF_FAILURE', | ||
| data: { | ||
| error_code: data.error_code, | ||
| reason: data.reason, | ||
| }, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Scope REGISTERED_COMMITMENT remap to register flow only
At Line 41, this branch triggers PROVE_ALREADY_REGISTERED for any circuit. That can misroute non-register flows into account recovery instead of normal failure handling.
Proposed fix
- if (data.status === 5 && data.error_code === 'REGISTERED_COMMITMENT') {
+ if (circuitType === 'register' && data.status === 5 && data.error_code === 'REGISTERED_COMMITMENT') {
return {
shouldDisconnect: true,
actorEvent: { type: 'PROVE_ALREADY_REGISTERED' },
analytics: [
{
event: 'SOCKETIO_PROOF_FAILURE',
data: {
error_code: data.error_code,
reason: data.reason,
},
},
],
};
}📝 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.
| // Status 5 with REGISTERED_COMMITMENT → route to recovery flow | |
| if (data.status === 5 && data.error_code === 'REGISTERED_COMMITMENT') { | |
| return { | |
| shouldDisconnect: true, | |
| actorEvent: { type: 'PROVE_ALREADY_REGISTERED' }, | |
| analytics: [ | |
| { | |
| event: 'SOCKETIO_PROOF_FAILURE', | |
| data: { | |
| error_code: data.error_code, | |
| reason: data.reason, | |
| }, | |
| }, | |
| ], | |
| }; | |
| } | |
| // Status 5 with REGISTERED_COMMITMENT → route to recovery flow | |
| if (circuitType === 'register' && data.status === 5 && data.error_code === 'REGISTERED_COMMITMENT') { | |
| return { | |
| shouldDisconnect: true, | |
| actorEvent: { type: 'PROVE_ALREADY_REGISTERED' }, | |
| analytics: [ | |
| { | |
| event: 'SOCKETIO_PROOF_FAILURE', | |
| data: { | |
| error_code: data.error_code, | |
| reason: data.reason, | |
| }, | |
| }, | |
| ], | |
| }; | |
| } |
| if (!version || major < 3 || (major === 3 && minor < 2)) { | ||
| console.log(`[rn-sdk-test-app] Skipping iOS pod install: Ruby ${version || 'not found'} < 3.2 required. Install Ruby 3.2+ to enable.`); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
Early exit bypasses .xcode.env.local cleanup and can leave stale build state.
On this return path, the script exits before the cleanup at Line 46. If .xcode.env.local already exists, it remains and can poison later iOS builds with stale env values. Run that cleanup before this process.exit(0) (or in a shared finally-style path) so all exits are safe.
Proposed fix
const rubyVersion = spawnSync('ruby', ['-e', 'puts RUBY_VERSION'], { encoding: 'utf8' });
const version = (rubyVersion.stdout || '').trim();
const [major, minor] = version.split('.').map(Number);
if (!version || major < 3 || (major === 3 && minor < 2)) {
+ fs.rmSync(path.join(iosDir, '.xcode.env.local'), { force: true });
console.log(`[rn-sdk-test-app] Skipping iOS pod install: Ruby ${version || 'not found'} < 3.2 required. Install Ruby 3.2+ to enable.`);
process.exit(0);
}📝 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.
| if (!version || major < 3 || (major === 3 && minor < 2)) { | |
| console.log(`[rn-sdk-test-app] Skipping iOS pod install: Ruby ${version || 'not found'} < 3.2 required. Install Ruby 3.2+ to enable.`); | |
| process.exit(0); | |
| } | |
| if (!version || major < 3 || (major === 3 && minor < 2)) { | |
| fs.rmSync(path.join(iosDir, '.xcode.env.local'), { force: true }); | |
| console.log(`[rn-sdk-test-app] Skipping iOS pod install: Ruby ${version || 'not found'} < 3.2 required. Install Ruby 3.2+ to enable.`); | |
| process.exit(0); | |
| } |
| const onNext = useCallback(async () => { | ||
| if (stepNum < 4) { | ||
| navigate(`/tunnel/tour/${stepNum + 1}`); | ||
| return; | ||
| } | ||
|
|
||
| const selectedDoc = await loadSelectedDocument(client); | ||
|
|
There was a problem hiding this comment.
Catch loadSelectedDocument() failures before the last tour step dead-ends.
If that read rejects, onNext exits with an unhandled promise rejection and the user stays stuck on step 4. This path needs a fallback route instead of letting the error escape.
🔧 Proposed fix
const onNext = useCallback(async () => {
if (stepNum < 4) {
navigate(`/tunnel/tour/${stepNum + 1}`);
return;
}
- const selectedDoc = await loadSelectedDocument(client);
+ let selectedDoc = null;
+ try {
+ selectedDoc = await loadSelectedDocument(client);
+ } catch {
+ navigate('/tunnel/kyc');
+ return;
+ }
console.log('selected Doc', selectedDoc);
const isRegisteredRealDoc = selectedDoc?.metadata?.isRegistered === true;📝 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.
| const onNext = useCallback(async () => { | |
| if (stepNum < 4) { | |
| navigate(`/tunnel/tour/${stepNum + 1}`); | |
| return; | |
| } | |
| const selectedDoc = await loadSelectedDocument(client); | |
| const onNext = useCallback(async () => { | |
| if (stepNum < 4) { | |
| navigate(`/tunnel/tour/${stepNum + 1}`); | |
| return; | |
| } | |
| let selectedDoc = null; | |
| try { | |
| selectedDoc = await loadSelectedDocument(client); | |
| } catch { | |
| navigate('/tunnel/kyc'); | |
| return; | |
| } | |
| console.log('selected Doc', selectedDoc); | |
| const isRegisteredRealDoc = selectedDoc?.metadata?.isRegistered === true; |
|
|
||
| const selectedDoc = await loadSelectedDocument(client); | ||
|
|
||
| console.log('selected Doc', selectedDoc); |
There was a problem hiding this comment.
Remove the raw selectedDoc log.
selectedDoc is loaded from persisted identity state, so dumping the whole object here can leak document metadata or PII into browser/native logs.
🔧 Proposed fix
- console.log('selected Doc', selectedDoc);📝 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.
| console.log('selected Doc', selectedDoc); |
| } catch (err) { | ||
| analytics.trackEvent('tunnel_result_failure', { | ||
| error: err instanceof Error ? err.message : 'Failed to send result', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Silent failure leaves user stranded on success screen without sending result.
If lifecycle.setResult throws, the catch block logs analytics but doesn't navigate away, show an error, or retry. The user sees the success screen, believes everything worked, and may dismiss—but the verification result was never delivered to the requesting app.
Proposed fix
} catch (err) {
analytics.trackEvent('tunnel_result_failure', {
error: err instanceof Error ? err.message : 'Failed to send result',
});
+ navigate('/tunnel/proof/result', {
+ replace: true,
+ state: { success: false, error: 'Failed to send verification result. Please try again.' },
+ });
}| * For browser preview, we use small fixed values for visual padding. | ||
| */ | ||
| export const WEB_SAFE_AREA = createSafeAreaProps({ top: 16, bottom: 16 }); | ||
| export const WEB_SAFE_AREA = createSafeAreaProps({ top: 0, bottom: 16 }); |
There was a problem hiding this comment.
Restore a non-zero top preview inset.
Line 8 removes the top padding from WEB_SAFE_AREA, so every browser-preview screen using this helper will render flush against the top edge. That regresses the safe-area padding this utility is meant to provide.
Suggested fix
-export const WEB_SAFE_AREA = createSafeAreaProps({ top: 0, bottom: 16 });
+export const WEB_SAFE_AREA = createSafeAreaProps({ top: 16, bottom: 16 });📝 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.
| export const WEB_SAFE_AREA = createSafeAreaProps({ top: 0, bottom: 16 }); | |
| export const WEB_SAFE_AREA = createSafeAreaProps({ top: 16, bottom: 16 }); |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/screens/home/ProofHistoryList.tsx (1)
190-204:⚠️ Potential issue | 🟠 MajorDisclosure count is inflated after normalization
After Line 190 normalization,
disclosuresincludesminimum_age(number) andexcluded_countries(array). The current truthy-value count at Line 202 over-counts requested disclosures.Proposed fix
- const disclosureCount = Object.values(disclosures).filter( - value => value, - ).length; + const disclosureCount = [ + disclosures.issuing_state, + disclosures.name, + disclosures.passport_number, + disclosures.nationality, + disclosures.date_of_birth, + disclosures.gender, + disclosures.expiry_date, + disclosures.ofac, + (disclosures.excluded_countries?.length ?? 0) > 0, + disclosures.minimum_age != null, + ].filter(Boolean).length;app/src/screens/home/ProofHistoryScreen.tsx (1)
195-210:⚠️ Potential issue | 🟠 MajorDisclosure badge count miscalculates normalized configs
With normalized disclosures (Line 195), counting all truthy values (Line 208) includes non-boolean fields and inflates the badge count.
Proposed fix
- const disclosureCount = Object.values(disclosures).filter( - value => value, - ).length; + const disclosureCount = [ + disclosures.issuing_state, + disclosures.name, + disclosures.passport_number, + disclosures.nationality, + disclosures.date_of_birth, + disclosures.gender, + disclosures.expiry_date, + disclosures.ofac, + (disclosures.excluded_countries?.length ?? 0) > 0, + disclosures.minimum_age != null, + ].filter(Boolean).length;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddea747a-ca16-4862-b2c9-a4b0507f6f0d
📒 Files selected for processing (5)
app/src/screens/home/ProofHistoryList.tsxapp/src/screens/home/ProofHistoryScreen.tsxpackages/webview-app/src/utils/selfAppContext.tssdk/qrcode-angular/src/lib/common.tssdk/sdk-common/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/sdk-common/index.ts
- sdk/qrcode-angular/src/lib/common.ts
- packages/webview-app/src/utils/selfAppContext.ts
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c023eba9-77d4-4b39-bbaf-9c0ffd6e29c3
📒 Files selected for processing (4)
common/src/utils/appType.tsnew-common/src/app/builder.tssdk/qrcode-angular/src/lib/common.tssdk/sdk-common/index.ts
| function normalizeDisclosures(raw: Record<string, unknown>): Record<string, unknown> { | ||
| const result = { ...raw }; | ||
| if ('excludedCountries' in result && !('excluded_countries' in result)) { | ||
| result.excluded_countries = result.excludedCountries; | ||
| } | ||
| delete result.excludedCountries; | ||
| if ('minimumAge' in result && !('minimum_age' in result)) { | ||
| result.minimum_age = result.minimumAge; | ||
| } | ||
| delete result.minimumAge; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The Angular SDK now returns a disclosure object that disagrees with its public type.
The local SelfAppDisclosureConfig still advertises excludedCountries / minimumAge, but normalizeDisclosures() removes those keys and Line 650 saves only excluded_countries / minimum_age. Any consumer that reads builder.build().disclosures.minimumAge will compile and then get undefined at runtime. Keep the returned public object camelCase, or dual-expose both key styles until this SDK surface is migrated.
Also applies to: 650-650
| function normalizeDisclosures( | ||
| raw: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| const result = { ...raw }; | ||
| if ('excludedCountries' in result && !('excluded_countries' in result)) { | ||
| result.excluded_countries = result.excludedCountries; | ||
| } | ||
| delete result.excludedCountries; | ||
| if ('minimumAge' in result && !('minimum_age' in result)) { | ||
| result.minimum_age = result.minimumAge; | ||
| } | ||
| delete result.minimumAge; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Returned SDK objects no longer match the public disclosure type.
SelfAppDisclosureConfig in this file still exposes excludedCountries and minimumAge, but this helper deletes those keys and Line 656 stores only excluded_countries / minimum_age in SelfApp.disclosures. That means new SelfAppBuilder(...).build().disclosures.minimumAge still type-checks for integrators and now returns undefined at runtime. This mismatch is already visible in packages/webview-app/src/utils/selfAppContext.ts:1-50, which now populates snake_case keys against this type. Keep the public object camelCase, or expose both key styles on the returned object during the transition.
Also applies to: 656-658
Set WEB_SAFE_AREA top to 0 — native shell handles status bar insets edge-to-edge. Add flex wrappers to ConfirmIdentificationScreen and VerificationResultScreen so StatusState fills viewport. Bump @selfxyz/euclid and euclid-core to 1.4.0 (edge-to-edge backgrounds, responsive animations, centered text, fixed button visibility). Delete orphaned KycMockScreen replaced by TunnelKycWrapper.
excludedCountries and minimumAge were camelCase while all other fields in SelfAppDisclosureConfig used snake_case. Renames them to excluded_countries and minimum_age across all four type definition files (common, new-common, sdk-common, qrcode-angular) and updates all consumers. Adds normalizeDisclosureConfig() to handle backward compatibility when parsing QR/deep-link payloads and stored proof history that still use the old camelCase keys. The native shell wire format (minimumAge: prefix in the disclosures URL param) is preserved unchanged — only the TypeScript type fields and their consumers are renamed.
Add normalizeDisclosureConfig() to ProofHistoryScreen and ProofHistoryList where stored disclosure JSON is parsed, for consistency with ProofHistoryDetailScreen. Add normalizeDisclosureConfig() to the two standalone SDK copies (sdk-common, qrcode-angular) so their consumers have access to the migration helper without importing from @selfxyz/common. Add comment in selfAppContext.ts explaining the intentional minimumAge: wire-format prefix.
…nally Reverts the public SDK interface (sdk-common, qrcode-angular) back to camelCase for minimumAge and excludedCountries to avoid a breaking change for integrators. All four SelfAppBuilder copies (common, new-common, sdk-common, qrcode-angular) now normalize camelCase to snake_case in the constructor, ensuring internal code always receives consistent snake_case keys. Removes normalizeDisclosureConfig from public SDK exports. App-side normalize calls remain for stored proof history and deeplink data that may contain old camelCase keys.
Math.random() is not cryptographically secure and must not be used for generating values in ECDSA cryptographic operations. Replace with crypto.getRandomValues() to prevent potential key recovery attacks.
…ionalityLeaf Function declares bigint return type but had code paths returning undefined via bare return statements and a missing return in the catch block. This could propagate undefined where bigint is expected.
Tranquil-Flow
changed the title
76633ac to
1f44ce3
Compare
GitHub Actions
Failure
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: 5114e724-eb1f-4b46-91a7-82cb7c8b41d2
📒 Files selected for processing (13)
.gitignoreapp/src/navigation/deeplinks.tsapp/src/screens/home/ProofHistoryDetailScreen.tsxapp/src/screens/home/ProofHistoryList.tsxapp/src/screens/home/ProofHistoryScreen.tsxapp/src/utils/disclosureUtils.tscommon/src/utils/appType.tscommon/src/utils/circuits/registerInputs.tscommon/src/utils/kyc/ecdsa/utils.tscommon/src/utils/trees.tsnew-common/src/app/builder.tsnew-common/src/foundation/types/app.tspackages/webview-app/src/utils/selfAppContext.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- app/src/screens/home/ProofHistoryScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/webview-app/src/utils/selfAppContext.ts
- new-common/src/foundation/types/app.ts
- app/src/screens/home/ProofHistoryDetailScreen.tsx
- common/src/utils/appType.ts
- app/src/utils/disclosureUtils.ts
| if (passport.length !== 9) { | ||
| console.log('parsed passport length is not 9:', i, passport); | ||
| return; | ||
| return BigInt(0); | ||
| } | ||
| if (nationality.length !== 3) { | ||
| console.log('parsed nationality length is not 3:', i, nationality); | ||
| return; | ||
| return BigInt(0); | ||
| } | ||
| try { | ||
| const fullHash = poseidon12(passport.concat(nationality)); | ||
| return generateSmallKey(fullHash); | ||
| } catch (err) { | ||
| console.log('err : passport', err, i, passport); | ||
| return BigInt(0); |
There was a problem hiding this comment.
Remove sensitive identity fields from error logs in this function.
Line 827/Line 838 log passport, and Line 831 logs nationality. These are sensitive identity-derived values and should not be emitted to logs.
Proposed fix
export function getPassportNumberAndNationalityLeaf(
passport: (bigint | number)[],
nationality: (bigint | number)[],
i?: number
): bigint {
if (passport.length !== 9) {
- console.log('parsed passport length is not 9:', i, passport);
+ console.warn('parsed passport length is not 9:', i, { passportLength: passport.length });
return BigInt(0);
}
if (nationality.length !== 3) {
- console.log('parsed nationality length is not 3:', i, nationality);
+ console.warn('parsed nationality length is not 3:', i, { nationalityLength: nationality.length });
return BigInt(0);
}
try {
const fullHash = poseidon12(passport.concat(nationality));
return generateSmallKey(fullHash);
} catch (err) {
- console.log('err : passport', err, i, passport);
+ console.error('err : passport hash generation failed', err, i);
return BigInt(0);
}
}As per coding guidelines: "NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers."
The lockfile was stale after rebasing, causing CI to fail with Yarn's hardened mode lockfile validation.
GitHub Actions
Inactive
|
@greptileai review |
Greptile SummaryThis PR standardizes Key changes:
Issue — Confidence Score: 3/5Not safe to merge as-is — two public SDK files have a type/runtime mismatch that will silently break any consumer accessing the renamed disclosure fields. The app-internal changes are correct and complete. However,
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function normalizeDisclosures(raw: Record<string, unknown>): Record<string, unknown> { | ||
| const result = { ...raw }; | ||
| if ('excludedCountries' in result && !('excluded_countries' in result)) { | ||
| result.excluded_countries = result.excludedCountries; | ||
| } | ||
| delete result.excludedCountries; | ||
| if ('minimumAge' in result && !('minimum_age' in result)) { | ||
| result.minimum_age = result.minimumAge; | ||
| } | ||
| delete result.minimumAge; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
normalizeDisclosures in this file is identical to normalizeDisclosureConfig in common/src/utils/appType.ts (and duplicated again in sdk/sdk-common and sdk/qrcode-angular). If new-common can import from @selfxyz/common, consider importing the shared utility instead of maintaining four copies of the same logic.
If standalone packaging constraints prevent shared imports, this is acceptable, but worth tracking so future changes to the normalization logic don't diverge across copies.
1 participant
Summary
excludedCountries→excluded_countriesandminimumAge→minimum_ageinSelfAppDisclosureConfigacross all four type definition files (common,new-common,sdk-common,qrcode-angular)registerInputs.ts,disclosureUtils.ts,selfAppContext.ts,ProofHistoryDetailScreen.tsxnormalizeDisclosureConfig()incommon/src/utils/appType.tsfor backward compatibility with old QR/deep-link payloads and stored proof history that still use camelCase keysNotes
The native shell wire format (
minimumAge:prefix in thedisclosuresURL param, and?excludedCountries=URL param) is intentionally unchanged — this is an internal transport layer. Only the TypeScript type fields and their consumers are renamed.Closes SELF-1194
Summary by CodeRabbit
Bug Fixes
Chores