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

getCustomErrorMessage: Keep unknown errors. #34

Closed
wants to merge 1 commit into from

Conversation

ilya-bobyr
Copy link

While null is more indicative, it seems more practical to return the original error. The most common use case is to show the error to the user. Returning null either loses useful information, or makes the usage more cumbersome, requiring additional checks at each call site.

While `null` is more indicative, it seems more practical to return the
original error.  The most common use case is to show the error to the
user.  Returning `null` either loses useful information, or makes the
usage more cumbersome, requiring additional checks at each call site.
@mikemaccana
Copy link
Collaborator

My concern is that this is misleading: the function has not been able to find a custom error message. The cost of having someone check for a custom error message is preferable over a function that fails silently, returning what seems like a custom error message when it has not been able to find one.

@mikemaccana mikemaccana closed this Jun 5, 2024
@ilya-bobyr
Copy link
Author

Look at how you use the function yourself:

      const customErrorMessage = getCustomErrorMessage(
        systemProgramErrors,
        error
      );
      throw new Error(customErrorMessage);

https://github.com/solana-developers/professional-education/blame/8904cfd94eaf467bbf77a44cec61f8f5a44110bb/labs/favorites/tests/favorites.ts#L64-L68

If the custom error is not found, you just get null thrown, with both the original error and the stack trace lost.
And in the bootcamp class, students were confused by this situation - this is why I started looking into it in the first place.

I think, in most cases, the fact that the custom error was not found is less important.
All these errors are just test messages.
And the most likely case it that they are going to be output for a user to see.

You can still chain multiple custom error transformers, if your transaction calls more than one program.

I do agree that there could be a case when someone really cares to check if the search was successful.
But it seems to be a less likely case.

There are several alternative solutions available, should the need arise.
One might want to do either of the following:

  • Write an alternative function that does return null.
  • Just check if the returned value is different.
  • Write a function that is more specialized to a particular case.
    For example, throw the modified error, rather than returning it as a string.

In either case, my point is that it is more helpful to cover the more common case first.

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.

None yet

2 participants