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

Improve log poller codec usage and optimise log event discriminator matching #1014

Merged
merged 8 commits into from
Jan 25, 2025

Conversation

ilija42
Copy link
Contributor

@ilija42 ilija42 commented Jan 14, 2025

Description

  • Fix event discriminator handling, fix codec event tests.
  • Implement very optimised discriminator extractor for faster log poller event matching
  • Init codec through NewCodec in LP which resolves discriminators automatically on Decode

@ilija42 ilija42 requested review from a team as code owners January 14, 2025 18:23
@ilija42 ilija42 requested review from reductionista and EasterTheBunny and removed request for a team and reductionista January 14, 2025 18:23
@ilija42 ilija42 force-pushed the improve-lp-codec-usage branch 2 times, most recently from a3ad8c0 to 577d1be Compare January 14, 2025 18:34
@ilija42 ilija42 requested a review from reductionista January 14, 2025 18:37
@ilija42 ilija42 force-pushed the improve-lp-codec-usage branch from 0c844c5 to e0d05b3 Compare January 14, 2025 19:28
@ilija42 ilija42 force-pushed the improve-lp-codec-usage branch from e0d05b3 to 3e58e35 Compare January 14, 2025 19:32
@ilija42 ilija42 changed the title Improve log poller codec usage Improve log poller codec usage and optimise log event discriminator matching Jan 23, 2025
@reductionista
Copy link
Contributor

Implement very optimised discriminator extractor for faster log poller event matching

Faster than what? It can't be faster than the existing implementation which just does a simple lookup in knownDiscriminators without decoding anything. Is the advantage that this is easier to read or more maintainable?

Comment on lines 124 to 125
// TODO isn't discrimintaor already checked above in MatchingFiltersForEncodedEvent?
if len(log.Data) < 8 {
Copy link
Contributor

@reductionista reductionista Jan 23, 2025

Choose a reason for hiding this comment

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

I added this check to address a PR comment from @dhaidashenko requesting more validation, in case it's not 8 bytes. I added it, but noted that it isn't something that can happen with the existing code--would just protect against someone accidentally introducing a bug later.

I don't care much either way whether we leave it or take it out, but if you want to take it out you should get @dhaidashenko approval.

Copy link
Contributor

Choose a reason for hiding this comment

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

(If not, remove TODO comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DecodeSubKey uses codec decode which also checks for discriminator, so I don't think that there is any need for this extra validation step

@reductionista
Copy link
Contributor

The description of the PR as optimizing the event matching is misleading, since it's not optimizing anything relative to the current implementation. But it's a more self-contained way of handling the discriminators we might be able to re-use at some point, and adds negligible overhead so I am happy with it. Just pointed out a few minor cosmetic issues to fix.

if err != nil {
if err = decoder.Decode(ctx, raw, decodedEvent, filter.EventName); err != nil {
err = fmt.Errorf("failed to decode sub key raw data: %v, for filter: %s, for subKeyPath: %v, err: %w", raw, subKeyPath, filter.Name, err)
lggr.Criticalw(err.Error())
Copy link
Contributor Author

@ilija42 ilija42 Jan 24, 2025

Choose a reason for hiding this comment

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

critical because it shouldn't ever happen, it would mean that log discriminators match, but what the onchain contract emitted is malformed

@jadepark-dev jadepark-dev merged commit a855d4d into develop Jan 25, 2025
36 checks passed
@jadepark-dev jadepark-dev deleted the improve-lp-codec-usage branch January 25, 2025 14:19
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