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

Fetch TokenBalances values in Realtime and Catchup Indexers #583

Merged
merged 6 commits into from
Aug 29, 2018

Conversation

igorffs
Copy link
Contributor

@igorffs igorffs commented Aug 21, 2018

#514

Motivation

This PR makes the Indexers to fetch the TokenBalances value and value_fetched_at both in their own way:

  • The Realtime indexer fetches the data synchronously
  • The Catchup indexer fetches the data assynchronously

WIP

  • The values are already fetching the values after we've tested it local, but the code still needs some improvements and tests.

@igorffs igorffs added the wip label Aug 21, 2018
@igorffs igorffs mentioned this pull request Aug 21, 2018
@feliperenan feliperenan force-pushed the iff-index-token-balances branch 2 times, most recently from 68bd356 to 31a6756 Compare August 21, 2018 23:16
@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from 9222b21 to 7fd5768 Compare August 22, 2018 14:30
from(
tb in TokenBalance,
where: is_nil(tb.value_fetched_at),
select: %TokenBalance{
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we want to send the TokenBalance struct, I think we don't need to select the specific fields. Even we don't need the whole TokenBalance, IMO sending the all attributes it's not a problem at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have changed it already. Please, take a look and see if it makes sense to you as well.

cc @igorffs

@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from dd232fe to 3d26a79 Compare August 22, 2018 19:23
@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from 0d42fff to 60bc01d Compare August 22, 2018 20:30
params = [%{to: address, data: data}]
request(%{id: id, method: "eth_call", params: params})
end

# Block.block_number()
Copy link
Contributor

Choose a reason for hiding this comment

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

✂️

@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from 02cb530 to 900bb22 Compare August 23, 2018 12:58
@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from 900bb22 to 2cb5ba9 Compare August 23, 2018 13:03
@KronicDeth
Copy link
Contributor

Rebased on iff-index-token-balances after iff-index-token-balances was rebased on master

@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch 2 times, most recently from 17bc6c8 to 49226a1 Compare August 23, 2018 13:21
@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from 49226a1 to 47cd800 Compare August 23, 2018 13:27
@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from 47cd800 to e83209c Compare August 23, 2018 13:40
@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from e83209c to a7b9272 Compare August 23, 2018 13:49
@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from c8bd5b5 to 09752d3 Compare August 23, 2018 23:15
@KronicDeth
Copy link
Contributor

KronicDeth commented Aug 23, 2018

Changed to be against master now that the old base's PR ( #582) has been merged to master

@KronicDeth KronicDeth changed the base branch from iff-index-token-balances to master August 23, 2018 23:16
@feliperenan feliperenan added the ready for review This PR is ready for reviews. label Aug 24, 2018
@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from 022a515 to 0a5b735 Compare August 27, 2018 13:44
@feliperenan
Copy link
Contributor

Hey @KronicDeth, I applied your suggestions. Could please you take a look again? 🙏

@KronicDeth
Copy link
Contributor

@feliperenan you fixed apps/explorer/config/dev/parity.exs, but missed the duplicate change in apps/explorer/config/prod/parity.exs

@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from 0a5b735 to c765560 Compare August 27, 2018 14:45
@feliperenan
Copy link
Contributor

@KronicDeth fixed.

Copy link
Contributor

@acravenho acravenho left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Great work!

@feliperenan
Copy link
Contributor

@gfreh This one is ready for QA :).

Copy link
Contributor

@alexgaribay alexgaribay left a comment

Choose a reason for hiding this comment

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

Looks good 👍 !

@KronicDeth
Copy link
Contributor

If you run into ON CONFLICT DO UPDATE command cannot affect row a second time with insert_token_balances in the stacktrace while running this locally or testing, you're hitting #614 and there's a fix in #619.

@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from fdf5d8c to 59c60e3 Compare August 28, 2018 17:28
@coveralls
Copy link

coveralls commented Aug 28, 2018

Pull Request Test Coverage Report for Build b05dc011-82e2-4f18-9dce-289f9ebf9e11

  • 15 of 15 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 89.769%

Totals Coverage Status
Change from base Build 0e13a1c6-ee6d-4286-a4ba-f630d5908717: 0.2%
Covered Lines: 1711
Relevant Lines: 1906

💛 - Coveralls

@KronicDeth
Copy link
Contributor

The new lines added to execute_contract_function and build_eth_call_payload are uncovered

screen shot 2018-08-28 at 12 53 10 pm

The old lines for build_eth_call_payload are also uncovered, so you'll likely need to add tests for build_eth_call_payload in general since we missed the coverage while coveralls.io reporting was broken.

@KronicDeth KronicDeth force-pushed the iff-frg-fetch-token-balances-values branch from 59c60e3 to ad50a21 Compare August 28, 2018 18:00
@feliperenan feliperenan force-pushed the iff-frg-fetch-token-balances-values branch from 4277b53 to e4a9463 Compare August 28, 2018 18:52
@acravenho
Copy link
Contributor

@igorffs @feliperenan Is there any further work that needs to be completed on this PR? If not I would like to get this merged asap. We have a DApp that needs this data today through an API endpoint.

@feliperenan feliperenan merged commit 196f298 into master Aug 29, 2018
@feliperenan
Copy link
Contributor

@acravenho done!, we are just doing some tests here 😅.

@acravenho
Copy link
Contributor

@feliperenan Thank you for the quick work 👍

@sabondano This has now been merged which was a previous blocker to the tokenbalance API endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR is ready for reviews. team: developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants