feat: notice URL support for issuance#1593
Merged
Merged
Conversation
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds an optional Changes
sequenceDiagram
participant Client as Client
participant API as API Gateway Service
participant Agent as Agent
participant DB as Database
Client->>API: POST createOidcCredentialOffer (may include noticeUrl)
API->>Agent: createCredentialOfferOnAgent(request)
Agent-->>API: response (credential offer)
API->>DB: SELECT template by filterTemplateIds (if single template)
DB-->>API: template (includes noticeUrl?)
alt single template with template.noticeUrl
API->>API: attach template.noticeUrl to response
else multiple templates or no template.noticeUrl
alt request.noticeUrl present
API->>API: attach request.noticeUrl to response
end
end
API-->>Client: credential offer response (with noticeUrl when available)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
285-291: Validation inconsistency withCreateOidcCredentialOfferDto.This DTO uses
@IsString()fornoticeUrl, whileCreateOidcCredentialOfferDtoinissuer-sessions.dto.tsuses@IsUrl()for the same conceptual field. This inconsistency may lead to different validation behavior:
- Templates: accepts any string for
noticeUrl- Credential offers: requires valid URL format
If service-layer validation via
validateNoticeUrlis the authoritative check (as per the project's pattern for intent notices), consider aligning both DTOs to use@IsString(), or document why they differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts` around lines 285 - 291, The noticeUrl property in Oid4vcIssuerTemplateDto (property noticeUrl) uses `@IsString`(), while CreateOidcCredentialOfferDto (class CreateOidcCredentialOfferDto) uses `@IsUrl`(), causing inconsistent validation; pick one approach and update the DTO to match: either change noticeUrl in Oid4vcIssuerTemplateDto to use `@IsUrl`() to require a valid URL, or change CreateOidcCredentialOfferDto to `@IsString`() if validateNoticeUrl in the service is the canonical check—apply the chosen decorator to the noticeUrl properties and update any related validation/error messages to keep behavior consistent across Oid4vcIssuerTemplateDto, CreateOidcCredentialOfferDto, and the validateNoticeUrl usage.apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (1)
629-641: Redundant database fetch for single-template case.When
filterTemplateIds.length === 1, line 633 fetches the template by ID, but this template data was already retrieved at line 533 (getAllOfferTemplates). Consider reusing the existing data:♻️ Suggested optimization
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` const response = createCredentialOfferOnAgent.response as any; if (1 === filterTemplateIds.length) { - const template = await this.oid4vcIssuanceRepository.getTemplateById(filterTemplateIds[0]); + const template = getAllOfferTemplates[0]; if (template?.noticeUrl) { response.noticeUrl = template.noticeUrl; } } else if (createOidcCredentialOffer.noticeUrl) { response.noticeUrl = createOidcCredentialOffer.noticeUrl; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts` around lines 629 - 641, The code redundantly calls this.oid4vcIssuanceRepository.getTemplateById when filterTemplateIds.length === 1; instead reuse the template object already retrieved earlier by the getAllOfferTemplates call (the collection variable populated around the prior call) to obtain its noticeUrl and assign it to response.noticeUrl; remove the extra getTemplateById call and fall back to createOidcCredentialOffer.noticeUrl only if the previously fetched template is missing or has no noticeUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts`:
- Line 25: The updateTemplate service currently omits the noticeUrl field so
updates and rollback recovery lose that value; modify updateTemplate to extract
noticeUrl from the incoming UpdateCredentialTemplate payload (same way
createTemplate does) and include noticeUrl in the update payload sent to the
DB/ORM and in the rollback payload used on failure (ensure the unique symbols
updateTemplate, createTemplate, and noticeUrl are used to locate and mirror the
createTemplate handling).
---
Nitpick comments:
In `@apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts`:
- Around line 285-291: The noticeUrl property in Oid4vcIssuerTemplateDto
(property noticeUrl) uses `@IsString`(), while CreateOidcCredentialOfferDto (class
CreateOidcCredentialOfferDto) uses `@IsUrl`(), causing inconsistent validation;
pick one approach and update the DTO to match: either change noticeUrl in
Oid4vcIssuerTemplateDto to use `@IsUrl`() to require a valid URL, or change
CreateOidcCredentialOfferDto to `@IsString`() if validateNoticeUrl in the service
is the canonical check—apply the chosen decorator to the noticeUrl properties
and update any related validation/error messages to keep behavior consistent
across Oid4vcIssuerTemplateDto, CreateOidcCredentialOfferDto, and the
validateNoticeUrl usage.
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 629-641: The code redundantly calls
this.oid4vcIssuanceRepository.getTemplateById when filterTemplateIds.length ===
1; instead reuse the template object already retrieved earlier by the
getAllOfferTemplates call (the collection variable populated around the prior
call) to obtain its noticeUrl and assign it to response.noticeUrl; remove the
extra getTemplateById call and fall back to createOidcCredentialOffer.noticeUrl
only if the previously fetched template is missing or has no noticeUrl.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e61d41-352d-4672-ad68-1f2bb481e90f
📒 Files selected for processing (7)
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsapps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.tsapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tsapps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.tsapps/oid4vc-issuance/src/oid4vc-issuance.service.tslibs/prisma-service/prisma/migrations/20260326140323_add_notice_url_to_credential_templates/migration.sqllibs/prisma-service/prisma/schema.prisma
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
shitrerohit
approved these changes
Mar 30, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (1)
633-635: Avoid redundant template lookup in single-template path.Line 634 re-queries template by ID even though
getAllOfferTemplatesalready contains it. Reusing in-memory data removes an extra DB call and avoids TOCTOU drift.♻️ Suggested simplification
- if (1 === filterTemplateIds.length) { - const template = await this.oid4vcIssuanceRepository.getTemplateById(filterTemplateIds[0]); + if (1 === filterTemplateIds.length) { + const template = getAllOfferTemplates[0]; if (template?.noticeUrl) { response.noticeUrl = template.noticeUrl; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts` around lines 633 - 635, When handling the single-template branch (when filterTemplateIds.length === 1) avoid re-querying the DB via this.oid4vcIssuanceRepository.getTemplateById; instead locate the template in the in-memory list returned earlier by getAllOfferTemplates (find the item whose id matches filterTemplateIds[0]) and use that template (e.g., template?.noticeUrl) — remove the redundant repository call to prevent an extra DB hit and TOCTOU drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 408-409: The rollback is incomplete because the update payload
includes optional fields signerOption and noticeUrl (added via ...(signerOption
!== undefined ? { signerOption } : {}) and ...(noticeUrl !== undefined ? {
noticeUrl } : {})) but the rollback payload built later does not restore those
fields if agent sync fails; update the rollback construction to include
restoring signerOption and noticeUrl when they were present in the original
update (mirror the same conditional spread logic into the rollback payload),
referencing the same variables signerOption and noticeUrl used in the update so
the DB is fully reverted on failure.
- Around line 630-639: Before assigning to response.noticeUrl in the create
credential flow, add the same defensive type-guard used elsewhere: check the
createCredentialOfferOnAgent.response value (the local variable response) and if
it's a string, parse it into an object (e.g., response = JSON.parse(response))
or otherwise ensure it's an object before mutating; then proceed to set
response.noticeUrl using the existing branches that reference
createCredentialOfferOnAgent.response, createOidcCredentialOffer, and the
template from oid4vcIssuanceRepository.getTemplateById.
---
Nitpick comments:
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 633-635: When handling the single-template branch (when
filterTemplateIds.length === 1) avoid re-querying the DB via
this.oid4vcIssuanceRepository.getTemplateById; instead locate the template in
the in-memory list returned earlier by getAllOfferTemplates (find the item whose
id matches filterTemplateIds[0]) and use that template (e.g.,
template?.noticeUrl) — remove the redundant repository call to prevent an extra
DB hit and TOCTOU drift.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20ff18a5-4e3c-4b3a-b448-77388d72aa9b
📒 Files selected for processing (1)
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.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
2 participants
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.
Summary by CodeRabbit