-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[BCI-3988] - FilteredLogs receive []Expression instead of whole KeyFilter #14109
Changes from all commits
6910e36
6ddba9b
5bb5e81
955b89c
f56ab63
6d43c8f
394b81d
46f3e0e
ab0e828
8933841
7550e2b
1b19993
26420ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"chainlink": minor | ||
--- | ||
|
||
FilteredLogs receive Expression instead of whole KeyFilter. #internal |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ import ( | |
"go.uber.org/zap/zapcore" | ||
|
||
"github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
"github.com/smartcontractkit/chainlink-common/pkg/types/query" | ||
"github.com/smartcontractkit/chainlink-common/pkg/types/query/primitives" | ||
commonutils "github.com/smartcontractkit/chainlink-common/pkg/utils" | ||
|
||
htMocks "github.com/smartcontractkit/chainlink/v2/common/headtracker/mocks" | ||
|
@@ -2091,3 +2093,39 @@ func TestFindLCA(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestWhere(t *testing.T) { | ||
address := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") | ||
eventSig := common.HexToHash("0xabcdef1234567890abcdef1234567890abcdef1234") | ||
ts := time.Now() | ||
|
||
expr1 := logpoller.NewAddressFilter(address) | ||
expr2 := logpoller.NewEventSigFilter(eventSig) | ||
expr3 := query.Timestamp(uint64(ts.Unix()), primitives.Gte) | ||
expr4 := logpoller.NewConfirmationsFilter(evmtypes.Confirmations(0)) | ||
|
||
t.Run("Valid combination of filters", func(t *testing.T) { | ||
result, err := logpoller.Where(expr1, expr2, expr3, expr4) | ||
assert.NoError(t, err) | ||
assert.Equal(t, []query.Expression{expr1, expr2, expr3, expr4}, result) | ||
}) | ||
|
||
t.Run("No expressions (should return empty slice)", func(t *testing.T) { | ||
result, err := logpoller.Where() | ||
assert.NoError(t, err) | ||
assert.Equal(t, []query.Expression{}, result) | ||
}) | ||
|
||
t.Run("Invalid boolean expression", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are the valid and invalid expressions documented somewhere? Would it make sense to go through all them in these tests and show how to properly use them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This The underlying function has extensive tests here which uses the test cases defined here. Also the usage eg. defined in the function comment is clear Let me know your thoughts if this seems enough |
||
invalidExpr := query.Expression{ | ||
BoolExpression: query.BoolExpression{ | ||
Expressions: []query.Expression{}, | ||
}, | ||
} | ||
|
||
result, err := logpoller.Where(invalidExpr) | ||
assert.Error(t, err) | ||
assert.EqualError(t, err, "all boolean expressions should have at least 2 expressions") | ||
assert.Equal(t, []query.Expression{}, result) | ||
}) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these filters are being combined as a logical "AND" - the events need to match all filters. What if you want a logical "OR" - events that match at least one of the filters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing this function at a high level, maybe it's redundant given the underlying tests. More details in my previous comment here: #14109 (comment)