refactor(backend): move fee calculation to backend #1755

BlairCurrey · 2023-08-21T15:54:53Z

Changes proposed in this pull request

updates to latest open-payments package and openapi schema
renames sendAmount to debitAmount across backend
moves fee calculation to backend on quote create
fixes fee calculation for cross-currency (different send/receive assets)

Context

Fixes: #1639 and Fixes: #1640

There are lots of file changes, but it's mostly renaming. More interesting code changes are the packages/backend/src/open_payments/quote/service.ts ones.

Checklist

netlify · 2023-08-21T15:54:59Z

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

Name	Link
🔨 Latest commit	`b9b22cc`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6503340c7245570008c3588f

BlairCurrey · 2023-09-08T14:22:24Z

packages/auth/migrations/20230907201006_rename-limits-jsonb-keys.js

+  // renames sendAmount keys (if any) to debitAmount in limits jsonb column
+  return knex('accesses')
+    .update({
+      limits: knex.raw(
+        "limits - 'sendAmount' || jsonb_build_object('debitAmount', limits->'sendAmount')"
+      )
+    })
+    .whereRaw("limits \\? 'sendAmount'")
+}


This is hard to read but I could not find a simpler way. Addresses #1755 (comment)

To test I initialized rafiki with this extra test migration (before 20230907201006_rename-limits-jsonb-keys.js) which seeds some dummy data: https://gist.github.com/BlairCurrey/094b5df41f36f94befacbf727c3535ef

Then I verified the accesses records matched expectations after rebuilding rafiki from migrations.

sabineschaller

I have to run but here is a first part of a review.

sabineschaller · 2023-09-11T12:42:51Z

packages/auth/migrations/20230907201006_rename-limits-jsonb-keys.js

Is that to be backwards compatible? Do we think that is necessary?

Yes, to ensure our renaming efforts dont cause any errors on pre-existing data. There is some discussion in the main body of the PR. My initial reasoning is here: #1755 (comment)

I think it probably is necessary because any existing records will have the old name and the application code has been changed to expect the new one. So it might break something on testnet if we dont do this.

Makes sense. I wasn't aware of the discussion and didn't think about testnet. My bad.

No worries... I fee like higher level discussion on PR's on gh is kind of a mess.

openapi/schemas.yaml

sabineschaller · 2023-09-11T12:44:18Z

packages/backend/migrations/20230904145439_rename-quote-sendamountvalue.js

Same question as above

For this one I guess we could rename it in the model with a get and set but I think this is simpler.

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

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

sabineschaller · 2023-09-11T13:00:06Z

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

+      debitAmountValue < quote.debitAmount.value ||
+      debitAmountValue <= BigInt(0)


How would that happen?

This check already existed and I considered if it was still necessary since we aren't getting the value from the ASE but I wasn't sure, so I left it. I looked at it again though and think it can be removed.

debitAmountValue < quote.debitAmount.value

would be true if fees are negative (aside from fees, debitAmountValue === quote.debitAmount.value). not possible since we're checking that on fee create

debitAmountValue <= BigInt(0)

true if quote.debitAmount.value + fees is negative. and if fees cant be negative the the quote.debitAmount.value would have to be, and it cant because we're checking that before returning the ilp quote from startQuote

I didn't realize it was just a refactor. I also looked more closely now and I think you are right. This can probably be removed since it checks the quote after it is returned from the ASE in the current flow.

I could remove this one OK but I tried the same for fixed send and an existing test failed (which was explicitly looking for this case). Perhaps the inputs shouldn't cause these errors but I think it's probably safer to leave them just in case.

sabineschaller · 2023-09-12T08:45:37Z

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

+      debitAmountValue < quote.debitAmount.value ||
+      debitAmountValue <= BigInt(0)


I didn't realize it was just a refactor. I also looked more closely now and I think you are right. This can probably be removed since it checks the quote after it is returned from the ASE in the current flow.

sabineschaller · 2023-09-12T08:49:35Z

packages/backend/src/openapi/webhooks.yaml

@@ -160,7 +160,7 @@ components:
            - createdAt
            - updatedAt
            - expiresAt
-            - sendAmount
+            - debitAmount


It seems like we have some validation error here. Check in "files changed" if you can't see it here.

@sabineschaller Hmm, I see it. More details from the CI job show the warning:

OpenAPI "servers" must be present and non-empty array

https://github.com/interledger/rafiki/actions/runs/6174411421/job/16758949762?pr=1755#step:6:12

Looks unrelated to this change and we've been getting it for a while:

2 months ago: f5c1f66#diff-90da7214f1ef2d21dca042e424b4079b684ab3017fb6c278e4abab4453855f24

3 months ago: 22ef1dc#diff-90da7214f1ef2d21dca042e424b4079b684ab3017fb6c278e4abab4453855f24R1

Seems like it needs the servers attribute defined like we do in some of our other specs. Not sure what would be appropriate. What about something like this (similar to exchange rate yaml)?

servers: - url: 'https://account-servicing-entity.com/webhooks'

Found the webhook url in some config files and concluded that url is right. I added it which seems to have fixed the warning.

sabineschaller · 2023-09-12T08:51:58Z

packages/backend/src/config/app.ts

@@ -88,8 +88,6 @@ export const Config = {
  incomingPaymentWorkers: envInt('INCOMING_PAYMENT_WORKERS', 1),
  incomingPaymentWorkerIdle: envInt('INCOMING_PAYMENT_WORKER_IDLE', 200), // milliseconds

-  quoteUrl: envString('QUOTE_URL', 'http://127.0.0.1:4001/quote'),


Can you remove that from the integration docs, please?

Looks like there were a few places to remove that. I also updated the explanation for setting fees in the integration doc e6f6f34, de3a256

sabineschaller

LGTM

mkurapov

I would also recommend running pnpm generate:types in the token-introspection package to update the types properly.

Also, I'm not sure if we discussed, but for the documentation API schema, we should be seeing sendAmount be changed to debitAmount as well, correct?

mkurapov · 2023-09-14T13:07:13Z

localenv/mock-account-servicing-entity/app/routes/quotes.ts

@@ -19,7 +19,7 @@ export type Quote = {
  paymentType: PaymentType


~~We could remove this file altogether now, right?~~
will be removed in Nathan's PR

mkurapov · 2023-09-14T13:16:01Z

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

-  Accept: string
-  'Content-Type': string
-  'Rafiki-Signature'?: string
+function getFees(fee: Fee | undefined, principal: bigint): bigint {


Maybe this method can live in feeService? just a suggestion

I considered that as well. I think that's probably a good idea.

I added the method to the model since we already had the fee model here. It felt wrong to pass the fee into the fee service like feeService.calculate(fee, principal) or to re-lookup the fee. Later if we just have the feeId or assetId we can do feeService.calculate(id, principal) which gets the fee then calls the calculate method.

mkurapov

🚀

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

Co-authored-by: Max Kurapov <max@interledger.org>

BlairCurrey added 6 commits August 15, 2023 09:44

feat(backend): WIP - add feeid to quote table

4f458ba

refactor(backend): rename quote gql field

7dc1d60

Merge branch 'main' into bc/1639/add-fees-to-quote

e39bb88

Merge branch 'main' into bc/1639/add-fees-to-quote

35ed878

fix(backend): add fee relation

752df9b

feat(backend): WIP calculate fees in quote create

088058b

BlairCurrey added 15 commits August 23, 2023 09:02

refactor(backend): rename gql quote field

5cce896

fix(backend): rm unused with graph fetched

7bc19df

Merge branch 'main' into bc/1639/add-fees-to-quote

8383ed5

Merge branch 'main' into bc/1639/add-fees-to-quote

a0b2c30

feat(backend): move fee calculation to quote service

874bcf6

fix(backend): use quote error

d58532c

Merge branch 'main' into bc/1639/add-fees-to-quote

4026c23

Merge branch 'main' into bc/1639/add-fees-to-quote

d4e0bdf

fix(backend): rm unused fn

9635035

refactor(backend): rename quote send amount value

1fd63fd

refactor(backend): rename quote send amount to debit amount

dc88ba8

refactor(backend): rename out payment send amount to debit

e393681

chore(backend): format

9a09017

chore(documentation): update gql docs

393198b

fix(backend,localenv): trx amount

c5ea21d

BlairCurrey changed the title ~~Bc/1639/add fees to quote~~ refactor(backend): move fee calculation to backend Sep 4, 2023

BlairCurrey commented Sep 8, 2023

View reviewed changes

BlairCurrey added 2 commits September 8, 2023 10:51

Merge branch 'main' into bc/1639/add-fees-to-quote

fc6d045

chore(auth): format

316e7f7

sabineschaller reviewed Sep 11, 2023

View reviewed changes

BlairCurrey added 2 commits September 11, 2023 11:34

fix(backend): remove asset join

7e2df45

refactor(backend): remove variable assignment

6a5bf5b

sabineschaller reviewed Sep 12, 2023

View reviewed changes

BlairCurrey mentioned this pull request Sep 13, 2023

feat(mock-ase): call setFee during seed script #1861

Merged

5 tasks

BlairCurrey added 5 commits September 13, 2023 09:50

feature(backend): change error

310fce3

Merge branch 'main' into bc/1639/add-fees-to-quote

937e495

reactor(documentation): update fee explanation

e6f6f34

chore: remove quote url references

de3a256

fix(openapi): missing server warning

633ed35

sabineschaller previously approved these changes Sep 14, 2023

View reviewed changes

mkurapov reviewed Sep 14, 2023

View reviewed changes

BlairCurrey added 2 commits September 14, 2023 10:13

chore(token-introspection): generate types

05155a8

refactor(documentation): rename sendAmount to debitAmount

db49061

BlairCurrey dismissed sabineschaller’s stale review via db49061 September 14, 2023 14:29

mkurapov previously approved these changes Sep 14, 2023

View reviewed changes

refactor(backend): move fee calc to model

c639a76

BlairCurrey dismissed mkurapov’s stale review via c639a76 September 14, 2023 16:10

chore(backend): format

c4fa113

mkurapov reviewed Sep 14, 2023

View reviewed changes

packages/backend/src/open_payments/quote/service.ts Outdated Show resolved Hide resolved

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

b9b22cc

Co-authored-by: Max Kurapov <max@interledger.org>

BlairCurrey requested a review from mkurapov September 14, 2023 16:42

mkurapov approved these changes Sep 14, 2023

View reviewed changes

BlairCurrey merged commit c29c1a7 into main Sep 14, 2023

BlairCurrey deleted the bc/1639/add-fees-to-quote branch September 14, 2023 17:44

sabineschaller added the breaking Issue/PR that introduces breaking changes label Sep 19, 2023

BlairCurrey mentioned this pull request Sep 22, 2023

feat(backend): rename "payment pointer" tables & columns to use "wallet address" #1937

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(backend): move fee calculation to backend #1755

refactor(backend): move fee calculation to backend #1755

BlairCurrey commented Aug 21, 2023 •

edited

Loading

netlify bot commented Aug 21, 2023 •

edited

Loading

BlairCurrey Sep 8, 2023 •

edited

Loading

sabineschaller left a comment

sabineschaller Sep 11, 2023

BlairCurrey Sep 11, 2023 •

edited

Loading

sabineschaller Sep 12, 2023

BlairCurrey Sep 12, 2023

sabineschaller Sep 11, 2023

BlairCurrey Sep 11, 2023

sabineschaller Sep 11, 2023

BlairCurrey Sep 11, 2023

sabineschaller Sep 12, 2023

BlairCurrey Sep 13, 2023

sabineschaller Sep 12, 2023

sabineschaller Sep 12, 2023

BlairCurrey Sep 13, 2023 •

edited

Loading

BlairCurrey Sep 13, 2023

sabineschaller Sep 12, 2023

BlairCurrey Sep 13, 2023 •

edited

Loading

sabineschaller left a comment

mkurapov left a comment

mkurapov Sep 14, 2023 •

edited

Loading

mkurapov Sep 14, 2023

BlairCurrey Sep 14, 2023

BlairCurrey Sep 14, 2023

mkurapov left a comment

		debitAmountValue < quote.debitAmount.value \|\|
		debitAmountValue <= BigInt(0)

		@@ -19,7 +19,7 @@ export type Quote = {
		paymentType: PaymentType

refactor(backend): move fee calculation to backend #1755

refactor(backend): move fee calculation to backend #1755

Conversation

BlairCurrey commented Aug 21, 2023 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Aug 21, 2023 • edited Loading

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

BlairCurrey Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

sabineschaller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Sep 11, 2023 • 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

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

Choose a reason for hiding this comment

BlairCurrey Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

sabineschaller left a comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

mkurapov Sep 14, 2023 • 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 left a comment

Choose a reason for hiding this comment

BlairCurrey commented Aug 21, 2023 •

edited

Loading

netlify bot commented Aug 21, 2023 •

edited

Loading

BlairCurrey Sep 8, 2023 •

edited

Loading

BlairCurrey Sep 11, 2023 •

edited

Loading

BlairCurrey Sep 13, 2023 •

edited

Loading

BlairCurrey Sep 13, 2023 •

edited

Loading

mkurapov Sep 14, 2023 •

edited

Loading