adding `replyTo` field in http request body to submit RFQ #140

jiyoontbd · 2024-01-20T01:58:26Z

http-client:
Add replyTo field to send message request if it was included with RFQ message.
Updated endpoint per spec change.
Created an overloaded method to help elucidate between sendMessage() that sends a non-rfq message or an rfq message without a replyTo field, and sendMessage() that sends specifically an rfq message with a replyTo field

http-server
Updated endpoint per spec change. Now anticipating an optional replyTo field included in the request JSON.

…f exists). wrote tests for client change. rename submitrfq handler to createexchange httpserver handler. checking validity of url in the handler. wrote test for handler change

codecov · 2024-01-20T02:00:55Z

Codecov Report

Merging #140 (b074ee3) into main (c9eb53a) will increase coverage by 1.43%.
The diff coverage is 96.07%.

❗ Current head b074ee3 differs from pull request most recent head 9c27b39. Consider uploading reports for the commit 9c27b39 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   76.19%   77.63%   +1.43%     
==========================================
  Files          32       33       +1     
  Lines         752      778      +26     
  Branches       77       74       -3     
==========================================
+ Hits          573      604      +31     
  Misses        133      133              
+ Partials       46       41       -5

Components	Coverage Δ
protocol	`85.00% <ø> (+1.45%)`	⬆️
httpclient	`78.89% <93.93%> (+1.72%)`	⬆️

diehuxx · 2024-01-22T21:02:00Z

httpclient/src/main/kotlin/tbdex/sdk/httpclient/TbdexHttpClient.kt

 * @throws TbdexResponseException for request or response errors.
 */
- fun sendMessage(message: Message) {
+ fun sendMessage(sendMessageRequest: SendMessageRequest) {


This reads like javascript. We should check at compile-time that replyTo is only present with RFQs. Currently, this does the check at runtime. I'm wondering if there's a more kotlin-idiomatic way to write this.

What do you think of separating the methods?

// Sends messages, including RFQs, but doesn't support replyTo` sendMessage(message: Message) // Sends only RFQs and supports replyTo sendRfq(rfq: Rfq, replyTo: string?)

Or maybe overloaded methods if you want to keep the name sendMessage for both.

// Sends messages, including RFQs, but doesn't support replyTo` sendMessage(message: Message) // Sends only RFQs and supports replyTo sendMessage(rfq: Rfq, replyTo: string?)

i did sendMessage(message:Message, replyTo: String? = null), since sendMessage() is used anytime the client is sending any tbdex message. i think the second option can also work...

wondering what we want the dev ex to be like. should we separate it out at the method level, or should we let the client just use one method and this method takes care of the validation / inclusion of replyTo field in the request?

discussed with @diehuxx - going with the second approach, with sendMessage(rfq: Rfq, replyTo: string) for the overloaded method.

diehuxx · 2024-01-22T21:04:22Z

httpserver/src/main/kotlin/tbdex/sdk/httpserver/TbdexHttpServer.kt

@@ -99,7 +99,7 @@ class TbdexHttpServer(private val config: TbdexHttpServerConfig) {

 route("/exchanges") {
 post("/{exchangeId}/rfq") {


Update endpoint as per spec change
https://github.com/TBD54566975/tbdex/pull/222/files#diff-1ab7678648ffe33c3535373210080ab41cd2c38573890fdfcf008700ef14c3c7R279

ok. just a side q - would we leave the other endpoints as /exchanges/{exchangeId}/order|close?

httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/CreateExchange.kt

diehuxx · 2024-01-22T21:18:44Z

httpserver/src/test/kotlin/tbdex/sdk/httpserver/handlers/CreateExchangeTest.kt

 import tbdex.sdk.protocol.serialization.Json
 import kotlin.test.assertContains
 import kotlin.test.assertEquals

-class SubmitRfqTest : ServerTest() {
+class CreateExchangeTest : ServerTest() {
 @Test
 fun `returns BadRequest if no request body is provided`() = runBlocking {
 val response = client.post("/exchanges/123/rfq") {


Update endpoints in this file as per spec
https://github.com/TBD54566975/tbdex/pull/222/files#diff-1ab7678648ffe33c3535373210080ab41cd2c38573890fdfcf008700ef14c3c7R279

i think i'd like to hold off on updating the endpoints until we have the new tickets / PRs that phoebe referenced in this comment to update client and server code that incorporates the new endpoint style.

actually i should just get rid of the /rfq bit since we've decided that can go, but the one up the air is whether /{exchangeId} should go also. updating

diehuxx · 2024-01-22T21:26:19Z

httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/CreateExchange.kt

+ call.respond(HttpStatusCode.BadRequest, errorResponse)
+ return
+ }
+
 try {
 exchangesApi.getExchange(message.metadata.exchangeId.toString())
 val errorDetail = ErrorDetail(detail = "RFQ already exists.")


Tangential to the replyTo stuff, but can you update the catch (_: Exception) below to catch (_: NoSuchElementException) and add comment // exchangesApi.getExchange throws if no existing exchange is found. For a sec I was pretty confused why we were swallowing an error so egregiously.

Further musings: maybe this isn't the most graceful API for ExchangeApi. Personally I would return an empty list rather than throw. But that's a separate PR if we decide to do that.

fixed - yea, this try catch is a bit confusing!

in this request handler to create exchange, we want to make sure that exchangesApi.getExchange(exchangeId) does not return an exchange.

this try catch is actually hoping an NoSuchElementException is thrown when an exchange with the exchangeId is not found - which is what we want, because we want to create a new exchange

as you said, we should probably return an empty list if an exchange with the exchangeId is not found instead of throwing an exception.

…eExchange.kt Co-authored-by: Diane Huxley <dhuxley@tbd.email>

…es Rfq message with a replyTo field

diehuxx · 2024-01-24T22:38:43Z

httpclient/src/main/kotlin/tbdex/sdk/httpclient/TbdexHttpClient.kt

@@ -84,20 +87,41 @@ object TbdexHttpClient {
 val pfiServiceEndpoint = getPfiServiceEndpoint(pfiDid)
 val url = "$pfiServiceEndpoint/exchanges/$exchangeId/$kind"


If message is a Rfq, this will send to the wrong endpoint. We need to omit $kind if kind == 'rfq'.

absolutely correct, ty!

diehuxx · 2024-01-24T22:42:49Z

httpclient/src/main/kotlin/tbdex/sdk/httpclient/models/CreateExchangeRequest.kt

+ * @property rfq Rfq tbdex message received.
+ * @property replyTo Optional URL to be included in the request to create exchange.
+ */
+class CreateExchangeRequest(


Since this is only used in tests, can we move it into test/ directory? Or is it intended to be used by consumers of this library in some way?

we do use this in Tbdexhttpclient#sendMessage method where i'm passing in rfq and replyTo (the new overloaded method)

val body: RequestBody = Json.stringify(CreateExchangeRequest(message, replyTo)) .toRequestBody(jsonMediaType)

diehuxx

good work @jiyoontbd !

phoebe-lew · 2024-01-31T07:30:50Z

httpclient/src/main/kotlin/tbdex/sdk/httpclient/TbdexHttpClient.kt

- * @throws TbdexResponseException for request or response errors.
+ * @param message The message to send.
+ *
+ * @throws TbdexResponseException for response errors.
 */
 fun sendMessage(message: Message) {


@jiyoontbd what do you think about having a separate fun createExchange(rfq: RFQ, replyTo: String?) function for creating an exchange?

The reason being, there are now two functions that can be called to create an exchange. This one can accept messages of any type (including RFQ), but does not support replyTo URL. The other is also called sendMessage, but you can actually only send an RFQ, not just any message type.

You would essentially have one client method for creating an exchange, which can have a replyTo or not (optional), and a different method for all subsequent messages.

Does that make sense?

yea. so you're saying we'd call createExchange() for sending RFQ (with or without replyTo), and sendMessage() for everything else? i'll give that a go.

i wish we could type guard for sendMessage() to prevent RFQ from being passed in as message in kotlin. so without the type guard, i'd have to check the kind and throw IllegalArgumentException. i'm not sure if i like that dev ex. i suppose we could explicitly say pls don't use this method if you're sending an RFQ!!11 in the ktdocs.

i'm leaning towards keeping the overloaded sendMessage(), would love to hear @diehuxx 's thoughts

i suppose we could explicitly say pls don't use this method if you're sending an RFQ!!11 in the ktdocs.

I would really prefer not to resort to that. IMO as long as we're leveraging the type system to enforce correct message types and options at compile-time, I can be flexible about how we name methods.

If I were writing this, I would keep the overloaded sendMessage AND add a createExchange endpoint which takes an RFQ and a replyTo. Admittedly, my ruby background is showing in this suggestion, as aliasing methods is common in ruby.

Feel free to take this idea or not. As long as the type system can enforce our choices, @jiyoontbd I trust your judgement.

i'm good with that. the code editor should show the difference between the two. commit here

phoebe-lew · 2024-01-31T07:31:02Z

httpclient/src/main/kotlin/tbdex/sdk/httpclient/TbdexHttpClient.kt

@@ -208,4 +237,25 @@ object TbdexHttpClient {
 errors = errors
 )
 }
+
+ private fun validateMessage(message: Message) {


changing httpclient to send rfq request with rfq and replyTo field (i…

30101d1

…f exists). wrote tests for client change. rename submitrfq handler to createexchange httpserver handler. checking validity of url in the handler. wrote test for handler change

jiyoontbd marked this pull request as ready for review January 20, 2024 01:59

jiyoontbd requested review from mistermoe, phoebe-lew, michaelneale, KendallWeihe, ethan-tbd, tomdaffurn and diehuxx as code owners January 20, 2024 01:59

jiyoontbd mentioned this pull request Jan 20, 2024

Introduce Webhooks / Callbacks TBD54566975/tbdex#167

Closed

diehuxx reviewed Jan 22, 2024

View reviewed changes

httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/CreateExchange.kt Outdated Show resolved Hide resolved

diehuxx reviewed Jan 22, 2024

View reviewed changes

httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/CreateExchange.kt Outdated Show resolved Hide resolved

diehuxx reviewed Jan 22, 2024

View reviewed changes

jiyoontbd and others added 5 commits January 24, 2024 12:08

catching nosuchelementexception

ae4713a

Update httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/Creat…

d7b968c

…eExchange.kt Co-authored-by: Diane Huxley <dhuxley@tbd.email>

Update httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/Creat…

ba08a73

…eExchange.kt Co-authored-by: Diane Huxley <dhuxley@tbd.email>

removing /rfq from endpoint

65de422

fixing tests

fcff6e1

jiyoontbd requested a review from diehuxx January 24, 2024 17:37

jiyoontbd added 3 commits January 24, 2024 15:13

writing an overload method for sendMessage() which specifically handl…

8f14f01

…es Rfq message with a replyTo field

removing a duplicated type. small refactorings

729854b

adding kirah to codeowners

26e49f3

jiyoontbd changed the title ~~adding replyTo field in http request body to submit RFQ~~ adding replyTo field in http request body to submit RFQ Jan 24, 2024

jiyoontbd added 2 commits January 24, 2024 15:46

removing unused type

6d86b59

removing unused getOrder()

ae0d434

diehuxx reviewed Jan 24, 2024

View reviewed changes

checking message type before defining url in sendMessage(message)

b63a0ab

diehuxx approved these changes Jan 29, 2024

View reviewed changes

phoebe-lew reviewed Jan 31, 2024

View reviewed changes

jiyoontbd added 2 commits January 31, 2024 22:05

adding an alias method for createExchange(). added a couple more tests

2eed618

adding more description in the original sendMessage() ktdoc

e790f32

diehuxx approved these changes Feb 2, 2024

View reviewed changes

phoebe-lew approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'main' of github.com:TBD54566975/tbdex-kt into 132-replyTo

b074ee3

jiyoontbd linked an issue Feb 5, 2024 that may be closed by this pull request

Implement replyTo property and associated changes #132

Closed

Merge branch 'main' of github.com:TBD54566975/tbdex-kt into 132-replyTo

9c27b39

jiyoontbd merged commit 4dc1e42 into main Feb 6, 2024
3 of 5 checks passed

jiyoontbd deleted the 132-replyTo branch February 6, 2024 15:12

jiyoontbd linked an issue Feb 6, 2024 that may be closed by this pull request

Introduce Webhooks / Callbacks TBD54566975/tbdex#167

Closed

jiyoontbd linked an issue Mar 22, 2024 that may be closed by this pull request

Make replyTo field available to SubmitCallbacks #195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding `replyTo` field in http request body to submit RFQ #140

adding `replyTo` field in http request body to submit RFQ #140

jiyoontbd commented Jan 20, 2024 •

edited

Loading

codecov bot commented Jan 20, 2024 •

edited

Loading

diehuxx Jan 22, 2024

jiyoontbd Jan 24, 2024

jiyoontbd Jan 24, 2024

jiyoontbd Jan 24, 2024

diehuxx Jan 22, 2024

jiyoontbd Jan 24, 2024

diehuxx Jan 22, 2024

jiyoontbd Jan 23, 2024

jiyoontbd Jan 24, 2024

jiyoontbd Jan 24, 2024

diehuxx Jan 22, 2024 •

edited

Loading

jiyoontbd Jan 23, 2024

diehuxx Jan 24, 2024

jiyoontbd Jan 25, 2024

diehuxx Jan 24, 2024

jiyoontbd Jan 25, 2024

diehuxx left a comment

phoebe-lew Jan 31, 2024

jiyoontbd Jan 31, 2024

jiyoontbd Jan 31, 2024

diehuxx Feb 1, 2024 •

edited

Loading

jiyoontbd Feb 1, 2024

phoebe-lew Jan 31, 2024

		@@ -99,7 +99,7 @@ class TbdexHttpServer(private val config: TbdexHttpServerConfig) {

		route("/exchanges") {
		post("/{exchangeId}/rfq") {

		@@ -84,20 +87,41 @@ object TbdexHttpClient {
		val pfiServiceEndpoint = getPfiServiceEndpoint(pfiDid)
		val url = "$pfiServiceEndpoint/exchanges/$exchangeId/$kind"

adding replyTo field in http request body to submit RFQ #140

adding replyTo field in http request body to submit RFQ #140

Conversation

jiyoontbd commented Jan 20, 2024 • edited Loading

codecov bot commented Jan 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diehuxx Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diehuxx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diehuxx Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adding `replyTo` field in http request body to submit RFQ #140

adding `replyTo` field in http request body to submit RFQ #140

jiyoontbd commented Jan 20, 2024 •

edited

Loading

codecov bot commented Jan 20, 2024 •

edited

Loading

diehuxx Jan 22, 2024 •

edited

Loading

diehuxx Feb 1, 2024 •

edited

Loading