feat: simplify wallet provisioning flow (detach did creation) #1528
Conversation
WalkthroughThreaded orgId through provisioning, service, and repository layers; createTenant now returns richer IStoreOrgAgentDetails; wallet provisioning and agent spin-up statuses renamed to WALLET_CREATED/DID_CREATED; repository adds updateAgentSpinupStatus; DTO validation and success message text updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API Gateway
participant Controller as AgentServiceController
participant Service as AgentServiceService
participant Repo as AgentServiceRepository
participant Provision as AgentProvisioning
Client->>API: POST /create-tenant (CreateTenantDto)
API->>Controller: createTenant(payload)
Controller->>Service: createTenant(payload, user)
Service->>Repo: createOrgAgent(WALLET_CREATED?, userId, orgId)
Repo-->>Service: orgAgentRecord (orgId, tenantId, walletName)
Service->>Provision: provisionWallet(orgAgentRecord, user)
Provision-->>Service: walletProvisionResult (walletName, agentEndPoint)
Service->>Repo: storeOrgAgentDetails({orgId, tenantId, walletName, agentEndPoint})
Repo-->>Service: storedOrgAgentDetails (IStoreOrgAgentDetails)
Service->>Repo: updateAgentSpinupStatus(orgId)
Service-->>Controller: IStoreOrgAgentDetails
Controller-->>Client: 200 OK (IStoreOrgAgentDetails)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
pranalidhanavade
force-pushed
the
feat/create-wallet-api-changes
branch
from
b0a232f to
7314f1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agent-service/src/agent-service.service.ts (1)
746-770: <!-- [scratchpad_start]
[task verify_review_comment_1/1 COMPLETE][observations]
- No explanatory comments for why COMPLETED check was commented out
- The code is identical between createTenant and processWalletProvision except for this missing check
- Both methods get agent details and check status
- processWalletProvision has BOTH checks, createTenant only has PROCESSED check
- No indication this is intentional architectural difference
[analysis]
FINAL VERIFICATION RESULTS:
- ✓ CONFIRMED: agentStatusResponse is unused - it's created but never returned or used
- ✓ CONFIRMED: COMPLETED check was removed from createTenant but exists in processWalletProvision
- ✓ CONFIRMED: Removal creates a gap - allows createTenant to proceed even if wallet already COMPLETED
- ✓ CONFIRMED: No comment or documentation explaining why this check was removed
- ✓ CONFIRMED: This is a real issue - potential duplicate wallet creation vulnerability
Review comment is ACCURATE and VALID.
Original severity:
Final verdict: Issues are confirmed and still need to be fixed.
Final tag:[done]
Ready to generate final rewritten review comment with confirmed findings.
</scratchpad_start>Based on my verification, I've confirmed both issues raised in the review comment. Here's the final rewritten review:
Remove unused variable and restore duplicate-wallet prevention check.
Unused variable:
agentStatusResponse(lines 748-750) is assigned but never used—remove it.Missing COMPLETED status check: The check for
AgentSpinUpStatus.COMPLETED(lines 753-759) has been commented out, but it exists and is active in theprocessWalletProvision()method (line 193). Without this check,createTenant()can proceed even if a wallet already exists in COMPLETED status, potentially allowing duplicate wallet creation or overwriting. Restore this check to match the validation logic inprocessWalletProvision():async createTenant(payload: ITenantDto, user: IUserRequestInterface): Promise<IStoreOrgAgentDetails> { try { - const agentStatusResponse = { - agentSpinupStatus: AgentSpinUpStatus.PROCESSED - }; const getOrgAgent = await this.agentServiceRepository.getAgentDetails(payload.orgId); + if (AgentSpinUpStatus.COMPLETED === getOrgAgent?.agentSpinUpStatus) { + this.logger.error(`Your wallet is already been created.`); + throw new ConflictException(ResponseMessages.agent.error.walletAlreadyCreated, { + cause: new Error(), + description: ResponseMessages.errorMessages.conflict + }); + } if (AgentSpinUpStatus.PROCESSED === getOrgAgent?.agentSpinUpStatus) {
♻️ Duplicate comments (1)
apps/agent-service/src/agent-service.service.ts (1)
836-850: Clean up commented-out code and verify the PROCESSED status workflow.
Commented-out code: Multiple lines (837-840, 847) contain commented-out code that should be removed per the new workflow. This was flagged by static analysis.
Status set to PROCESSED: Setting
agentSpinUpStatustoAgentSpinUpStatus.PROCESSED(line 840) instead ofCOMPLETEDindicates the wallet is not fully ready. This aligns with the new workflow where status updates to COMPLETED after DID creation (lines 965-967). Ensure downstream consumers handle the PROCESSED state correctly.Apply this diff to clean up the commented-out code:
const storeOrgAgentData: IStoreOrgAgentDetails = { - // did: tenantDetails.DIDCreationOption.did, - // isDidPublic: true, - // didDoc: tenantDetails.DIDCreationOption.didDocument || tenantDetails.DIDCreationOption.didDoc, //changed the didDoc into didDocument agentSpinUpStatus: AgentSpinUpStatus.PROCESSED, agentsTypeId: agentTypeId, orgId: payload.orgId, agentEndPoint: platformAdminSpinnedUp.org_agents[0].agentEndPoint, orgAgentTypeId, tenantId: tenantDetails.walletResponseDetails['id'], walletName: payload.label, - // ledgerId: ledgerIdData.map((item) => item.id), id: agentProcess?.id, apiKey: await this.commonService.dataEncryption(tenantDetails.walletResponseDetails['token']) };
🧹 Nitpick comments (1)
apps/agent-service/src/agent-service.service.ts (1)
147-148: Address or remove the TODO comment.The TODO comment indicates uncertainty about whether to get the first element from
ledgerName. The current implementation passes the full array togetLedgerDetails, which appears to be the intended behavior. Either clarify the logic and remove the TODO, or implement the necessary change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/agent-provisioning/src/agent-provisioning.service.ts(1 hunks)apps/agent-service/src/agent-service.controller.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(12 hunks)apps/agent-service/src/repositories/agent-service.repository.ts(6 hunks)apps/api-gateway/src/agent-service/dto/create-tenant.dto.ts(1 hunks)libs/common/src/response-messages/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api-gateway/src/agent-service/dto/create-tenant.dto.ts (3)
libs/validations/maxLength.ts (1)
MaxLength(3-28)libs/common/src/cast.helper.ts (1)
trim(39-43)libs/validations/minLength.ts (1)
MinLength(3-28)
apps/agent-service/src/agent-service.controller.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (1)
IStoreOrgAgentDetails(177-201)
apps/agent-service/src/repositories/agent-service.repository.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (2)
ICreateOrgAgent(449-451)IStoreOrgAgentDetails(177-201)
apps/agent-service/src/agent-service.service.ts (4)
apps/agent-service/src/interface/agent-service.interface.ts (4)
ICreateOrgAgent(449-451)ITenantDto(56-73)IUserRequestInterface(237-250)IStoreOrgAgentDetails(177-201)apps/ledger/src/schema/interfaces/schema.interface.ts (1)
IUserRequestInterface(5-18)apps/connection/src/interfaces/connection.interfaces.ts (1)
IUserRequestInterface(21-33)apps/api-gateway/src/connection/interfaces/index.ts (1)
IUserRequestInterface(3-15)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 147-147: Complete the task associated to this "TODO" comment.
[warning] 748-748: Remove this useless assignment to variable "agentStatusResponse".
[warning] 753-759: Remove this commented out code.
[warning] 874-874: Remove this commented out code.
[warning] 858-865: Remove this commented out code.
[warning] 879-881: Remove this commented out code.
[warning] 867-867: Remove this commented out code.
[warning] 869-869: Remove this commented out code.
[warning] 1129-1137: Remove this commented out code.
🔇 Additional comments (11)
apps/agent-provisioning/src/agent-provisioning.service.ts (1)
1-10: LGTM!The import reorganization improves readability by grouping related imports together.
libs/common/src/response-messages/index.ts (1)
226-226: Appropriate message update for the new synchronous flow.The change from an "initiated" message to "created successfully" aligns with the updated API behavior that now returns richer wallet details immediately rather than a pending status.
apps/agent-service/src/repositories/agent-service.repository.ts (4)
1-27: LGTM!Import reorganization is clean. The addition of
ICreateOrgAgentandIStoreOrgAgentDetailsinterfaces supports the updated method signatures.
103-120: LGTM!The
createOrgAgentmethod now correctly accepts and storesorgId, enabling proper organization context tracking from the initial agent creation.
159-211: Return type is narrower thanIStoreOrgAgentDetailsinterface.The method's explicit return type specifies only 6 fields (
orgId,agentSpinUpStatus,agentEndPoint,tenantId,walletName,id), but theIStoreOrgAgentDetailsinterface (used in the service layer) has 24 optional fields. This is intentional narrowing for this specific flow, but ensure callers don't expect additional fields likeapiKey,did, orverkeyfrom this method's return value.
263-282: LGTM!The new
updateAgentSpinupStatusmethod correctly updates the agent status toCOMPLETEDusingorgIdas the unique identifier. Error handling follows the established pattern in this repository.apps/agent-service/src/agent-service.controller.ts (2)
1-31: LGTM!Import reorganization is clean and properly includes
IStoreOrgAgentDetailsto support the updated return type.
49-55: I'll help you verify this review comment about the breaking change in thecreateTenantmethod's return type. Let me search the codebase to understand the scope of this change and its impact on downstream consumers.
<function_calls>
#!/bin/bashSearch for create-tenant message pattern usage and callers
rg -n -A 10 "'create-tenant'" --type=ts
echo "---"
echo "Searching for alternative patterns:"
rg -n '"create-tenant"' --type=tsecho "---"
echo "Searching for old response type references:"
rg -n "IAgentSpinUpSatus|IAgentSpinupStatus|agent.*status" --type=ts -i
</function_calls><function_calls>
#!/bin/bashFind IStoreOrgAgentDetails definition
rg -n "interface IStoreOrgAgentDetails|type IStoreOrgAgentDetails|export.*IStoreOrgAgentDetails" --type=ts
echo "---"
echo "Search for any related agent details types:"
rg -n "interface.*Agent.*Details|type.*Agent.*Details" --type=ts
</function_calls><function_calls>
#!/bin/bashCheck the actual file to see the full context around the method
cat -n apps/agent-service/src/agent-service.controller.ts | head -100
</function_calls><function_calls>
#!/bin/bashFind all message pattern usages in the codebase
rg -n "MessagePattern|send.*create-tenant|@MessagePattern" --type=ts | grep -i "create-tenant|send"
echo "---"
echo "Look for RPC client usage for agent-service:"
rg -n "agent-service|AgentService" --type=ts | head -30
</function_calls>apps/agent-service/src/agent-service.service.ts (3)
130-130: LGTM!The simplified return of
AgentSpinUpStatus.COMPLETEDafter successful wallet provisioning is clean and appropriate.
445-447: LGTM!The updated
createOrgAgentsignature now properly acceptsorgId, enabling correct organization association during agent creation. The repository call is updated accordingly.
965-967: LGTM!The conditional status update to
COMPLETEDafter DID creation correctly implements the two-step workflow. The check forPROCESSEDstatus ensures this only applies to the new wallet creation flow.
|
Tip For best results, initiate chat on the files or code changes. @pranalidhanavade, hello! How can I help you with this pull request? I can assist you with:
Please let me know what you'd like me to do! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
GHkrishna
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agent-service/src/agent-service.service.ts (1)
805-811: Add validation for the wallet token.The validation only checks for
walletResponseDetails.id, but the wallet token is also critical. Line 1088 validates bothidandtoken. For consistency, this validation should also check for the token.Apply this diff:
- if (!tenantDetails?.walletResponseDetails?.id) { + if (!tenantDetails?.walletResponseDetails?.id || !tenantDetails?.walletResponseDetails?.token) { this.logger.error(`Error in getting wallet id and wallet did`); throw new NotFoundException(ResponseMessages.agent.error.notAbleToSpinUpAgent, { cause: new Error(), description: ResponseMessages.errorMessages.notFound }); }
♻️ Duplicate comments (1)
apps/agent-service/src/agent-service.service.ts (1)
1088-1090: Fix the validation logic error.The condition uses
&&(AND) operators which is incorrect. IfwalletResponseDetailsis falsy, accessing.idor.tokenwill throw aTypeError. The validation should check if ANY of these are missing using||(OR).Apply this diff:
- if (!walletResponseDetails && !walletResponseDetails.id && !walletResponseDetails.token) { + if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { throw new InternalServerErrorException('Error while creating the wallet'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/agent-service.service.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/agent-service.service.ts (2)
apps/agent-service/src/interface/agent-service.interface.ts (4)
ICreateOrgAgent(449-451)ITenantDto(56-73)IUserRequestInterface(237-250)IStoreOrgAgentDetails(177-201)apps/connection/src/interfaces/connection.interfaces.ts (1)
IUserRequestInterface(21-33)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Take the required action to fix the issue indicated by this comment.
🔇 Additional comments (5)
apps/agent-service/src/agent-service.service.ts (5)
825-825: LGTM: Two-phase status flow.Setting
agentSpinUpStatustoPROCESSEDhere is correct. The status will be updated toCOMPLETEDwhencreateDidis called (lines 927-929), which aligns with the separation of wallet and DID creation.
1091-1093: LGTM: Simplified return value.The method now returns only
walletResponseDetailswithout DID information, which correctly reflects the separation of wallet and DID creation flows as intended by this PR.
745-758: I'll verify that callers can handle the new return type for thecreateTenantmethod. Let me search for all usages of this method.
<function_calls>
#!/bin/bashFind all call sites of createTenant method
rg -n -C3 'createTenant(' --type=ts --type=js
</function_calls>
<function_calls>
#!/bin/bashFind files that contain createTenant references (controllers, gateways, etc.)
fd -e ts -e js | xargs rg -l 'createTenant' | head -20
</function_calls>
927-929: I'll help you verify the agent status update behavior. Let me search for theupdateAgentSpinupStatusmethod implementation to understand what status it sets.
<function_calls>
#!/bin/bashSearch for the updateAgentSpinupStatus method definition
rg -n -A15 'updateAgentSpinupStatus' --type ts --type js
</function_calls>I need to search for the method implementation to verify the status update behavior. Let me refine the search:
<function_calls>
#!/bin/bashFirst, find files that contain updateAgentSpinupStatus
fd -e ts -e js | xargs rg -l 'updateAgentSpinupStatus'
Then search for the method definition with context
rg -n 'updateAgentSpinupStatus\s*(' -A20 --type ts
</function_calls>
444-446: Repository method signature verified and matches the service method call.The repository method
createOrgAgentatapps/agent-service/src/repositories/agent-service.repository.ts:103has been correctly updated to accept theorgIdparameter:async createOrgAgent(agentSpinUpStatus: number, userId: string, orgId: string): Promise<ICreateOrgAgent>This matches the service method call at line 446 which passes all three parameters in the correct order. The parameter passing is consistent across both call sites (lines 233 and 796).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/agent-service/src/agent-service.service.ts (2)
129-129: Status semantically misleading — async spinup not awaited.The return of
AgentSpinUpStatus.COMPLETEDis misleading since_agentSpinupat line 236 runs asynchronously withoutawait. The HTTP response claims completion before the actual spinup finishes. Consider returningIN_PROGRESSorPROCESSINGinstead.
146-148: Resolve the FIXME comment about ledgerName handling.The FIXME indicates an unresolved decision about whether to use the first element or process all elements of the
ledgerNamearray. Please clarify the intended behavior and remove the FIXME.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/agent-service.service.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/agent-service.service.ts (3)
apps/agent-service/src/interface/agent-service.interface.ts (4)
ICreateOrgAgent(449-451)ITenantDto(56-73)IUserRequestInterface(237-250)IStoreOrgAgentDetails(177-201)apps/ledger/src/schema/interfaces/schema.interface.ts (1)
IUserRequestInterface(5-18)apps/connection/src/interfaces/connection.interfaces.ts (1)
IUserRequestInterface(21-33)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Take the required action to fix the issue indicated by this comment.
🔇 Additional comments (5)
apps/agent-service/src/agent-service.service.ts (5)
444-446: LGTM!The
createOrgAgentsignature correctly updated to acceptorgId, and the repository call properly passes it through. This aligns with the flow changes to associate org agents with their organization from creation.
745-758: LGTM!The return type change to
IStoreOrgAgentDetailsprovides richer tenant/agent details to callers, aligning with the PR objective to simplify the wallet provisioning flow.
805-805: Good use of optional chaining.The
tenantDetails?.walletResponseDetails?.idsafely handles potential null/undefined values in the response chain.
824-840: Verify the intentional PROCESSED status in the new flow.The tenant is now stored with
AgentSpinUpStatus.PROCESSEDinstead ofCOMPLETED. Based on lines 927-929, the status gets updated toCOMPLETEDwhencreateDidis called and the agent is inPROCESSEDstate. This implements the new two-phase approach where wallet creation and DID creation are decoupled.Ensure this flow is documented and that API consumers understand they need to call
createDidto complete the provisioning.
927-929: Good addition for completing the provisioning flow.This correctly updates the agent status to
COMPLETEDafter DID creation when the agent was inPROCESSEDstate, completing the two-phase wallet provisioning flow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/agent-service/src/agent-service.service.ts (3)
120-134:walletProvisionstill returnsCOMPLETEDwhile spin‑up runs asynchronously.
walletProvisionawaitsprocessWalletProvisionbut that method starts_agentSpinupwithoutawait, so the HTTP call can return before the agent is actually spun up, while you returnAgentSpinUpStatus.COMPLETED(line 129). This can mislead API consumers who may assume the agent is ready.Consider either:
- returning a “PROCESSED/IN_PROGRESS”‑style status from
walletProvision, or- awaiting
_agentSpinupinsideprocessWalletProvisionsoCOMPLETEDtruly means done.Also applies to: 232-247
142-210: Clarify and resolve theledgerNameTODO to avoid future ambiguity.The TODO on line 146 about “get first element from ledgerName” is still unresolved while you pass
agentSpinupDto.ledgerName(possibly an array) directly intogetLedgerDetails. Please decide and encode the intended behavior (single ledger vs multi‑ledger) and remove the TODO, e.g.:
- If only one ledger is supported here, explicitly use
agentSpinupDto.ledgerName[0].- If multiple ledgers are valid, make that clear in the DTO and repository API and ensure downstream code handles multiple ledger IDs consistently.
745-763: Shared‑agent_createTenantcan still leave orphaned orgAgent records on failure.In
_createTenant, you create an orgAgent record (line 796) and later persist final details (line 838). If anything aftercreateOrgAgentfails (tenant wallet creation, encryption, storing details, etc.), the catch block only callsthis.handleError(...)and rethrows (lines 842‑845). There is no call toremoveOrgAgent(agentProcess.id), so failed attempts can leave a PROCESSED orgAgent row that blocks future spin‑ups with “walletAlreadyProcessing”.Recommend restoring explicit cleanup in this catch:
} catch (error) { this.handleError(error, payload.clientSocketId); - - throw error; + if (agentProcess && agentProcess.id) { + await this.agentServiceRepository.removeOrgAgent(agentProcess.id); + } + throw error; }This matches the pattern you already use in
_agentSpinupand avoids inconsistent state.Also applies to: 771-846
🧹 Nitpick comments (2)
apps/agent-service/src/agent-service.service.ts (2)
481-491: Ledger handling in_agentSpinupis reasonable;nullvs[]is a minor contract choice.Initializing
ledgerIdDatalazily and then mapping toledgerId: ledgerIdData ? ledgerIdData.map(...) : nullis functionally fine. Just ensure callers and DB schema treatnulland “no ledgers” consistently (previously this may have been[]), otherwise consider standardizing on an empty array.Also applies to: 542-557
1068-1094: Wallet response validation is now safe; optional chaining would further simplify it.The updated check:
if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token)fixes the earlier
&&bug and avoids accessing.id/.tokenon a nullish value; returning{ walletResponseDetails }(lines 1091‑1093) matches how_createTenantnow consumes it.If you want to follow the Sonar suggestion and reduce verbosity, you can use optional chaining:
if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/agent-service.service.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/agent-service.service.ts (4)
apps/agent-service/src/interface/agent-service.interface.ts (4)
ICreateOrgAgent(449-451)ITenantDto(56-73)IUserRequestInterface(237-250)IStoreOrgAgentDetails(177-201)apps/ledger/src/schema/interfaces/schema.interface.ts (1)
IUserRequestInterface(5-18)apps/connection/src/interfaces/connection.interfaces.ts (1)
IUserRequestInterface(21-33)apps/api-gateway/src/connection/interfaces/index.ts (1)
IUserRequestInterface(3-15)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1088-1088: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (2)
apps/agent-service/src/agent-service.service.ts (2)
233-233: Org‑scopedcreateOrgAgentlooks consistent with new flow.Passing
agentSpinupDto.orgIdintocreateOrgAgentand threadingorgIdthrough to the repository aligns org‑agent records with their organization and supports later cleanup/status updates. The try/catch with logging aroundcreateOrgAgentis straightforward and fine.Also applies to: 444-453
885-931: UpdatingagentSpinUpStatusto completed on successful DID creation is a good addition.The new block that calls
updateAgentSpinupStatus(orgId)whenagentDetails.agentSpinUpStatus === AgentSpinUpStatus.PROCESSED(lines 927‑929) cleanly ties the state transition to successful DID creation, aligning with the “wallet provision first, DID later” design.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/agent-service/src/agent-service.service.ts (2)
136-150: ClarifyledgerNameusage and tightenledgerIdDatatyping.
The TODO at Line 146 (
// TODO: Do we want to get first element from ledgerName) leaves behavior ambiguous. Currently you're passing the entireledgerNamearray (or[Ledgers.Indicio_Demonet]) intogetLedgerDetailsand later normalizingagentSpinupDto.ledgerNameto an array. Please either:
- Decide to support multiple ledgers and remove/replace the TODO with a clear comment that
ledgerNameis intentionally an array, or- If only one ledger is supported, change the call to something like:
this.agentServiceRepository.getLedgerDetails( agentSpinupDto.ledgerName?.[0] ?? Ledgers.Indicio_Demonet );This will also address the Sonar TODO warning.
In
_agentSpinup,let ledgerIdData;(Line 491) is implicitlyany. Since you later callledgerIdData.map((item) => item.id), consider typing it explicitly, e.g.:let ledgerIdData: ledgers[] | undefined;This makes the intent and shape clearer and avoids implicit-
anysurprises.Also applies to: 178-210, 491-491
771-846: Prevent tenant spin-up from getting stuck in PROCESSED and fix minor issues in_createTenant.Two behaviors here are worth tightening:
Orphaned
org_agentsrow on failure (can permanently block tenant creation).
_createTenantcreates an org agent row with statusPROCESSED:agentProcess = await this.agentServiceRepository.createOrgAgent( agentSpinUpStatus, user?.id, payload.orgId );but in the catch block you only call
this.handleError(error, payload.clientSocketId);and rethrow. There is no cleanup ofagentProcess. BecausecreateTenantshort-circuits when it seesAgentSpinUpStatus.PROCESSEDfor that org (Line 749), any failure aftercreateOrgAgent(e.g., during platform admin lookup, tenant wallet creation, or storing details) will leave the org in a permanently “processing” state until manually fixed in the DB.Consider explicitly removing or resetting that orgAgent entry in the catch:
} catch (error) {
this.handleError(error, payload.clientSocketId);throw error;
if (agentProcess?.id) {await this.agentServiceRepository.removeOrgAgent(agentProcess.id);}await this.handleError(error, payload.clientSocketId);throw error;This aligns the shared-tenant path with the dedicated-wallet path, where failures clean up partial state.
Unused
ledgerIdDatain_createTenant.
let ledgerIdData = [];is assigned fromgetLedgerDetails(Line 791) but never used afterwards. Either wire it into the stored data (if tenant records should track ledger IDs) or remove it to avoid confusion.Possibly invalid error key
notAbleToSpinUpAgent.In the wallet-id check:
throw new NotFoundException(ResponseMessages.agent.error.notAbleToSpinUpAgent, { ... });the key
notAbleToSpinUpAgentdoes not appear in the providedResponseMessages.agent.errorsnippet (there arenotAbleToSpinUp,notAbleToSpinup,notAbleToSpinp, etc.). If this key isn’t defined, the exception will carry a generic/not-intended message. Please either add the key tolibs/common/src/response-messages/index.tsor reuse one of the existing constants that best matches the intent.
🧹 Nitpick comments (3)
apps/agent-service/src/agent-service.service.ts (3)
120-133: Remove unusedagentProcessinwalletProvisionand delegate cleanup toagentWalletProvision.In
walletProvision,agentProcessis declared but never assigned; the catch block callshandleErrorOnWalletProvisionwithagentProcessalwaysundefined, so that method is effectively a no-op here. All real cleanup and socket error emission already happen insideagentWalletProvision/_agentSpinup.You can simplify and avoid confusion by removing the local
agentProcessand its use:- async walletProvision( + async walletProvision( agentSpinupDto: IAgentSpinupDto, user: IUserRequestInterface ): Promise<{ agentSpinupStatus: AgentSpinUpStatus; }> { - let agentProcess: ICreateOrgAgent; try { await this.agentWalletProvision(agentSpinupDto, user); return { agentSpinupStatus: AgentSpinUpStatus.COMPLETED }; } catch (error) { - this.handleErrorOnWalletProvision(agentSpinupDto, error, agentProcess); throw new RpcException(error.response ?? error); } }
444-447: ReusecreateOrgAgenthelper inside_createTenantfor consistency.You’ve introduced
createOrgAgentas a centralized helper wrapping the repository call with logging and error handling (Lines 444-447), but_createTenantstill callsthis.agentServiceRepository.createOrgAgent(...)directly (Line 795).For consistency and to avoid diverging behavior over time, consider reusing the helper:
- agentProcess = await this.agentServiceRepository.createOrgAgent(agentSpinUpStatus, user?.id, payload.orgId); + agentProcess = await this.createOrgAgent(agentSpinUpStatus, user?.id, payload.orgId);Also, typing
agentProcessin_createTenantasICreateOrgAgent | undefinedwould better document what this variable holds.Also applies to: 771-797
1068-1094: Validation ofwalletResponseDetailsis now safe; optional chaining can simplify per Sonar.The new guard:
if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { throw new InternalServerErrorException('Error while creating the wallet'); }correctly protects against null/undefined
walletResponseDetailsand missingid/token, fixing the earlier bug where.idcould be accessed on a nullish value.To satisfy the Sonar suggestion and make it a bit more idiomatic, you could express the same logic with optional chaining:
- if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { + if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/agent-service.service.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/agent-service.service.ts (3)
apps/agent-service/src/interface/agent-service.interface.ts (5)
IAgentSpinupDto(5-32)IUserRequestInterface(237-250)ICreateOrgAgent(449-451)ITenantDto(56-73)IStoreOrgAgentDetails(177-201)apps/connection/src/interfaces/connection.interfaces.ts (1)
IUserRequestInterface(21-33)apps/api-gateway/src/connection/interfaces/index.ts (1)
IUserRequestInterface(3-15)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1088-1088: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (1)
apps/agent-service/src/agent-service.service.ts (1)
885-931: Good alignment: advanceagentSpinUpStatusto COMPLETED only after DID creation.The new block in
createDid:if (agentDetails.agentSpinUpStatus === AgentSpinUpStatus.PROCESSED) { await this.agentServiceRepository.updateAgentSpinupStatus(orgId); }ensures the stored
agentSpinUpStatustransitions fromPROCESSEDtoCOMPLETEDonly after a DID has actually been created and stored. This matches the goal of decoupling wallet provisioning from DID creation while still reflecting a final “completed” state once DID setup finishes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/agent-service/src/agent-service.service.ts (2)
128-129: Return statusCOMPLETEDremains semantically misleading.The method awaits
provisionWallet, butprovisionWalletinternally calls_agentSpinupwithoutawait(line 236). The actual agent spinup runs asynchronously and signals completion via WebSocket events, yet the HTTP response already claimsAgentSpinUpStatus.COMPLETED. Consider returningAgentSpinUpStatus.PROCESSEDorIN_PROGRESSto accurately reflect the asynchronous nature of the operation.
146-148: Resolve the TODO aboutledgerNamearray handling.The TODO comment indicates an unresolved decision about how to handle the
ledgerNamearray. The current code passes the array directly togetLedgerDetails, but it's unclear whether:
- Only the first element should be used
- All elements should be processed
- The array handling is intentionally correct
Please clarify the intended behavior and remove the TODO once resolved.
🧹 Nitpick comments (1)
apps/agent-service/src/agent-service.service.ts (1)
1088-1093: Consider using optional chaining for cleaner validation.The validation logic is now correct. For improved readability, consider using optional chaining as suggested by static analysis:
- if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { + if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/agent-service.service.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/agent-service.service.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (5)
IAgentSpinupDto(5-32)IUserRequestInterface(237-250)ICreateOrgAgent(449-451)ITenantDto(56-73)IStoreOrgAgentDetails(177-201)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1088-1088: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (5)
apps/agent-service/src/agent-service.service.ts (5)
444-446: LGTM!The
orgIdparameter threading throughcreateOrgAgentis correctly implemented. The method signature and repository call are consistent.
745-758: LGTM!The return type change from
IAgentSpinUpSatustoIStoreOrgAgentDetailsis correctly propagated throughcreateTenantand_createTenant. This provides richer tenant/agent details as intended by the PR.
802-811: LGTM!The simplified tenant creation flow correctly:
- Creates the tenant wallet via
createTenantAndNotify- Uses optional chaining (
tenantDetails?.walletResponseDetails?.id) for safe property access- Throws appropriate error when wallet creation fails
824-840: LGTM!Setting
agentSpinUpStatus: AgentSpinUpStatus.PROCESSEDcorrectly reflects the decoupled workflow where the wallet is created but DID creation is deferred. The status transitions toCOMPLETEDwhencreateDidis subsequently called (lines 927-929). This aligns with the PR objective to detach DID creation from wallet provisioning.
927-929: Status transition to COMPLETED is correctly implemented.The
updateAgentSpinupStatusmethod properly updates the agent spinup status fromPROCESSEDtoCOMPLETEDwhen DID creation is successful. The logic at lines 927-929 correctly completes the decoupled wallet/DID creation workflow.
apps/agent-service/src/repositories/agent-service.repository.ts
Outdated
Show resolved
Hide resolved
apps/agent-service/src/repositories/agent-service.repository.ts
Outdated
Show resolved
Hide resolved
| * @param orgId | ||
| */ | ||
| // eslint-disable-next-line camelcase | ||
| async updateAgentSpinupStatus(orgId: string): Promise<org_agents> { |
There was a problem hiding this comment.
I think most of the update function can be combined like the: updateLedgerId, updateTenantToken
Signed-off-by: pranalidhanavade <[email protected]>
…out did for shared agent Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
pranalidhanavade
force-pushed
the
feat/create-wallet-api-changes
branch
from
aefd1fe to
b05e0ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/agent-service/src/agent-service.service.ts (2)
128-129: Status COMPLETED is misleading—wallet provisioning completes before async agent spinup finishes.The method returns
AgentSpinUpStatus.COMPLETEDafter callingprovisionWallet, but the actual agent spinup process is initiated asynchronously at line 236 withoutawait. This means the HTTP response claims completion while the background spinup is still in progress, potentially misleading API consumers.Consider either:
- Return
AgentSpinUpStatus.IN_PROGRESSorAgentSpinUpStatus.PROCESSINGto reflect the asynchronous nature, OR- Await the
_agentSpinupcall to ensure completion before returning COMPLETED status.
851-854: Missing cleanup for orphaned agent record on error.If an error occurs in
_createTenantafteragentProcessis created at line 796, the catch block at lines 851-854 callshandleErrorbut does not clean up the orphaned agent record. ThehandleErrormethod (lines 1186-1193) only emits a socket event and doesn't remove the agent.This could leave the database in an inconsistent state with a PROCESSED agent that never completed.
Apply this diff to add cleanup:
} catch (error) { this.handleError(error, payload.clientSocketId); + + if (agentProcess?.id) { + await this.agentServiceRepository.removeOrgAgent(agentProcess.id); + } + throw error; }apps/api-gateway/src/agent-service/dto/create-tenant.dto.ts (1)
27-28: Add validation decorators or exclude orgId from client input.The
orgIdproperty still lacks validation/serialization decorators. Based on past review comments, this remains unresolved:
- If
orgIdis expected from the client request body, add@ApiProperty(),@IsString(), and@IsNotEmpty()decorators.- If
orgIdis set server-side (which seems likely based on the AI summary), use@Exclude()fromclass-transformerto prevent client injection.Apply this diff if
orgIdshould be excluded from client input:+import { Exclude } from 'class-transformer'; + export class CreateTenantDto { ... + @Exclude() orgId: string; }Or if
orgIdis client-provided:+ @ApiProperty() + @IsString({ message: 'orgId must be in string format.' }) + @IsNotEmpty({ message: 'orgId is required.' }) orgId: string;
🧹 Nitpick comments (1)
apps/agent-service/src/agent-service.service.ts (1)
1097-1099: Consider optional chaining for cleaner validation.The validation logic is now correct with OR operators. However, you can make it more concise using optional chaining:
Apply this diff:
- if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { + if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/agent-provisioning/src/agent-provisioning.service.ts(1 hunks)apps/agent-service/src/agent-service.controller.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(11 hunks)apps/agent-service/src/repositories/agent-service.repository.ts(6 hunks)apps/api-gateway/src/agent-service/dto/create-tenant.dto.ts(1 hunks)libs/common/src/response-messages/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/agent-service/src/agent-service.controller.ts
- apps/agent-provisioning/src/agent-provisioning.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api-gateway/src/agent-service/dto/create-tenant.dto.ts (3)
libs/validations/maxLength.ts (1)
MaxLength(3-28)libs/common/src/cast.helper.ts (1)
trim(39-43)libs/validations/minLength.ts (1)
MinLength(3-28)
apps/agent-service/src/agent-service.service.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (4)
IAgentSpinupDto(5-32)IUserRequestInterface(237-250)ICreateOrgAgent(449-451)IStoreOrgAgentDetails(177-201)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1097-1097: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (7)
libs/common/src/response-messages/index.ts (1)
226-226: LGTM! Clear and user-friendly message.The updated success message is more descriptive and aligns well with the simplified wallet provisioning flow described in the PR objectives.
apps/agent-service/src/repositories/agent-service.repository.ts (3)
103-120: LGTM! Threading orgId through agent creation.The addition of the
orgIdparameter tocreateOrgAgentproperly establishes the organization relationship at agent creation time, which aligns with the simplified wallet provisioning flow.
159-211: LGTM! Enriched return type provides better context.The expanded return type from
storeOrgAgentDetailsnow includes essential agent details (orgId, agentEndPoint, tenantId, walletName) in addition to the ID and status. This richer data shape supports the simplified wallet provisioning flow by allowing callers to access all necessary tenant/agent details without additional queries.The consistent select clauses in both create and update paths ensure uniform behavior.
263-282: LGTM! Clean method for updating agent spinup status.The new
updateAgentSpinupStatusmethod provides a focused API for transitioning agents to COMPLETED status. The implementation is straightforward with appropriate error handling.apps/agent-service/src/agent-service.service.ts (3)
745-763: LGTM! Enriched return type provides better tenant details.The updated
createTenantsignature now returnsIStoreOrgAgentDetailsinstead of a simple status object, providing callers with comprehensive tenant and agent information. This aligns well with the simplified wallet provisioning flow.
771-855: LGTM! Simplified tenant creation with richer response.The
_createTenantmethod now returns complete organization agent details, streamlining the tenant provisioning workflow. The removal of DID creation and related socket notifications from this flow aligns with the PR objective to decouple DID creation from wallet provisioning.
936-938: LGTM! Proper status transition after DID creation.The logic to update agent spinup status to COMPLETED after DID creation (when status is PROCESSED) properly completes the agent provisioning lifecycle. This aligns with the decoupled DID creation workflow.
Signed-off-by: pranalidhanavade <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/agent-service/src/agent-service.service.ts (3)
120-134: Status COMPLETED is misleading—agent spinup runs asynchronously.The method returns
AgentSpinUpStatus.COMPLETEDafter awaitingprovisionWallet(line 128), butprovisionWalletcalls_agentSpinupwithout await (line 236). The actual agent spinup runs asynchronously and signals completion via websocket events. The HTTP response status should reflectIN_PROGRESSorPROCESSINGinstead ofCOMPLETEDto accurately represent the asynchronous nature of the operation.
146-148: Resolve the TODO comment about ledgerName handling.The TODO indicates uncertainty about whether to use the first element or the entire
ledgerNamearray. The current implementation passes the full array (or a default array) togetLedgerDetails, which suggests multi-ledger support is intended. Either remove the TODO if the current approach is correct, or clarify and implement the intended behavior explicitly.Based on static analysis.
851-854: Missing cleanup for orphaned agent record on error.If an error occurs after creating the
agentProcessrecord (line 796), the catch block (line 852) callshandleErrorbut doesn't remove the orphaned agent record. This could leave the database in an inconsistent state with aPROCESSEDagent that never completed.Apply this diff to add cleanup:
} catch (error) { this.handleError(error, payload.clientSocketId); + + if (agentProcess?.id) { + await this.agentServiceRepository.removeOrgAgent(agentProcess.id); + } + throw error; }
🧹 Nitpick comments (1)
apps/agent-service/src/agent-service.service.ts (1)
1097-1102: LGTM! Validation logic fixed and return simplified.The validation at line 1097 now correctly uses
||operators to short-circuit whenwalletResponseDetailsor its required properties are missing. The return statement is also simplified to return onlywalletResponseDetails.Optional improvement: Consider using optional chaining for even cleaner validation:
if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }Based on static analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/agent-service/src/agent-service.service.ts(11 hunks)apps/agent-service/src/repositories/agent-service.repository.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/repositories/agent-service.repository.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (2)
ICreateOrgAgent(449-451)IStoreOrgAgentDetails(177-201)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1097-1097: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (6)
apps/agent-service/src/repositories/agent-service.repository.ts (3)
103-120: LGTM! Signature updated to include orgId.The method now correctly accepts and persists
orgIdduring agent creation, aligning with the PR objective to thread organization context through the provisioning flow.
159-211: LGTM! Return type expanded to provide richer tenant details.The method now returns a comprehensive object containing
orgId,agentSpinUpStatus,agentEndPoint,tenantId,walletName, andidinstead of just theid. Both create and update paths consistently select the same fields, which aligns with the updated controller/service layer expectations.
263-282: LGTM! New method to mark agent spinup completion.The
updateAgentSpinupStatusmethod correctly updates the agent status toCOMPLETEDfor a given organization. The implementation is straightforward with appropriate error logging.apps/agent-service/src/agent-service.service.ts (3)
444-453: LGTM! Method signature updated to include orgId.The
createOrgAgentservice method now correctly accepts and passesorgIdto the repository layer, maintaining consistency across the provisioning flow.
936-938: LGTM! Agent spinup status updated upon DID creation.When a DID is created and the agent status is still
PROCESSED, this logic correctly updates it toCOMPLETED. This aligns with the PR objective to detach DID creation from the wallet provisioning flow, allowing the agent status to be completed only after explicit DID creation.
824-834: LGTM! Tenant creation sets status to PROCESSED, awaiting DID creation.The tenant creation flow correctly sets
agentSpinUpStatustoPROCESSEDrather thanCOMPLETED. This aligns with the PR objective to detach DID creation—the agent spinup will be markedCOMPLETEDonly when a DID is explicitly created viacreateDid(lines 936-938).
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/agent-service/src/repositories/agent-service.repository.ts (1)
159-178: Avoid wipingcreatedBy/lastChangedByon updates instoreOrgAgentDetails.On the update path (
idpresent),userIdis often not passed by callers (e.g._storeOrgAgentDetails,_createTenant), so:const { id, orgId, did, userId, ledgerId, didDoc, ...commonFields } = storeOrgAgentDetails; const data = { ...commonFields, orgId, ledgerId: firstLedgerId, createdBy: userId, lastChangedBy: userId, didDocument: didDoc, orgDid: did };will overwrite existing
createdByandlastChangedBywithundefined/null, losing audit information whenever an existing agent row is updated.Safer pattern: only set
createdByon create, and always setlastChangedBywhen you actually have a userId:- const data = { - ...commonFields, - orgId, - ledgerId: firstLedgerId, - createdBy: userId, - lastChangedBy: userId, - didDocument: didDoc, - orgDid: did - }; + const baseData = { + ...commonFields, + orgId, + ledgerId: firstLedgerId, + didDocument: didDoc, + orgDid: did + }; + + const data = id + ? { + ...baseData, + ...(userId && { lastChangedBy: userId }) + } + : { + ...baseData, + createdBy: userId, + lastChangedBy: userId + }; @@ - const query = id - ? this.prisma.org_agents.update({ - where: { id }, - data, + const query = id + ? this.prisma.org_agents.update({ + where: { id }, + data, @@ - : this.prisma.org_agents.create({ - data, + : this.prisma.org_agents.create({ + data,This preserves original
createdByon updates while still tracking the latest modifier.Also applies to: 181-207
apps/agent-service/src/agent-service.service.ts (1)
120-137: Agent spin-up/DID status is misleading vs actual flow; alignAgentSpinUpStatususage with detached DID creation.Right now:
walletProvisionreturns{ agentSpinupStatus: AgentSpinUpStatus.DID_CREATED }afterawait this.provisionWallet(...).provisionWallettriggers_agentSpinup(...)withoutawait, so the HTTP call completes before spin-up is actually done._agentSpinupstores org-agent details via_storeOrgAgentDetails, which hardcodesagentSpinUpStatus: AgentSpinUpStatus.DID_CREATEDin_buildStoreOrgAgentData.- Actual DID creation is now handled by
createDid, which also bumps status fromWALLET_CREATED→DID_CREATEDviaupdateAgentSpinupStatus.This leads to two problems:
The API reports
DID_CREATEDsynchronously even though:
- Agent spin-up is still in progress (fire-and-forget
_agentSpinup).- No DID has been created yet (that’s now a separate
createDidcall).The DB status for dedicated wallets is set to
DID_CREATEDas soon as wallet provisioning finishes, socreateDidcan run while the status already claims DID is created, andupdateAgentSpinupStatusis never used for that path.To match the “detach DID creation” goal and keep statuses meaningful:
- Treat
AgentSpinUpStatus.WALLET_CREATEDas “wallet provisioned/agent record created” andDID_CREATEDas “at least one DID successfully written and stored”.- In that model:
_buildStoreOrgAgentDatashould setagentSpinUpStatus: AgentSpinUpStatus.WALLET_CREATED.walletProvisionshould return{ agentSpinupStatus: AgentSpinUpStatus.WALLET_CREATED }(or evenPENDING) rather thanDID_CREATED.- Only
createDidshould advance status toDID_CREATEDviaupdateAgentSpinupStatusonce DID persistence succeeds.Concrete sketch:
// walletProvision - await this.provisionWallet(agentSpinupDto, user); - return { agentSpinupStatus: AgentSpinUpStatus.DID_CREATED }; + await this.provisionWallet(agentSpinupDto, user); + return { agentSpinupStatus: AgentSpinUpStatus.WALLET_CREATED }; // _buildStoreOrgAgentData - isDidPublic: true, - agentSpinUpStatus: AgentSpinUpStatus.DID_CREATED, + isDidPublic: true, + agentSpinUpStatus: AgentSpinUpStatus.WALLET_CREATED,With this, the lifecycle becomes:
PENDING → WALLET_CREATED(after wallet/agent provisioning) and only later→ DID_CREATEDviacreateDid, which keeps both API responses and DB state honest.Also applies to: 225-227, 474-555, 617-623, 887-932
🧹 Nitpick comments (3)
apps/agent-service/src/repositories/agent-service.repository.ts (2)
102-111: TypeagentSpinUpStatusasAgentSpinUpStatusinstead ofnumber.
createOrgAgentcurrently acceptsagentSpinUpStatus: numbereven though the enum is imported. Tightening this toAgentSpinUpStatuswill prevent accidental invalid values and better document the lifecycle.- async createOrgAgent(agentSpinUpStatus: number, userId: string, orgId: string): Promise<ICreateOrgAgent> { + async createOrgAgent( + agentSpinUpStatus: AgentSpinUpStatus, + userId: string, + orgId: string + ): Promise<ICreateOrgAgent> {
263-282:updateAgentSpinupStatusis fine; consider returning a narrowed projection.The update-by-
orgIdtoAgentSpinUpStatus.DID_CREATEDmatches the new enum and shared-wallet DID lifecycle. If callers only need status/orgId, you might return a narrowedselectsimilar tostoreOrgAgentDetailsfor consistency, but that’s an optional polish.apps/agent-service/src/agent-service.service.ts (1)
146-148: ResolveledgerNameTODO and optionally simplify wallet-response validation.A couple of small but worthwhile cleanups:
- Clarify
ledgerNamehandling and remove the TODOconst [platformConfig, getAgentType, ledgerIdData] = await Promise.all([ // ... this.agentServiceRepository.getLedgerDetails( - // TODO: Do we want to get first element from ledgerName - agentSpinupDto.ledgerName ? agentSpinupDto.ledgerName : [Ledgers.Indicio_Demonet] + agentSpinupDto.ledgerName && agentSpinupDto.ledgerName.length + ? agentSpinupDto.ledgerName + : [Ledgers.Indicio_Demonet] ) ]);Combined with:
agentSpinupDto.ledgerName = agentSpinupDto.ledgerName?.length ? agentSpinupDto.ledgerName : [Ledgers.Indicio_Demonet];you’re already treating
ledgerNameas an array with sensible defaulting. I’d suggest:
- Keeping the “array of ledger names” contract.
- Removing the TODO comment.
- Optionally adding a brief JSDoc on
IAgentSpinupDto.ledgerNameto state it’s an array and that all entries are resolved viagetLedgerDetails.
- Optional: use optional chaining for tenant wallet validation
The validation in
createTenantAndNotifyis now logically correct:if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { throw new InternalServerErrorException('Error while creating the wallet'); }To satisfy the static-analysis hint and improve readability, you could shrink it to:
- if (!walletResponseDetails || !walletResponseDetails.id || !walletResponseDetails.token) { + if (!walletResponseDetails?.id || !walletResponseDetails?.token) { throw new InternalServerErrorException('Error while creating the wallet'); }Behavior is equivalent, just cleaner.
Also applies to: 185-190, 200-203, 394-421, 1090-1095
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/agent-service/src/agent-service.service.ts(14 hunks)apps/agent-service/src/repositories/agent-service.repository.ts(6 hunks)libs/enum/src/enum.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/agent-service/src/repositories/agent-service.repository.ts (1)
apps/agent-service/src/interface/agent-service.interface.ts (2)
ICreateOrgAgent(449-451)IStoreOrgAgentDetails(177-201)
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/agent-service.service.ts
[warning] 146-146: Complete the task associated to this "TODO" comment.
[warning] 1090-1090: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🔇 Additional comments (1)
libs/enum/src/enum.ts (1)
88-91: Enum rename keeps numeric contract; semantics look consistent.Renaming to
WALLET_CREATED/DID_CREATEDwhile keeping values1/2preserves DB compatibility and clarifies intent around wallet vs DID lifecycle; callers just need to treat1as “wallet ready” and2as “DID created/agent fully ready”.
| if (AgentSpinUpStatus.WALLET_CREATED === getOrgAgent?.agentSpinUpStatus) { | ||
| this.logger.error(`Your wallet is already created.`); | ||
| throw new ConflictException(ResponseMessages.agent.error.walletAlreadyCreated, { | ||
| cause: new Error(), | ||
| description: ResponseMessages.errorMessages.conflict | ||
| }); | ||
| } | ||
|
|
||
| if (AgentSpinUpStatus.PROCESSED === getOrgAgent?.agentSpinUpStatus) { | ||
| this.logger.error(`Your wallet is already processing.`); | ||
| throw new ConflictException(ResponseMessages.agent.error.walletAlreadyProcessing, { | ||
| cause: new Error(), | ||
| description: ResponseMessages.errorMessages.conflict | ||
| }); | ||
| } | ||
|
|
||
| // Create tenant | ||
| this._createTenant(payload, user); | ||
| return agentStatusResponse; | ||
| const createdTenant = await this._createTenant(payload, user); | ||
| return createdTenant; | ||
| } catch (error) { |
There was a problem hiding this comment.
Prevent orphan org_agents on tenant creation errors and tighten “wallet already created” guard.
Two related issues in the shared-agent tenant path:
- Orphan
org_agentsrecords on failure
In _createTenant:
const agentSpinUpStatus = AgentSpinUpStatus.WALLET_CREATED;
agentProcess = await this.agentServiceRepository.createOrgAgent(agentSpinUpStatus, user?.id, payload.orgId);
// ...
const orgAgentDetails = await this.agentServiceRepository.storeOrgAgentDetails(storeOrgAgentData);
// ...
return orgAgentDetails;
} catch (error) {
this.handleError(error, payload.clientSocketId);
throw error;
}If any error occurs after agentProcess is created (e.g. tenant creation fails, NATS issues, invitation failure), the catch block calls handleError, which only emits a socket event and logs. The partially created org_agents row is never cleaned up, leaving the DB in an inconsistent state.
You already handle cleanup correctly elsewhere (_agentSpinup and handleErrorOnWalletProvision remove the agent on error). Do the same here:
} catch (error) {
- this.handleError(error, payload.clientSocketId);
- throw error;
+ this.handleError(error, payload.clientSocketId);
+
+ if (agentProcess?.id) {
+ try {
+ await this.agentServiceRepository.removeOrgAgent(agentProcess.id);
+ } catch (cleanupError) {
+ this.logger.error(
+ `[_createTenant] - Failed to cleanup org_agent ${agentProcess.id}: ${JSON.stringify(cleanupError)}`
+ );
+ }
+ }
+ throw error;
}- “Wallet already created” only checks
WALLET_CREATED
In createTenant:
const getOrgAgent = await this.agentServiceRepository.getAgentDetails(payload.orgId);
if (AgentSpinUpStatus.WALLET_CREATED === getOrgAgent?.agentSpinUpStatus) {
this.logger.error(`Your wallet is already created.`);
throw new ConflictException(ResponseMessages.agent.error.walletAlreadyCreated, ...);
}Once a DID is created and status is advanced to DID_CREATED, this guard no longer triggers, even though the org clearly already has a wallet. You probably want to block duplicate wallet creation for any non‑PENDING state, e.g.:
- if (AgentSpinUpStatus.WALLET_CREATED === getOrgAgent?.agentSpinUpStatus) {
+ if (
+ getOrgAgent &&
+ [AgentSpinUpStatus.WALLET_CREATED, AgentSpinUpStatus.DID_CREATED].includes(
+ getOrgAgent.agentSpinUpStatus
+ )
+ ) {
this.logger.error(`Your wallet is already created.`);
throw new ConflictException(ResponseMessages.agent.error.walletAlreadyCreated, {
cause: new Error(),
description: ResponseMessages.errorMessages.conflict
});
}This gives a consistent business rule and avoids leaking low-level DB uniqueness errors back to clients.
Also applies to: 764-848
🤖 Prompt for AI Agents
In apps/agent-service/src/agent-service.service.ts around lines 742 to 752, the
current logic throws a ConflictException only when agentSpinUpStatus ===
WALLET_CREATED and creates an org_agent before tenant creation without cleaning
it up on downstream failures; update two things: 1) tighten the pre-check to
treat any non-PENDING agentSpinUpStatus (e.g. DID_CREATED, WALLET_CREATED, or
any terminal state) as a conflict so duplicate wallet/tenant creation is blocked
consistently, and 2) ensure that any partial org_agent created during the
_createTenant path is removed on error — after creating the initial org_agent
record, wrap subsequent steps in try/catch and in the catch call the same
cleanup used elsewhere (remove/delete the org_agent record via the repository)
before rethrowing or handling the error and emitting socket events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/agent-service/src/repositories/agent-service.repository.ts (2)
263-283: Acknowledge TODO and recommend tracking.The new method correctly updates the agent status to
DID_CREATEDas part of the decoupled provisioning flow. The TODO comment (line 268) appropriately notes the opportunity to consolidate multiple single-field update methods (updateAgentSpinupStatus,updateLedgerId,updateTenantToken) into a generic update function.Consider creating an issue to track this refactoring to prevent the TODO from becoming stale.
286-286: Duplicate TODO comment.This TODO duplicates the one at line 268. Consider keeping a single TODO at the top of the related update methods or consolidating them into a single comment block to avoid redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-service/src/repositories/agent-service.repository.ts(6 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
apps/agent-service/src/repositories/agent-service.repository.ts
[warning] 286-286: Complete the task associated to this "TODO" comment.
[warning] 268-268: Complete the task associated to this "TODO" comment.
🔇 Additional comments (4)
apps/agent-service/src/repositories/agent-service.repository.ts (4)
1-27: LGTM!Import changes appropriately add the new interfaces (
ICreateOrgAgent,IStoreOrgAgentDetails,IStoreDidDetails) and enum values needed to support the updated method signatures throughout this repository.
103-120: LGTM!The addition of
orgIdto thecreateOrgAgentsignature and data payload correctly associates the agent record with the organization at creation time, aligning with the simplified wallet provisioning flow.
181-206: LGTM!The conditional query construction cleanly handles both update and create paths with identical
selectclauses, ensuring a consistent return shape. Returning the Promise directly is appropriate.
168-178: No action needed. TheledgerIdhandling on line 169 is correct and aligns with the interface definition (ledgerId?: string[]). The code properly handles both array and falsy values as intended.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.