Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

Add external_id field #163

Merged
merged 8 commits into from
Feb 20, 2024
Merged

Add external_id field #163

merged 8 commits into from
Feb 20, 2024

Conversation

phoebe-lew
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Feb 8, 2024

🦋 Changeset detected

Latest commit: 7dd8478

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@tbdex/protocol Patch
@tbdex/http-client Patch
@tbdex/http-server Patch

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

Copy link
Contributor

github-actions bot commented Feb 8, 2024

TBDocs Report

🛑 Errors: 0
⚠️ Warnings: 12

@tbdex/protocol

  • Project entry file: packages/protocol/src/main.ts

@tbdex/http-client

  • Project entry file: packages/http-client/src/main.ts
📄 File: packages/http-client/src/errors/request-error.ts
⚠️ extractor:ae-missing-release-tag: "RequestErrorParams" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal) #L1
⚠️ extractor:ae-undocumented: Missing documentation for "RequestErrorParams". #L1
⚠️ extractor:ae-undocumented: Missing documentation for "recipientDid". #L13
⚠️ extractor:ae-undocumented: Missing documentation for "url". #L14
📄 File: packages/http-client/src/errors/request-token-error.ts
⚠️ extractor:ae-missing-release-tag: "RequestTokenErrorParams" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal) #L3
⚠️ extractor:ae-undocumented: Missing documentation for "RequestTokenErrorParams". #L3
📄 File: packages/http-client/src/errors/response-error.ts
⚠️ extractor:ae-missing-release-tag: "ResponseErrorParams" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal) #L3
⚠️ extractor:ae-undocumented: Missing documentation for "ResponseErrorParams". #L3
⚠️ extractor:ae-undocumented: Missing documentation for "statusCode". #L15
⚠️ extractor:ae-undocumented: Missing documentation for "details". #L16
⚠️ extractor:ae-undocumented: Missing documentation for "recipientDid". #L17
⚠️ extractor:ae-undocumented: Missing documentation for "url". #L18

@tbdex/http-server

  • Project entry file: packages/http-server/src/main.ts

TBDocs Report Updated at 2024-02-20T17:12:38Z 7dd8478

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Merging #163 (7dd8478) into main (ebefc54) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   92.48%   92.50%   +0.01%     
==========================================
  Files          37       37              
  Lines        2995     3000       +5     
  Branches      321      322       +1     
==========================================
+ Hits         2770     2775       +5     
  Misses        225      225              
Components Coverage Δ
protocol 93.41% <100.00%> (+0.01%) ⬆️
http-client 93.63% <ø> (ø)
http-server 89.49% <ø> (ø)

@KendallWeihe
Copy link
Contributor

(I've been out of the loop so apologies if this is easily answered)

Should we add this to the spec?

Are there implications that the external_id will be echoed back to Alice in subsequent messages (such as Quote or OrderStatus)?

@KendallWeihe
Copy link
Contributor

lol literally the next PR I pulled up to review was this, disregard the first question

@phoebe-lew
Copy link
Contributor Author

Are there implications that the external_id will be echoed back to Alice in subsequent messages (such as Quote or OrderStatus)?

@KendallWeihe this is a good point actually...perhaps we should only echo back the IDs which were sent by alice?

@KendallWeihe
Copy link
Contributor

@KendallWeihe this is a good point actually...perhaps we should only echo back the IDs which were sent by alice?

I would think this would be a good practice. Given Alice includes an external_id in a message, then subsequent messages from the PFI to Alice would also include the external_id. Though we may want to think through downstream implications? And how does this relate to the original need for adding external_id in the first place? And is the external_id associated to the Exchange as a whole or just for the given message (and subsequent messages?) wherein it was included?

@phoebe-lew
Copy link
Contributor Author

@KendallWeihe was thinking about this more...I think we can actually assume that the entire contents of a message (metadata + message body) are visible between Alice and the PFI. If the PFI doesn't want to expose an internal ID, it can simply not populate the value. It's likely that there are a bunch of IDs correlated to a single exchange (one for payout side, one for payin side at least), those IDs are in a separate context from tbdex messages. The PFI can map from tbdex message id to payment rail specific IDs in its own business logic.

@KendallWeihe
Copy link
Contributor

@phoebe-lew makes sense to me. I'm curious, was there a motivating use case for adding external_id to the protocol or are we thinking ahead to potential use cases?

@jiyoonie9 jiyoonie9 linked an issue Feb 13, 2024 that may be closed by this pull request
@phoebe-lew
Copy link
Contributor Author

I'm curious, was there a motivating use case for adding external_id to the protocol or are we thinking ahead to potential use cases?

@KendallWeihe guess it's a potential use case, but letting clients pass in an arbitrary reference/id is pretty standard practice with most payment APIs.

* main:
  Stricten, test, and bugfix http-server (#170)
  Version Packages (#167)
  Update web5/dids, web5/credentials, web5/crypto, web5/common to latest  (#177)
  Add exchange state machine (#168)
  Stricten http-client (#169)
  Make `pfiDid` required property in `TbdexHttpServer` (#166)
@phoebe-lew phoebe-lew merged commit 5e9a7a2 into main Feb 20, 2024
15 of 17 checks passed
@phoebe-lew phoebe-lew deleted the plew.external-id branch February 20, 2024 17:12
diehuxx pushed a commit that referenced this pull request Feb 20, 2024
* main:
  Add external_id field (#163)
  Stricten, test, and bugfix http-server (#170)
  Version Packages (#167)
  Update web5/dids, web5/credentials, web5/crypto, web5/common to latest  (#177)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add externalId to metadata
4 participants