Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(backend): add local payment #2857

Open
wants to merge 78 commits into
base: main
Choose a base branch
from

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Aug 10, 2024

Changes proposed in this pull request

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

Context

fixes #2834
fixes #2854
fixes #2855

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

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

netlify bot commented Aug 10, 2024

Deploy Preview for brilliant-pasca-3e80ec ready!

Name Link
🔨 Latest commit 3149428
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/672e4f7b20d5190007c35505
😎 Deploy Preview https://deploy-preview-2857--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 105 to 126
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()
})
})
Copy link
Contributor Author

@BlairCurrey BlairCurrey Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@BlairCurrey BlairCurrey Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@BlairCurrey BlairCurrey Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BlairCurrey BlairCurrey changed the title feat(backend): add local payment quote migration feat(backend): add local payment Aug 10, 2024
- 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
@github-actions github-actions bot added type: tests Testing related type: source Changes business logic labels Aug 13, 2024
packages/backend/src/payment-method/local/service.ts Outdated Show resolved Hide resolved
Comment on lines 105 to 126
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()
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
(payment.quote.estimatedExchangeRate ||
payment.quote.lowEstimatedExchangeRate.valueOf())
)
Math.ceil(Number(alreadySentAmount) * payment.quote.estimatedExchangeRate)
Copy link
Contributor Author

@BlairCurrey BlairCurrey Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some initial comments, will be back to this!

Comment on lines 6 to 23
exports.up = function (knex) {
return knex('quotes')
.whereNull('estimatedExchangeRate')
.update({
// TODO: vet this more... looks like the low* fields were (inadvertently?)
// made nullable when updating from bigint to decimal. If they are null
// anywhere then this wont work.
estimatedExchangeRate: knex.raw('?? / ??', [
'lowEstimatedExchangeRateNumerator',
'lowEstimatedExchangeRateDenominator'
])
})
.then(() => {
return knex.schema.table('quotes', (table) => {
table.decimal('estimatedExchangeRate', 20, 10).notNullable().alter()
})
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlairCurrey ah yes, it looks like making them decimal didn't keep the null constraint. Good to know for the future. It should always be set in the context of the ILP quote

"highEstimatedExchangeRateDenominator"
)
SELECT
uuid_generate_v4(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use gen_random_uuid here instead? Then we don't need the CREATE EXTENSION...

Comment on lines 13 to 24
export async function createIlpQuoteDetailsService(
deps_: ServiceDependencies
): Promise<IlpQuoteDetailsService> {
const deps = {
...deps_,
logger: deps_.logger.child({ service: 'IlpQuoteDetailsService' })
}
return {
getByQuoteId: (quoteId: string) =>
getIlpQuoteDetailsByQuoteId(deps, quoteId)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative, we can also use the Objection model IlpQuoteDetails.query() directly in the IlpPaymentService.pay method without needing a separate service

Comment on lines 145 to 155
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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we insert ilpQuoteDetails directly in the IlpPaymentService.getQuote method? Then, this service doesn't need to know any details related to ILP (or any other payment method for that matter)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Still applies I believe

Copy link
Contributor Author

@BlairCurrey BlairCurrey Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this part. Yeah I think that's right... I was motivated to put it here due to it's relation in the db to quotes but as detailed when retrieving it in pay, its really more of a ilp payment method concern. Guess it makes sense to put this part in the getQuote method and remove from the quote model as you suggested here: #2857 (comment)

edit: actually, the quote does not exist yet when we'd want to create the ilpQuoteDetails... guess we would need to drop the relationship in order to move the insertion into getQuote. not sure that's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to fully decouple the quote ilp payment method. That is, not dealing with the ilpQuoteDetails nor handle any logic based on ILP vs. local quotes in the quote service. The ilp specific stuff (ilpQuoteDetails) will be handled in the payment method service. To achieve this we are dropping the FK relationship which allows us to create the ilpQuoteDetails in the payment method's getQuote.

public feeId?: string
public fee?: Fee

public ilpQuoteDetails?: IlpQuoteDetails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment in the service, we probably could get away without this if we create ilpQuoteDetails in IlpPaymentService

Comment on lines 254 to 257
// TODO: no timeout? theoretically possible (perhaps even correct?) but need to
// update IncomingPayment state to COMPLETE some other way. trxOrError.post usually
// will (via onCredit) but post will return error instead because its already posted.
timeout: deps.config.tigerBeetleTwoPhaseTimeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes I remember we aren't actually handling single-phase transfers in the accountingService.createTransfer method. This is the relevant issue I made a while back:

In the meantime, for sure makes sense to keep the timeout + transfer.post

Comment on lines 223 to 246
// TODO: remove incoming state check? perhaps only applies ilp account middleware where its checking many different things
if (incomingPayment.state === IncomingPaymentState.Pending) {
try {
await deps.accountingService.createLiquidityAccount(
incomingPayment,
LiquidityAccountType.INCOMING
)
} catch (err) {
if (!(err instanceof AccountAlreadyExistsError)) {
deps.logger.error(
{ incomingPayment, err },
'Failed to create liquidity account for local incoming payment'
)
throw new PaymentMethodHandlerError(
'Received error during local payment',
{
description:
'Unknown error while trying to create liquidity account',
retryable: false
}
)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think worthwhile to keep this sanity check. We can throw an error if payment is not Pending

)
}
}
await trxOrError.post()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can throw if this returns a transfer error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed this... added.

@@ -229,10 +239,10 @@ function calculateFixedSendQuoteAmounts(
quote: Quote,
maxReceiveAmountValue: bigint
): CalculateQuoteAmountsWithFeesResult {
// TODO: derive fee from debitAmount and convert that to receiveAmount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think there is some discrepancy here, or the current behaviour has some incorrect assumptions. Going to think it over some more... this is a tricky one

@github-actions github-actions bot added the pkg: auth Changes in the GNAP auth package. label Oct 23, 2024
@github-actions github-actions bot removed the pkg: auth Changes in the GNAP auth package. label Oct 29, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 145 to 155
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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Still applies I believe

)
}
}
await trxOrError.post()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good point, made it required again

BlairCurrey and others added 3 commits October 31, 2024 11:19
…r-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru

Co-authored-by: Max Kurapov <[email protected]>
Making optional depends on single phase transfer
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


exports.up = function (knex) {
return knex('quotes')
.whereNull('estimatedExchangeRate')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which cases the estimatedExchangeRate was null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this return the same result?

Copy link
Contributor Author

@BlairCurrey BlairCurrey Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


export interface StartQuoteOptions {
quoteId?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be required?

Copy link
Contributor Author

@BlairCurrey BlairCurrey Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, thank you... outdated comment

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

@mkurapov mkurapov Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@BlairCurrey BlairCurrey Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
      },

Copy link
Contributor

@mkurapov mkurapov Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
3 participants