Skip to content

Conversation

@Vishalkulkarni45
Copy link
Collaborator

@Vishalkulkarni45 Vishalkulkarni45 commented Sep 18, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Standardized age verification across attestations, improving accuracy for non-Aadhaar proofs and ensuring consistent results across document types.
    • Updated timestamp validation to honor an end-of-day window, reducing unintended expiry failures and aligning behavior with other platforms.
    • Improves overall reliability of verification outcomes without requiring any user action.
  • Chores
    • No changes to public APIs; compatibility maintained.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

Walkthrough

The Go SDK updates align minimumAge parsing for non-Aadhaar with the Aadhaar path by using a string slice instead of a single-byte conversion. Timestamp validation now treats circuit timestamps as valid through end-of-day by comparing against a computed end-of-day value.

Changes

Cohort / File(s) Summary
Minimum age parsing alignment
sdk/sdk-go/utils.go
Non-Aadhaar minimumAge extraction now uses the full string slice [OlderThanStart:OlderThanEnd+1], matching Aadhaar handling; removes prior single-byte numeric formatting path.
Timestamp EOD validation
sdk/sdk-go/verifier.go
validateTimestamp adds end-of-day computation (circuitTimestampEOD = circuitTimestamp + 23h59m59s) and compares EOD to oneDayAgo; adds comments for parity with TypeScript logic.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Utils as FormatRevealedDataPacked
  participant Data as RevealedData

  Caller->>Utils: FormatRevealedDataPacked(Data)
  Utils->>Data: Read OlderThanStart, OlderThanEnd, isAadhaar
  alt Aadhaar or Non-Aadhaar (new path)
    Utils->>Utils: minAge = string slice [start:end+1]
  end
  Utils-->>Caller: Packed output with minAge
  note right of Utils: Non-Aadhaar no longer uses single-byte decimal
Loading
sequenceDiagram
  autonumber
  participant Verifier as validateTimestamp
  participant Input as circuitTimestamp
  participant Clock as Now()

  Verifier->>Clock: Get current time
  Clock-->>Verifier: now
  Verifier->>Verifier: oneDayAgo = now - 24h
  Verifier->>Verifier: circuitTimestampEOD = circuitTimestamp + 23:59:59
  alt Old check (previous)
    note over Verifier: circuitTimestamp.Before(oneDayAgo)
  else New check (current)
    Verifier->>Verifier: isOld = circuitTimestampEOD.Before(oneDayAgo)
  end
  Verifier-->>Caller: Validation result
  note right of Verifier: Aligns with TS end-of-day semantics
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Strings, not bytes, now tell the age,
The clock extends the day’s last page.
Go whispers, “Wait till midnight’s done,”
Before it calls a timestamp shunned.
Two tweaks align the paths and time—
A tidier beat, a truer chime. ⏳✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: go-sdk" is directly related to the changes since the diff modifies the Go SDK, but it is overly generic and does not describe the specific fixes (minimumAge parsing alignment and end-of-day timestamp handling). Because it references the affected component it satisfies the "partially related" criterion and the check passes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/go-sdk

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86c595b and d517ee8.

📒 Files selected for processing (2)
  • sdk/sdk-go/utils.go (1 hunks)
  • sdk/sdk-go/verifier.go (1 hunks)
🔇 Additional comments (1)
sdk/sdk-go/verifier.go (1)

632-637: End-of-day tolerance: confirm spec parity and future-side semantics.

EOD padding for the “too old” check looks correct and matches the intent. The “future” check still uses strict now+24h, which is asymmetric with the EOD leniency. If the TypeScript SDK considers “tomorrow’s date” valid through end-of-day as well, mirror that here; otherwise please confirm this intentional asymmetry.

Comment on lines +462 to 464
} else {
minimumAge = string(revealedDataPackedBytes[revealedDataIndices.OlderThanStart : revealedDataIndices.OlderThanEnd+1])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

MinimumAge raw slice may include NULs/non‑digits; normalize to digits to avoid mis-parse in validation.

Switching non‑Aadhaar to a raw byte slice can surface padding bytes (0x00) or non‑digit characters, causing fmt.Sscanf() in validateWithConfig to parse partially or fail, leading to incorrect allow/deny. Normalize to a two‑digit numeric string before returning.

Apply:

- } else {
-   minimumAge = string(revealedDataPackedBytes[revealedDataIndices.OlderThanStart : revealedDataIndices.OlderThanEnd+1])
- }
+ } else {
+   // Normalize to two-digit numeric string; strip non-digits/NULs
+   raw := revealedDataPackedBytes[revealedDataIndices.OlderThanStart : revealedDataIndices.OlderThanEnd+1]
+   buf := make([]byte, 0, 2)
+   for _, b := range raw {
+     if b >= '0' && b <= '9' {
+       buf = append(buf, b)
+     }
+   }
+   switch len(buf) {
+   case 0:
+     minimumAge = "00"
+   case 1:
+     minimumAge = "0" + string(buf)
+   default:
+     minimumAge = string(buf[:2])
+   }
+ }
📝 Committable suggestion

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

Suggested change
} else {
minimumAge = string(revealedDataPackedBytes[revealedDataIndices.OlderThanStart : revealedDataIndices.OlderThanEnd+1])
}
} else {
// Normalize to two-digit numeric string; strip non-digits/NULs
raw := revealedDataPackedBytes[revealedDataIndices.OlderThanStart : revealedDataIndices.OlderThanEnd+1]
buf := make([]byte, 0, 2)
for _, b := range raw {
if b >= '0' && b <= '9' {
buf = append(buf, b)
}
}
switch len(buf) {
case 0:
minimumAge = "00"
case 1:
minimumAge = "0" + string(buf)
default:
minimumAge = string(buf[:2])
}
}
🤖 Prompt for AI Agents
In sdk/sdk-go/utils.go around lines 462 to 464, the code assigns minimumAge
directly from a raw byte slice which may include NULs or non-digit bytes;
instead, extract only digit characters from
revealedDataPackedBytes[revealedDataIndices.OlderThanStart:revealedDataIndices.OlderThanEnd+1],
build a normalized two‑digit string (if no digits -> "00", if one digit ->
prepend '0', if multiple -> use the last two digits), and assign that sanitized
string to minimumAge so downstream fmt.Sscanf() receives a clean two‑digit
numeric value.

@remicolin remicolin merged commit 664be08 into dev Sep 19, 2025
14 checks passed
@remicolin remicolin deleted the fix/go-sdk branch September 19, 2025 21:27
remicolin added a commit that referenced this pull request Sep 20, 2025
* chore: bump v2.6.5 rd2 (#1067)

* commit wip version bump

* remove from building

* chore: update tooling dependencies (#1069)

* chore: update tooling dependencies

* chore: align react typings and node types

* update lock

* chore: minor fixes across monorepo (#1068)

* small fixes

* fixes

* fix gesture handler error

* ci fixes

* fix yarn build; add workflow ci (#1075)

* add new workspace ci

* disable package version check for now

* build before checks

* format

* fix in future pr

* feat: add functions for disclosing aadhaar attributes (#1033)

* feat: add functions for disclosing aadhaar attributes

* format

* chore: update monorepo artifacts (#1079)

* remove unneeded artifacts, skip building circuits

* update md files

* cleans up unused parts of sdk interface, adds inline documentation, (#1078)

* cleans up unused parts of sdk interface, adds inline documentation,

* fix up build

* yolo

* Feat/aadhaar sdk (#1082)

* feat: add aadhaar support to the ts sdk

* feat: aadhaar support to go sdk

* chore: refactor

* move clearPassportData, markCurrentDocumentAsRegistered, reStorePassportDataWithRightCSCA to SDK (#1041)

* Move self app store to mobile sdk (#1040)

* chore(mobile-sdk-alpha): remove unused tslib dependency (#1053)

* remove tslib -- seems unused

* remove deps accidentally added to root

* build file

* remove unused imports (#1055)

* fix: sha256 signed attr tests (#1058)

* fix mock screen launch (#1059)

* Hotfix: Belgium ID cards (#1061)

* feat: parse belgium TD1 mrz android

* feat: Parse Belgium TD1 MRZ IOS

* fix: OFAC trees not found (#1060)

* fix: relax OFAC tree response validation

* test: cover OFAC tree edge cases

* fix stateless

* revert and fix types

* fix tests

* [SELF-723] feat: add structured NFC and Proof logging (#1048)

* feat: add structured NFC logging

* fix ci

* Fix: add deps

* logging fixes. use breadcrumbs

* fix android build

* update SeverityLevel

* [SELF-705] feat: add proof event logging (#1057)

* feat: add proof event logging

* refactor: unify sentry event logging

* fix types

* fix mock

* simplify

* code rabbit feedback

* fix tests

---------

Co-authored-by: seshanthS <[email protected]>

* skip on dev (#1063)

* don't get fancy just disable (#1064)

* saw it building so gonna try (#1065)

* chore: bump v2.6.5 rd2 (#1067)

* commit wip version bump

* remove from building

* chore: update tooling dependencies (#1069)

* chore: update tooling dependencies

* chore: align react typings and node types

* update lock

* chore: minor fixes across monorepo (#1068)

* small fixes

* fixes

* fix gesture handler error

* ci fixes

* fix yarn build; add workflow ci (#1075)

* add new workspace ci

* disable package version check for now

* build before checks

* format

* fix in future pr

* feat: add functions for disclosing aadhaar attributes (#1033)

* feat: add functions for disclosing aadhaar attributes

* format

* chore: update monorepo artifacts (#1079)

* remove unneeded artifacts, skip building circuits

* update md files

* chore: update hub contract address

* format

* fix: add aadhaar in AllIds

* chore: bump to v1.1.0-beta

---------

Co-authored-by: vishal <[email protected]>
Co-authored-by: Leszek Stachowski <[email protected]>
Co-authored-by: Aaron DeRuvo <[email protected]>
Co-authored-by: Justin Hernandez <[email protected]>
Co-authored-by: Seshanth.S🐺 <[email protected]>
Co-authored-by: seshanthS <[email protected]>

* feat: change to gcp attestation verification (#959)

* feat: change to gcp attestation verification

* lint

* fix e2e test

* chore: don't check PCR0 mapping if building the app locally

* fmt:fix

---------

Co-authored-by: Justin Hernandez <[email protected]>

* Mobile SDK: move provingMachine from the app (#1052)

* Mobile SDK: move provingMachine from the app

* lint, fixes

* fix web build?

* lint

* fix metro build, add deps

* update lock files

* move the status handlers and proving machine tests

* may it be

* fix up

* yolo

---------

Co-authored-by: Justin Hernandez <[email protected]>
Co-authored-by: Aaron DeRuvo <[email protected]>

* Revert "Mobile SDK: move provingMachine from the app (#1052)" (#1084)

This reverts commit 8983ac2.

* fix: sdk (#1085)

* bump sdk (#1086)

* chore update mobile app types (#1087)

* clean up types

* clean up additional types

* format

* fix types

* feat: add contract utils (#1088)

* Feat/contracts npm publish (#1089)

* chore: ci to publish contracts

* yarn fmt

* fix: use celo sepolia in common (#1091)

* chore: export selfappbuilder (#1092)

* [SELF-747] feat: clone android passport reader during setup (#1080)

* chore: remove android private modules doc

* private repo pull

* skip private modules

* remove unused circuits building

* save wip

* format

* restore tsconfig

* fix package install

* fix internal repo cloning

* unify logic and fix cloning

* git clone internal repos efficiently

* formatting

* run app yarn reinstall from root

* coderabbit feedback

* coderabbit suggestions

* remove skip private modules logic

* fix: ensure PAT is passed through yarn-install action and handle missing PAT gracefully

- Update yarn-install action to pass SELFXYZ_INTERNAL_REPO_PAT to yarn install
- Make setup-private-modules.cjs skip gracefully when PAT is unavailable in CI
- Fixes issue where setup script was throwing error instead of skipping for forks

* prettier

* fix clone ci

* clone ci fixes

* fix import export sorts

* fix instructions

* fix: remove SelfAppBuilder re-export to fix duplicate export error

- Remove SelfAppBuilder import/export from @selfxyz/qrcode
- Update README to import SelfAppBuilder directly from @selfxyz/common
- Fixes CI build failure with duplicate export error

* fix: unify eslint-plugin-sort-exports version across workspaces

- Update mobile-sdk-alpha from 0.8.0 to 0.9.1 to match other workspaces
- Removes yarn.lock version conflict causing CI/local behavior mismatch
- Fixes quality-checks workflow linting failure

* fix: bust qrcode SDK build cache to resolve stale SelfAppBuilder issue

- Increment GH_SDK_CACHE_VERSION from v1 to v2
- Forces CI to rebuild artifacts from scratch instead of using cached version
- Resolves quality-checks linter error showing removed SelfAppBuilder export

* skip job

* test yarn cache

* bump cache version to try and fix the issue

* revert cache version

* refactor: use direct re-exports for cleaner qrcode package structure

- Replace import-then-export pattern with direct re-exports
- Keep SelfAppBuilder export with proper alphabetical sorting (before SelfQRcode)
- Maintain API compatibility as documented in README
- Eliminates linter sorting issues while keeping clean code structure

* fix: separate type and value imports in README examples

- Import SelfApp as type since it's an interface
- Import SelfAppBuilder as value since it's a class
- Follows TypeScript best practices and improves tree shaking

* address version mismatches and package resolutions (#1081)

* fix package version mismatches and resolutions

* fixes

* update lock

* fix comma

* fixes

* fix packages

* update packages

* remove firebase analytics. not needed

* fix: aadhaar verifier abi (#1096)

* fix: aadhaar verifier abi

* bump: core

* fix: go-sdk (#1090)

* SELF-725: add iOS qrcode opener and aadhaar screen (#1038)

* add iOS qrcode opener and aadhaar screen

* format

* fix test

* add Image-picker android (#1077)

* add image-picker android

* fix validation

* feat: implement Aadhaar upload success and error screens, enhance AadhaarNavBar with dynamic progress indication

- Added AadhaarUploadedSuccessScreen and AadhaarUploadErrorScreen components for handling upload outcomes.
- Updated AadhaarNavBar to reflect current upload step with dynamic progress bar.
- Integrated new screens into navigation flow for Aadhaar upload process.
- Introduced blue check and warning SVG icons for visual feedback on success and error states.

* feat: generate mock aadhar (#1083)

* feat: generate mock aadhar

* add yarn.lock

* update yarn.lock

* update protocolStore, update types, start modifying provingMachine

* Register mock aadhar (#1093)

* Register mock aadhar

* fix ofac

* temp: generate name

* fix dob

* Add Aadhaar support to ID card component and screens

- Integrated Aadhaar icon and conditional rendering in IdCardLayout.
- Updated AadhaarUploadScreen to process QR codes and store Aadhaar data.
- Modified navigation and button text in AadhaarUploadedSuccessScreen.
- Added mock data generation for Aadhaar in the mobile SDK.
- Updated ManageDocumentsScreen to include Aadhaar document type.
- Enhanced error handling and validation for Aadhaar QR code processing.
- Added utility functions for Aadhaar data extraction and commitment processing.

* aadhaar disclose - wip (#1094)

* fix: timestamp cal of extractQRDataFields

* Feat/aadhar fixes (#1099)

* Fix - android aadhar qr scanner

* fixes

* update text

* yarn nice

* run prettier

* Add mock Aadhaar certificates for development

- Introduced hardcoded Aadhaar test certificates for development purposes.
- Moved Aadhaar mock private and public keys to a dedicated file for better organization.
- Updated the mock ID document generation utility to utilize the new Aadhaar mock certificates.

* prettier write

* add 'add-aadhaar' button (#1100)

* Update .gitleaks.toml to include path for mock certificates in the common/dist directory

* yarn nice

* Enhance Aadhaar error handling with specific error types

- Updated the AadhaarUploadErrorScreen to display different messages based on the error type (general or expired).
- Modified the AadhaarUploadScreen to pass the appropriate error type when navigating to the error screen.
- Set initial parameters for the home screen to include a default error type.

* Update passport handling in proving machine to support Aadhaar document category

- Modified the handling of country code in the useProvingStore to return 'IND' for Aadhaar documents.
- Ensured that the country code is only fetched from passport metadata for non-Aadhaar documents.

* tweak layout, text, change email to support, hide help button

* fix ci, remove aadhaar logging, add aadhaar events

* remove unused aadhaar tracking events

* update globs

* fix gitguardian config

* don't track id

---------

Co-authored-by: Justin Hernandez <[email protected]>
Co-authored-by: Seshanth.S🐺 <[email protected]>
Co-authored-by: vishal <[email protected]>

* fix aadhaar screen test (#1101)

* add iOS qrcode opener and aadhaar screen

* format

* fix test

* add Image-picker android (#1077)

* add image-picker android

* fix validation

* feat: implement Aadhaar upload success and error screens, enhance AadhaarNavBar with dynamic progress indication

- Added AadhaarUploadedSuccessScreen and AadhaarUploadErrorScreen components for handling upload outcomes.
- Updated AadhaarNavBar to reflect current upload step with dynamic progress bar.
- Integrated new screens into navigation flow for Aadhaar upload process.
- Introduced blue check and warning SVG icons for visual feedback on success and error states.

* feat: generate mock aadhar (#1083)

* feat: generate mock aadhar

* add yarn.lock

* update yarn.lock

* update protocolStore, update types, start modifying provingMachine

* Register mock aadhar (#1093)

* Register mock aadhar

* fix ofac

* temp: generate name

* fix dob

* Add Aadhaar support to ID card component and screens

- Integrated Aadhaar icon and conditional rendering in IdCardLayout.
- Updated AadhaarUploadScreen to process QR codes and store Aadhaar data.
- Modified navigation and button text in AadhaarUploadedSuccessScreen.
- Added mock data generation for Aadhaar in the mobile SDK.
- Updated ManageDocumentsScreen to include Aadhaar document type.
- Enhanced error handling and validation for Aadhaar QR code processing.
- Added utility functions for Aadhaar data extraction and commitment processing.

* aadhaar disclose - wip (#1094)

* fix: timestamp cal of extractQRDataFields

* Feat/aadhar fixes (#1099)

* Fix - android aadhar qr scanner

* fixes

* update text

* yarn nice

* run prettier

* Add mock Aadhaar certificates for development

- Introduced hardcoded Aadhaar test certificates for development purposes.
- Moved Aadhaar mock private and public keys to a dedicated file for better organization.
- Updated the mock ID document generation utility to utilize the new Aadhaar mock certificates.

* prettier write

* add 'add-aadhaar' button (#1100)

* Update .gitleaks.toml to include path for mock certificates in the common/dist directory

* yarn nice

* Enhance Aadhaar error handling with specific error types

- Updated the AadhaarUploadErrorScreen to display different messages based on the error type (general or expired).
- Modified the AadhaarUploadScreen to pass the appropriate error type when navigating to the error screen.
- Set initial parameters for the home screen to include a default error type.

* Update passport handling in proving machine to support Aadhaar document category

- Modified the handling of country code in the useProvingStore to return 'IND' for Aadhaar documents.
- Ensured that the country code is only fetched from passport metadata for non-Aadhaar documents.

* tweak layout, text, change email to support, hide help button

* fix ci, remove aadhaar logging, add aadhaar events

* remove unused aadhaar tracking events

* update globs

* fix gitguardian config

* don't track id

* fix test

---------

Co-authored-by: turnoffthiscomputer <[email protected]>
Co-authored-by: Seshanth.S🐺 <[email protected]>
Co-authored-by: vishal <[email protected]>

---------

Co-authored-by: Justin Hernandez <[email protected]>
Co-authored-by: Nesopie <[email protected]>
Co-authored-by: Aaron DeRuvo <[email protected]>
Co-authored-by: vishal <[email protected]>
Co-authored-by: Leszek Stachowski <[email protected]>
Co-authored-by: Seshanth.S🐺 <[email protected]>
Co-authored-by: seshanthS <[email protected]>
Co-authored-by: Justin Hernandez <[email protected]>
Co-authored-by: Vishalkulkarni45 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants