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.

Sign up for GitHub

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): rename "payment pointer" tables & columns to use "wallet address" #1937

Merged
merged 8 commits into from
Oct 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
09c3331
feat(backend): rename payment pointer tables & payment columns to use…
Sep 19, 2023
4749733
feat: rename column in walletAddressKeys
Sep 19, 2023
83f3c42
feat: remove .then in table column rename
Sep 20, 2023
d8ea382
chore: formatting
njlie Sep 20, 2023
d1cca1c
feat: rename paymentPointer keys in webhook event json column
njlie Oct 2, 2023
dba4176
feat: rename more data fields, type values
njlie Oct 3, 2023
bc761b0
feat: rename/drop indices & constraints
njlie Oct 11, 2023
b7fc854
feat: update rollbacks
njlie Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions packages/backend/migrations/20230918113102_rename_payment_pointer_tables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.up = function (knex) {
return Promise.all([
knex.schema.renameTable('paymentPointers', 'walletAddresses'),
knex.schema.alterTable('walletAddresses', function (table) {
table.foreign(['assetId']).references('assets.id')
table.unique('url')
}),
knex.raw(
'ALTER INDEX "paymentPointers_pkey" RENAME TO "walletAddresses_pkey"'
),
knex.raw(
'ALTER INDEX "paymentpointers_url_index" RENAME TO "walletAddresses_url_index"'
),
knex.raw(
'ALTER INDEX "paymentpointers_processat_index" RENAME TO "walletAddresses_processat_index"'
),
knex.raw(
'ALTER TABLE "walletAddresses" DROP CONSTRAINT "paymentpointers_url_unique"'
),
knex.raw(
'ALTER TABLE "walletAddresses" DROP CONSTRAINT "paymentpointers_assetid_foreign"'
),
knex.schema.renameTable('paymentPointerKeys', 'walletAddressKeys'),
knex.schema.alterTable('walletAddressKeys', function (table) {
table.renameColumn('paymentPointerId', 'walletAddressId')
table.foreign('walletAddressId').references('walletAddresses.id')
}),
knex.raw(
'ALTER INDEX "paymentPointerKeys_pkey" RENAME TO "walletAddressKeys_pkey"'
),
knex.raw(
'ALTER TABLE "walletAddressKeys" DROP CONSTRAINT "paymentpointerkeys_paymentpointerid_foreign"'
),
knex.schema.alterTable('quotes', function (table) {
table.dropForeign(['paymentPointerId'])
table.dropIndex(['paymentPointerId', 'createdAt', 'id'])
table.renameColumn('paymentPointerId', 'walletAddressId')
table.foreign('walletAddressId').references('walletAddresses.id')
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 not actually sure, what happens to the existing paymentPointer.id foreign key here? Does it get properly handled in the rename, or dropped?

Copy link
Contributor

@BlairCurrey BlairCurrey Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This also applies the indexes (not pictured above) in line 16: table.index(['walletAddressId', 'createdAt', 'id']).

Looks like the migration is duplicating these. Two fkey constraints with different fkey names but the same relationship. I think we can just remove the table.foreign('walletAddressId').references('walletAddresses.id') line (and the indexes). This is what I see in Indexes and Foreign-key constraints: from doing a \d quotes after this migration.

quote:

Indexes:
    "quotes_pkey" PRIMARY KEY, btree (id)
    "quotes_paymentpointerid_createdat_id_index" btree ("walletAddressId", "createdAt", id)
    "quotes_walletaddressid_createdat_id_index" btree ("walletAddressId", "createdAt", id)
Foreign-key constraints:
    "quotes_assetid_foreign" FOREIGN KEY ("assetId") REFERENCES assets(id)
    "quotes_feeid_foreign" FOREIGN KEY ("feeId") REFERENCES fees(id)
    "quotes_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    "quotes_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)

and here is some of \d "walletAddresses" for good measure (see the duplicate quote fkey):

Referenced by:
    TABLE ""incomingPayments"" CONSTRAINT "incomingpayments_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE ""incomingPayments"" CONSTRAINT "incomingpayments_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE ""outgoingPayments"" CONSTRAINT "outgoingpayments_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE ""outgoingPayments"" CONSTRAINT "outgoingpayments_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE ""walletAddressKeys"" CONSTRAINT "paymentpointerkeys_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE "quotes" CONSTRAINT "quotes_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE "quotes" CONSTRAINT "quotes_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)
    TABLE ""walletAddressKeys"" CONSTRAINT "walletaddresskeys_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id)

Copy link
Contributor

@BlairCurrey BlairCurrey Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually instead of removing these lines we might want to just drop the old one. This way we end up with "quotes_walletaddressid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id) instead of "quotes_paymentpointerid_foreign" FOREIGN KEY ("walletAddressId") REFERENCES "walletAddresses"(id).

So doing like:

    knex.schema.alterTable('quotes', function (table) {
      table.dropForeign(['paymentPointerId'])
      table.dropIndex(['paymentPointerId', 'createdAt', 'id'])
      table.renameColumn('paymentPointerId', 'walletAddressId')
      table.foreign('walletAddressId').references('walletAddresses.id')
      table.index(['walletAddressId', 'createdAt', 'id'])
    }),

table.index(['walletAddressId'])
table.index(['createdAt'])
table.index(['id'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The indexes are not aggregated because the original indexes weren't either. Aggregating them produces, for example:

Indexes:
"outgoingpayments_walletaddressid_createdat_id_index" btree ("walletAddressId", "createdAt", id)

Whereas originally the indexes were not aggregated:

"outgoingpayments_paymentpointerid__index" btree ("paymentPointerId")
"outgoingpayments_createdat_index" btree ("createdAt")
"outgoingpayments_id_index" btree (id)

So table.index is called separately each time to preserve this.

Copy link
Contributor

@BlairCurrey BlairCurrey Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The indexes are not aggregated because the original indexes weren't either.

It seems like they are originally 1 composite index to me but maybe I'm missing something... without running this migration I see "outgoingpayments_paymentpointerid_createdat_id_index" btree ("paymentPointerId", "createdAt", id) (same for quotes, did not check the rest). It's also what I would expect from the original migration:

rafiki/packages/backend/migrations/20221129213751_create_outgoing_payments_table.js

Line 28 in 07cff89

table.index(['paymentPointerId', 'createdAt', 'id'])

If they were seperate indexes I also don't think we'd need to do table.dropIndex(['paymentPointerId', 'createdAt', 'id']), but instead would just drop the payment pointer one (and thus not have to re-add the others).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, they're composite. A misreading on my part earlier. So we'll keep the composite dropping & adding of these indexes.

}),
knex.schema.alterTable('incomingPayments', function (table) {
table.dropForeign(['paymentPointerId'])
table.dropIndex(['paymentPointerId', 'createdAt', 'id'])
table.renameColumn('paymentPointerId', 'walletAddressId')
table.foreign('walletAddressId').references('walletAddresses.id')
table.index(['walletAddressId'])
table.index(['createdAt'])
table.index(['id'])
}),
knex.schema.alterTable('outgoingPayments', function (table) {
table.dropForeign(['paymentPointerId'])
table.dropIndex(['paymentPointerId', 'createdAt', 'id'])
table.renameColumn('paymentPointerId', 'walletAddressId')
table.foreign('walletAddressId').references('walletAddresses.id')
table.index(['walletAddressId'])
table.index(['createdAt'])
table.index(['id'])
}),
knex('webhookEvents')
.update({
// renames paymentPointer keys (if any) to walletAddress in data json column
data: knex.raw(
"data::jsonb - 'paymentPointer' || jsonb_build_object('walletAddress', data::jsonb->'paymentPointer')"
)
})
.whereRaw("data->'paymentPointer' is not null"),
knex('webhookEvents')
.update({
// renames paymentPointerId keys (if any) to walletAddressId in data json column
data: knex.raw(
"data::jsonb - 'paymentPointerId' || jsonb_build_object('walletAddressId', data::jsonb->'paymentPointerId')"
)
})
.whereRaw("data->'paymentPointerId' is not null"),
knex('webhookEvents')
.update({
// renames paymentPointerUrl keys (if any) to walletAddressUrl in data json column
data: knex.raw(
"data::jsonb - 'paymentPointerUrl' || jsonb_build_object('walletAddressUrl', data::jsonb->'paymentPointerUrl')"
)
})
.whereRaw("data->'paymentPointerUrl' is not null"),
knex('webhookEvents')
.update({
// renames payment_pointer.not_found values (if any) to wallet_address.not_found for type key in data json column
type: knex.raw("REPLACE(type, 'payment_pointer.', 'wallet_address.')")
})
.whereLike('type', 'payment_pointer%')
])
Comment on lines +78 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

i inserted some dummy data before this migration and this and similar all seem to be working correctly

}

/**
* @param { import("knex").Knex } knex
* @returns { Promise<void> }
*/
exports.down = function (knex) {
return Promise.all([
knex.schema.renameTable('walletAddresses', 'paymentPointers'),
knex.schema.renameTable('walletAddressKeys', 'paymentPointerKeys'),
knex.schema.alterTable('paymentPointerKeys', function (table) {
table.dropForeign(['walletAddressId'])
table.renameColumn('walletAddressId', 'paymentPointerId')
table.foreign('paymentPointerId').references('paymentPointers.id')
}),
knex.schema.alterTable('quotes', function (table) {
table.dropForeign(['walletAddressId'])
table.dropIndex(['walletAddressId', 'createdAt', 'id'])
table.renameColumn('walletAddressId', 'paymentPointerId')
table.foreign('paymentPointerId').references('paymentPointers.id')
table.index(['paymentPointerId', 'createdAt', 'id'])
}),
knex.schema.alterTable('incomingPayments', function (table) {
table.dropForeign(['walletAddressId'])
table.dropIndex(['walletAddressId', 'createdAt', 'id'])
table.renameColumn('walletAddressId', 'paymentPointerId')
table.foreign('paymentPointerId').references('paymentPointers.id')
table.index(['paymentPointerId', 'createdAt', 'id'])
}),
knex.schema.alterTable('outgoingPayments', function (table) {
table.dropForeign(['walletAddressId'])
table.dropIndex(['walletAddressId', 'createdAt', 'id'])
table.renameColumn('walletAddressId', 'paymentPointerId')
table.foreign('paymentPointerId').references('paymentPointers.id')
table.index(['paymentPointerId', 'createdAt', 'id'])
}),
knex('webhookEvents')
.update({
data: knex.raw(
"data::jsonb - 'walletAddress' || jsonb_build_object('paymentPointer', data::jsonb->'walletAddress')"
)
})
.whereRaw("data->'walletAddress' is not null"),
knex('webhookEvents')
.update({
data: knex.raw(
"data::jsonb - 'walletAddressId' || jsonb_build_object('paymentPointerId', data::jsonb->'walletAddressId')"
)
})
.whereRaw("data->'walletAddressId' is not null"),
knex('webhookEvents')
.update({
data: knex.raw(
"data::jsonb - 'walletAddressUrl' || jsonb_build_object('paymentPointerUrl', data::jsonb->'walletAddressUrl')"
)
})
.whereRaw("data->'walletAddressUrl' is not null"),
knex('webhookEvents')
.update({
type: knex.raw("REPLACE(type, 'wallet_address.', 'payment_pointer.')")
})
.whereLike('type', 'wallet_address%')
])
}
2 changes: 1 addition & 1 deletion packages/backend/src/graphql/resolvers/combined_payments.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ describe('Payment', (): void => {
id: combinedOutgoingPayment.id,
type: combinedOutgoingPayment.type,
metadata: combinedOutgoingPayment.metadata,
paymentPointerId: combinedOutgoingPayment.paymentPointerId,
paymentPointerId: combinedOutgoingPayment.walletAddressId,
state: combinedOutgoingPayment.state,
createdAt: combinedOutgoingPayment.createdAt.toISOString()
})
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/graphql/resolvers/combined_payments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function paymentToGraphql(payment: CombinedPayment): SchemaPayment {
id: payment.id,
type: payment.type,
state: payment.state,
paymentPointerId: payment.paymentPointerId,
paymentPointerId: payment.walletAddressId,
metadata: payment.metadata,
createdAt: new Date(payment.createdAt).toISOString(),
__typename: 'Payment'
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/graphql/resolvers/incoming_payment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export function paymentToGraphql(
): SchemaIncomingPayment {
return {
id: payment.id,
paymentPointerId: payment.paymentPointerId,
paymentPointerId: payment.walletAddressId,
state: payment.state,
expiresAt: payment.expiresAt.toISOString(),
incomingAmount: payment.incomingAmount,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/graphql/resolvers/outgoing_payment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export function paymentToGraphql(
): SchemaOutgoingPayment {
return {
id: payment.id,
paymentPointerId: payment.paymentPointerId,
paymentPointerId: payment.walletAddressId,
state: payment.state,
error: payment.error,
stateAttempts: payment.stateAttempts,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/graphql/resolvers/quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export const getPaymentPointerQuotes: PaymentPointerResolvers<ApolloContext>['qu
export function quoteToGraphql(quote: Quote): SchemaQuote {
return {
id: quote.id,
paymentPointerId: quote.paymentPointerId,
paymentPointerId: quote.walletAddressId,
receiver: quote.receiver,
debitAmount: quote.debitAmount,
receiveAmount: quote.receiveAmount,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/payment/combined/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class CombinedPayment extends PaginationModel {
// common to both incoming and outgoing payments
public id!: string
public state!: OutgoingPaymentState | IncomingPaymentState
public paymentPointerId!: string
public walletAddressId!: string
public metadata?: Record<string, unknown>
public client?: string
public createdAt!: Date
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/payment/incoming/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ export class IncomingPayment
const data: IncomingPaymentData = {
incomingPayment: {
id: this.id,
paymentPointerId: this.paymentPointerId,
paymentPointerId: this.walletAddressId,
createdAt: new Date(+this.createdAt).toISOString(),
expiresAt: this.expiresAt.toISOString(),
receivedAmount: {
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/payment/incoming/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ async function createIncomingPayment(
}
const incomingPayment = await IncomingPayment.query(trx || deps.knex)
.insertAndFetch({
paymentPointerId,
walletAddressId: paymentPointerId,
client,
assetId: paymentPointer.asset.id,
expiresAt,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/payment/outgoing/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class OutgoingPayment
const data: PaymentData = {
payment: {
id: this.id,
paymentPointerId: this.paymentPointerId,
paymentPointerId: this.walletAddressId,
state: this.state,
receiver: this.receiver,
debitAmount: {
Expand Down
4 changes: 2 additions & 2 deletions packages/backend/src/open_payments/payment/outgoing/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ async function createOutgoingPayment(
const payment = await OutgoingPayment.query(trx)
.insertAndFetch({
id: options.quoteId,
paymentPointerId,
walletAddressId: paymentPointerId,
metadata: options.metadata,
state: OutgoingPaymentState.Funding,
client: options.client,
Expand All @@ -130,7 +130,7 @@ async function createOutgoingPayment(
.withGraphFetched('[quote.asset]')

if (
payment.paymentPointerId !== payment.quote.paymentPointerId ||
payment.walletAddressId !== payment.quote.walletAddressId ||
payment.quote.expiresAt.getTime() <= payment.createdAt.getTime()
) {
throw OutgoingPaymentError.InvalidQuote
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/payment_pointer/key/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Pojo } from 'objection'

export class PaymentPointerKey extends BaseModel {
public static get tableName(): string {
return 'paymentPointerKeys'
return 'walletAddressKeys'
}

public id!: string
Expand Down
12 changes: 6 additions & 6 deletions packages/backend/src/open_payments/payment_pointer/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,24 @@ export class PaymentPointer
implements ConnectorAccount, LiquidityAccount
{
public static get tableName(): string {
return 'paymentPointers'
return 'walletAddresses'
}

static relationMappings = () => ({
asset: {
relation: Model.HasOneRelation,
modelClass: Asset,
join: {
from: 'paymentPointers.assetId',
from: 'walletAddresses.assetId',
to: 'assets.id'
}
},
keys: {
relation: Model.HasManyRelation,
modelClass: PaymentPointerKey,
join: {
from: 'paymentPointers.id',
to: 'paymentPointerKeys.paymentPointerId'
from: 'walletAddresses.id',
to: 'walletAddressKeys.paymentPointerId'
}
}
})
Expand Down Expand Up @@ -175,7 +175,7 @@ class SubresourceQueryBuilder<
export abstract class PaymentPointerSubresource extends BaseModel {
public static readonly urlPath: string

public readonly paymentPointerId!: string
public readonly walletAddressId!: string
public paymentPointer?: PaymentPointer

public abstract readonly assetId: string
Expand All @@ -190,7 +190,7 @@ export abstract class PaymentPointerSubresource extends BaseModel {
modelClass: PaymentPointer,
join: {
from: `${this.tableName}.paymentPointerId`,
to: 'paymentPointers.id'
to: 'walletAddresses.id'
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/quote/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class Quote extends PaymentPointerSubresource {
json = super.$formatJson(json)
return {
id: json.id,
paymentPointerId: json.paymentPointerId,
paymentPointerId: json.walletAddressId,
receiver: json.receiver,
debitAmount: {
...json.debitAmount,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/open_payments/quote/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ async function createQuote(
return await Quote.transaction(deps.knex, async (trx) => {
const quote = await Quote.query(trx)
.insertAndFetch({
paymentPointerId: options.paymentPointerId,
walletAddressId: options.paymentPointerId,
assetId: paymentPointer.assetId,
receiver: options.receiver,
debitAmount: {
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/tests/combinedPayment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function toCombinedPayment(
return CombinedPayment.fromJson({
type,
id: payment.id,
paymentPointerId: payment.paymentPointerId,
paymentPointerId: payment.walletAddressId,
state: payment.state,
metadata: payment.metadata,
client: payment.client,
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/tests/quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export async function createQuote(

return await Quote.query()
.insertAndFetch({
paymentPointerId,
walletAddressId: paymentPointerId,
assetId: paymentPointer.assetId,
receiver: receiverUrl,
debitAmount,
Expand Down