Open
Conversation
There was an incorrect assumption when defining event trigger matches on "complex" data values (i.e. those, spanning multiple data words: bytes, string, ...). Previously, we encoded the `Length` in the trigger definition and expected to read that much from log data in order to match. However, this would break, when the actual log data had a different alignment (due to previous non-targeted arguments). Instead we could remove `Length` from `LogValueRef`, and rely on the encoded length in the event data (see https://docs.soliditylang.org/en/latest/abi-spec.html) at the specified `Offset`. We added more tests, that use a `SimulatedBackend` EVM to make sure, we can properly decode and match actual EVM generated logs. Since `Length` is no longer defined, we had to adjust some tests and remove some edge case checks that no longer apply.
konradkonrad
force-pushed
the
complex_event_data
branch
from
8e1e8a9 to
96ffa89
Compare
konradkonrad
force-pushed
the
complex_event_data
branch
from
e9b683b to
5c2d6a8
Compare
konradkonrad
force-pushed
the
complex_event_data
branch
from
1faf6d2 to
4faa937
Compare
konradkonrad
force-pushed
the
complex_event_data
branch
from
4faa937 to
2d5735a
Compare
jannikluhn
reviewed
Mar 19, 2026
Following a PR comment, this introduces a boolean flag, to signal, whether a LogPredicateValue should be read as a single word (`Dynamic: false`) or an ABI encoded data slice of "dynamic size" (`Dynamic: true`).
konradkonrad
force-pushed
the
complex_event_data
branch
from
9261f79 to
20aa947
Compare
jannikluhn
reviewed
Mar 20, 2026
Contributor
jannikluhn
left a comment
jannikluhn
left a comment
There was a problem hiding this comment.
looks good, just one note about the RLP encoding/decoding
| buf.ListEnd(listIndex) | ||
| } | ||
| buf.WriteBool(r.Dynamic) | ||
| buf.WriteUint64(r.Offset) |
Contributor
There was a problem hiding this comment.
I think this shouldn't work, it needs a buf.List() call at the start to announce it's encoding an RLP list, not individual values. The same in the decoding function. However, since the encoding is static now (not either a list or a single value depending on the length), I don't think the custom EncodeRLP and DecodeRLP functions are necessary at all anymore. The RLP package should be able to encode/decode LogValueRef values out of the box. (missed this in my previous review)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was an incorrect assumption when defining event trigger
matches on "complex" data values (i.e. those, spanning multiple data
words: bytes, string, ...).
Previously, we encoded the
Lengthin the trigger definition andexpected to read that much from log data in order to match. However,
this would break, when the actual log data had a different alignment
(due to previous non-targeted arguments).
Instead we could remove
LengthfromLogValueRef,and rely on the encoded length in the event data (see
https://docs.soliditylang.org/en/latest/abi-spec.html) at the specified
Offset.We added more tests, that use a
SimulatedBackendEVM to make sure, wecan properly decode and match actual EVM generated logs.
Since
Lengthis no longer defined, we had to adjust some tests andremove some edge case checks that no longer apply.
This increments the
Versionbyte of event trigger definition RLP encoding to0x02.Relates to shutter-network/shutter-api#84
Critical for: shutter-network/shutter-api#110