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

feat: add listcontracts endpoint #1608

Merged
merged 1 commit into from
Mar 27, 2019
Merged

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented Mar 19, 2019

The first part of #1569

This adds an rpc endpoint to list contracts according to the spec on that issue. We will probably want to leave this open until #1596 is merged, so that I can add those filters to this PR

Changelog

Enhancements

  • Adds a listcontracts action to the contracts rpc endpoint. See the attached issue for more information
  • Adds a field Address to the contract model, to support the list endpoint.
  • Modularize some of the pagination code so that you can set one of the pagination parameters without having to set both.

@ghost ghost assigned zachdaniel Mar 19, 2019
@ghost ghost added the in progress label Mar 19, 2019
@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch from 18c55de to 0bbb850 Compare March 19, 2019 19:10
@coveralls
Copy link

coveralls commented Mar 19, 2019

Pull Request Test Coverage Report for Build 650c2e71-7717-4778-b171-ee1718c2e0cd

  • 56 of 68 (82.35%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 83.223%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/block_scout_web/lib/block_scout_web/controllers/api/rpc/contract_controller.ex 19 20 95.0%
apps/explorer/lib/explorer/chain/smart_contract.ex 0 1 0.0%
apps/explorer/lib/explorer/chain.ex 0 10 0.0%
Totals Coverage Status
Change from base Build fa78c39f-3560-4dee-ae87-834dbc4dd4c2: 0.09%
Covered Lines: 4390
Relevant Lines: 5275

💛 - Coveralls

@ghost ghost assigned ayrat555 Mar 20, 2019
@ayrat555 ayrat555 removed their assignment Mar 20, 2019
},
%{
code: "200",
description: "error",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't understand this example. error with status code 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, this is just the error description that everything uses. I don't know why, but it returns a 200 code but its an error everywhere in the rpc :(

@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch 3 times, most recently from 895a561 to bf50b9c Compare March 20, 2019 14:11
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 Could you please add a screenshot of how will API endpoint look on the APIs page? And please update CHNAGELOG.md

@zachdaniel
Copy link
Contributor Author

Screen Shot 2019-03-21 at 09 24 50

Screen Shot 2019-03-21 at 09 25 02

@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch from bf50b9c to e285dae Compare March 21, 2019 13:25
@ghost ghost assigned vbaranov Mar 25, 2019
@vbaranov
Copy link
Member

@zachdaniel #1596 has been merged

@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch 2 times, most recently from a2c98d5 to cfb4647 Compare March 25, 2019 18:37
@vbaranov vbaranov self-requested a review March 26, 2019 09:56
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

I've tried it locally with Sokol. I've found some bugs. I don't know, would you continue to fix them in this PR or you will do this in a separate one where you will implement #1569 (comment). Please, let me know. The list of the bugs is below:

  1. I think we could extend the output data model with the decompiler version.
  2. &filter=unverified or &filter=not_decompiled - Not only smart-contracts are returned, but also simple address without contact code. I suppose we should return only smart-contracts for all filters in this endpoint

@vbaranov
Copy link
Member

@zachdaniel in addition, If I choose incorrect filter (which is unlisted) it returns me all addresses instead of returning of empty list

@zachdaniel
Copy link
Contributor Author

The pattern in the RPC that I could see was to ignore parameters that were invalid (not my preferred way of doing it, just what was being done before. I would be fine with returning an error response instead of just returning all results, but I wouldn't want to return an empty list. Also, yeah this should definitely only have addresses with contract_code. I'll fix these and add tests for them.

@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch 3 times, most recently from 11b3c7c to 4d5b42b Compare March 26, 2019 15:48
@zachdaniel zachdaniel force-pushed the add-list-smart-contracts-endpoint branch from 4d5b42b to e694d14 Compare March 26, 2019 16:04
@vbaranov vbaranov self-requested a review March 26, 2019 18:53
@vbaranov vbaranov merged commit 4f0671d into master Mar 27, 2019
@ghost ghost removed the in progress label Mar 27, 2019
@vbaranov vbaranov deleted the add-list-smart-contracts-endpoint branch March 27, 2019 13:56
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.

None yet

5 participants