feat: application level errors

[SantiagoPittella]

closes #1106

[SantiagoPittella]

@Mirko-von-Leipzig @bobbinth in this draft, besides creating all the enums, I added this errors in the SyncNullifiers endpoint as an example, where I created an internal error enum that is then converted into a tonic::Status struct by passing through the SyncNullifiersGrpcError, as we do in the block producer's (SubmitProvenBatch & SubmitProvenTransaction).

Additionally, should I create a trait for the methods implemented by the *GrpcError enums? That way we could implement at least one default implementation easily (tonic_code).

[Mirko-von-Leipzig]

I think this largely makes sense

[Mirko-von-Leipzig]

Should we create a macro_rules to create these enums?

grpc_error!(
    SyncNullifiersGrpcError {
        InvalidBlockRange,
        InvalidPrefixLength,
    }
);

which generates

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[repr(u8)]
pub enum SyncNullifiersGrpcError {
    Internal = 0,
    DeserializationFailed = 1,
    InvalidBlockRange = 2,
    InvalidPrefixLength = 3,
}

impl GrpcError for SyncNullifiersGrpcError {
    fn api_code(self) -> u8 {
        self as u8
    }

    fn is_internal(&self) -> bool {
        self == &Self::Internal
    }
}

[SantiagoPittella]

Due to the variable length of variants for each enum, I kept the manual enum definition and added the macro for the trait implementation. This is what I did first:

#[allow(unused_macros)]
macro_rules! grpc_error {
    ($error:ident {
        $($field:ident),* $(,)?
    }) => {
        grpc_error!(@build_enum $error, 2, $($field),*);
    };
    
    // Build the enum with proper discriminant assignment
    (@build_enum $error:ident, $next:expr, $field:ident) => {
        #[derive(Debug, Clone, Copy, PartialEq, Eq)]
        #[repr(u8)]
        pub enum $error {
            // Internal Server Error
            Internal = 0,
            // Malformed data
            DeserializationFailed = 1,
            $field = $next,
        }

        impl GrpcError for $error {
            fn api_code(self) -> u8 {
                self as u8
            }

            fn is_internal(&self) -> bool {
                self == &Self::Internal
            }
        }
    };
    
    (@build_enum $error:ident, $next:expr, $field1:ident, $field2:ident) => {
        #[derive(Debug, Clone, Copy, PartialEq, Eq)]
        #[repr(u8)]
        pub enum $error {
            // Internal Server Error
            Internal = 0,
            // Malformed data
            DeserializationFailed = 1,
            $field1 = $next,
            $field2 = $next + 1,
        }

        impl GrpcError for $error {
            fn api_code(self) -> u8 {
                self as u8
            }

            fn is_internal(&self) -> bool {
                self == &Self::Internal
            }
        }
    };

And so on for more variants

[Mirko-von-Leipzig]

It would be nice if we could ensure we are doing this sort of error handling correctly for the user-facing RPC endpoints.

This would require some further thought, but basically instead of a Result<T, tonic::Status> we would want Result<T, E> where E is our own error type that then gets converted to a Status. Doing this cleanly isn't straight-forward though.

Also ideal would be if this error handling was only done at the RPC component since that's the "user" access point. But I think we have to do it in the store and block-producer since they're the only ones with full error info.

This isn't really a concern for this PR; more just musings on my part (so you can ignore this).

[Mirko-von-Leipzig]

Should this be an error? Or can we just ignore this e.g. in Rust its just an empty range/iterator.

(5..0).len() == 0

[Mirko-von-Leipzig]

This could be a method on GrpcError I think?

[SantiagoPittella]

Hmm not sure, GrpcError is implemented for this new *GrpcErrors. While into_tonic_status is implemented for the higher-level errors, using the value.as_report() to enrich the status

[Mirko-von-Leipzig]

Do we need to export this? Its internal to the crate iiuc. Though I always gets confused as to whether export means pub or just that its usable outside the module.

[SantiagoPittella]

The trait methods defined here are called from the store and block-producer crates, inside each domain error Into<tonic::Status> implementation, so we need to export it

[Mirko-von-Leipzig]

Should the error handling doc go into the proto/readme.md since that is the formal schema?

[Mirko-von-Leipzig]

As a meta note, I feel like it makes more sense for our internal crates to reference the schema docs rather than having their own?

[SantiagoPittella]

For the second comment, should this be done for the endpoints as a whole? or just for the errors?

[sergerad]

Why couldn't we use the macro for this one? Anything we could change to be able to use it?

[SantiagoPittella]

Did it an applied it in almost all the error, I could not use it in the AddTransactionError since we look for the internal variants in VerifyTxError. To use it there, we should flatten the variants of that error

[sergerad]

Right, so the problem is here

#[derive(Debug, Error)]
pub enum AddTransactionError {
    #[error("transaction verification failed")]
    VerificationFailed(#[from] VerifyTxError),

I guess its not ideal that in order to use the macro we need flat error enums. I wonder how complex it would be to support the nesting in the macro. Probably be pretty unwieldy.

If it unblocks using the macro for such errors, I would opt to not worry about internal variants and just map them all 1-to-1. But I don't know if that unblocks anything.

[SantiagoPittella]

Since with all other error enums we don´t have this problem of nested error where some variants are internal errors, I think that defining the explicit conversions only for this exceptions (only 1, this one ATM) is fine. We can track if this situation changes and more complex variants are added before changing the approach, wdyt?

[drahnr]

+1 to flattening.

[SantiagoPittella]

double checking, you mean to flatten the variants from VerifyTxError into AddTransactionError, right? @drahnr

[sergerad]

Yea I do think that is probably a reasonable requirement - any error returned via gRPC needs to be flattened.

[drahnr]

Looks good to me, mid-term we should discuss how we can trim the manual labor to a minum, i.e. vie exposing more errors directly, or with minimal translation (except for anything DB or secretes related).