Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

663/mk/use open payments client #706

Merged
merged 69 commits into from
Nov 4, 2022
Merged

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Oct 28, 2022

Changes proposed in this pull request

  • This PR uses the client from the open-payments package to make remote open-payments server calls when trying to find a receiver of a payment, either through an incoming-payment or a connection. This basically incrorporates the client into the existing flow that OpenPaymentsClientService had for fetching the remote open-payments resources. Since OpenPaymentsClientService will no longer be used in the way that its name suggests, it's been renamed to Receiver service, which is an abstraction of an OpenPayments incoming-payment or connection.
  • In addition, the incoming payment and connection models now have a .toOpenPaymentsType method, that enables entities to go from the domain model to an OpenPayments type, the type that comes from the open api spec type generation in the open-payments package.

Context

Progress toward #663, and roadmap item interledger/roadmap#20

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

mkurapov and others added 30 commits October 12, 2022 16:41
@mkurapov mkurapov mentioned this pull request Oct 31, 2022
4 tasks
Base automatically changed from 663/mk/update-response-validation to main October 31, 2022 18:44
@@ -0,0 +1,217 @@
import { IocContract } from '@adonisjs/fold'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test looks quite a lot different that the previous OpenPaymentsClientService one, there are some duplicated code but IMO more straightforward

Comment on lines +151 to +157
const clientGetIncomingPaymentSpy = jest
.spyOn(openPaymentsClient.incomingPayment, 'get')
.mockImplementationOnce(async () =>
incomingPayment.toOpenPaymentsType({
ilpStreamConnection: connectionService.get(incomingPayment)
})
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of using nock here, I just stub out the return value of the openPaymentsClient call

Comment on lines +13 to +16
interface OpenPaymentsConnectionWithIlpAddress
extends Omit<OpenPaymentsConnection, 'ilpAddress'> {
ilpAddress: IlpAddress
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfourtunately need this because the base constructor needs to have ilpAddress: IlpAddress instead of a string. Using static fromOpenPaymentsConnection as a wrapper for the constructor, since we can return undefined from it directly

) {}
}

export class Connection extends ConnectionBase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to its own model.ts file

Comment on lines +39 to +42
const paymentPointer = await createPaymentPointer(deps)
const incomingPayment = await createIncomingPayment(deps, {
paymentPointerId: paymentPointer.id
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be able to just stub out the instances here instead of actually hitting the services/dbs for example, but that is for a future improvement

@mkurapov
Copy link
Contributor Author

mkurapov commented Nov 2, 2022

I merged off of an earlier branch (which is now merged in), hence the many commits

@mkurapov mkurapov marked this pull request as ready for review November 2, 2022 23:02
@mkurapov mkurapov requested a review from wilsonianb November 2, 2022 23:02
@@ -112,6 +114,10 @@ export function initIocContainer(
const config = await deps.use('config')
return await createOpenAPI(config.authServerSpec)
})
container.singleton('openPaymentsClient', async (deps) => {
const logger = await deps.use('logger')
return createOpenPaymentsClient({ logger })
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how #722 shapes up, this might also take config.privateKey + a key id

import { ILPStreamConnection } from 'open-payments'
import { IncomingPayment } from '../payment/incoming/model'

export type ConnectionJSON = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't necessarily have to happen in this pr, but I think the routes should use generated types from open-payments

Comment on lines 250 to 259
incomingAmount: this.incomingAmount
? {
...this.incomingAmount,
value: this.incomingAmount.value.toString()
}
: undefined,
receivedAmount: {
...this.receivedAmount,
value: this.receivedAmount.value.toString()
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
incomingAmount: this.incomingAmount
? {
...this.incomingAmount,
value: this.incomingAmount.value.toString()
}
: undefined,
receivedAmount: {
...this.receivedAmount,
value: this.receivedAmount.value.toString()
},
incomingAmount: this.incomingAmount
? serializeAmount(this.incomingAmount)
: undefined,
receivedAmount: serializeAmount(this.receivedAmount),

and change AmountJSON to an open-payments generated type?

export function serializeAmount(amount: Amount): AmountJSON {
return {
value: amount.value.toString(),
assetCode: amount.assetCode,
assetScale: amount.assetScale
}
}

Copy link
Contributor Author

@mkurapov mkurapov Nov 4, 2022

Choose a reason for hiding this comment

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

I will incorporate serializeAmount but will hold off removing AmountJSON, as it will balloon this PR a bit. Removing TypeJSON from the whole package can come as a separate/standalone PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I keep having to remind myself that you've managed to avoid changing open-payments at all in this pr.

packages/backend/src/open_payments/receiver/model.test.ts Outdated Show resolved Hide resolved
@mkurapov mkurapov requested a review from wilsonianb November 4, 2022 15:16
@mkurapov mkurapov merged commit 7c4f93b into main Nov 4, 2022
@mkurapov mkurapov deleted the 663/mk/use-open-payments-client branch November 4, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: ci Changes to the CI type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants