Change ExecError related functions to be associated-fns

[rrtoledo]

Description

This PR attempts to associate the function get_step_reported_error to ExecError.

Issue Link

Resolves #1181

Type of change

• [x ] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Contents

Rationale

We wanted to move the function get_step_reported_error to an imp ExecError, I also moved the part related to OogError to its own associated function From<OpCodeId> for OogError. I changed an if/else block into a match expecting to handle more errors for CREATE/2.

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From<GethExecStep> for ExecError but the return type should be changed to Option<ExecError> since the step may not have an error (step.error is optional).

How Has This Been Tested?

No tests were added as no new functionalities were added.

[ed255]

LGTM!

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From for ExecError but the return type should be changed to Option since the step may not have an error (step.error is optional).

This sounds great to me! Since this is a small PR and the update improvement is also small I would suggest that you implement this approach in this same PR :)

I will review again after the update to submit my approve.

[rrtoledo]

LGTM!

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From for ExecError but the return type should be changed to Option since the step may not have an error (step.error is optional).

This sounds great to me! Since this is a small PR and the update improvement is also small I would suggest that you implement this approach in this same PR :)

I will review again after the update to submit my approve.

@ed255 I couldn't implement the From for the Option for Option, the options were disturbing the compiler, so I implemented without option and with panic instead. Have a look!

[ed255]

LGTM!

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From for ExecError but the return type should be changed to Option since the step may not have an error (step.error is optional).

This sounds great to me! Since this is a small PR and the update improvement is also small I would suggest that you implement this approach in this same PR :)
I will review again after the update to submit my approve.

@ed255 I couldn't implement the From for the Option for Option, the options were disturbing the compiler, so I implemented without option and with panic instead. Have a look!

Ah, Rust doesn't allow you to do that in this module, it would allow you to implement the From for Option<ExecError> in the same module where GethExecStep is defined (which is in eth-types), but that mean implementing code where it doesn't belong.

The Rust std has another trait that may be more appropriate for this case: https://doc.rust-lang.org/std/convert/trait.TryFrom.html
You could implement TryFrom<&GethExecStep> for ExecError. It requires an error type, which could just be () because here we don't care about it. With this the change in mapping/src/circuit_input_builder/input_state_ref.rs could even look better:

        if let Ok(error) = ExecError::try_from(step) {
            return Ok(Some(error);
        }

With no unwrap needed in the from implementation: step.error.clone().unwrap(). The panic!("Unknown GethExecStep.error: {}", error) could be kept because it can help us detect an unexpected value returned by geth that we don't handle.

Edit: The above comment is a nitpick, I think the current approach looks good already! So take this as a suggestion and feel free to ignore it ;)

[rrtoledo]

LGTM!

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From for ExecError but the return type should be changed to Option since the step may not have an error (step.error is optional).

This sounds great to me! Since this is a small PR and the update improvement is also small I would suggest that you implement this approach in this same PR :)
I will review again after the update to submit my approve.

@ed255 I couldn't implement the From for the Option for Option, the options were disturbing the compiler, so I implemented without option and with panic instead. Have a look!

Ah, Rust doesn't allow you to do that in this module, it would allow you to implement the From for Option<ExecError> in the same module where GethExecStep is defined (which is in eth-types), but that mean implementing code where it doesn't belong.

The Rust std has another trait that may be more appropriate for this case: https://doc.rust-lang.org/std/convert/trait.TryFrom.html You could implement TryFrom<&GethExecStep> for ExecError. It requires an error type, which could just be () because here we don't care about it. With this the change in mapping/src/circuit_input_builder/input_state_ref.rs could even look better:

        if let Ok(error) = ExecError::try_from(step) {
            return Ok(Some(error);
        }

With no unwrap needed in the from implementation: step.error.clone().unwrap(). The panic!("Unknown GethExecStep.error: {}", error) could be kept because it can help us detect an unexpected value returned by geth that we don't handle.

Edit: The above comment is a nitpick, I think the current approach looks good already! So take this as a suggestion and feel free to ignore it ;)

Thanks for all the info, it is very helpful (I need these remarks)! I'll try your suggestions after I'm done with #1111. :)

[rrtoledo]

``The compiler complains if I remove the unwrap as `as_str()` is not implemented for `Option<>`, so it recommends me to write an `expect()` instead.

LGTM!

Personally I would update the function get_step_reported_error to take as input an GethExecStep and in that case directly implement it as a From for ExecError but the return type should be changed to Option since the step may not have an error (step.error is optional).

This sounds great to me! Since this is a small PR and the update improvement is also small I would suggest that you implement this approach in this same PR :)
I will review again after the update to submit my approve.

@ed255 I couldn't implement the From for the Option for Option, the options were disturbing the compiler, so I implemented without option and with panic instead. Have a look!

Ah, Rust doesn't allow you to do that in this module, it would allow you to implement the From for Option<ExecError> in the same module where GethExecStep is defined (which is in eth-types), but that mean implementing code where it doesn't belong.

The Rust std has another trait that may be more appropriate for this case: https://doc.rust-lang.org/std/convert/trait.TryFrom.html You could implement TryFrom<&GethExecStep> for ExecError. It requires an error type, which could just be () because here we don't care about it. With this the change in mapping/src/circuit_input_builder/input_state_ref.rs could even look better:

        if let Ok(error) = ExecError::try_from(step) {
            return Ok(Some(error);
        }

With no unwrap needed in the from implementation: step.error.clone().unwrap(). The panic!("Unknown GethExecStep.error: {}", error) could be kept because it can help us detect an unexpected value returned by geth that we don't handle.

Edit: The above comment is a nitpick, I think the current approach looks good already! So take this as a suggestion and feel free to ignore it ;)

The compiler complains when I remove unwrap() as I am using as_str() (needed to compare with the constant errors). It recommends me to use an expect() instead.

Another thing I tried while playing around was using an ok_or() instead. See :

impl TryFrom<&GethExecStep> for ExecError {
    type Error = ();

    fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
        step.error
            .clone()
            .ok_or(())
            .map(|error| match error.as_str() {...}

[ed255]

The compiler complains when I remove unwrap() as I am using as_str() (needed to compare with the constant errors). It recommends me to use an expect() instead.

Another thing I tried while playing around was using an ok_or() instead. See :

impl TryFrom<&GethExecStep> for ExecError {
    type Error = ();

    fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
        step.error
            .clone()
            .ok_or(())
            .map(|error| match error.as_str() {...}

How about something like this?

It converts the Option<String> to Result<String,()> with ok_or() (using a reference instead of cloning), and then with ? it returns if it's Error (the Option was None), or evaluates to the Ok value if it's Ok (the Option was Some)

impl TryFrom<&GethExecStep> for ExecError {
    type Error = ();

    fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
        // let error: String = step.error.clone().unwrap();
        Ok(match step.error.as_ref().ok_or(())?.as_str() {
            GETH_ERR_OUT_OF_GAS | GETH_ERR_GAS_UINT_OVERFLOW => {
                // NOTE: We report a GasUintOverflow error as an OutOfGas error
                let oog_err = OogError::from(&step.op);
                ExecError::OutOfGas(oog_err)
            }
            error => {
                if error.starts_with(GETH_ERR_STACK_OVERFLOW) {
                    ExecError::StackOverflow
                } else if error.starts_with(GETH_ERR_STACK_UNDERFLOW) {
                    ExecError::StackUnderflow
                } else {
                    panic!("Unknown GethExecStep.error: {}", error);
                }
            }
        })
    }
}

[rrtoledo]

The compiler complains when I remove unwrap() as I am using as_str() (needed to compare with the constant errors). It recommends me to use an expect() instead.
Another thing I tried while playing around was using an ok_or() instead. See :

impl TryFrom<&GethExecStep> for ExecError {
    type Error = ();

    fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
        step.error
            .clone()
            .ok_or(())
            .map(|error| match error.as_str() {...}

How about something like this?

It converts the Option<String> to Result<String,()> with ok_or() (using a reference instead of cloning), and then with ? it returns if it's Error (the Option was None), or evaluates to the Ok value if it's Ok (the Option was Some)

impl TryFrom<&GethExecStep> for ExecError {
    type Error = ();

    fn try_from(step: &GethExecStep) -> Result<Self, Self::Error> {
        // let error: String = step.error.clone().unwrap();
        Ok(match step.error.as_ref().ok_or(())?.as_str() {
            GETH_ERR_OUT_OF_GAS | GETH_ERR_GAS_UINT_OVERFLOW => {
                // NOTE: We report a GasUintOverflow error as an OutOfGas error
                let oog_err = OogError::from(&step.op);
                ExecError::OutOfGas(oog_err)
            }
            error => {
                if error.starts_with(GETH_ERR_STACK_OVERFLOW) {
                    ExecError::StackOverflow
                } else if error.starts_with(GETH_ERR_STACK_UNDERFLOW) {
                    ExecError::StackUnderflow
                } else {
                    panic!("Unknown GethExecStep.error: {}", error);
                }
            }
        })
    }
}

LGTM! I just pushed it :)

[ed255]

LGTM! This looks nicer now :D

[han0110]

Besides from the leftover Edu mentioned, other LGTM!