Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve substrate-compat #1265

Merged
merged 6 commits into from
Nov 24, 2023
Merged

improve substrate-compat #1265

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
628800c
improve `substrate-compat`
yjhmelody Nov 15, 2023
2d47656
Merge branch 'master' into improve-substrate-compact
yjhmelody Nov 17, 2023
148f648
Merge branch 'master' into improve-substrate-compact
jsdw Nov 21, 2023
dce54bb
From => Into
jsdw Nov 21, 2023
79d5f79
Merge branch 'master' into improve-substrate-compact
jsdw Nov 23, 2023
b818646
Merge branch 'master' into improve-substrate-compact
jsdw Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 8 additions & 35 deletions subxt/src/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,51 +129,24 @@ pub trait Header: Sized + Encode + Decode {
#[cfg(feature = "substrate-compat")]
mod substrate_impls {
use super::*;
use primitive_types::{H256, U256};

impl<N, H> Header for sp_runtime::generic::Header<N, H>
impl<T: sp_runtime::traits::Header> Header for T
where
Self: Encode + Decode,
N: Copy + Into<U256> + Into<u64> + TryFrom<U256>,
H: sp_runtime::traits::Hash + Hasher,
u64: From<<T as sp_runtime::traits::Header>::Number>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove this once upstream merged.

Copy link
Member

@niklasad1 niklasad1 Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/cc @lexnv @jsdw thoughts?

Copy link
Collaborator

@jsdw jsdw Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator

@jsdw jsdw Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marked this conversation as resolved.
Show resolved Hide resolved
{
type Number = N;
type Hasher = H;
type Number = T::Number;
type Hasher = T::Hashing;

fn number(&self) -> Self::Number {
self.number
*self.number()
}
}

impl Hasher for sp_core::Blake2Hasher {
type Output = H256;
impl<T: sp_runtime::traits::Hash> Hasher for T {
jsdw marked this conversation as resolved.
Show resolved Hide resolved
type Output = T::Output;

fn hash(s: &[u8]) -> Self::Output {
<Self as sp_core::Hasher>::hash(s)
}
}

impl Hasher for sp_runtime::traits::BlakeTwo256 {
type Output = H256;

fn hash(s: &[u8]) -> Self::Output {
<Self as sp_core::Hasher>::hash(s)
}
}

impl Hasher for sp_core::KeccakHasher {
type Output = H256;

fn hash(s: &[u8]) -> Self::Output {
<Self as sp_core::Hasher>::hash(s)
}
}

impl Hasher for sp_runtime::traits::Keccak256 {
type Output = H256;

fn hash(s: &[u8]) -> Self::Output {
<Self as sp_core::Hasher>::hash(s)
<T as sp_runtime::traits::Hash>::hash(s)
}
}
}
Loading