-
Notifications
You must be signed in to change notification settings - Fork 68
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
feat: remove block range limit in eth_getLogs
when filtering with a single address
#2236
Conversation
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
=======================================
Coverage 75.15% 75.15%
=======================================
Files 13 13
Lines 644 644
Branches 118 118
=======================================
Hits 484 484
Misses 115 115
Partials 45 45 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
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.
LG
1 nit
Signed-off-by: Luis Mastrangelo <[email protected]>
@ebadiere @AlfredoG87 I would appreciate your input here if you have some time, thanks! |
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.
Overall looks good, leaved some comments around testing and a question on process.env.TEST
that I don't understand, if is a new feature flag it should be documented and use a constant at the top of the class as other feature flags do, and should have a more descriptive name than TEST
2 other questions:
-
should we really allow for
unlimited
amount of blocks? could it become too much too handle, maybe having a different limit altogether? like maybe, 100,000 blocks or something. let me know your rationale here.
Have we tested limits on the mirror node for timestamp of query? (`contracts/${MirrorNodeClient.ADDRESS_PLACEHOLDER}/results/logs) -
On the tests, what happens if we increase the default limit of
1000
to lets say2000
, it would fail?
packages/relay/src/lib/services/ethService/ethCommonService/index.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Quality Gate passedIssues Measures |
The mirror node supports the query without timestamp. The |
… single address (#2236) * Fix address type Signed-off-by: Luis Mastrangelo <[email protected]> * Remove block range limit when filtering with a single address Signed-off-by: Luis Mastrangelo <[email protected]> * Fix single address condition Signed-off-by: Luis Mastrangelo <[email protected]> * Add unit tests Signed-off-by: Luis Mastrangelo <[email protected]> * Add acceptance test for large block range with a single address Signed-off-by: Luis Mastrangelo <[email protected]> * Fix acceptance test Signed-off-by: Luis Mastrangelo <[email protected]> * Update `eth_getLogs`'s openrpc docs summary and restrictions Signed-off-by: Luis Mastrangelo <[email protected]> * Remove unneeded `console.log` Signed-off-by: Luis Mastrangelo <[email protected]> * Remove `env.TEST` usage as suggested by Fredy Signed-off-by: Luis Mastrangelo <[email protected]> * Improve test titles to use `it should` format as suggested by Fredy Signed-off-by: Luis Mastrangelo <[email protected]> * Include more details in the openrpc `eth_getLogs` description Signed-off-by: Luis Mastrangelo <[email protected]> --------- Signed-off-by: Luis Mastrangelo <[email protected]>
… single address (#2236) * Fix address type Signed-off-by: Luis Mastrangelo <[email protected]> * Remove block range limit when filtering with a single address Signed-off-by: Luis Mastrangelo <[email protected]> * Fix single address condition Signed-off-by: Luis Mastrangelo <[email protected]> * Add unit tests Signed-off-by: Luis Mastrangelo <[email protected]> * Add acceptance test for large block range with a single address Signed-off-by: Luis Mastrangelo <[email protected]> * Fix acceptance test Signed-off-by: Luis Mastrangelo <[email protected]> * Update `eth_getLogs`'s openrpc docs summary and restrictions Signed-off-by: Luis Mastrangelo <[email protected]> * Remove unneeded `console.log` Signed-off-by: Luis Mastrangelo <[email protected]> * Remove `env.TEST` usage as suggested by Fredy Signed-off-by: Luis Mastrangelo <[email protected]> * Improve test titles to use `it should` format as suggested by Fredy Signed-off-by: Luis Mastrangelo <[email protected]> * Include more details in the openrpc `eth_getLogs` description Signed-off-by: Luis Mastrangelo <[email protected]> --------- Signed-off-by: Luis Mastrangelo <[email protected]> Signed-off-by: Alfredo Gutierrez <[email protected]>
feat: remove block range limit in `eth_getLogs` when filtering with a single address (#2236) * Fix address type * Remove block range limit when filtering with a single address * Fix single address condition * Add unit tests * Add acceptance test for large block range with a single address * Fix acceptance test * Update `eth_getLogs`'s openrpc docs summary and restrictions * Remove unneeded `console.log` * Remove `env.TEST` usage as suggested by Fredy * Improve test titles to use `it should` format as suggested by Fredy * Include more details in the openrpc `eth_getLogs` description --------- Signed-off-by: Luis Mastrangelo <[email protected]> Signed-off-by: Alfredo Gutierrez <[email protected]> Co-authored-by: Luis Mastrangelo <[email protected]>
Description:
This PR removes the block range limit, e.g., 1000, when the filter in the
eth_getLogs
contains a single address (the optionaladdress
param might be either a string or an addresses array).This change allows consumers to query all the log entries of the given address. For now, we permit only a single address, but in the future we might relax this constraint as well.
The block range validation is also used in
eth_newFilter
, but it should not be affected by this change.Related issue(s):
Fixes #2175
Notes for reviewer:
It also fixes function type signatures, _i.e., use
string[]
instead of[string]
.Checklist