-
Notifications
You must be signed in to change notification settings - Fork 75
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: Adds draft design doc for new block explorer REST API endpoints #3342
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Konstantina Blazhukova <[email protected]>
7088d1c
to
b840444
Compare
Test Results 23 files - 3 352 suites - 55 47m 8s ⏱️ - 23m 38s For more details on these failures, see this check. Results for commit 17b8254. ± Comparison against base commit 7b12e74. This pull request removes 4 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start.
I think we have to think through the way in which this would deploy and scale a bit more.
docs/design/additional-endpoints.md
Outdated
# Design Document: ERC20/ERC721/ERC1155 Token Transfer Events API, Tokens Owned by an Address | ||
|
||
## Overview | ||
- This document outlines the design for implementing new endpoints allowing exposure of more EVM centric data, similar to Etherscan's and BlockScout's APIs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This document outlines the design for implementing new endpoints allowing exposure of more EVM centric data, similar to Etherscan's and BlockScout's APIs. | |
- This document outlines the design for implementing new endpoints allowing exposure of more EVM centric data that support Block Explorer (e.g. Etherscan, BlockScout) API needs. |
docs/design/additional-endpoints.md
Outdated
### Overview | ||
|
||
Introduce two new endpoints: | ||
- `getTokenTransfers` - to fetch token transfer events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: what are the params?
You should move the params explanation lower to here.
Also note what's mandatory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i think in the overview its enough to state the format of the endpoint and the detailed explanation down where it is, do you think thats fine or should I move it all to this section?
Also I have chosen this classic REST format, which differentiates from the way etherscan and blockscout has implemented their endpoints, entirely with query parameters, not sure if thats gonna affect the user. Do they expect the exactly same format
In etherscan the ednpoints look like this:
GET /api
?module=account
&action=tokentx
&contractaddress=0x9f8f72aa9304c8b593d555f12ef6589cc3a579a2
&address=0x4e83362442b8d1bec281594cea3050c8eb01311c
&page=1
&offset=100
&startblock=0
&endblock=27025780
&sort=asc
&apikey=YourApiKeyToken
docs/design/additional-endpoints.md
Outdated
- Test address matching | ||
- Test token standard detection | ||
|
||
2. **Acceptance Tests** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acceptance tests should be in teh form of E2E features a User would go through with a focus on the input and the outputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edited them : )
docs/design/additional-endpoints.md
Outdated
|
||
### Dependencies | ||
|
||
- ERC registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so, please expand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this. Please expand on how ERC Registry is involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start waiting on a bit more structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great process! Many questions and suggestions
docs/design/additional-endpoints.md
Outdated
|
||
### Dependencies | ||
|
||
- ERC registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this. Please expand on how ERC Registry is involved.
bc9bfcc
to
441d39b
Compare
Signed-off-by: Konstantina Blazhukova <[email protected]>
441d39b
to
f81f789
Compare
Signed-off-by: Konstantina Blazhukova <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start, looking forward to discussing more
|
||
#### 4. CacheService | ||
Redis-based caching service: | ||
- Caches frequent queries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call out the cached objects. I'd like to ensure that we only cache what is needed e.g. in some cases the relay caches the full account or token object but we really only need a few fields from the mirror node response.
In this case do we need to cache a full log or can we cache only a subset of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we dont need to cache the full object, so I changed and specified what we will need
|
||
#### 4. CacheService | ||
Redis-based caching service: | ||
- Caches frequent queries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suggest an TTL for these caches components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/design/additional-endpoints.md
Outdated
|
||
### Performance Considerations | ||
|
||
1. Possibly restrict the number of logs returned by the MN to a maximum of 10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have a max on the logs endpoint we can likely match that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, investigating this. Seems like its 100 so added this as a value @Nana-EC
docs/design/additional-endpoints.md
Outdated
packages/ | ||
rest-api/ | ||
└── src/ | ||
├── config/ # Configuration for REST API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a section for this, could you add one that calls out which config fields you plan to add and what their defaults might be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one :) Its a starting point tho, maybe something comes up later during implementation
docs/design/additional-endpoints.md
Outdated
### Package Structure | ||
``` | ||
packages/ | ||
rest-api/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, another approach would be rest-server
package with the controllers under there and the services under the relay
package or this new rest-api
package.
In this way the service implementation could be change in the future a transparent way or the underlying server logic - both packages decoupled from each other.
Let's chat about it, the above is food for thought and might hint towards the final form we want to get to in a tentative refactor of the repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add it as a point to discuss in the meeting
docs/design/additional-endpoints.md
Outdated
|
||
### Dependencies | ||
|
||
* ERC registry - As explained in the _Flow_ section, the TokenService will need to fetch the ERC registry to get the token standard (ERC20/721/ 1155) contracts and filter the response by only those matching the standard, wanted in the request. E.g if the request is for ERC20, the TokenService will need to fetch the ERC registry to get the ERC20 contracts and filter the response from the MN by only those matching the standard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should note that initially we'll use the static outputs from the smart contract repo. We should be able to import that package soon but int eh early stage we can copy over the files as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
Signed-off-by: Konstantina Blazhukova <[email protected]>
…ents Signed-off-by: Konstantina Blazhukova <[email protected]>
Signed-off-by: Konstantina Blazhukova <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestions @konstantinabl.
To confirm after some thought we should indeed position this from the get go as an independently deployable instance.
Should be something like http-explorer-server
containing the specific etherscan focused controllers and server logic.
Initially we can colocate the api logic including services in the relay
package then revisit when we pick up the repo refactor
docs/design/additional-endpoints.md
Outdated
|
||
| Endpoint | Description | | ||
|----------|-------------| | ||
| `GET /api?module=account&action=tokentx&address={address}&contractaddress={contractaddress}&startblock={startblock}&endblock={endblock}&page={page}&offset={offset}&sort={asc\|desc}` | Get ERC20 token transfer events | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make the etherscan nature explicit
Somethings like what snowtrace does https://testnet.snowtrace.io/documentation/api-swagger
| `GET /api?module=account&action=tokentx&address={address}&contractaddress={contractaddress}&startblock={startblock}&endblock={endblock}&page={page}&offset={offset}&sort={asc\|desc}` | Get ERC20 token transfer events | | |
| `GET /api/v1/etherscan?module=account&action=tokentx&address={address}&contractaddress={contractaddress}&startblock={startblock}&endblock={endblock}&page={page}&offset={offset}&sort={asc\|desc}` | Get ERC20 token transfer events | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nana-EC I get what you mean, but I think their design is like this because they have different networks available. And will we have something else than etherscan, imo it may be confusing to have it like this, since we may also want to redesign the endpoint format in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why having a unique oath would be preferred. I that way the etherscan support approach can be left alone and a future version that we designed with better REST principles could be added with my conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with Nana.
|
||
Currently MN doesn't support EVM centric queries, like token transfer events for specific standards like ERC20, ERC721, ERC1155 or balance of an address for a specific token. | ||
|
||
## Goals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at
https://api-testnet.snowtrace.io/api?module=contract&action=getsourcecode&address=0x5425890298aed601595a70AB815c96711a31Bc65, a user may use this when looking for ABI
I think this might be another goal to capture. P2 in nature for this story though.
This is very interesting as it may require calling the verification service.
@acuarica do you see any other option to retrieve such information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the verification service is the source of truth when getting the ABI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Love how meticulous this design doc is! Left comments and quetions around
3c2b0ca
to
75de0a2
Compare
Signed-off-by: Konstantina Blazhukova <[email protected]>
75de0a2
to
17b8254
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job. I left some comments
|
||
### Main Implementation Components | ||
|
||
#### 1. TokenController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it makes sense to call it Etherscan token Controller?
an alternative is to create it inside folder called etherscan. There could be other proxy apis in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I name it token controller, since I plan it will be responsible for all the endpoints from the token module but i may put it into etherscan folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine we implement etherscan and snowflake. Same controlles for both, or separated ( folder/ naming)? what will the approach?
Personally Iam used to split to find easierm group things. Whats the usual approach here?
### Performance Considerations | ||
|
||
1. Possibly restrict the number of logs returned by the MN to a maximum of 100 (as is the MN limit) | ||
2. Possibly restrict the block range to a maximum of 10000 blocks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the restrictions at etherscan side?
they say "This API endpoint returns a maximum of 10000 records only." which is lees, because we are talking a about transactions (records)
I suggest to have the same limits if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure what you mean. In etherscan 10000 records limit is for this endpoint, which we are not going to implement. The limit i mentioned here is for this one,
https://docs.blockscout.com/devs/apis/rpc/account#get-token-transfer-events-by-address, where they have limit for 10000 blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see the limit in code? I am not able to see it on the doc. Only
Up to a maximum of 10,000 token transfer events. Also available through the GraphQL token_transfers query.
no really seeing 10k blocks limit
docs/design/additional-endpoints.md
Outdated
|
||
| Endpoint | Description | | ||
|----------|-------------| | ||
| `GET /api?module=account&action=tokentx&address={address}&contractaddress={contractaddress}&startblock={startblock}&endblock={endblock}&page={page}&offset={offset}&sort={asc\|desc}` | Get ERC20 token transfer events | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with Nana.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3342 +/- ##
===========================================
- Coverage 85.30% 62.71% -22.60%
===========================================
Files 69 65 -4
Lines 4688 4535 -153
Branches 1050 1040 -10
===========================================
- Hits 3999 2844 -1155
- Misses 374 1194 +820
- Partials 315 497 +182
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Description:
Creates a design document for the new block explorer REST API endpoints
Related issue(s):
Fixes #3340
Notes for reviewer: