Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added support for DRPC protocol #1753

Merged
merged 24 commits into from
Feb 23, 2024

Conversation

wadeking98
Copy link
Contributor

@wadeking98 wadeking98 commented Feb 9, 2024

Usage:

Send DRPC request and wait for response:

const listenResponse = await aliceAgentmodules.drpc.sendRequest(connectionRecord.id, {
  jsonrpc:'2.0', 
  method:'hello', 
  id:1
})
const response = await listenResponse()

After the listener sends the response, the sender will receive a curried function that will resolve to the response

Listen for DRPC request and send response:

const { request, sendResponse } = await faberAgent.modules.drpc.recvRequest()
await sendResponse({
  jsonrpc:'2.0', 
  result:'Hello', 
  id:1
})

DRPC notification

to send a DRPC notification simply set the id to be null
the response will then be an empty object

DRPC batch requests

DRPC supports sending multiple request objects over a single connection

const batchListen = await aliceAgent.modules.drpc.sendRequest(aliceConnection.id, [
  { jsonrpc: '2.0', method: 'hello', id: 1 },
  { jsonrpc: '2.0', method: 'add', params: [2, 3, 7], id: 2 },
  { jsonrpc: '2.0', method: 'parseFoo', params: { foo: 'bar' }, id: 3 },
  { jsonrpc: '2.0', method: 'error', id: 4 },
])
const batchResponse = await batchListen()

the response will be an array of DRPC responses

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@amanji
Copy link
Contributor

amanji commented Feb 10, 2024

I may have interpreted the RFC differently, or maybe this is how it's intended to work in credo-ts, but in ACA-Py the agent doesn't directly handle the JSON-RPC object (i.e. it doesn't handle the RPC method execution) it simply forwards that through to a controller. The controller is responsible for handling the JSON-RPC accordingly and formulating a response that the agent will convey back.

I'm quoting this line from the Key Concepts of RFC 0804:

... The interpretation of the request, how to carry out the request, the content of the response, and the interpretation of the response, are all up to the business logic (controllers) of the participating agents...

Perhaps ACA-Py's implementation is more simplistic though, the agent simply handles the DRPC Request and Response messages and ensures it is valid in accordance with the JSON-RPC specification and the Aries DRPC Protocol.

@wadeking98
Copy link
Contributor Author

wadeking98 commented Feb 10, 2024

The interpretation of the request, how to carry out the request, the content of the response, and the interpretation of the response, are all up to the business logic (controllers)

I think it might be stretching the RFC a little bit, but I think it's still within the bounds of the RFC and it provides a nicer interface for the controller from my perspective.

From one perspective the agent is kind of doing the RPC interpretation because the handler function is being executed by the agent... but from another perspective it's the controller that's handling RPC execution since it's the controller's function that's being passed into the agent and used as the RPC handler.

It seems to me it could be argued either way based on semantics, but ultimately its the business logic that defines the handler and so its the business logic that decides "how the request is interpreted", "how to carry out the request", etc. even though the agent is the one executing the handler.

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98
Copy link
Contributor Author

Also the methodHandler approach provides some encapsulation which will make the business logic on the controller simpler. I'm open to forwarding all DRPC messages to the controller, but I think this approach makes things a bit tidier IMHO

@amanji
Copy link
Contributor

amanji commented Feb 10, 2024

The mechanism may still work with registering method handlers in the agent as you've implemented, so from that perspective it's just a different approach, not necessarily incorrect.

I'm just considering whether users would expect similar mechanics when encountering protocol implementations across the different frameworks, especially as described in the RFC. Ultimately, so long as the business logic can be handled or passed from outside the agent and will not constrain complex use cases, the protocol still works. Perhaps the wording in the RFC needs to be less prescriptive.

I don't think I have enough experience to give a definitive answer, or whether it matters to the community, so I would lean on them to weigh in. These are just my thoughts at a first glance.

Either way, I look forward to giving this a spin when's it's merged.

@amanji
Copy link
Contributor

amanji commented Feb 10, 2024

Also I didn't mean to post my first message three times 😅

@wadeking98
Copy link
Contributor Author

Maybe we could get @swcurran 's opinion on this?

Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wadeking98 ! I'm pretty excited about this new protocol and having it supported in Credo.

I've left a good amount of comments in the code but in general I think it will be important to put all this stuff in a separate module instead of adding it to core. It will need a bit of overhead but should not be hard if you take action-menu or question-answer as a base.

Other than that, I think the API is quite simple and I find interesting that you manage everything within the framework. It can be also a good idea to leave the possibility of handling methods from the controller side (e.g. a config setting that does not automatically respond according to the registered method handlers) but of course that can be left for a further PR in case is proven to be useful in real-world usage.

BTW I don't know what others think but I think 'DRPC' in class names should be changed to 'Dprc' to be more aligned with our coding style (e.g. classes with Did* or OpenId4Vc*

packages/core/src/modules/drpc/DRPCMessageRole.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessageEvents.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/messages/DRPCMessage.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/messages/DRPCMessage.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/messages/DRPCMessage.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
packages/core/src/agent/BaseAgent.ts Outdated Show resolved Hide resolved
packages/core/src/modules/drpc/DRPCMessagesApi.ts Outdated Show resolved Hide resolved
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98 wadeking98 force-pushed the feat-drpc-protocol branch 4 times, most recently from c022d20 to a2b5e7f Compare February 13, 2024 20:31
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98
Copy link
Contributor Author

@genaris @TimoGlastra The validate CI Stage is failing after updating the module README for some reason but other than that it's ready for re-review

@TimoGlastra
Copy link
Contributor

Did you run yarn format locally?

@wadeking98
Copy link
Contributor Author

Did you run yarn format locally?

That did the trick, should be all ready now

@wadeking98
Copy link
Contributor Author

@TimoGlastra @genaris would you mind doing a quick re-review when you have a second?

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor notes, mainly for consistency.

packages/core/src/modules/connections/ConnectionsApi.ts Outdated Show resolved Hide resolved
packages/drpc/CHANGELOG.md Outdated Show resolved Hide resolved
packages/drpc/src/DrpcModule.ts Show resolved Hide resolved
packages/drpc/src/DrpcRole.ts Outdated Show resolved Hide resolved
packages/drpc/src/DrpcState.ts Outdated Show resolved Hide resolved
packages/drpc/src/messages/DrpcRequest.ts Outdated Show resolved Hide resolved
packages/drpc/src/messages/DrpcResponse.ts Outdated Show resolved Hide resolved
packages/drpc/src/messages/DrpcResponse.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few more comments but overall looks great!

packages/drpc/src/DrpcModule.ts Show resolved Hide resolved
packages/drpc/src/DrpcState.ts Outdated Show resolved Hide resolved
packages/drpc/README.md Outdated Show resolved Hide resolved
packages/drpc/src/services/DrpcService.ts Outdated Show resolved Hide resolved
packages/drpc/src/repository/DrpcMessageRecord.ts Outdated Show resolved Hide resolved
packages/drpc/src/services/DrpcService.ts Outdated Show resolved Hide resolved
packages/drpc/tests/drpc-messages.e2e.test.ts Show resolved Hide resolved
packages/drpc/src/DrpcApi.ts Outdated Show resolved Hide resolved
packages/drpc/src/DrpcApi.ts Outdated Show resolved Hide resolved
packages/drpc/src/DrpcApi.ts Outdated Show resolved Hide resolved
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@genaris genaris merged commit 4f58925 into openwallet-foundation:main Feb 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants