Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Add error info for getKeyringForAccount #87

Merged
merged 3 commits into from
May 4, 2021

Conversation

andrepimenta
Copy link
Member

@andrepimenta andrepimenta commented Apr 26, 2021

The mobile team has seen multiple errors coming from "No keyring found for the requested account." so this PR adds more info to that error in order to help diagnosing the problem.
Sentry error: https://sentry.io/organizations/metamask/issues/2044540268/?project=2299799&referrer=github_integration
Mobile issue: MetaMask/metamask-mobile#2497

Resolves #86

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

The text "No winners" and "No candidates" might end up being more confusing than helpful 🤔 Those terms don't have any meaning to consumers of this library. Maybe it would be more helpful to express these failure cases as three separate errors?

I see three separate scenarios:

  • The address passed in is invalid/empty
  • There are no keyrings
  • There are keyrings, but none match the address.

@andrepimenta
Copy link
Member Author

The text "No winners" and "No candidates" might end up being more confusing than helpful 🤔 Those terms don't have any meaning to consumers of this library. Maybe it would be more helpful to express these failure cases as three separate errors?

I see three separate scenarios:

  • The address passed in is invalid/empty
  • There are no keyrings
  • There are keyrings, but none match the address.

I agree 100%! Just fixed, thank you!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrepimenta andrepimenta merged commit f3adfaf into main May 4, 2021
@andrepimenta andrepimenta deleted the add-error-info-get-keyring branch May 4, 2021 16:29
@andrepimenta andrepimenta mentioned this pull request May 4, 2021
@cppfuns
Copy link

cppfuns commented Sep 30, 2021

Encountered related problems, please take a look at this submission can solve the problem I encountered
MetaMask/metamask-mobile#2497 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error messages
3 participants