Skip to content

Conversation

@jpraynaud
Copy link
Member

Content

This PR includes...

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • All check jobs of the CI have succeeded
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)
    • Add ADR blog post or Dev ADR entry (if relevant)
    • No new TODOs introduced

Issue(s)

Closes #2726

@jpraynaud jpraynaud self-assigned this Nov 14, 2025
Copilot finished reviewing on behalf of jpraynaud November 14, 2025 16:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a simple aggregator discovery mechanism to enable the Mithril client to automatically discover and connect to available aggregators in a network.

Key changes:

  • Introduces a new mithril-aggregator-discovery crate with discoverer implementations
  • Refactors the ClientBuilder API to support aggregator discovery modes (automatic vs. explicit URL)
  • Adds capability-based filtering and randomization of aggregator selection

Reviewed Changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
internal/mithril-aggregator-discovery/Cargo.toml Defines the new aggregator discovery crate with dependencies
internal/mithril-aggregator-discovery/src/lib.rs Exports the public API for aggregator discovery
internal/mithril-aggregator-discovery/src/interface.rs Defines the AggregatorDiscoverer trait
internal/mithril-aggregator-discovery/src/model.rs Implements MithrilNetwork and AggregatorEndpoint models
internal/mithril-aggregator-discovery/src/http_config_discoverer.rs HTTP-based discoverer that fetches aggregator lists from remote configuration
internal/mithril-aggregator-discovery/src/capabilities_discoverer.rs Filters aggregators by required capabilities
internal/mithril-aggregator-discovery/src/rand_discoverer.rs Shuffles aggregator order using randomization
internal/mithril-aggregator-discovery/src/test/ Test utilities and doubles for testing
mithril-client/src/client.rs Refactors client builder to support aggregator discovery
mithril-client/Cargo.toml Adds dependency on the new discovery crate
Makefile, Cargo.toml, .github/workflows/ci.yml Build configuration updates
README.md Documentation for the new crate

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

impl AggregatorDiscovererFake {
/// Creates a new `AggregatorDiscovererFake` instance with the provided results.
pub fn new(results: Vec<AggregatorListReturn>) -> Self {
dbg!(&results);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Debug statement should be removed. This dbg! macro call will print to stderr in production code and is typically used only during development.

Suggested change
dbg!(&results);

Copilot uses AI. Check for mistakes.
pub trait AggregatorDiscoverer: Sync + Send {
/// Get an iterator over a list of available aggregators in a Mithril network.
///
/// Note: there is no guarantee that the returned aggregators is sorted, complete or up-to-date.
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Corrected grammar: 'aggregators is' should be 'aggregators are'.

Suggested change
/// Note: there is no guarantee that the returned aggregators is sorted, complete or up-to-date.
/// Note: there is no guarantee that the returned aggregators are sorted, complete or up-to-date.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,44 @@
[package]
name = "mithril-aggregator-discovery"
description = "Mechanisms to discover aggregator available in a Mithril network."
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Corrected grammar: 'aggregator available' should be 'aggregators available'.

Suggested change
description = "Mechanisms to discover aggregator available in a Mithril network."
description = "Mechanisms to discover aggregators available in a Mithril network."

Copilot uses AI. Check for mistakes.
let aggregator_endpoint = match self.aggregator_discovery {
AggregatorDiscoveryType::Url(ref url) => url.clone(),
AggregatorDiscoveryType::Automatic => {
todo!("Implement automatic aggregator discovery")
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The todo!() macro will panic at runtime if this code path is executed. Since AggregatorDiscoveryType::Automatic is a public API variant, this creates a potential runtime failure. Consider either removing the Automatic variant until implemented, or returning a proper error with context.

Suggested change
todo!("Implement automatic aggregator discovery")
return Err(anyhow!("Automatic aggregator discovery is not yet implemented"));

Copilot uses AI. Check for mistakes.

- [**Mithril aggregator client**](./internal/mithril-aggregator-client): a client to request data from a Mithril aggregator, used by **Mithril network** nodes and client library.

- [**Mithril aggregator discovery**](./internal/mithril-aggregator-discovery): mechanisms to discover available Mithril aggregator, used by **Mithril network** nodes and client library.
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Corrected grammar: 'aggregator' should be 'aggregators'.

Suggested change
- [**Mithril aggregator discovery**](./internal/mithril-aggregator-discovery): mechanisms to discover available Mithril aggregator, used by **Mithril network** nodes and client library.
- [**Mithril aggregator discovery**](./internal/mithril-aggregator-discovery): mechanisms to discover available Mithril aggregators, used by **Mithril network** nodes and client library.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Nov 14, 2025

Test Results

    4 files  ± 0    172 suites  +4   24m 20s ⏱️ + 1m 7s
2 230 tests +20  2 230 ✅ +20  0 💤 ±0  0 ❌ ±0 
6 975 runs  +80  6 975 ✅ +80  0 💤 ±0  0 ❌ ±0 

Results for commit c9f8423. ± Comparison against base commit 57e1490.

♻️ This comment has been updated with latest results.

@jpraynaud jpraynaud force-pushed the jpraynaud/2726-simple-aggregator-discovery branch from 945d3c2 to c74fb50 Compare November 17, 2025 16:29
@jpraynaud jpraynaud force-pushed the jpraynaud/2726-simple-aggregator-discovery branch from c74fb50 to 434891f Compare November 18, 2025 17:50
Implementation for 'AggregatorDiscoverer' trait.
…ities' type

Which allows to combine signed entity types and aggregate signature type in a flexible way.
@jpraynaud jpraynaud force-pushed the jpraynaud/2726-simple-aggregator-discovery branch from 434891f to c9f8423 Compare November 19, 2025 17:46
));
}
if required_capabilities.len() == 1 {
return required_capabilities.into_iter().next().unwrap();

Check warning

Code scanning / clippy

unneeded return statement Warning

unneeded return statement
));
}
if required_capabilities.len() == 1 {
return required_capabilities.into_iter().next().unwrap();

Check warning

Code scanning / clippy

unneeded return statement Warning

unneeded return statement
if required_capabilities.len() == 1 {
return required_capabilities.into_iter().next().unwrap();
} else {
return RequiredAggregatorCapabilities::And(required_capabilities);

Check warning

Code scanning / clippy

unneeded return statement Warning

unneeded return statement
if required_capabilities.len() == 1 {
return required_capabilities.into_iter().next().unwrap();
} else {
return RequiredAggregatorCapabilities::And(required_capabilities);

Check warning

Code scanning / clippy

unneeded return statement Warning

unneeded return statement
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.

Implement a simple aggregator discovery mechanism

2 participants