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.

Sign up for GitHub

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

Jump to bottom

Add logpoller.FilteredLogs and support for filtering on subkey #1020

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Add logpoller.FilteredLogs and support for filtering on subkey #1020

wants to merge 5 commits into from

Conversation

reductionista
Copy link
Contributor

@reductionista reductionista commented Jan 17, 2025 •
edited
Loading

This adds FilteredLogs to logPoller.Service to complete the LogPoller API, as well as these functions and types in parser.go to support filtering events by subkeys:

func NewEventBySubkeyFilter(subkeyIndex uint64, valueComparers []primitives.ValueComparator) (query.Expression, error)
func (v *pgDSLParser) VisitEventSubkeysByValueFilter(p *eventBySubkeyFilter)
type IndexedValueComparator

Also adds a test for filtering on subkeys to parser_test.go

Supports: #1016

ilija42
ilija42 reviewed Jan 20, 2025
View reviewed changes
pkg/solana/chain.go Outdated Show resolved Hide resolved
ilija42
ilija42 reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@ilija42 ilija42 left a comment

Choose a reason for hiding this comment

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

Its a bit hard to review because it has all the changes from the filter PR, can you point this to improve-lp-codec-usage

@reductionista
Copy link
Contributor Author

reductionista commented Jan 21, 2025 •
edited
Loading

Its a bit hard to review because it has all the changes from the filter PR, can you point this to improve-lp-codec-usage

It's all in a single commit, so I figured it wouldn't be too difficult to look at... probably should have linked to the commit directly instead of the PR:
ce7cdc2

The Log Processing PR has been waiting for CODEOWNER approval since Thursday, not sure what the holdup is I was hoping it would be merged already. Otherwise I would have raised this against that branch instead of against develop. Hopefully will get merged quickly on Tues so I can just rebase onto develop.

EasterTheBunny
EasterTheBunny previously approved these changes Jan 22, 2025
View reviewed changes
@reductionista reductionista marked this pull request as ready for review January 22, 2025 20:36
@reductionista reductionista requested a review from a team as a code owner January 22, 2025 20:36
ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
pkg/solana/logpoller/parser.go Outdated
@@ -22,6 +22,7 @@ const (
addressFieldName = "address"
eventSigFieldName = "event_sig"
defaultSort = "block_number ASC, log_index ASC"
subKeysFieldName = "subkey_values"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be unused

ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
pkg/solana/logpoller/parser.go Outdated
}

// Add 1 since postgresql arrays are 1-indexed.
subkeyIdx := v.args.withIndexedField("subkey_index", p.SubkeyIndex+1)
Copy link
Contributor

@ilija42 ilija42 Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

should this be a constant?

ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
pkg/solana/logpoller/parser.go Outdated

comps := make([]string, len(p.ValueComparers))
for idx, comp := range p.ValueComparers {
comps[idx], v.err = makeComp(comp, v.args, "subkey_value", subkeyIdx, "subkey_values[:%s] %s :%s")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this string pulled out above as a constant, but its unused

ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
pkg/solana/logpoller/parser.go Outdated
Comment on lines 63 to 64
type eventBySubkeyFilter struct {
SubkeyIndex uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type eventBySubkeyFilter struct {
SubkeyIndex uint64
type eventBySubKeyFilter struct {
SubKeyIndex uint64

ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
pkg/solana/logpoller/parser.go Outdated
}

// Add 1 since postgresql arrays are 1-indexed.
subkeyIdx := v.args.withIndexedField("subkey_index", p.SubkeyIndex+1)
Copy link
Contributor

@ilija42 ilija42 Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
subkeyIdx := v.args.withIndexedField("subkey_index", p.SubkeyIndex+1)
subKeyIdx := v.args.withIndexedField("subkey_index", p.SubKeyIndex+1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. There are a few dozen more places where it's subkey or Subkey, so I just went through and updated all of them.

ilija42
ilija42 reviewed Jan 23, 2025
View reviewed changes
Copy link
Contributor

@ilija42 ilija42 left a comment

Choose a reason for hiding this comment

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

left some nit comments, but looks good otherwise

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
52.8% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilija42 ilija42 ilija42 left review comments

@EasterTheBunny EasterTheBunny EasterTheBunny left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reductionista @EasterTheBunny @ilija42