Update get verification config visibility #663
Conversation
* remove sdk/tests * chore: update yarn.lock --------- Co-authored-by: Ayman <[email protected]>
* implement self uups upgradeable * small changes in identityVerificationHubImplV2 * delete aderyn.toml * chore: add custom verifier * chnage return output * feat: use self structs and a Generic output struct * feat: add userIdentifier, nullifier, forbiddencountries to returned output * add root view functions from registry * fix: build and compilation errors * add userDefined data into selfVerificationRoot * "resolve conflicts" * fix compilation problem * fix how to register verification config * test: CustomVerifier * fix verification root and hub integration * add scope check in hub impl * replace poseidon hash to ripemd+sha256 * add todo list * feat: refactor and add test cases for generic formatter * add performUserIdentifierCheck in basicVerification * change how to handle additionalData and fix stack too deep * start adding test codes * fix dependency problems in monorepo * fix: forbidden countries (#612) LGTM! * able to run test code * pass happy path * delete unused codes * change error code name, add caller address validation and add scripts to run test and build in monorepo * add all test cases in vcAndDisclose flow * remove comment out * chore: use actual user identifier outputs * success in registration tests * cover all cases * pass contractVersion instead of circuitVersion * fix disclose test * chore: add natspecs for ImplHubV2, CustomVerifier and GenericFormatter * change val name and remove unused lines * add val name change * remove userIdentifier from return data * feat: use GenericDiscloseOutput struct in verfication hook fix test cases for user identifier * chore: change the function order for Hub Impl V2 (#625) * fix nat specs * add nat spec in SelfStructs --------- Co-authored-by: Ayman <[email protected]> Co-authored-by: Nesopie <[email protected]>
* add missed files * add NFCMethodSelectionScreen * bump android build --------- Co-authored-by: Justin Hernandez <[email protected]>
* Simplify developer mode gesture * Enable dev mode on MockData screen with five taps
* fix: vc_and_disclose_id test * chore: yarn prettier
* Add help button and error modal actions * fix the screen management * yarn nice
* bump version and build numbers * remove tamagui/toast * fix marketing version
* update gesture version and bump android build * bump and fix ios build * update lock files * fixes * fix fotoapparat library source
* refactor: update HappyBirthday contract to V2 with support for E-Passport and EUID cards, introduce bonus multipliers, and enhance verification logic * refactor: update Airdrop contract to V2 with support for E-Passport and EU ID Card attestations * refactor: remove BASIS_POINTS constant from Airdrop contract * feat: introduce SelfIdentityERC721 contract for issuing NFTs based on verified identity credentials, replacing SelfPassportERC721 * fix: update verification functions in Airdrop, HappyBirthday, and SelfIdentityERC721 contracts to use customVerificationHook
* merge dev branch into main (#624) * remove sdk/tests (#622) * remove sdk/tests * chore: update yarn.lock --------- Co-authored-by: Ayman <[email protected]> * fix: add range check on paddedInLength of shaBytesDynamic (#623) * fix ci (#626) --------- Co-authored-by: Ayman <[email protected]> Co-authored-by: Vishalkulkarni45 <[email protected]> * update contracts (#628) * remove sdk/tests (#622) * remove sdk/tests * chore: update yarn.lock --------- Co-authored-by: Ayman <[email protected]> * fix: add range check on paddedInLength of shaBytesDynamic (#623) * fix ci (#626) * implement self uups upgradeable (#592) * implement self uups upgradeable * small changes in identityVerificationHubImplV2 * delete aderyn.toml * chore: add custom verifier * chnage return output * feat: use self structs and a Generic output struct * feat: add userIdentifier, nullifier, forbiddencountries to returned output * add root view functions from registry * fix: build and compilation errors * add userDefined data into selfVerificationRoot * "resolve conflicts" * fix compilation problem * fix how to register verification config * test: CustomVerifier * fix verification root and hub integration * add scope check in hub impl * replace poseidon hash to ripemd+sha256 * add todo list * feat: refactor and add test cases for generic formatter * add performUserIdentifierCheck in basicVerification * change how to handle additionalData and fix stack too deep * start adding test codes * fix dependency problems in monorepo * fix: forbidden countries (#612) LGTM! * able to run test code * pass happy path * delete unused codes * change error code name, add caller address validation and add scripts to run test and build in monorepo * add all test cases in vcAndDisclose flow * remove comment out * chore: use actual user identifier outputs * success in registration tests * cover all cases * pass contractVersion instead of circuitVersion * fix disclose test * chore: add natspecs for ImplHubV2, CustomVerifier and GenericFormatter * change val name and remove unused lines * add val name change * remove userIdentifier from return data * feat: use GenericDiscloseOutput struct in verfication hook fix test cases for user identifier * chore: change the function order for Hub Impl V2 (#625) * fix nat specs * add nat spec in SelfStructs --------- Co-authored-by: Ayman <[email protected]> Co-authored-by: Nesopie <[email protected]> * prettier (#629) --------- Co-authored-by: Ayman <[email protected]> Co-authored-by: Vishalkulkarni45 <[email protected]> Co-authored-by: nicoshark <[email protected]> Co-authored-by: Nesopie <[email protected]> * fix: vc_and_disclose_id test (#640) * fix: vc_and_disclose_id test * chore: yarn prettier * fix: check if a config id exists * chore: change the function where the config not set verification is happening * fix: add await * feat: add getConfigId function in SelfVerificationRoot (#650) * feat: add getConfigId function in SelfVerificationRoot * update comment --------- Co-authored-by: motemotech <[email protected]> * chore: fix ofac end index in eu id cards * chore: fix tests * fix: example contracts and tests --------- Co-authored-by: turnoffthiscomputer <[email protected]> Co-authored-by: Vishalkulkarni45 <[email protected]> Co-authored-by: nicoshark <[email protected]>
…ed documentation and library linkage for CustomVerifier. Update initialization process to reflect changes in V2 implementation, ensuring proper setup for proxy deployment. (#658)
* fix build issues * generate disclosure proof with euids * generate disclosure proof with euids * Eu id updates 2 (#648) * update vc_and_disclose_id test (dev branch) (#641) * fix: vc_and_disclose_id test * chore: yarn prettier * Show modal on NFC scan error (#642) * Add help button and error modal actions * fix the screen management * yarn nice * Bump build v2.5.4: ios 132; android 71 (#631) * bump version and build numbers * remove tamagui/toast * fix marketing version * fix: update TD1 and TD3 checks (#643) * bum yarn.lock * add version and user defined data --------- Co-authored-by: Vishalkulkarni45 <[email protected]> Co-authored-by: Justin Hernandez <[email protected]> Co-authored-by: Seshanth.S🐺 <[email protected]> * remove the mock user define data * get the useridentifier as a hash from the user defined data * chore: add version and userDefinedData * feat: use the version in register / dsc proofs as well * update calculateUserIdentifierHash * yarn nice * refactor: consolidate user context data handling and update payload structure * fix typing issues on sha1 * remove console.log(sha1) * fix sha1 import * refactor: streamline userDefinedData handling and adjust payload type for circuit * refactor: update sha1 usage and enhance logging in calculateUserIdentifierHash * yarn nice * yarn lint common * use ts-ignore for sha1 import * fix app ci tests * fix typing issue * remove unused ts-ignore * cast uuid before calling generateinputs * bump qrcode version * add tsup on the qrcode sdk * fix: exports on selfxyz/qrcode * update how we define config.version * fix yarn imports * yarn format --------- Co-authored-by: Vishalkulkarni45 <[email protected]> Co-authored-by: Justin Hernandez <[email protected]> Co-authored-by: Seshanth.S🐺 <[email protected]> Co-authored-by: Ayman <[email protected]>
* Fix previous rebase error * Refactor deployment module for Identity Verification Hub V2.
* fix: sdk build configs * chore: SelfBackendVerifier (WIP) * feat: add custom verification * feat: consider destination chain in user defined data * chore: export attestation id * chore: export attestation id * chore: export config storage * chore: don't throw an error if the proof is not valid * chore: trim abi and rm typechain types * refactor * chore: rm unnecessary exports * 📝 Add docstrings to `fix/sdk` (#653) Docstrings generation was requested by @remicolin. * #652 (comment) The following files were modified: * `sdk/core/src/utils/hash.ts` * `sdk/core/src/utils/proof.ts` * `sdk/core/src/utils/utils.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * review fixes * chore: fix package.json cjs types * chore: add minor changes to checks * feat: add InMemoryConfigStore, allIds constant and verificationResult type * chore: export Verification config * feat: change the verification config types * fix: throw issues early if verification config is null * fix: update yarn.lock file * chore: lint * fix: rm ts expect error directive * fix: contract tests * use excluded countries instead forbidden countries list * chore: change types in constnats --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughThis update introduces substantial enhancements across the codebase, including multi-document support (passport and ID card) in both smart contracts and client logic, major refactoring and type safety improvements in the SDK, and more robust configuration and error handling. It also upgrades build systems, workflows, and package metadata to support clearer module boundaries, dual ESM/CJS output, and stricter CI policies. Several Solidity contracts are upgraded or replaced, and new utility and configuration abstractions are introduced throughout the stack. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant SDK
participant SmartContract
User->>App: Select document type (passport/ID)
App->>SDK: Generate proof inputs (category-aware)
SDK->>SDK: Select selectors/SMTs based on document type
SDK->>SmartContract: Submit proof and user context
SmartContract->>SmartContract: Validate config ID, attestation ID, and proof
SmartContract-->>SDK: Return verification result
SDK-->>App: Display result, show user-defined data if present
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
…file paths and module name for deployment commands.
…bV2.ts. Include manual verification steps and common failure reasons to assist users during deployment.
…o public in IdentityVerificationHubImplV2 contract to allow external access.
kevinsslin
force-pushed
the
Update-get-verification-config-visibility
branch
from
e37c486 to
94e241c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
sdk/qrcode/components/SelfQRcode.tsx (1)
157-157: Inconsistent export of type-only imported entity.
SelfAppis imported as a type-only import on line 24 but exported as a value here. This creates an inconsistency.Apply this diff to fix the export consistency:
-export { SelfQRcode, SelfApp, SelfAppBuilder }; +export { SelfQRcode, SelfAppBuilder }; +export type { SelfApp };
🧹 Nitpick comments (17)
app/jest.setup.js (1)
50-53: Remove the unnecessary empty constructor.The empty constructor serves no purpose and should be removed for cleaner code.
- // Mock flush policy classes - const MockFlushPolicy = class { - constructor() {} - }; + // Mock flush policy classes + const MockFlushPolicy = class {};contracts/package.json (1)
26-26: Duplicate deployment script may cause confusion.The new
deploy:verifier:idscript runs the same command asdeploy:verifiers:all. Consider clarifying the purpose or removing the duplicate.If both scripts serve different purposes, consider adding a comment to clarify their distinction. Otherwise, consider removing the duplicate to avoid confusion.
app/android/build.gradle (1)
39-39: Consider jcenter() deprecation in future updates.While the repository reordering is fine, note that jcenter() has been deprecated by JFrog. Consider migrating dependencies to mavenCentral() or other repositories in future updates.
app/src/utils/utils.ts (1)
6-7: Consider breaking down the complex TD1 regex for better maintainability.The TD1_REGEX pattern is extremely long and complex, making it difficult to read and maintain. Consider breaking it into smaller, named regex components or using a more modular approach.
Example refactor:
-const TD1_REGEX = - /^(?<documentType>[A-Z0-9<]{2})(?<issuingCountry>[A-Z<]{3})(?<documentNumber>[A-Z0-9<]{9})(?<checkDigitDocumentNumber>[0-9]{1})(?<optionalData1>[A-Z0-9<]{15})(?<dateOfBirth>[0-9]{6})(?<checkDigitDateOfBirth>[0-9]{1})(?<sex>[MF<]{1})(?<dateOfExpiry>[0-9]{6})(?<checkDigitDateOfExpiry>[0-9]{1})(?<nationality>[A-Z<]{3})(?<optionalData2>[A-Z0-9<]{7})/; +const TD1_DOCUMENT_TYPE = '[A-Z0-9<]{2}'; +const TD1_COUNTRY = '[A-Z<]{3}'; +const TD1_DOC_NUMBER = '[A-Z0-9<]{9}'; +// ... other components +const TD1_REGEX = new RegExp(`^(?<documentType>${TD1_DOCUMENT_TYPE})(?<issuingCountry>${TD1_COUNTRY})...`);app/src/screens/prove/ProveScreen.tsx (1)
237-260: Well-implemented userDefinedData display section.The conditional UI section is cleanly implemented with appropriate styling and positioning. The use of optional chaining and proper React patterns is good.
Consider adding content validation/sanitization for the userDefinedData since it's user-provided content that could potentially contain unsafe characters or scripts.
<View backgroundColor={slate300} padding={15} borderRadius={8} marginBottom={10} > - <BodyText fontSize={14} color={black} lineHeight={20}> - {selectedApp.userDefinedData} - </BodyText> + <BodyText fontSize={14} color={black} lineHeight={20}> + {sanitizeUserInput(selectedApp.userDefinedData)} + </BodyText> </View>common/package.json (1)
12-12: Consider the implications of pointing types to CJS build.All type declarations now point to the CommonJS build (
./dist/cjs/...) instead of ESM. This could potentially cause TypeScript resolution issues for ESM consumers who expect types to align with their module format.Consider whether this change is intentional for compatibility reasons, or if separate type declarations should be maintained for both CJS and ESM builds.
Also applies to: 17-17, 22-22, 35-35
sdk/core/src/store/interface.ts (1)
6-6: Fix typos in documentation comments.There are typos in the parameter descriptions: "identifiier" should be "identifier".
- * @param id - An identifiier associated with the verification config + * @param id - An identifier associated with the verification config- * @param id - An identifiier associated with the verification config + * @param id - An identifier associated with the verification configAlso applies to: 12-12
common/src/utils/hash.ts (1)
160-160: Use lowercase 'bigint' type for consistency.TypeScript convention prefers lowercase primitive types over constructor types.
-): BigInt { +): bigint {sdk/core/src/utils/proof.ts (1)
14-21: Document the magic numbers for better maintainability.The hardcoded values
93,94, and31lack documentation about their meaning and origin. Consider adding comments or constants to explain these values.switch (attestationId) { case 1: - return 93 / 31; + return 93 / 31; // 93 bytes total data, 31 bytes per signal case 2: - return Math.ceil(94 / 31); + return Math.ceil(94 / 31); // 94 bytes total data, 31 bytes per signal default: throw new Error(`Invalid attestation ID: ${attestationId}`); }sdk/core/src/utils/id.ts (2)
75-75: Remove debugging console.log statement.This debugging statement should be removed from production code or replaced with proper logging.
- console.log('ofac', ofac);
5-90: Consider refactoring this large function for better maintainability.This function is quite large and handles multiple responsibilities. Consider breaking it down into smaller, focused functions for better readability and testability.
For example, you could extract:
- Data extraction logic into separate functions
- String transformation logic
- Field-specific processing
This would make the code easier to maintain and test individual components.
sdk/core/src/utils/utils.ts (1)
3-5: Consider more descriptive function nameThe function name
trimu0000is not immediately clear. Consider renaming to something more descriptive likeremoveNullCharactersorfilterNullChars.-function trimu0000(unpackedReveal: string[]): string[] { +function removeNullCharacters(unpackedReveal: string[]): string[] { return unpackedReveal.filter((value) => value !== '\u0000'); }app/src/utils/proving/provingMachine.ts (1)
589-589: Consider using a more structured logging approach for production.The console.log statement may not be appropriate for production environments where structured logging is preferred.
Consider replacing with a structured logger or adding a log level check:
- console.log('Passport is not registered with local CSCA'); + // Consider using a structured logger instead + console.log('Passport is not registered with local CSCA');app/src/utils/proving/provingUtils.ts (2)
77-77: Simplify the type assignment logic.The conditional logic for assigning the
typefield could be clearer by explicitly handling both cases.Consider making the logic more explicit:
- type: circuitName === 'vc_and_disclose' ? 'disclose' : 'disclose_id', + type: circuitName === 'vc_and_disclose' ? 'disclose' : 'disclose_id',Actually, the current implementation is fine for readability.
80-80: Simplify the boolean expression as suggested by static analysis.The ternary operator for a boolean assignment can be simplified.
Apply the static analysis suggestion:
- onchain: endpointType === 'celo' ? true : false, + onchain: endpointType === 'celo',contracts/contracts/example/SelfIdentityERC721.sol (1)
135-135: Minor gas optimization: Use pre-increment.Consider using pre-increment for slightly better gas efficiency.
-uint256 tokenId = _tokenIdCounter++; +uint256 tokenId = ++_tokenIdCounter - 1;Alternatively, you could separate the operations:
-uint256 tokenId = _tokenIdCounter++; +uint256 tokenId = _tokenIdCounter; +_tokenIdCounter++;sdk/core/src/SelfBackendVerifier.ts (1)
253-266: Consider logging verification errors.Silently catching and ignoring errors during proof verification could hide important issues. Consider at least logging these errors for debugging purposes.
let isValid = false; try { isValid = await verifierContract.verifyProof( proof.a, [ [proof.b[0][1], proof.b[0][0]], [proof.b[1][1], proof.b[1][0]], ], proof.c, publicSignals ); } catch (error) { + console.error('Proof verification failed:', error); isValid = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/ios/Podfile.lockis excluded by!**/*.lockcommon/bun.lockbis excluded by!**/bun.lockbyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (82)
.github/workflows/block-non-dev-to-main.yml(1 hunks).github/workflows/npm-publish.yml(4 hunks)app/android/android-passport-reader/app/build.gradle(1 hunks)app/android/app/build.gradle(2 hunks)app/android/build.gradle(1 hunks)app/android/gradle.properties(2 hunks)app/android/react-native-passport-reader/android/build.gradle(1 hunks)app/index.js(1 hunks)app/ios/OpenPassport/Info.plist(1 hunks)app/ios/Self.xcodeproj/project.pbxproj(4 hunks)app/jest.config.cjs(1 hunks)app/jest.setup.js(1 hunks)app/package.json(4 hunks)app/src/layouts/SimpleScrolledTitleLayout.tsx(2 hunks)app/src/screens/dev/MockDataScreen.tsx(2 hunks)app/src/screens/misc/LaunchScreen.tsx(2 hunks)app/src/screens/passport/NFCMethodSelectionScreen.tsx(5 hunks)app/src/screens/passport/PassportNFCScanScreen.tsx(6 hunks)app/src/screens/passport/PassportNFCTroubleScreen.tsx(4 hunks)app/src/screens/prove/ProveScreen.tsx(1 hunks)app/src/screens/settings/SettingsScreen.tsx(2 hunks)app/src/types/react-native-passport-reader.d.ts(1 hunks)app/src/utils/ethers.ts(0 hunks)app/src/utils/nfcScanner.ts(1 hunks)app/src/utils/proving/provingInputs.ts(5 hunks)app/src/utils/proving/provingMachine.ts(8 hunks)app/src/utils/proving/provingUtils.ts(1 hunks)app/src/utils/proving/validateDocument.ts(2 hunks)app/src/utils/utils.ts(1 hunks)circuits/tests/disclose/vc_and_disclose.test.ts(2 hunks)circuits/tests/disclose/vc_and_disclose_id.test.ts(2 hunks)common/.npmignore(1 hunks)common/index.ts(2 hunks)common/package.json(5 hunks)common/src/utils/appType.ts(3 hunks)common/src/utils/circuits/generateInputs.ts(2 hunks)common/src/utils/circuits/uuid.ts(0 hunks)common/src/utils/hash.ts(2 hunks)contracts/contracts/IdentityVerificationHubImplV2.sol(3 hunks)contracts/contracts/example/Airdrop.sol(1 hunks)contracts/contracts/example/HappyBirthday.sol(1 hunks)contracts/contracts/example/SelfIdentityERC721.sol(1 hunks)contracts/contracts/example/SelfPassportERC721.sol(0 hunks)contracts/contracts/libraries/CircuitAttributeHandlerV2.sol(1 hunks)contracts/contracts/tests/TestSelfVerificationRoot.sol(1 hunks)contracts/contracts/tests/testGenericFormatter.sol(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/ignition/modules/deployTestSelfVerificationRoot.ts(1 hunks)contracts/ignition/modules/deployV2.ts(0 hunks)contracts/ignition/modules/hub/deployHubV2.ts(1 hunks)contracts/package.json(3 hunks)contracts/scripts/setVerifiersV2.ts(4 hunks)contracts/test/v2/discloseId.test.ts(8 hunks)contracts/test/v2/disclosePassport.test.ts(8 hunks)sdk/core/.gitignore(1 hunks)sdk/core/index.ts(1 hunks)sdk/core/package.json(3 hunks)sdk/core/scripts/copyAbi.sh(1 hunks)sdk/core/src/SelfBackendVerifier.ts(1 hunks)sdk/core/src/abi/IdentityRegistryImplV1.ts(0 hunks)sdk/core/src/abi/IdentityVerificationHubImpl.json(1 hunks)sdk/core/src/abi/Registry.json(1 hunks)sdk/core/src/abi/Verifier.json(1 hunks)sdk/core/src/abi/VerifyAll.ts(0 hunks)sdk/core/src/constants/contractAddresses.ts(0 hunks)sdk/core/src/errors.ts(1 hunks)sdk/core/src/index.ts(1 hunks)sdk/core/src/store/DefaultConfigStore.ts(1 hunks)sdk/core/src/store/InMemoryConfigStore.ts(1 hunks)sdk/core/src/store/interface.ts(1 hunks)sdk/core/src/types/types.ts(1 hunks)sdk/core/src/utils/constants.ts(1 hunks)sdk/core/src/utils/hash.ts(1 hunks)sdk/core/src/utils/id.ts(1 hunks)sdk/core/src/utils/proof.ts(1 hunks)sdk/core/src/utils/utils.ts(1 hunks)sdk/core/tsconfig.json(2 hunks)sdk/qrcode/components/SelfQRcode.tsx(2 hunks)sdk/qrcode/index.ts(1 hunks)sdk/qrcode/package.json(4 hunks)sdk/qrcode/tsconfig.json(2 hunks)sdk/qrcode/tsup.config.ts(1 hunks)
💤 Files with no reviewable changes (7)
- sdk/core/src/abi/VerifyAll.ts
- app/src/utils/ethers.ts
- contracts/contracts/example/SelfPassportERC721.sol
- contracts/ignition/modules/deployV2.ts
- sdk/core/src/constants/contractAddresses.ts
- common/src/utils/circuits/uuid.ts
- sdk/core/src/abi/IdentityRegistryImplV1.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/jest.setup.js
[error] 52-52: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
common/src/utils/hash.ts
[error] 160-160: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
app/src/utils/proving/provingUtils.ts
[error] 80-80: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
sdk/core/src/SelfBackendVerifier.ts
[error] 141-141: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🪛 actionlint (1.7.7)
.github/workflows/block-non-dev-to-main.yml
12-12: "github.head_ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run_circuit_tests
🔇 Additional comments (127)
sdk/core/scripts/copyAbi.sh (2)
38-38: Inconsistent summary: echo added, not just newline
The AI summary mentions only a newline was added, but the diff clearly introduces anecho "ABI files copied and written successfully."statement.Likely an incorrect or invalid review comment.
38-38: Approved: Helpful completion log
The new echo provides clear end-of-script feedback, which is a harmless and useful enhancement.app/ios/OpenPassport/Info.plist (1)
24-24: Version update looks good.Standard version increment from 2.5.3 to 2.5.4 aligns with the coordinated release updates mentioned in the AI summary.
sdk/core/.gitignore (2)
2-2: Good addition of generated files to gitignore.Excluding
src/typechain-types/is appropriate since these are generated TypeChain type files that shouldn't be committed to version control.
5-6: Proper file formatting with trailing newline.Good practice to include a newline at the end of the file.
common/.npmignore (1)
1-3: Excellent addition to exclude build artifacts from npm packages.Properly excludes TypeScript build info files from the published package, keeping the package clean and reducing size.
contracts/contracts/libraries/CircuitAttributeHandlerV2.sol (1)
91-91: Let’s pinpoint the actualofacStart/ofacEndentries forEU_ID_CARDin the handler:#!/bin/bash # Locate EU_ID_CARD entry and its OFAC byte ranges in CircuitAttributeHandlerV2.sol rg -n 'EU_ID_CARD' -C5 contracts/contracts/libraries/CircuitAttributeHandlerV2.sol rg -n 'ofac(Start|End)' -C3 contracts/contracts/libraries/CircuitAttributeHandlerV2.solcontracts/contracts/tests/testGenericFormatter.sol (1)
1-3: Good addition of standard Solidity file headers.The SPDX license identifier and pragma statement are proper additions that align with Solidity best practices for licensing clarity and compiler version specification.
app/jest.setup.js (1)
62-65: LGTM! Mock enhancement looks good.The addition of the lowercase
beforekey and flush policy classes properly extends the Segment Analytics mock for better test compatibility.app/android/gradle.properties (2)
13-13: LGTM! Good build performance optimizations.The increased heap size to 4GB and additional JVM flags will improve build performance for large Android projects. The parallel GC and UTF-8 encoding are good additions.
19-19: Excellent build optimizations for development workflow.Enabling parallel execution, build caching, and configuration on demand will significantly improve build times. The offline mode option and SDK warning suppression enhance the development experience.
Also applies to: 48-58
app/index.js (1)
24-24: ```shell
#!/bin/bashDescription: Retry searching for ToastProvider usage and @tamagui/toast imports without relying on ripgrep’s file-type definitions
echo "Searching for ToastProvider usage in the codebase..."
rg -A5 "ToastProvider" -g '*.{js,jsx,ts,tsx}' .echo "Searching for @tamagui/toast imports..."
rg -A5 "@tamagui/toast" -g '*.{js,jsx,ts,tsx}' .</details> <details> <summary>app/android/android-passport-reader/app/build.gradle (1)</summary> `77-77`: **LGTM! Correct Maven coordinates fix.** The dependency coordinates were properly corrected from the invalid `RedApparat:Fotoapparat` to the correct `fotoapparat:fotoapparat` following Maven naming conventions. </details> <details> <summary>app/jest.config.cjs (1)</summary> `5-5`: **LGTM! Proper Jest configuration for gesture handler.** Adding `react-native-gesture-handler` to the transform ignore patterns ensures Jest properly processes the package during tests, especially important with the version update. </details> <details> <summary>sdk/qrcode/index.ts (1)</summary> `5-5`: **LGTM! Excellent type-only import/export optimization.** Converting `SelfApp` to type-only import and export is a great optimization that will improve tree-shaking and reduce bundle size, since `SelfApp` is only used for typing purposes. Also applies to: 11-11 </details> <details> <summary>app/android/react-native-passport-reader/android/build.gradle (1)</summary> `37-38`: **Verify the compatibility and security of the updated jmrtd dependency.** The version bump from 0.7.42 to 0.8.1 appears to address environment-dependent errors. Please ensure the new version is compatible with your Android build environment and doesn't introduce any breaking changes. ```shell #!/bin/bash # Description: Check for security advisories and compatibility information for jmrtd library # Check Maven Central for available versions and release notes curl -s "https://search.maven.org/solrsearch/select?q=g:org.jmrtd+AND+a:jmrtd&core=gav&rows=10&wt=json" | jq '.response.docs[] | select(.v == "0.8.1" or .v == "0.7.42") | {version: .v, timestamp: .timestamp}'sdk/core/src/index.ts (1)
1-1: LGTM! Proper ESM module resolution.Adding the explicit
.jsfile extension is correct for ESM module resolution and aligns with modern JavaScript/TypeScript best practices.circuits/tests/disclose/vc_and_disclose_id.test.ts (1)
25-25: LGTM! Consistent with user identifier handling refactoring.The addition of
castFromUUIDimport and its application to theuser_identifieraligns well with the broader system updates for standardized UUID processing in circuit inputs.Also applies to: 42-42
circuits/tests/disclose/vc_and_disclose.test.ts (1)
23-23: LGTM! Maintains consistency across test suite.The
castFromUUIDimport and usage pattern matches the ID card test file, ensuring consistent user identifier processing across all circuit tests.Also applies to: 44-44
app/src/screens/misc/LaunchScreen.tsx (1)
29-33: LGTM! Gesture standardization improves usability.The change from a two-finger tap to a five-tap gesture simplifies developer mode activation while maintaining adequate protection against accidental triggers. The variable rename to
devModeTapis more descriptive and aligns with the updated gesture behavior.app/src/screens/dev/MockDataScreen.tsx (1)
263-269: LGTM! Consistent gesture standardization across screens.The change from a two-finger triple tap to a five-tap gesture maintains the same functionality while improving consistency with other screens in the app. This standardization effort enhances developer experience.
app/android/app/build.gradle (2)
89-90: LGTM! Version bump for release.The version code and name updates are appropriate for a new release.
163-163: LGTM! Corrected Fotoapparat dependency coordinates.The fix from "com.github.RedApparat" to "com.github.fotoapparat" corrects the Maven group ID to the proper coordinates for the Fotoapparat library.
contracts/scripts/setVerifiersV2.ts (3)
5-5: LGTM! Refactored to use common package.The import change to use "@selfxyz/common" centralizes shared constants and improves maintainability.
29-29: LGTM! Standardized environment variable naming.The change from
CELO_KEYtoPRIVATE_KEYuses more generic and standard naming conventions for private key environment variables.Also applies to: 70-70
95-95: LGTM! Added helpful debug logging.The registry address debug log will aid in troubleshooting deployment and registry resolution issues.
common/src/utils/circuits/generateInputs.ts (2)
38-38: LGTM! Removed unused import.The
castFromUUIDimport is no longer needed since the function call was removed from the code.
264-264: ```shell
#!/bin/bash
echo "=== Declaration and computation of user_identifier in generateInputs.ts ==="
rg -n "user_identifier" common/src/utils/circuits/generateInputs.tsecho -e "\n=== Checking castFromUUID usage in generateInputs.ts ==="
rg -n "castFromUUID" common/src/utils/circuits/generateInputs.tsecho -e "\n=== Top of file (imports & helpers) ==="
sed -n '1,60p' common/src/utils/circuits/generateInputs.ts</details> <details> <summary>sdk/qrcode/components/SelfQRcode.tsx (2)</summary> `24-24`: **Import is correct for type-only usage.** The type-only import of `SelfApp` is appropriate since it's only used in the TypeScript interface definition. --- `4-5`: **Modern ES module JSON import syntax.** The `with { type: 'json' }` syntax is the correct modern approach for importing JSON files in ES modules. </details> <details> <summary>app/src/screens/settings/SettingsScreen.tsx (1)</summary> `139-143`: **Simplified developer mode activation gesture.** The change from two-finger tap to single-finger five-tap gesture simplifies the developer mode activation and aligns with similar changes across other screens. </details> <details> <summary>sdk/core/tsconfig.json (2)</summary> `6-7`: **Updated output directories for ESM build.** The change to `./dist/esm` output directories aligns with the modular build improvements and ESM support mentioned in the PR objectives. --- `18-22`: **Added project reference for better build integration.** The project reference to the common package enables better TypeScript project coordination and build optimization. </details> <details> <summary>contracts/package.json (2)</summary> `3-3`: **Major version bump reflects significant changes.** The version bump from 0.0.8 to 1.0.1 appropriately reflects the substantial contract updates mentioned in the PR summary. --- `11-15`: **Restricted published files to specific contracts.** The selective file inclusion improves package clarity by only publishing the essential contract interfaces and implementations. </details> <details> <summary>app/src/utils/proving/validateDocument.ts (2)</summary> `10-10`: **Added import for ID card attestation support.** The import of `ID_CARD_ATTESTATION_ID` enables multi-document verification support. --- `82-86`: **Dynamic attestation ID selection for multi-document support.** The conditional logic correctly selects the appropriate attestation ID based on document category, supporting both passport and ID card verification flows. </details> <details> <summary>contracts/contracts/tests/TestSelfVerificationRoot.sol (2)</summary> `86-88`: **New test utility function looks appropriate.** The `setVerificationConfigNoHub` function provides a clean way to set verification config directly for testing invalid config scenarios without hub interaction. The naming is clear and descriptive. --- `90-96`: **Function reformatting improves readability.** The multiline formatting of the `getConfigId` function parameters enhances readability while preserving the original logic. </details> <details> <summary>app/android/build.gradle (1)</summary> `51-51`: **Dependency substitution correction looks good.** The fix from `com.github.RedApparat:Fotoapparat:2.7.0` to `com.github.fotoapparat:fotoapparat:2.7.0` corrects the group and artifact IDs to match the proper Maven coordinates. </details> <details> <summary>app/ios/Self.xcodeproj/project.pbxproj (2)</summary> `426-426`: **Build version increment is consistent.** The CURRENT_PROJECT_VERSION update from 129 to 133 is applied consistently across both Debug and Release configurations. Also applies to: 564-564 --- `541-541`: **Marketing version update is properly coordinated.** The MARKETING_VERSION bump to 2.5.4 aligns with the coordinated version updates across platforms mentioned in the summary. Also applies to: 678-678 </details> <details> <summary>sdk/core/src/utils/hash.ts (1)</summary> `1-15`: **Well-implemented cryptographic utility function.** The `calculateUserIdentifierHash` function correctly implements the SHA-256 → RIPEMD-160 double hashing pattern, similar to Bitcoin address generation. The implementation is deterministic, well-documented, and properly formats the output as a 40-character hex string with "0x" prefix. </details> <details> <summary>app/src/types/react-native-passport-reader.d.ts (2)</summary> `6-7`: **LGTM! CAN support properties added correctly.** The addition of `canNumber` and `useCan` properties to the `ScanOptions` interface follows proper TypeScript patterns and enables Card Access Number functionality for passport scanning. --- `13-13`: **LGTM! Reset method added for state management.** The `reset(): void` method addition provides necessary state management capability for the passport reader, which aligns with the usage mentioned in the AI summary. </details> <details> <summary>sdk/qrcode/tsconfig.json (3)</summary> `6-7`: **LGTM! Output directory structure supports dual module format.** The change to `./dist/esm` output directory aligns with the dual ESM/CJS build strategy mentioned in the AI summary. --- `27-31`: **LGTM! Project reference enables proper cross-package type checking.** The addition of the project reference to `../../common` enables TypeScript project coordination and proper type checking across the monorepo packages. --- `3-4`: ```shell #!/bin/bash # Description: Locate package.json files and extract any "engines" field, then search for "engines" occurrences echo "== Extracting engines fields from package.json ==" find . -name package.json -exec jq -r '.engines // "No engines field"' {} \; echo -e "\n== Searching for 'engines' key in repository ==" grep -R '"engines"' -n .sdk/core/src/abi/Registry.json (1)
1-30: LGTM! Well-formed ABI artifact for identity registry interface.The ABI structure follows standard Hardhat artifact format correctly. The
checkIdentityCommitmentRootfunction signature is appropriate for validating merkle roots in the identity verification system.common/index.ts (3)
25-25: LGTM! SMT utility export added correctly.The
buildSMTimport and export addition follows the existing pattern and provides Sparse Merkle Tree construction capability to consumers of the common package.
31-31: LGTM! Hash utility exports added consistently.The addition of
calculateUserIdentifierHashandgetSolidityPackedUserContextDataimports follows the existing import pattern and provides necessary cryptographic utilities.
66-68: LGTM! Export statements maintain consistency.The new utility functions are properly added to the main export block, maintaining consistency with the existing export structure.
sdk/core/src/abi/IdentityVerificationHubImpl.json (1)
1-45: LGTM! Well-structured ABI for identity verification hub V2.The ABI artifact is correctly formatted and defines appropriate view functions for the V2 identity verification hub. The
discloseVerifierandregistryfunctions provide clean separation of concerns for attestation ID lookups.sdk/core/src/abi/Verifier.json (1)
1-37: LGTM! Well-structured ZK proof verifier ABI.The ABI correctly defines a standard Groth16 proof verification interface with appropriate parameter types and view function modifier. The fixed array sizes and parameter structure align with typical zero-knowledge proof verification contracts.
app/src/utils/utils.ts (1)
10-11: Improved MRZ format detection logic.The enhanced logic using regex validation instead of simple prefix checks should provide more accurate document type detection. The approach correctly tests the MRZ content structure rather than just basic format characteristics.
app/src/utils/nfcScanner.ts (2)
37-37: Good addition of state reset for clean scanning.Adding
PassportReader.reset()at the start ensures the scanner begins in a clean state for each operation, which is a good defensive programming practice.
42-44: Excellent feature parity improvement with iOS.The addition of CAN (Card Access Number) parameter support brings Android scanning functionality in line with the iOS implementation. The use of nullish coalescing operators provides safe defaults for optional parameters.
contracts/contracts/IdentityVerificationHubImplV2.sol (3)
164-166: Good addition of specific error for configuration validation.The new
ConfigNotSet()error provides clear feedback when verification configurations are missing or invalid, improving error handling and debugging.
633-633: Appropriate visibility change for external config access.Changing
getVerificationConfigV2frominternaltopublicallows external contracts and tools to verify configuration existence and content, which is useful for transparency and debugging.
645-648: Excellent config validation to prevent ID mismatch.The validation check that ensures the stored config's generated ID matches the requested
configIdis a critical security improvement. This prevents using misconfigured or corrupted verification configs and provides clear error feedback.common/package.json (3)
3-3: Verify the version bump justification.The version jump from 0.0.2 to 0.0.5 is significant for a patch-level change. Ensure this aligns with the scope of changes and semantic versioning practices.
42-42: Good addition of memory-optimized test script.The new
test-basescript with--max-old-space-size=8192is well-configured for handling large datasets, which aligns with the cryptographic operations this package likely performs.
63-63: Dependencies align with enhanced cryptographic functionality.The addition of
ethersand@types/js-sha1dependencies supports the new utility functions mentioned in the summary for hashing and blockchain operations.Also applies to: 80-80
sdk/qrcode/tsup.config.ts (1)
1-15: Well-configured tsup setup for modern package distribution.The configuration properly supports:
- Dual CJS/ESM output for maximum compatibility
- TypeScript declaration generation
- Multiple entry points for modular consumption
- Development-friendly watch mode
- Modern ES2020 target
This aligns well with the package.json changes mentioned in the summary.
.github/workflows/npm-publish.yml (4)
6-6: Verify the branch change from main to dev is intentional.Publishing from the
devbranch instead ofmainchanges the release workflow. Ensure this aligns with your intended deployment strategy.
66-66: Good simplification of publish commands.Switching from
yarn npm publishtonpm publishdirectly is cleaner and more standard.Also applies to: 94-94, 121-121
69-69: Enhanced authentication with dual token support.Adding
NPM_AUTH_TOKENalongsideNODE_AUTH_TOKENprovides better compatibility and redundancy for npm authentication.Also applies to: 97-97, 124-124
89-89: Critical fix for workspace name.The correction from
@sdk/qrcodeto@selfxyz/qrcodeis essential for proper package resolution and builds.sdk/core/src/store/DefaultConfigStore.ts (2)
16-18: Consider the implications of returning empty actionId.The
getActionIdmethod always returns an empty string, which might cause issues if the consuming code expects a meaningful action identifier. Consider whether this should throw an error, return a default value, or be documented as unsupported.Verify how
getActionIdis used in the verification flow to ensure empty strings won't cause failures.
4-19: Simple and appropriate default implementation.This class serves as a good default implementation for single-configuration scenarios where ID-based lookup isn't needed. The implementation is clean and follows the interface contract correctly.
common/src/utils/appType.ts (2)
23-26: Well-designed interface extensions.The new properties enhance the SelfApp configuration:
version: Enables API evolution and compatibility trackingchainID: Properly restricted to Celo network values (42220 mainnet, 44787 testnet)userDefinedData: Provides flexibility for custom application dataThe type constraints are appropriate and align with the Celo blockchain integration.
99-102: Proper default value handling in constructor.The constructor correctly sets default values:
chainIDbased onendpointType(staging_celo → 44787, others → 42220)versionwith fallback to 2userDefinedDataas empty stringThe logic properly handles different deployment environments.
sdk/qrcode/package.json (2)
3-3: Verify the major version jump reasoning.The version increased from 0.0.19 to 1.0.8, which represents a significant jump. Ensure this aligns with breaking changes or migration to a stable API.
11-25: LGTM: Proper dual ESM/CommonJS export configuration.The export structure correctly supports both module systems with appropriate type declarations for each format.
contracts/test/v2/disclosePassport.test.ts (2)
119-119: LGTM: Consistent user identifier formatting.The standardization of user identifier formatting with "0x" prefix is applied consistently across all test cases.
Also applies to: 303-303, 423-423, 700-700, 773-773, 834-834, 895-895
186-212: LGTM: Proper test coverage for invalid config ID.The test correctly validates that verification fails when a config is set directly on the test contract without being registered in the hub, expecting the
ConfigNotSeterror.contracts/ignition/modules/deployTestSelfVerificationRoot.ts (2)
3-17: LGTM: Comprehensive documentation and usage examples.The module includes clear usage instructions, parameter descriptions, and verification commands.
20-24: Verify hardcoded default values for production safety.The default hub address and scope value are hardcoded. Ensure the hub address is valid for the target network and consider using environment-specific configurations.
#!/bin/bash # Verify the hardcoded hub address exists and is a valid contract echo "Checking if the hardcoded hub address is a valid contract..." if command -v cast >/dev/null 2>&1; then cast code 0x3e2487a250e2A7b56c7ef5307Fb591Cc8C83623D --rpc-url https://alfajores-forno.celo-testnet.org else echo "Cast not available. Please verify the address manually on a block explorer." fiapp/src/screens/passport/PassportNFCTroubleScreen.tsx (3)
52-57: LGTM: Improved gesture accessibility.Changing from two-finger to single-finger tap improves usability and accessibility for users.
63-64: LGTM: Enhanced user navigation options.The secondary button provides a clear, discoverable way for users to access NFC options without relying solely on the hidden gesture.
17-17: LGTM: Consistent quote standardization.The text content updates use consistent straight single quotes throughout.
Also applies to: 25-25, 33-33, 37-37
app/package.json (4)
3-3: LGTM: Appropriate patch version increment.Version bump from 2.5.3 to 2.5.4 is consistent with incremental changes.
42-42: LGTM: Comprehensive setup script.The new setup script includes all necessary steps for a clean development environment setup, including iOS-specific dependencies and cleanup.
75-75: LGTM: Toast dependency addition aligns with UI changes.The
@tamagui/toastdependency supports the toast functionality mentioned in the AI summary.
98-98: Fixed version for gesture handler ensures stability.Changing from
^2.22.1to2.24.0(fixed version) provides more predictable behavior, likely intentional for compatibility with the gesture changes in the UI.app/src/screens/passport/PassportNFCScanScreen.tsx (5)
6-10: LGTM! Import consolidation improves code organization.The consolidation of related imports from
@selfxyz/commonenhances readability and maintainability.
71-71: Good addition of NFC trouble navigation hook.The new
goToNFCTroublehook provides clean navigation to help functionality, consistent with other navigation patterns in the component.
80-93: Well-implemented error modal consolidation.The
openErrorModalfunction provides a clean, unified approach to error handling with both dismiss and help options. The implementation correctly uses navigation dependencies and follows React patterns.
310-324: Excellent accessibility implementation for the help button.The help button UI enhancement is well-implemented with:
- Proper XStack layout for alignment
- Accessible aria-label
- Consistent styling with slate500 color
- Clear visual hierarchy with the CircleHelp icon
73-78: ```shell
#!/bin/bashRetry search for devModeTap gesture patterns without restricting file types
echo "Searching for all Gesture.Tap usages across .tsx and .ts files..."
rg -A 5 "Gesture.Tap" -g '.tsx' -g '.ts'</details> <details> <summary>contracts/ignition/modules/hub/deployHubV2.ts (3)</summary> `5-67`: **Outstanding documentation enhances maintainability.** The comprehensive documentation covering deployment steps, configuration, and troubleshooting scenarios is exemplary. This significantly improves the developer experience and reduces deployment issues. --- `69-75`: **Proper library deployment and linkage implementation.** The CustomVerifier library deployment and linking to the implementation contract is correctly implemented. This ensures the contract dependencies are properly resolved during deployment. --- `80-82`: ```shell #!/bin/bash # Search for Solidity files (including V2) and inspect initialize function signatures echo "Finding all .sol files:" sol_files=$(find . -type f -name "*.sol") if [ -z "$sol_files" ]; then echo "No Solidity files found." else echo "$sol_files" echo for f in $sol_files; do echo "---- Inspecting $f ----" rg -n "function initialize" "$f" || echo " (no initialize function in $f)" echo done fi # Search for TypeChain-generated V2 contract interfaces and inspect initialize() echo "Finding TypeChain V2 typings:" tc_files=$(find . -type f -path "*typechain*" -name "*V2*.ts") if [ -z "$tc_files" ]; then echo "No TypeChain V2 files found." else echo "$tc_files" echo for f in $tc_files; do echo "---- Inspecting $f ----" rg -n "initialize\(" "$f" || echo " (no initialize signature in $f)" echo done fisdk/core/index.ts (2)
1-9: Good ESM compatibility with explicit file extensions.The addition of
.jsextensions to import statements improves ESM compatibility and ensures proper module resolution in modern JavaScript environments.
16-23: Export reorganization aligns with architectural changes.The shift from user identification utilities to configuration management and verification types reflects the broader SDK refactoring toward a configuration-based verification architecture. This improves modularity and separation of concerns.
sdk/core/src/errors.ts (2)
1-12: Comprehensive error enumeration for configuration validation.The
ConfigMismatchenum provides excellent coverage of potential configuration validation errors, supporting detailed error reporting and debugging.
14-29: Well-implemented custom error class with proper inheritance.The
ConfigMismatchErrorclass is excellently implemented with:
- Proper Error class extension
- Array-based issue collection for multiple validation failures
- Formatted error messages
- Correct prototype chain setup
- Convenient static helper method
This enables comprehensive configuration validation error reporting.
app/src/layouts/SimpleScrolledTitleLayout.tsx (2)
11-16: Clean interface extension maintains backward compatibility.The interface extension with optional
secondaryButtonTextandonSecondaryButtonPressproperties maintains backward compatibility while enabling new functionality.
37-41: Proper conditional rendering and button placement.The conditional rendering ensures the secondary button only appears when both props are provided, and the placement above the dismiss button follows good UX patterns. The margin bottom (
mb="$2") provides appropriate spacing.contracts/test/v2/discloseId.test.ts (3)
45-45: LGTM: Improved snapshot managementThe change to use a single snapshot in the
beforehook and revert/recreate it inafterEachis more efficient than the previous multiple snapshot approach.
125-125: LGTM: Consistent hex string formattingAdding the "0x" prefix to userIdentifierBigInt ensures consistent hex string formatting across all test cases, which aligns with Ethereum's hex string conventions.
Also applies to: 312-312, 671-671, 735-735, 800-800, 865-865
195-221: LGTM: Comprehensive test for invalid config IDThe new test case effectively validates that verification fails when using an unregistered config ID. The test correctly:
- Sets verification config directly on the test contract without registering it on the hub
- Expects the
ConfigNotSeterror from the hub contract- Follows proper test structure and error handling
app/src/screens/passport/NFCMethodSelectionScreen.tsx (3)
47-47: LGTM: Extended CAN support to AndroidAdding Android support for CAN authentication expands the functionality across platforms.
58-65: LGTM: Well-designed MRZ correction featureThe new 'mrzCorrection' method provides a useful fallback for cases where automatic MRZ reading fails. The implementation supports both platforms and has clear labeling.
84-105: LGTM: Proper user store integrationThe user store integration follows React patterns correctly with appropriate state selectors and update handlers.
sdk/core/src/utils/utils.ts (1)
13-23: LGTM: Well-implemented country code unpackingThe function correctly:
- Filters null characters before processing
- Groups characters into 3-character country codes
- Validates the length before adding to results
- Includes comprehensive JSDoc documentation
contracts/hardhat.config.ts (3)
57-57: LGTM: Consistent environment variable namingStandardizing from
CELO_KEYtoPRIVATE_KEYimproves consistency across network configurations.
59-63: LGTM: Consistent network renamingRenaming
celoAlfajorestoalfajoressimplifies the naming convention and maintains consistency with the etherscan configuration.Also applies to: 70-70
77-88: Verify API URL updatesThe API URLs have been updated to use
api.etherscan.io/v2/api?chainid=format. Please verify these endpoints are correct and operational for both Celo mainnet and Alfajores testnet.Are the Etherscan API v2 endpoints https://api.etherscan.io/v2/api?chainid=42220 and https://api.etherscan.io/v2/api?chainid=44787 the correct endpoints for Celo and Alfajores networks?sdk/core/src/types/types.ts (2)
11-46: LGTM: Comprehensive and well-structured type definitionsThe new type definitions provide excellent structure for the verification workflow:
VerificationConfigoffers flexible verification parametersVerificationResultprovides detailed verification outcomesGenericDiscloseOutputcovers all necessary disclosure fieldsAttestationIdensures type safety with the discloseIndices constantThe types are well-documented and follow TypeScript best practices.
5-9: Verify impact of removing pubSignals propertyThe removal of
pubSignalsfromVcAndDiscloseProofis a breaking change. Ensure all consumers of this type have been updated to handle this change.#!/bin/bash # Description: Search for usage of pubSignals property on VcAndDiscloseProof type # Expected: Find any remaining usage that needs to be updated rg -A 3 -B 3 "VcAndDiscloseProof.*pubSignals|\.pubSignals" --type tsapp/src/utils/proving/provingMachine.ts (3)
667-673: Good improvement in error handling.The addition of
actor?.send({ type: 'CONNECT_ERROR' })before throwing errors ensures the state machine is properly notified of connection failures, which improves the overall error handling flow.
642-646: Verify circuit mapping consistency across document categories.The circuit mapping logic conditionally selects between
DISCLOSEandDISCLOSE_IDbased on document category. Ensure that the mapping structure in the protocol store consistently provides both options.Let me verify the protocol store structure to ensure both circuit mappings are available:
#!/bin/bash # Description: Verify that both DISCLOSE and DISCLOSE_ID mappings exist in the protocol store # Expected: Find references to both DISCLOSE and DISCLOSE_ID circuit mappings rg -A 3 -B 3 "DISCLOSE.*\?" --type ts rg -A 3 -B 3 "circuits_dns_mapping" --type ts
842-848: Let’s inspect the actual implementation to confirm its edge-case handling:#!/bin/bash # Show the function definition and its immediate logic for getSolidityPackedUserContextData rg -n "export function getSolidityPackedUserContextData" -A20 common/src/utils/hash.tssdk/core/src/utils/constants.ts (1)
1-92: Well-structured constants with good type safety.The file provides a clean, typed approach to managing attestation data indices. The use of
constassertions and theAllIdsmap for efficient lookups are good design choices.However, consider adding JSDoc comments to document the purpose of each index group:
+/** + * Indices for various fields in disclose circuit outputs, keyed by attestation ID. + * Attestation ID 1: Passport documents + * Attestation ID 2: ID card documents + */ export const discloseIndices = {sdk/core/package.json (2)
3-25: Excellent dual package setup for ESM/CJS compatibility.The package configuration properly supports both ESM and CJS consumers with correct entry points, type declarations, and exports configuration. The version bump to 1.0.0 signals API stability.
30-30: ```shell
#!/bin/bash
set -e
echo "=== package.json files in repo ==="
fd package.json -t fif [ -f package.json ]; then
echo "=== root-level package.json (lines 1-200) ==="
sed -n '1,200p' package.json
else
echo "No root-level package.json found."
fi</details> <details> <summary>app/src/utils/proving/provingInputs.ts (4)</summary> `65-71`: **Good integration of user context data.** The extraction of user-defined data and chain ID from `selfApp`, combined with the calculation of `userIdentifierHash`, properly integrates the new user context features. --- `155-168`: ```shell #!/bin/bash # Fetch the attributeToPosition mapping to verify total bit length rg -n "attributeToPosition" -C5 "attributeToPosition" app/src/utils/proving/provingInputs.ts
75-75: ```shell
#!/bin/bashExtract the implementation of getSelectorDg1 to verify handling of both 'passport' and 'id_card'
rg -n -C 20 "getSelectorDg1" app/src/utils/proving/provingInputs.ts
--- `170-184`: ```shell #!/bin/bash # Gather attributeToPosition_ID mapping definitions across TypeScript files rg -n "attributeToPosition_ID" -A200 --type ts > mapping.txt echo "Extracted mapping lines:" head -n 20 mapping.txt echo "..." # Compute maximum end index from the extracted mapping python3 - << 'PY' import re with open('mapping.txt') as f: text = f.read() # Find all [start, end] pairs pairs = re.findall(r'\[\s*(\d+)\s*,\s*(\d+)\s*\]', text) if not pairs: print("No position entries found.") exit(1) ends = [int(end) for _, end in pairs] print("Max end index found:", max(ends)) PYapp/src/utils/proving/provingUtils.ts (1)
35-44: Excellent type safety improvements.The template literal types and suffix unions provide strong type safety for the different proof categories. This is a significant improvement over string literals.
contracts/contracts/example/SelfIdentityERC721.sol (2)
87-92: Implementation looks good.The custom
_existsfunction and its usage ingetIdentityAttributesis a valid approach since newer OpenZeppelin ERC721 versions don't expose_existspublicly. The implementation correctly checks token ownership.Also applies to: 152-154
140-140: Consider data exposure in event emission.The event emits the entire
outputstruct which may contain sensitive information. Verify that all fields inGenericDiscloseOutputV2are intended to be publicly visible on-chain.#!/bin/bash # Description: Check the GenericDiscloseOutputV2 struct definition to understand what data is being exposed # Search for the struct definition ast-grep --pattern 'struct GenericDiscloseOutputV2 { $$$ }' # Also search in interface files fd -e sol | xargs rg -A 20 "struct GenericDiscloseOutputV2"contracts/contracts/example/Airdrop.sol (2)
246-275: Well-implemented verification hook with proper safeguards.The verification hook correctly implements double-registration prevention through both nullifier and user identifier checks. The validation flow and state updates are properly ordered.
200-202: ```shell
#!/bin/bash
file=contracts/contracts/example/Airdrop.solShow imports related to verifiers and types
echo "===== Imports ====="
rg -n "import" -n 1-30 "$file"Show contract declaration and state variables
echo -e "\n===== Contract & State Variables ====="
rg -n -B 2 -A 10 "contract Airdrop" "$file"Show registration function context (around output.userIdentifier)
echo -e "\n===== Registration Function Context ====="
rg -n -B 10 -A 5 "output.userIdentifier" "$file"Show any verifier calls inside the contract
echo -e "\n===== Verifier Calls ====="
rg -n "verify" -A 3 "$file"</details> <details> <summary>contracts/contracts/example/HappyBirthday.sol (1)</summary> `189-199`: **Good implementation of birthday window check.** The time difference calculation correctly handles both past and future birthdays within the claimable window by using absolute difference. This provides a fair claiming period around the user's birthday. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| run: | | ||
| if [[ "${{ github.head_ref }}" != "dev" ]]; then | ||
| echo "You can only merge from dev to main." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Fix security vulnerability with untrusted input.
The direct use of ${{ github.head_ref }} in the shell script poses a security risk, as this value is potentially untrusted and could lead to script injection.
Apply this diff to use environment variables instead:
- name: Block PRs not from dev
+ env:
+ HEAD_REF: ${{ github.head_ref }}
run: |
- if [[ "${{ github.head_ref }}" != "dev" ]]; then
+ if [[ "$HEAD_REF" != "dev" ]]; then
echo "You can only merge from dev to main."
exit 1
fi📝 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.
| run: | | |
| if [[ "${{ github.head_ref }}" != "dev" ]]; then | |
| echo "You can only merge from dev to main." | |
| exit 1 | |
| fi | |
| - name: Block PRs not from dev | |
| env: | |
| HEAD_REF: ${{ github.head_ref }} | |
| run: | | |
| if [[ "$HEAD_REF" != "dev" ]]; then | |
| echo "You can only merge from dev to main." | |
| exit 1 | |
| fi |
🧰 Tools
🪛 actionlint (1.7.7)
12-12: "github.head_ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🤖 Prompt for AI Agents
In .github/workflows/block-non-dev-to-main.yml around lines 12 to 16, the script
directly uses the untrusted input `${{ github.head_ref }}` in the shell command,
which risks script injection. To fix this, assign `${{ github.head_ref }}` to a
safe environment variable before the run step, then reference that environment
variable inside the shell script condition. This prevents direct injection and
secures the script execution.
| "qrcode.react": "^4.1.0", | ||
| "react-spinners": "^0.14.1", | ||
| "socket.io-client": "^4.8.1", | ||
| "tsup": "^8.5.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant tsup dependency.
tsup appears in both dependencies and devDependencies. Build tools should typically only be in devDependencies unless there's a specific runtime requirement.
"dependencies": {
"@selfxyz/common": "workspace:^",
"js-sha1": "^0.7.0",
"js-sha256": "^0.11.0",
"js-sha512": "^0.9.0",
"lottie-react": "^2.4.0",
"node-forge": "^1.3.1",
"poseidon-lite": "^0.3.0",
"qrcode.react": "^4.1.0",
"react-spinners": "^0.14.1",
"socket.io-client": "^4.8.1",
- "tsup": "^8.5.0",
"uuid": "^10.0.0"
},Also applies to: 69-69
🤖 Prompt for AI Agents
In sdk/qrcode/package.json at lines 51 and 69, the dependency "tsup" is listed
in both "dependencies" and "devDependencies". Since "tsup" is a build tool,
remove it from the "dependencies" section and keep it only in "devDependencies"
to avoid redundancy and follow best practices.
| async getConfig(configId: string): Promise<VerificationConfig> { | ||
| return this.configs.get(configId); | ||
| } |
There was a problem hiding this comment.
Fix interface contract violation in getConfig method.
The getConfig method can return undefined when a config doesn't exist, but the IConfigStorage interface promises to return VerificationConfig. This violates the interface contract and could cause runtime errors.
- async getConfig(configId: string): Promise<VerificationConfig> {
- return this.configs.get(configId);
- }
+ async getConfig(configId: string): Promise<VerificationConfig> {
+ const config = this.configs.get(configId);
+ if (!config) {
+ throw new Error(`Configuration not found for ID: ${configId}`);
+ }
+ return config;
+ }📝 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.
| async getConfig(configId: string): Promise<VerificationConfig> { | |
| return this.configs.get(configId); | |
| } | |
| async getConfig(configId: string): Promise<VerificationConfig> { | |
| const config = this.configs.get(configId); | |
| if (!config) { | |
| throw new Error(`Configuration not found for ID: ${configId}`); | |
| } | |
| return config; | |
| } |
🤖 Prompt for AI Agents
In sdk/core/src/store/InMemoryConfigStore.ts around lines 22 to 24, the
getConfig method currently returns the result of this.configs.get(configId),
which can be undefined if the config does not exist, violating the
IConfigStorage interface that requires a VerificationConfig return. To fix this,
modify getConfig to check if the config exists and handle the undefined case
appropriately, either by throwing an error or returning a default
VerificationConfig, ensuring the method always returns a VerificationConfig as
promised by the interface.
| {method.key === 'mrzCorrection' && | ||
| selectedMethod === 'mrzCorrection' && ( | ||
| <YStack marginTop={12} gap={8}> | ||
| <Input | ||
| placeholder="Enter Passport/ID Number" | ||
| value={passportNumber} | ||
| onChangeText={onPassportNumberChange} | ||
| /> | ||
|
|
||
| <BodyText>Birth Date (YYMMDD)</BodyText> | ||
| <Input | ||
| placeholder="YYMMDD" | ||
| value={dateOfBirth} | ||
| onChangeText={onDateOfBirthChange} | ||
| keyboardType="numeric" | ||
| maxLength={6} | ||
| /> | ||
|
|
||
| <BodyText>Date of Expiry (YYMMDD)</BodyText> | ||
| <Input | ||
| placeholder="YYMMDD" | ||
| value={dateOfExpiry} | ||
| onChangeText={onDateOfExpiryChange} | ||
| keyboardType="numeric" | ||
| maxLength={6} | ||
| /> | ||
| </YStack> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for MRZ fields
The MRZ correction inputs lack validation for date formats and passport number constraints. Consider adding:
- YYMMDD format validation for dates
- Character restrictions for passport numbers
- Visual feedback for invalid inputs
<Input
placeholder="YYMMDD"
value={dateOfBirth}
onChangeText={onDateOfBirthChange}
keyboardType="numeric"
maxLength={6}
+ autoCompleteType="off"
+ textContentType="none"
/>
+ {dateOfBirth && !/^\d{6}$/.test(dateOfBirth) && (
+ <Description color="red">
+ Please enter date in YYMMDD format
+ </Description>
+ )}📝 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.
| {method.key === 'mrzCorrection' && | |
| selectedMethod === 'mrzCorrection' && ( | |
| <YStack marginTop={12} gap={8}> | |
| <Input | |
| placeholder="Enter Passport/ID Number" | |
| value={passportNumber} | |
| onChangeText={onPassportNumberChange} | |
| /> | |
| <BodyText>Birth Date (YYMMDD)</BodyText> | |
| <Input | |
| placeholder="YYMMDD" | |
| value={dateOfBirth} | |
| onChangeText={onDateOfBirthChange} | |
| keyboardType="numeric" | |
| maxLength={6} | |
| /> | |
| <BodyText>Date of Expiry (YYMMDD)</BodyText> | |
| <Input | |
| placeholder="YYMMDD" | |
| value={dateOfExpiry} | |
| onChangeText={onDateOfExpiryChange} | |
| keyboardType="numeric" | |
| maxLength={6} | |
| /> | |
| </YStack> | |
| )} | |
| {method.key === 'mrzCorrection' && | |
| selectedMethod === 'mrzCorrection' && ( | |
| <YStack marginTop={12} gap={8}> | |
| <Input | |
| placeholder="Enter Passport/ID Number" | |
| value={passportNumber} | |
| onChangeText={onPassportNumberChange} | |
| /> | |
| <BodyText>Birth Date (YYMMDD)</BodyText> | |
| <Input | |
| placeholder="YYMMDD" | |
| value={dateOfBirth} | |
| onChangeText={onDateOfBirthChange} | |
| keyboardType="numeric" | |
| maxLength={6} | |
| autoCompleteType="off" | |
| textContentType="none" | |
| /> | |
| {dateOfBirth && !/^\d{6}$/.test(dateOfBirth) && ( | |
| <Description color="red"> | |
| Please enter date in YYMMDD format | |
| </Description> | |
| )} | |
| <BodyText>Date of Expiry (YYMMDD)</BodyText> | |
| <Input | |
| placeholder="YYMMDD" | |
| value={dateOfExpiry} | |
| onChangeText={onDateOfExpiryChange} | |
| keyboardType="numeric" | |
| maxLength={6} | |
| /> | |
| </YStack> | |
| )} |
🤖 Prompt for AI Agents
In app/src/screens/passport/NFCMethodSelectionScreen.tsx between lines 163 and
190, the MRZ correction input fields currently lack validation. Add validation
logic to ensure the date inputs (dateOfBirth and dateOfExpiry) strictly follow
the YYMMDD format and the passportNumber input enforces allowed character
constraints. Implement visual feedback such as error messages or input border
color changes to indicate invalid inputs to the user.
| const CELO_MAINNET_RPC_URL = 'https://forno.celo.org'; | ||
| const CELO_TESTNET_RPC_URL = 'https://alfajores-forno.celo-testnet.org'; | ||
|
|
||
| const IDENTITY_VERIFICATION_HUB_ADDRESS = '0x0000000000000000000000000000000000000000'; |
There was a problem hiding this comment.
Critical: Invalid mainnet contract address.
The IDENTITY_VERIFICATION_HUB_ADDRESS is set to the zero address, which will cause contract connection failures in production. This needs to be updated with the actual deployed contract address before mainnet usage.
-const IDENTITY_VERIFICATION_HUB_ADDRESS = '0x0000000000000000000000000000000000000000';
+const IDENTITY_VERIFICATION_HUB_ADDRESS = '<ACTUAL_MAINNET_ADDRESS>'; // TODO: Update with deployed addressAlso applies to: 45-47
🤖 Prompt for AI Agents
In sdk/core/src/SelfBackendVerifier.ts at lines 24 and 45-47, the constant
IDENTITY_VERIFICATION_HUB_ADDRESS is incorrectly set to the zero address, which
is invalid for mainnet deployment. Replace this placeholder with the actual
deployed mainnet contract address to ensure proper contract connection in
production environments.
| revealedDataTypes.expiry_date, | ||
| ]; | ||
| const publicSignals = pubSignals.map(String).map((x) => (/[a-f]/g.test(x) ? '0x' + x : x)); | ||
| //check if user context hash matches |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve hex string detection logic.
The current regex /[a-f]/g only checks for lowercase hex characters and could have false positives. Consider using a more robust check.
-const publicSignals = pubSignals.map(String).map((x) => (/[a-f]/g.test(x) ? '0x' + x : x));
+const publicSignals = pubSignals.map(String).map((x) => {
+ // Check if the string is a valid hex without 0x prefix
+ return /^[0-9a-fA-F]+$/.test(x) && x.length > 0 ? '0x' + x : x;
+});📝 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 publicSignals = pubSignals.map(String).map((x) => (/[a-f]/g.test(x) ? '0x' + x : x)); | |
| const publicSignals = pubSignals.map(String).map((x) => { | |
| // Check if the string is a valid hex without 0x prefix | |
| return /^[0-9a-fA-F]+$/.test(x) && x.length > 0 ? '0x' + x : x; | |
| }); |
🤖 Prompt for AI Agents
In sdk/core/src/SelfBackendVerifier.ts at line 72, the current regex used to
detect hex strings only checks for lowercase letters a-f and may produce false
positives. Update the regex to check for both uppercase and lowercase hex
characters (0-9, a-f, A-F) and ensure the entire string matches a hex pattern,
not just the presence of any hex character. Replace the regex with a more
precise pattern like `/^[0-9a-fA-F]+$/` to accurately identify hex strings
before prefixing with '0x'.
| result = await this.verifyAllContract.verifyAll(timestamp, vcAndDiscloseHubProof, types); | ||
| } catch (error: any) { | ||
| let errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
| if ( | ||
| error && | ||
| typeof error === 'object' && | ||
| error.message && | ||
| error.message.includes('INVALID_FORBIDDEN_COUNTRIES') | ||
| ) { | ||
| errorMessage = | ||
| 'The forbidden countries list in the backend does not match the list provided in the frontend SDK. Please ensure both lists are identical.'; | ||
| } | ||
| verificationConfig = await this.configStorage.getConfig(configId); | ||
| } catch (error) { | ||
| issues.push({ type: ConfigMismatch.ConfigNotFound, message: 'Config not found' }); | ||
| } finally { | ||
| if (!verificationConfig) throw new ConfigMismatchError(issues); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix unsafe throw in finally block.
The throw statement in the finally block will override any exception from the try block, potentially masking important error information. This violates error handling best practices.
Refactor the error handling logic:
-let verificationConfig: VerificationConfig | null;
-try {
- verificationConfig = await this.configStorage.getConfig(configId);
-} catch (error) {
- issues.push({ type: ConfigMismatch.ConfigNotFound, message: 'Config not found' });
-} finally {
- if (!verificationConfig) throw new ConfigMismatchError(issues);
-}
+let verificationConfig: VerificationConfig | null = null;
+try {
+ verificationConfig = await this.configStorage.getConfig(configId);
+} catch (error) {
+ issues.push({ type: ConfigMismatch.ConfigNotFound, message: 'Config not found' });
+}
+
+if (!verificationConfig) {
+ throw new ConfigMismatchError(issues);
+}📝 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.
| try { | |
| result = await this.verifyAllContract.verifyAll(timestamp, vcAndDiscloseHubProof, types); | |
| } catch (error: any) { | |
| let errorMessage = error instanceof Error ? error.message : 'Unknown error'; | |
| if ( | |
| error && | |
| typeof error === 'object' && | |
| error.message && | |
| error.message.includes('INVALID_FORBIDDEN_COUNTRIES') | |
| ) { | |
| errorMessage = | |
| 'The forbidden countries list in the backend does not match the list provided in the frontend SDK. Please ensure both lists are identical.'; | |
| } | |
| verificationConfig = await this.configStorage.getConfig(configId); | |
| } catch (error) { | |
| issues.push({ type: ConfigMismatch.ConfigNotFound, message: 'Config not found' }); | |
| } finally { | |
| if (!verificationConfig) throw new ConfigMismatchError(issues); | |
| } | |
| let verificationConfig: VerificationConfig | null = null; | |
| try { | |
| verificationConfig = await this.configStorage.getConfig(configId); | |
| } catch (error) { | |
| issues.push({ type: ConfigMismatch.ConfigNotFound, message: 'Config not found' }); | |
| } | |
| if (!verificationConfig) { | |
| throw new ConfigMismatchError(issues); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 141-141: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🤖 Prompt for AI Agents
In sdk/core/src/SelfBackendVerifier.ts around lines 136 to 142, the throw
statement is inside the finally block, which can override any error thrown in
the try block and mask important error details. To fix this, remove the throw
from the finally block and instead place it after the try-catch-finally
structure. Ensure that you only throw the ConfigMismatchError if
verificationConfig is falsy and handle errors properly without suppressing
original exceptions.
| uint256 public euidBonusMultiplier = 200; // 200% = 100% bonus | ||
|
|
||
| /// @notice Bonus multiplier for E-Passport card users (in basis points) | ||
| uint256 public passportBonusMultiplier = 100; // 100% = 50% bonus |
There was a problem hiding this comment.
Missing implementation for passport bonus multiplier.
The contract defines passportBonusMultiplier but doesn't apply it for E-Passport attestations. This seems inconsistent with the contract's description of supporting differentiated bonuses.
Add the missing logic:
// Apply bonus multiplier for EUID card users
if (output.attestationId == AttestationId.EU_ID_CARD) {
finalAmount = (claimableAmount * euidBonusMultiplier) / BASIS_POINTS;
+} else if (output.attestationId == AttestationId.E_PASSPORT) {
+ finalAmount = (claimableAmount * passportBonusMultiplier) / BASIS_POINTS;
}Also applies to: 146-149
🤖 Prompt for AI Agents
In contracts/contracts/example/HappyBirthday.sol around lines 39 to 42 and also
lines 146 to 149, the passportBonusMultiplier variable is declared but not used
in the logic for calculating bonuses for E-Passport card users. To fix this, add
the necessary conditional logic to apply the passportBonusMultiplier when
processing E-Passport attestations, ensuring the bonus calculation
differentiates between EUID and E-Passport users as intended by the contract
design.
|
|
||
| // Create birthday in current year (format: YYMMDD) | ||
| string memory dobInThisYear = string(abi.encodePacked("25", month, day)); | ||
| uint256 dobInThisYearTimestamp = Formatter.dateToUnixTimestamp(dobInThisYear); |
There was a problem hiding this comment.
Critical: Hardcoded year will break after 2025.
The hardcoded year "25" will cause incorrect birthday calculations starting in 2026. This needs to be dynamically calculated from the current block timestamp.
Replace with dynamic year calculation:
-// Create birthday in current year (format: YYMMDD)
-string memory dobInThisYear = string(abi.encodePacked("25", month, day));
+// Get current year from block timestamp
+uint256 currentYear = (block.timestamp / 365 days) + 1970; // Unix epoch year
+uint256 yearLastTwoDigits = currentYear % 100;
+string memory yearStr = Formatter.uintToString(yearLastTwoDigits);
+if (yearLastTwoDigits < 10) {
+ yearStr = string(abi.encodePacked("0", yearStr));
+}
+string memory dobInThisYear = string(abi.encodePacked(yearStr, month, day));Note: You'll need to implement or import a uintToString helper function.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/contracts/example/HappyBirthday.sol at line 186, the year "25" is
hardcoded in the dobInThisYear string, which will cause incorrect birthday
calculations after 2025. Replace the hardcoded "25" with a dynamically
calculated year derived from the current block timestamp, converting the year to
a string using a uintToString helper function that you should implement or
import. This ensures the year portion of the date updates correctly with the
blockchain time.
|
merged into dev |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation
Refactor
Tests
Revert