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

LogPoller DSL Parser #959

Open
wants to merge 10 commits into
base: feature/NONEVM-745-logpoller-db-models
Choose a base branch
from

Conversation

EasterTheBunny
Copy link
Contributor

@EasterTheBunny EasterTheBunny commented Dec 4, 2024

Description

This PR adds a DSL parser that converts basic query filters into postgresql queries for logpoller. Additionally solana address and transaction hashes are handled.

NONEVM-744

Requires Dependencies

@EasterTheBunny EasterTheBunny requested review from a team as code owners December 4, 2024 18:10
@EasterTheBunny EasterTheBunny changed the base branch from develop to feature/NONEVM-745-logpoller-db-models December 4, 2024 18:11
@@ -156,3 +157,27 @@ func (o *DSORM) SelectLogs(ctx context.Context, start, end int64, address Public
}
return logs, nil
}

func (o *DSORM) FilteredLogs(ctx context.Context, filter []query.Expression, limitAndSort query.LimitAndSort, _ string) ([]Log, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking the FilteredLogs method that calls this would also be in this PR, but I guess it makes more sense if I just include it in my Main LogPoller Loop PR... probably too difficult to add it here without the struct that's defined in RegisterFilters PR.

Copy link
Contributor

@reductionista reductionista Dec 7, 2024

Choose a reason for hiding this comment

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

Oh, I do think we should add some tests of this function to orm_tests.go.

In evm, there are several tests that test it in different ways: TestORM_IndexedLogs, TestORM_SelectLogsCreatedAfter, TestORM_SelectIndexedLogsByTxHash, TestORM_SelectLogsWithSigsByBlockRangeFilter. Those also test the corresponding legacy functions which we don't need. But maybe we could just combine the parts that test FilteredLogs together into a single TestFilteredLogs here?


result, args, err := parser.buildQuery(chainID, expressions, limiter)
expected := logsQuery(
" WHERE chain_id = :solana_chain_id " +
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be :chain_id rather than :solana_chain_id, to match the db schema.

Also, hoping we can move this code to chainlink-framework at some point to re-use it for many chain families, abstracting it a bit to fill in the custom data types.

ErrInvalidCursorDir = errors.New("invalid cursor direction")
ErrInvalidCursorFormat = errors.New("invalid cursor format")
ErrInvalidSortDir = errors.New("invalid sort direction")
ErrInvalidSortType = errors.New("invalid sort by type")
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call on defining these all together! This will help later with abstraction.

}

func (v *pgDSLParser) whereClause(expressions []query.Expression, limiter query.LimitAndSort) (string, error) {
segment := fmt.Sprintf("WHERE %s = :solana_chain_id", chainIDFieldName)
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
segment := fmt.Sprintf("WHERE %s = :solana_chain_id", chainIDFieldName)
segment := fmt.Sprintf("WHERE %s = :chain_id", chainIDFieldName)

(see my comment in parser_test.go about this... probably should have left it here instead)

case *pgDSLParser:
v.VisitEventSigFilter(f)
}
}
Copy link
Contributor

@reductionista reductionista Dec 7, 2024

Choose a reason for hiding this comment

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

I think we're missing an essential feature here that CCIP will require, and that we won't be able to implement QueryKey without. You have eventSigFilter and eventByAddressFitler, but we also need an eventBySubKeyFilter so that we can filter by subkey.

We'll need that to implement support for IndexedSequencesKeyFilter and several other IndexedSequences... filters that can be passed to QueryKey. CCIP definitely uses some of those.

Copy link
Contributor

@reductionista reductionista Dec 7, 2024

Choose a reason for hiding this comment

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

See: func (b *EventBinding) encodeComparator(comparator *primitives.Comparator) (query.Expression, error) in chainlink/core/services/relay/evm/read/event.go)

We'll have to call NewEventBySubKey there instead of NewEventByTopicFilter and NewEventByWordFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be adding this when I do the remapping.

Copy link
Contributor

@reductionista reductionista left a comment

Choose a reason for hiding this comment

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

Mostly missing eventBySubKeyFilter, but see comments suggesting adding some tests for FilteredLogs and one minor change (:solana_chain_id -> :chain_id)

v.expression = fmt.Sprintf(
"%s = :%s",
txHashFieldName,
v.args.withIndexedField(txHashFieldName, txHash),
Copy link
Contributor

Choose a reason for hiding this comment

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

solana.PublicKey does not implement Value method, so not sure if the query won't error. Might need to wrap it into type defined in types file.

pkg/solana/logpoller/query.go Show resolved Hide resolved
@dhaidashenko dhaidashenko force-pushed the feature/NONEVM-745-logpoller-db-models branch from 8de5211 to df7ab9f Compare December 20, 2024 15:01
@dhaidashenko dhaidashenko requested review from a team as code owners December 20, 2024 15:01
@dhaidashenko dhaidashenko force-pushed the feature/NONEVM-745-logpoller-db-models branch from df7ab9f to 4178d4c Compare December 20, 2024 15:14
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.

3 participants