-
Notifications
You must be signed in to change notification settings - Fork 31
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 dashboard census-feature parity across beacon, history, state networks #313
Conversation
57c54d1
to
68b0f7c
Compare
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 very good. I did find a bug! But it is only a 1 line difference to fix it!
This PR doesn't appear to update audit
data depending on the selector, but as I said on discord we can always do that in the next PR, This PR already makes a lot of progress and is very helpful.
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!
We already talked about this on the call, but I'll just leave it for posterity:
Next time you move a big chunk of code (say >hundred lines) from one place to another, would you mind doing it in a separate refactor commit? I find it much harder to review when there is a move & change at the same time: I can't use the diff to see what changed from the previous implementation. I ended up doing it myself here, which made it so much easier to review the meaningful changes in routes.rs.
Glados currently only gathers and displays census data for the history network. This PR:
?network=
on the backend.Bonus: continues the migration toward raw SQL and simplifies the calls for readability and maintenance purposes.
Implements #293 for census data.
Note that all audit data will still show history, which is a bit awkward (history audit data displayed next to beacon/state census data) but it will toggle audit data per network in the next PR.