-
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(open-payments): adding open-payments package #669
Conversation
@@ -0,0 +1,64 @@ | |||
import { components } from './types' | |||
import axios, { AxiosInstance } from 'axios' |
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.
considered universal/isomorphic fetch ?
node 18 LTS is due very soon
https://dev.to/cloudx/nodejs-18-fetch-api-test-runner-module-and-more-2ckg
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.
Yes, looks to be out Oct 25th: https://github.com/nodejs/release#release-schedule. I have axios
for now as I was mirroring the existing OpenPaymentsClientService
https://github.com/interledger/rafiki/blob/main/packages/backend/src/open_payments/client/service.ts#L153-L180. I know we use axios
in a few places so maybe have a team discussion whether we want to upgrade node and replace axios
with fetch
altogether once node 18 is out
}, | ||
"devDependencies": { | ||
"@types/node": "^18.7.12", | ||
"openapi-typescript": "^4.5.0", |
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 wrested quite a bit with this, but I wasn't able to use the newest version of this library. Using the programmatic way of generating types via openapiTS
anything above v5 does not work and result in ERR_REQUIRE_ESM
errors. Versions 5 and above do not support backwards compatibility with CommonJS, especially given the fact that having type: module
in package.json
breaks using this package as a workspace in the /backend
project.
one two three are the related issues in the openapi-typescript
repo.
@@ -0,0 +1,5 @@ | |||
export default { | |||
OPEN_PAYMENTS_OPEN_API_URL: |
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.
want to make sure the same URL is used for type generation and (soon to be added) open API response validation
|
||
await get(axiosInstance, { | ||
url: `${baseUrl}/incoming-payment`, | ||
accessToken: 'accessToken' |
Check failure
Code scanning / CodeQL
Hard-coded credentials
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 is a test file)
`${baseUrl}/incoming-payment`, | ||
{ | ||
headers: { | ||
Authorization: 'GNAP accessToken', |
Check failure
Code scanning / CodeQL
Hard-coded credentials
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 is a test file)
export const createClient = ( | ||
args?: CreateOpenPaymentClientArgs | ||
): OpenPaymentsClient => { | ||
const axios = createAxiosInstance(args) | ||
|
||
return { | ||
incomingPayment: { | ||
get: (args: GetArgs) => get<IncomingPayment>(axios, args) | ||
}, | ||
ilpStreamConnection: { | ||
get: (args: GetArgs) => get<ILPStreamConnection>(axios, args) | ||
} | ||
} | ||
} |
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.
Wanted to keep very simple for the first PR, will be adding open-api validation next.
import { components } from './generated/types' | ||
|
||
export type IncomingPayment = components['schemas']['incoming-payment'] | ||
export type ILPStreamConnection = components['schemas']['ilp-stream-connection'] |
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.
Can definitely be changed to something less wordy than ILPStreamConnection
, but I was just following the naming in the spec
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.
Will add documentation once there is a proper usage of the client in the |
@@ -0,0 +1,702 @@ | |||
/** |
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.
At one point we had (tried to have) a check that the generated
files matched the result of generate
(here generate:types
) script.
Do you think that's worth doing here?
https://github.com/interledger/rafiki/pull/453/files
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.
@wilsonianb what we can do is instead of that check, is actually just run the script on a pre-commit hook to generate the types and stage the updated file for committing. Instead of doing it on every commit, we can just do that only if the src/types.ts
file has been changed, for example. Thoughts?
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.
That sounds good to me.
We did recently remove pre-commit hooks, but hopefully it won't be an issue to support this
import { components } from './generated/types' | ||
|
||
export type IncomingPayment = components['schemas']['incoming-payment'] | ||
export type ILPStreamConnection = components['schemas']['ilp-stream-connection'] |
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.
@wilsonianb the openpayments spec split has caused the type generation to break, it will require a change to that repo to support generating these types. Basically, when referencing the shared schema, the types are generated as such: Basically, nothing gets generated for the shared schema: This is because the shared schemas do not list components under eg the shared schema is declared as |
* feat(open-payments-client): initial POC of open-payments-client * feat(open-payments-client): cleanup * feat(open-payments): remove open-payments-client folder * feat(open-payments): make open-payments folder and use script for type generation * feat(open-payments): fix rootDir * feat(open-payments): downgrading generation lib, updating how script is ran * feat(open-payments): update default config usage * feat(open-payments): update pnpm-lock.yaml * feat(open-payments): update pnpm-lock.yaml * feat(open-payments): adding tests * feat(open-payments): update pnpm-lock.yaml * feat(open-payments): simplify test * feat(open-payments): updating workflows * feat(open-payments): pin the open api spec to the most recent commit * feat(open-payments): use updated RS spec * feat(open-payments): building open-api package during workflow * Revert "feat(open-payments): building open-api package during workflow" This reverts commit e11ec65.
Changes proposed in this pull request
This is a first PR for roadmap item: interledger/roadmap#20. More discussion can be found there.
This PR deals with creating a new package,
open-payments
. It uses the openapi-typescript package to generate Typescript types from the Open API resource server spec, and then exports the types (to be used in the/backend
package). It also exports a client to be used when making rafiki to rafiki calls via the Open Payments API.The usage of the client is as follows:
Additional changes are to follow, want to keep the PR sizes manageable:
open-api
package)/backend
package, replacing how the incoming-payments and connections are grabbed in the existingOpenPaymentsClientService
Context
The related issues are #663, and #662.
Checklist
fixes #number