Notify banned solvers #3262
Notify banned solvers #3262
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
# Conflicts: # crates/autopilot/src/domain/competition/participation_guard/db.rs # crates/autopilot/src/domain/competition/participation_guard/mod.rs # crates/autopilot/src/run.rs
# Conflicts: # crates/autopilot/src/domain/competition/participation_guard/db.rs
squadgazzz
force-pushed
the
notify-banned-solvers
branch
from
640f68f to
43e650f
Compare
| @@ -106,6 +112,28 @@ impl Validator { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| /// Try to notify all the non-settling solvers in a background task. | |||
| fn notify_solvers(non_settling_drivers: &[Arc<infra::Driver>]) { | |||
There was a problem hiding this comment.
Is this the best place to notify? (db.rs file)
There was a problem hiding this comment.
Agree, domain should only be aware of the call:
self_.0.infra.notify()
while the implementation should be inside infra
| tokio::task::spawn(future.in_current_span()); | ||
| } | ||
|
|
||
| pub async fn notify( |
There was a problem hiding this comment.
why two functions with the same name but one public and the other private? is the public one really needed? 🤔
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error("Unable to notify solver")] | ||
| UnableToNotify, |
There was a problem hiding this comment.
Maybe it is worth to add some context information like solver address or name?
| #[derive(Debug)] | ||
| pub enum BanReason { | ||
| /// The driver won multiple consecutive auctions but never settled them. | ||
| UnsettledConsecutiveAuctions, |
There was a problem hiding this comment.
Shouldn't we differentiate the ban reason between non-settled and low-settled rate?
There was a problem hiding this comment.
| @@ -106,6 +112,28 @@ impl Validator { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| /// Try to notify all the non-settling solvers in a background task. | |||
| fn notify_solvers(non_settling_drivers: &[Arc<infra::Driver>]) { | |||
There was a problem hiding this comment.
Agree, domain should only be aware of the call:
self_.0.infra.notify()
while the implementation should be inside infra
| fn from(value: NotifyRequest) -> Self { | ||
| match value { | ||
| NotifyRequest::UnsettledConsecutiveAuctions => { | ||
| notify::Kind::Banned(notify::BanReason::UnsettledConsecutiveAuctions) |
There was a problem hiding this comment.
nit improvement proposal: As a solver I would be interested in knowing the block number when i will be enabled again.
| req: axum::Json<dto::NotifyRequest>, | ||
| ) -> Result<hyper::StatusCode, (hyper::StatusCode, axum::Json<Error>)> { | ||
| let solver = &state.solver().name().0; | ||
| tracing::trace!(?req, ?solver, "received a notification"); |
There was a problem hiding this comment.
nit: probably ok to set it to debug as we don't start with a lot of notifications.
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { |
There was a problem hiding this comment.
nit: Personally I would be ok with omitting completely the error case for notify endpoint (be consistent with how the driver propagates it to solvers) as we always considered this endpoint not too important for system to work and we were not interested to act on failure of notification.
This is a follow-up to #3257, which was requested in #3257 (comment). Implements solver notification mechanism in case the driver is banned. Reroutes the notification request to the solver through the internal driver. External solvers need to be communicated about this change.