fix: nats call microservices#1514
Conversation
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@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 WalkthroughThis PR systematically refactors NATS communication patterns across multiple services by replacing custom Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/api-gateway/src/connection/connection.service.ts (1)
39-49: Apply consistent error handling pattern across all methods.The
sendBasicMessage(line 47) andgetUrl(line 63) methods still use the olderror.responsepattern, whilesendQuestion(line 35) now uses the safererror?.response ?? errorpattern. This inconsistency should be resolved.Apply this diff to standardize error handling in
sendBasicMessage:} catch (error) { - throw new RpcException(error.response); + throw new RpcException(error?.response ?? error); }Apply this diff to standardize error handling in
getUrl:} catch (error) { - throw new RpcException(error.response); + throw new RpcException(error?.response ?? error); }Also applies to: 56-65
apps/cloud-wallet/src/cloud-wallet.service.ts (3)
151-151: Fix malformed URL template - extra closing brace.There's an extra
}at the end of the template string that would produce a malformed URL.Apply this diff:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}`;
173-173: Fix malformed URL template - extra closing brace.Same issue as line 151: extra
}at the end of the template string.Apply this diff:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}`;
267-279: Missing await on async encryption - stores Promise instead of value.Line 274 calls
this.commonService.dataEncryption()withoutawait, but the same method is awaited on lines 63-66 and 258. This will store a Promise object inagentApiKeyinstead of the encrypted string, breaking subsequent operations.Apply this diff:
const cloudWalletResponse: ICloudWalletDetails = { createdBy: userId, label, lastChangedBy: userId, tenantId: createCloudWalletResponse.id, type: CloudWalletType.SUB_WALLET, userId, - agentApiKey: this.commonService.dataEncryption(createCloudWalletResponse.token), + agentApiKey: await this.commonService.dataEncryption(createCloudWalletResponse.token), agentEndpoint, email, key: walletKey, connectionImageUrl };apps/agent-service/src/agent-service.service.ts (1)
497-517: Rewrite the review comment to confirm the critical issue is verified.The issue in the review comment has been verified as accurate and critical. The actual implementation at lines 709-720 shows that
_getALlLedgerDetails()has a catch block that only logs the error without a return statement, causing it to implicitly returnundefined. Both call sites (lines 501 and 791) immediately invoke.find()on the result without null checks, which will throw a runtime error if the NATS request fails.
Guard against undefined ledgerList when NATS ledger retrieval fails
The
_getALlLedgerDetails()method (lines 709–720) has a catch block that logs errors but does not rethrow or return a safe value, causing it to implicitly returnundefined. Both call sites—at line 501 and in_createTenantat line 791—immediately call.find(...)on the result without null checks, triggering a runtime error if the NATS request fails.Recommended fix:
- Modify
_getALlLedgerDetails()to rethrow the error or return an empty array:async _getALlLedgerDetails(): Promise<ILedger[]> { try { const pattern = { cmd: 'get-all-ledgers' }; const payload = {}; const result = await this.natsClient.send<ILedger[]>(this.agentServiceProxy, pattern, payload); return result; } catch (error) { - this.logger.error(`[natsCall] - error in while fetching all the ledger details : ${JSON.stringify(error)}`); + this.logger.error( + `[natsCall] - error while fetching all the ledger details : ${JSON.stringify(error)}` + ); + throw error; } }
🧹 Nitpick comments (9)
apps/ledger/src/schema/schema.service.ts (1)
405-408: TODO comment appropriately documents future refactor.The comment correctly identifies the nested mapping pattern that should be removed as part of the broader refactoring effort. When implementing this, you'll also need to update the method return type to
Promise<string>and adjust callers (e.g., line 200 that accesses.response).Do you want me to generate the complete refactoring changes for this method, or would you prefer to open an issue to track this across all three locations?
apps/connection/src/connection.controller.ts (1)
67-67: Unused parameter in payload type.The
userIdparameter is included in the payload type but is never destructured or used in the method body. Consider removing it from the type definition if it's not needed by the service.Apply this diff if the parameter is truly unused:
- async getConnectionRecordsByOrgId(payload: { orgId: string; userId: string }): Promise<number> { + async getConnectionRecordsByOrgId(payload: { orgId: string }): Promise<number> {apps/connection/src/connection.service.ts (1)
576-595: Avoid usinganytype; specify a more precise return type.The method uses
Promise<any>as the return type with a linter suppression comment. Consider defining a proper interface for the out-of-band connection invitation response structure to improve type safety.If the response structure is known, define an interface:
interface IOutOfBandInvitationResult { invitationUrl: string; invitationDid?: string; outOfBandRecord: { id: string; // ... other fields }; }Then update the return type:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - ): Promise<any> { + ): Promise<IOutOfBandInvitationResult> {apps/verification/src/verification.controller.ts (1)
48-50: Consider defining explicit response types for better type safety.The return type has been broadened from
Promise<string>toPromise<object>. While this aligns with the NATS client refactoring, it reduces type safety for API consumers. Consider defining a specific interface for the proof presentation response structure.For example:
export interface IProofPresentationResponse { // Define the actual response structure here // based on what the agent service returns } async getProofPresentationById(payload: { proofId: string; orgId: string; user: IUserRequest }): Promise<IProofPresentationResponse> { return this.verificationService.getProofPresentationById(payload.proofId, payload.orgId); }apps/verification/src/verification.service.ts (2)
571-578: Consider using consistent return types across similar methods.These methods use
Promise<unknown>while most other refactored methods usePromise<object>. For consistency and clarity, consider standardizing on eitherobjectorunknownthroughout the service layer. If there's a specific reason for usingunknownhere (e.g., more varied response structures), it would be helpful to document that.Example for consistency:
-async _sendOutOfBandProofRequest(payload: IProofRequestPayload): Promise<unknown> { +async _sendOutOfBandProofRequest(payload: IProofRequestPayload): Promise<object> { try { const pattern = { cmd: 'agent-send-out-of-band-proof-request' }; - const result = await this.natsClient.send(this.verificationServiceProxy, pattern, payload); + const result = await this.natsClient.send<object>(this.verificationServiceProxy, pattern, payload); return result;Also applies to: 676-688
938-952: Consider using a more type-safe return type.The method uses
Promise<any>as the return type, which provides no type safety. Consider usingPromise<unknown>or defining a specific interface for the verified proof details structure to improve type safety and developer experience.Example:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - async _getVerifiedProofDetails(payload: IVerifiedProofData): Promise<any> { + async _getVerifiedProofDetails(payload: IVerifiedProofData): Promise<unknown> { try { //nats call in agent for fetch verified proof details const pattern = { cmd: 'get-agent-verified-proof-details' }; const result = await this.natsClient.send(this.verificationServiceProxy, pattern, payload); return result;apps/agent-service/src/interface/agent-service.interface.ts (1)
560-572: Clarify ILedger vs ILedgers to avoid type confusion
ILedger(string dates,networkUrl: string | null) coexists withILedgers(Date fields, non‑nullablenetworkUrl). This split is probably intentional (transport DTO vs ORM/domain), but the nearly identical names can be confusing.Consider either:
- Brief comment explaining when to use
ILedgervsILedgers, or- Renaming one to make the distinction explicit (e.g.,
ILedgerDto).apps/agent-service/src/agent-service.service.ts (2)
515-517: Wallet provision NATS call is correctly factored, but consider tightening null checksUsing
_walletProvisionwith a typedPartial<IStoreOrgAgent>and then validatingif (!agentDetails)is reasonable. Given this is a cross‑service NATS call, you may also want to assert that the minimal required fields are present (e.g.,agentEndPointandagentToken) before proceeding to buildagentEndPointand fetch the base wallet token, to fail fast on partial responses.For example:
-const agentDetails = await this._walletProvision(walletProvisionPayload); -if (!agentDetails) { +const agentDetails = await this._walletProvision(walletProvisionPayload); +if (!agentDetails || !agentDetails.agentEndPoint || !agentDetails.agentToken) { this.logger.error(`Agent not able to spin-up`); throw new BadRequestException(ResponseMessages.agent.error.notAbleToSpinup, { cause: new Error(), description: ResponseMessages.errorMessages.badRequest }); }Please confirm the wallet-provisioning NATS handler indeed returns
agentEndPointandagentTokenfields matchingIStoreOrgAgentso these checks are appropriate.
722-733: _walletProvision NATS call: good error propagation, consider standardizing log prefixThe wallet provisioning helper correctly:
- Uses
this.natsClient.send<Partial<IStoreOrgAgent>>(...), and- Logs and rethrows on error, so callers can handle failure.
For consistency with other logs, you might want to keep the
[natsCall]prefix but also include the command name to make log analysis easier, e.g.:this.logger.error( `[natsCall][wallet-provisioning] Error in wallet provision : ${JSON.stringify(error)}` );Behavior-wise, this looks fine.
Since this depends on the NATSClient API, please confirm that
send<Partial<IStoreOrgAgent>>is the intended method and that the return type matches the actual message handler response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/agent-service/src/agent-service.service.ts(5 hunks)apps/agent-service/src/interface/agent-service.interface.ts(2 hunks)apps/api-gateway/src/connection/connection.service.ts(1 hunks)apps/cloud-wallet/src/cloud-wallet.service.ts(1 hunks)apps/connection/src/connection.controller.ts(4 hunks)apps/connection/src/connection.service.ts(21 hunks)apps/ledger/src/schema/schema.service.ts(3 hunks)apps/verification/src/interfaces/verification.interface.ts(1 hunks)apps/verification/src/verification.controller.ts(3 hunks)apps/verification/src/verification.service.ts(21 hunks)apps/webhook/src/webhook.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: GHkrishna
Repo: credebl/platform PR: 1509
File: apps/oid4vc-verification/src/oid4vc-verification.service.ts:368-372
Timestamp: 2025-11-11T04:24:30.878Z
Learning: In the NATSClient class (libs/common/src/NATSClient.ts), the sendNatsMessage method accepts a command as a string parameter and internally converts it to the object pattern format { cmd: string } before sending via NATS, so callers should pass the command as a plain string (e.g., 'agent-create-oid4vp-verifier') rather than an object.
📚 Learning: 2025-11-11T04:24:30.878Z
Learnt from: GHkrishna
Repo: credebl/platform PR: 1509
File: apps/oid4vc-verification/src/oid4vc-verification.service.ts:368-372
Timestamp: 2025-11-11T04:24:30.878Z
Learning: In the NATSClient class (libs/common/src/NATSClient.ts), the sendNatsMessage method accepts a command as a string parameter and internally converts it to the object pattern format { cmd: string } before sending via NATS, so callers should pass the command as a plain string (e.g., 'agent-create-oid4vp-verifier') rather than an object.
Applied to files:
apps/verification/src/verification.service.tsapps/connection/src/connection.service.tsapps/agent-service/src/agent-service.service.ts
🔇 Additional comments (25)
apps/api-gateway/src/connection/connection.service.ts (1)
35-35: Improved error handling with null-safe fallback.The updated pattern
error?.response ?? errorcorrectly prevents potential null/undefined access errors and provides a sensible fallback whenerror.responseis absent.apps/ledger/src/schema/schema.service.ts (2)
433-436: TODO comment is consistent with the refactoring plan.Same nested mapping pattern as in
_createSchema. The caller at line 322 also accesses.responseand will need updating when this refactor is implemented.
558-561: TODO comment completes the set of three locations.This is the final method with the nested mapping pattern. All three TODOs now document the refactoring work consistently.
apps/webhook/src/webhook.service.ts (2)
4-7: LGTM! Imports correctly updated.The import changes properly reflect the removal of unused dependencies.
RpcExceptionis retained (used for error handling), whileInject,ClientProxy, andCommonServiceimports were correctly removed.
13-13: Dependency removal verified as correct.The verification confirms that neither
webhookProxynorcommonServiceare referenced anywhere in the webhook module. The constructor simplification is valid—the service only requireswebhookRepositoryfor webhook CRUD operations and uses direct HTTPfetchcalls for webhook delivery. No breaking changes introduced.apps/cloud-wallet/src/cloud-wallet.service.ts (1)
43-47: LGTM! Cleanup of unused dependencies.The removal of the NATS client proxy and cache manager injections is appropriate since this service doesn't use either. The constructor now only includes the dependencies actually used throughout the service.
apps/connection/src/connection.controller.ts (2)
49-52: LGTM! Return type properly reflects the service response.The method now correctly returns
IConnectionListinstead ofPromise<string>, aligning with the updated service implementation that returns unwrapped connection list data.
106-107: Excellent catch! Typo fixed in method call.The method call was corrected from
sendBasicMesage(misspelled) tosendBasicMessage, which aligns with the renamed service method.apps/connection/src/connection.service.ts (7)
226-238: LGTM! Return type correctly unwrapped.The method now returns
IConnectionListdirectly instead of a wrapped{ response: string }object, and the NATS client call properly types the response. This simplifies the API and improves type safety.
253-262: Improved error handling with null-safe operators.The error handling now uses optional chaining (
error?.error?.reason,error?.status) to safely access nested error properties, preventing potential runtime errors from undefined access.
282-287: Method simplified with direct return.The method now returns the NATS client call result directly without wrapping, which is cleaner and aligns with the updated return type
Promise<IConnectionDetailsById>.
289-301: Consistent NATS call pattern applied.The method has been updated to use the direct NATS client call pattern consistently with other methods in the service. Error logging and propagation remain intact.
339-364: Return type properly unwrapped.The
_receiveInvitationUrlmethod now returnsIReceiveInvitationResponsedirectly instead of wrapping it in a response object, and the NATS call is properly typed. This improves API consistency across the service.
650-678: Excellent fix! Method name typo corrected.The method was renamed from
sendBasicMesagetosendBasicMessage, fixing a spelling error. This aligns with the controller's corrected method call and improves code maintainability.
64-82: NATS client call pattern verified as correct.The
natsClient.send()method signature confirms it accepts three parameters:serviceProxy,pattern: object, andpayload. The code correctly passes the pattern object format{ cmd: 'agent-create-connection-legacy-invitation' }, which is the appropriate way to invoke this method. The usage is consistent across the codebase and matches the NATSClient implementation inlibs/common/src/NATSClient.ts.apps/verification/src/interfaces/verification.interface.ts (1)
238-246: LGTM! Interface extension aligns with service refactoring.The addition of
outOfBandRecordandproofRecordThIdfields to theIInvitationinterface properly supports the new NATS client response structure. The optional nature of these fields maintains backward compatibility.apps/verification/src/verification.service.ts (7)
184-193: LGTM! Correct NATS client migration.The method correctly migrates from the legacy
natsCallwrapper to directthis.natsClient.send<object>usage, eliminating the response wrapper and returning the payload directly.
224-248: LGTM! Error handling updated for new NATS client structure.The error handling correctly adapts to the new NATS client response format, using
error?.error?.reasoninstead of the previouserror.responsepattern. The optional chaining ensures safe property access.
278-372: LGTM! Array handling preserved with new return types.The method correctly maintains the distinction between single and array responses while adapting to the new
objectreturn types. The error handling for duplicate attributes is also well-implemented.
399-442: LGTM! Verify presentation methods correctly refactored.Both
verifyPresentationand_verifyPresentationcorrectly adopt the new NATS client pattern with appropriate error handling for the new response structure.
563-569: LGTM! Appropriately uses specific return type.This method correctly uses
Promise<string>as the return type since it specifically returns a URL string, demonstrating proper type specificity where the structure is well-known.
622-668: LGTM! Correct usage of new IInvitation interface.The method correctly accesses the newly added
outOfBandRecordandproofRecordThIdproperties directly from the NATS response, properly using optional chaining and aligning with the interface changes.
754-937: LGTM! Property access patterns correctly adapted.The method correctly updates all property access to use direct navigation with appropriate optional chaining, consistent with the removal of response wrappers from the NATS client refactoring.
apps/agent-service/src/interface/agent-service.interface.ts (1)
212-231: agentToken field on IStoreOrgAgent aligns with new walletProvision flowThe new optional
agentToken?: string;fits the_walletProvisionresult shape and its later use in_agentSpinup(agentDetails?.agentToken). No issues from a typing perspective.apps/agent-service/src/agent-service.service.ts (1)
57-60: Type imports for WalletDetails / ILedger / IStoreOrgAgent look consistentBringing these interfaces in from
./interface/agent-service.interfacealigns the service typings with the shared DTOs and matches their usages further down in this file.
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
|
I think we can get it merged as the sonar cube issues are mostly regarding duplication in agent-service of agent service microservice |
* fix: nats call from nats client for verification service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: ledger nats fix in agent-service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: remove unused variable Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: nats calls in connection service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: webhook service unwanted imports Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * chore: add todos to remove nested mapping Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: sonar cube issues Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: coderabbit suggestions Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* fix: nats call from nats client for verification service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: ledger nats fix in agent-service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: remove unused variable Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: nats calls in connection service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: webhook service unwanted imports Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * chore: add todos to remove nested mapping Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: sonar cube issues Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: coderabbit suggestions Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* fix: nats call from nats client for verification service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: ledger nats fix in agent-service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: remove unused variable Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: nats calls in connection service Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: webhook service unwanted imports Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * chore: add todos to remove nested mapping Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: sonar cube issues Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: coderabbit suggestions Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
2 participants
What
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor