Skip to content

Transaction filter #5

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 21 commits into from
Apr 8, 2024
Merged

Transaction filter #5

merged 21 commits into from
Apr 8, 2024

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Jan 24, 2024

Description

Created a transaction filter. The structure is largely similar to a block filter since it was referenced, but it is designed to enable search for each data using TxResult.

Additionally, I utilized the method chaining to implement a query-like search. This allows the application of complex filtering criteria in a simplified manner.

Example Usage

filteredTxResults := NewTxFilter().
    Height(100).
    GasUsed(500, 1000).
    Apply()

f.ClearConditions()

@notJoon notJoon changed the title [WIP] transaction filter Transaction filter Jan 25, 2024
@notJoon notJoon marked this pull request as ready for review January 25, 2024 06:21
@notJoon notJoon requested a review from a team as a code owner January 25, 2024 06:21
@notJoon notJoon requested a review from zivkovicmilos January 29, 2024 03:29
@zivkovicmilos
Copy link
Member

Hey @notJoon,

Thank you for the PR 🙏

I apologize for the late reply on this, it slipped through my notifications.
I'll take a look at the PR soon and give feedback, I have a feeling we can improve it just by skimming through

@ajnavarro
Copy link
Collaborator

I think we can avoid applying filters with elements on memory and do it directly using pebbleDB and their properties to iterate through keys in order.

Have a look at #12 . This approach can evolve even more adding secondary indexes. Not needed right now because queries take less than 20ms.

@notJoon
Copy link
Member Author

notJoon commented Feb 29, 2024

I think we can avoid applying filters with elements on memory and do it directly using pebbleDB and their properties to iterate through keys in order.

yeah, I think that would be much efficient than this filtering things.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 🙏

Even though this PR doesn't contain connecting logic with the RPC layer, I see it as a good foundation for v1 of transaction filtering.

I've left some comments that should be addressed, I think we can greatly simplify the code

@notJoon notJoon requested a review from zivkovicmilos March 28, 2024 07:00
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

Thank you for addressing the comments 🙏

I've left a few suggestions, otherwise it looks good to go

@zivkovicmilos
Copy link
Member

Hey @notJoon, what's the status on this PR? Did you get a chance to check the comments?

@notJoon
Copy link
Member Author

notJoon commented Apr 5, 2024

Hey @notJoon, what's the status on this PR? Did you get a chance to check the comments?

I apologize for the late reply. I should be able to get it all done by in this weekend.

@notJoon notJoon requested a review from zivkovicmilos April 8, 2024 05:15
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I'll open up a PR with @ajnavarro to connect this out with the RPC layer

Thank you for resolving the convos 🙏

@zivkovicmilos zivkovicmilos merged commit 2a70d0e into main Apr 8, 2024
3 checks passed
@zivkovicmilos zivkovicmilos deleted the tx-subscription-type branch April 8, 2024 08:14
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants