chore(backend): allow asset scale 2 to asset scale 9 payments #2748

mkurapov · 2024-05-27T17:08:05Z

Changes proposed in this pull request

Fix issue where we are unable to send payments between asset scale 2 and asset scale 9 wallet addresses because the ILP rate probe exchange rate numerators were too big to store in the DB as bigints. See details in issue.
Update small logic where we allow fees to make up more than 50% of receive amount: https://github.com/interledger/rafiki/pull/2748/files#r1616401580
Some minor error/logging additions, refactoring create quote function slightly

Context

Fixes #2747

Checklist

…inator fields in the quotes table

netlify · 2024-05-27T17:08:21Z

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

Name	Link
🔨 Latest commit	`5242127`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66570fd292791300088c1db8

mkurapov · 2024-05-27T17:09:15Z

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

+    if (receipt.error) {
+      throw receipt.error
+    }


move this check before the debug logger, so we don't continue to log "'ILP payment completed'" when there is an error

mkurapov · 2024-05-27T17:10:06Z

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

          client: options.client,
          feeId: sendingFee?.id,
-          estimatedExchangeRate: quote.estimatedExchangeRate,
-          additionalFields: quote.additionalFields


don't store additionalFields anymore

mkurapov · 2024-05-27T17:25:25Z

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

@@ -245,20 +246,6 @@ async function pay(
  }
 }

-function fromJSONtoRatio(ratio: unknown): Pay.Ratio {


not needed anymore, we don't use additionalFields quote field to get these ratio amounts

mkurapov · 2024-05-27T21:35:36Z

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

-    if (receiveAmountValue <= exchangeAdjustedFees) {
-      throw QuoteError.NegativeReceiveAmount
-    }


Behaviour change:
We used to throw if the receiveAmount was less than the fees: e.g. if receiveAmountValue = 100, and exchangeAdjustedFees were 60, we would throw, even though receiveAmountValue of 40 seems like it should be possible to do. (In other words, more than 50% of the transfer in this situation goes to fees, which wouldn't be nice for the user, but not necessarily a problem/bug)

We discussed this in a slack thread.

mkurapov · 2024-05-27T21:38:49Z

packages/backend/migrations/20240524102509_drop_quote_ratio_columns.js

+    table.string('highEstimatedExchangeRateNumerator').alter()
+    table.string('highEstimatedExchangeRateDenominator').alter()
+
+    table.dropColumn('additionalFields')


Originally, additionalFields was a temp solution when building out support for multiple payment methods. Now I decided to get rid of this extra field, since we will not be building that out.

mkurapov · 2024-05-27T22:28:22Z

packages/backend/migrations/20240524102509_drop_quote_ratio_columns.js

+    table.decimal('minExchangeRateNumerator', 64, 0).alter()
+    table.decimal('minExchangeRateDenominator', 64, 0).alter()
+    table.decimal('lowEstimatedExchangeRateNumerator', 64, 0).alter()
+    table.decimal('lowEstimatedExchangeRateDenominator', 64, 0).alter()
+    table.decimal('highEstimatedExchangeRateNumerator', 64, 0).alter()
+    table.decimal('highEstimatedExchangeRateDenominator', 64, 0).alter()


needed something bigger than 20, (I don't think we will ever need more than 20 ever), but just being generous so we don't run into this issue again

Seems reasonable. Inspecting the db shows these are stored as numeric types (same as decimal) in postgres. Found this in the docs about how much storage this would take:

Numeric values are physically stored without any extra leading or trailing zeroes. Thus, the declared precision and scale of a column are maximums, not fixed allocations. (In this sense the numeric type is more akin to varchar(n) than to char(n).) The actual storage requirement is two bytes for each group of four decimal digits, plus three to eight bytes overhead.

https://www.postgresql.org/docs/current/datatype-numeric.html

So it varies on actual size of the data, not just this declaration. So I don't really see a downside of leaving lots of extra headroom.

Were you able to test this migration against any existing data? Since we're going from lower to higher precision I imagine it probably works fine but figure it's worth testing.

@BlairCurrey I just made some transactions on main then rebuilt the backend and continued making transactions without issue

mkurapov · 2024-05-27T22:28:54Z

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

-  if (!maxReceiveAmountValue) {
-    // FixedDelivery
-    const fees = quote.fee?.calculate(debitAmountValue) ?? 0n
-    debitAmountValue = BigInt(debitAmountValue) + fees
-    if (
-      debitAmountValue < quote.debitAmount.value ||
-      debitAmountValue <= BigInt(0)
-    ) {
-      throw QuoteError.InvalidAmount
-    }
-  } else {


split these up into functions, logic remains the same other than the comment below

mkurapov · 2024-05-27T22:29:48Z

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

@@ -681,41 +697,5 @@ describe('IlpPaymentService', (): void => {
        expect((error as PaymentMethodHandlerError).retryable).toBe(false)
      }
    })
-
-    test('throws if invalid quote data', async (): Promise<void> => {


we don't use additionalFields so parsing JSON is not required, so this test is not necessary

mkurapov · 2024-05-28T08:03:54Z

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

@@ -272,6 +284,7 @@ describe('IlpPaymentService', (): void => {
        minDeliveryAmount: -1n
      } as Pay.Quote)

+      expect.assertions(4)


to make sure exceptions in catch are tracked

chore(backend): update how we store exchange rate numerator and denom…

d026727

…inator fields in the quotes table

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels May 27, 2024

mkurapov commented May 27, 2024

View reviewed changes

mkurapov added 2 commits May 28, 2024 00:26

chore(backend): make fields numeric

e1a7b85

chore(backend): update error descriptions

60da00e

mkurapov commented May 27, 2024

View reviewed changes

mkurapov changed the title ~~chore(backend): update how we store exchange rate numerator and denom…~~ chore(backend): allow asset scale 2 to asset scale 9 payment May 27, 2024

mkurapov changed the title ~~chore(backend): allow asset scale 2 to asset scale 9 payment~~ chore(backend): allow asset scale 2 to asset scale 9 payments May 27, 2024

mkurapov commented May 28, 2024

View reviewed changes

mkurapov marked this pull request as ready for review May 28, 2024 08:04

mkurapov requested review from BlairCurrey and raducristianpopa May 28, 2024 08:04

mkurapov added 4 commits May 29, 2024 12:34

chore(backend): rename migration name

d9c0d66

chore(backend): add test testing for large amounts

2d1bff6

Merge branch 'main' into 2747/mk/fix-quotes-table

7d2d45e

chore: update logger message

5242127

BlairCurrey approved these changes May 29, 2024

View reviewed changes

mkurapov merged commit ddffe93 into main May 29, 2024
42 checks passed

mkurapov deleted the 2747/mk/fix-quotes-table branch May 29, 2024 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(backend): allow asset scale 2 to asset scale 9 payments #2748

chore(backend): allow asset scale 2 to asset scale 9 payments #2748

mkurapov commented May 27, 2024 •

edited

Loading

netlify bot commented May 27, 2024 •

edited

Loading

mkurapov May 27, 2024 •

edited

Loading

mkurapov May 27, 2024

mkurapov May 27, 2024

mkurapov May 27, 2024 •

edited

Loading

mkurapov May 27, 2024 •

edited

Loading

mkurapov May 27, 2024

BlairCurrey May 28, 2024

BlairCurrey May 28, 2024 •

edited

Loading

mkurapov May 29, 2024

mkurapov May 27, 2024

mkurapov May 27, 2024

mkurapov May 28, 2024

chore(backend): allow asset scale 2 to asset scale 9 payments #2748

chore(backend): allow asset scale 2 to asset scale 9 payments #2748

Conversation

mkurapov commented May 27, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented May 27, 2024 • edited Loading

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

mkurapov May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov May 27, 2024 • edited Loading

Choose a reason for hiding this comment

mkurapov May 27, 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 May 28, 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

Choose a reason for hiding this comment

mkurapov commented May 27, 2024 •

edited

Loading

netlify bot commented May 27, 2024 •

edited

Loading

mkurapov May 27, 2024 •

edited

Loading

mkurapov May 27, 2024 •

edited

Loading

mkurapov May 27, 2024 •

edited

Loading

BlairCurrey May 28, 2024 •

edited

Loading