Conversation
PR Title Validation for conventional commit type✅ All good! PR title follows the conventional commit type. |
Jira Pull Request LinkThis Pull Request refers to Jira issues: |
mastro993
requested review from
a team,
ChrisMattew,
RiccardoMolinari95,
freddi301,
gispada and
soixdev91
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request implements IO-WALLET-RFC-0029 to extend credential metadata at issuance time. The PR adds the verification claim and spec_version to stored credentials, eliminating the need to re-parse verification data on each access. It also significantly simplifies analytics property computation by removing helper functions and reducing indirection.
Changes:
- Added
spec_versionandverificationfields toStoredCredentialtype and included them at credential issuance - Refactored
isItwCredentialto use the pre-extractedverificationfield instead of parsing it each time - Simplified analytics property building by removing
getWalletStatusandgetPIDMixpanelStatushelper functions in favor of direct selector usage - Removed the
useItwFeaturesEnabledhook and replaced all usages with direct calls toitwLifecycleIsITWalletValidSelector - Updated theme hooks (
useThemeColorByCredentialType,useHeaderPropsByCredentialType) to derive L3 state internally
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/features/itwallet/common/utils/itwTypesUtils.ts | Added Verification type alias and new spec_version and verification fields to StoredCredential |
| ts/features/itwallet/common/utils/itwCredentialUtils.ts | Added extractVerification utility and refactored isItwCredential to use pre-extracted verification field |
| ts/features/itwallet/common/utils/itwIssuanceUtils.ts | Updated PID issuance to include spec_version and verification in stored credentials |
| ts/features/itwallet/common/utils/itwCredentialIssuanceUtils.ts | Updated general credential issuance to include spec_version and verification |
| ts/features/itwallet/common/utils/constants.ts | Added WALLET_SPEC_VERSION constant (placeholder for future dynamic retrieval) |
| ts/features/itwallet/common/utils/itwStyleUtils.ts | Refactored hooks to derive L3 state internally instead of accepting it as a parameter |
| ts/features/itwallet/common/hooks/useItwFeaturesEnabled.ts | Deleted the hook as it's been replaced by direct selector usage |
| ts/features/itwallet/credentials/store/reducers/migrations.ts | Added migration version 7 to populate spec_version and verification on existing credentials |
| ts/features/itwallet/credentials/store/reducers/tests/migrations.test.ts | Added comprehensive tests for migration version 7 including error handling scenarios |
| ts/features/itwallet/analytics/properties/basePropertyBuilder.ts | Simplified property building by removing helper functions and outdated L3 credential check |
| ts/features/itwallet/analytics/properties/propertyUpdaters.ts | Updated to use direct selector calls and simplified status mapping |
| ts/features/itwallet/analytics/utils/index.ts | Updated mapPIDStatusToMixpanel to handle undefined status |
| ts/features/itwallet/presentation/details/* | Replaced useItwFeaturesEnabled hook calls with direct selector usage |
Comments suppressed due to low confidence (1)
ts/features/itwallet/analytics/properties/basePropertyBuilder.ts:84
- The JSDoc comment mentions "If
isItwL3is true and the credential exists but is not an ITW credential, returnsnot_available" but this check was removed from the implementation (lines 94-97 were deleted in the diff). The comment should be updated to reflect the actual behavior, which no longer validates whether a credential is an ITW credential.
/**
* Returns the Mixpanel status for a credential type, considering IT Wallet.
* - If `isItwL3` is explicitly false, returns `"not_available"`.
* - If `isItwL3` is true and the credential exists but is not an ITW credential, returns `"not_available"`.
* - Otherwise, retrieves the credential from the store and maps it to Mixpanel status.
* - Returns `"not_available"` if the credential is missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ts/features/itwallet/credentials/store/reducers/__tests__/migrations.test.ts
Show resolved
Hide resolved
ts/features/itwallet/credentials/store/reducers/__tests__/migrations.test.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7838 +/- ##
==========================================
- Coverage 60.55% 60.51% -0.04%
==========================================
Files 1934 1947 +13
Lines 42667 42999 +332
Branches 9861 9938 +77
==========================================
+ Hits 25838 26022 +184
- Misses 16752 16888 +136
- Partials 77 89 +12
... and 101 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| const pidStatus = itwCredentialsEidStatusSelector(state); | ||
|
|
||
| const ITW_STATUS_V2 = getWalletStatus(state); | ||
| const ITW_PID = getPIDMixpanelStatus(state, true); |
There was a problem hiding this comment.
When only eID is available, ITW_ID_V2 should be mapped, but ITW_PID must stay not_available. After upgrade, V2 props must stop being sent to preserve their last value. getPIDMixpanelStatus was used for that.
There was a problem hiding this comment.
I've updated the functions. Could you check and verify if they are ok?
There was a problem hiding this comment.
Now it works correctly! 💪🏻
| * @param credential - The stored credential fields needed to extract verification | ||
| * @returns The verification object or undefined if extraction fails | ||
| */ | ||
| export const extractVerification = ({ |
There was a problem hiding this comment.
The verification claim is going to be simplified in v1.3.3: apparently it will only include trust_framework and assurance_level. Since we are persisting this data now, would it be more future proof to ignore evidence?
There was a problem hiding this comment.
Do you mean by manually removing the evidence claim during the extraction? Do you think ignoring it now might cause problems later?
There was a problem hiding this comment.
Yes, I would remove evidence: we've never used it and it will be removed soon. I think it's better to consider it as not available since now, so we don't use it and won't need to make it optional in the future.
There was a problem hiding this comment.
I've dropped evidence from the verification object (5755158).
We just need to make sure to remove this step once we will be on 1.3.3
3 participants
Short description
This PR implements IO-WALLET-RFC-0029 by extending the metadata stored with credentials at the time of issuance.
It adds the parsed verification claim directly to the
StoredCredentialobject during credential issuance, eliminating the need to retrieve and re-parse it on subsequent accesses.Additionally, it introduces the
WALLET_SPEC_VERSIONconstant to track which wallet specification version was used to obtain each credential. This constant is stored in the StoredCredential object at issuance time and will later be replaced by a value directly obtained from theio-react-native-walletpackage for better maintainability.List of changes proposed in this pull request
This pull request refactors and improves the IT Wallet analytics and credential handling code. The main focus is on simplifying property computation for analytics, improving type safety and consistency, and enhancing credential data structures to support future features. It also removes unused or redundant code and introduces a wallet specification versioning system.
Analytics Property Refactoring and Simplification:
getWalletStatusandgetPIDMixpanelStatushelper functions, replacing them with direct selector usage and a unifiedmapPIDStatusToMixpanelfunction for status mapping. This reduces indirection and improves readability. [1] [2] [3] [4]Credential Data Structure Enhancements:
spec_versionfield and averificationfield to theStoredCredentialtype, ensuring credentials now carry wallet specification versioning and a pre-extracted verification object for easier downstream checks. [1] [2] [3]Verificationtype alias for better type safety and clarity when handling verification objects.Credential Verification Utilities:
isItwCredentialutility to use the newverificationfield on credentials, simplifying the logic and improving reliability. Added a newextractVerificationutility to consistently extract verification data from credentials based on their format.extractVerificationutility and to includespec_version. [1] [2] [3]Theme and UI Hook Improvements:
useThemeColorByCredentialType,useHeaderPropsByCredentialType) to always derive L3 design state from the Redux selector, removing the need to pass this as a parameter, which simplifies their usage and reduces potential for errors. [1] [2]useItwFeaturesEnabledhook, cleaning up the codebase.Constants and Versioning:
WALLET_SPEC_VERSIONconstant to track the current wallet specification version, with a placeholder for future dynamic retrieval.These changes modernize and streamline how IT Wallet analytics and credential data are handled, setting the stage for more robust analytics and easier future enhancements.
How to test
masterbranch, obtain a PID and at least one credential