feat: payment method handler service (quoting) #1974
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
3e3ac24 to
b6d3ea7
Compare
| test.each` | ||
| incomingAssetCode | incomingAmountValue | debitAssetCode | expectedDebitAmount | exchangeRate | slippage | description | ||
| ${'EUR'} | ${100n} | ${'USD'} | ${101n} | ${1.0} | ${0} | ${'same currency, no slippage'} | ||
| ${'USD'} | ${100n} | ${'USD'} | ${102n} | ${1.0} | ${0.01} | ${'same currency, some slippage'} | ||
| ${'EUR'} | ${100n} | ${'USD'} | ${113n} | ${0.9} | ${0.01} | ${'cross currency, exchange rate < 1'} | ||
| ${'EUR'} | ${100n} | ${'USD'} | ${51n} | ${2.0} | ${0.01} | ${'cross currency, exchange rate > 1'} | ||
| `( |
There was a problem hiding this comment.
This is a good demonstration of the interledger Pay library behaviour. This may look odd, but some of these are programmed into the library, for example, the off-by-one "issue", where an extra 1 unit is added to the debitAmount to account for possible packet loss.
Some behavior is weird for example, slippage being taken into account for a payment within the same asset.
We have taken note of this behaviour, and will continue investigating.
| } | ||
| } | ||
|
|
||
| async function getQuote( |
There was a problem hiding this comment.
basically copy of the ILP-related quoteService.create method
| additionalFields: { | ||
| lowEstimatedExchangeRate: ilpQuote.lowEstimatedExchangeRate, | ||
| highEstimatedExchangeRate: ilpQuote.highEstimatedExchangeRate, | ||
| minExchangeRate: ilpQuote.minExchangeRate, | ||
| maxPacketAmount: ilpQuote.maxPacketAmount | ||
| } |
There was a problem hiding this comment.
ILP specific stuff is now in the additionalFields (will be the same case for other payment methods)
| exchangeRate?: number | ||
| } & ({ debitAmountValue: bigint } | { receiveAmountValue: bigint }) | ||
|
|
||
| export function mockQuote( |
There was a problem hiding this comment.
not used in this PR, but will make sense in the next
| container.singleton('ilpPaymentService', async (deps) => { | ||
| return createIlpPaymentService({ | ||
| logger: await deps.use('logger'), | ||
| knex: await deps.use('knex'), | ||
| config: await deps.use('config'), | ||
| makeIlpPlugin: await deps.use('makeIlpPlugin'), | ||
| ratesService: await deps.use('ratesService') | ||
| }) | ||
| }) | ||
|
|
||
| container.singleton('paymentMethodHandlerService', async (deps) => { | ||
| return createPaymentMethodHandlerService({ | ||
| logger: await deps.use('logger'), | ||
| knex: await deps.use('knex'), | ||
| ilpPaymentService: await deps.use('ilpPaymentService') | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Not used yet, but will be in the next PR
| export interface StartQuoteOptions { | ||
| paymentPointer: PaymentPointer | ||
| debitAmount?: Amount | ||
| receiveAmount?: Amount | ||
| receiver: Receiver | ||
| } | ||
|
|
||
| export interface PaymentQuote { | ||
| paymentPointer: PaymentPointer | ||
| receiver: Receiver | ||
| debitAmount: Amount | ||
| receiveAmount: Amount | ||
| additionalFields: Record<string, unknown> | ||
| } | ||
|
|
||
| export interface PaymentMethodService { | ||
| getQuote(quoteOptions: StartQuoteOptions): Promise<PaymentQuote> | ||
| } |
There was a problem hiding this comment.
Interfaces that will need to be satisfied for all payment method handlers
| container.singleton('paymentMethodHandlerService', async (deps) => { | ||
| return createPaymentMethodHandlerService({ | ||
| logger: await deps.use('logger'), | ||
| knex: await deps.use('knex'), | ||
| ilpPaymentService: await deps.use('ilpPaymentService') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
I understand this is a minor part of integrating different payment methods into Rafiki, but I'm currently a bit confused. Wouldn't it be more appropriate to have this as factory that is instantiated at runtime based on the payment method?
What I mean by that:
export async function createPaymentMethodHandlerService({
logger,
knex,
- ilpPaymentService
+ paymentMethod // ('ilp', '...');
}: ServiceDependencies): Promise<PaymentMethodHandlerService> {
const log = logger.child({
service: "PaymentMethodHandlerService",
});
const deps: ServiceDependencies = {
logger: log,
knex,
ilpPaymentService,
};
+ const paymentMethod = new Payment(paymentMethod)
return {
- getQuote: (method, quoteOptions) =>
- getPaymentMethodService(deps, method).getQuote(quoteOptions),
+ getQuote: (quoteOptions) =>
+ paymentMethod.getQuote(quoteOptions),
};
}There was a problem hiding this comment.
To try and build on this idea, maybe it could take in an object whose keys are the names of supported payment methods, and the values are its services. So getQuote would then look like:
/*
PaymentMethods: {
ilp: IlpPaymentService
...other future payment method services
}
paymentMethods: PaymentMethods
*/
getQuote: (method, quoteOptions) => paymentMethods[method].getQuote(quoteOptions)
There was a problem hiding this comment.
@raducristianpopa the PaymentMethodHandlerService is our "factory" in this case already, but instead of a new instance of a class we're just returning the service/singleton for the specific payment method that extends from PaymentMethodService. I could rename PaymentMethodHandlerService to PaymentMethodFactory if that seems better?
@njlie yeah I think I'll change it to the suggestion since the types make it sort of "safe" and prevent passing in an invalid payment method 👍
Changes proposed in this pull request
paymentManagerHandlerServicewhich will know how to call different payment method services for each payment method (for now, justilpPaymentService)ilpPaymentServicethat has agetQuotemethod which handles ILP payments only (essentially copied over fromQuoteService.createadditionalFieldsproperty to quotes to store any payment method-specific quote informationNote: these new services are only added in this PR, not used (to break up into smaller PRs). Next up:
paymentMethodHandlerService.getQuoteinQuoteService.createto abstract away ILP quotingContext
Progress towards #1967:
Checklist
fixes #number