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

New error handling design #507

Closed
wants to merge 21 commits into from
Closed

New error handling design #507

wants to merge 21 commits into from

Conversation

volovyks
Copy link
Collaborator

@volovyks volovyks commented Feb 18, 2021

  • message and type became mandatory in TypedError. It should prevent us from creating UntypedErrors, and errors without description in the future.
  • errorPath added to all ServerErrros's as a part of errorContext.
  • error context moved to ServerError since it's ServerError specific.
  • typed errors deleted

Breaking Changes:

  • TypedError.message became mandatory field
  • TypedError.type became mandatory field
  • context moved from TypedError to ServerError
  • Transaction errors changed their type from X_Error to ServerError

TypedError is exported as a part of near-api-js interface, but I don't think that it's intended to be used in other projects.

@volovyks volovyks marked this pull request as ready for review March 21, 2021 19:48
@volovyks volovyks requested a review from vgrichina as a code owner March 21, 2021 19:48
@volovyks volovyks self-assigned this Mar 21, 2021
@volovyks volovyks added the devx label Mar 21, 2021
@volovyks volovyks requested review from chadoh and mikedotexe March 22, 2021 20:11
@volovyks volovyks marked this pull request as draft March 24, 2021 15:46
@volovyks volovyks marked this pull request as ready for review March 25, 2021 02:30
@volovyks volovyks linked an issue Mar 25, 2021 that may be closed by this pull request
@volovyks volovyks requested review from frol and khorolets March 25, 2021 13:08
src/utils/errors.ts Show resolved Hide resolved
Comment on lines 21 to 23
export class ServerErrorContext {
transactionHash?: string;
errorPath?: Record<string, any>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

transactionHash and errorPath are only relevant to the broadcast_* and tx (EXPERIMENTAL_tx_status) endpoints, so I would call it TransactionExecutionFailureContext, and make both of the fields required. I guess errorPath can be renamed to transactionExecutionError (it will have either ActionError or InvalidTxError)

src/account.ts Outdated Show resolved Hide resolved
src/utils/rpc_errors.ts Outdated Show resolved Hide resolved
src/utils/rpc_errors.ts Outdated Show resolved Hide resolved
@volovyks volovyks requested a review from frol March 29, 2021 09:08
@volovyks volovyks marked this pull request as draft March 29, 2021 13:40
@volovyks
Copy link
Collaborator Author

I Will request feedback after these changes: #515 (comment)

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this will be closed in another 7 days.

@volovyks volovyks removed the Stale label Apr 29, 2021
@mikedotexe
Copy link
Contributor

Update: Serhii will break out some code into separate PRs.

@volovyks
Copy link
Collaborator Author

This fix requires too many breaking changes that are hard to introduce properly. Some good parts will be cherry-picked in later PR's.

@volovyks volovyks closed this May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants