feat: add properties for quoting #275
Conversation
🦋 Changeset detectedLatest commit: e40d55a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| @@ -576,14 +612,23 @@ paths: | |||
| id: 'https://openpayments.guide/alice/quotes/8c68d3cc-0a0f-4216-98b4-4fa44a6c88cf' | |||
There was a problem hiding this comment.
should the quote request body still include sendAmount or be changed to maxSendAmount?
Even though it doesn't match maxSendAmount I think sendAmount is good to leave as is
There was a problem hiding this comment.
I was thinking about this as well while working on interledger/rafiki#1639. I think it might be confusing to have sendAmount going in an maxSendAmount coming back. The user might not understand they are the same thing and wonder if maxSendAmount is derived or transformed from sendAmount instead of set directly. If they are the same thing they should probably have the same name.
There was a problem hiding this comment.
It's all debitAmount now, even in the grant
.changeset/tender-ravens-breathe.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@interledger/open-payments': minor | |||
There was a problem hiding this comment.
I think this should be major since it contains breaking changes
There was a problem hiding this comment.
I must have run the wrong command 🙃
There was a problem hiding this comment.
It's technically still a major change, but it feels so minor...
There was a problem hiding this comment.
I think it should still be major since it's breaking, and I think we probably need to bump the resource-server.yaml and auth-server.yaml version as well. Thats how we handled it when I renamed a field here: #270. Although the spec version bump was a minor bump 🤔... don't quite remember that reasoning.
On the topic of it feeling minor, Max pointed to some relevant blog posts and hackernews discussion in this comment #270 (comment). I don't see any quick, easy wins there and think just doing a major bump still makes the most sense but it is interesting reading.
There was a problem hiding this comment.
Yeah +1 on making this a major since it is breaking.
The spec can stay minor IMO
There was a problem hiding this comment.
I added a couple of comments but I have to admit I still find this really confusing. Maybe it would be better if we just added fees on the incoming payment first to review what that looks like when consumed by the sender.
Then we have a separate PR that adds fees on outgoing payments as that's a very different audience and seems to (I think) imply some pass through of info it has read from the incoming payment.
sabineschaller
requested review from
adrianhopebailie,
mkurapov,
BlairCurrey and
njlie
.changeset/tender-ravens-breathe.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@interledger/open-payments': minor | |||
There was a problem hiding this comment.
Yeah +1 on making this a major since it is breaking.
The spec can stay minor IMO
openapi/resource-server.yaml
Outdated
| $ref: ./schemas.yaml#/components/schemas/amount | ||
| description: "The total amount that should be deducted from the sender's account when the corresponding outgoing payment has been paid. " | ||
| expiresAt: | ||
| type: string | ||
| description: The date and time when the calculated `sendAmount` is no longer valid. |
There was a problem hiding this comment.
Still reference to sendAmount here
| @@ -576,7 +563,7 @@ paths: | |||
| id: 'https://openpayments.guide/alice/quotes/8c68d3cc-0a0f-4216-98b4-4fa44a6c88cf' | |||
| paymentPointer: 'https://openpayments.guide/alice/' | |||
| receiver: 'https://openpayments.guide/aplusvideo/incoming-payments/45d495ad-b763-4882-88d7-aa14d261686e' | |||
| sendAmount: | |||
| debitAmount: | |||
There was a problem hiding this comment.
reference to sendAmount in the examples still
| @@ -641,7 +628,6 @@ paths: | |||
| - receiver | |||
| - sendAmount | |||
| additionalProperties: false | |||
|
|
|||
There was a problem hiding this comment.
I think the quote still has sendAmount, are we keeping that?
There was a problem hiding this comment.
No, it's all debitAmount now, since the quote also has fees included.
There was a problem hiding this comment.
Looks good 👍 Final comment, can you please update this final reference to sendAmount: https://github.com/interledger/open-payments/blob/main/packages/open-payments/README.md?plain=1#L182
Changes proposed in this pull request
sendAmounttodebitAmountContext
We originally planned to add fees to the OP resources, however, we decided that fees should be included in the
debitAmount. The difference indebitAmount - receiveAmountcan be considered being the fees. The ASE may list those fees separately on the consent screen displayed to the user.