Skip to content

Add aggregator toggle API and fix single proof pass-throug#95

Open
bomanaps wants to merge 2 commits intograndinetech:devnet-4from
bomanaps:feat/aggregator-api
Open

Add aggregator toggle API and fix single proof pass-throug#95
bomanaps wants to merge 2 commits intograndinetech:devnet-4from
bomanaps:feat/aggregator-api

Conversation

@bomanaps
Copy link
Copy Markdown
Contributor

Comment thread lean_client/http_api/src/routing.rs Outdated
Comment thread lean_client/http_api/src/aggregator_handlers.rs Outdated
Comment thread lean_client/http_api/src/aggregator_handlers.rs Outdated
Comment thread lean_client/http_api/src/aggregator_handlers.rs Outdated
@ArtiomTr
Copy link
Copy Markdown
Collaborator

also looks like CI fails, as code is not formatted

@bomanaps bomanaps requested a review from ArtiomTr April 22, 2026 05:51
// Runner
// ---------------------------------------------------------------------------

fn run_api_endpoint_test(path: impl AsRef<Path>) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to define helper that is used only in one place - just use it directly in test


fn run_api_endpoint_test(path: impl AsRef<Path>) {
let json_content = fs::read_to_string(path.as_ref())
.unwrap_or_else(|e| panic!("read test vector {}: {}", path.as_ref().display(), e));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just use .expect here

println!("\n{}", test_name);
println!(" {}", case.info.description);

let rt = tokio::runtime::Runtime::new().expect("tokio runtime");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can avoid creating tokio runtime by hand, by using tokio::test macro

Comment on lines +116 to +119
let listener = TcpListener::bind("127.0.0.1:0")
.await
.expect("bind listener");
let addr = listener.local_addr().expect("local addr");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to use real tcp socket for testing - you can just use axum router directly, as shown in their examples, i.e. https://github.com/tokio-rs/axum/blob/de9f13d809e86f8bf24292aac0af16d97ef91195/examples/testing/src/main.rs#L68

}

server_handle.abort();
println!(" \x1b[32m✓ PASS\x1b[0m");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to add formatting / logging to these - the cargo will automatically report results

Comment on lines +96 to +97
println!("\n{}", test_name);
println!(" {}", case.info.description);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably no need to print this info


(
StatusCode::OK,
Json(json!({"is_aggregator": ctrl.is_enabled()})),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if this is also defined as a struct, not via json! macro

/// - `503 Service Unavailable` — controller not wired
pub async fn handle_toggle(
State(ctrl): State<SharedController>,
body: Result<Json<ToggleRequest>, JsonRejection>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably should be this instead:

Suggested change
body: Result<Json<ToggleRequest>, JsonRejection>,
Json(body): Json<ToggleRequest>,

so you don't have to handle BAD_REQUEST error code yourself.

Comment on lines +18 to +24
impl Default for HttpServerConfig {
fn default() -> Self {
Self {
http_address: IpAddr::V4(Ipv4Addr::LOCALHOST),
http_port: DEFAULT_HTTP_PORT,
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably unified the default values accross both Default trait and clap:

#[derive(Debug, Clone, Args)]
pub struct HttpServerConfig {
    #[clap(long = "http-address", default_value_t = HttpServerConfig::default().http_address)]
    http_address: IpAddr,

    #[clap(long = "http-port", default_value_t = HttpServerConfig::default().http_port)]
    http_port: u16,
}

to keep those in single place

Comment on lines +1 to +5
pub mod aggregator_controller;
mod aggregator_handlers;
mod config;
pub mod handlers;
mod routing;
pub mod routing;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a good idea, to expose crate internals to outside world - you should export only needed parts via pub use instead. If some internals are needed for testing, then probably it is a good idea to move tests into the same file as internals, not to expose them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants