Open
Conversation
sagarkhole4
requested review from
RinkalBhojani,
shitrerohit,
sujitaw and
tipusinghaw
📝 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(issuanceSessionId)
Alloc->>DB: fetch issued_oid4vc_credentials
Alloc-->>OID: [{listId,index,statusListUri}]
loop per allocation
OID->>Agent: oidcRevokeCredential(url, orgId, statusListDetails)
Agent->>Agent: resolve org agent API key
Agent->>Agent: POST statusListDetails to url
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 Possibly related PRs
Suggested labels
Suggested reviewers
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 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: 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
Comment on lines
+191
to
+193
| @ApiPropertyOptional({ example: true, description: 'Flag to enable revocation for the issued credentials' }) | ||
| @IsOptional() | ||
| isRevocable?: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts | sed -n '180,200p'Repository: credebl/platform
Length of output: 846
🏁 Script executed:
cat -n apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts | sed -n '380,400p'Repository: credebl/platform
Length of output: 811
🏁 Script executed:
wc -l apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsRepository: credebl/platform
Length of output: 128
🏁 Script executed:
rg "isRevocable" --type ts -B 2 -A 2Repository: credebl/platform
Length of output: 4843
🏁 Script executed:
rg "if.*isRevocable|\.isRevocable" --type ts -B 1 -A 1Repository: credebl/platform
Length of output: 1701
🏁 Script executed:
head -30 apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.tsRepository: credebl/platform
Length of output: 938
Add @IsBoolean() validator to isRevocable fields in both DTOs.
Without boolean validation, requests can send "false" or "true" strings which pass validation. Since JavaScript treats non-empty strings as truthy, the if (dto.isRevocable) checks in the service layer will incorrectly enable revocation even when "false" is provided.
Both occurrences in CreateOidcCredentialOfferDto (lines 191-193) and CreateCredentialOfferD2ADto (lines 389-391) need the @IsBoolean() decorator added after @IsOptional(). Update the imports to include IsBoolean from class-validator.
🤖 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/issuer-sessions.dto.ts` around
lines 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.
Comment on lines
+746
to
+767
| if (oidcCredentialD2APayload.isRevocable) { | ||
| const hasSdJwt = oidcCredentialD2APayload.credentials.some((c) => c.format === CredentialFormat.SdJwtVc); | ||
| if (hasSdJwt) { | ||
| const agentDetailsForAlloc = await this.oid4vcIssuanceRepository.getAgentEndPoint(orgId); | ||
| if (!agentDetailsForAlloc?.orgDid) { | ||
| throw new BadRequestException('Organization DID is required for revocable credentials'); | ||
| } | ||
| for (const cred of oidcCredentialD2APayload.credentials) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| 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}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't silently downgrade revocable D2A offers.
This branch only logs allocator failures and still sends the offer onward. A caller can ask for isRevocable: true and receive SD-JWT credentials with no statusListDetails, after which /revoke will reject them as non-revocable. Retry the allocator or fail the whole request once any revocable credential can't reserve an index.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 755-755: 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/src/oid4vc-issuance.service.ts` around lines 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.
Comment on lines
+44
to
+58
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance degradation risk: unbounded loop when bitmap is nearly full.
The random allocation strategy 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 or timeouts in production.
Consider bounding the random attempts and falling back to a linear scan, or maintaining a free-list for O(1) allocation.
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');
}📝 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
| 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; | |
| } | |
| } | |
| } | |
| public allocate(): number { | |
| if (this.allocatedCount === this.capacity) { | |
| throw new Error('No indexes left'); | |
| } | |
| 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
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.
Comment on lines
+653
to
+662
| 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 | ||
| } |
There was a problem hiding this comment.
Persist the owning org on issued allocation rows.
revokeCredential() currently loads allocations by issuanceSessionId alone, but this table has no orgId/issuer relation or lookup index. The new /orgs/:orgId/.../revoke path therefore can't prove the session belongs to that org, and every revoke will scan this table. Add orgId (FK), persist it from saveCredentialAllocation(), and query by [orgId, issuanceSessionId]. Mirror the same change in libs/prisma-service/prisma/migrations/20260323183000_index_allocation/migration.sql.
🤖 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
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.
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
tipusinghaw
force-pushed
the
feat/sd-jwt-revocation-flow
branch
from
380e0cc to
fe14467
Compare
Signed-off-by: Tipu_Singh <[email protected]>
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
Comment on lines
+927
to
965
| async revokeCredential(issuanceSessionId: string, orgId: string): Promise<object> { | ||
| try { | ||
| if (!issuanceSessionId) { | ||
| throw new BadRequestException('Please provide a valid issuanceSessionId'); | ||
| } | ||
|
|
||
| const allocations = await this.statusListAllocatorService.getCredentialAllocations(issuanceSessionId); | ||
|
|
||
| const url = getAgentUrl( | ||
| await this.getAgentEndpoint(orgId), | ||
| CommonConstants.OIDC_ISSUER_SESSIONS_BY_ID, | ||
| `${issuanceSessionId}/revoke` | ||
| ); | ||
|
|
||
| const pattern = { cmd: 'agent-service-oid4vc-revoke-credential' }; | ||
|
|
||
| let lastResponse; | ||
| if (allocations && 0 < allocations.length) { | ||
| for (const allocation of allocations) { | ||
| const payload: any = { | ||
| url, | ||
| orgId, | ||
| statusListDetails: { | ||
| listId: allocation.listId, | ||
| index: allocation.index, | ||
| listSize: Number(CommonConstants.DEFAULT_STATUS_LIST_SIZE) | ||
| } | ||
| }; | ||
| lastResponse = await this.natsCall(pattern, payload); | ||
| } | ||
| } else { | ||
| throw new BadRequestException('Credential is not revocable as no status list allocation was found.'); | ||
| } | ||
|
|
||
| return lastResponse; | ||
| } catch (error) { | ||
| this.logger.error(`[revokeCredential] - error: ${JSON.stringify(error)}`); | ||
| throw new RpcException(error.response ?? error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the agent-service handles the revoke endpoint and whether it updates status list state
ast-grep --pattern $'@MessagePattern({ cmd: "agent-service-oid4vc-revoke-credential" })
$$$
'
# Check if there's any status list update logic in agent service
rg -n "statusList" apps/agent-service/src/ -C3Repository: credebl/platform
Length of output: 1780
🏁 Script executed:
# Check if the repository is already available in the working directory
find . -name "oid4vc-issuance.service.ts" -type f 2>/dev/null | head -5Repository: credebl/platform
Length of output: 1873
🏁 Script executed:
# Search for the agent-service revoke handler pattern
rg -n "agent-service-oid4vc-revoke-credential" -A 20 2>/dev/null | head -50Repository: credebl/platform
Length of output: 4028
🏁 Script executed:
# Look for status list update/revoke logic
rg -n "statusList|revoke" --type ts -i | grep -E "(update|revoke)" | head -30Repository: credebl/platform
Length of output: 3054
🏁 Script executed:
# Find agent-service.service.ts and search for oidcRevokeCredential implementation
rg -n "oidcRevokeCredential" apps/agent-service/src/ -A 15 | head -60Repository: credebl/platform
Length of output: 4007
🏁 Script executed:
# Search for status list update logic that might be related to revocation
rg -n "statusList|updateStatus|revokedIndices" apps/agent-service/src/ -i -B 2 -A 5 | head -80Repository: credebl/platform
Length of output: 2170
🏁 Script executed:
# Check if there's any documentation or tests showing the expected behavior
find . -name "*.spec.ts" -o -name "*.test.ts" | xargs rg -l "revokeCredential" 2>/dev/null | head -5Repository: credebl/platform
Length of output: 42
Only the last revocation response is returned when multiple allocations exist.
When revoking a credential with multiple status list allocations, the method loops through each allocation calling the agent revoke endpoint, but only lastResponse is returned (line 955-956). If an intermediate allocation fails, that failure is swallowed and only the final response is captured. Consider collecting all responses or ensuring error handling covers intermediate failures.
The status list bit update is delegated to the external agent service—the agent endpoint handles the actual bitmap update via the statusListDetails payload, so local state update is not needed here.
🤖 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 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.
Comment on lines
+17
to
+29
| -- CreateTable | ||
| CREATE TABLE "issued_oid4vc_credentials" ( | ||
| "id" UUID NOT NULL, | ||
| "credentialId" TEXT NOT NULL, | ||
| "listId" UUID NOT NULL, | ||
| "index" INTEGER NOT NULL, | ||
| "issuanceSessionId" VARCHAR NOT NULL, | ||
| "createDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "updatedAt" TIMESTAMP(3) NOT NULL, | ||
| "statusListUri" TEXT NOT NULL, | ||
|
|
||
| CONSTRAINT "issued_oid4vc_credentials_pkey" PRIMARY KEY ("id") | ||
| ); |
There was a problem hiding this comment.
Add index on issuanceSessionId for query performance.
The getCredentialAllocations(issuanceSessionId) method queries this table by issuanceSessionId, but there's no index on that column. This will result in full table scans.
🔧 Proposed fix
Add after line 38:
-- CreateIndex
CREATE INDEX "issued_oid4vc_credentials_issuanceSessionId_idx" ON "issued_oid4vc_credentials"("issuanceSessionId");🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 23-23: Use VARCHAR2 instead of VARCHAR.
[failure] 23-23: Add the mandatory size constraint.
🤖 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 - 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.
| "credentialId" TEXT NOT NULL, | ||
| "listId" UUID NOT NULL, | ||
| "index" INTEGER NOT NULL, | ||
| "issuanceSessionId" VARCHAR NOT NULL, |
There was a problem hiding this comment.
Add size constraint to VARCHAR column.
Per static analysis, issuanceSessionId VARCHAR lacks a mandatory size constraint. While PostgreSQL allows unbounded VARCHAR, it's best practice to specify a maximum length for data integrity.
🔧 Proposed fix
- "issuanceSessionId" VARCHAR NOT NULL,
+ "issuanceSessionId" VARCHAR(255) NOT NULL,📝 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
| "issuanceSessionId" VARCHAR NOT NULL, | |
| "issuanceSessionId" VARCHAR(255) NOT NULL, |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 23-23: Use VARCHAR2 instead of VARCHAR.
[failure] 23-23: Add the mandatory size constraint.
🤖 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`
at 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.
Signed-off-by: Tipu_Singh <[email protected]>
|
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.
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