-
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 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
Quality Gate passedIssues Measures |
Test Results 21 files - 2 297 suites - 52 48m 40s ⏱️ -32s For more details on these failures, see this check. Results for commit b840444. ± Comparison against base commit 13d09e9. This pull request removes 4 tests.
♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3342 +/- ##
==========================================
- Coverage 85.04% 84.96% -0.09%
==========================================
Files 65 69 +4
Lines 4508 4688 +180
Branches 1023 1050 +27
==========================================
+ Hits 3834 3983 +149
- Misses 330 393 +63
+ Partials 344 312 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
@@ -0,0 +1,160 @@ | |||
# Design Document: ERC20/ERC721/ERC1155 Token Transfer Events API, Tokens Owned by an Address |
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 would name it Block Explorer API Support
# 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. |
- Match Etherscan's response format for easy integration | ||
- Maintain performance with large datasets | ||
- Provide endpoints to fetch tokens owned by an address | ||
|
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.
Add Non-Goals, noting that complete API support of existing EthereScan and BlockScout APIs.
As we expand we'll address gaps
|
||
## `getTokenTransfers` | ||
|
||
### 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.
This should be a new package not under relay.
We want it to be a separate deployable service so that it can be scaled separately from the relay and not impact the core APIs
If not the alternative is make endpoint support configureable so that a deployemnt can decide to support explorer APIs only or along with core APIs
### 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
- 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
|
||
### 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.
Good start waiting on a bit more structure
Description:
Related issue(s):
Fixes #3340
Notes for reviewer: