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): remove payment pointer from url, retrieve it from request query or body #2042

Merged
merged 6 commits into from
Oct 20, 2023

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Oct 17, 2023

Changes proposed in this pull request

  • Removes the payment pointer from the path of subresource and collection requests on wallet addresses.
  • Retrieves the wallet address from the query params if it is a GET request or from the request body if it is a POST request on a subresource/collection.

Context

Fixes #1939.

Checklist

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

@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Oct 17, 2023
@@ -104,7 +104,7 @@ exports.down = function (knex) {
'ALTER INDEX "walletAddresses_pkey" RENAME TO "paymentPointers_pkey"'
),
knex.raw(
'ALTER INDEX "walletAddresses_url_index" RENAME TO "paymentPointers_url_index"'
'ALTER INDEX "walletaddresses_url_index" RENAME TO "paymentpointers_url_index"'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing some typos in the migration

@njlie njlie force-pushed the nl/1939/remove-wallet-address-path branch from 32edfd3 to 7a0775d Compare October 17, 2023 17:21
@@ -128,7 +128,7 @@ exports.down = function (knex) {
'ALTER INDEX "walletAddressKeys_pkey" RENAME TO "paymentPointerKeys_pkey"'
),
knex.raw(
'ALTER TABLE "paymentpointerKeys" DROP CONSTRAINT "walletaddresskeys_paymentpointerid_foreign"'
'ALTER TABLE "paymentPointerKeys" DROP CONSTRAINT "walletaddresskeys_walletaddressid_foreign"'
Copy link
Member

Choose a reason for hiding this comment

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

What is walletaddresskeys_walletaddressid_foreign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a foreign key on the table walletAddressKeys that links it to walletAddress via its id property. The line referenced here is the rollback for this migration, so it's part of reverting the foreign key from referring to wallet addresses to payment pointers.

Comment on lines +74 to 114
if (!incomingPayment || !incomingPayment.walletAddress) return ctx.throw(404)
const connection = deps.connectionService.get(incomingPayment)
ctx.body = incomingPaymentToBody(
ctx.walletAddress,
incomingPayment.walletAddress,
incomingPayment,
connection
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming the connection part is going to be removed in a subsequent PR from @BlairCurrey ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct.

Copy link
Contributor

@BlairCurrey BlairCurrey Oct 18, 2023

Choose a reason for hiding this comment

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

This part will actually still be here but with a paired down connection type. After talking w/ Max our plan was to leave the connection service because we still need to get credentials from the stream server. So we will just have ConnectionService.get that returns something like this (and getting rid of the current Connection model):

{
  ilpAddress: IlpAddress
  sharedSecret: Buffer
}

So internally we still have some concept of connections but they are basically just the stream credentials. I could probably see a case for putting this somewhere in IncomingPaymentsService or the model but for now this is the simpler refactor.

EDIT: we have since decided to change this to rename this to streamCredentialService

@njlie njlie requested a review from sabineschaller October 18, 2023 15:18
@njlie njlie force-pushed the nl/1939/remove-wallet-address-path branch from 4c780c6 to 646d1dc Compare October 18, 2023 16:22
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 the createQuote needs to get the walletAddress from body now, instead of ctx.

async function createQuote(
deps: ServiceDependencies,
ctx: CreateContext<CreateBody>
): Promise<void> {
const { body } = ctx.request
const options: CreateQuoteOptions = {
walletAddressId: ctx.walletAddress.id,
receiver: body.receiver,
client: ctx.client

Copy link
Contributor Author

@njlie njlie Oct 18, 2023

Choose a reason for hiding this comment

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

I updated the middleware file to get walletAddress from the body or query when it adds it to the context, so the actual route handlers can still grab walletAddress from the context.

For some routes (e.g. GET /incoming-payments/:id) the identifier is enough to get the payment, so we just return the walletAddress that's fetched along with the payyment.

@BlairCurrey
Copy link
Contributor

Some usages of paymentPointer snuck back in when reintroducing those lost commits (noticed on my branch as well):

describe('get unauthenticated incoming payment', (): void => {
test('Can get incoming payment with public fields', async (): Promise<void> => {
const incomingPayment = await createIncomingPayment(deps, {
paymentPointerId: paymentPointer.id,
expiresAt,
incomingAmount,
metadata
})
const ctx = setup<ReadContextWithAuthenticatedStatus>({
reqOpts: {
headers: { Accept: 'application/json' },
method: 'GET',
url: `/incoming-payments/${incomingPayment.id}`
},
params: {
id: incomingPayment.id
},
paymentPointer
})
ctx.authenticated = false
await expect(incomingPaymentRoutes.get(ctx)).resolves.toBeUndefined()
expect(ctx.response).toSatisfyApiSpec()
expect(ctx.body).toEqual({
receivedAmount: {
value: '0',
assetCode: asset.code,
assetScale: asset.scale
}
})
})
})

paymentPointerId: ctx.paymentPointer.id

ctx.walletAddressUrl = `https://${ctx.request.host}/${ctx.params.paymentPointerPath}`

"raw": "{{pfryPaymentPointer}}/incoming-payments/{{incomingPaymentId}}",
"host": [
"{{pfryPaymentPointer}}"

@github-actions github-actions bot added the type: tests Testing related label Oct 19, 2023
Copy link
Contributor

@BlairCurrey BlairCurrey left a comment

Choose a reason for hiding this comment

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

Looks good. I'm seeing 18 build errors, all related to my connection/payment methods changes which are fixed on my branch. Migrations running and everything.

@njlie njlie merged commit 1b24ebe into nl-open-payments-updates Oct 20, 2023
@njlie njlie deleted the nl/1939/remove-wallet-address-path branch October 20, 2023 14:54
njlie added a commit that referenced this pull request Oct 23, 2023
sabineschaller added a commit that referenced this pull request Oct 26, 2023
* feat(backend): rename "payment pointer" tables & columns to use "wallet address" (#1937)

Co-authored-by: Nathan Lie <[email protected]>

* feat(backend, mock-ase): rename payment pointer to wallet address in rafiki code (#2029)

* fix(backend): rename paymentPointerId column in combinedPaymentsView to walletAddressId (#2043)

* docs: replace payment pointer with wallet address (#1951)

* feat(backend): remove connections route (#1940) (#2051)

Co-authored-by: Ömer Talha Özdemir <[email protected]>

* feat(backend): unauthenticated get incoming payment (#1952) (#2052)

Co-authored-by: Ömer Talha Özdemir <[email protected]>
Co-authored-by: Blair Currey <[email protected]>

* feat(auth): fix auth tests for access tokens (#2053)

* feat(backend): remove payment pointer from url, retrieve it from request query or body (#2042)

* refactor(backend): swap connection for methods (#2054)

Co-authored-by: Nathan Lie <[email protected]>

* fix(backend): resolve breaking build after main rebase (#2085)

* fix(backend): combined_payment, ilp service tests (#2094)

* fix(backend): combined_payment, ilp service tests

* chore(backend): format

* fix(backend): bad test refactor

* fix(postman): invalid json (#2096)

* fix: fetch walletAddress graph in quote and outgoing payment service (#2099)

* fix(postman): env var usage (#2107)

* feat(backend): add quotes route to exceptions in wallet address middleware (#2108)

* chore: formatting

* fix(backend): fix GET /incoming-payments (#2111)

* fix(backend): remove wallet address from OP response id and fix receiver service (#2115)

* fix: remove wallet address from op resource urls

* feat: return auth server url in public incoming payment and fix receiver service

* fix: quote issue

* fix: walletAddress middleware

* chore: update postman

* fix: listing

* chore: remove console.log

* fix: progress towards making listing work

* fix(backend): fix create remote incoming payment (#2121)

* fix(postman): misc postman fixes

* fix(postman): complete incoming payment (#2126)

* fix: file name correction (#2127)

* fix: GET resource lists, use op client (#2122)

* fix(postman): update list urls

* fix(openapi): rm defaults for first, last

* refactor(postman): query param order for readability

* fix(backend): update to new open-payments pkgs

* fix(backend): use op client to get authServer

* fix(backend): rm old code

* fix(backend): receiver tests

* chore: cleanup

* feat: make list query parameter configurable

* chore: update OP package and fix types

---------

Co-authored-by: Sabine Schaller <[email protected]>

* fix(postman): additional fixes

* fix: wallet address on the fly creation (#2129)

* chore: update postman environments

* Postman: update description for P2P example

* Postman: Remove finalizationReason from getGrants API + fix createWalletAddress

* chore: bump open-payments package in auth

* chore: update last few remaining mentions of payment pointers

* chore(postman): remove references to payment pointer

* chore: update another reference to pp

---------

Co-authored-by: Nathan Lie <[email protected]>
Co-authored-by: Ömer Talha Özdemir <[email protected]>
Co-authored-by: Blair Currey <[email protected]>
Co-authored-by: Sabine Schaller <[email protected]>
Co-authored-by: Sarah Jones <[email protected]>
Co-authored-by: Max Kurapov <[email protected]>
Co-authored-by: Max Kurapov <[email protected]>
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
Development

Successfully merging this pull request may close these issues.

3 participants