Move BoxError downcast out of impl From<BoxError> for TransactionError

[arya2]

Motivation

It would be better for the compiler to complain about a mismatch between BoxError / TransactionError when using the ? operator, and to perform the conversions where the type of BoxError is more explicitly known.

TransactionError is currently the only error type with a From impl handling downcasts.

This could be used in the block verifier for VerifyBlockError::ValidateProposal as well if defined in a trait.

Possible Design

impl TransactionError {
    pub(crate) fn downcast_from<ErrorType>(mut err: BoxError) -> Self
    where
        ErrorType: std::error::Error + Into<TransactionError>,
    {
        err.downcast::<ErrorType>()
            .map(|e| Self::from(*e))
            .or_else(Self::internal_downcast_err)
    }
}

Related Work

• Dedupe Verification error types (Block, Chain) #1186

[teor2345]

These downcasts are located in the same place, so we don't accidentally implement conflicting downcasts. Moving the downcasts out of the conversion could make the code harder to maintain, and create more bugs.

It would be better for the compiler to complain when there isn't a valid conversion for a particular type of error and to perform the conversions where the type of BoxError is more explicitly known.

Unfortunately, it is impossible to know the type of many errors, because tower::Buffer converts all service readiness and response errors to BoxError:
https://docs.rs/tower/latest/tower/buffer/struct.Buffer.html#impl-Service%3CRequest%3E

Before we can prioritise or estimate this ticket, it will need a specific design that takes these issues into account.

[arya2]

Unfortunately, it is impossible to know the type of many errors, because tower::Buffer converts all service readiness and response errors to BoxError

Ah, I see. Still, it might be a good idea to do the conversion outside a From impl so it's more obvious where the conversions are happening. I added a possible design.

[teor2345]

It would be better for the compiler to complain when there isn't a valid conversion for a particular type of error

I don't think the design in the ticket does this?

[arya2]

It would be better for the compiler to complain when there isn't a valid conversion for a particular type of error

I don't think the design in the ticket does this?

My wording there may have been a bit vague, it's more to disallow using ? or into() without explicitly defining how the BoxError should be converted into a TransactionError so it's more obvious when a new variant needs to be added to TransactionError.

It might also be nice to annotate where each type is expected.

[teor2345]

It might also be nice to annotate where each type is expected.

I think we can use concrete types, rather than comments, so the compiler can check them.

I've added this as a TODO in the code, so we do it when we're rewriting that error handling.