search

interledger / open-payments

Protocol to setup payments between entities on the Web based on GNAP
https://openpayments.dev
Apache License 2.0
204 stars 34 forks source link

fix(open-payments): change outgoingPayment.create return type #488

Closed BlairCurrey closed 4 months ago

BlairCurrey commented 4 months ago

Changes proposed in this pull request

Context

fixes issue raised in this comment: https://github.com/interledger/open-payments/pull/480#issuecomment-2202353862

looks like it was probably first encountered here https://github.com/interledger/web-monetization-extension/pull/386/files

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 8ad7061e0506f76a0efe3e14bfd6982b86e3325a

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

This PR includes changesets to release 1 package | Name | Type | | -------------------------- | ----- | | @interledger/open-payments | 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

netlify[bot] commented 4 months ago

Deploy Preview for openpayments-preview canceled.

Name Link
Latest commit 8ad7061e0506f76a0efe3e14bfd6982b86e3325a
Latest deploy log https://app.netlify.com/sites/openpayments-preview/deploys/668d66a967d169000830ea32
sidvishnoi commented 4 months ago

Why do we have two interfaces - OutgoingPaymentWithSpentAmounts and OutgoingPayment? The only difference I see between them are two keys: "grantSpentDebitAmount" | "grantSpentReceiveAmount", that are optional. So, I think we can rename OutgoingPaymentWithSpentAmounts to OutgoingPayment and keep only one of them (and won't be a breaking change either, as optional keys got added).

BlairCurrey commented 4 months ago

Why do we have two interfaces - OutgoingPaymentWithSpentAmounts and OutgoingPayment?

Originally we added a different type for the POST /outgoing-payment endpoint because we wanted to return these additional fields on that endpoint only. GET /outgoing-payment will never have these fields, for example.

I felt like having them as optional properties on the other /outgoing-payment endpoints was misleading because we know they will never be defined. Although it would still adhere to the spec, since they are optional, and does simplify the types. 🤔

mkurapov commented 4 months ago

I think having an explicit interface is safer (and this still can still be a minor version bump I think, since its just net new fields).

We should have the types for the operations adhere close to the spec as possible -> in this case, if we end up making grantSpentDebitAmount required (or add a new field, or anything else to OutgoingPayment like @BlairCurrey said, it might be misleading/cause issues)