Open
Conversation
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
sagarkhole4
requested review from
RinkalBhojani,
shitrerohit,
sujitaw and
tipusinghaw
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds revocable SD‑JWT support: new STATUS_LIST_HOST env var and startup check, DB models/migration for status-list allocations and issued credentials, bitmap-based allocator service, allocation/persistence/release integrated into issuance flows, NATS + HTTP revoke handlers across API Gateway, OID4VC service, and agent-service. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant GW as API_Gateway
participant OID as OID4VC_Service
participant Alloc as StatusListAllocator
participant DB as Database
participant Agent as Agent_Service
Client->>GW: POST /create (isRevocable, SD‑JWT)
GW->>OID: createOidcCredentialOffer(...)
OID->>Alloc: allocate(orgId, issuerDid)
Alloc->>DB: find/create active allocation, update bitmap
Alloc-->>OID: {listId, index}
OID->>Agent: create credential offer (agent)
Agent-->>OID: issuanceSessionId
OID->>Alloc: saveCredentialAllocation(issuanceSessionId, listId, index)
Alloc->>DB: insert issued_oid4vc_credentials
OID-->>GW: credential offer + statusListDetails
GW-->>Client: 200 OK
sequenceDiagram
participant Client as Client
participant GW as API_Gateway
participant OID as OID4VC_Service
participant Alloc as StatusListAllocator
participant DB as Database
participant Agent as Agent_Service
Client->>GW: POST /revoke (issuanceSessionId)
GW->>OID: revokeCredential(issuanceSessionId)
OID->>Alloc: getCredentialAllocations(orgId, issuanceSessionId)
Alloc->>DB: fetch issued_oid4vc_credentials
Alloc-->>OID: [{listId,index,statusListUri}]
loop per allocation
OID->>Agent: oidcRevokeCredential({ url: statusListUri, orgId })
Agent->>Agent: resolve org agent API key
Agent->>Agent: POST {} to url with authorization header
Agent-->>OID: revoke response
OID->>Alloc: release(listId,index)
Alloc->>DB: update bitmap, decrement allocatedCount
end
OID-->>GW: revoked result
GW-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (4)
apps/oid4vc-issuance/src/status-list-allocator.service.ts (4)
130-133: PotentialallocatedCountdrift between DB and bitmap.The DB update uses
activeList.allocatedCount + 1(the stale value read at transaction start), but the actual allocated count may differ if the bitmap already had bits set from prior runs. Consider using the allocator's internally computed count for consistency.Suggested fix
Expose the internal count from the allocator:
// In RandomBitmapIndexAllocator public getAllocatedCount(): number { return this.allocatedCount; }Then use it during the update:
await tx.status_list_allocation.update({ where: { id: activeList.id }, data: { bitmap: Buffer.from(allocator.export()), - allocatedCount: activeList.allocatedCount + 1 + allocatedCount: allocator.getAllocatedCount() } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` around lines 130 - 133, The DB update uses activeList.allocatedCount + 1 which can drift from the allocator's real state; add an accessor on RandomBitmapIndexAllocator (e.g., getAllocatedCount() or expose allocatedCount) and use allocator.getAllocatedCount() when writing the updated data (bitmap and allocatedCount) instead of activeList.allocatedCount + 1 so the saved allocatedCount matches the bitmap exported by allocator.
6-6: Prefernode:cryptomodule specifier.Using the
node:prefix makes it explicit this is a Node.js built-in module and avoids potential conflicts with npm packages of the same name.Suggested fix
-import { randomInt, randomUUID } from 'crypto'; +import { randomInt, randomUUID } from 'node:crypto';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` at line 6, The import currently pulls randomInt and randomUUID from 'crypto'; update the module specifier to use the Node built-in form 'node:crypto' (i.e., change the import that references randomInt and randomUUID to import from 'node:crypto') so it explicitly uses the Node.js core module and avoids name conflicts with similarly named npm packages.
137-147: Fragile error detection via string comparison.Checking
error.message === 'No indexes left'is brittle—message changes or localization would break this logic. Consider using a custom error class for reliable identification.Suggested approach
Define a custom error at the top of the file:
export class NoIndexesLeftError extends Error { constructor() { super('No indexes left'); this.name = 'NoIndexesLeftError'; } }Then throw and catch it explicitly:
- throw new Error('No indexes left'); + throw new NoIndexesLeftError();- if ('No indexes left' === error.message) { + if (error instanceof NoIndexesLeftError) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` around lines 137 - 147, The catch block is using a brittle string comparison on error.message to detect the "No indexes left" case; define and export a custom error class (e.g., NoIndexesLeftError) and throw that from the allocation logic instead of a plain Error, then update the catch to use "if (error instanceof NoIndexesLeftError)" to reliably identify the condition; keep the retry behavior (calling tx.status_list_allocation.update with activeList.id to set isActive: false and rethrow a new Error('Retry allocation, list full') or propagate as appropriate).
9-11: Mark immutable members asreadonly.These members are initialized in the constructor and never reassigned. Marking them
readonlyimproves type safety and communicates intent.Suggested fix
- private bitmap: Uint8Array; - private capacity: number; + private readonly bitmap: Uint8Array; + private readonly capacity: number; private allocatedCount: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` around lines 9 - 11, The fields bitmap, capacity, and allocatedCount in the StatusListAllocator class are set in the constructor and never reassigned; mark them readonly to express immutability and improve type safety by changing their declarations to readonly bitmap: Uint8Array, readonly capacity: number, and readonly allocatedCount: number (ensure there are no later assignments to these properties elsewhere such as in methods of StatusListAllocator before making them readonly).
🤖 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/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts`:
- Around line 191-193: The isRevocable property in both
CreateOidcCredentialOfferDto and CreateCredentialOfferD2ADto lacks a boolean
validator, allowing string "true"/"false" to pass and cause incorrect truthy
checks; add `@IsBoolean`() immediately after `@IsOptional`() on the isRevocable
field in both DTO classes and update the imports to include IsBoolean from
class-validator so the validation enforces a real boolean type before
service-layer checks.
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 746-767: The code currently swallows allocation errors for
revocable SD-JWT credentials (inside the oidcCredentialD2APayload.isRevocable
branch and loop over oidcCredentialD2APayload.credentials) and simply logs via
this.logger.warn, which can produce non-revocable offers; change the logic so
that when a credential with format CredentialFormat.SdJwtVc requires
statusListDetails and statusListAllocatorService.allocate(orgId,
agentDetailsForAlloc.orgDid) fails you either retry the allocation a limited
number of times or propagate the failure by throwing (e.g., throw new
BadRequestException or an appropriate error) so the whole request fails; update
the loop around the allocation attempt (where allocError is caught) to implement
the retry policy or rethrow the error instead of only logging so no revocable
credential is sent without statusListDetails.
In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts`:
- Around line 44-58: The allocate() method uses an unbounded random retry loop
which degrades when the bitmap is nearly full; modify allocate() to try a
bounded number of random attempts (e.g., MAX_RANDOM_ATTEMPTS) using
randomInt(this.capacity) and isSet(idx), and if none succeed fall back to a
deterministic linear scan across the bitmap to find the first unset index, call
set(idx) and increment this.allocatedCount before returning; keep the existing
capacity check and throw when full. Use clear symbols from the file such as
allocate(), isSet(), set(), this.capacity and this.allocatedCount, and introduce
a local MAX_RANDOM_ATTEMPTS constant to control the fallback threshold.
In `@libs/prisma-service/prisma/schema.prisma`:
- Around line 653-662: Add an orgId FK to the issued_oid4vc_credentials model
and persist it when allocations are created: update the Prisma model
issued_oid4vc_credentials to include orgId (UUID) with a foreign key relation to
your org/issuer table and add a composite index on (orgId, issuanceSessionId);
update saveCredentialAllocation() to set orgId when creating rows and update
revokeCredential() to load allocations by the composite key [orgId,
issuanceSessionId] instead of issuanceSessionId alone; also mirror the schema
change by adding the orgId column, FK constraint and the composite index in
libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql
so queries can be scoped and indexed by org.
- Line 235: The org_agents model currently declares the field webhookSecret
twice; remove the duplicate declaration so only a single webhookSecret String?
`@db.VarChar` field remains in the org_agents model, ensuring the remaining
declaration has the correct name, type and attributes; then run Prisma
format/generate to verify the schema compiles and update any code that
referenced the removed duplicate if necessary.
---
Nitpick comments:
In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts`:
- Around line 130-133: The DB update uses activeList.allocatedCount + 1 which
can drift from the allocator's real state; add an accessor on
RandomBitmapIndexAllocator (e.g., getAllocatedCount() or expose allocatedCount)
and use allocator.getAllocatedCount() when writing the updated data (bitmap and
allocatedCount) instead of activeList.allocatedCount + 1 so the saved
allocatedCount matches the bitmap exported by allocator.
- Line 6: The import currently pulls randomInt and randomUUID from 'crypto';
update the module specifier to use the Node built-in form 'node:crypto' (i.e.,
change the import that references randomInt and randomUUID to import from
'node:crypto') so it explicitly uses the Node.js core module and avoids name
conflicts with similarly named npm packages.
- Around line 137-147: The catch block is using a brittle string comparison on
error.message to detect the "No indexes left" case; define and export a custom
error class (e.g., NoIndexesLeftError) and throw that from the allocation logic
instead of a plain Error, then update the catch to use "if (error instanceof
NoIndexesLeftError)" to reliably identify the condition; keep the retry behavior
(calling tx.status_list_allocation.update with activeList.id to set isActive:
false and rethrow a new Error('Retry allocation, list full') or propagate as
appropriate).
- Around line 9-11: The fields bitmap, capacity, and allocatedCount in the
StatusListAllocator class are set in the constructor and never reassigned; mark
them readonly to express immutability and improve type safety by changing their
declarations to readonly bitmap: Uint8Array, readonly capacity: number, and
readonly allocatedCount: number (ensure there are no later assignments to these
properties elsewhere such as in methods of StatusListAllocator before making
them readonly).
🪄 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: 82f7c59f-cc77-4ee0-ae03-25214d229064
📒 Files selected for processing (17)
.env.demo.env.sampleapps/agent-service/src/agent-service.controller.tsapps/agent-service/src/agent-service.service.tsapps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.tsapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tsapps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsapps/oid4vc-issuance/src/main.tsapps/oid4vc-issuance/src/oid4vc-issuance.controller.tsapps/oid4vc-issuance/src/oid4vc-issuance.module.tsapps/oid4vc-issuance/src/oid4vc-issuance.service.tsapps/oid4vc-issuance/src/status-list-allocator.service.tslibs/common/src/common.constant.tslibs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sqllibs/prisma-service/prisma/schema.prisma
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
tipusinghaw
force-pushed
the
feat/sd-jwt-revocation-flow
branch
from
380e0cc to
fe14467
Compare
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
apps/oid4vc-issuance/src/status-list-allocator.service.ts (1)
44-58:⚠️ Potential issue | 🟠 MajorPerformance degradation risk: unbounded loop when bitmap is nearly full.
The random allocation strategy in the
while (true)loop becomes increasingly inefficient as the bitmap fills up. At 90% capacity, ~10 attempts per allocation are expected; at 99%, ~100 attempts. This can cause severe latency spikes.🔧 Proposed fix: bounded random with linear fallback
public allocate(): number { if (this.allocatedCount === this.capacity) { throw new Error('No indexes left'); } - while (true) { - const idx = randomInt(this.capacity); - - if (!this.isSet(idx)) { - this.set(idx); - this.allocatedCount++; - return idx; + const maxRandomAttempts = 10; + for (let attempt = 0; attempt < maxRandomAttempts; attempt++) { + const idx = randomInt(this.capacity); + if (!this.isSet(idx)) { + this.set(idx); + this.allocatedCount++; + return idx; } } + + // Fallback: linear scan from random starting point + const start = randomInt(this.capacity); + for (let offset = 0; offset < this.capacity; offset++) { + const idx = (start + offset) % this.capacity; + if (!this.isSet(idx)) { + this.set(idx); + this.allocatedCount++; + return idx; + } + } + + throw new Error('No indexes left'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` around lines 44 - 58, The allocate() method uses an unbounded random retry loop which degrades as the bitmap fills; change allocate() (and use of isSet, set, allocatedCount, capacity) to first try a bounded number of random attempts (e.g., maxRetries = max(10, Math.ceil(capacity * 0.01))) and if none succeed fall back to a deterministic linear scan over the bitmap to find the first clear index, set it, increment allocatedCount and return it; preserve the existing full-capacity check and throw the same error if no index is free after the fallback.libs/prisma-service/prisma/schema.prisma (2)
235-235:⚠️ Potential issue | 🔴 CriticalRemove duplicate
webhookSecretfield declaration.The
org_agentsmodel declareswebhookSecrettwice (lines 235 and 243), which violates Prisma schema syntax and will block client generation and migrations.🔧 Remove duplicate
Remove line 243:
organisation organisation? `@relation`(fields: [orgId], references: [id]) webhookUrl String? `@db.VarChar` - webhookSecret String? `@db.VarChar` org_dids org_dids[]Also applies to: 243-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/schema.prisma` at line 235, The org_agents model contains a duplicate field declaration for webhookSecret which breaks Prisma schema parsing; open the prisma schema where the model org_agents is defined, locate both webhookSecret entries and remove the redundant one (leave a single webhookSecret String? `@db.VarChar` definition), then run Prisma validate/generate to ensure the schema compiles; reference the org_agents model and the webhookSecret field when making the change.
653-662:⚠️ Potential issue | 🟠 MajorAdd
orgIdto enable org-scoped revocation queries.The
revokeCredential()method queries allocations only byissuanceSessionId, but the/orgs/:orgId/.../revokeendpoint receives anorgIdparameter that isn't validated against the allocation ownership. WithoutorgId:
- Any org could potentially revoke another org's credentials if they guess the session ID
- Queries lack an efficient index path
🔧 Proposed addition
model issued_oid4vc_credentials { id String `@id` `@default`(uuid()) `@db.Uuid` credentialId String `@unique` listId String `@db.Uuid` index Int issuanceSessionId String `@db.VarChar` + orgId String `@db.Uuid` createDateTime DateTime `@default`(now()) `@db.Timestamptz`(6) updatedAt DateTime `@updatedAt` statusListUri String + + organisation organisation `@relation`(fields: [orgId], references: [id]) + + @@index([orgId, issuanceSessionId]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/schema.prisma` around lines 653 - 662, Add an org-scoped identifier to the issued_oid4vc_credentials model and make queries/indexing org-aware: add a new field orgId (String, `@db.Uuid`) to the issued_oid4vc_credentials model and create a composite index on (orgId, issuanceSessionId) so revocation queries can filter by org efficiently; update the revokeCredential implementation (the revokeCredential function and the /orgs/:orgId/.../revoke endpoint handler) to require and validate the orgId parameter and include orgId in the database WHERE clause when looking up allocations/credentials.apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (1)
774-790:⚠️ Potential issue | 🟠 MajorDon't silently downgrade revocable D2A offers.
This branch catches allocation errors and only logs a warning, allowing the offer to proceed without
statusListDetails. A caller requestingisRevocable: truewill receive SD-JWT credentials that cannot be revoked, and subsequent/revokecalls will fail.Either retry allocation or fail the entire request when any revocable credential can't reserve an index.
🔧 Proposed fix
if ((cred as any).format === CredentialFormat.SdJwtVc && !cred.statusListDetails) { - try { - const allocation = await this.statusListAllocatorService.allocate(orgId, agentDetailsForAlloc.orgDid); - cred.statusListDetails = { - listId: allocation.listId, - index: allocation.index, - listSize: Number(CommonConstants.DEFAULT_STATUS_LIST_SIZE) - }; - } catch (allocError) { - this.logger.warn(`Could not allocate status list index: ${allocError.message}`); - } + const allocation = await this.statusListAllocatorService.allocate(orgId, agentDetailsForAlloc.orgDid); + cred.statusListDetails = { + listId: allocation.listId, + index: allocation.index, + listSize: Number(CommonConstants.DEFAULT_STATUS_LIST_SIZE) + }; }🤖 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 774 - 790, The code currently catches allocation errors for SD-JWT credentials and only logs a warning, allowing revocable offers to proceed without statusListDetails; change this so allocation failures cause the request to fail (or be retried) instead of silently downgrading revocability: in the loop over oidcCredentialD2APayload.credentials, where you check (cred as any).format === CredentialFormat.SdJwtVc and call statusListAllocatorService.allocate(orgId, agentDetailsForAlloc.orgDid), remove the silent catch or implement a short retry and then throw an error (or return a failed response) if allocation still fails; ensure cred.statusListDetails is only left unset when revocability was not requested (check the isRevocable flag in the surrounding context), and replace this.logger.warn(...) with throwing an Error (or rejecting the request) so callers who requested isRevocable: true do not receive non-revocable credentials.
🧹 Nitpick comments (4)
libs/prisma-service/prisma/schema.prisma (1)
653-662: Consider adding relation fromlistIdtostatus_list_allocation.The
listIdfield referencesstatus_list_allocation.listIdbut lacks a Prisma relation. This prevents referential integrity checks and cascade behavior.🔧 Proposed addition
model issued_oid4vc_credentials { id String `@id` `@default`(uuid()) `@db.Uuid` credentialId String `@unique` listId String `@db.Uuid` index Int issuanceSessionId String `@db.VarChar` createDateTime DateTime `@default`(now()) `@db.Timestamptz`(6) updatedAt DateTime `@updatedAt` statusListUri String + + statusListAllocation status_list_allocation `@relation`(fields: [listId], references: [listId]) }And add the back-relation to
status_list_allocation:model status_list_allocation { ... organisation organisation `@relation`(fields: [orgId], references: [id]) + issuedCredentials issued_oid4vc_credentials[] @@index([orgId, isActive]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/schema.prisma` around lines 653 - 662, The issued_oid4vc_credentials model has a listId String field that intends to reference status_list_allocation.listId but lacks a Prisma relation; add a proper relation by changing listId to a foreign key field with `@relation`(references: [listId], fields: [listId]) (or similar) on the issued_oid4vc_credentials model and add the complementary back-relation field (e.g., issuedCredentials or issued_oid4vc_credentials) on the status_list_allocation model so Prisma enforces referential integrity and enables cascade behavior; locate the models by the names issued_oid4vc_credentials and status_list_allocation to implement the relation attributes and any desired onDelete/onUpdate behavior.libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql (1)
17-41: Consider adding foreign key fromissued_oid4vc_credentials.listIdtostatus_list_allocation.listId.The migration creates a foreign key from
status_list_allocation.orgIdtoorganisation.id, but there's no FK constraint linkingissued_oid4vc_credentials.listIdtostatus_list_allocation.listId. Without this, deleting astatus_list_allocationrow will orphanissued_oid4vc_credentialsrecords.🔧 Proposed addition
Add after line 41:
-- AddForeignKey ALTER TABLE "issued_oid4vc_credentials" ADD CONSTRAINT "issued_oid4vc_credentials_listId_fkey" FOREIGN KEY ("listId") REFERENCES "status_list_allocation"("listId") ON DELETE RESTRICT ON UPDATE CASCADE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql` around lines 17 - 41, The migration is missing a foreign key linking issued_oid4vc_credentials.listId to status_list_allocation.listId; add an ALTER TABLE statement to create constraint issued_oid4vc_credentials_listId_fkey that references status_list_allocation("listId") with ON DELETE RESTRICT ON UPDATE CASCADE so issued_oid4vc_credentials rows cannot be orphaned when a status_list_allocation is deleted; place this ADD CONSTRAINT after the other ALTER TABLE statements in the migration.apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
485-489: Consider conditionally includingisRevocablein the envelope.
isRevocableis always assigned tobaseEnvelope, even whenundefined. While this works, it adds an unnecessary property to non-revocable offers.♻️ Optional cleanup
const baseEnvelope: BuiltCredentialOfferBase = { credentials: builtCredentials, - ...(finalPublicIssuerId ? { publicIssuerId: finalPublicIssuerId } : {}), - isRevocable: dto.isRevocable + ...(finalPublicIssuerId ? { publicIssuerId: finalPublicIssuerId } : {}), + ...(dto.isRevocable !== undefined ? { isRevocable: dto.isRevocable } : {}) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts` around lines 485 - 489, The baseEnvelope currently always includes isRevocable (even when undefined); change the object construction in credential-sessions.builder.ts so that isRevocable is only spread into baseEnvelope when dto.isRevocable is not undefined (e.g., use a conditional spread instead of always setting isRevocable). Locate the baseEnvelope construction (BuiltCredentialOfferBase, baseEnvelope, builtCredentials, finalPublicIssuerId, dto.isRevocable) and replace the unconditional isRevocable assignment with a conditional spread like ...(dto.isRevocable !== undefined ? { isRevocable: dto.isRevocable } : {}).apps/oid4vc-issuance/src/status-list-allocator.service.ts (1)
8-11: Consider marking class members asreadonlyand usingnode:prefix.Per static analysis:
bitmapandcapacityare never reassigned; mark themreadonly- Prefer
node:cryptoovercryptofor Node.js built-in modules♻️ Suggested changes
-import { randomInt, randomUUID } from 'crypto'; +import { randomInt, randomUUID } from 'node:crypto'; export class RandomBitmapIndexAllocator { - private bitmap: Uint8Array; - private capacity: number; + private readonly bitmap: Uint8Array; + private readonly capacity: number; private allocatedCount: number;Also applies to: 6-6
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts` around lines 8 - 11, Mark the class fields bitmap and capacity in RandomBitmapIndexAllocator as readonly (since they are assigned only once) by adding the readonly modifier to their declarations, and update any related type annotations if needed; also change the built-in crypto import to use the node: prefix (replace import from "crypto" with "node:crypto") so the module uses Node's recommended specifier.
🤖 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 927-965: The revokeCredential method currently overwrites
lastResponse for each allocation and only returns the final result (symbols:
revokeCredential, allocations, lastResponse, natsCall, pattern, payload), which
swallows intermediate failures; change the logic to accumulate each natsCall
result into an array (e.g., responses) and return that array, and ensure
per-allocation errors are not silently swallowed by either (a) letting any
natsCall error throw immediately (wrap the call so it bubbles as an RpcException
with context including allocation.listId/index) or (b) capturing success/error
per allocation in the returned array (e.g., { allocation, result } or {
allocation, error }) so callers can see which allocations failed; keep the
existing outer try/catch to log and rethrow RpcException as before.
In
`@libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql`:
- Around line 17-29: The migration creates the "issued_oid4vc_credentials" table
but lacks an index on issuanceSessionId which makes
getCredentialAllocations(issuanceSessionId) perform full table scans; add a
CREATE INDEX for issuanceSessionId (e.g.,
issued_oid4vc_credentials_issuanceSessionId_idx) immediately after the table
creation in the migration to improve query performance and ensure the index name
matches your naming convention and the column "issuanceSessionId" in the
"issued_oid4vc_credentials" table.
- Line 23: The migration defines the column "issuanceSessionId" as VARCHAR with
no size; update the SQL to declare a bounded length (e.g., change
"issuanceSessionId" VARCHAR NOT NULL to "issuanceSessionId" VARCHAR(255) NOT
NULL or another appropriate max length for your data) so the column has an
explicit size constraint; ensure the chosen length matches any corresponding
model/schema constraints used elsewhere.
---
Duplicate comments:
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 774-790: The code currently catches allocation errors for SD-JWT
credentials and only logs a warning, allowing revocable offers to proceed
without statusListDetails; change this so allocation failures cause the request
to fail (or be retried) instead of silently downgrading revocability: in the
loop over oidcCredentialD2APayload.credentials, where you check (cred as
any).format === CredentialFormat.SdJwtVc and call
statusListAllocatorService.allocate(orgId, agentDetailsForAlloc.orgDid), remove
the silent catch or implement a short retry and then throw an error (or return a
failed response) if allocation still fails; ensure cred.statusListDetails is
only left unset when revocability was not requested (check the isRevocable flag
in the surrounding context), and replace this.logger.warn(...) with throwing an
Error (or rejecting the request) so callers who requested isRevocable: true do
not receive non-revocable credentials.
In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts`:
- Around line 44-58: The allocate() method uses an unbounded random retry loop
which degrades as the bitmap fills; change allocate() (and use of isSet, set,
allocatedCount, capacity) to first try a bounded number of random attempts
(e.g., maxRetries = max(10, Math.ceil(capacity * 0.01))) and if none succeed
fall back to a deterministic linear scan over the bitmap to find the first clear
index, set it, increment allocatedCount and return it; preserve the existing
full-capacity check and throw the same error if no index is free after the
fallback.
In `@libs/prisma-service/prisma/schema.prisma`:
- Line 235: The org_agents model contains a duplicate field declaration for
webhookSecret which breaks Prisma schema parsing; open the prisma schema where
the model org_agents is defined, locate both webhookSecret entries and remove
the redundant one (leave a single webhookSecret String? `@db.VarChar` definition),
then run Prisma validate/generate to ensure the schema compiles; reference the
org_agents model and the webhookSecret field when making the change.
- Around line 653-662: Add an org-scoped identifier to the
issued_oid4vc_credentials model and make queries/indexing org-aware: add a new
field orgId (String, `@db.Uuid`) to the issued_oid4vc_credentials model and create
a composite index on (orgId, issuanceSessionId) so revocation queries can filter
by org efficiently; update the revokeCredential implementation (the
revokeCredential function and the /orgs/:orgId/.../revoke endpoint handler) to
require and validate the orgId parameter and include orgId in the database WHERE
clause when looking up allocations/credentials.
---
Nitpick comments:
In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts`:
- Around line 485-489: The baseEnvelope currently always includes isRevocable
(even when undefined); change the object construction in
credential-sessions.builder.ts so that isRevocable is only spread into
baseEnvelope when dto.isRevocable is not undefined (e.g., use a conditional
spread instead of always setting isRevocable). Locate the baseEnvelope
construction (BuiltCredentialOfferBase, baseEnvelope, builtCredentials,
finalPublicIssuerId, dto.isRevocable) and replace the unconditional isRevocable
assignment with a conditional spread like ...(dto.isRevocable !== undefined ? {
isRevocable: dto.isRevocable } : {}).
In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts`:
- Around line 8-11: Mark the class fields bitmap and capacity in
RandomBitmapIndexAllocator as readonly (since they are assigned only once) by
adding the readonly modifier to their declarations, and update any related type
annotations if needed; also change the built-in crypto import to use the node:
prefix (replace import from "crypto" with "node:crypto") so the module uses
Node's recommended specifier.
In
`@libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql`:
- Around line 17-41: The migration is missing a foreign key linking
issued_oid4vc_credentials.listId to status_list_allocation.listId; add an ALTER
TABLE statement to create constraint issued_oid4vc_credentials_listId_fkey that
references status_list_allocation("listId") with ON DELETE RESTRICT ON UPDATE
CASCADE so issued_oid4vc_credentials rows cannot be orphaned when a
status_list_allocation is deleted; place this ADD CONSTRAINT after the other
ALTER TABLE statements in the migration.
In `@libs/prisma-service/prisma/schema.prisma`:
- Around line 653-662: The issued_oid4vc_credentials model has a listId String
field that intends to reference status_list_allocation.listId but lacks a Prisma
relation; add a proper relation by changing listId to a foreign key field with
`@relation`(references: [listId], fields: [listId]) (or similar) on the
issued_oid4vc_credentials model and add the complementary back-relation field
(e.g., issuedCredentials or issued_oid4vc_credentials) on the
status_list_allocation model so Prisma enforces referential integrity and
enables cascade behavior; locate the models by the names
issued_oid4vc_credentials and status_list_allocation to implement the relation
attributes and any desired onDelete/onUpdate behavior.
🪄 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: 9c58ec6d-7475-4ceb-b132-cb8b4c0fc488
📒 Files selected for processing (17)
.env.demo.env.sampleapps/agent-service/src/agent-service.controller.tsapps/agent-service/src/agent-service.service.tsapps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.tsapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tsapps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsapps/oid4vc-issuance/src/main.tsapps/oid4vc-issuance/src/oid4vc-issuance.controller.tsapps/oid4vc-issuance/src/oid4vc-issuance.module.tsapps/oid4vc-issuance/src/oid4vc-issuance.service.tsapps/oid4vc-issuance/src/status-list-allocator.service.tslibs/common/src/common.constant.tslibs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sqllibs/prisma-service/prisma/schema.prisma
✅ Files skipped from review due to trivial changes (5)
- libs/common/src/common.constant.ts
- .env.demo
- apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts
- apps/oid4vc-issuance/src/oid4vc-issuance.module.ts
- .env.sample
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts
- apps/agent-service/src/agent-service.controller.ts
- apps/agent-service/src/agent-service.service.ts
- apps/oid4vc-issuance/src/oid4vc-issuance.controller.ts
- apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
- apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (2)
250-254: Remove unnecessary non-null assertions.SonarCloud correctly flags
attr.children!as redundant—the guard at line 241 already confirmsattr.childrenexists and has length > 0.♻️ Proposed fix
if (Array.isArray(payloadValue)) { - const childFrame = buildDisclosureFrameFromTemplate(attr.children!); + const childFrame = buildDisclosureFrameFromTemplate(attr.children); frame[attr.key] = payloadValue.map(() => ({ ...childFrame })); } else { - const childFrame = buildDisclosureFrameFromTemplate(attr.children!, payloadValue as Record<string, any>); + const childFrame = buildDisclosureFrameFromTemplate(attr.children, payloadValue as Record<string, any>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts` around lines 250 - 254, The non-null assertions on attr.children are redundant because the surrounding guard already ensures attr.children exists and is non-empty; remove the unnecessary "!" usages and pass attr.children directly to buildDisclosureFrameFromTemplate in both branches (the array-mapped branch and the plain-object branch) so calls become buildDisclosureFrameFromTemplate(attr.children) and buildDisclosureFrameFromTemplate(attr.children, payloadValue as Record<string, any>), keeping behavior unchanged.
272-272: Remove commented-out debug code.♻️ Proposed fix
- // console.log('Built disclosure frame:', JSON.stringify(frame, null, 2)); return frame;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts` at line 272, Remove the commented-out debug statement "// console.log('Built disclosure frame:', JSON.stringify(frame, null, 2));"—delete the line entirely; if you need retained debug output, replace it with a proper logger call (e.g., logger.debug) in the function that builds the disclosure frame and reference the variable "frame" there instead.
🤖 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/libs/helpers/credential-sessions.builder.ts`:
- Around line 245-247: The TODO indicates missing type validation for payload
values: locate the block referencing payloadValue and the template definition
(search for symbols payloadValue and templateDefinition or the function that
builds credential sessions, e.g.,
CredentialSessionsBuilder/buildCredentialSession) and add runtime checks that
enforce payloadValue is either an object or an array of objects, then validate
structure according to the template (if templateDefinition.type === 'array'
require an array of objects; if 'object' require a single object). On validation
failure throw/return a descriptive error (or ValidationError) and add unit tests
covering object vs array cases and template mismatches; keep error messages tied
to the same function names (payloadValue/templateDefinition) so logs/tests can
find the source.
- Around line 248-251: The array branch in buildDisclosureFrameFromTemplate
fails to pass each element's payload into the recursive call and uses a shallow
spread for copies; update the Array.isArray(payloadValue) case so for each
element you call buildDisclosureFrameFromTemplate(attr.children!,
payloadElement) (or equivalent overload) to propagate the element's payload, and
create deep independent copies of the returned child frame (e.g., by
constructing each entry via a fresh recursive call or using a deterministic
deep-clone of the child frame) before assigning frame[attr.key] =
payloadValue.map(...), ensuring nested _sd arrays and objects are not shared
across array items.
---
Nitpick comments:
In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts`:
- Around line 250-254: The non-null assertions on attr.children are redundant
because the surrounding guard already ensures attr.children exists and is
non-empty; remove the unnecessary "!" usages and pass attr.children directly to
buildDisclosureFrameFromTemplate in both branches (the array-mapped branch and
the plain-object branch) so calls become
buildDisclosureFrameFromTemplate(attr.children) and
buildDisclosureFrameFromTemplate(attr.children, payloadValue as Record<string,
any>), keeping behavior unchanged.
- Line 272: Remove the commented-out debug statement "// console.log('Built
disclosure frame:', JSON.stringify(frame, null, 2));"—delete the line entirely;
if you need retained debug output, replace it with a proper logger call (e.g.,
logger.debug) in the function that builds the disclosure frame and reference the
variable "frame" there instead.
🪄 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: 0e8b9e68-e6ce-4d59-a512-f82155158cc4
📒 Files selected for processing (1)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts
Comment on lines
+245
to
+247
| //todo: | ||
| //1) Need to handle the type validation here to ensure payloadValue is in expected format (object or array of objects) | ||
| //2) Need to add add validation based on template definition (e.g. if template defines an array, payload must be an array, etc.) |
There was a problem hiding this comment.
Address the TODO comment before merging.
The comment acknowledges missing type validation for payload values. Consider creating an issue to track this work if it cannot be completed in this PR.
Do you want me to open a new issue to track this validation task?
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 245-245: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts` around
lines 245 - 247, The TODO indicates missing type validation for payload values:
locate the block referencing payloadValue and the template definition (search
for symbols payloadValue and templateDefinition or the function that builds
credential sessions, e.g., CredentialSessionsBuilder/buildCredentialSession) and
add runtime checks that enforce payloadValue is either an object or an array of
objects, then validate structure according to the template (if
templateDefinition.type === 'array' require an array of objects; if 'object'
require a single object). On validation failure throw/return a descriptive error
(or ValidationError) and add unit tests covering object vs array cases and
template mismatches; keep error messages tied to the same function names
(payloadValue/templateDefinition) so logs/tests can find the source.
Comment on lines
+248
to
+251
| if (Array.isArray(payloadValue)) { | ||
| // Array of objects → [{ _sd: [...] }, ...] | ||
| const childFrame = buildDisclosureFrameFromTemplate(attr.children!); | ||
| frame[attr.key] = payloadValue.map(() => ({ ...childFrame })); |
There was a problem hiding this comment.
Array handling has two issues: inconsistent payload propagation and shallow copy.
-
Missing payload propagation: Unlike the object branch (line 254), the array branch doesn't pass individual payload elements to the recursive call. This means nested children within array elements won't get payload-aware processing (e.g., detecting nested arrays).
-
Shallow copy hazard:
{ ...childFrame }creates shallow copies that share references to nested objects like_sdarrays. If any frame element is mutated, all copies are affected.
🔧 Proposed fix
if (Array.isArray(payloadValue)) {
- // Array of objects → [{ _sd: [...] }, ...]
- const childFrame = buildDisclosureFrameFromTemplate(attr.children!);
- frame[attr.key] = payloadValue.map(() => ({ ...childFrame }));
+ // Array of objects → [{ _sd: [...] }, ...] — build per-element frame
+ frame[attr.key] = payloadValue.map((element) =>
+ buildDisclosureFrameFromTemplate(attr.children, element as Record<string, any>)
+ );
} else {📝 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
| if (Array.isArray(payloadValue)) { | |
| // Array of objects → [{ _sd: [...] }, ...] | |
| const childFrame = buildDisclosureFrameFromTemplate(attr.children!); | |
| frame[attr.key] = payloadValue.map(() => ({ ...childFrame })); | |
| if (Array.isArray(payloadValue)) { | |
| // Array of objects → [{ _sd: [...] }, ...] — build per-element frame | |
| frame[attr.key] = payloadValue.map((element) => | |
| buildDisclosureFrameFromTemplate(attr.children, element as Record<string, any>) | |
| ); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 250-250: This assertion is unnecessary since it does not change the type of the expression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts` around
lines 248 - 251, The array branch in buildDisclosureFrameFromTemplate fails to
pass each element's payload into the recursive call and uses a shallow spread
for copies; update the Array.isArray(payloadValue) case so for each element you
call buildDisclosureFrameFromTemplate(attr.children!, payloadElement) (or
equivalent overload) to propagate the element's payload, and create deep
independent copies of the returned child frame (e.g., by constructing each entry
via a fresh recursive call or using a deterministic deep-clone of the child
frame) before assigning frame[attr.key] = payloadValue.map(...), ensuring nested
_sd arrays and objects are not shared across array items.
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
…ebl/platform into feat/sd-jwt-revocation-flow Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (1)
673-727:⚠️ Potential issue | 🔴 CriticalRemove the duplicate persistence block before this can compile.
Lines 674/701 redeclare
parsedResponse, Lines 681/708 redeclareissuanceSessionId, and the firstsaveCredentialAllocation()call omits the neworgIdargument. Keep one block withorgId, and fail revocable offers when the agent response has no session id so allocated indexes are not left without lookup rows.🐛 Proposed fix
- // Revocation logic to save the credential offer id and status list details in DB for later use during revocation - let parsedResponse; - if ('string' === typeof createCredentialOfferOnAgent.response) { - parsedResponse = JSON.parse(createCredentialOfferOnAgent.response); - } else { - parsedResponse = createCredentialOfferOnAgent.response; - } - - const issuanceSessionId = - parsedResponse.issuanceSessionId || - parsedResponse.credentialOfferId || - parsedResponse.id || - parsedResponse.issuanceSession?.id; - if (issuanceSessionId && createOidcCredentialOffer.isRevocable) { - for (const cred of buildOidcCredentialOffer.credentials) { - if (cred.statusListDetails) { - const statusListUri = `${process.env.STATUS_LIST_HOST}/status-lists/${cred.statusListDetails.listId}`; - await this.statusListAllocatorService.saveCredentialAllocation( - `${issuanceSessionId}-${cred.statusListDetails.index}`, - cred.statusListDetails.listId, - cred.statusListDetails.index, - issuanceSessionId, - statusListUri - ); - } - } - } - let parsedResponse; if ('string' === typeof createCredentialOfferOnAgent.response) { parsedResponse = JSON.parse(createCredentialOfferOnAgent.response); @@ - if (issuanceSessionId && createOidcCredentialOffer.isRevocable) { + if (createOidcCredentialOffer.isRevocable) { + if (!issuanceSessionId) { + throw new InternalServerErrorException('Issuance session ID missing from agent response'); + } + for (const cred of buildOidcCredentialOffer.credentials) { if (cred.statusListDetails) { const statusListUri = `${process.env.STATUS_LIST_HOST}/status-lists/${cred.statusListDetails.listId}`;🤖 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 673 - 727, Duplicate persistence logic for parsedResponse/issuanceSessionId must be removed; keep a single block that parses createCredentialOfferOnAgent.response into parsedResponse, computes issuanceSessionId (from parsedResponse.issuanceSessionId || parsedResponse.credentialOfferId || parsedResponse.id || parsedResponse.issuanceSession?.id), and only proceeds if issuanceSessionId and createOidcCredentialOffer.isRevocable; call statusListAllocatorService.saveCredentialAllocation with the orgId argument (use signature saveCredentialAllocation(orgId, allocationId, listId, index, issuanceSessionId, statusListUri)); if the offer is revocable but issuanceSessionId is missing, throw an error (or otherwise fail) so you do not allocate status list indexes without a lookup row; reference buildOidcCredentialOffer.credentials and cred.statusListDetails when building statusListUri and allocationId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.demo:
- Around line 248-257: The .env.demo contains a duplicated environment variable
STATUS_LIST_HOST which can silently override the intended value; remove the
duplicate declaration so only one STATUS_LIST_HOST exists (keep the intended one
and delete the other) and ensure the remaining STATUS_LIST_HOST is set or
documented appropriately so startup has a single canonical value.
In @.env.sample:
- Around line 276-285: Duplicate declaration of STATUS_LIST_HOST exists; remove
the redundant blank entry so only a single STATUS_LIST_HOST key remains. Locate
the two occurrences of STATUS_LIST_HOST in the snippet, delete the second/extra
line (the one near the end), and ensure the remaining single STATUS_LIST_HOST is
the one used for local configuration so apps/oid4vc-issuance/src/main.ts can
correctly detect its presence.
In `@apps/agent-service/src/agent-service.service.ts`:
- Around line 1495-1501: The current catch in _oidcRevokeCredential logs
JSON.stringify(error) which can leak headers including the decrypted agent API
key; change the logging to a sanitized message that does not serialize the full
error or include request config/headers. Specifically, in
agent-service.service.ts inside _oidcRevokeCredential replace
JSON.stringify(error) with a concise log that includes error.message (and
optionally error.response?.status or a short, redacted error.response?.data) but
never error.config or headers; ensure you do not log getApiKey or any
Authorization header and throw the original error after logging.
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 795-864: The catch block currently releases any
cred.statusListDetails regardless of who allocated it; change the allocation
loop (inside the isRevocable && hasSdJwt branch where
statusListAllocatorService.allocate is called and cred.statusListDetails is set)
to record allocated entries into a local array (e.g., allocatedStatusEntries as
objects containing listId and index and possibly cred reference or a symbol);
then in the catch block, iterate only over that allocatedStatusEntries array and
call statusListAllocatorService.release for those entries, leaving pre-existing
cred.statusListDetails untouched; update references to
statusListAllocatorService.allocate, cred.statusListDetails, and the catch
release loop to use the new allocatedStatusEntries collection.
In `@apps/oid4vc-issuance/src/status-list-allocator.service.ts`:
- Around line 111-151: The current allocation can race when two transactions
read the same active bitmap and both allocate the same index; fix by serializing
updates with a DB-level uniqueness guard plus a retry loop using optimistic
conditional update or row-level locking: add a migration creating a partial
unique index on status_list_allocation(orgId, issuerDid) WHERE isActive = true
to enforce one active list; inside the transaction, when selecting the active
allocation use a SELECT FOR UPDATE (via tx.$queryRaw) or re-read the row before
updating, allocate with RandomBitmapIndexAllocator on the in-transaction bitmap,
then attempt an UPDATE that includes a conditional WHERE matching the previous
bitmap/allocatedCount (or use the FOR UPDATE-locked row) and retry the
allocate/update sequence on conflict up to N times; update references to
status_list_allocation.update, RandomBitmapIndexAllocator.allocate,
allocator.export, and the bitmap/allocatedCount fields accordingly.
- Around line 204-212: The code always decrements allocatedCount after calling
RandomBitmapIndexAllocator.release(), but release() is a no-op for an
already-free index so allocatedCount can go out of sync; change the logic to
only decrement allocatedCount when a bit was actually cleared — either by using
a boolean return value from RandomBitmapIndexAllocator.release(index) if
available, or by checking the bit state before releasing (e.g., call
allocator.isSet(index) or allocator.get(index) prior to
allocator.release(index)), and only update status_list_allocation.allocatedCount
= Math.max(0, allocation.allocatedCount - 1) when that check indicates the bit
was previously set; still persist bitmap with Buffer.from(allocator.export()) as
before.
---
Duplicate comments:
In `@apps/oid4vc-issuance/src/oid4vc-issuance.service.ts`:
- Around line 673-727: Duplicate persistence logic for
parsedResponse/issuanceSessionId must be removed; keep a single block that
parses createCredentialOfferOnAgent.response into parsedResponse, computes
issuanceSessionId (from parsedResponse.issuanceSessionId ||
parsedResponse.credentialOfferId || parsedResponse.id ||
parsedResponse.issuanceSession?.id), and only proceeds if issuanceSessionId and
createOidcCredentialOffer.isRevocable; call
statusListAllocatorService.saveCredentialAllocation with the orgId argument (use
signature saveCredentialAllocation(orgId, allocationId, listId, index,
issuanceSessionId, statusListUri)); if the offer is revocable but
issuanceSessionId is missing, throw an error (or otherwise fail) so you do not
allocate status list indexes without a lookup row; reference
buildOidcCredentialOffer.credentials and cred.statusListDetails when building
statusListUri and allocationId.
🪄 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: 139e7d88-65f5-4a90-8760-ced955f8b2c7
📒 Files selected for processing (9)
.env.demo.env.sampleapps/agent-service/src/agent-service.controller.tsapps/agent-service/src/agent-service.service.tsapps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsapps/oid4vc-issuance/src/oid4vc-issuance.service.tsapps/oid4vc-issuance/src/status-list-allocator.service.tslibs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sqllibs/prisma-service/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
… counters Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
…ebl/platform into feat/sd-jwt-revocation-flow Signed-off-by: Sagar Khole <sagar.khole@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.
Feat/sd jwt revocation flow
Summary by CodeRabbit
New Features
Chores