TBD54566975 / tbdex

56 stars 25 forks source link

Introduce Webhooks / Callbacks #167

Closed mistermoe closed 7 months ago

mistermoe commented 11 months ago

Motivation

Currently, in order to receive new messages for a given exchange, Alice must poll the respective PFI's http api. A handful of people have brought up adding webhook / callback support.

Two approaches came to mind that may work.

1. replyTo property

We could consider adding a replyTo property to rfq.metadata or more generally message.metadata. replyTo is a fully qualified URI which effectively acts as a callback URL / webhook. The value of replyTo could either be a DID or just a URL.

if replyTo is present, a PFI would send any/all new messages for a given exchange to the supplied URL

2. DID service endpoint

Alternatively, this could happen implicitly by resolving the sender's DID and looking for a predefined service endpoint type. if the predefined service endpoint is present, a PFI would send any/all new messages for a given exchange to that service endpoint.


In order to support either approach, we'll need to specify how the webhook url works. e.g.

michaelneale commented 11 months ago

the replyTo feels more self documenting as it invites you to consider it as you construct your first message (and the replyTo could itself be a did I guess?). but not basing that on any super deep thinking.

bradleydwyer commented 11 months ago

We should while thinking through this design consider compatibility with DWMsgs/DWNs. HTTP URLs may couple tbDEX to HTTP.

michaelneale commented 11 months ago

good point @bradleydwyer - as if its an "alice" sending a message, her replyto/DID could point to a DWN to receive messages in a self-custodial situation.

edit: even if they aren't http the terminology "webhook" is probably ok if used (replyto is more explicit in that regard) come to think of it.

diehuxx commented 8 months ago

3. Move callback into HTTP API

Another approach is to move the callback URL up an abstraction into the HTTP API. For ex, we could update the POST /exchanges/:exchange_id/rfq endpoint request body to take both the RFQ itself plus a replyTo field.

Putting the callback URL into the HTTP API instead of the TBDex message has a couple benefits:

Against DID Service Endpoint

My least favorite option is the DID service endpoint approach because:

mistermoe commented 8 months ago

Alice may want to keep her callback URL private

good point!

mistermoe commented 8 months ago

@diehuxx i imagine we'd want something like a TbdexRequest then yeah? an object that contains message and in this case replyTo?

michaelneale commented 8 months ago

if the replyTo is itself a did URL, then it doesn't have to be coupled to http? then it could remain(?) in the message. I'm not sure about the callback url containing sensitive information, feels like it wouldn't be optimal if it was assumed to be sensitive.

diehuxx commented 8 months ago

i imagine we'd want something like a TbdexRequest then yeah? an object that contains message and in this case replyTo?

Exactly. Though, I'm not sure how much we care about keeping the notion of TbdexRequest standard across different HTTP endpoints. My gut instinct is it doesn't matter as much. What's more important is keeping each endpoint's payload backwards compatible over time.

diehuxx commented 8 months ago

if the replyTo is itself a did URL, then it doesn't have to be coupled to http?

I'm not sure if that would work with DWNs currently. Even if that does work, there's transports beyond http and did url that would be excluded.

then it could remain(?) in the message.

What's the benefit of doing so?

I'm not sure about the callback url containing sensitive information, feels like it wouldn't be optimal if it was assumed to be sensitive.

Not sure I understand what you mean by optimal?

michaelneale commented 8 months ago

@diehuxx yeah I might not really get the context here so feel free to ignore me!

the idea of using dids as they are universal (but then if the doc is heavy to update then that isn't ideal - but that may be an assumption, and a user can have dids for many purposes). Chanelling @csuwildcat here! (and it would mean you could have another did separate to sender with a service endpoint, but maybe that is too chonky of an idea!).

Remaining in the message is related to sensitive information: if there is sensitive information in the url, then isn't that about privacy and or trust, and in one of those cases it is better in the message than out of band? (again just being more general, not sure of specific case here).

michaelneale commented 8 months ago

@diehuxx i think I get you now I read it again. If we want it to be “web” hook then pulling replyTo into http api like you said may be simpler and the right thing while still allowing service endpoints at some point in future?

mistermoe commented 8 months ago

@diehuxx i'm on board.

technically speaking replyTo at the request level can support DID and vanilla URLs since we'll state the value of replyTo must be a valid URI.

this allows for people to leverage service + relativeRef DID Parameters like so: did:example:123?service=tbdex-callback&relativeRef=/callbacks/tbdex. cool thing here is that we don't have to be prescriptive at all about service ids or refs

at some point we can explore using replyTo to point to a DWN in the same manner described above though PFIs would need to explicitly support it via the DID Relative URLs section of the DWN spec here

phoebe-lew commented 8 months ago

+1 for proposal 3, although I would suggest naming the request something like createExchangeRequest or exchangeRequest over tbdexRequest, since it will contain the thing that kicks off an exchange (the Rfq), and exchange-level data.

@mistermoe and I talked through specifying callbacks at an exchange level rather than at a DID level...in summary, it feels like there could be some potential extra flexibility with how wallets want to leverage the callback URL, for not much of a UX tradeoff (looking at a one off registerCallback request vs populating a field with the URI per exchange).

diehuxx commented 8 months ago

(looking at a one off registerCallback request vs populating a field with the URI per exchange).

@phoebe-lew Making sure I understand. No pushback, just clarifying. By this do you mean we could have a separate endpoint called registerCallback that takes exchange_id and callback URI? Sounds like this would allow submitRfq to keep its current request structure. If this is what you mean, I'm into it.

phoebe-lew commented 8 months ago

@diehuxx nop, let's just go with proposal 3 and introduce a createExchangeRequest (or other name) endpoint, which will effectively replace the existing data structure for the request to submit an RFQ. It will wrap the RFQ itself and a callback URL (optional).

The registerCallback option is generally scoped to whatever your system's concept of a tenant is (probably a DID in our case), but allowing the caller to scope it per exchange allows for more flexibility.

We can always change it in future if we decide it's better to register the callback against the DID.

jiyoontbd commented 8 months ago

happy to implement option 3, but can someone tell me the difference between option 3 and option 1?

seems like both are suggesting that we add the replyTo field to the POST /exchanges/:exchange_id request body, except 1 suggests we put it in the metadata, and 3 suggests we put it in the RfqData. is that the only difference or am i missing something

diehuxx commented 8 months ago

@jiyoontbd Option 1 places replyTo in the TBDex Message (in the metadata) making it part of the protocol spec. Option 3 places replyTo in the HTTP request separate of the TBDex message, making it part of the http-api but not part of the underlying message protocol.

jiyoontbd commented 8 months ago

@jiyoontbd Option 1 places replyTo in the TBDex Message (in the metadata) making it part of the protocol spec. Option 3 places replyTo in the HTTP request separate of the TBDex message, making it part of the http-api but not part of the underlying message protocol.

@diehuxx oooh, i think i am understanding where i've misunderstood. do you mean put replyTo as an http request header, and not inside RfqData in a tbdex message? or are you thinking of some other way in which to incorporate replyTo as part of the http request

diehuxx commented 8 months ago

Doing a little bookkeeping because I often lose track of these things.

PRs in progress: Spec change -- thanks @phoebe-lew! https://github.com/TBD54566975/tbdex/pull/222 Javascript change -- thanks @jiyoontbd! https://github.com/TBD54566975/tbdex-js/pull/142

@amika-sq Flagging this issue so it's on your radar for tbdex-swift. See Phoebe's spec change PR for the new design.

diehuxx commented 8 months ago

@jiyoontbd Option 1 places replyTo in the TBDex Message (in the metadata) making it part of the protocol spec. Option 3 places replyTo in the HTTP request separate of the TBDex message, making it part of the http-api but not part of the underlying message protocol.

@diehuxx oooh, i think i am understanding where i've misunderstood. do you mean put replyTo as an http request header, and not inside RfqData in a tbdex message? or are you thinking of some other way in which to incorporate replyTo as part of the http request

That's mostly right. You're right that replyTo will not be in the RfqData. My only correction is that replyTo will be in the http request body of submitRfq, not the header.

Also (as noted in in @phoebe-lew's spec PR), we are changing the name of submitRfq to createExchange and changing the endpoint.

jiyoontbd commented 7 months ago

@amika-sq tagging you for tbdex swift work. please see tbdex-js and/or tbdex-kt impls for details

KendallWeihe commented 7 months ago

So I hate to come late to the party here and advocate for a pivot, but I feel compelled to articulate a stance for Option 1 actually. With full recognition it may not be worth pursuing, at least at this time, but I want to make the articulation here while it's fresh in my focus (I just came across this ticket).

Here's my alternative proposal:

Here are my reasons why I don't like where we landed:

Again, I just wanted to document this for future reference. Not a priority by any means, and can be ignored.