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: ExecutionOutcome.status should be public #288

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robert-zaremba
Copy link

@robert-zaremba robert-zaremba commented Aug 10, 2023

Today, the only way to check the transaction error content is to serialize it with JSON or format it with default display formatter.
Moreover it's hard to check an error in a particular receipt.
Example: https://github.com/near-ndc/voting-v1/blob/8f1c14a/elections/tests/iah.rs#L169

Potential workaround would be to make Error.repr public. But the flow will be even more complicated:

    let failures = res.receipt_failures();
    for f in failures {
        let err = f.into_result().err().unwrap();
        match err.repr { // <-- Doesn't work because repr is private
            // moreover the following is too complicated: requires the knowledge of ErrorRepr
            ErrorRepr::Custom(kind, error) => { /* match and check erro, which is a Box */ }
            _ ....
        };
    }

@ChaoticTempest
Copy link
Member

We didn't expose status since the errors coming from nearcore are very likely to change until they are stabilized with near-primitives version 1.0. This increases the likelihood of breaking changes when it comes time to upgrade dependencies. It also increases the churn when it comes to having someone update their workspaces version. Alternatively, if you want to match against the errors, you can call downcast directly on the errors to get the specific near_primitives error you want

@robert-zaremba
Copy link
Author

This increases the likelihood of breaking changes when it comes time to upgrade dependencies.

But the breaking changes are only related to tests.
I strongly prefer to have such a breaking change rather than formatting the outcomes to string and then doing string pattern matching. Moreover, if most likely "that" breaking change will also break existing flow which relay on the string formatting).
Exposing the status today won't break anything.

If you are strongly against it, then could you propose an alternative (other than formatting into a string)

@robert-zaremba
Copy link
Author

UP, any chance we can merge this?

@frol
Copy link
Collaborator

frol commented Aug 28, 2023

@robert-zaremba Have you tried downcast_ref as @ChaoticTempest suggested?

if let Some(err) = f.into_result().unwrap_err().source().unwrap().downcast_ref::<near_primitives::errors::TxExecutionError>() {
    dbg!(err);
}

@frol
Copy link
Collaborator

frol commented Sep 9, 2023

@robert-zaremba Did my snippet help? Should we close this for now (until we are ready to create an API-stable version of the outcome status)?

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.

3 participants