feat: make to_compact borrow
#9488
Conversation
github-actions
bot
added
the
C-debt
| #[main_codec(no_arbitrary, no_serde)] | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| struct GenesisAccountRef<'a> { |
There was a problem hiding this comment.
Bridge types get slightly more troublesome for types that do not support Copy (or we choose not to).
This also introduces lifetimes into the reth_codec macro which makes things slightly more complex just for bridge types.
There was a problem hiding this comment.
i guess we could give Cow support to to_compact, but idk how complex that would be tbh
| /// The account's private key. Should only be used for testing. | ||
| private_key: Option<&'a B256>, | ||
| } | ||
|
|
||
| #[main_codec] | ||
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | ||
| struct GenesisAccount { |
There was a problem hiding this comment.
from_compact is not supported by types with lifetimes/references, so we still need this bridge type when calling from_compact
| } | ||
|
|
||
| fn from_compact(_: &[u8], _: usize) -> (Self, &[u8]) { | ||
| unimplemented!() |
There was a problem hiding this comment.
I don't like this blanket impl because this then can be called by any type.
there's an argument for splitting these functions into two traits, but not in this pr
There was a problem hiding this comment.
this then can be called by any type
I agree it would be cleaner to have two traits (it wasn't necessary until this PR imo) , but calling this would have to be very intentional and weird &T::from_compact
but could be done as a follow up
joshieDo
changed the title
to_compact borrow
joshieDo
requested review from
rkrasiuk,
Rjected,
rakita,
onbjerg,
shekhirin and
DaniPopes
as code owners
|
rfr, but no merge |
There was a problem hiding this comment.
can't review my own pr hehe
overall I think this is an improvement, but this really shows that the compact trait has a lot of shortcomings
I'd like to see some settings documented on the proc macro
and have a few questions
and would be nice if the vec specialization would also work for slices
| pub fn #test() { | ||
| #fuzz(#ident::default()) | ||
| let fn_from_compact = if has_lifetime { | ||
| quote! { unimplemented!("from_compact not supported with ref structs") } |
There was a problem hiding this comment.
I think this a reasonable approach, we only need this for bridge types really
| let topics = self.topics().iter().collect::<Vec<_>>(); | ||
| topics.specialized_to_compact(&mut buffer); |
There was a problem hiding this comment.
can we make this work for &[] as well?
| } | ||
|
|
||
| fn from_compact(_: &[u8], _: usize) -> (Self, &[u8]) { | ||
| unimplemented!() |
There was a problem hiding this comment.
I don't like this blanket impl because this then can be called by any type.
there's an argument for splitting these functions into two traits, but not in this pr
| version.as_bytes().to_vec().to_compact(buf); | ||
| git_sha.as_bytes().to_vec().to_compact(buf); | ||
| build_timestamp.as_bytes().to_vec().to_compact(buf) |
There was a problem hiding this comment.
would be could if this would work on slices
| version.as_bytes().to_vec().to_compact(buf); | ||
| git_sha.as_bytes().to_vec().to_compact(buf); | ||
| build_timestamp.as_bytes().to_vec().to_compact(buf) |
There was a problem hiding this comment.
is the to_vec still required?
| @@ -37,7 +37,7 @@ pub fn maybe_generate_tests(args: TokenStream, ast: &DeriveInput) -> TokenStream | |||
| let len = field.encode(&mut buf); | |||
| let mut b = &mut buf.as_slice(); | |||
| let decoded = super::#type_ident::decode(b).unwrap(); | |||
| assert_eq!(field, decoded); | |||
| assert_eq!(field, decoded, "maybe_generate_tests::rlp"); | |||
closes #9487
needs more work on derive impls next