improve substrate-compat

[yjhmelody]

Should impl all header/hasher types defined in substrate or other side for subxt related traits.

And we should support Into<u64> for Substrate BlockNumber directly. See paritytech/polkadot-sdk#2334

[yjhmelody]

We could remove this once upstream merged.

[niklasad1]

This essentially breaks things and only will work with block number types that implements Into<u64>?!

I don't understand the benefit with this except simplifying the trait a bit?
Can you explain please?

However, I think the hashing stuff you added below simplifies things

[yjhmelody]

See subxt Header:

/// This represents the block header type used by a node.
pub trait Header: Sized + Encode + Decode {
    /// The block number type for this header.
    type Number: Into<u64>;
    /// The hasher used to hash this header.
    type Hasher: Hasher;

    /// Return the block number of this header.
    fn number(&self) -> Self::Number;

    /// Hash this header.
    fn hash(&self) -> <Self::Hasher as Hasher>::Output {
        Self::Hasher::hash_of(self)
    }
}

[yjhmelody]

I also do not like the Into<u64>, but subxt simplify related type defintions.

[niklasad1]

Ok, I see it's probably better to change the number type on the header trait then.

/cc @lexnv @jsdw thoughts?

[jsdw]

I don't think that we should have a generic impl<T: sp_runtime::traits::Header> Header for T impl behind substrate-compat, because it would mean that enabling substrate-compat would potentially lead to breakages, ie it would prevent any other Header impls from being valid.

Edit: I tested this and it worked ok actually :)

[jsdw]

I also do not like the Into, but subxt simplify related type defintions.

Ok, I see it's probably better to change the number type on the header trait then.

Maybe I'm missing something; what would you propose changing about the number type? Into<u64> is just to allow us to return some valid numbers and compare them etc (it was a simplification over requiring a bunch of bounds, really; easier to just ask for a number, and prob it is always u32/u64 anyway?).

I'm open to simplifying some bounds here if you find any that aren't needed though :)

[jsdw]

Ok, so I thought that a general impl<T: sp_runtime::traits::Hash> Hasher for T {type impl would be an issue, but it does compile OK, and I did try writing an example with a custom type and could still implement Header for that with substrate-compat enabled, so maybe Rust is being more lenient than I thought it would be. I'll have to check further, but if it's actually happy with the generic impls + custom ones then so am I!

[jsdw]

Ok, so contrary to my understanding of how trait coherence worked, these generic impls seem to work quite well, and they def simplify the code here.

The only disadvantage really is that enabling the "substrate-compat" feature flag can result in compile errors if you've already implemented the relevant traits yourself on some type, but this feels like quite a niche thing.

If people do report issues then we can revert to defining a Compat<T> struct that impls the relevant traits, which would be 100% non-breaking but a touch more tedious to use too (you need to wrap substrate types in it in the config and unwrap if accessing).

So I'm ok with accepting this!