Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
feat: Adds draft design doc for new block explorer REST API endpoints #3342
Changes from 1 commit
b840444
f81f789
800e769
f1be973
09f5fa8
e6f883c
17b8254
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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:
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
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
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 : )
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.