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

feat: rlp encoding for logs with generic event data #553

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

prestwich
Copy link
Member

closes #544

Motivation

Allow rlp encoding of logs after their data has been ABI decoded.

Solution

Add a set of methods and a blanket Encode implementation for
Log<T> where for<'a> &'a T: Into<LogData>,

Note that this change does not allow decoding, as that would require embedding the ABI error into the RLP error decode impl

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich prestwich added the enhancement New feature or request label Mar 4, 2024
@prestwich prestwich self-assigned this Mar 4, 2024
@prestwich prestwich marked this pull request as ready for review March 4, 2024 22:06
@gakonst
Copy link
Member

gakonst commented Mar 8, 2024

@Rjected ptal

Copy link
Collaborator

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

mostly nits, otherwise lgtm

crates/sol-types/tests/doctests/events.rs Show resolved Hide resolved
crates/primitives/src/log.rs Outdated Show resolved Hide resolved
Comment on lines +143 to +152
impl<T> alloy_rlp::Encodable for Log<T>
where
for<'a> &'a T: Into<LogData>,
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see, can just be converted to Log<LogData> when serializing

Copy link
Member Author

@prestwich prestwich Mar 9, 2024

Choose a reason for hiding this comment

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

right. and because we are usually using sol! there is no problem

This is significantly less ergonomic with DynSolEvent tho, as a decoded sol event can't be assumed to be validly encoded the way a decoded sol! struct with SolEvent can be. That
assumption allows the infallible reserialize

cc @DaniPopes do you think that we should clone + attach the DynSolEvent as a prop on the DecodedEvent in alloy-dyn-abi so that we can assume valid construction and encoding? This would let us impl From<&DecodedEvent> for LogData

crates/primitives/src/log.rs Show resolved Hide resolved
@prestwich prestwich requested a review from Rjected March 9, 2024 20:32
@prestwich
Copy link
Member Author

cc @DaniPopes what's up with the tests?

@DaniPopes
Copy link
Member

DaniPopes commented Mar 11, 2024

You can update them by running with env TRYBUILD=overwrite, make sure to be on the latest nightly (and with -Zthreads=1 if you overwrite it)

Copy link
Collaborator

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

This looks good to me

@Rjected
Copy link
Collaborator

Rjected commented Mar 11, 2024

this also needs to be rebased

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

needs clippy

@prestwich
Copy link
Member Author

prestwich commented Mar 12, 2024

clippy issue seems to have been introduced by merge of main. why?

Please do not merge until we know why this is the case

@Rjected
Copy link
Collaborator

Rjected commented Mar 12, 2024

clippy issue seems to have been introduced by merge of main. why?

Please do not merge until we know why this is the case

the new nightly clippy doesnt like things that are never constructed, including in tests

@onbjerg
Copy link
Member

onbjerg commented Mar 12, 2024

hmm unsure, was this struct ever used? doing

git grep "StructProp" (git rev-list --all)

yields nothing except changelog and the struct def itself

weird that it wouldn't be caught before this?

edit: not weird given dan comment

@prestwich
Copy link
Member Author

prestwich commented Mar 12, 2024

the new nightly clippy doesnt like things that are never constructed, including in tests

why are we using nightly clippy? didnt we decide that was a mistake with ethers as it introduced unstable lints and situations like this where CI breaks for no reason?

@prestwich prestwich force-pushed the prestwich/blanket-log-encode branch from 2ff92d4 to ee44732 Compare March 13, 2024 18:24
@prestwich
Copy link
Member Author

CI is unrelated.

@prestwich prestwich merged commit 5edbf5e into main Mar 13, 2024
20 of 21 checks passed
@prestwich prestwich deleted the prestwich/blanket-log-encode branch March 13, 2024 18:35
@DaniPopes
Copy link
Member

DaniPopes commented Mar 13, 2024

This breaks RLP encoding for Log<LogData> (the default). Fixed in #573.

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

Successfully merging this pull request may close these issues.

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