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: add gnap error response schema to spec #519

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Nov 13, 2024

Changes proposed in this pull request

  • Adds a gnap-error schema to the spec, and references them in the error responses of the auth server endpoint specs.

Context

Closes #518.

Copy link

changeset-bot bot commented Nov 13, 2024

🦋 Changeset detected

Latest commit: e505fc3

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 Minor

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 Nov 13, 2024

Deploy Preview for openpayments-preview ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/openpayments-preview/deploys/673ddd8c8bbc94930f519907
😎 Deploy Preview https://deploy-preview-519--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.

@njlie njlie force-pushed the nl/518/gnap-errors branch from 6d06c97 to 01d8f9b Compare November 13, 2024 22:37
- user_denied
- request_denied
- unknown_interaction
- too_fast
Copy link

@oana-lolea oana-lolea Nov 14, 2024

Choose a reason for hiding this comment

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

I think too_fast shouldn't be indented, because in the generated file "unknown_interaction - too_fast" should be two separated values.

@@ -532,6 +588,26 @@ components:
- debitAmount
- required:
- receiveAmount
gnap-error:
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of having separate objects per error, and then using specific ones per-response?
e.g. we wouldn't have too_fast in a normal grant request, just for grant continuation.

Maybe that is too specific, but curious to hear your opinion

Copy link
Contributor Author

@njlie njlie Nov 14, 2024

Choose a reason for hiding this comment

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

I considered that, but it felt at times overly granular to have a schema for an error that was only referenced once - too_fast is a pretty good example of that, as it's only thrown by the grant continuation endpoint.

In hindsight though, I think it would be more useful to use the specific ones per-response. Biggest reason to me being that these routes may throw the same http code for different reasons, in particular 401 could be thrown by the signature middleware or the actual business logic...

@njlie njlie requested review from oana-lolea and mkurapov November 15, 2024 20:57
@njlie njlie requested a review from oana-lolea November 19, 2024 23:24
@mkurapov
Copy link
Contributor

@njlie given we verify the Open Payments API responses in the Open Payments client, this actually might be breaking if we are returning incorrect errors in the Rafiki AS, particularly if an integrator is on an old version of Rafiki. Is that correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Add GNAP error schema to auth server error responses
3 participants