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

Don't fetch internal transactions for simple token transfers #1305

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

KronicDeth
Copy link
Contributor

Resolves #1303

Changelog

Enhancements

  • Don't fetch internal transactions for transactions that have token transfers from the logs if the status is :ok and the transaction did not create a contract.

@KronicDeth KronicDeth added enhancement New feature or request team: architecture labels Jan 4, 2019
@KronicDeth KronicDeth self-assigned this Jan 4, 2019
@ghost ghost added the in progress label Jan 4, 2019
@KronicDeth KronicDeth added the ready for review This PR is ready for reviews. label Jan 4, 2019
@acravenho
Copy link
Contributor

A couple cleanup items I noticed:

warning: variable "changes" is unused
  lib/explorer/chain/import/runner/transactions.ex:178

warning: variable "hash" is unused
  lib/indexer/block/realtime/fetcher.ex:337

@KronicDeth
Copy link
Contributor Author

@acravenho fixed those already in the force push right before your comment.

@acravenho
Copy link
Contributor

These are the results after indexing Goerli on 1303 and then on master. It appears the catchup indexer is indexing everything.

Build Total Transactions Internal Transactions
master 21,101 4,795
1303 21,101 25,201

@KronicDeth
Copy link
Contributor Author

KronicDeth commented Jan 4, 2019

@acravenho 2811d6c contains a fix where one of the returns was inverted due to the realtime logic returning the opposite value from the importer, since one is deciding whether to fetch and one is decide whether to mark them as already fetched. Please test against Goerli again with this version.

@coveralls
Copy link

coveralls commented Jan 4, 2019

Pull Request Test Coverage Report for Build 19cce54d-5d7d-4a4f-9125-9dc4930ed85d

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 85.778%

Totals Coverage Status
Change from base Build c9610129-db26-419f-82e7-1a8006ee2878: 0.03%
Covered Lines: 3836
Relevant Lines: 4472

💛 - Coveralls

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.

Looks great!!! 2091 compared to 25,201 prior to the new rules.

Resolves #1303

Don't fetch internal transactions for transactions that have token
transfers from the logs if the status is `:ok` and the transaction did
not create a contract.
@KronicDeth KronicDeth merged commit 553decc into master Jan 4, 2019
@ghost ghost removed the in progress label Jan 4, 2019
@KronicDeth KronicDeth deleted the 1303 branch January 4, 2019 18:37
ayrat555 added a commit that referenced this pull request Oct 4, 2019
it was disabled in #1305.
Note that it's not applicable for parity. Because we fetch internal
transactions for it block by block so all internal transactions are
fetched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review This PR is ready for reviews. team: architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants