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.

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

Type-safe redesign #29

Open
michaelsproul opened this issue Feb 2, 2025 · 0 comments
Open

Type-safe redesign #29

michaelsproul opened this issue Feb 2, 2025 · 0 comments

Comments

@michaelsproul
Copy link
Member

I started thinking about redesigning the TreeHash trait while working on this PR to fix some broken implementations:

The current trait is arguably dangerous to use, due to the prevalence of panicky methods:

impl<N: Unsigned + Clone> TreeHash for Bitfield<Variable<N>> {
fn tree_hash_type() -> TreeHashType {
TreeHashType::List
}
fn tree_hash_packed_encoding(&self) -> PackedEncoding {
unreachable!("List should never be packed.")
}
fn tree_hash_packing_factor() -> usize {
unreachable!("List should never be packed.")
}

This is due to the duplication of packing-related information across multiple methods. If we allow ourselves a breaking change, we could extend TreeHashType with the packing factor, and combine tree_hash_type and tree_hash_packing_factor into a single method:

pub enum TreeHashType {
    Basic { packing_factor: usize },
    Vector,
    List,
    Container,
}

pub trait TreeHash {
    fn tree_hash_type() -> TreeHashType;

    fn tree_hash_packed_encoding(&self) -> PackedEncoding;

    fn tree_hash_root(&self) -> Hash256;
}

This makes invalid states (like TreeHashType::Vector and a call to tree_hash_packing_factor) unrepresentable, which is great. However we still have the issue of the tree_hash_packed_encoding method. It is not well-defined for types other than Basic ones.

Perhaps the simplest option would be to make tree_hash_packed_encoding return a Result<PackedEncoding>, and error for non-basic types.

A more "techy" option would be to include some opaque struct to act as a token for unlocking the tree_hash_packed_encoding method. Something like:

/// This struct can't be constructed except by the library.
pub struct PackedEncodingToken<T> {
    _phantom: PhantomData<T>,
}

pub enum TreeHashType<T> {
    Basic {
        packing_factor: usize,
        token: PackedEncodingToken<T>,
    },
    Vector,
    List,
    Container,
}

pub trait TreeHash {
    fn tree_hash_type() -> TreeHashType<Self>;

    fn tree_hash_packed_encoding(&self, token: PackedEncodingToken<Self>) -> PackedEncoding;

    fn tree_hash_root(&self) -> Hash256;
}

This design prevents downstream users from calling tree_hash_packed_encoding unless they've obtained a PackedEncodingToken from calling tree_hash_type. It does not prevent library maintainers from adding incorrect implementations of tree_hash_packed_encoding, but would prevent those implementations from being reachable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant