feat: use payment manager handler in quote service #1994
Conversation
…nt-method-handler-quoting # Conflicts: # packages/backend/src/index.ts # packages/backend/src/payment-method/handler/service.test.ts # packages/backend/src/payment-method/handler/service.ts
github-actions
bot
added
type: tests
| assetScale: receiver.assetScale | ||
| }, | ||
| debitAmount: quote.debitAmount, | ||
| receiveAmount: quote.receiveAmount, | ||
| // Cap at MAX_INT64 because of postgres type limits. | ||
| maxPacketAmount: |
There was a problem hiding this comment.
Thoughts on refactoring Quote so that ILP-related fields are more delineated from "base" quote stuff? I was thinking it might help keep the amount of columns in quote reasonable when other payment methods become supported.
There was a problem hiding this comment.
💯
I have it in a separate issue:
# Conflicts: # packages/backend/src/index.ts
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| */ | ||
| exports.up = function (knex) { | ||
| return knex.schema.alterTable('quotes', (table) => { | ||
| table.json('additionalFields').nullable() |
There was a problem hiding this comment.
Is this meant to be temporary until ilp related stuff is migrated in #2000?
otherwise I'm concerned about the schemaless-ness of jsonb
There was a problem hiding this comment.
Long term, temporary. I need to determine what non generic items are needed for the mojaloop quote, and decide how to store those as well. I imagine we end up having a separate tables for method-specific quoting/transfer properties
| ${1000} | ${0} | ${0} | ${1000n} | ${'no fees'} | ||
| ${1000n} | ${150} | ${0} | ${1150n} | ${'fixed fee'} | ||
| ${1000n} | ${0} | ${200} | ${1020n} | ${'basis point fee'} | ||
| ${1000n} | ${150} | ${200} | ${1170n} | ${'fixed and basis point fee'} | ||
| `( | ||
| '$description', | ||
| withConfigOverride( | ||
| () => config, | ||
| { slippage: 0 }, | ||
| async ({ | ||
| incomingAmountValue, | ||
| fixedFee, | ||
| basisPointFee, | ||
| expectedQuoteDebitAmountValue | ||
| }): Promise<void> => { | ||
| const incomingPayment = await createIncomingPayment(deps, { | ||
| paymentPointerId: receivingPaymentPointer.id, | ||
| incomingAmount: { | ||
| assetCode: asset.code, | ||
| assetScale: asset.scale, | ||
| value: incomingAmountValue | ||
| } | ||
| }) | ||
| await Fee.query().insertAndFetch({ | ||
| assetId: asset.id, | ||
| type: FeeType.Sending, | ||
| fixedFee, | ||
| basisPointFee | ||
| }) | ||
|
|
||
| const quote = await quoteService.create({ | ||
| paymentPointerId: sendingPaymentPointer.id, | ||
| receiver: incomingPayment.getUrl(receivingPaymentPointer) | ||
| }) | ||
| assert.ok(!isQuoteError(quote)) | ||
|
|
||
| expect(quote.debitAmount).toEqual({ | ||
| async ({ | ||
| incomingAmountValue, | ||
| fixedFee, | ||
| basisPointFee, | ||
| expectedQuoteDebitAmountValue | ||
| }): Promise<void> => { |
There was a problem hiding this comment.
Can you elaborate on how there isnt an off by one issue here? Did we gain any insight into that issue?
As I recall the default slippage introduced some variance in this test and manual tests (via postman), which is why I tried to test without slippage originally. Although it appears that using the default slippage doesnt modify the values at all here and there is no off by one issue?
There was a problem hiding this comment.
This one is because the actual quote itself is mocked, there's no actual ILP quoting happening here. The off-by-one issue was demonstrated in the other PR: #1974 (comment)
There was a problem hiding this comment.
aha, makes sense. Good, glad there is something preserving that issue.
Changes proposed in this pull request
quoteService.createby usingpaymentMethodHandlerService.getQuote-Adding
additionalFieldsto the quote model for storing method-specific information that may need to be stored in between quoting and transfers (like the ILP maxPacketAmount and exchangeRate info)Context
Progress towards #1967
In the previous PR, #1974, the
PaymentMethodHandlerServicewas introduced, but not used by anything. In this PR,quoteServicewill now use this service to get a quote, without needing to know exactly how an "ILP" quote happens.Checklist
fixes #number