Merged
Conversation
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR reactivates and registers a DIDComm Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Client
participant Controller as ProofController
participant Module as DIDComm Module
participant Agent as Agent
Client->>Controller: POST /didcomm/proofs/request-proof
Controller->>Module: requestProof(request, options)
Module->>Agent: perform DIDComm proof flow / create exchange
Agent-->>Module: returns DidCommProofExchangeRecord
Module-->>Controller: returns exchange record
Controller-->>Client: 200 Created (exchange record)
Client->>Controller: POST /didcomm/proofs/{id}/accept-request
Controller->>Module: selectCredentialsForRequest({proofExchangeRecordId})
Module->>Agent: query credentials & accept request
Agent-->>Module: updated exchange record
Module-->>Controller: updated record
Controller-->>Client: 200 Updated (accept confirmation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
GHkrishna
added
enhancement
RinkalBhojani
approved these changes
Apr 3, 2026
![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)
src/controllers/types.ts (1)
265-272:⚠️ Potential issue | 🔴 CriticalRegenerate
routes.tsto reflect the renamed property.The property in
AcceptProofProposalhas been renamed fromproofRecordIdtoproofExchangeRecordId(line 266), but the auto-generatedsrc/routes/routes.tsstill uses the old property name (line 2092). This mismatch will causeproofExchangeRecordIdto beundefinedat runtime when clients send requests validated against the stale schema.Run the TSOA route generation command (e.g.,
npx tsoa routes) to synchronize the auto-generated routes with the updated interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/types.ts` around lines 265 - 272, The generated routes are out of sync with the AcceptProofProposal interface: the property was renamed from proofRecordId to proofExchangeRecordId but routes.ts still expects proofRecordId, causing undefined values at runtime; regenerate the TSOA routes so the schema matches the updated interface (run the TSOA route generation, e.g., npx tsoa routes), verify routes.ts now references AcceptProofProposal.proofExchangeRecordId instead of proofRecordId, and commit the updated routes file.
🧹 Nitpick comments (1)
src/routes/routes.ts (1)
2076-2081: Prefer explicit proof format DTOs overanyfor request validation.At Line 2080, Line 2093, Line 2107, and Line 2121, using
dataType: "any"disables schema validation for core payloads and pushes failures deeper into controller/service logic.♻️ Suggested schema hardening
- "proofFormats": {"dataType":"any","required":true}, + "proofFormats": {"ref":"DidCommProofFormats","required":true},Apply the same change pattern across all proof request DTOs, then regenerate routes from typed controller contracts.
Also applies to: 2089-2094, 2102-2108, 2117-2122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/routes.ts` around lines 2076 - 2081, The schema uses dataType: "any" for the proofFormats payload in the RequestProofProposalOptions (and the other proof request DTOs noted), which disables validation; replace each "proofFormats": {"dataType":"any","required":true} with a reference to a new explicit DTO schema (e.g., "ProofFormats", "PresentationProofFormats" or similar) that enumerates the allowed structure, create those schema definitions in the same routes/schema block, update the referenced names in RequestProofProposalOptions and the other DTOs (the other occurrences you flagged) and then regenerate the routes from the typed controller contracts so the updated schema is enforced at validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/didcomm/proofs/ProofController.ts`:
- Around line 194-203: The response currently blanks out invitationDid when
createRequestOptions?.invitationDid is truthy, losing the actual DID; in
ProofController (around the return that builds
invitationUrl/invitation/outOfBandRecord) change the assignment for
invitationDid so it returns the real invitationDid value (the local variable
invitationDid or value from outOfBandRecord) instead of ''. Ensure the ternary
or conditional uses invitationDid when createRequestOptions?.invitationDid is
present so the response accurately reflects which DID was used to create the
invitation.
- Around line 221-239: The endpoint acceptRequest currently ignores the client's
filter flags; forward body.filterByPresentationPreview and
body.filterByNonRevocationRequirements into the call to
request.agent.modules.didcomm.proofs.selectCredentialsForRequest (the call that
builds requestedCredentials) so the selection honors those options, and ensure
the same symbols (proofExchangeRecordId, filterByPresentationPreview,
filterByNonRevocationRequirements) are passed in the options object;
alternatively remove those fields from the request body if you choose not to
support them.
- Around line 104-108: The acceptProposal endpoint declares a path param
'/:proofRecordId' but the acceptProposal method omits `@Path`('proofRecordId') and
never validates it, allowing mismatched IDs; fix by adding a
`@Path`('proofRecordId') proofRecordId: string parameter to the method (matching
how acceptRequest and acceptPresentation do it), verify it equals
acceptProposal.proofExchangeRecordId (throw 400 if mismatch), and pass the
validated proofRecordId or the acceptProposal payload to
request.agent.modules.didcomm.proofs.acceptProposal so the route and OpenAPI
spec are consistent.
- Around line 78-83: The controller currently forces protocolVersion: 'v1' when
calling request.agent.modules.didcomm.proofs.proposeProof in the proposeProof
method, which blocks v2-only proof formats; update the endpoint to either accept
protocolVersion from the RequestProofProposalOptions payload (add
protocolVersion to the RequestProofProposalOptions type and pass it through to
proposeProof) or validate that the incoming proofFormats are v1-compatible
before hardcoding 'v1' (e.g., check proofFormats contents in proposeProof and
throw/return a clear error if non-v1 formats like anonCreds/DIF are present);
reference proposeProof, RequestProofProposalOptions, protocolVersion, and
proofFormats when implementing the change so callers can choose v1/v2 or the
controller enforces v1-only formats.
In `@src/routes/routes.ts`:
- Around line 2089-2093: The AcceptProofProposal contract currently allows the
proofRecordId to come from both the URL path (:proofRecordId) and the request
body (AcceptProofProposal.proofRecordId), creating a mismatch risk; fix by
making the path param the single source of truth: remove proofRecordId from the
AcceptProofProposal body schema/DTO, update the controller method
signature/decorator for the accept-proposal handler to accept proofRecordId via
`@Path` (or equivalent) and keep proofFormats in the body, then regenerate the
TSOA routes so the generated routes.ts uses only the path parameter for
proofRecordId.
---
Outside diff comments:
In `@src/controllers/types.ts`:
- Around line 265-272: The generated routes are out of sync with the
AcceptProofProposal interface: the property was renamed from proofRecordId to
proofExchangeRecordId but routes.ts still expects proofRecordId, causing
undefined values at runtime; regenerate the TSOA routes so the schema matches
the updated interface (run the TSOA route generation, e.g., npx tsoa routes),
verify routes.ts now references AcceptProofProposal.proofExchangeRecordId
instead of proofRecordId, and commit the updated routes file.
---
Nitpick comments:
In `@src/routes/routes.ts`:
- Around line 2076-2081: The schema uses dataType: "any" for the proofFormats
payload in the RequestProofProposalOptions (and the other proof request DTOs
noted), which disables validation; replace each "proofFormats":
{"dataType":"any","required":true} with a reference to a new explicit DTO schema
(e.g., "ProofFormats", "PresentationProofFormats" or similar) that enumerates
the allowed structure, create those schema definitions in the same routes/schema
block, update the referenced names in RequestProofProposalOptions and the other
DTOs (the other occurrences you flagged) and then regenerate the routes from the
typed controller contracts so the updated schema is enforced at validation time.
🪄 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: 5b44f2ae-1823-45cc-8aa4-d1a828296d95
📒 Files selected for processing (4)
src/controllers/didcomm/proofs/ProofController.tssrc/controllers/types.tssrc/routes/routes.tssrc/routes/swagger.json
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>
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
3 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.
What
Summary by CodeRabbit
New Features
Enhancements
Documentation
Breaking
'/:proofRecordId/accept-proposal', the keyproofRecordIdhas been updated toproofExchangeRecordId/:proofRecordId/accept-proposalis changed to/accept-proposalNote: This is because
proofRecordIdis not being used in the API. An equivalent key is instead accepted in body.