-
Notifications
You must be signed in to change notification settings - Fork 188
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
Adds operator ejections dataapi endpoint #810
Conversation
97a2e2d
to
9b8a1d7
Compare
3591ccf
to
3dfbef9
Compare
b273283
to
5a7683d
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.
It will be useful to also return whether the ejection hit rate limiting which is logged as well cc @0x0aa0
// @Summary Fetch list of operator ejections over last N days. | ||
// @Tags OperatorsInfo | ||
// @Produce json | ||
// @Param days query int false "Lookback in days [default: 1]" |
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.
Might be more helpful passing hours
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 think days is preferable as it doesn't require math to query weekly/monthey/quarterly ejections and the typical cadence of ejections is <= 1 per day (mainnet had 3 ejections over last 7 days. testnet had 6 ejections over last 7 days).
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.
It can be any number of times per day. And the query can be interested in a number of hours.
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.
Whats the use case where you only want ejections in the last hour, but don't want ejections from the previous 24hr, and you are unable to filter results by block timestamp?
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.
It's all possible, but just adding friction
// @Produce json | ||
// @Param days query int false "Lookback in days [default: 1]" | ||
// @Param operator_id query string false "Operator ID filter [default: all operators]" | ||
// @Param first query int false "Return first N ejections [default: 1000]" |
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'm not sure we want to expose these two at API, they can be internal controls
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.
The reason subgraph has these is to force pagination on the caller to limit response from getting too big. I think we should pass pagination onto our callers as well. We shouldn't do pagination for them and thus return a huge response payload.
I considered setting internal limits and not exposing these, but it seems like we should allow people to find every ejection that has ever occurred - in which case we should just provide them the tools to do it.
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 deviates from the existing API norms and creates inconsistent API experience.
Adding more parameters also complicates things, even this implementation of pagination is not quite correct (a second call will is not strictly relative to the first one). I would prefer a simpler semantics with a caveat that it'll provide at most MIN{N, pastNumHoursWindow} items.
operator_id
can be specified to limit ejection query to specific operator.Checks