fix: improve error ergonomics and delegate to From()

[daniilrrr]

Ticket

• Related Linear Ticket: SEQ-156

What does this PR do?

• Summary: Improves error handling to be more granular and decoupled. From this comment

• Key Changes:

  • Function returns a generic error and the interpretation is left up to the caller.
  • Support for ? operator via From() implementations

Does this PR introduce any breaking changes (API/schema)?

• No

Do any environment variables need to be updated or added before deployment?

• No:

How can this PR be tested?

Nothing has functionally changed and unit tests pass

[linear]

SEQ-156 Handling of error cases for eth_sendRawTransaction

We need to be aware of the possible error cases for eth_sendRawTransaction. In particular, we need to know whether we should screen them out in our sequencer (e.g. transaction fragments, invalid signatures), or whether we should forward it to the contract and let op-translator ignore it.

A general heuristic is that if an error does not require knowledge of chain state, we should handle it on the sequencer side. If it does require knowledge of chain state, we should probably ignore it in op-translator (e.g. for the case of nonce reuse)

Acceptance criteria:

• Source code for error cases in geth and/or reth is located
• Complete list of errors is available
• List of errors is split into ones that require knowledge of chain state and ones that do not require knowledge of chain state

[RomanHodulak]

Hey Daniil, that's pretty cool work. This PR is definitely an improvement..

I think there are still some additional improvements we can make. Could be done separately. I left comments for reference.

[RomanHodulak]

In some of the cases, like this one, there are multiple errors grouped into one variant, here it is InvalidInput, but the variants are then only distinguished by the string message.

We need to have enum variants for every case so that every error can be matched.

[daniilrrr]

ok got it. What is the advantage of every error being able to be matched?

[daniilrrr]

My thought was that this endpoint will be called by via JSON-RPC so it would be helpful to return a string with additional detail to the caller, but classify it as the same type of error on our end like InvalidParams or InvalidRequest

[RomanHodulak]

It can be handled.

Our goal should be to have every error uniquely identifiable by a complete and finite amount of variants each easily matched by a match statement so that the higher level can decide to react to every kind of error differently.

In Rust libraries, it is often times the case that there is one top level Error for the whole library that is broken down into multiple variants, sometimes more than 1 level deep.

A downstream library can compose such errors into its own error. Say you use bcs library for decoding. Your error can look like this:

pub enum Error {
    Decoding(bcs::Error),
}

And you can look at the bcs::Error definition for more granular distinction of the error.

In this case you would also want to implement From<bcs::Error> for Error like so:

impl From<bcs::Error> for Error {
    fn from(value: bcs::Error) -> Self {
        Self::Decoding(value)
    }
}

Implementing this makes the ? operator compatible for calls that return Result<..., bcs::Error> inside function that returns Result<..., Error>, where Error is the error defined in the code snippet above.

Error message is usually a separate concern belonging to a Display implementation. The errors usually should not carry the error message as its field - due to the fact that they are uniquely identifiable, the message can be created based on that ID.

---

Having one error variant capture multiple error cases and is not broken down to individual cases by a child enum, but by a string, that error is no longer uniquely identifiable. If you would want to identify the error, you would have to match on String which has practically infinite amount of cases.

[daniilrrr]

really appreciate all the detail on this explanation, that's awesome and extremely helpful. I've implemented what I think are the changes you're describing. I left the top-level Error struct intact which has a 1:1 mapping to the RPC error codes, and then created sub enum types within those like InvalidParamsError.

if this isn't quite what you have in mind, let's pair in the morn and finish this PR off

[WillPapper]

Thank you for the detailed description @RomanHodulak! This is so helpful and very informative

[RomanHodulak]

Nice work! I appreciate the way you addressed the code review.