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

Don't cut bytes from non-address topics + account for empty topics #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

catalyst17
Copy link
Contributor

@catalyst17 catalyst17 commented Oct 18, 2024

TL;DR

Improved handling of topic values in the getTopicValueFormat function.

What changed?

  • Modified the getTopicValueFormat function to handle empty topic strings.
  • Updated the logic for processing non-empty topics:
    • Now uses ethereum.FromHex to convert the topic to bytes
      • ⚠️ prev. used ethereum.HexToAddress was wrong in this context cause topics are not necessarily addresses)
    • Ensures the byte slice is exactly 32 bytes by left-padding with zeros.
    • Converts the padded bytes to a hash and returns the hexadecimal representation.

How to test?

  1. Test the function with an empty string input to ensure it returns an empty string.
  2. Test with various non-empty topic strings, including:
    • Short strings (less than 32 bytes)
    • Exactly 32-byte strings
    • Strings longer than 32 bytes
  3. Verify that the output is always a 32-byte hexadecimal string (or empty for empty input).

Why make this change?

This change improves the robustness of topic value handling:

  • It correctly handles empty topics, which are stored as empty strings by the indexer.
  • It ensures consistent 32-byte padding for all non-empty topics, regardless of their original length.
  • This approach aligns better with Ethereum's topic encoding standards, potentially fixing issues related to topic matching or filtering.

@catalyst17 catalyst17 changed the title fix: don't cut bytes from non-address topics + account for empty topics Don't cut bytes from non-address topics + account for empty topics Oct 18, 2024
Copy link
Contributor Author

catalyst17 commented Oct 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @catalyst17 and the rest of your teammates on Graphite Graphite

@catalyst17 catalyst17 marked this pull request as ready for review October 18, 2024 14:34
@catalyst17 catalyst17 force-pushed the arsenii/fix-topic-value-format branch from 6912e8c to bd55ca1 Compare October 18, 2024 14:37
if topic == "" {
// if there is no indexed topic, indexer stores an empty string
// we shouldn't pad and hexify such an argument then
return ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change this to nil already since we're already working on this or will it blow it up too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a separate thing I wanted to bring up, actually

saw this and thought that maybe we can actually leave it as is, but change the columns to not allow NULLs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, sounds good. Doesn't really matter '' or null for our use case. We can change the schema and then apply it manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we anyway seem to have no benefit of NULL-ability in these columns and bloom filters are behaving the same way with negation - we can at least optimise some space and performance by completely turning it of

(and, consequently, continuing to use empty strings for topics that don't exist)

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.

2 participants