-
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
Adding AS types & grant requests support to open-payments client #727
Changes from 6 commits
5ae5f3f
deb3081
9b1b87a
17af881
ac71e7c
f0febf0
5aa5370
ca946c8
94f95e5
0a453d5
6eff96c
b219aef
4252c8e
7b6407f
86304fe
f8154ba
1e5a71a
d9d7f7d
d2c0295
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { createGrantRoutes } from './grant' | ||
import { OpenAPI, HttpMethod, createOpenAPI } from 'openapi' | ||
import config from '../config' | ||
import { defaultAxiosInstance, silentLogger } from '../test/helpers' | ||
|
||
describe('grant', (): void => { | ||
let openApi: OpenAPI | ||
|
||
beforeAll(async () => { | ||
openApi = await createOpenAPI(config.OPEN_PAYMENTS_AS_OPEN_API_URL) | ||
}) | ||
|
||
const axiosInstance = defaultAxiosInstance | ||
const logger = silentLogger | ||
|
||
describe('createGrantRoutes', (): void => { | ||
test('creates response validators for grant requests', async (): Promise<void> => { | ||
jest.spyOn(openApi, 'createResponseValidator') | ||
|
||
createGrantRoutes({ axiosInstance, openApi, logger }) | ||
expect(openApi.createResponseValidator).toHaveBeenNthCalledWith(1, { | ||
path: '/', | ||
method: HttpMethod.POST | ||
}) | ||
expect(openApi.createResponseValidator).toHaveBeenNthCalledWith(2, { | ||
path: '/', | ||
method: HttpMethod.POST | ||
}) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
import { HttpMethod } from 'openapi' | ||
import { ClientDeps } from '.' | ||
import { | ||
getASPath, | ||
InteractiveGrant, | ||
InteractiveGrantRequest, | ||
NonInteractiveGrant, | ||
NonInteractiveGrantRequest | ||
} from '../types' | ||
import { post } from './requests' | ||
|
||
interface RequestGrantArgs<T> { | ||
url: string | ||
request: T | ||
} | ||
|
||
export interface GrantRoutes { | ||
requestInteractiveGrant( | ||
args: RequestGrantArgs<InteractiveGrantRequest> | ||
): Promise<InteractiveGrant> | ||
requestNonInteractiveGrant( | ||
args: RequestGrantArgs<NonInteractiveGrantRequest> | ||
): Promise<NonInteractiveGrant> | ||
} | ||
|
||
export const createGrantRoutes = (clientDeps: ClientDeps): GrantRoutes => { | ||
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. We'll also need the grant continuation route, in order to support the full interactive grant flow. |
||
const { axiosInstance, openApi, logger } = clientDeps | ||
|
||
const createInteractiveGrantValidator = | ||
openApi.createResponseValidator<InteractiveGrant>({ | ||
path: getASPath('/'), | ||
method: HttpMethod.POST | ||
}) | ||
const createNonInteractiveGrantValidator = | ||
openApi.createResponseValidator<NonInteractiveGrant>({ | ||
path: getASPath('/'), | ||
method: HttpMethod.POST | ||
}) | ||
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. unfourtunately, needed both to pass type checking when given as arguments to the 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. If we keep |
||
|
||
return { | ||
requestInteractiveGrant: ( | ||
args: RequestGrantArgs<InteractiveGrantRequest> | ||
) => | ||
post( | ||
{ axiosInstance, logger }, | ||
{ url: args.url, body: args.request }, | ||
createInteractiveGrantValidator | ||
), | ||
requestNonInteractiveGrant: ( | ||
args: RequestGrantArgs<NonInteractiveGrantRequest> | ||
) => | ||
post( | ||
{ axiosInstance, logger }, | ||
{ url: args.url, body: args.request }, | ||
createNonInteractiveGrantValidator | ||
) | ||
} | ||
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. At first, I wanted to do something tricky with types & method overloading, where we have 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.
My concern with splitting
requestInteractiveGrant
/requestNonInteractiveGrant
is that I don't think we've standardized what grant requests are interactive. And thus, a client might not be expected to know when to use either.For example, Rafiki has made
incoming-payment
-only grants non-interactive, but I don't know that that's required:@sabineschaller or @njlie or @adrianhopebailie might be able to correct me on this.
A compromise could be to do:
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.
requestNonInteractiveGrant
seems fine as is, since the request should just fail if the AS requires RO interaction for that grant.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 separated these, since at some point, I thought the client need to know that they need to pass in
interact
?https://github.com/interledger/open-payments/blob/main/openapi/auth-server.yaml#L93-L94
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.
Given our mix of grants for different resources needing interact or not, I've been assuming a client might just always assume a grant request requires interaction, but also be able to handle getting back a
NonInteractiveGrant
response.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 still leaning towards
and clients can utilize
isInteractiveGrant
/isNonInteractiveGrant
.I got the impression from Adrian that their policy agent AS might be determining what grant requests require interaction (vs having it Open Payments standardized).
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, I see what you mean, it's more of a configurable, per-instance thing instead of standard, makes sense.
Going to make changes to assume client will always require interaction, but call might return either or.
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 ok also keeping:
But maybe requiring the
interact
field to be specified isn't unreasonable? 🤔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 even in the demo Postman example, there is an interact field present when requesting the incoming payment grant -> which returns a non-interactive one.
We could run with a single call to make things simple for now. Going to play around w it