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

Impl TryFrom<alloy_rpc_types::Log> for alloy_primitives::Log #50

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Nov 29, 2023

Came across the following use case:

  • Retrieve logs with Provider::get_logs() that returns a Vec of alloy_rpc_types::Log
  • Then decode logs using SolEvent::decode_log_object() that accept a alloy_primitives::Log

So I needed to convert alloy_rpc_types::Log to alloy_primitives::Log, hence this PR.

@leruaa leruaa changed the title Imp From rpc-types to primitives Log Imp TryFrom<alloy_rpc_types::Log> for alloy_primitives::Log Nov 29, 2023
@leruaa leruaa changed the title Imp TryFrom<alloy_rpc_types::Log> for alloy_primitives::Log Impl TryFrom<alloy_rpc_types::Log> for alloy_primitives::Log Nov 29, 2023
Copy link
Contributor

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

looks good! i have a nit:

crates/rpc-types/src/eth/log.rs Outdated Show resolved Hide resolved
@prestwich
Copy link
Member

@Evalir this ties back to the conversation we were having in telegram the other day. Should RPC types instead embed a primitive Log?

@Evalir
Copy link
Contributor

Evalir commented Nov 29, 2023

@prestwich I think it should—this rpc type is a fuller version of the log object, and the additional APIs from the primitive log add additional safety. However, there's some work to do if we want to embed it here:

  • alloy_primitives::Log should be LogData, as Log omits the emitter (as you said on tg)
  • We then need to add a Log type with the emitter. This can then also remove the raw_log.rs type which is essentially the primitive log with the emitter.
  • Finally, we can embed Log into this rpc type.

@leruaa
Copy link
Contributor Author

leruaa commented Nov 29, 2023

@Evalir Updated following your comment.

Would you like I create an issue for the bigger refactoring you mentioned? I would be happy to work on this.

@prestwich
Copy link
Member

  • alloy_primitives::Log should be LogData, as Log omits the emitter (as you said on tg)
  • We then need to add a Log type with the emitter. This can then also remove the raw_log.rs type which is essentially the primitive log with the emitter.
  • Finally, we can embed Log into this rpc type.

Is there a good way to make it generic over encoded and decoded logs? E.g. can we achieve something like this

struct Log<T = LogData> {
    emitter: Address,
    inner: T,
}

impl Log<LogData> { 
    fn try_decode<T: SolEvent>(self) -> Result<Log<T>, Self>;
} 
impl Log<T> where T: sol_types::SolEvent { 
    fn encode(self) -> Log<LogData>;
}

impl RlpEncodable for Log<LogData> { ... }
impl RlpEncodable for Log<T> where T: sol_types::SolEvent { ... }

@gakonst gakonst merged commit 0ba6b61 into alloy-rs:main Dec 18, 2023
17 checks passed
@leruaa leruaa deleted the convert_log branch December 18, 2023 21:10
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