feat(host,tasks): complete aggregation api #387
Conversation
Write in-memory implementation Stub out sqlite implementation
petarvujovic98
temporarily deployed
to
test-environment
GitHub Actions
Inactive
petarvujovic98
temporarily deployed
to
test-environment
GitHub Actions
Inactive
| if aggregation_request.proofs.is_empty() { | ||
| return Err(anyhow::anyhow!("No proofs provided").into()); | ||
| } |
There was a problem hiding this comment.
IMO, we don't have to check the proofs.
There was a problem hiding this comment.
maybe update the error message?? as if proofs vec is empty, the aggregation request is also invalid, no need to cancel any more.
| let proof_type = ProofType::from_str( | ||
| aggregation_request | ||
| .proof_type | ||
| .as_deref() | ||
| .unwrap_or_default(), | ||
| )?; | ||
| inc_host_req_count(0); | ||
| inc_guest_req_count(&proof_type, 0); |
There was a problem hiding this comment.
To maintain consistency with the 'cancel' API across versions 1 and 2, I recommend removing these two lines of code, as we don't record metrics for 'cancel' API (at least not for now, maybe we can add some metrics for 'cancel' API)
There was a problem hiding this comment.
LGTM. also the block id based metrics should be upgraded/refactoried, let's do it later.
|
|
||
| prover_state | ||
| .task_channel | ||
| .try_send(Message::CancelAggregate(aggregation_request.clone()))?; |
There was a problem hiding this comment.
The cancelling aggregation request may be non-existing. In order to prevent useless 'cancel' requests, we should check the status of the target aggregation request by get_aggregation_task_status() before sending signal to task_channel.
All of theses changes were commentted and discussed at taikoxyz#387 1. Remove checking of empty proof of Aggregation Cancel request. For this case, querying task status will return None, and we will response with NotFound error directly. See also taikoxyz#387 (comment) 2. Remove metrics recording to maintain consistency with v1 and v2 See also taikoxyz#387 (comment) 3. Check task status before signal cancel to prover
* feat(host): complete the aggregation API * feat(tasks): complete aggregation task APIs Write in-memory implementation Stub out sqlite implementation * feat(host): update v3 Aggregation Cancel API All of theses changes were commentted and discussed at #387 1. Remove checking of empty proof of Aggregation Cancel request. For this case, querying task status will return None, and we will response with NotFound error directly. See also #387 (comment) 2. Remove metrics recording to maintain consistency with v1 and v2 See also #387 (comment) 3. Check task status before signal cancel to prover --------- Co-authored-by: Petar Vujovic <[email protected]> Co-authored-by: smtmfft <[email protected]>
No description provided.