(fast5) extract /settle handling into separate component - #4860
(fast5) extract /settle handling into separate component#4860MartinquaXD wants to merge 1 commit into
/settle handling into separate component#4860Conversation
|
Claude finished @MartinquaXD's task in 1m 9s —— View job PR review: extract
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Sends a `/settle` request to `driver` and returns the tx id of the | ||
| /// resulting settlement once it is mined (or an error if the submission | ||
| /// deadline was crossed first). | ||
| pub async fn settle( |
There was a problem hiding this comment.
The old settle in run_loop.rs carried #[instrument(skip_all, fields(driver = driver.name, solution_uid))], which put driver and solution_uid on the span so nested logs (including wait_for_settlement_transaction's own span) inherited them. This attribute wasn't ported to the coordinator's settle, so those fields drop out of the tracing context for the whole settle path — a subtle change for a PR that's meant to be behavior-preserving.
Consider re-adding it:
| pub async fn settle( | |
| #[instrument(skip_all, fields(driver = driver.name, solution_uid))] | |
| pub async fn settle( |
MartinquaXD
changed the title
/settle handling into separate component/settle handling into separate component
Moves `settle`, `wait_for_settlement_transaction`, `store_execution_started/ended`, and the `SettleError` enum out of `RunLoop` into a new `run_loop::settle_call_coordinator` submodule. `RunLoop` holds an `Arc<SettleCallCoordinator>` built once in `new`; `start_settlement_execution` calls `self.settle_coordinator.settle(...)` with the same `settle::Request` it always built. Pure refactor — no behavior change — so the fast-path handler in a later PR can reuse the submission machinery without ballooning that PR. Signed-off-by: MartinquaXD <martin@cow.fi>
42aae50 to
a0e0106
Compare
| maintenance: MaintenanceSync, | ||
| /// How long we wait for the driver to signal the settlement completed | ||
| /// before considering the call timed out. | ||
| max_settlement_transaction_wait: Duration, |
There was a problem hiding this comment.
I was surprised to see this as a global parameter (I confused it with the deadline). Digging into it, it looks like we currently allow solvers 1 minute on all chains to process the /settle which seems way too long and a potential DOS vector. Why don't we use our standard HTTP timeout?
Unrelated to this PR, but possibly a quick fix to do on the side?
There was a problem hiding this comment.
Isn't the endpoint supposed to return only when the settlement was made? Then we can't change to the default 10s, at least on mainnet since we're allowing up to 3 blocks delay for the settlement
| tracing::info!(driver = %driver_.name, solution = %solution_id, "settling"); | ||
| let submission_start = Instant::now(); | ||
|
|
||
| let request = settle::Request { |
There was a problem hiding this comment.
nanonit, would move request construction into the coordinator to avoid the settle dependency in runloop altogether.
| @@ -0,0 +1,217 @@ | |||
| //! Coordinates a `/settle` call against a driver: enforces the block-based | |||
There was a problem hiding this comment.
Definitely a big fan of this refactor. Reducing the 1k+ line runloop.rs beast is nice. We should probably do the same for the other parts of the runloop.
I just wonder about where those components should live? Are they part of the domain? I also think we might be able to make the names a little bit more "sexy" (I'm not a fan of yet another Coordinator 🧑
maybe domain::settle.rs and calling the struct Stage and the public function call. On the runloop struct it would then be settle: Arc<settle::Stage> and in the code settle::call(...).
But not sure what our current "taste" for clean code is.
There was a problem hiding this comment.
There's definitely a will but you're hitting one of the hardest problems in CS — picking names 😆
The stage makes sense IMO, but it would make more if we slowly start moving towards it, it would make the codebase slightly easier to understand, but if we "half-do it" it's worse in the long term
3 participants
Description
The autopilot will soon monitor new orders and initiate the fast path for orders that need it. For that it would be very nice if we can just reuse the existing settle logic of the runloop. That way we get the driver communication, storing order events, monitoring solutions, logging, etc. for free.
Changes
Extract settle handling related code into
SettleCallCoordinatorand use it in the runloop.How to test
just a refactor - should not change any behavior