SELF-531: Clean the parameters in the Self SDK #793
Conversation
…ed components - Updated SelfApp interface to use logoUrl instead of logoBase64. - Modified SelfAppBuilder and README files to reflect the change in logo property. - Adjusted SelfBackendVerifier to accept network type instead of mockPassport. - Updated related documentation and examples to ensure consistency across the codebase.
- Change mockPassport boolean parameter to network ('mainnet' | 'testnet')
- Update RPC URL logic to use network-specific endpoints
- Export Network type for type safety
- BREAKING CHANGE: SelfBackendVerifier constructor API updated
- Replace logoBase64 with logoUrl in SelfApp interface (PNG URLs only) - Set version 2 as automatic default in SelfAppBuilder - Update constructor to use explicit assignment instead of spread - BREAKING CHANGE: SelfApp interface now uses logoUrl instead of logoBase64
- Update ProofHistory interface to use logoUrl instead of logoBase64 - Maintain database backward compatibility (logoBase64 column stores logoUrl) - Add type casting for database row mapping to handle schema transition - Update proof insertion logic to use new logoUrl property
- Update ProveScreen to use logoUrl from selectedApp - Update ProofHistoryScreen logo rendering logic for logoUrl - Update ProofHistoryDetailScreen logo source handling for logoUrl - Maintain compatibility for both URL and base64 logo formats in UI
- Replace logoBase64 with logoUrl in database test mock data - Update proofHistoryStore test fixtures to use logoUrl - Update proving machine test to use logoUrl in selfApp mock - Use realistic URL examples instead of base64 strings in tests
- Update SelfBackendVerifier example to use 'mainnet' string instead of false - Add clear comment explaining network parameter options - Ensure documentation reflects the new network parameter API
- Add comprehensive developer documentation for Claude Code AI assistant - Include monorepo structure, development commands, and architecture patterns - Document testing strategies and key files for understanding the codebase - Provide guidance for AI-assisted development workflows
WalkthroughThis update replaces all usage of the Changes
Sequence Diagram(s)sequenceDiagram
participant AppScreen as React Native Screen
participant Store as ProofHistoryStore
participant DB as SQLite DB
AppScreen->>Store: addProof({logoUrl, ...})
Store->>DB: insertProof({logoUrl → logoBase64 column, ...})
DB-->>Store: Insert result
Store-->>AppScreen: Updated proof history (logoUrl)
AppScreen->>AppScreen: Render logo using logoUrl (only supports URLs)
sequenceDiagram
participant SDKUser as Developer
participant SDK as SelfBackendVerifier
SDKUser->>SDK: new SelfBackendVerifier(network: 'mainnet' | 'testnet')
SDK->>SDK: Select RPC URL & contract address based on network
SDK-->>SDKUser: Instance ready with correct endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kevinsslin
changed the title
- Fixed QRCode workspace formatting issue that was causing lint failures - Added comprehensive CI/CD requirements section to CLAUDE.md covering: - All GitHub Actions workflows (general-checks, app, circuits, contracts) - Security scanning setup (Gitleaks, GitGuardian) - Pre-commit hooks configuration - Environment requirements for different workspaces - Added linting section explaining ESLint warnings vs errors policy - Updated QRCode README with proper table formatting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
sdk/qrcode/README.md (1)
1-1: Apply Prettier formatting to READMEPrettier flagged code style issues in
sdk/qrcode/README.md. To keep our docs consistent and unblock the CI pipeline, please run:npx prettier --write sdk/qrcode/README.md• File affected:
sdk/qrcode/README.mdapp/tests/src/stores/proofHistoryStore.test.ts (2)
128-148: Inconsistent test coverage: Some test cases missing logoUrl property.Several test cases create mock proof objects without the
logoUrlproperty, while others include it. This inconsistency could mask potential issues with logo handling when the property is undefined.Consider adding
logoUrlto all test cases for consistency:const mockProof = { appName: 'TestApp', sessionId: 'session-123', userId: 'user-456', userIdType: 'uuid', endpointType: 'celo', status: ProofStatus.PENDING, disclosures: '{"test": "data"}', + logoUrl: 'https://example.com/logo.png', } as const;Also applies to: 191-230, 282-312
27-28: Address TypeScriptanyusage to improve type safety.The
anytype usage flagged by ESLint reduces type safety. Consider creating proper type interfaces for the mocked database and socket objects.-const mockDatabase = database as any; -const mockIo = io as any; +const mockDatabase = database as jest.Mocked<typeof database>; +const mockIo = io as jest.MockedFunction<typeof io>;
🧹 Nitpick comments (4)
CLAUDE.md (3)
1-4: Consider relocating or cross-linking this guide to established docs sectionsDropping a standalone
CLAUDE.mdin the repo root risks discoverability issues—contributors will likely look forCONTRIBUTING.md,DEVELOPMENT.md, or the main README first. Either:-# CLAUDE.md +# DEVELOPMENT.md <!-- or merge into CONTRIBUTING.md -->or explicitly reference it from the README/CONTRIBUTING to avoid bit-rot.
20-27: Document required Node & Yarn versions to avoid build painThe command list assumes the correct toolchain is already installed, but
yarn@4PnP and Circom tooling are notoriously version-sensitive. Add a short “Prerequisites” bullet right above the command table, e.g.:+### Prerequisites +- Node.js ≥ 18.17 (LTS) +- Yarn 4.6.0 (Plug’n’Play enabled) +- Rust toolchain ≥ 1.70 (for circom_tester)This heads off common “cannot find module” and native-addon build errors on fresh clones.
120-131: Unify formatting commands to reduce cognitive overheadTwo different script names (
fmtvsformat) for essentially the same Prettier pass is easy to forget and causes CI churn. Prefer one canonical script and make the other an alias:-### App-specific formatting: -cd app && yarn fmt:fix -cd app && yarn fmt +### Formatting (all workspaces): +yarn format:fix # runs prettier on root & app via workspaces +yarn format # check modeThen wire
app/package.json:"scripts": { - "fmt": "prettier --check .", - "fmt:fix": "prettier --write ." + "format": "prettier --check .", + "format:fix": "prettier --write ." }A single mental model keeps new contributors from tripping CI.
app/tests/utils/proving/provingMachine.generatePayload.test.ts (1)
10-133: Address ESLint warning for explicit 'any' usage.The pipeline flagged an ESLint warning about "Unexpected any" usage. Consider adding proper TypeScript types to improve type safety in test mocks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CLAUDE.md(1 hunks)app/src/screens/home/ProofHistoryDetailScreen.tsx(1 hunks)app/src/screens/home/ProofHistoryScreen.tsx(1 hunks)app/src/screens/prove/ProveScreen.tsx(2 hunks)app/src/stores/database.ts(2 hunks)app/src/stores/proof-types.ts(1 hunks)app/src/stores/proofHistoryStore.ts(2 hunks)app/tests/src/stores/database.test.ts(3 hunks)app/tests/src/stores/proofHistoryStore.test.ts(3 hunks)app/tests/utils/proving/provingMachine.generatePayload.test.ts(1 hunks)common/src/utils/appType.ts(2 hunks)sdk/core/README.md(3 hunks)sdk/core/src/SelfBackendVerifier.ts(2 hunks)sdk/core/src/index.ts(1 hunks)sdk/qrcode/README.md(2 hunks)sdk/qrcode/utils/websocket.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit Configuration File
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
app/tests/utils/proving/provingMachine.generatePayload.test.tsapp/tests/src/stores/proofHistoryStore.test.tsapp/tests/src/stores/database.test.ts
app/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
app/src/**/*.{ts,tsx,js,jsx}: Review React Native TypeScript code for:
- Component architecture and reusability
- State management patterns
- Performance optimizations
- TypeScript type safety
- React hooks usage and dependencies
- Navigation patterns
Files:
app/src/stores/proofHistoryStore.tsapp/src/stores/proof-types.tsapp/src/screens/home/ProofHistoryDetailScreen.tsxapp/src/stores/database.tsapp/src/screens/home/ProofHistoryScreen.tsxapp/src/screens/prove/ProveScreen.tsx
🪛 GitHub Actions: General Self CI
sdk/qrcode/README.md
[warning] 1-1: Prettier code style warning: Code style issues found. Run Prettier with --write to fix.
app/tests/utils/proving/provingMachine.generatePayload.test.ts
[warning] 10-133: ESLint warnings: 'Unexpected any' (@typescript-eslint/no-explicit-any).
app/src/stores/proofHistoryStore.ts
[warning] 39-200: ESLint warnings: Multiple 'Unexpected console statement' (no-console) and 'Unexpected any' (@typescript-eslint/no-explicit-any).
app/tests/src/stores/proofHistoryStore.test.ts
[warning] 27-31: ESLint warnings: 'Unexpected any' (@typescript-eslint/no-explicit-any).
app/src/stores/database.ts
[warning] 47-57: ESLint warnings: 'Unexpected console statement' (no-console).
sdk/core/README.md
[warning] 1-1: Prettier code style warning: Code style issues found. Run Prettier with --write to fix.
app/src/screens/home/ProofHistoryScreen.tsx
[warning] 297-297: ESLint warning: 'Unexpected console statement' (no-console).
app/src/screens/prove/ProveScreen.tsx
[warning] 97-122: ESLint warnings: 'Unexpected console statement' (no-console).
app/tests/src/stores/database.test.ts
[warning] 14-17: ESLint warnings: 'Unexpected any' (@typescript-eslint/no-explicit-any).
⏰ 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: run_circuit_tests
🔇 Additional comments (28)
app/tests/utils/proving/provingMachine.generatePayload.test.ts (1)
94-94: Property rename correctly implemented in test mock.The change from
logoBase64tologoUrlaligns with the breaking API changes. Test coverage is maintained with the updated property name.sdk/qrcode/utils/websocket.ts (1)
8-8: Interface property rename correctly implemented.The change from
logoBase64tologoUrlin theWebAppInfointerface is consistent with the breaking API changes and improves websocket message efficiency by using URLs instead of base64 data.app/src/stores/proof-types.ts (1)
17-17: Type interface correctly updated for breaking change.The property rename from
logoBase64?tologoUrl?maintains the optional nature and string type while aligning with the API breaking changes. This ensures type safety across the application.sdk/qrcode/README.md (2)
33-33: Documentation example correctly updated.The example code properly demonstrates the new
logoUrlparameter with a URL value instead of base64 data, accurately reflecting the breaking API changes.
82-82: Parameter description accurately updated.The table correctly describes
logoUrlas "URL to PNG logo" instead of the previous "Base64-encoded logo", providing clear guidance for developers.sdk/core/src/index.ts (1)
1-1: Network type export correctly added for TypeScript support.Adding the
Networktype export enables consumers to properly type the network parameter in the updatedSelfBackendVerifierconstructor, supporting the breaking change from booleanmockPassportto typednetworkparameter.app/tests/src/stores/proofHistoryStore.test.ts (1)
66-66: LGTM! Logo property migration correctly implemented in test data.The test data has been properly updated to use
logoUrlwith valid URL values. This aligns with the broader refactoring to use URLs instead of base64 strings for logos.Also applies to: 109-109, 246-246
app/src/stores/proofHistoryStore.ts (1)
172-186: Excellent backward compatibility strategy with clear documentation.The approach of storing
logoUrlvalues in the existinglogoBase64database column is well-documented and maintains backward compatibility effectively. The comments clearly explain the mapping logic.app/src/stores/database.ts (2)
108-108: Well-executed backward compatibility with clear documentation.The database schema maintains the existing
logoBase64column name while storing URL data, with excellent documentation explaining the purpose. This approach avoids breaking changes to the mobile app database structure.Also applies to: 131-131
120-136: Secure parameterized query implementation.The SQL insertion uses proper parameterized queries to prevent SQL injection attacks. The parameter mapping correctly stores the
logoUrlvalue in thelogoBase64column for backward compatibility.app/src/screens/home/ProofHistoryDetailScreen.tsx (2)
115-131: Correct logo rendering logic with proper React hooks usage.The
logoSourcememoization correctly handles the property rename fromlogoBase64tologoUrlwhile maintaining backward compatibility for both HTTP URLs and base64 strings. The dependency array is properly updated to reflect the new property name.
120-130: Robust image source handling supports multiple formats.The conditional logic properly handles HTTP/HTTPS URLs and base64 strings, providing flexibility for different logo formats. This maintains compatibility during the migration period.
app/src/screens/home/ProofHistoryScreen.tsx (3)
188-196: Consistent logo handling implementation across components.The logo source logic correctly migrates from
logoBase64tologoUrlwhile maintaining the same backward compatibility handling for both URL and base64 formats. This ensures consistent behavior across the application.
296-299: Appropriate error handling for component rendering.The try-catch block with console.error provides valuable debugging information when proof items fail to render, which is essential for troubleshooting data parsing issues. The graceful fallback to
nullprevents UI crashes.
176-302: Excellent React Native performance optimizations.The
renderItemfunction is properly memoized withuseCallbackand includes appropriate dependencies. The component handles list rendering efficiently with proper key extraction and end-reached handling.sdk/core/README.md (3)
25-25: LGTM: Clear parameter documentation improvement.The change from
mockPassportboolean tonetworkstring parameter makes the API more self-documenting and extensible.
135-135: Documentation example updated correctly.The example properly demonstrates the new string-based network parameter usage.
212-212: Consistent logo parameter migration.The documentation correctly reflects the change from
logoBase64tologoUrl, aligning with the broader refactoring.app/tests/src/stores/database.test.ts (3)
76-76: Test data correctly migrated to logoUrl.The mock proof object properly reflects the new URL-based logo handling.
98-98: SQL parameter expectations updated consistently.The test correctly expects
logoUrlparameter in the database insertion.
144-144: Good documentation of backward compatibility approach.The comment clearly explains that
logoUrlis stored in the existinglogoBase64column, which maintains database compatibility while supporting the new URL format.app/src/screens/prove/ProveScreen.tsx (2)
75-75: Proof history integration updated correctly.The
addProofHistorycall properly uses the newlogoUrlparameter.
107-127: Logo handling simplified and secured.The new logic correctly validates HTTP/HTTPS URLs and provides helpful warnings for invalid formats. This approach is more secure and performant than base64 handling.
Consider adding additional URL validation if you need to restrict specific domains or schemes for security purposes:
// Check if the logo is a valid URL if ( selectedApp.logoUrl.startsWith('http://') || selectedApp.logoUrl.startsWith('https://') ) { + // Optional: Add domain whitelist validation + try { + const url = new URL(selectedApp.logoUrl); + // Add any domain restrictions here if needed + } catch { + console.warn('Invalid URL format:', selectedApp.logoUrl); + return null; + } return { uri: selectedApp.logoUrl }; }sdk/core/src/SelfBackendVerifier.ts (3)
27-27: Well-defined Network type for type safety.The union type provides clear constraints and better developer experience than the previous boolean approach.
40-40: Constructor parameter improved with sensible default.The
networkparameter with'mainnet'default maintains backward compatibility while providing clearer semantics than the previous booleanmockPassport.
45-50: Network selection logic implemented correctly.The conditional logic properly maps network types to their respective RPC URLs and contract addresses. The testnet/staging configuration is clearly separated from mainnet.
common/src/utils/appType.ts (2)
13-13: Interface correctly migrated to URL-based logos.The change from
logoBase64tologoUrlaligns with the broader refactoring to standardize on URL-based logo handling.
92-106: Constructor refactoring improves maintainability.The explicit field assignment approach is more readable and maintainable than the previous spread operator pattern. The version 2 default and empty string logoUrl default are appropriate choices.
This refactoring also makes it easier to track which fields have defaults and ensures all required fields are properly handled.
| const totalCount = results.total_count || 0; | ||
| for (let i = 0; i < results.rows.length; i++) { | ||
| const row = results.rows[i]; | ||
| const row = results.rows[i] as any; // Database row has logoBase64 column |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for database row mapping.
The any cast reduces type safety. Consider creating a proper interface for the database row structure to maintain type checking while acknowledging the column name mismatch.
+interface DatabaseRow {
+ id: string;
+ sessionId: string;
+ appName: string;
+ endpointType: string;
+ status: ProofStatus;
+ errorCode?: string;
+ errorReason?: string;
+ timestamp: number;
+ disclosures: string;
+ logoBase64?: string; // Stores logoUrl for backward compatibility
+ userId: string;
+ userIdType: string;
+}
- const row = results.rows[i] as any; // Database row has logoBase64 column
+ const row = results.rows[i] as DatabaseRow;📝 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 row = results.rows[i] as any; // Database row has logoBase64 column | |
| // At the top of the file (e.g. after imports), add: | |
| interface DatabaseRow { | |
| id: string; | |
| sessionId: string; | |
| appName: string; | |
| endpointType: string; | |
| status: ProofStatus; | |
| errorCode?: string; | |
| errorReason?: string; | |
| timestamp: number; | |
| disclosures: string; | |
| logoBase64?: string; // Stores logoUrl for backward compatibility | |
| userId: string; | |
| userIdType: string; | |
| } | |
| // …later in the code, replace the `any` cast: | |
| - const row = results.rows[i] as any; // Database row has logoBase64 column | |
| + const row = results.rows[i] as DatabaseRow; |
🤖 Prompt for AI Agents
In app/src/stores/proofHistoryStore.ts at line 172, the database row is cast to
'any', which reduces type safety. Define a TypeScript interface representing the
expected structure of the database row, including the 'logoBase64' column, and
use this interface instead of 'any' for the row variable to improve type
checking and maintain clarity about the data shape.
transphorm
changed the title
Clean SDK Parameters
This PR cleans up the Self SDK API with three key improvements for better developer experience.
This PR contains BREAKING CHANGES that require code updates:
1. SelfBackendVerifier Constructor
2. SelfApp Interface
🔄 Migration Guide
Backend Verifier Migration
Find and replace in your codebase:
App Builder Migration
Update your SelfAppBuilder calls:
Logo Migration Tips
TypeScript Updates
If you're using TypeScript, update your type imports:
🎯 Why These Changes
logoUrl: URLs are standard web practice, better performance (no large base64 in QR codes)network: Clear intent vs confusing boolean, easier to extend with new networks📱 Mobile App Impact
Mobile app users: No action required. Existing app installations continue working.
Technical detail: We maintained database compatibility by keeping the
logoBase64column but storing URL values in it. This avoids forcing app updates while modernizing the SDK interface.✅ Validation
Test your changes:
Summary by CodeRabbit
New Features
Refactor
logoBase64) with logo URLs (logoUrl) across the app, SDK, and documentation.logoUrlinstead oflogoBase64.'mainnet' | 'testnet'values.Documentation
logoUrlproperty and revised usage examples.Tests
logoUrlin place oflogoBase64for logo handling.