feat(backend): add local payment #2857

BlairCurrey · 2024-08-10T04:51:18Z

Changes proposed in this pull request

adds local payments (doesnt go through connector for quote/pay)

Context

Checklist

netlify · 2024-08-10T04:51:36Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`f120aab`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/673668b169615100085d8e54

BlairCurrey · 2024-08-10T04:55:58Z

packages/backend/migrations/20240809171654_add_ilp_quote_details_table.js

+      return knex.schema.alterTable('quotes', function (table) {
+        table.bigInteger('maxPacketAmount').notNullable().alter()
+        table.decimal('minExchangeRateNumerator', 64, 0).notNullable().alter()
+        table.decimal('minExchangeRateDenominator', 64, 0).notNullable().alter()
+        table
+          .decimal('lowEstimatedExchangeRateNumerator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('lowEstimatedExchangeRateDenominator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('highEstimatedExchangeRateNumerator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('highEstimatedExchangeRateDenominator', 64, 0)
+          .notNullable()
+          .alter()
+      })
+    })


this down will fail if local quotes are added before migration is rolled back run because they dont have these non null columns. We are setting back to null because that was the original state before the migration. What should we do?

leave them as nullable in the down. Doesnt fully restore the original state but not sure we can, short of losing the new quotes that dont fit in the previous model.

keep nullable and set to sensible(ish) defaults (derive from exchange rate)?

lose the new quotes

do nothing and require manual remediation

IDK, not sure any of these options are great, but its also kind of an edge case.

Let's split this up into multiple migrations, something like:

First one will backfill estimatedExchangeRate, and mark the field as required

second one will create ilpQuoteDetails

third one will backfill ilpQuoteDetails

Then we deploy the code changes to start reading from ilpQuoteDetails

last one will drop ilp fields from quotes

this way, we dont run into the risk of losing data

Let's split this up into multiple migrations, something like:

First one will backfill estimatedExchangeRate, and mark the field as required

second one will create ilpQuoteDetails

third one will backfill ilpQuoteDetails

Then we deploy the code changes to start reading from ilpQuoteDetails

last one will drop ilp fields from quotes

this way, we dont run into the risk of losing data

yeah I suppose multiple migrations make sense. More control over up/down if needed without downside.

In terms of this part:

third one will backfill ilpQuoteDetails

Then we deploy the code changes to start reading from ilpQuoteDetails

last one will drop ilp fields from quotes

Is the idea that we put the drop ilp fields from quotes in another release and communicate as much in patch notes? I mean I dont see a way to ensure integrators arent just upgrading multiple releases and running them all at once in that case. I guess this is still better and if some integrator does run into an issue we can fix it before we release the migration that will lose the data.

I think this all will go in one release, I was just thinking of how to make the changes safely (or in separate PRs).

If you want, we can have this:

Then we deploy the code changes to start reading from ilpQuoteDetails

released the same time as we backfill ilpQuoteDetails (and drop ilp fields from quotes for that matter)

thinking about it more, i dont think we can split the create ilpQuoteDetails table and the migration to backfill it.

if migrations are run through create ilpQuoteDetails (but not backfilled), and then quotes are created in the application with ilpQuoteDetails, then they are backfilled, then the quotes created via the application will try to be inserted again in the ilpQuoteDetails (and it will fail because the quoteId has a unique constraint). I think we need to enforce they are run together by keeping them in the same migration. I think splitting the other ones as described are still fine.

- trouble updating the services. this may not be the best way. also not sure I can prevent querying for non-local quotes on LocalQuote (and vice-versa for ILPQuote). - the idea behind seperate models was a firm Quote type (it has the ilp specific props or it doesnt instead of 1 type MAYBE having them) - typeguards could work instead but seemed messier. or maybe I can still have seperate quote service methods returning different types?

…e details - includes some WIP changes including gql field removal and handling missing ilp quote details

…isLocal - stubs in isLocal variable in place of actualy receiver.isLocal property - updates/adds test. new test expectedly fails because there is no way to set the receiver to local yet. can implement after isLocal is added to receiver

packages/backend/src/payment-method/local/service.ts

packages/backend/src/open_payments/payment/outgoing/service.ts

mkurapov · 2024-08-19T15:57:35Z

packages/backend/migrations/20240809171654_add_ilp_quote_details_table.js

+      return knex.schema.alterTable('quotes', function (table) {
+        table.bigInteger('maxPacketAmount').notNullable().alter()
+        table.decimal('minExchangeRateNumerator', 64, 0).notNullable().alter()
+        table.decimal('minExchangeRateDenominator', 64, 0).notNullable().alter()
+        table
+          .decimal('lowEstimatedExchangeRateNumerator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('lowEstimatedExchangeRateDenominator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('highEstimatedExchangeRateNumerator', 64, 0)
+          .notNullable()
+          .alter()
+        table
+          .decimal('highEstimatedExchangeRateDenominator', 64, 0)
+          .notNullable()
+          .alter()
+      })
+    })


Let's split this up into multiple migrations, something like:

First one will backfill estimatedExchangeRate, and mark the field as required

second one will create ilpQuoteDetails

third one will backfill ilpQuoteDetails

Then we deploy the code changes to start reading from ilpQuoteDetails

last one will drop ilp fields from quotes

this way, we dont run into the risk of losing data

packages/backend/src/payment-method/ilp/service.ts

BlairCurrey · 2024-08-20T19:19:07Z

packages/backend/src/open_payments/payment/outgoing/lifecycle.ts

-        (payment.quote.estimatedExchangeRate ||
-          payment.quote.lowEstimatedExchangeRate.valueOf())
-    )
+    Math.ceil(Number(alreadySentAmount) * payment.quote.estimatedExchangeRate)


wont have the low lowEstimatedExchangeRate to fall back on anymore (it will be optional) so I ensured estimatedExchangeRate is always set in the migration. Set it to be lowEstimatedExchangeRate where null, just like we're setting it in the ilp getQuote method.

- for new local payment method service, sentAmount matches debitAmount, whereas is matches receive amount for local/remote ilp payments

…e/outgoing payment

mkurapov

I tested local payment same currency and cross currency payments (without fees) and everything works well for me. Just a few outstanding comments

mkurapov · 2024-10-30T23:29:42Z

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

+      const maxPacketAmount = quote.additionalFields.maxPacketAmount as bigint
+      graph.ilpQuoteDetails = {
+        maxPacketAmount:
+          MAX_INT64 < maxPacketAmount ? MAX_INT64 : maxPacketAmount, // Cap at MAX_INT64 because of postgres type limits.
+        minExchangeRate: quote.additionalFields.minExchangeRate as Pay.Ratio,
+        lowEstimatedExchangeRate: quote.additionalFields
+          .lowEstimatedExchangeRate as Pay.Ratio,
+        highEstimatedExchangeRate: quote.additionalFields
+          .highEstimatedExchangeRate as Pay.PositiveRatio
+      }
+    }


^ Still applies I believe

mkurapov · 2024-10-30T23:32:22Z

packages/backend/src/payment-method/local/service.ts

+        )
+    }
+  }
+  await trxOrError.post()


(I'm guessing you are still working on addressing changes), but this one as well

mkurapov · 2024-10-30T23:38:20Z

packages/backend/src/accounting/psql/service.ts

@@ -264,7 +264,7 @@ export async function createTransfer(
            debitAccount: accountMap[transfer.sourceAccountId],
            creditAccount: accountMap[transfer.destinationAccountId],
            amount: transfer.amount,
-            timeoutMs: BigInt(args.timeout * 1000)


Should this stay required? since we do not support single phase transfer in this flow just yet

yeah good point, made it required again

...API - only locally/Peer-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru

…r-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru Co-authored-by: Max Kurapov <max@interledger.org>

Making optional depends on single phase transfer

raducristianpopa

Migrations look good. Double-checked again with data from the previous test wallet.

raducristianpopa · 2024-11-01T11:34:11Z

packages/backend/migrations/20240916181643_require_estimated_exchange_rate.js

+
+exports.up = function (knex) {
+  return knex('quotes')
+    .whereNull('estimatedExchangeRate')


In which cases the estimatedExchangeRate was null?

I dont know of any specific cases where it would be null, but since it's nullable we should handle that possibility.

- add runtime check to ilp payment method implementation requireing it

mkurapov · 2024-11-04T17:20:47Z

...API - only locally/Peer-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru

@@ -0,0 +1,73 @@
+meta {
+  name: Create Receiver -local Incoming Payment-


I think still need to change the actual file name to reflect this name

mkurapov · 2024-11-04T19:09:04Z

packages/backend/src/open_payments/payment/outgoing/service.ts

@@ -429,7 +429,7 @@ async function validateGrantAndAddSpentAmountsToPayment(
    .andWhereNot({
      id: payment.id
    })
-    .withGraphFetched('[quote.asset]')
+    .withGraphFetched('quote.asset')


does this return the same result?

yeah, the brackets are just required if there are multiple joins. could have left as-is but noticed while cleaning up other areas (when I stopped joining everything on ilpQuoteDetails).

mkurapov · 2024-11-04T19:15:56Z

packages/backend/src/payment-method/handler/service.ts


 export interface StartQuoteOptions {
+  quoteId?: string


should this be required?

its only used by the ilp implementation (for the ilpQuoteDetails). the ilp getQuote throws an error if not provided. I figure we dont want to require it for the local getQuote since its not used.

It's a little loose type-wise but I don't think there is a better way unless we make more fundamental changes to the payment handler or something. Unless we simply want to require it even for the local getQuote?

Makes sense, we can keep it as optional in that case

mkurapov · 2024-11-04T19:18:49Z

test/integration/integration.test.ts

-      }
+          value: BigInt(quote.receiveAmount.value)
+        })
+        // TODO: fix bug where fixed-send (regardless of local/remote) is not completeing


I think because the incoming payment doesn't have an amount it would need to be completed manually

whoops, thank you... outdated comment

mkurapov · 2024-11-04T19:38:16Z

packages/backend/src/payment-method/local/service.ts

+  } else if (receiveAmount) {
+    receiveAmountValue = receiveAmount.value
+    const converted = await convert({
+      sourceAmount: receiveAmountValue,


Thinking about this: since we are moving money between source <> destination asset in the end (e.g. USD > EUR) we actually need sourceAmount & sourceAsset to always be the sending currency (USD),
since in reality it may cost more for a certain provider to move USD > EUR vs EUR > USD.

Basically, if we need to get the receiveAmount, instead of using EUR <> USD rate, we would do
converted = ceil(receiveAmount / USD <> EUR rate)

I hardcoded in the rates because I'm not sure where to get the sending currency (USD), since there is no debitAmount here. Also, I'm not actually sure how to trigger this block as opposed to receiver.incomingAmount further down. That's what the fixed send p2p example triggers. Although I think your comment applies to both places so that detail probably doesnt matter.

Anyways, just wanted to validate this. With your calculation patched in and doing a fixed send local payment, I see an outgoing payment with these amounts:

"receiveAmount": { "assetCode": "EUR", "assetScale": 2, "value": "1000" }, "debitAmount": { "assetCode": "USD", "assetScale": 2, "value": "1221" }, "sentAmount": { "assetCode": "USD", "assetScale": 2, "value": "1099" },

The way it is now, the receiveAmount is the same and debitAmount and sentAmount are 1 higher:

"receiveAmount": { "assetCode": "EUR", "assetScale": 2, "value": "1000" }, "debitAmount": { "assetCode": "USD", "assetScale": 2, "value": "1222" }, "sentAmount": { "assetCode": "USD", "assetScale": 2, "value": "1100" },

For the same payment over ILP I see the same 1099 sentAmount from your revised calculation but a different debitAmount. Not sure if slippage would account for this (in which case there is no problem) or something else.

"receiveAmount": { "assetCode": "EUR", "assetScale": 2, "value": "1000" }, "debitAmount": { "assetCode": "USD", "assetScale": 2, "value": "1234" }, "sentAmount": { "assetCode": "USD", "assetScale": 2, "value": "1099" },

I'm not sure where to get the sending currency

We should have this on the walletAddress.asset. So for those two branches without a debitAmount, we do the calculation as described above:
debitAmountValue = ceil(receiveAmount / (scaled USD <> EUR rate))
(scaled because we could have a difference in assetScale)

Also, the estimated exchange rate can be the exchange rate that's received directly from the rates service, instead of having to do estimatedExchangeRate: Number(receiveAmountValue) / Number(debitAmountValue)

🤦 Of course, the destinationAsset passed in to the rates call.

- comment indicated there was a bug where there was not. incoming payment not completing was expected because it doesnt have an amount and didnt expire

BlairCurrey · 2024-11-14T16:43:56Z

packages/backend/src/rates/util.ts

+export interface ConvertResults {
+  amount: bigint
+  scaledExchangeRate: number
+}


I preferred to return the scaledExchangeRate here instead of re-calculating in local getQuote because:

it should always be the same and duplicating the calculation creates opportunity to diverge.

it was kind of a lot of code duplication and kinda specific stuff I was doing for each conversion case debitAmount, receiver.incomingAmount, receiveAmount. just felt like this was cleaner.

I mention it though because it did require updating to handle the type in many places where we didnt otherwise care about it.

BlairCurrey · 2024-11-14T16:47:34Z

packages/backend/src/rates/service.ts

-    if (sameCode) return convert({ exchangeRate: 1.0, ...opts })
+  async convert(opts: RateConvertOpts): Promise<ConvertResults | ConvertError> {
+    const {
+      reverseDirection = false,


convert function is same as before except this reverseDirection controls which conversion function is used (convert or new convertReverse). Alternatively considered making a different ratesService.convertReverse - perhaps thats a bit more explicit but I didn't have a strong preference and found this to be simpler.

mkurapov · 2024-11-14T17:22:13Z

packages/backend/src/rates/util.ts

+export function convertReverse(opts: ConvertOptions): ConvertResults {
+  const scaleDiff = opts.sourceAsset.scale - opts.destinationAsset.scale
+  const scaledExchangeRate = opts.exchangeRate * 10 ** scaleDiff
+
+  return {
+    amount: BigInt(Math.ceil(Number(opts.sourceAmount) / scaledExchangeRate)),
+    scaledExchangeRate
+  }


This looks good and returing scaledExchangeRate makes a lot of sense, I'm wondering though whether convertReverse might be unclear.

What do you think about the apporoach of having either sourceAmount OR destinationAmount provided as an argument to this function?
In our example, USD -> EUR, USD would be the sourceAsset, and when we pass in destinationAmount in EUR (without providing sourceAmount) the function would figure out out the proper direction.

feat(backend): add local payment quote migration

bb77152

github-actions bot added the pkg: backend Changes in the backend package. label Aug 10, 2024

BlairCurrey commented Aug 10, 2024

View reviewed changes

BlairCurrey changed the title ~~feat(backend): add local payment quote migration~~ feat(backend): add local payment Aug 10, 2024

BlairCurrey added 2 commits August 12, 2024 09:54

refactor(backend): change model/services to reflect optional ilp quot…

257fd01

…e details - includes some WIP changes including gql field removal and handling missing ilp quote details

github-actions bot added type: tests Testing related type: source Changes business logic labels Aug 13, 2024

BlairCurrey added 6 commits August 14, 2024 09:54

Merge branch 'main' into bc/2834/non-ilp-local-payments

d9fa20e

feat(backend): WIP local payment method with getQuote

2e76ee2

feat(backend): add local payment method to payment method handler

4fab5d8

chore(backend): fix format

b17db75

Merge branch 'main' into bc/2834/non-ilp-local-payments

6e37420

mkurapov reviewed Aug 19, 2024

View reviewed changes

BlairCurrey added 2 commits August 20, 2024 14:16

feat(backend): local payment .pay

320ee21

Merge branch 'main' into bc/2834/non-ilp-local-payments

07569a7

BlairCurrey commented Aug 20, 2024

View reviewed changes

BlairCurrey added 11 commits August 20, 2024 15:50

chore: rm comment

f3d5f5d

chore: WIP debugging wrong sentAmount

d795ce2

- for new local payment method service, sentAmount matches debitAmount, whereas is matches receive amount for local/remote ilp payments

chore: rm comment

251c60a

Merge branch 'main' into bc/2834/non-ilp-local-payments

8f0bcb8

feat(backend): use receiver.isLocal to control payment method in quot…

eca01f1

…e/outgoing payment

Merge branch 'main' into bc/2834/non-ilp-local-payments

770d7ea

Merge branch 'main' into bc/2834/non-ilp-local-payments

3849db7

fix(backend): added source amount

170bcc3

fix(backend): p2p case (cross currency, local, fixed send)

88618ce

fix: lint error

823c512

chore: rm logs

2e9f043

github-actions bot removed the pkg: auth Changes in the GNAP auth package. label Oct 29, 2024

BlairCurrey added 4 commits October 30, 2024 09:56

fix(integration): wa typo

c40db9e

chore: rm bruno test examples

0dbb2d3

refactor: mv debitAmountMinusFees to fee calc and clarify TODO

61b1010

Merge branch 'main' into bc/2834/non-ilp-local-payments

0dea97d

mkurapov reviewed Oct 31, 2024

View reviewed changes

BlairCurrey mentioned this pull request Oct 31, 2024

Rework Fixed Send Fee Calculation #3055

Open

BlairCurrey and others added 3 commits October 31, 2024 11:19

Update bruno/collections/Rafiki/Examples/Admin API - only locally/Pee…

6c995d9

…r-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru Co-authored-by: Max Kurapov <max@interledger.org>

fix: make timeout required again

86f6db6

Making optional depends on single phase transfer

feat: error when post fails in local pay

3fc6924

raducristianpopa reviewed Nov 1, 2024

View reviewed changes

BlairCurrey added 4 commits November 1, 2024 10:28

refactor(backend): add optional quoteId to getQuote args

6634b8f

- add runtime check to ilp payment method implementation requireing it

refactor: rm ilp quote details out of quote service

6706557

refactor: insert ilp quote details in ilp getQuote

998ee25

fix(backend): payment handler test

a4cf57a

mkurapov reviewed Nov 5, 2024

View reviewed changes

BlairCurrey added 6 commits November 5, 2024 11:54

chore(bruno): rename request

1835bc4

chore(integration): rm erroneous todo comment

7db833b

- comment indicated there was a bug where there was not. incoming payment not completing was expected because it doesnt have an amount and didnt expire

Merge branch 'main' into bc/2834/non-ilp-local-payments

4437438

Merge branch 'main' into bc/2834/non-ilp-local-payments

3149428

fix(backend): local quote amounts, estimatedExchangeRaet

fe63991

chore(backend): format

e6cad1e

BlairCurrey commented Nov 14, 2024

View reviewed changes

BlairCurrey requested a review from mkurapov November 14, 2024 16:49

mkurapov reviewed Nov 14, 2024

View reviewed changes

refactor(backend): rate convert methods to be explicit

f120aab

mkurapov approved these changes Nov 18, 2024

View reviewed changes

BlairCurrey merged commit eae95ad into main Nov 18, 2024
42 checks passed

BlairCurrey deleted the bc/2834/non-ilp-local-payments branch November 18, 2024 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): add local payment #2857

feat(backend): add local payment #2857

BlairCurrey commented Aug 10, 2024 •

edited

Loading

netlify bot commented Aug 10, 2024 •

edited

Loading

BlairCurrey Aug 10, 2024 •

edited

Loading

mkurapov Aug 19, 2024

BlairCurrey Aug 20, 2024 •

edited

Loading

mkurapov Sep 16, 2024

BlairCurrey Sep 16, 2024 •

edited

Loading

mkurapov Aug 19, 2024

BlairCurrey Aug 20, 2024 •

edited

Loading

mkurapov left a comment

mkurapov Oct 30, 2024

mkurapov Oct 30, 2024

mkurapov Oct 30, 2024

BlairCurrey Oct 31, 2024

raducristianpopa left a comment

raducristianpopa Nov 1, 2024

BlairCurrey Nov 1, 2024

mkurapov Nov 4, 2024

mkurapov Nov 4, 2024

BlairCurrey Nov 5, 2024 •

edited

Loading

mkurapov Nov 4, 2024

BlairCurrey Nov 5, 2024 •

edited

Loading

mkurapov Nov 5, 2024

mkurapov Nov 4, 2024

BlairCurrey Nov 5, 2024

mkurapov Nov 4, 2024 •

edited

Loading

BlairCurrey Nov 7, 2024 •

edited

Loading

mkurapov Nov 11, 2024 •

edited

Loading

mkurapov Nov 11, 2024

BlairCurrey Nov 11, 2024

BlairCurrey Nov 14, 2024 •

edited

Loading

BlairCurrey Nov 14, 2024

mkurapov Nov 14, 2024

		@@ -0,0 +1,73 @@
		meta {
		name: Create Receiver -local Incoming Payment-

feat(backend): add local payment #2857

feat(backend): add local payment #2857

Conversation

BlairCurrey commented Aug 10, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Aug 10, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

BlairCurrey Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Aug 20, 2024 • edited Loading

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

raducristianpopa 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

BlairCurrey Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Nov 5, 2024 • 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

mkurapov Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

mkurapov Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Aug 10, 2024 •

edited

Loading

netlify bot commented Aug 10, 2024 •

edited

Loading

BlairCurrey Aug 10, 2024 •

edited

Loading

BlairCurrey Aug 20, 2024 •

edited

Loading

BlairCurrey Sep 16, 2024 •

edited

Loading

BlairCurrey Aug 20, 2024 •

edited

Loading

BlairCurrey Nov 5, 2024 •

edited

Loading

BlairCurrey Nov 5, 2024 •

edited

Loading

mkurapov Nov 4, 2024 •

edited

Loading

BlairCurrey Nov 7, 2024 •

edited

Loading

mkurapov Nov 11, 2024 •

edited

Loading

BlairCurrey Nov 14, 2024 •

edited

Loading