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

fix: add fields for contract filter performance #1966

Merged
merged 1 commit into from
May 23, 2019

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented May 16, 2019

Resolves #1951

Changelog

Bug Fixes

  • Due to poor performance with the filters for not decompiled and not verified smart contracts in the JSON RPC, this adds supporting fields on addresses to signify decompiled/verified contract addresses.
  • Adds a Temporary fetcher that will backfill the old data.

Incompatible Changes

  • For performance reasons, I had to remove the "DecompilerVersion" attribute from the API response for listing contracts. I can add it back in if necessary, but things will become much slower. Since this was a new API endpoint, I don't think it would be too bad to remove that field. Let me know.

Checklist for your PR

  • I added an entry to CHANGELOG.md with this PR
  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a regression test to prevent the bug from silently reappearing again.
  • I checked whether I should update the docs and did so if necessary

@ghost ghost assigned zachdaniel May 16, 2019
@ghost ghost added the in progress label May 16, 2019
@zachdaniel zachdaniel force-pushed the contract-filter-performance branch from 8cdc73c to d1a4f76 Compare May 16, 2019 17:49
@coveralls
Copy link

coveralls commented May 16, 2019

Pull Request Test Coverage Report for Build 240d86fb-4f10-4437-8b44-89a58d69cd0b

  • 14 of 22 (63.64%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 81.062%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/explorer/lib/explorer/chain.ex 6 14 42.86%
Files with Coverage Reduction New Missed Lines %
apps/block_scout_web/lib/block_scout_web/views/api/rpc/contract_view.ex 2 88.46%
Totals Coverage Status
Change from base Build 6e7bcf0a-1460-46ab-87e4-0e3dae4d7eb0: -0.06%
Covered Lines: 4717
Relevant Lines: 5819

💛 - Coveralls

@zachdaniel zachdaniel force-pushed the contract-filter-performance branch from d1a4f76 to 4781505 Compare May 16, 2019 18:05
@zachdaniel zachdaniel force-pushed the contract-filter-performance branch 7 times, most recently from 69927ca to 6fcaf81 Compare May 17, 2019 07:12
Copy link
Contributor

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

a couple of comments about testing. feel free to ignore them

@zachdaniel zachdaniel force-pushed the contract-filter-performance branch 6 times, most recently from c664260 to 5613799 Compare May 20, 2019 14:49
vbaranov added a commit that referenced this pull request May 20, 2019
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.

@zachdaniel thanks for the fix. I've installed it to stg server.

I've tested it by preliminary have been deleting indices addresses_decompiled_index, addresses_verified_index and, at least, this endpoint became to work almost instantly with offset 50 - 90. For example, /api?module=contract&action=listcontracts&offset=50&filter=4. Though, it still returns "504 gateway timeout for the limit >= 100".

As it works even without those indices (though, with a small limit of entries in the response), I would suggest to remove them from PR, because of those indices tend to increase write time into addresses table, thus increasing the number of connections to DB.

@zachdaniel zachdaniel force-pushed the contract-filter-performance branch 2 times, most recently from 5e83e71 to 0968cbd Compare May 22, 2019 05:05
@zachdaniel
Copy link
Contributor Author

@vbaranov Alright, I removed the temporary fetcher from this PR, so that it can safely be merged. I also double checked, and the data is fully migrated. There are no decompiled_smart_contracts that aren't marked as decompiled=TRUE in the address table, and all decompiled_smart_contracts are marked as imported=TRUE. When we're sure we are done with it, we can drop the imported field from decompiled_smart_contracts.

@zachdaniel zachdaniel force-pushed the contract-filter-performance branch 3 times, most recently from 9a355c9 to 9cb3c94 Compare May 22, 2019 17:30
@zachdaniel zachdaniel force-pushed the contract-filter-performance branch from 9cb3c94 to 465af2f Compare May 22, 2019 19:18
@zachdaniel zachdaniel force-pushed the contract-filter-performance branch from 465af2f to d2336bc Compare May 22, 2019 19:18
@vbaranov vbaranov self-requested a review May 23, 2019 06:30
@vbaranov vbaranov merged commit 8ed8640 into master May 23, 2019
@vbaranov vbaranov deleted the contract-filter-performance branch May 28, 2019 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listing of not-yet-decompiled contracts seems to stall with 504/502 occasionally
5 participants