Improve WalletConfig and VPTokenSigningResult / UnsignedVPToken#124
Open
KiruthikaJeyashankar wants to merge 8 commits into
Open
Improve WalletConfig and VPTokenSigningResult / UnsignedVPToken#124KiruthikaJeyashankar wants to merge 8 commits into
KiruthikaJeyashankar wants to merge 8 commits into
Conversation
1. rename validatePreRegisteredVerifier -> validateTrustedVerifier
2. remove supportedRequestUriMethods
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
- The id will be the linkability factor between data to be signed and signed data which is used for VP construction Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
|
Warning Review limit reached
More reviews will be available in 46 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Walkthrough
ChangesWalletConfig API Cleanup
VP Token Identifier Threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KiruthikaJeyashankar
force-pushed
the
lsh-feedback-1.0
branch
from
99ba84a to
3bbb372
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: 6
🧹 Nitpick comments (2)
Tests/OpenID4VPTests/TestData/TestUtils.swift (1)
296-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the helper argument to match the updated API terminology.
Keeping
validatePreregisteredVerifierin this shared test utility makes the migration look partial and can mislead future test updates.♻️ Suggested patch
- validatePreregisteredVerifier: Bool = true + validateTrustedVerifier: Bool = true ) -> WalletConfig { return WalletConfig( @@ - validateTrustedVerifier: validatePreregisteredVerifier + validateTrustedVerifier: validateTrustedVerifier ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/OpenID4VPTests/TestData/TestUtils.swift` around lines 296 - 306, Rename the function parameter `validatePreregisteredVerifier` to `validateTrustedVerifier` to align with the updated API terminology reflected in the WalletConfig field name. This change should be made in the function signature where the parameter is declared with its default value, and then use the renamed parameter in the subsequent WalletConfig initialization where it is assigned to the validateTrustedVerifier property.Tests/OpenID4VPTests/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestTests.swift (1)
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
wallet_nonceexplicit in the request-object payload setup.This path now validates POST semantics; explicitly injecting
wallet_noncehere avoids hidden fixture coupling and makes the test intent clearer.♻️ Suggested patch
- let requestUriResponse = createRequestUriResponse(createAuthorizationRequestObject(clientIdPrefix: .decentralizedIdentifier, authorizationRequestParams: mergeMaps(authorizationRequestParamsWithValue, DidSchemeClientIdParameters[.v1]!), applicableFields: authRequestWithDidByValue + [AuthorizationRequestFieldConstants.walletNonce], specVersion: .draft23) ) + let requestUriResponse = createRequestUriResponse( + createAuthorizationRequestObject( + clientIdPrefix: .decentralizedIdentifier, + authorizationRequestParams: mergeMaps( + authorizationRequestParamsWithValue, + DidSchemeClientIdParameters[.v1]!, + [AuthorizationRequestFieldConstants.walletNonce: "mock-nonce"] + ), + applicableFields: authRequestWithDidByValue + [AuthorizationRequestFieldConstants.walletNonce], + specVersion: .draft23 + ) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/OpenID4VPTests/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestTests.swift` at line 91, The test is implicitly relying on fixture coupling for wallet_nonce instead of explicitly injecting it into the request object. Modify the createAuthorizationRequestObject call to explicitly pass wallet_nonce as a parameter value in the authorizationRequestParams (using mergeMaps to include it) rather than only specifying it in the applicableFields array, so that the test clearly shows the actual wallet_nonce value being used and makes the POST semantics validation intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Sources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/types/Mdoc/UnsignedMdocVPTokenBuilder.swift`:
- Around line 36-49: The existingDocTypes set is no longer needed because the
payload is now keyed by per-credential identifier rather than docType, so
duplicate docTypes across credentials no longer cause collisions. Remove the
existingDocTypes variable declaration and eliminate the existingDocTypes
parameter from all calls to buildPayloadAndUnsignedVPToken throughout the file.
Also remove any logic that checks or updates this set, including at the other
locations mentioned (lines around 66-78 and 115-123).
In `@Sources/OpenID4VP/Common/Utils.swift`:
- Around line 254-264: The current implementation in the loop iterating over
unsignedVPTokenResults only collects matching VPTokenSigningResults for known
unsigned token identifiers, silently dropping any signing results with unmatched
ids. After the loop completes, add validation to check if all signing results
from signingResults have been accounted for by comparing the matched identifiers
against all identifiers in the original signingResults collection. If any
signing results have identifiers that were never matched to an unsigned token,
throw an error with details about which identifiers were unexpected instead of
silently ignoring them, ensuring the fail-fast behavior expected from the
caller-facing response construction path.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/AuthorizationResponseHandlerTests.swift`:
- Around line 403-412: The test verifies that a missing signing result (when
constructUnsignedVPTokens drops the last token but VPTokenSigningResult only
maps the others) causes an error, but it only checks the wrapped server error on
line 425 and does not assert the underlying root cause. Add an assertion after
the wrapped error check to verify that the underlying cause of the error
specifically relates to the missing identifier-linked signing result, ensuring
the failure path is correctly diagnosed rather than just catching any server
error.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/UnsignedVPToken/UnsignedMdocVPTokenBuilderTests.swift`:
- Around line 84-86: The test at lines 85 and 220 assumes each payload map
(draft23Payload and v1Payload) contains exactly one UUID-keyed entry but does
not assert this assumption before accessing .values.first. Add XCTAssertEqual
assertions to verify that the count property of each dictionary equals 1 before
extracting the first value, ensuring the test will fail if extra entries are
accidentally produced in the payload maps.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swift`:
- Line 30: The VPTokenSigningResult instances throughout the test file use
hardcoded id: "uuid1" values that do not match their corresponding mapping or
payload identifiers, which prevents proper testing of id-linkability behavior.
Update each VPTokenSigningResult(id:) call to use the actual identifier from its
associated mapping or payload rather than the hardcoded "uuid1" value. This
applies to all instances mentioned at lines 30, 110, 158, 211, 212, 431, 497,
544, 545, 617, 670, 702, 836, 884, and 931 where the id parameter should be
replaced with the correct corresponding identifier for that test case.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/VpTokenSigningResult/VPTokenSigningResultTests.swift`:
- Around line 8-10: The VPTokenSigningResult initialization tests are setting
the id field during initialization but not verifying it with an assertion. Add
XCTAssertEqual assertions to verify that the id parameter passed to
VPTokenSigningResult initializer is correctly stored and returned by the result
object. This verification should be added in both test methods mentioned in the
comment (around lines 8 and 22) to ensure Codable regressions on the id field
are properly caught during testing.
---
Nitpick comments:
In
`@Tests/OpenID4VPTests/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestTests.swift`:
- Line 91: The test is implicitly relying on fixture coupling for wallet_nonce
instead of explicitly injecting it into the request object. Modify the
createAuthorizationRequestObject call to explicitly pass wallet_nonce as a
parameter value in the authorizationRequestParams (using mergeMaps to include
it) rather than only specifying it in the applicableFields array, so that the
test clearly shows the actual wallet_nonce value being used and makes the POST
semantics validation intent explicit.
In `@Tests/OpenID4VPTests/TestData/TestUtils.swift`:
- Around line 296-306: Rename the function parameter
`validatePreregisteredVerifier` to `validateTrustedVerifier` to align with the
updated API terminology reflected in the WalletConfig field name. This change
should be made in the function signature where the parameter is declared with
its default value, and then use the renamed parameter in the subsequent
WalletConfig initialization where it is assigned to the validateTrustedVerifier
property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62a9e0af-b5fa-48ab-878d-9880db130778
📒 Files selected for processing (32)
Package.swiftSources/OpenID4VP/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestHandler.swiftSources/OpenID4VP/AuthorizationRequest/AuthorizationRequestHandler/types/PreRegisteredPrefixAuthorizationRequestHandler.swiftSources/OpenID4VP/AuthorizationRequest/Utils/AuthorizationRequestUtils.swiftSources/OpenID4VP/AuthorizationResponse/AuthorizationResponseHandler.swiftSources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/UnsignedVPToken.swiftSources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/UnsignedVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/types/LdpVP/UnsignedLdpVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/types/Mdoc/UnsignedMdocVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/UnsignedVPToken/types/SdJwt/UnsignedSdJwtVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/VPTokenBuilderUtils.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/types/LdpVp/LdpVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/types/Mdoc/MdocVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPTokenSigningResult/VpTokenSigningResult.swiftSources/OpenID4VP/Common/Utils.swiftSources/OpenID4VP/Constants/OpenId4VPSpecConstants/WalletMetadataConstants.swiftSources/OpenID4VP/Constants/RequestUriMethod.swiftSources/OpenID4VP/NetworkManager/NetworkManager.swiftSources/OpenID4VP/Wallet/WalletConfig.swiftTests/OpenID4VPTests/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestTests.swiftTests/OpenID4VPTests/AuthorizationRequest/AuthorizationRequestTests.swiftTests/OpenID4VPTests/AuthorizationRequest/Utils/AuthorizationRequestUtilsTests.swiftTests/OpenID4VPTests/AuthorizationResponse/AuthorizationResponseHandlerTests.swiftTests/OpenID4VPTests/AuthorizationResponse/UnsignedVPToken/UnsignedMdocVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/Mdoc/MdocVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VpTokenSigningResult/VPTokenSigningResultTests.swiftTests/OpenID4VPTests/OpenID4VPTests.swiftTests/OpenID4VPTests/TestData/TestData.swiftTests/OpenID4VPTests/TestData/TestUtils.swiftTests/OpenID4VPTests/Wallet/WalletConfigTests.swift
💤 Files with no reviewable changes (2)
- Sources/OpenID4VP/Constants/OpenId4VPSpecConstants/WalletMetadataConstants.swift
- Sources/OpenID4VP/AuthorizationRequest/AuthorizationRequestHandler/ClientIdPrefixBasedAuthorizationRequestHandler.swift
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
Contributor
Author
|
@coderabbitai Great work identifying the functional correctness improvements through stricter assertions. Please add the following to your learning points as well: ensure that unit tests provide a robust safety net. Any meaningful change to business or coding logic should result in a failing test, whereas pure refactoring should not require test changes beyond necessary mock or test setup updates. Tests should focus on validating behavior and outcomes, giving confidence that unintended logic changes are detected early. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ ✏️ Learnings added
|
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
Contributor
Author
|
@coderabbitai approve if all set |
|
✅ Action performedComments resolved. Approval is disabled; enable |
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift (1)
408-422: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename this test to match the now-throwing behavior.
The test name says “DoesNotThrow” but it now expects an error; this is misleading when triaging failures.
Suggested rename
- func testDcqlBuildWithEmptySignedDataDoesNotThrow() throws { + func testDcqlBuildThrowsWhenSignedDataIsEmpty() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift` around lines 408 - 422, Rename the test method `testDcqlBuildWithEmptySignedDataDoesNotThrow` to reflect that `dcqlBuilder.build(...)` now throws when given empty `signedData`; keep the existing `XCTAssertThrowsError` assertion and use a name that describes the invalid-signature failure path for `uuid1` so failures are easier to triage.Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swift (1)
354-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPayload-type negative tests are currently exercising different failure paths.
testBuildThrowsWhenPayloadIsNotStringDictionaryuses a string dictionary, and the DCQL variant also mixes identifier keys (uuid1vsuuid-1). That weakens payload-type regression coverage.Tighten fixtures to isolate payload-type validation
- vpTokenSigningPayload: ["uuid-1":"invalid-payload"], + vpTokenSigningPayload: ["uuid-1": 1], ... - vpTokenSigningResults: [], + vpTokenSigningResults: [VPTokenSigningResult(id: "uuid-1", signedData: Data("sig".utf8))],- vpTokenSigningPayload: ["uuid1": 1], + vpTokenSigningPayload: ["uuid-1": 1],Also applies to: 700-721
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swift` around lines 354 - 380, The negative payload-type tests are not isolating the intended validation path because `testBuildThrowsWhenPayloadIsNotStringDictionary` currently uses a string dictionary and the related DCQL test mixes identifier keys like `uuid1` and `uuid-1`. Update the fixtures in `SdJwtVPTokenBuilderTests` so the payload-type failure is triggered by a non-string-dictionary payload shape, and keep the credential identifier keys consistent between `CredentialInputDescriptorMapping` and `VPTokenSigningPayload` in both the `build` test and the DCQL variant to ensure the assertion matches the actual payload-type error path.
🧹 Nitpick comments (3)
Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift (1)
170-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an out-of-order signing-results variant to lock in id-based matching.
This path still feeds signing results in mapping order; reversing them here would better enforce the PR’s order-independence contract.
Minimal hardening example
vpTokenSigningResults: [ - VPTokenSigningResult(id: "uuid1", signedData: Data("mockSig1".utf8)), - VPTokenSigningResult(id: "uuid2", signedData: Data("mockSig2".utf8)) + VPTokenSigningResult(id: "uuid2", signedData: Data("mockSig2".utf8)), + VPTokenSigningResult(id: "uuid1", signedData: Data("mockSig1".utf8)) ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift` around lines 170 - 176, Add a test case in LdpVPTokenBuilderTests that passes VPTokenSigningResult values in a different order than the credentialInputDescriptorMappings to verify build matches results by id rather than array position. Update the existing LdpVPTokenBuilder build invocation to use reversed signing-results order (or add a dedicated variant) and assert the produced token still maps uuid1 and uuid2 to the correct signedData. Use the builder.build and VPTokenSigningResult symbols to locate the test and keep the assertion focused on id-based matching.Sources/OpenID4VP/AuthorizationResponse/AuthorizationResponseHandler.swift (1)
293-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThread the unsigned-token map through
getData.Line 421 reads
self.unsignedVPTokenResultseven though both callers already receive anunsignedVPTokenResultsargument. Passing the map into the helper keeps the lookup pure and avoids accidental stale handler-state coupling.♻️ Proposed refactor
- let (vpTokenSigningResultsForFormat, unsignedVPTokenResult) = try getData(vpTokenSigningResults, credentialFormat: credentialFormat) + let (vpTokenSigningResultsForFormat, unsignedVPTokenResult) = try getData( + vpTokenSigningResults, + unsignedVPTokenResults: unsignedVPTokenResults, + credentialFormat: credentialFormat + )- let (vpTokenSigningResultsForFormat, unsignedVPTokenResult) = try getData(vpTokenSigningResults, credentialFormat: credentialFormat) + let (vpTokenSigningResultsForFormat, unsignedVPTokenResult) = try getData( + vpTokenSigningResults, + unsignedVPTokenResults: unsignedVPTokenResults, + credentialFormat: credentialFormat + )- private func getData(_ vpTokenSigningResults: [FormatType: [VPTokenSigningResult]], credentialFormat: FormatType) throws -> (vpTokenSigningResultsForFormat : [VPTokenSigningResult], unsignedVPTokenResult: (vpTokenSigningPayload: VPTokenSigningPayload, unsignedVPTokens: [UnsignedVPToken])) { + private func getData( + _ vpTokenSigningResults: [FormatType: [VPTokenSigningResult]], + unsignedVPTokenResults: [FormatType: (vpTokenSigningPayload: VPTokenSigningPayload, unsignedVPTokens: [UnsignedVPToken])], + credentialFormat: FormatType + ) throws -> (vpTokenSigningResultsForFormat: [VPTokenSigningResult], unsignedVPTokenResult: (vpTokenSigningPayload: VPTokenSigningPayload, unsignedVPTokens: [UnsignedVPToken])) { let vpTokenSigningResultsForFormat = vpTokenSigningResults[credentialFormat] ?? [] guard let unsignedVPTokenResult = unsignedVPTokenResults[credentialFormat] else {Also applies to: 330-330, 421-430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OpenID4VP/AuthorizationResponse/AuthorizationResponseHandler.swift` at line 293, Thread the unsigned-token map through `getData` instead of reading handler state inside it. Update `AuthorizationResponseHandler.getData` and its callers so they pass the existing `unsignedVPTokenResults` argument through, and replace the `self.unsignedVPTokenResults` lookup in that helper with the passed-in map. Keep the logic in `AuthorizationResponseHandler` and `getData` pure by using the per-call data already available at the call sites.Sources/OpenID4VP/AuthorizationResponse/VPToken/types/SdJwt/SdJwtVPTokenBuilder.swift (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFinish the UUID→identifier cleanup in this path.
The flow now uses
identifier, butuuidToUnsignedKBT,extractUuidToUnsignedKBT, and the error messages still sayuuid, which makes id-mismatch diagnostics misleading.♻️ Proposed cleanup
- let uuidToUnsignedKBT = try extractUuidToUnsignedKBT(from: unsignedVPTokenResult) + let identifierToUnsignedKBT = try extractIdentifierToUnsignedKBT(from: unsignedVPTokenResult) ... - let unsignedKBJwt = uuidToUnsignedKBT[identifier] + let unsignedKBJwt = identifierToUnsignedKBT[identifier] ... - let uuidToUnsignedKBT = try extractUuidToUnsignedKBT(from: unsignedVPTokenResult) + let identifierToUnsignedKBT = try extractIdentifierToUnsignedKBT(from: unsignedVPTokenResult) ... - let unsignedKBJwt = uuidToUnsignedKBT[identifier] + let unsignedKBJwt = identifierToUnsignedKBT[identifier] ... - throw InvalidData(message: "Missing Key Binding JWT for uuid: \(identifier)", className: className) + throw InvalidData(message: "Missing Key Binding JWT for identifier: \(identifier)", className: className) ... - throw InvalidData(message: "Unexpected key binding jwt for uuid: \(identifier)", className: className) + throw InvalidData(message: "Unexpected key binding jwt for identifier: \(identifier)", className: className)- private func extractUuidToUnsignedKBT( - from unsignedVPTokenResult: (vpTokenSigningPayload: Any?, unsignedVPTokens: [UnsignedVPToken]) + private func extractIdentifierToUnsignedKBT( + from unsignedVPTokenResult: (vpTokenSigningPayload: VPTokenSigningPayload, unsignedVPTokens: [UnsignedVPToken]) ) throws -> [String: String] { - guard let uuidToUnsignedKBT = unsignedVPTokenResult.vpTokenSigningPayload as? [String: String] else { - throw InvalidData(message: "Missing uuidToUnsignedKBT in payload", className: className) + guard let identifierToUnsignedKBT = unsignedVPTokenResult.vpTokenSigningPayload as? [String: String] else { + throw InvalidData(message: "Missing identifierToUnsignedKBT in payload", className: className) } - return uuidToUnsignedKBT + return identifierToUnsignedKBT }private func extractIdentifier(from identifier: String?) throws -> String { - guard let uuid = identifier else { + guard let identifier else { throw InvalidData(message: "identifier is null in CredentialInputDescriptorMapping for SD-JWT", className: className) } - return uuid + return identifier }Also applies to: 53-53, 65-75, 95-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/OpenID4VP/AuthorizationResponse/VPToken/types/SdJwt/SdJwtVPTokenBuilder.swift` at line 19, Finish the UUID-to-identifier cleanup in SdJwtVPTokenBuilder by renaming the remaining UUID-based symbols and messages to match the new identifier terminology. Update the local variable and helper names around unsigned VP token extraction, including the call to extractUuidToUnsignedKBT and any related error text, so the code consistently uses identifier rather than uuid. Also review the affected builder path and its diagnostics to ensure all mismatch errors mention identifier-based terms and no stale uuid wording remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift`:
- Around line 408-422: Rename the test method
`testDcqlBuildWithEmptySignedDataDoesNotThrow` to reflect that
`dcqlBuilder.build(...)` now throws when given empty `signedData`; keep the
existing `XCTAssertThrowsError` assertion and use a name that describes the
invalid-signature failure path for `uuid1` so failures are easier to triage.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swift`:
- Around line 354-380: The negative payload-type tests are not isolating the
intended validation path because
`testBuildThrowsWhenPayloadIsNotStringDictionary` currently uses a string
dictionary and the related DCQL test mixes identifier keys like `uuid1` and
`uuid-1`. Update the fixtures in `SdJwtVPTokenBuilderTests` so the payload-type
failure is triggered by a non-string-dictionary payload shape, and keep the
credential identifier keys consistent between `CredentialInputDescriptorMapping`
and `VPTokenSigningPayload` in both the `build` test and the DCQL variant to
ensure the assertion matches the actual payload-type error path.
---
Nitpick comments:
In `@Sources/OpenID4VP/AuthorizationResponse/AuthorizationResponseHandler.swift`:
- Line 293: Thread the unsigned-token map through `getData` instead of reading
handler state inside it. Update `AuthorizationResponseHandler.getData` and its
callers so they pass the existing `unsignedVPTokenResults` argument through, and
replace the `self.unsignedVPTokenResults` lookup in that helper with the
passed-in map. Keep the logic in `AuthorizationResponseHandler` and `getData`
pure by using the per-call data already available at the call sites.
In
`@Sources/OpenID4VP/AuthorizationResponse/VPToken/types/SdJwt/SdJwtVPTokenBuilder.swift`:
- Line 19: Finish the UUID-to-identifier cleanup in SdJwtVPTokenBuilder by
renaming the remaining UUID-based symbols and messages to match the new
identifier terminology. Update the local variable and helper names around
unsigned VP token extraction, including the call to extractUuidToUnsignedKBT and
any related error text, so the code consistently uses identifier rather than
uuid. Also review the affected builder path and its diagnostics to ensure all
mismatch errors mention identifier-based terms and no stale uuid wording
remains.
In
`@Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swift`:
- Around line 170-176: Add a test case in LdpVPTokenBuilderTests that passes
VPTokenSigningResult values in a different order than the
credentialInputDescriptorMappings to verify build matches results by id rather
than array position. Update the existing LdpVPTokenBuilder build invocation to
use reversed signing-results order (or add a dedicated variant) and assert the
produced token still maps uuid1 and uuid2 to the correct signedData. Use the
builder.build and VPTokenSigningResult symbols to locate the test and keep the
assertion focused on id-based matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b94d0621-a1cb-46e1-a20d-6d6e0e795c40
📒 Files selected for processing (11)
Sources/OpenID4VP/AuthorizationResponse/AuthorizationResponseHandler.swiftSources/OpenID4VP/AuthorizationResponse/CredentialPresentationRequestMapping/CredentialToCredentialQueryIdMapping.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/VPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/VPTokenBuilderUtils.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/types/LdpVp/LdpVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/types/Mdoc/MdocVPTokenBuilder.swiftSources/OpenID4VP/AuthorizationResponse/VPToken/types/SdJwt/SdJwtVPTokenBuilder.swiftSources/OpenID4VP/Common/Utils.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/LdpVp/LdpVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/Mdoc/MdocVPTokenBuilderTests.swiftTests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/SdJwt/SdJwtVPTokenBuilderTests.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/OpenID4VP/AuthorizationResponse/VPToken/VPTokenBuilderUtils.swift
- Sources/OpenID4VP/Common/Utils.swift
- Sources/OpenID4VP/AuthorizationResponse/VPToken/types/LdpVp/LdpVPTokenBuilder.swift
- Sources/OpenID4VP/AuthorizationResponse/VPToken/types/Mdoc/MdocVPTokenBuilder.swift
- Tests/OpenID4VPTests/AuthorizationResponse/VPToken/Types/Mdoc/MdocVPTokenBuilderTests.swift
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
KiruthikaJeyashankar
force-pushed
the
lsh-feedback-1.0
branch
from
a44fee3 to
288a10c
Compare
…ken / signing result Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wallet Config updates
Rename validatePreRegisteredVerifier → validateTrustedVerifier aligning with the trustedVerifiers convention
Remove supportedRequestUriMethods config
VPTokenSigningResult / UnsignedVPToken Updates
Include id in the VPTokenSigningResult / UnsignedVPToken data as random UUID
i. This id stays as the linkability factor for UnsignedVPToken and VPTokenSigningResult for building VP
ii. Ordering of UnsignedVPToken and VPTokenSigningResult does not matter, maintenance of order MAY add complexity to consumer considering that the the linkability factor as ID is introduced
Relates: inji/inji-wallet#2499
Summary by CodeRabbit
New Features
Bug Fixes
.postmethods are no longer silently downgraded.Breaking Changes