-
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
Open
BlairCurrey
wants to merge
77
commits into
main
Choose a base branch
from
bc/2834/non-ilp-local-payments
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 9 commits
Commits
Show all changes
77 commits
Select commit
Hold shift + click to select a range
bb77152
feat(backend): add local payment quote migration
BlairCurrey fc06e24
feat(backend): WIP seperate ILPModels, LocalQuote, BaseQuote models
BlairCurrey 257fd01
refactor(backend): change model/services to reflect optional ilp quot…
BlairCurrey d9fa20e
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 2e76ee2
feat(backend): WIP local payment method with getQuote
BlairCurrey 4fab5d8
feat(backend): add local payment method to payment method handler
BlairCurrey b17db75
chore(backend): fix format
BlairCurrey 6e37420
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 9ef7f94
feat(backend): stub in control payment handler service with receiver …
BlairCurrey 320ee21
feat(backend): local payment .pay
BlairCurrey 07569a7
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey f3d5f5d
chore: rm comment
BlairCurrey d795ce2
chore: WIP debugging wrong sentAmount
BlairCurrey 251c60a
chore: rm comment
BlairCurrey 8f0bcb8
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey eca01f1
feat(backend): use receiver.isLocal to control payment method in quot…
BlairCurrey 770d7ea
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 3849db7
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 170bcc3
fix(backend): added source amount
BlairCurrey 88618ce
fix(backend): p2p case (cross currency, local, fixed send)
BlairCurrey 823c512
fix: lint error
BlairCurrey 2e9f043
chore: rm logs
BlairCurrey 3fb02e5
fix: quote service test
BlairCurrey 569f029
fix: lint errors
BlairCurrey dae1de1
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 2016469
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 3182944
refactor(backend): split migrations
BlairCurrey 3665188
refactor(backend): rm migration that was split into many
BlairCurrey 5b8bad3
WIP bruno requests for testing
BlairCurrey b49e08e
feat(backend): start rm ilpQuoteDetail join on op where not used
BlairCurrey eedacb5
fix(backend): rm unecessary ilpQuoteDetail join
BlairCurrey 27a64b2
chore(backend): format
BlairCurrey 26d0539
fix(backend): dont join op on quote.ilpQuoteDetails on get
BlairCurrey 2723961
fix(backend): rm ilpQuoteDetails join on op cancel
BlairCurrey 08126c5
fix(backend): rm unecessary join in op validate grant amount
BlairCurrey 205447d
fix(backend): rm join from fundPayment
BlairCurrey 0471d84
fix(backend): rm unecessary join, unused method
BlairCurrey 7b5142d
fix(backend): fetch ilpQuoteDetails where used instead of joining
BlairCurrey 773e4f2
chore(backend): move ilpquotedetails dir
BlairCurrey 0a3adf4
chore(backend): rm console.log
BlairCurrey 347f9b0
fix(backend): rm ilpQuoteDetails joins from quote service
BlairCurrey 37d399e
chore(backend): rm console.log
BlairCurrey 78d42b6
refactor(backend): rename sourceAmount to debitAmountMinusFees
BlairCurrey a96d654
chore(backend): cleanup, rm unused fee method
BlairCurrey 7941f05
test(backend): add local payment tests
BlairCurrey 753a57b
chore: format
BlairCurrey ed30066
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 8476615
fix(bruno): local open payments requests
BlairCurrey f8a979c
test(backend): add integration tests for local payments
BlairCurrey 5cee618
chore(backend): cleanup
BlairCurrey d41777e
chore: restore old version of date definition in test
BlairCurrey e0f988d
chore: cleanup
BlairCurrey 5d631bd
fix: rm unused import
BlairCurrey ececad2
test(integration): new case - p2p, fixed-send, local
BlairCurrey 63011ae
chore(integration): rename test for consistency
BlairCurrey bd12e30
fix(backend): throw error in pay if incoming payment is not pending
BlairCurrey b0eaf8d
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 0d69a3d
feat(backend): simplify migrations
BlairCurrey 31e6cc7
chore(backend): clarify comment
BlairCurrey bb9d334
chore(auth): format
BlairCurrey b0897d9
refactor(backend): use IlpQuoteDetails model directly in ilp payment …
BlairCurrey 2d8048a
refactor(backend): rm ilpQuoteDetails service
BlairCurrey 15d8929
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey c40db9e
fix(integration): wa typo
BlairCurrey 0dbb2d3
chore: rm bruno test examples
BlairCurrey 61b1010
refactor: mv debitAmountMinusFees to fee calc and clarify TODO
BlairCurrey 0dea97d
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey 6c995d9
Update bruno/collections/Rafiki/Examples/Admin API - only locally/Pe…
BlairCurrey 86f6db6
fix: make timeout required again
BlairCurrey 3fc6924
feat: error when post fails in local pay
BlairCurrey 6634b8f
refactor(backend): add optional quoteId to getQuote args
BlairCurrey 6706557
refactor: rm ilp quote details out of quote service
BlairCurrey 998ee25
refactor: insert ilp quote details in ilp getQuote
BlairCurrey a4cf57a
fix(backend): payment handler test
BlairCurrey 1835bc4
chore(bruno): rename request
BlairCurrey 7db833b
chore(integration): rm erroneous todo comment
BlairCurrey 4437438
Merge branch 'main' into bc/2834/non-ilp-local-payments
BlairCurrey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
147 changes: 147 additions & 0 deletions
147
packages/backend/migrations/20240809171654_add_ilp_quote_details_table.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
/** | ||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.up = function (knex) { | ||
return ( | ||
knex.schema | ||
// Create new table with columns from "quotes" to migrate | ||
.createTable('ilpQuoteDetails', function (table) { | ||
table.uuid('id').notNullable().primary() | ||
table.uuid('quoteId').notNullable().unique() | ||
table.foreign('quoteId').references('quotes.id') | ||
|
||
table.bigInteger('maxPacketAmount').notNullable() | ||
table.decimal('minExchangeRateNumerator', 64, 0).notNullable() | ||
table.decimal('minExchangeRateDenominator', 64, 0).notNullable() | ||
table.decimal('lowEstimatedExchangeRateNumerator', 64, 0).notNullable() | ||
table | ||
.decimal('lowEstimatedExchangeRateDenominator', 64, 0) | ||
.notNullable() | ||
table.decimal('highEstimatedExchangeRateNumerator', 64, 0).notNullable() | ||
table | ||
.decimal('highEstimatedExchangeRateDenominator', 64, 0) | ||
.notNullable() | ||
|
||
table.timestamp('createdAt').defaultTo(knex.fn.now()) | ||
table.timestamp('updatedAt').defaultTo(knex.fn.now()) | ||
}) | ||
.then(() => { | ||
// Enable uuid_generate_v4 | ||
return knex.raw(`CREATE EXTENSION IF NOT EXISTS "uuid-ossp";`) | ||
}) | ||
.then(() => { | ||
// Migrate data from quotes to ilpQuoteDetails. | ||
return knex.raw(` | ||
INSERT INTO "ilpQuoteDetails" ( | ||
id, | ||
"quoteId", | ||
"maxPacketAmount", | ||
"minExchangeRateNumerator", | ||
"minExchangeRateDenominator", | ||
"lowEstimatedExchangeRateNumerator", | ||
"lowEstimatedExchangeRateDenominator", | ||
"highEstimatedExchangeRateNumerator", | ||
"highEstimatedExchangeRateDenominator" | ||
) | ||
SELECT | ||
uuid_generate_v4(), | ||
id AS "quoteId", | ||
"maxPacketAmount", | ||
"minExchangeRateNumerator", | ||
"minExchangeRateDenominator", | ||
"lowEstimatedExchangeRateNumerator", | ||
"lowEstimatedExchangeRateDenominator", | ||
"highEstimatedExchangeRateNumerator", | ||
"highEstimatedExchangeRateDenominator" | ||
FROM "quotes"; | ||
`) | ||
}) | ||
.then(() => { | ||
// TODO: test this more thoroughly. | ||
// Might need to seed in migration preceeding this? | ||
// Cant simply withold htis migration. Application code will fail when trying | ||
// to insert ... | ||
return knex('quotes') | ||
.whereNull('estimatedExchangeRate') | ||
.update({ | ||
estimatedExchangeRate: knex.raw('?? / ??', [ | ||
'lowEstimatedExchangeRateNumerator', | ||
'lowEstimatedExchangeRateDenominator' | ||
]) | ||
}) | ||
}) | ||
.then(() => { | ||
return knex.schema.alterTable('quotes', function (table) { | ||
table.dropColumn('maxPacketAmount') | ||
table.dropColumn('minExchangeRateNumerator') | ||
table.dropColumn('minExchangeRateDenominator') | ||
table.dropColumn('lowEstimatedExchangeRateNumerator') | ||
table.dropColumn('lowEstimatedExchangeRateDenominator') | ||
table.dropColumn('highEstimatedExchangeRateNumerator') | ||
table.dropColumn('highEstimatedExchangeRateDenominator') | ||
}) | ||
}) | ||
) | ||
} | ||
|
||
/** | ||
* @param { import("knex").Knex } knex | ||
* @returns { Promise<void> } | ||
*/ | ||
exports.down = function (knex) { | ||
return knex.schema | ||
.alterTable('quotes', function (table) { | ||
// restore columns without not null constraint | ||
table.bigInteger('maxPacketAmount') | ||
table.decimal('minExchangeRateNumerator', 64, 0) | ||
table.decimal('minExchangeRateDenominator', 64, 0) | ||
table.decimal('lowEstimatedExchangeRateNumerator', 64, 0) | ||
table.decimal('lowEstimatedExchangeRateDenominator', 64, 0) | ||
table.decimal('highEstimatedExchangeRateNumerator', 64, 0) | ||
table.decimal('highEstimatedExchangeRateDenominator', 64, 0) | ||
}) | ||
.then(() => { | ||
// Migrate data back to quotes table from ilpQuote | ||
return knex.raw(` | ||
UPDATE "quotes" | ||
SET | ||
"maxPacketAmount" = "ilpQuoteDetails"."maxPacketAmount", | ||
"minExchangeRateNumerator" = "ilpQuoteDetails"."minExchangeRateNumerator", | ||
"minExchangeRateDenominator" = "ilpQuoteDetails"."minExchangeRateDenominator", | ||
"lowEstimatedExchangeRateNumerator" = "ilpQuoteDetails"."lowEstimatedExchangeRateNumerator", | ||
"lowEstimatedExchangeRateDenominator" = "ilpQuoteDetails"."lowEstimatedExchangeRateDenominator", | ||
"highEstimatedExchangeRateNumerator" = "ilpQuoteDetails"."highEstimatedExchangeRateNumerator", | ||
"highEstimatedExchangeRateDenominator" = "ilpQuoteDetails"."highEstimatedExchangeRateDenominator" | ||
FROM "ilpQuoteDetails" | ||
WHERE "quotes"."id" = "ilpQuoteDetails"."quoteId" | ||
`) | ||
}) | ||
.then(() => { | ||
// Apply the not null constraints after data insertion | ||
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() | ||
}) | ||
}) | ||
.then(() => { | ||
return knex.schema.dropTableIfExists('ilpQuoteDetails') | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,11 +120,7 @@ function getAdjustedAmounts( | |
// This is only an approximation of the true amount delivered due to exchange rate variance. Due to connection failures there isn't a reliable way to track that in sync with the amount sent (particularly within ILP payments) | ||
// eslint-disable-next-line no-case-declarations | ||
const amountDelivered = BigInt( | ||
Math.ceil( | ||
Number(alreadySentAmount) * | ||
(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 commentThe reason will be displayed to describe this comment to others. Learn more. wont have the low |
||
) | ||
let maxReceiveAmount = payment.receiveAmount.value - amountDelivered | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
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:
estimatedExchangeRate
, and mark the field as requiredilpQuoteDetails
ilpQuoteDetails
Then we deploy the code changes to start reading from
ilpQuoteDetails
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.
yeah I suppose multiple migrations make sense. More control over up/down if needed without downside.
In terms of this part:
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:
released the same time as we backfill
ilpQuoteDetails
(anddrop 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.