-
Notifications
You must be signed in to change notification settings - Fork 89
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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() | ||
}) | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
- 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
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() | ||
}) | ||
}) |
There was a problem hiding this comment.
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
(payment.quote.estimatedExchangeRate || | ||
payment.quote.lowEstimatedExchangeRate.valueOf()) | ||
) | ||
Math.ceil(Number(alreadySentAmount) * payment.quote.estimatedExchangeRate) |
There was a problem hiding this comment.
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.
- for new local payment method service, sentAmount matches debitAmount, whereas is matches receive amount for local/remote ilp payments
…e/outgoing payment
There was a problem hiding this 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!
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() | ||
}) | ||
}) | ||
} |
There was a problem hiding this comment.
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(), |
There was a problem hiding this comment.
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...
export async function createIlpQuoteDetailsService( | ||
deps_: ServiceDependencies | ||
): Promise<IlpQuoteDetailsService> { | ||
const deps = { | ||
...deps_, | ||
logger: deps_.logger.child({ service: 'IlpQuoteDetailsService' }) | ||
} | ||
return { | ||
getByQuoteId: (quoteId: string) => | ||
getIlpQuoteDetailsByQuoteId(deps, quoteId) | ||
} | ||
} |
There was a problem hiding this comment.
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
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 | ||
} | ||
} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Still applies I believe
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
// 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 |
There was a problem hiding this comment.
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
// 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 | ||
} | ||
) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this 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
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 | ||
} | ||
} |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
...API - only locally/Peer-to-Peer Local Payment/Create Receiver -remote Incoming Payment-.bru
Outdated
Show resolved
Hide resolved
…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
There was a problem hiding this 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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- add runtime check to ilp payment method implementation requireing it
@@ -0,0 +1,73 @@ | |||
meta { | |||
name: Create Receiver -local Incoming Payment- |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be required?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
test/integration/integration.test.ts
Outdated
} | ||
value: BigInt(quote.receiveAmount.value) | ||
}) | ||
// TODO: fix bug where fixed-send (regardless of local/remote) is not completeing |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"
},
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Changes proposed in this pull request
Context
fixes #2834
fixes #2854
fixes #2855
Checklist
fixes #number