Feat: Oid4vc delete issuer#343
Conversation
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR exposes the OpenId4VcIssuerRepository from the credo-ts package and implements a deleteIssuer endpoint in the issuer controller and service layer that leverages this repository to delete issuers by ID. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/controllers/openid4vc/issuers/issuer.service.ts`:
- Around line 47-52: The deleteIssuer method currently bypasses the OpenId4VcApi
and uses OpenId4VcIssuerRepository.deleteById because the public API in
`@credo-ts/openid4vc`@0.6.1 lacks a delete method; add a concise comment above the
repository call (in deleteIssuer) stating this library limitation and warning
that repository-only deletion may leave orphaned state (issuance
sessions/credential offers) since no higher-level cleanup is performed, then
simplify the implementation by removing the .then() chain and using await
issuanceRepository.deleteById(agentReq.agent.context, issuerId) followed by
returning the success message.
🧹 Nitpick comments (3)
patches/@credo-ts+openid4vc+0.6.1+001+add-export-for-issuance-repository.patch (1)
1-33: Patch-based export is fragile and version-coupled.This patch modifies built artifacts inside
node_modules. It will silently break if@credo-ts/openid4vcis upgraded past0.6.1and the internal file paths or export structure change. Consider opening an upstream PR/issue to exposeOpenId4VcIssuerRepositoryfrom the package's public API so this patch can eventually be removed.src/controllers/openid4vc/issuers/issuer.Controller.ts (1)
82-92: Clarify theidparameter semantics — it differs from other endpoints.Other endpoints in this controller use
publicIssuerId(the logical/public issuer identifier), but this endpoint acceptsidwhich maps to the internal record ID (per the JSDoc and the service'sdeleteByIdcall). This inconsistency could confuse API consumers who might pass apublicIssuerIdand get a "not found" error. Consider either:
- Renaming the param to
recordIdto make the distinction explicit, or- Accepting
publicIssuerId, resolving it to a record, then deleting — consistent with how the rest of the API surface works.src/controllers/openid4vc/issuers/issuer.service.ts (1)
49-51: Minor: preferasync/awaitover mixingawaitwith.then().The current pattern is functional but inconsistent with the rest of the codebase which uses straight
await.Suggested simplification
public async deleteIssuer(agentReq: Req, issuerId: string) { const issuanceRepository = agentReq.agent.dependencyManager.resolve(OpenId4VcIssuerRepository) - return await issuanceRepository.deleteById(agentReq.agent.context, issuerId).then(() => { - return { message: 'Record deleted successfully' } - }) + await issuanceRepository.deleteById(agentReq.agent.context, issuerId) + return { message: 'Record deleted successfully' } }
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
|



What?
API to delete an OID4VC issuer.
How?
Uncommented the previous code.
Updated the service-level call for the delete-issuer module.
Added a patch to export the
OpenId4VcIssuerRepositorymodule from the OID4VC build.Summary by CodeRabbit