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

Make Bytes map to Bytes in SolType #545

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

rachel-bousfield
Copy link
Contributor

@rachel-bousfield rachel-bousfield commented Feb 29, 2024

Motivation

alloy-sol-types exposes a sol_data::T variant for each of the primitive types. For example, alloy-primitives::FixedBytes is represented by an alloy-sol-types::FixedBytes, whose's SolType impl points back to the former via the associated RustType.

impl<const N: usize> SolType for FixedBytes<N>
where
    ByteCount<N>: SupportedFixedBytes,
{
    type RustType = RustFixedBytes<N>;   // alias for the primitive
    // rest of impl   
}

In each case RustType points back to the alloy primitive, except for Bytes. Currently, Bytes instead maps to Vec<u8>.

impl SolType for Bytes {
    type RustType = Vec<u8>;
}

This creates issues for encoding & decoding patterns in SDKs like Stylus, which declare traits like.

pub trait AbiType {
    type SolType: SolType<RustType = Self>;
}

In particular, we lose the bidirectional relationship the other primitives have. The current work-around in Stylus is to declare a second, incompatible Bytes type that implements SolType with a correspondingSolValueType, but this is suboptimal since it's both confusing and not officially supported by Alloy.

/// **Note:** this trait is an implementation detail. As such, it should not
/// be implemented directly unless implementing a custom
/// [`SolType`](crate::SolType), which is also discouraged. Consider
/// using [`SolValue`](crate::SolValue) instead.
ρ trait SolTypeValue<T: superSolType> { ... }

Solution

The Solution is to make Bytes map to Bytes, which it turns out is a very small diff.

PR Checklist

  • Added Tests
  • Added Documentation
  • breaking changes

@prestwich
Copy link
Member

the original goal of using Vec<u8> (like using [u8;N] in the past) was to make it more natural for rust developers. i.e. these are the types they would use anyway, with no conversion or new API necessary. Given that we've moved away from that goal over time, I'm fine making this change

Couple things:

  • can we ease instantiation of Bytes with a bytes![1,2,3] macro to match other bespoke types?
  • in what situations will a user use Vec<u8>, get the 32x blowup in encoding size, end up with the wrong blob, and ask in alloy chat what they did wrong?

@rachel-bousfield
Copy link
Contributor Author

rachel-bousfield commented Feb 29, 2024

@prestwich Regarding the first point, there's currently a macro_rules bytes! macro that lets you create a Bytes from string literals. If we were to move to a proc macro, bytes![...] could expand to Bytes::from(vec![...]) for non-str inputs.

macro_rules! bytes {
    () => {
        $crate::Bytes::new()
    };

    ($($s:literal)+) => {{
        // force const eval
        const STATIC_BYTES: &'static [u8] = &$crate::hex!($($s)+);
        $crate::Bytes::from_static(STATIC_BYTES)
    }};
}

For the second point, something that's nice about this is that bytes now expands to Bytes. You'd need to put []uint8 in an event, say, for the generated struct to actually have a Vec<u8> if I understand things correctly.

@prestwich
Copy link
Member

prestwich commented Mar 1, 2024

you can add a macro rules arm like this:

    [$($inner:literal),+] => {{
        // force const eval
        const STATIC_BYTES: &'static [u8] = &[$($inner),+];
        $crate::Bytes::from_static(STATIC_BYTES)
    }}

    #[test]
    fn bytes_macros() {
        const B1: Bytes = bytes!("010203040506070809");
        const B2: Bytes = bytes![1, 2, 3, 4, 5, 6, 7, 8, 9];

        assert_eq!(B1, B2);
    }

@prestwich prestwich self-assigned this Mar 13, 2024
@prestwich prestwich requested a review from mattsse March 13, 2024 23:04
@prestwich prestwich merged commit 9ba8081 into alloy-rs:main Mar 13, 2024
21 checks passed
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

Successfully merging this pull request may close these issues.

4 participants