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

Fix validation script after adding structured errors #235

Merged
merged 11 commits into from
Nov 7, 2024

Conversation

amanusk
Copy link
Collaborator

@amanusk amanusk commented Oct 20, 2024

This does requires messages to be a list, which might not be what we want


This change is Reviewable

@kkovaacs
Copy link

This is great -- but I don't think we should change the specification in that way just to make validation possible.

Looks like there's an open issue on @json-schema-tools/dereferencer for handling recursive schemas: json-schema-tools/dereferencer#608

Would it be possible to:

  • rebase this branch to the current state of things (after merging the starknet_getStorageProof change)
  • include just the schema fixes that are purely fixes and maybe skip that single change that makes the child CONTRACT_EXECUTION_ERROR an array?

@amanusk
Copy link
Collaborator Author

amanusk commented Oct 22, 2024

Agree that it is a bit backwards and not ideal.
I'll try exploring ways to fix the validation script in a way that does not require this change.
But it would be a shame to forgo all validations going forward, and since this component is in the main API schema, it breaks validating all other schemas as well :(

@ArielElp ArielElp self-requested a review November 7, 2024 13:08
Comment on lines +3756 to +3761
"CONTRACT_EXECUTION_ERROR": {
"description": "structured error that can later be processed by wallets or sdks",
"title": "contract execution error",
"$ref": "#/components/schemas/CONTRACT_EXECUTION_ERROR_INNER"
},
"CONTRACT_EXECUTION_ERROR_INNER": {
Copy link

Choose a reason for hiding this comment

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

Ah, this is a much neater trick than the previous one! Thanks for finding out how to make this work! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good thing you insisted

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1, 3 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @kkovaacs)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kkovaacs)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Dismissed @kkovaacs from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @kkovaacs)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @amanusk)

@ArielElp ArielElp merged commit 5f81980 into starkware-libs:v0.8.0 Nov 7, 2024
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.

3 participants