Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Limit for logs filter. #2180

Merged
merged 3 commits into from
Sep 21, 2016
Merged

Limit for logs filter. #2180

merged 3 commits into from
Sep 21, 2016

Conversation

tomusdrw
Copy link
Collaborator

Similar to #2073

This time it allows to specify optional limit parameter to newFilter and then subsequent calls to getFilterLogs or getFilterChanges will return limited number of logs.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 19, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.131% when pulling 7c0831e on limit-filter into 48be609 on master.

@jacogr
Copy link
Contributor

jacogr commented Sep 19, 2016

From a documentation and use perspective, is the API (where 25 indicates the limit) -

eth_newFilter({
  from: 'earliest',
  to: 'latest',
}, 25)

or

eth_newFilter({
  from: 'earliest',
  to: 'latest',
  limit: 25
})

@tomusdrw
Copy link
Collaborator Author

Hmm, currently it's the first option (additional parameter), I guess the latter one is much better though.
I'll update the PR.

@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 20, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 20, 2016
@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Sep 20, 2016

Fixed the API, limit is now part of the Filter object. Complete reference:

eth_getLogs({ from: 'earliest', limit: 10 }) // returns 10 last logs
let filterId = eth_newFilter({
  from: 'earliest',
  to: 'latest',
  limit: 25
})
eth_getFilterChanges(filterId) // returns changes, but at most 25 last logs
eth_getFilterLogs(filterId) // returns logs matching filter, but at most 25 last logs

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 85.166% when pulling db43462 on limit-filter into 48be609 on master.

@@ -59,7 +65,8 @@ impl Clone for Filter {
from_block: self.from_block.clone(),
to_block: self.to_block.clone(),
address: self.address.clone(),
topics: topics[..].to_vec()
topics: topics[..].to_vec(),
Copy link
Contributor

@rphmeier rphmeier Sep 21, 2016

Choose a reason for hiding this comment

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

side note: seems that topics only has four members -- why bother allocating a Vec at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, it can contain up to 4 entries - but that's true, we could substitute this with array of 4 Option. I will prepare a separate PR.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 21, 2016
@gavofyork gavofyork merged commit 8c111da into master Sep 21, 2016
@gavofyork gavofyork deleted the limit-filter branch September 21, 2016 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants