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: handle MethodNotFound errors in JsonRpcProvider #1028

Closed
wants to merge 12 commits into from

Conversation

andy-haynes
Copy link
Collaborator

@andy-haynes andy-haynes commented Nov 10, 2022

Motivation

Currently JsonRpcProvider does not do any error parsing when the error message is on the result key (e.g. response.result rather than the expected response.error). These raw errors are not intuitive to users.

Description

This PR checks for string values of result.error on the RPC response and throws an exception if it matches a known pattern for an RPC error using the existing getErrorTypeFromErrorMessage method. This makes it more consistent with the error handling performed for structured errors.

I don't currently know which other RPC endpoints return errors in this way but if it is possible to enumerate them we can ensure they have the same coverage here.

Before:
image

After:
image

Checklist

  • Read the contributing guidelines
  • Commit messages follow the conventional commits spec
  • Performed a self-review of the PR
  • Added automated tests
  • Manually tested the change

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2022

⚠️ No Changeset found

Latest commit: 9f6fa84

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

@idea404 idea404 left a comment

Choose a reason for hiding this comment

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

Good first step to reducing vagueness in these messages
Will keep an eye out for dev feedback on this going forward in NAJ and WS-JS

@idea404
Copy link
Contributor

idea404 commented Nov 14, 2022

Add to CI as well if error_messsages.json is up to date according to nearcore dump
refer to this file on error type matching logic

@idea404 idea404 self-assigned this Nov 15, 2022
@idea404 idea404 added bug Something isn't working enhancement New feature or request labels Nov 15, 2022
@idea404
Copy link
Contributor

idea404 commented Nov 16, 2022

I'm puzzled as to why tests fail now for test/account_multisig.test.js despite there being no changes there

@denbite
Copy link
Contributor

denbite commented Jan 21, 2024

The idea of having understandable error messages looks great to me and was implemented in #1275 based on the latest version of the library and without merge conflicts

I'd suggest closing this one so we don't leave it out of date

@vikinatora
Copy link
Collaborator

vikinatora commented Feb 8, 2024

Closing as it's been resolved by #1275

@vikinatora vikinatora closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants