feat: add logs, add log dynamic decoding #271
Conversation
DaniPopes
force-pushed
the
dani/logs
branch
3 times, most recently
from
ee070d7
to
23a2510
Compare
crates/primitives/src/log.rs
Outdated
| )] | ||
| pub struct Log { | ||
| /// The indexed topic list. | ||
| pub topics: Vec<B256>, |
There was a problem hiding this comment.
pub allows for construction of invalid logs with 5+ topics
There was a problem hiding this comment.
types exist to solve skill issues :P
There was a problem hiding this comment.
i would agree we can add getters instead
There was a problem hiding this comment.
They might be invalid in EVM, but we can use invalid states for testing and maybe other usecases in the futures. I don't think we should lock ourselves into using arrays since it doesn't really improve anything, might be even worse by bloating the stack size of one log to 160 bytes or more. If you box it well you're back to a vec basically
| @@ -151,4 +151,9 @@ pub trait SolEvent: Sized { | |||
| let body = Self::decode_data(data, validate)?; | |||
| Ok(Self::new(topics, body)) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
hot take. We should consider #[doc(hide)] all the trait methods that do not operate on &Log or output Log, as users should generally not use them directly
There was a problem hiding this comment.
They should because Log can't borrow data while the other can
| /// Provides event encoding and decoding for the [`Event`] type. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented for types outside of this | ||
| /// crate. It is implemented only for [`Event`]. |
There was a problem hiding this comment.
Should this be a Resolve like interface that outputs a DynEvent and then uses that to (en/de)code?
There was a problem hiding this comment.
The only thing it would do is resolve the types, I don't think so
There was a problem hiding this comment.
i guess this is a question of how we conceptualize dyn-abi. if it has the same scope as sol-types then we should have dyn equivalents to everything in sol-types
the original idea was to have a runtime representation that was format-independent, and then be able to convert JSON or solidity lines, or type strings to the runtime representation. we're now kinda mixing that, where some things are resolved to the runtime repr, and some aren't. which leads to a somewhat weird API
There was a problem hiding this comment.
or for a concrete thing, if we do this as an ext trait then we duplicate code across these two impls
impl EventExt for Event {}
impl EventExt for syn_solidity::ItemEvent {}
I would prefer something like this to avoid duplication and move all coding through a specific runtime repr
trait ResolveEvent {
fn resolve(&self) -> DynEvent
}
impl ResolveEvent for JsonEvent {}
impl ResolveEvent for syn_soldity::ItemEvent {}
crates/primitives/src/log.rs
Outdated
| )] | ||
| pub struct Log { | ||
| /// The indexed topic list. | ||
| pub topics: Vec<B256>, |
There was a problem hiding this comment.
i would agree we can add getters instead
crates/dyn-abi/src/ext/abi.rs
Outdated
| @@ -92,8 +92,8 @@ impl JsonAbiExt for Constructor { | |||
| } | |||
|
|
|||
| #[inline] | |||
| fn decode_input(&self, data: &[u8]) -> Result<Vec<DynSolValue>> { | |||
| decode(data, &self.inputs) | |||
| fn decode_input(&self, data: &[u8], validate: bool) -> Result<Vec<DynSolValue>> { | |||
There was a problem hiding this comment.
What's our general stance on functions which have validate: bool? When is it OK to not validate?
There was a problem hiding this comment.
validate set to true is the same as calling the _whole version of the function in ethabi, which I didn't know existed prior to this
| // TODO: How do we verify here? | ||
| // wrong_event.decode_log_object(&log, true).unwrap_err(); |
There was a problem hiding this comment.
I guess that decoding 2 words into 1 (non indexed address) should fail? But currently doesn't, I'm not really sure how to fix this and it's out of scope
There was a problem hiding this comment.
ethabi::decode_whole says it "Fails, if some data left to decode" so I guess that would be enough to fix this
DaniPopes
force-pushed
the
dani/logs
branch
from
23a2510
to
8ebdc85
Compare
DaniPopes
force-pushed
the
dani/logs
branch
from
8ebdc85
to
82ba46b
Compare
* refactor: DynSolEvent and ResolveSolEvent * fix: num_topics * feature: Log::is_valid * test: make assertion * chore: getters on DynSolEvent * chore: remove dbg * lint: clippy * lint: clippy * nit: indent
* feat: add logs, add log dynamic decoding * feat: logs bloom function * merge fixes * DynSolEvent & ResolveSolEvent (#309) * refactor: DynSolEvent and ResolveSolEvent * fix: num_topics * feature: Log::is_valid * test: make assertion * chore: getters on DynSolEvent * chore: remove dbg * lint: clippy * lint: clippy * nit: indent --------- Co-authored-by: James Prestwich <[email protected]>
Motivation
Closes #210
Common
LogstructureSolution
LogstructureEventExttraitPR Checklist