refactor(backend): swap connection for methods #2054

BlairCurrey · 2023-10-18T19:32:19Z

Changes proposed in this pull request

updates to latest open-payments openapi spec (5.2.2)
refactors to remove connection and replace with methods
connection service was reduced to just get the stream credentials and has been renamed to streamCredentialService

Context

This branch is does not build yet, and not all tests pass, which is expected. This is due to other unimplemented changes. I fixed all build errors related to these change and the test failures I see locally appear related to pagination (possibly due to bug in our down migration in our dev branch at the moment?).

fixes Implement payment methods and finish removing connections #1962

Checklist

* feat(backend): mount resource server routes explicitly * chore: formatting * feat(backend): wip unauthenticated incoming payment get * fix(backend): token introspection middleware * chore(backend): cleanup * chore(backend): cleanup * test(backend): update incoming payment tests * test(backend): add tests for new option * feat(backend): add unauthed incoming payment * fix: accidentally removed postman request * refactor(backend): optional sig params * fix(backend): typo * fix(backend): unused args --------- Co-authored-by: Nathan Lie <nathanlie@Nathans-MacBook-Pro.local>

BlairCurrey · 2023-10-19T00:35:13Z

packages/backend/src/open_payments/quote/model.ts

+  public get method(): 'ilp' {
+    return 'ilp'
+  }
+


@mkurapov wasn't quite sure what our intention is for modeling payment methods.

The new openapi spec requires the method on the POST /quote body but I didn't actually add it to the create path (quoteRoutes.create, quoteService.create, createQuote test util etc) since its not stored anywhere and I'm not sure how we want to do that (method column on Quote? fkey column on Quote point to Methods table?)

I like having the idea of a PaymentMethods table that determines what types of payment methods are active or inactive, but for the sake of time this (hard-coding ilp) works well

BlairCurrey · 2023-10-19T00:41:55Z

the ilp payment method is hardcoded a number of places. I'm not against abstracting it a bit to handle multiple methods where it's clear and simple but I wanted to keep the scope to what is necessary to maintain functionality of ilp payment methods. In additional to maximizing the amount of time we have to update testnet, I think the right way to handle multiple methods will probably be more clear when we actually add the next method.

BlairCurrey · 2023-10-19T00:51:06Z

packages/backend/src/open_payments/receiver/model.ts

+    // TODO: ensure these are the right assetCode/Scales
+    this.assetCode = incomingPayment.receivedAmount.assetCode
+    this.assetScale = incomingPayment.receivedAmount.assetScale


I will address this TODO before merging. Previously we were getting these off the connection and I wasn't sure where we should get it now - Max suggested this and I believe it is correct. Tests seem to back it up but any more common-sense checks would be welcome

rafiki/packages/backend/src/open_payments/receiver/model.test.ts

Lines 49 to 51 in 04051d1

expect(receiver).toEqual({

assetCode: incomingPayment.asset.code,

assetScale: incomingPayment.asset.scale,

Other than fetching the walletAddress asset, I don't think there is really any other option that we have

mkurapov

Looks good

mkurapov · 2023-10-19T11:22:36Z

packages/backend/src/open_payments/connection/service.ts

-): string | undefined {
-  if (!payment.connectionId) {
-    return undefined
+  return {


we can probably get away with the if (!payment.connectionId) check, and remove connectionid altogether from the incoming payment model

I removed the connectionId from IncomingPayment. I kept this check to return undefined when payment is completed or expired to maintain the behavior tested here:

rafiki/packages/backend/src/open_payments/connection/service.test.ts

Lines 93 to 107 in 4f1c178

test.each`

state

${IncomingPaymentState.Completed}

${IncomingPaymentState.Expired}

`(

`returns undefined for $state incoming payment`,

async ({ state }): Promise<void> => {

await incomingPayment.$query(knex).patch({

state,

expiresAt:

state === IncomingPaymentState.Expired ? new Date() : undefined

})

expect(connectionService.get(incomingPayment)).toBeUndefined()

}

)

A beforeInsert hook ensured the connectionId was always set, and then it would be set to null in a beforeUpdate hook when the state was completed or expired. So it seems this was basically a roundabout way to check if the payment was expired or complete.

Wouldn't this mean that the Open Payments GET /incoming-payment/ fails if the payment is expired or completed since we won't be returning the methods array?

In this case, if there is no ilpStreamConnection passed into the incomingPayment.toOpenPaymentsType (here), we can just return an empty methods array

Wouldn't this mean that the Open Payments GET /incoming-payment/ fails if the payment is expired or completed since we won't be returning the methods array?

I think you're right but I dont think we can just add an empty methods array to toOpenPaymentsType when no connections are passed in. Sometimes it should return the payment without methods to satisfy the base incoming-payment openapi type. I guess we could make a seperate toOpenPaymentsTypeWithMethods (not overloaded) and call as appropriate. But should the method really be empty when the payment is expired/complete? Do we not want the actual payment method with credentials?

I'm thinking your original suggestion makes the most sense now. If we dont return undefined for completed or expired payments then we can handle completed/expired where its called if we want, and do nothing otherwise (such as GET /incoming-payment/{id}). I implemented that in this new commit 8396970

will need to revise this to ensure methods is an empty array when the result should match the incoming-payment-with-methods type and the state is completed or expired.

updated. The caller of toOpenPaymentsType has to distinguish between which type (with methods or without) so that we ensure we're returning the right object with respect to the methods field. We can no longer use the presence of the ilpStreamCredentials to determine it alone. An undefined ilpStreamCredentials should sometimes return no methods, and sometimes [] depending on where its called.

As such I removed the overloading (rather than perhaps adding an additional withMethods boolean) since it makes the internal function logic much simpler and the caller already has to differentiate between which type it wants anyways.

mkurapov · 2023-10-19T11:27:56Z

packages/backend/src/open_payments/quote/model.ts

+  public get method(): 'ilp' {
+    return 'ilp'
+  }
+


I like having the idea of a PaymentMethods table that determines what types of payment methods are active or inactive, but for the sake of time this (hard-coding ilp) works well

mkurapov · 2023-10-19T11:35:41Z

packages/backend/src/open_payments/receiver/model.ts

+    // TODO: ensure these are the right assetCode/Scales
+    this.assetCode = incomingPayment.receivedAmount.assetCode
+    this.assetScale = incomingPayment.receivedAmount.assetScale


Other than fetching the walletAddress asset, I don't think there is really any other option that we have

BlairCurrey · 2023-10-19T16:03:59Z

Almost makes me think we can just pass in the streamServer into incomingPaymentService and remove connectionService altogether, thoughts? @sabineschaller @BlairCurrey

I had a similar thought but I'm unsure if incomignPayment makes the most sense for this since it's ilp specific. As discussed elsewhere, I will change the service name to streamCredentials and move to the payment-method/ilp.

mkurapov · 2023-10-19T16:49:04Z

packages/backend/src/open_payments/quote/model.ts

@@ -20,7 +20,8 @@ export class Quote extends WalletAddressSubresource {
      'receiveAmount',
      'minExchangeRate',
      'lowEstimatedExchangeRate',
-      'highEstimatedExchangeRate'
+      'highEstimatedExchangeRate',
+      'method'


we should also be able to expect the method field as an argument to quote creation

I'm not sure if I fully understand so please clarify if I have something wrong. Given that this is an objection model, these quotes objects are created from inserting and querying the database. Rather than doing something like new Quote({..., method: 'ilp'}). So I cant pass it in to the quote creation because it doesnt exist in the database and I think we agreed that the database changes didn't need to be in this PR. And if its defined as a real class attribute, it will try to write ilp to a non-existant method column where ever we insert a quote.

At face value I think it would be appropriate to include all of that here (add to quoteService.create, adjust everywhere that's called, add migration). However I think we're sacrificing debugging of nl-open-payments-updates at this point to do so, and it's functional as-is. I can capture this improvement in a task if we like, and work on that if we have extra time, otherwise do it after the code-freeze.

I think what Max meant was adding method to the arguments of quoteService.create.

We can, but it will be unused until we make the db changes.

edit: im adding it to the create paths

added 6f3e16e

…for-methods

sabineschaller · 2023-10-20T15:56:25Z

packages/backend/src/open_payments/payment/incoming/routes.test.ts

I think we talked about this during our call:
We should test that expired and completed incoming payments include an empty methods array.

sabineschaller · 2023-10-20T15:59:16Z

packages/backend/src/open_payments/payment/incoming/service.test.ts

Same here, we should check that no stream credentials are created

credentials were never being generated in the incoming payment service. so I think thats a non-factor here unless Im missing something. I dont see anywhere I could add such a check in the service (I am adding one to the incoming/model.test.ts in addition to incoming/routes.test.ts).

im also going to restore the original behavior to streamCredentials.get where it returned undefined when the incoming payment was complete or expired (maybe this was your point here).

…yment

njlie · 2023-10-20T18:13:17Z

packages/backend/src/open_payments/payment/incoming/model.ts


  public toOpenPaymentsType(
    walletAddress: WalletAddress,
-    ilpStreamConnection?: Connection | string
+    ilpStreamCredentials?: IlpStreamCredentials


To spitball a future idea for this, maybe this parameter could be genericized into method which has a type that all payment methods implement and can call a .toOpenPayments() function.

Im not sure if I'm following but wanted to note there have been some recent changes that might affect this suggestion: #2054 (comment)

…ired payments

njlie · 2023-10-20T21:49:17Z

postman/collections/Interledger.json

@@ -1654,12 +1620,10 @@
 									}
 								],
 								"url": {
-									"raw": "http://localhost:4000/incoming-payments/{{incomingPaymentId}}",
-									"protocol": "http",
+									"raw": "{{pfryWalletAddress}}/incoming-payments/{{incomingPaymentId}}",


I think the postman collection needs to be updated again - looks like it swallowed the "remove wallet address from url" changes.

njlie · 2023-10-20T22:11:30Z

@mkurapov @sabineschaller If there's any other feedback you'd like to give in this PR feel free to make comment on pertinent lines as usual, but in the interest of keeping things moving I'd also like for you to create an issue so we can address it in a subsequent PR on the nl-open-payments feature branch.

Co-authored-by: Nathan Lie <lie4nathan@gmail.com>

* feat(backend): rename "payment pointer" tables & columns to use "wallet address" (#1937) Co-authored-by: Nathan Lie <nathanlie@Nathans-MacBook-Pro.local> * feat(backend, mock-ase): rename payment pointer to wallet address in rafiki code (#2029) * fix(backend): rename paymentPointerId column in combinedPaymentsView to walletAddressId (#2043) * docs: replace payment pointer with wallet address (#1951) * feat(backend): remove connections route (#1940) (#2051) Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> * feat(backend): unauthenticated get incoming payment (#1952) (#2052) Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> * feat(auth): fix auth tests for access tokens (#2053) * feat(backend): remove payment pointer from url, retrieve it from request query or body (#2042) * refactor(backend): swap connection for methods (#2054) Co-authored-by: Nathan Lie <lie4nathan@gmail.com> * fix(backend): resolve breaking build after main rebase (#2085) * fix(backend): combined_payment, ilp service tests (#2094) * fix(backend): combined_payment, ilp service tests * chore(backend): format * fix(backend): bad test refactor * fix(postman): invalid json (#2096) * fix: fetch walletAddress graph in quote and outgoing payment service (#2099) * fix(postman): env var usage (#2107) * feat(backend): add quotes route to exceptions in wallet address middleware (#2108) * chore: formatting * fix(backend): fix GET /incoming-payments (#2111) * fix(backend): remove wallet address from OP response id and fix receiver service (#2115) * fix: remove wallet address from op resource urls * feat: return auth server url in public incoming payment and fix receiver service * fix: quote issue * fix: walletAddress middleware * chore: update postman * fix: listing * chore: remove console.log * fix: progress towards making listing work * fix(backend): fix create remote incoming payment (#2121) * fix(postman): misc postman fixes * fix(postman): complete incoming payment (#2126) * fix: file name correction (#2127) * fix: GET resource lists, use op client (#2122) * fix(postman): update list urls * fix(openapi): rm defaults for first, last * refactor(postman): query param order for readability * fix(backend): update to new open-payments pkgs * fix(backend): use op client to get authServer * fix(backend): rm old code * fix(backend): receiver tests * chore: cleanup * feat: make list query parameter configurable * chore: update OP package and fix types --------- Co-authored-by: Sabine Schaller <sabine@interledger.org> * fix(postman): additional fixes * fix: wallet address on the fly creation (#2129) * chore: update postman environments * Postman: update description for P2P example * Postman: Remove finalizationReason from getGrants API + fix createWalletAddress * chore: bump open-payments package in auth * chore: update last few remaining mentions of payment pointers * chore(postman): remove references to payment pointer * chore: update another reference to pp --------- Co-authored-by: Nathan Lie <nathanlie@Nathans-MacBook-Pro.local> Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Co-authored-by: Sabine Schaller <sabine@interledger.org> Co-authored-by: Sarah Jones <sarah38186@gmail.com> Co-authored-by: Max Kurapov <maxkurapov@gmail.com> Co-authored-by: Max Kurapov <max@interledger.org>

BlairCurrey and others added 19 commits October 18, 2023 13:26

fix(backend): rm unused connection middleware

5f35ed9

fix(backend): rm get connection from receiver get

2596f56

refactor(backend): toOpenPaymentsType

388a8ac

refactor(backend): rm getByConnection

a7beb8a

chore: update openapi spec

9c8bf5f

fix(backend): update receiver

77907db

fix(backend): rm connection tests

5a25a69

refactor(backend): remove connectionService.getUrl

250fdfd

refactor(backend): rm Receiver.fromConnection

fedc3d2

fix(backend): spsps middleware ctx type

3fce224

refactor(backend): remove connection model

29c3154

refactor(backend): use updated mocks

ed18d01

fix(backend): comment cleanup

7a9dd1f

refactor(backend): update to new open payments type

6253f50

chore(backend): format

63c1425

fix(backend): re-rename paymentPointer -> walletAddress

8512740

fix(backend): use receiver constructor

9e2ccc1

chore(backend): update open-payments packages

43e2d5c

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Oct 18, 2023

BlairCurrey changed the title ~~Bc/1962/swap connection for methods~~ refactor(backend): swap connection for methods Oct 18, 2023

fix(backend): quote open payments type

04051d1

BlairCurrey commented Oct 19, 2023

View reviewed changes

fix(backend): receiver service tests

f00958a

mkurapov reviewed Oct 19, 2023

View reviewed changes

fix: re-rename paymentPointer -> walletAddress

f1e47bc

BlairCurrey added 2 commits October 19, 2023 11:37

refactor(backend): remove IncomingPayment.connectionId

49ddb99

fix(backend): bad receiver model test setup

05bcfcf

fix(backend): remove todo

318af8e

mkurapov reviewed Oct 19, 2023

View reviewed changes

BlairCurrey added 2 commits October 19, 2023 15:09

refactor(backend): rename connectionService to streamCredentialService

8d50c52

refactor(backend): rename connection file and move to ilp

8e2519f

BlairCurrey requested a review from mkurapov October 19, 2023 19:43

BlairCurrey added 5 commits October 19, 2023 22:33

refactor(backend): streamCredentialService.get to not return undefined

8396970

feat(backend): add method to quote create paths

6f3e16e

fix(backend): rm debug log

9fd8b16

Merge branch 'nl-open-payments-updates' into bc/1962/swap-connection-…

98039d9

…for-methods

fix(postman): rm extra port from url

e68b48e

sabineschaller reviewed Oct 20, 2023

View reviewed changes

fix(backend): return empty methods for complete or expired incomingPa…

718d962

…yment

njlie reviewed Oct 20, 2023

View reviewed changes

BlairCurrey added 3 commits October 20, 2023 14:16

fix(backend): toOpenPaymentsTypeWithMethods

5a4399a

refactor(backend): dont return stream credentials for complete or exp…

80b6f1f

…ired payments

fix(backend): use fixed open-payments paginationArgs

4f1f583

BlairCurrey requested a review from sabineschaller October 20, 2023 18:50

BlairCurrey added 2 commits October 20, 2023 15:21

fix(backend): add stream credential test cases

b1b0941

refactor(backend): implement openapi request changes

d01ea18

njlie reviewed Oct 20, 2023

View reviewed changes

docs: fix typos in Postman

65fbef0

njlie approved these changes Oct 20, 2023

View reviewed changes

njlie merged commit b941a72 into nl-open-payments-updates Oct 20, 2023

njlie deleted the bc/1962/swap-connection-for-methods branch October 20, 2023 22:12

njlie added a commit that referenced this pull request Oct 23, 2023

refactor(backend): swap connection for methods (#2054)

572dd09

Co-authored-by: Nathan Lie <lie4nathan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(backend): swap connection for methods #2054

refactor(backend): swap connection for methods #2054

BlairCurrey commented Oct 18, 2023 •

edited

Loading

BlairCurrey Oct 19, 2023 •

edited

Loading

mkurapov Oct 19, 2023

BlairCurrey commented Oct 19, 2023

BlairCurrey Oct 19, 2023

mkurapov Oct 19, 2023

mkurapov left a comment

mkurapov Oct 19, 2023

BlairCurrey Oct 19, 2023

mkurapov Oct 19, 2023

mkurapov Oct 19, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023

mkurapov Oct 19, 2023

mkurapov Oct 19, 2023

BlairCurrey commented Oct 19, 2023

mkurapov Oct 19, 2023

BlairCurrey Oct 19, 2023

sabineschaller Oct 20, 2023

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023

sabineschaller Oct 20, 2023

BlairCurrey Oct 20, 2023

sabineschaller Oct 20, 2023

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading

njlie Oct 20, 2023

BlairCurrey Oct 20, 2023 •

edited

Loading

njlie Oct 20, 2023

njlie commented Oct 20, 2023

	expect(receiver).toEqual({
	assetCode: incomingPayment.asset.code,
	assetScale: incomingPayment.asset.scale,

	test.each`
	state
	${IncomingPaymentState.Completed}
	${IncomingPaymentState.Expired}
	`(
	`returns undefined for $state incoming payment`,
	async ({ state }): Promise<void> => {
	await incomingPayment.$query(knex).patch({
	state,
	expiresAt:
	state === IncomingPaymentState.Expired ? new Date() : undefined
	})
	expect(connectionService.get(incomingPayment)).toBeUndefined()
	}
	)

refactor(backend): swap connection for methods #2054

refactor(backend): swap connection for methods #2054

Conversation

BlairCurrey commented Oct 18, 2023 • edited Loading

Changes proposed in this pull request

Context

Checklist

BlairCurrey Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Oct 19, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Oct 20, 2023 • edited Loading

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 commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Oct 20, 2023 • edited Loading

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 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie commented Oct 20, 2023

BlairCurrey commented Oct 18, 2023 •

edited

Loading

BlairCurrey Oct 19, 2023 •

edited

Loading

mkurapov Oct 19, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading

BlairCurrey Oct 20, 2023 •

edited

Loading