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

[Bug] Log doesn't allow implementation of traits for non-default generic parameter #544

Closed
Wodann opened this issue Feb 28, 2024 · 5 comments · Fixed by #553
Closed

[Bug] Log doesn't allow implementation of traits for non-default generic parameter #544

Wodann opened this issue Feb 28, 2024 · 5 comments · Fixed by #553
Labels
bug Something isn't working

Comments

@Wodann
Copy link

Wodann commented Feb 28, 2024

Component

primitives

What version of Alloy are you on?

0.6.3

Operating System

Linux

Describe the bug

The Log generic uses a default type = LogData. This means that for any traits that are implemented for Log, the traits are only implemented for Log::<LogData>. Given that Rust does not allow implementation of external traits on external types, this prohibits people from implementing traits on the Log type when using a locally defined CustomLogData.

E.g. when trying to impl alloy_rlp::Encodable for LogData<CustomLogData> this would fail with the compiler error:

only traits defined in the current crate can be implemented for types defined outside of the crate define and implement a trait or new type instead

To solve this, the trait implementations in https://github.com/alloy-rs/core/blob/main/crates/primitives/src/log.rs#L120 should be generic; e.g. impl<LogDataT: SomeTrait> alloy_rlp::Encodable for Log<LogDataT> { .. }.

@Wodann Wodann added the bug Something isn't working label Feb 28, 2024
@prestwich
Copy link
Member

without capturing log encoding semantics in primitives, how would you design SomeTrait?

@Wodann
Copy link
Author

Wodann commented Mar 4, 2024

My proposal would be:

trait ExecutionLogData {
  fn topics(&self) -> &[B256];

  fn data(&self) -> &Bytes;
}

This should provide you with all of the information to implement the current implementations of alloy_rlp::Encodable and alloy_rlp::Decodable.

@prestwich
Copy link
Member

The goal of parameterizing over the data is to allow replacing it with deserialized versions and dropping the serialized version.

So this might work for something like trait IntoLogData { fn into(&self) -> LogData }

@Wodann
Copy link
Author

Wodann commented Mar 4, 2024

Yeah, a simple LogDataT: Into<LogData> would suffice too for the two rlp traits 🙂

@prestwich
Copy link
Member

it needs a bit more complexity, as it would require RLP coding which is fallible. so it migh be something more like:

impl<T: TryInto<LogData, Error = alloy_rlp::Error>> alloy_rlp::Encodable for Log<T> { ... }
impl<T: TryInto<LogData, Error = Infallible>> alloy_rlp::Encodable for Log<T> { ... }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants