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

paginate eth logs #2366

Merged
merged 21 commits into from
Jul 22, 2019
Merged

paginate eth logs #2366

merged 21 commits into from
Jul 22, 2019

Conversation

ayrat555
Copy link
Contributor

@ayrat555 ayrat555 commented Jul 16, 2019

resolves #2353

Changelog

  • paginate eth logs

@coveralls
Copy link

coveralls commented Jul 16, 2019

Pull Request Test Coverage Report for Build eac09c44-7f16-48e9-ab6b-8a74f692b56a

  • 11 of 94 (11.7%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 79.63%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/block_scout_web/lib/block_scout_web/controllers/api/rpc/eth_controller.ex 2 3 66.67%
apps/explorer/lib/explorer/eth_rpc.ex 0 82 0.0%
Totals Coverage Status
Change from base Build e8dae500-635c-4255-b6c3-0243d5e67b8e: -1.3%
Covered Lines: 5129
Relevant Lines: 6441

💛 - Coveralls

Copy link
Contributor

@pasqu4le pasqu4le left a comment

Choose a reason for hiding this comment

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

LGTM, only a couple nitpick suggestions

apps/explorer/lib/explorer/eth_rpc.ex Outdated Show resolved Hide resolved
apps/explorer/lib/explorer/etherscan/logs.ex Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

  1. It is unclear from the docs is the pagination option is a part of params or pagination params are on the same level as params. If they are a part of params, they should be included in the example
  2. If I try to request this endpoint w/out pagination api endpoint responds to me with the number much less then 1000 logs entries. Try transfer method: 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef
  3. It is unclear should use all 3 params of pagination or could use only one to paginate
  4. which include parameters from the last log received from the previous request. - this is a wrong statement because the previous call includes neither block_number nor transaction_index, log_index. All of them have other names in the response of previous call

@ayrat555
Copy link
Contributor Author

ayrat555 commented Jul 22, 2019

@vbaranov

  1. I added an example with paging options.
  2. Did you include fromBlock, toBlock params?
    try curl -v -X POST --data '{"id": 0, "jsonrpc": "2.0", "method": "eth_getLogs", "params": [{"address": "0xc78Be425090Dbd437532594D12267C5934Cc6c6f", "fromBlock": "earliest", "toBlock": "latest", "topics": ["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"]}]}' "http://localhost:4000/api/eth_rpc"
    response contains exactly 1_000 items
  3. I updated a note
  4. I changed param names to macth json rpc spec

vbaranov added a commit that referenced this pull request Jul 22, 2019
@vbaranov
Copy link
Member

@ayrat555

Did you include fromBlock, toBlock params?

Yes, that is what the issue.

1 - 4 ✔️. One more thing in order to improve pagination UX: is it possible (if no performance concerns) to reorder response in that way, that all three pagination's params would be together? Therefore, the user can copy them in one click. Because currently, a user should copy every parameter to collect them all to make a request for the next page.

@ayrat555
Copy link
Contributor Author

@vbaranov it's possible. but do you think users will use pagination params not programmatically but by hand?

@vbaranov
Copy link
Member

@vbaranov it's possible. but do you think users will use pagination params not programmatically but by hand?

Got your point. It makes sense, that programmatic control is prevalent for API. If API users will have this feature request, let's implement it. But for now, let's leave it as is.

@vbaranov vbaranov self-requested a review July 22, 2019 14:49
@vbaranov vbaranov merged commit d020bd4 into master Jul 22, 2019
@vbaranov vbaranov deleted the ab-paginate-eth-logs branch July 22, 2019 14:50
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.

Pagination for eth_getLogs API endpoint
5 participants