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

chore(jsonrpc): updated rpc_errors_schema #5042

Closed
wants to merge 1 commit into from

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Oct 19, 2021

Update rpc_errors_schema.json after the introduction of TooManyFunctions in #4954

Copy link
Contributor

@chefsale chefsale left a comment

Choose a reason for hiding this comment

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

Don't have contaxt, but looks fine to me.

@matklad
Copy link
Contributor

matklad commented Oct 20, 2021

Couple of questions there:

  • should we have a test to make sure the thing stays in sync?
  • that error is part of a nightly protocol feature, if we end up removing or changing it in any way, will we be able to revert this change easily? As in, does this schema affect anything outside of nearcore which might be hard to change?

@frol
Copy link
Collaborator

frol commented Oct 20, 2021

that error is part of a nightly protocol feature, if we end up removing or changing it in any way, will we be able to revert this change easily? As in, does this schema affect anything outside of nearcore which might be hard to change?

@matklad Why that error variant is not behind a feature-flag then?

This file is used on near-api-js side to generate some "strictly typed errors"

@miraclx
Copy link
Contributor Author

miraclx commented Oct 20, 2021

  • should we have a test to make sure the thing stays in sync?

I'll add a check in CI that fails if there's a conflict in the schema

  • As in, does this schema affect anything outside of nearcore which might be hard to change?

As far as I know, it's used in https://github.com/near/near-api-js/blob/ee975a1edd84dff955302453fbcca1413d362bb7/fetch_error_schema.js#L8, but so far, it's tagged to a specific revision - 4c11499, we can revise it before updating the file on that end.

@miraclx miraclx marked this pull request as draft October 20, 2021 12:35
@matklad
Copy link
Contributor

matklad commented Oct 20, 2021

Why that error variant is not behind a feature-flag then?

The code that produces error is gated, so gating the error variant itself isn't strictly necessary. Doing that is possible, it's just requires a bunch of extra cfgs everywhere. I and @Longarithm looked at that and figured that there's little benefit in that. No strong opinion on my side here though, could add a feature-gate there.

@miraclx
Copy link
Contributor Author

miraclx commented Oct 20, 2021

Superseded by #5052

near-bulldozer bot pushed a commit that referenced this pull request Oct 21, 2021
…re flag (#5052)

Context: #5042 (comment)

Hides the `TooManyFunctions` error variant behind a feature gate to avoid it leaking into the `rpc_errors_schema.json` dump.
@miraclx miraclx closed this Oct 21, 2021
near-bulldozer bot pushed a commit that referenced this pull request Oct 22, 2021
…date (#5048)

Add CI sanity check to ensure RPC errors schema is always up-to-date

Do not merge until #5042 has either been merged or resolved, otherwise all new CI checks would fail on the unresolved conflict.
@Ekleog-NEAR Ekleog-NEAR deleted the update-errors-schema branch March 29, 2024 15:57
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.

4 participants