-
Notifications
You must be signed in to change notification settings - Fork 52
feat: support aggregator capabilities in aggregator discovery #2844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: support aggregator capabilities in aggregator discovery #2844
Conversation
f5c7cb6 to
2aede73
Compare
2aede73 to
01d53af
Compare
01d53af to
b2dbab8
Compare
There was a problem hiding this 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 adds support for displaying aggregator capabilities in the aggregator discovery process. The AggregatorDiscoverer trait has been made generic to support returning either basic endpoints or endpoints with capabilities, and the CLI now displays capability information (aggregate signature type and signed entity types) when discovering aggregators.
Changes:
- Made
AggregatorDiscoverertrait generic over return type to support bothAggregatorEndpointandAggregatorEndpointWithCapabilities - Added
AggregatorEndpointWithCapabilitiesmodel with capability information and conversion implementations - Updated CLI aggregator discovery command to display capabilities in a table format
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/mithril-aggregator-discovery/src/interface.rs | Made AggregatorDiscoverer trait generic with type parameter T |
| internal/mithril-aggregator-discovery/src/model.rs | Added AggregatorEndpointWithCapabilities struct and conversion implementations |
| internal/mithril-aggregator-discovery/src/capabilities_discoverer.rs | Updated to return AggregatorEndpointWithCapabilities and modified iterator logic |
| internal/mithril-aggregator-discovery/src/http_config_discoverer.rs | Updated trait implementation to specify AggregatorEndpoint type parameter |
| internal/mithril-aggregator-discovery/src/rand_discoverer.rs | Updated trait implementation to specify AggregatorEndpoint type parameter |
| internal/mithril-aggregator-discovery/src/test/double/discoverer.rs | Updated test double to specify AggregatorEndpoint type parameter |
| internal/mithril-aggregator-discovery/src/lib.rs | Exported AggregatorEndpointWithCapabilities |
| mithril-client/src/client.rs | Updated type annotations and made discover_aggregator return endpoints with capabilities |
| mithril-client-cli/src/commands/tools/aggregator_discovery.rs | Enhanced output table to display aggregate signature type and signed entity types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
b2dbab8 to
7534d16
Compare
7534d16 to
4661764
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one caution (see first comment).
Overall I feel that this api is fragile, when reading this PR I see two main problems:
- usage of async code inside sync code because it returns an Iterator (the main problem)
- the addition of a generic to the
AggregatorDiscoverertrait, making the api less easy to use
- may be addressed by using
futures::stream::TryStream, but there may be some hiccup when using it downstream as this means not using the standard iterator api anymore. - is more difficult to solve and would probably mean an extensive refactoring of the crate (maybe with returning concrete type instead of returning traits).
None of this is need immediate action, this code is okay as is.
| } | ||
| } | ||
|
|
||
| impl TryFrom<AggregatorEndpoint> for AggregatorEndpointWithCapabilities { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is dangerous, like Copilot said if called outside of tokio this will panic, instead of implementing TryFrom I think it would be better to instead add a retrieve_capabilities_blocking to AggregatorEndpoint which would be specify to not call it outside of a tokio runtime to avoid panic.
This would still be brittle as this don't change the fact that the capabilities discoverer have to run async code in a non-async method, but would be a avoid a second level of abstraction which hide this fact.
Also it could be interesting to explore using Handle::try_current() to avoid the panic if run outside of a tokio runtime but instead raising an error.
| .unwrap(); | ||
|
|
||
| let next_aggregator = aggregators.next(); | ||
| let next_aggregator = aggregators.next().map(|endpoint| endpoint.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nitpick] you should be able to also write this conversion like this:
| let next_aggregator = aggregators.next().map(|endpoint| endpoint.into()); | |
| let next_aggregator = aggregators.next().map(Into::into); |
Content
This PR includes support or aggregator capabilities in the aggregator discovery process.
The capabilities of the discovered aggregators are now available in the CLI output. This command:
Will output:
Pre-submit checklist
Issue(s)
Relates to #2726