add custom conversion error to handle additional situations (such as optimism deposit tx)

[zobront]

Motivation

paradigmxyz/reth#8763 implements conversion of Alloy transaction to Reth transaction for Optimism deposit transactions. In the conversion process, additional errors are required.

Solution

[EDIT] Originally, this PR added new errors for the three situations that were not already covered.

After additional discussion, the PR was revised to add a single Custom(String) error type that could be used in situations that were not already covered, rather than enumerating all possibilities.

The plan is to remove the whole enum in the future. From @prestwich:

yeah, let's move this to custom and then plan to delete the whole enum when we embed the tx envelope in the rpc response

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[prestwich]

Modifying rpc types to support optimism is generally not the plan for this repo. The only concession made so far is the optional signature in transaction response. I would suggest duplicating this error enum into a separate optimism-specific repo

[zobront]

@prestwich Would you be willing to add a Custom(String) variant to ConversionError?

[prestwich]

I'm not opposed to a Custom(String), but do we actually match on or destructure this enum anywhere? 🤔

[zobront]

I've searched Reth and Alloy and only references to this enum I see are:

• Converting Transaction to Signed in Alloy: https://github.com/alloy-rs/alloy/blob/main/crates/rpc-types-eth/src/transaction/mod.rs
• Converting Alloy Transaction to Reth Transaction in Reth: https://github.com/paradigmxyz/reth/blob/main/crates/primitives/src/alloy_compat.rs

Neither matches or destructures, just references the individual elements.

Any other repos I should be checking to confirm?

[prestwich]

Any other repos I should be checking to confirm?

not that i know of

at this point i am wondering if this is an example of non-useful type safety. Why is this error anything other than a string? we use structures &'static str for transaction building

cc @mattsse

[zobront]

From what I've seen, I agree with this perspective.

[mattsse]

these are OP specific ones, I don't like this because this OP specific and should not be included here

we should either add a custom(String) or Box

[prestwich]

would love you feedback about the discussion in the comments

[mattsse]

at this point i am wondering if this is an example of non-useful type safety.

yeah this can fail for a lot for reasons, and I guess it would be more convenient to just return a message, I doubt matching these variants is very useful. but we have a lot of reasonable ones already and I'd like to keep those and instead add an Other(String) variant

[prestwich]

yeah, let's move this to custom and then plan to delete the whole enum when we embed the tx envelope in the rpc response

[zobront]

@prestwich Updated based on conversation above, and adjusted the PR title & description to match.