-
Notifications
You must be signed in to change notification settings - Fork 871
Add new HTTP endpoint to get supply for a given state root #7102
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
base: unstable
Are you sure you want to change the base?
Conversation
a0bcc5f
to
2ceb554
Compare
Looks like this needs a sneaky |
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.
Thanks for the PR! This will be a neat endpoint to have. Just a couple of minor things
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.
Looks like it needs a make mdlint
to fix the Markdown formatting.
Also noticed a typo (we spellcheck the docs so this would've got picked up by CI as well)
For Markdown linting, I took the liberty of moving the check earlier in the test process as it's fast to run. In this MR, the suite failed nearly 15mins into testing when it could have failed much earlier. |
Nice, that's a good change! |
@macladson I have no idea why the windows release tests failed, are they known to be flaky or could my changes somehow be the cause? |
Not sure what happened but I retriggered the tests and it has passed now, must've just been flaky |
Hi @alecalve thanks for all your work so far! I was wondering if you'd be up for writing a test case for this endpoint? If not, I can go ahead and push up a test case myself. Thanks! |
I'll give it a try! |
@alecalve this looks great! There are two small lint issues that are blocking CI. If you run |
I wasn't aware of this final step of linting, I only ran
|
Some required checks have failed. Could you please take a look @alecalve? 🙏 |
Some required checks have failed. Could you please take a look @alecalve? 🙏 |
Issue Addressed
Computing how much ETH is on the Consensus layer requires currently querying
/eth/v1/beacon/states/{state_id}/validator_balances
and summing up all balances of all validators.That data is 68MB of JSON which has to be downloaded and processed.
Proposed Changes
This PR instead adds a simple enpoint where the computation is done at the node level, the returned data is therefore minimal.