-
Notifications
You must be signed in to change notification settings - Fork 9
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
update request handlers and add tests #237
Conversation
🦋 Changeset detectedLatest commit: 3d976c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
TBDocs Report 🛑 Errors: 0 @tbdex/protocol
@tbdex/http-client
@tbdex/http-server
TBDocs Report Updated at 2024-04-09T17:08:35Z |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
=======================================
Coverage 94.77% 94.78%
=======================================
Files 42 42
Lines 3541 3547 +6
Branches 390 392 +2
=======================================
+ Hits 3356 3362 +6
Misses 185 185
|
@@ -53,5 +54,7 @@ export async function getExchanges(request: Request, response: Response, opts: G | |||
const _result = await callback({ request, response }, queryParams) | |||
} | |||
|
|||
response.status(200).json({ data: exchanges }) | |||
const data: Message[][] = exchanges.map(exchange => exchange.messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the response body in the spec is an array of id
's, whereas this is a 2d array of exchange messages, right? are we not doing that because of other constraints (like because that would require more broad sweeping changes?) https://github.com/TBD54566975/tbdex/tree/main/specs/http-api#response-4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm good thought! the client currently expects Message[][]
so best to make a separate PR that updates both client and server according to new spec changes.
can fast follow with that but prob good to at least get this fix up so its at min bug free 🐛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for jumping on this @kirahsapong!
🚀
closes #236
also added tests and type check