feat: add properties for quoting #275

sabineschaller · 2023-07-31T12:33:46Z

Changes proposed in this pull request

updates resource server spec and auth server spec to change sendAmount to debitAmount

Context

We originally planned to add fees to the OP resources, however, we decided that fees should be included in the debitAmount. The difference in debitAmount - receiveAmount can be considered being the fees. The ASE may list those fees separately on the consent screen displayed to the user.

fixes New quoting mechanism #274

changeset-bot · 2023-07-31T12:33:49Z

🦋 Changeset detected

Latest commit: e40d55a

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	Major

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

openapi/resource-server.yaml

mkurapov · 2023-08-02T11:40:39Z

openapi/resource-server.yaml

@@ -576,14 +612,23 @@ paths:
                    id: 'https://openpayments.guide/alice/quotes/8c68d3cc-0a0f-4216-98b4-4fa44a6c88cf'


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

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.

It's all debitAmount now, even in the grant

mkurapov · 2023-08-02T11:42:04Z

.changeset/tender-ravens-breathe.md

@@ -0,0 +1,5 @@
+---
+'@interledger/open-payments': minor


I think this should be major since it contains breaking changes

I must have run the wrong command 🙃

It's technically still a major change, but it feels so minor...

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.

Yeah +1 on making this a major since it is breaking.
The spec can stay minor IMO

adrianhopebailie

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.

openapi/resource-server.yaml

adrianhopebailie

LGTM!

mkurapov

Few references to sendAmount left

mkurapov · 2023-09-05T13:58:44Z

.changeset/tender-ravens-breathe.md

@@ -0,0 +1,5 @@
+---
+'@interledger/open-payments': minor


Yeah +1 on making this a major since it is breaking.
The spec can stay minor IMO

mkurapov · 2023-09-05T13:59:42Z

openapi/resource-server.yaml

          $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.


Still reference to sendAmount here

mkurapov · 2023-09-05T14:00:20Z

openapi/resource-server.yaml

@@ -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:


reference to sendAmount in the examples still

mkurapov · 2023-09-05T14:01:09Z

openapi/resource-server.yaml

@@ -641,7 +628,6 @@ paths:
                    - receiver
                    - sendAmount
                  additionalProperties: false
-


I think the quote still has sendAmount, are we keeping that?

No, it's all debitAmount now, since the quote also has fees included.

mkurapov

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

feat: add properties for quoting

2db18db

sabineschaller requested review from adrianhopebailie and mkurapov July 31, 2023 12:34

sabineschaller added 5 commits July 31, 2023 14:37

fix: formatting

4defa14

fix: types

daa0c4c

fix: op package with new types

1222158

chore: add changeset

dea5221

fix: tests

e6994b3

mkurapov reviewed Aug 2, 2023

View reviewed changes

adrianhopebailie reviewed Aug 21, 2023

View reviewed changes

openapi/resource-server.yaml Outdated Show resolved Hide resolved

openapi/resource-server.yaml Outdated Show resolved Hide resolved

sabineschaller added 4 commits August 23, 2023 09:57

chore: remove fees, rename (max)sendAmount -> debit Amount

66e4ae9

fix: build

299b556

fix: amount description

22564ab

fix: generate types

2d3e9d1

sabineschaller requested review from adrianhopebailie, mkurapov, BlairCurrey and njlie August 23, 2023 08:16

adrianhopebailie approved these changes Aug 28, 2023

View reviewed changes

mkurapov reviewed Sep 5, 2023

View reviewed changes

sabineschaller added 4 commits September 6, 2023 09:30

fix: remaining sendAmounts

8b0f1fa

chore: versioning

9b49453

fix: generate types

2c77d1f

fix: server urls

2d4c905

mkurapov reviewed Sep 6, 2023

View reviewed changes

fix: replace final sendAmount

e40d55a

sabineschaller merged commit dd193c7 into main Sep 6, 2023

sabineschaller deleted the s2-quote-changes branch September 6, 2023 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add properties for quoting #275

feat: add properties for quoting #275

sabineschaller commented Jul 31, 2023 •

edited

Loading

changeset-bot bot commented Jul 31, 2023 •

edited

Loading

mkurapov Aug 2, 2023

BlairCurrey Aug 18, 2023

sabineschaller Aug 23, 2023

mkurapov Aug 2, 2023

sabineschaller Aug 17, 2023

sabineschaller Aug 23, 2023

BlairCurrey Aug 24, 2023 •

edited

Loading

mkurapov Sep 5, 2023

adrianhopebailie left a comment

adrianhopebailie left a comment

mkurapov left a comment

mkurapov Sep 5, 2023

mkurapov Sep 5, 2023

mkurapov Sep 5, 2023

mkurapov Sep 5, 2023

sabineschaller Sep 6, 2023

mkurapov left a comment

		@@ -576,14 +612,23 @@ paths:
		id: 'https://openpayments.guide/alice/quotes/8c68d3cc-0a0f-4216-98b4-4fa44a6c88cf'

feat: add properties for quoting #275

feat: add properties for quoting #275

Conversation

sabineschaller commented Jul 31, 2023 • edited Loading

Changes proposed in this pull request

Context

changeset-bot bot commented Jul 31, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianhopebailie left a comment

Choose a reason for hiding this comment

adrianhopebailie left a comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

sabineschaller commented Jul 31, 2023 •

edited

Loading

changeset-bot bot commented Jul 31, 2023 •

edited

Loading

BlairCurrey Aug 24, 2023 •

edited

Loading