Skip to content

feat: update outgoing payment limits spec #564

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

Merged
merged 11 commits into from
Apr 1, 2025

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Mar 12, 2025

Changes proposed in this pull request

  • Prevents outgoing payment grant requests where debitAmount and receiveAmount are both included in the limits.

Context

Closes #471.

Copy link

changeset-bot bot commented Mar 12, 2025

🦋 Changeset detected

Latest commit: 6a474dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@interledger/open-payments Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Mar 12, 2025

Deploy Preview for openpayments-preview ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/openpayments-preview/deploys/67e29d4638c80400b528c072
😎 Deploy Preview https://deploy-preview-564--openpayments-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 170 to 182
}> &
Partial<{
receiver?: external["schemas.yaml"]["components"]["schemas"]["receiver"];
interval?: components["schemas"]["interval"];
/** @description All amounts are maxima, i.e. multiple payments can be created under a grant as long as the total amounts of these payments do not exceed the maximum amount per interval as specified in the grant. */
debitAmount: external["schemas.yaml"]["components"]["schemas"]["amount"];
}> &
Partial<{
receiver?: external["schemas.yaml"]["components"]["schemas"]["receiver"];
interval?: components["schemas"]["interval"];
/** @description All amounts are maxima, i.e. multiple payments can be created under a grant as long as the total amounts of these payments do not exceed the maximum amount per interval as specified in the grant. */
receiveAmount: external["schemas.yaml"]["components"]["schemas"]["amount"];
}>;
Copy link
Contributor

Choose a reason for hiding this comment

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

These types would allow the client to take in both receiveAmount or debitAmount (because of the old openapi-typescript lib). I've updated it here (and upgraded to TS 5 here) so we can get those in first)

Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

I think we should add some runtime assertions as well when we are dealing with mutually exclusive types and not fully rely on TS errors.

@njlie njlie force-pushed the nl/471/outgoing-payment-limits-update branch from 04d1cd4 to d326dbc Compare March 17, 2025 20:39
outgoingPaymentAccess?.limits?.debitAmount &&
outgoingPaymentAccess.limits?.receiveAmount
) {
throw new Error('Invalid Grant Request')
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can throw an OpenPaymentsClientError here

@njlie njlie force-pushed the nl/471/outgoing-payment-limits-update branch from 4cc30fa to acca56a Compare March 18, 2025 18:14
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

I hoped that the openapi-typescript update will fix the XOR issue that we are facing for mutually exclusive types, but it looks like it is not generating proper XOR for oneOf. Right now, TypeScript will not complain if a client still provides both amounts.

Since this is a major bump, we can improve the types a bit. Let me know what you think about this.

Comment on lines 165 to 172
export type OutgoingPaymentLimitWithDebitAmount = Extract<
ASComponents['schemas']['access-outgoing']['limits'],
{ debitAmount: components['schemas']['amount'] }
>
export type OutgoingPaymentLimitWithReceiveAmount = Extract<
ASComponents['schemas']['access-outgoing']['limits'],
{ receiveAmount: components['schemas']['amount'] }
>
Copy link
Member

@raducristianpopa raducristianpopa Mar 19, 2025

Choose a reason for hiding this comment

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

Suggested change
export type OutgoingPaymentLimitWithDebitAmount = Extract<
ASComponents['schemas']['access-outgoing']['limits'],
{ debitAmount: components['schemas']['amount'] }
>
export type OutgoingPaymentLimitWithReceiveAmount = Extract<
ASComponents['schemas']['access-outgoing']['limits'],
{ receiveAmount: components['schemas']['amount'] }
>
// From: https://github.com/microsoft/TypeScript/issues/14094#issuecomment-373782604
type Without<T, U> = { [P in Exclude<keyof T, keyof U>]?: never }
type XOR<T, U> = T | U extends object
? (Without<T, U> & U) | (Without<U, T> & T)
: T | U
type XOR3<T, U, V> = XOR<T, XOR<U,V>>
export type AccessOutgoingBase = {
receiver?: ASComponents['schemas']['receiver']
interval?: ASComponents['schemas']['interval']
}
export type AccessOutgoingWithDebitAmount = AccessOutgoingBase & {
debitAmount: ASComponents['schemas']['amount']
}
export type AccessOutgoingWithReceiveAmount = AccessOutgoingBase & {
receiveAmount: ASComponents['schemas']['amount']
}
export type AccessOutgoingLimits = XOR3<
AccessOutgoingBase,
AccessOutgoingWithDebitAmount,
AccessOutgoingWithReceiveAmount
>
export type AccessOutgoing = {
type: 'outgoing-payment'
actions: ('create' | 'read' | 'read-all' | 'list' | 'list-all')[]
identifier: string
limits?: AccessOutgoingLimits
}
export type GrantRequestAccessItem =
| ASComponents['schemas']['access-incoming']
| AccessOutgoing
| ASComponents['schemas']['access-quote']

With the new GrantRequestAccessItem, we can now update GrantRequest from

export type GrantRequest = {
access_token: ASOperations['post-request']['requestBody']['content']['application/json']['access_token']
client: ASOperations['post-request']['requestBody']['content']['application/json']['client']
interact?: ASOperations['post-request']['requestBody']['content']['application/json']['interact']
}

to:

export type GrantRequest = {
  access_token: { access: GrantRequestAccessItem[] }
  client: ASOperations['post-request']['requestBody']['content']['application/json']['client']
  interact?: ASOperations['post-request']['requestBody']['content']['application/json']['interact']
}

Now, if a client will provide both amounts, a TypeScript error will be raised - in their IDE or at transpilation time:

open-payments/packages/open-payments on  nl/471/outgoing-payment-limits-update [!?] via   v20.18.3 pnpm tsc --noEmit
src/scratchpad.ts:26:15 - error TS2322: Type '{ assetScale: number; assetCode: string; value: string; }' is not assignable to type 'undefined'.

26               receiveAmount: {
                 ~~~~~~~~~~~~~


Found 1 error in src/scratchpad.ts:26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks great! Thanks for writing it up. The discussion on native XOR implementation is very spirited...

Copy link
Contributor

Choose a reason for hiding this comment

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

@raducristianpopa I would think it would say is not assignable to type 'never'. instead, but it seems to do the trick

Comment on lines 67 to 70
(outgoingPaymentAccess?.limits as OutgoingPaymentLimitWithDebitAmount)
?.debitAmount &&
(outgoingPaymentAccess?.limits as OutgoingPaymentLimitWithReceiveAmount)
?.receiveAmount
Copy link
Member

Choose a reason for hiding this comment

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

(if we proceed with the new types)

Suggested change
(outgoingPaymentAccess?.limits as OutgoingPaymentLimitWithDebitAmount)
?.debitAmount &&
(outgoingPaymentAccess?.limits as OutgoingPaymentLimitWithReceiveAmount)
?.receiveAmount
(outgoingPaymentAccess?.limits as OutgoingAccessWithDebitAmount)
?.debitAmount &&
(outgoingPaymentAccess?.limits as OutgoingAccessWithReceiveAmount)
?.receiveAmount

description: 'All amounts are maxima, i.e. multiple payments can be created under a grant as long as the total amounts of these payments do not exceed the maximum amount per interval as specified in the grant.'
$ref: ./schemas.yaml#/components/schemas/amount
interval:
$ref: '#/components/schemas/interval'
anyOf:
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 thinking that oneOf might be the property we should go for? Based on OpenAPI docs:

oneOf – validates the value against exactly one of the subschemas
anyOf – validates the value against any (one or more) of the subschemas

Comment on lines 72 to 74
throw new OpenPaymentsClientError('Invalid Grant Request', {
description: 'Invalid Request'
})
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this would help the SDK user find the source of the error more easily when using JS directly or when not caring about the TS errors.

Suggested change
throw new OpenPaymentsClientError('Invalid Grant Request', {
description: 'Invalid Request'
})
throw new OpenPaymentsClientError('Invalid Grant Request', {
description: 'Invalid grant request. Both "debitAmount" and "receiveAmount" were provided. Please specify only one of them.'
})

@njlie njlie force-pushed the nl/471/outgoing-payment-limits-update branch 2 times, most recently from a900d8f to 8669a93 Compare March 20, 2025 22:59
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

@njlie it looks like oneOf is working a bit differently from what I expected when reading the docs - https://swagger.io/docs/specification/v3_0/data-models/oneof-anyof-allof-not/#difference-between-anyof-and-oneof (???)

I think that the issue is with the interval and receiver properties since they "match" with multiple schemas when using oneOf. Reverting to anyOf should be the go to solution in this case. Sorry for putting you on the wrong track! 🙇‍♂️

@njlie
Copy link
Contributor Author

njlie commented Mar 24, 2025

@njlie it looks like oneOf is working a bit differently from what I expected when reading the docs - swagger.io/docs/specification/v3_0/data-models/oneof-anyof-allof-not#difference-between-anyof-and-oneof (???)

I think that the issue is with the interval and receiver properties since they "match" with multiple schemas when using oneOf. Reverting to anyOf should be the go to solution in this case. Sorry for putting you on the wrong track! 🙇‍♂️

I also interpreted it the same way as you, at first - so I thought I could "debug" what the issue with the validation was. But this explains why it's been so confusing. Thanks for flagging this anyways 👍

@njlie njlie force-pushed the nl/471/outgoing-payment-limits-update branch from cb0cb7d to ee71dd3 Compare March 24, 2025 17:13
@njlie njlie requested a review from raducristianpopa March 24, 2025 17:56
@mkurapov
Copy link
Contributor

Interesting, so oneOf to properly work needs all the keys in the objects to be exclusive from one another? Pretty odd but good to know 🤷

@njlie njlie merged commit 7fb1f25 into main Apr 1, 2025
11 checks passed
@njlie njlie deleted the nl/471/outgoing-payment-limits-update branch April 1, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update limits of outgoing payment grant creation
3 participants