Set RPC echo check logic as trait default implementation

[gabrielbosio]

Closes #2276.

[gabrielbosio]

Keeping this PR as draft until we complete the rest of the sub issues of #2241

[SantiagoPittella]

It looks good, I don't love the idea of the _unchecked methods but probably is for the best at least for now. We can keep track of this on a separate issue.

I guess another options could be to have a wrapper struct and keep NodeRpcClient as pure transport. Add a wrapper that owns verification:

pub struct VerifyingRpcClient<T>(T);

impl<T: NodeRpcClient> NodeRpcClient for VerifyingRpcClient<T> {
    async fn get_block_header_by_number(&self, block_num, include_mmr_proof)
        -> Result<(BlockHeader, Option<MmrProof>), RpcError>
    {
        let (h, p) = self.0.get_block_header_by_number(block_num, include_mmr_proof).await?;
        verify_block_num(block_num, h.block_num())?;
        Ok((h, p))
    }
    // ...
}

Or something like using two traits, one for raw impl and another for validations:

// What implementors write
pub trait RpcTransport: Send + Sync {
    async fn get_block_header_by_number(&self, ..) -> Result<..>;   // raw
    // ...
}

// What callers use.
pub trait NodeRpcClient: Send + Sync {
    async fn get_block_header_by_number(&self, ..) -> Result<..>;   // verified
    // ...
}

impl<T: RpcTransport> NodeRpcClient for T {
    async fn get_block_header_by_number(&self, block_num, mmr) -> Result<..> {
        let (h, p) = RpcTransport::get_block_header_by_number(self, block_num, mmr).await?;
        verify_block_num(block_num, h.block_num())?;
        Ok((h, p))
    }
    // ...
}

[igamigo]

It looks good, I don't love the idea of the _unchecked methods but probably is for the best at least for now. We can keep track of this on a separate issue.

I guess another options could be to have a wrapper struct and keep NodeRpcClient as pure transport. Add a wrapper that owns verification:

Or something like using two traits, one for raw impl and another for validations:

Yes, the first one of these approaches is pretty much what I was envisioning when we first discussed this feature. It's a bit boilerplatey, but should work out fine and be pretty flexible. The second one also sounds good, and we could also seal the trait. For names we could also use fetch_* or send_*_req, etc.