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

Updates #269

Merged
merged 16 commits into from
Mar 14, 2024
Merged

Updates #269

merged 16 commits into from
Mar 14, 2024

Conversation

mistermoe
Copy link
Contributor

@mistermoe mistermoe commented Mar 12, 2024

Overview

This PR updates protocol spec text to address several open issues. This PR can be looked at 1 commit at a time. I've included a table that maps the issue to the respective commits that address said issue. Context regarding the issue can be found in the issue itself. worth reading prior to checking out the commit!

Important

Commit that addresses #263 is worth looking at. feels a bit duplicative within the json schema in order to accommodate requiring estimatedSettlementTime in payout methods only. there may be a json schema trick to simplify. That aside, main callout is that we lose symmetry for payin / payout methods, but estimatedSettelementTime feels like an important concept to include.

Warning

the test vectors have not been updated. will address that in a subsequent PR after tbdex-js uptakes the changes included in this PR.

Generally wondering whether our current vectors provide any additional utility compared to simply writing tests that:

  • create a specific message or resource kind
  • use the json schemas to validate said message or resource
  • assert no validation errors.

not discounting the value of vectors in general. moreso just feeling like our current vectors feel a bit duplicative given that the json schemas could act as the source of truth for message structure. I realize we're also doing sig verification as part of our consuming the current vectors but we may be able to narrow the scope of the vectors themselves. anyway, random thoughts. will open an issue to track if relevant

Commits

issue commits
260 f55c588, 752aa4f
261 9dc5148
262 ff03448
263 0614cf5
253 5c1e67a

Co-Authored-By: corcillo <[email protected]>
@mistermoe mistermoe linked an issue Mar 13, 2024 that may be closed by this pull request
@mistermoe mistermoe marked this pull request as ready for review March 13, 2024 05:44
@mistermoe mistermoe requested a review from corcillo March 13, 2024 05:44
@mistermoe mistermoe linked an issue Mar 13, 2024 that may be closed by this pull request
hosted/json-schemas/offering.schema.json Show resolved Hide resolved
hosted/json-schemas/offering.schema.json Show resolved Hide resolved
specs/protocol/README.md Show resolved Hide resolved
@KendallWeihe
Copy link
Contributor

KendallWeihe commented Mar 13, 2024

Oooh interesting, commit-per-issue w/ a table breakdown in the PR description, I like!


Edit: okay this turned out to be not all that useful as the first thought I had when looking at a specific commit diff view was, "wait but what if a following commit changed this line and I'm wasting my time?" Nevertheless, I appreciate the thought and maybe I'm just doin-it-wrong!

@KendallWeihe
Copy link
Contributor

Not a priority, but I had a question arise just this AM around building test vectors (or something akin to) for our HTTP API implementations, so I went ahead and opened the ticket #273

@KendallWeihe
Copy link
Contributor

KendallWeihe commented Mar 13, 2024

there may be a json schema trick to simplify

Turns out there is a trick, by using allOf, here's an example

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "commonProperties": {
      "type": "object",
      "properties": {
        "name": { "type": "string" },
        "age": { "type": "integer" }
      },
      "required": ["name", "age"]
    }
  },
  "type": "object",
  "properties": {
    "basicProfile": {
      "$ref": "#/definitions/commonProperties"
    },
    "extendedProfile": {
      "allOf": [
        { "$ref": "#/definitions/commonProperties" },
        {
          "type": "object",
          "properties": {
            "hobby": { "type": "string" }
          },
          "required": ["hobby"]
        }
      ]
    }
  }
}

Or you another example https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#allof



> [!IMPORTANT]
> `estimatedSettlementTime` is used to provide a rough estimate for the expected latency between receiving a _payin_ and settling the respective _payout_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the estimated settlement time be a function of both the payin and the payout methods? Payin may be BTC which takes 10+ minutes when payout is debit which is instant

Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe clarify: is this estimate supposed to be the full settlement of the exchange (starting from Order message being accepted -> Close message being sent), or is it JUST for the payout portion (which would be a payout initiated/payin complete OrderStatus -> Close message being sent)?

And does "receiving" a payin mean the PFI has been notified that there is a payin transaction (assuming the payin method is not stored balance), or does received == payin has settled?

Copy link
Contributor Author

@mistermoe mistermoe Mar 13, 2024

Choose a reason for hiding this comment

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

Could maybe clarify: is this estimate supposed to be the full settlement of the exchange (starting from Order message being accepted -> Close message being sent), or is it JUST for the payout portion (which would be a payout initiated/payin complete OrderStatus -> Close message being sent)?

imo it should be just for the payout portion because the PFI does not control when alice pays in.

And does "receiving" a payin mean the PFI has been notified that there is a payin transaction (assuming the payin method is not stored balance), or does received == payin has settled?

received == payin has settled. in a stored balance scenario, "payin received" can be considered as instant or moot

lmk if that helps @KendallWeihe @phoebe-lew

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense but IMO it's also relevant to the payin method. Agreed that the onus is on Alice for the payin method, but the PFI may have unique characteristics for their given payin methods. For example, consider BTC as a payin method, there is no standard rule as for how many blocks need to be mined for the payment to be considered final (even more relevant in Ethereum where reorgs are frequent), so each PFI could define their own set of rules for how long it takes for that tx to be considered final; Alice could submit a BTC tx and even see it in the blockchain but it may still not be considered settled by the PFI.

},
"requiredPaymentDetails": {
"type": "object",
"description": "A JSON Schema containing the fields that need to be collected in order to use this payment method"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a JSON schema we can add a $ref to?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is intended to be a json schema for which the PFI can define for their given payment use case; as in, it's a generalized space for a PFI to define payment details via JSON Schema

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oh nice, interesting... yeah I guess that makes sense, a json schema is itself a type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. good call @decentralgabe. added you as commit co-author: efd97df

},
"paymentDetails": {
"type": "object",
"description": "An object containing the properties defined in the respective Offering's requiredPaymentDetails json schema"
Copy link
Contributor

Choose a reason for hiding this comment

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

could a $ref be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you have in mind @decentralgabe ? this property's value is an object that meets the requirements set by the offering's requiredPaymentDetails

},
"paymentDetails": {
"type": "object",
"description": "An object containing the properties defined in the respective Offering's requiredPaymentDetails json schema"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment: #269 (comment)

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

small comments, looks good

| field | data type | required | description |
| ------------------------- | --------------------------------------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------- |
| `kind` | string | Y | Type of payment method (i.e. `DEBIT_CARD`, `BITCOIN_ADDRESS`, `SQUARE_PAY`). considered to be a unique identifier |
| `estimatedSettlementTime` | uint | Y | estimated time taken to settle an order. expressed in seconds |
Copy link

Choose a reason for hiding this comment

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

Super nit: why not millis instead of seconds? I agree that the vast majority of payments with TBDex will be slower than 1 second, but I wonder if a small few may find it useful have very tight settlement times less than one second

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair point @diehuxx . went with seconds because anything less than 1 second i considered to be 0 aka "instant" bc i assume no one will be using tbdex for high frequency day trading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants