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 3 commits
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.
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.
Hmm, another approach would be
rest-server
package with the controllers under there and the services under therelay
package or this newrest-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
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
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?
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
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
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.
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