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

Jump to bottom

chore(backend): replace GrantReference with OutgoingPaymentGrant #800

Merged
merged 2 commits into from
Dec 1, 2022
Merged

chore(backend): replace GrantReference with OutgoingPaymentGrant #800

merged 2 commits into from
Dec 1, 2022

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Nov 30, 2022 •
edited
Loading

Changes proposed in this pull request

  • Remove grant references table
  • Store clientId directly on payment pointer subresources
  • Add outgoing payment grants for locking while validating grant limits on outgoing payment creation

Context

Checklist

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

@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related labels Nov 30, 2022
This was referenced Nov 30, 2022
Closed
Merged
@wilsonianb wilsonianb marked this pull request as ready for review November 30, 2022 00:57
sabineschaller
sabineschaller approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@sabineschaller sabineschaller left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thank you!

@wilsonianb
Copy link
Contributor Author

🤔 Would we still want to be able to link resources with the corresponding grants used to create them, for auditing or some other purpose?
I was just looking at section 2.1.3 of https://static.googleusercontent.com/media/nextbillionusers.google/en//tools/3PPI-2021-whitepaper.pdf which seems to actually recommend that the RS would store the grant permissions (or perhaps just keep things as is with GrantReference since our AS is currently bound to the RS)

@mkurapov
Copy link
Contributor

@wilsonianb It feels like the "RTP" is more of a general concept in this case right? i.e. RTP != RS?

In general, having more information about the grant would be a good thing IMO, could do further checks other than the clientId, for example...

mkurapov
mkurapov reviewed Nov 30, 2022
View reviewed changes
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.

Left a few comments, but wouldn't mind going the other way and expanding the info stored in the og grantreference table

packages/backend/src/open_payments/quote/routes.ts
@@ -60,7 +60,7 @@ async function createQuote(
receiver: body.receiver,
sendAmount: body.sendAmount && parseAmount(body.sendAmount),
receiveAmount: body.receiveAmount && parseAmount(body.receiveAmount),
grantId: ctx.grant?.grant
clientId: ctx.grant?.clientId
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this have to change with #688?

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 think we'd be able to keep our current logic of defining ctx.clientId for everything except read-all/list-all requests

rafiki/packages/backend/src/open_payments/auth/middleware.ts

Lines 80 to 84 in 3a3f9e8

// Unless the relevant grant action is ReadAll/ListAll add the
// clientId to ctx for Read/List filtering
if (access.actions.includes(action)) {
ctx.clientId = grant.clientId
}

packages/backend/src/open_payments/payment/outgoing/model.ts
Comment on lines +12 to +18
export class OutgoingPaymentGrant extends DbErrors(Model) {
public static get modelPaths(): string[] {
return [__dirname]
}
public static readonly tableName = 'outgoingPaymentGrants'
public id!: 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 in its own file? I was trying to find the model when I pulled down the branch originally, but wasn't able to right away

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 wanted to go all in on confining this to outgoing payments.
Are you thinking a separate model file in this directory, or its directory with its own service?

Copy link
Contributor

@mkurapov mkurapov Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense - I was thinking of having one model per file in general, but if its confined as such and won't grow beyond its current scope, then it's good here

packages/backend/src/open_payments/payment/outgoing/service.ts
//lock grant
await deps.grantReferenceService.lock(grant.grant, deps.knex)
// Lock grant
// TODO: update to use objection once it supports forNoKeyUpdate
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a separate issue for this? Or too far out/uncertain 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.

We have a PR that I could include in the comment

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 it's ok for now. Also, I'm guessing reviews haven't been fast on this because of: Vincit/objection.js#2335. Would be good to keep an eye on this thread

@wilsonianb
Copy link
Contributor Author

@wilsonianb It feels like the "RTP" is more of a general concept in this case right? i.e. RTP != RS?

In general, having more information about the grant would be a good thing IMO, could do further checks other than the clientId, for example...

I'm not sure if our RS is more like the RTP or FSP.

Either way, I think we should associate created resources with corresponding grants for auditing purposes. I think just storing the grant id on the resource is currently sufficient with the RS bound to the AS. (The single account provider can look up grant details in the AS db tables. Although maybe we'd need to expand admin api to do so.)
However, do we need that for incoming payments and quotes? If not, then this PR still provides association of outgoing payments with corresponding grants.

@mkurapov
Copy link
Contributor

mkurapov commented Dec 1, 2022

Yeah, because of how the RS & AS are tied together, all the info can be found in the AS anyway if an audit is needed. I like that this simplifies things a bit in general + we don't need extra DB calls for incoming payments & quotes. I could see this changes down the line but LGTM for now

@wilsonianb wilsonianb merged commit cbbc416 into main Dec 1, 2022
@wilsonianb wilsonianb deleted the bw-client-id branch December 1, 2022 19:54
@mkurapov mkurapov mentioned this pull request Dec 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkurapov mkurapov mkurapov approved these changes

@sabineschaller sabineschaller sabineschaller approved these changes

@JoblersTune JoblersTune Awaiting requested review from JoblersTune

@dclipp dclipp Awaiting requested review from dclipp

Assignees
No one assigned
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wilsonianb @mkurapov @sabineschaller