-
Notifications
You must be signed in to change notification settings - Fork 51
add shared aggregator client #2661
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
Conversation
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 introduces a new shared aggregator client for the Mithril network, providing a reusable HTTP client library for communicating with Mithril aggregators. The client includes query abstractions, error handling, API version compatibility checking, and comprehensive test coverage.
Key changes:
- New
mithril-aggregator-clientcrate with HTTP client functionality - API version provider extensions for testing
- Certificate details query implementation
- Comprehensive error handling and logging
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/mithril-aggregator-client/ |
Complete new crate implementing the aggregator client with builder pattern, query system, and error handling |
mithril-common/src/api_version.rs |
Added Default implementation and test extensions for APIVersionProvider |
mithril-common/src/test/api_version_extensions.rs |
New test utilities for API version provider testing |
README.md |
Updated documentation to include the new aggregator client |
Cargo.toml |
Added new crate to workspace members |
.github/workflows/ci.yml |
Updated CI to include documentation generation for the new crate |
dlachaume
left a comment
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.
Just need to add WASM compatibility, otherwise LGTM 🚀.
turmelclem
left a comment
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 🦙
cf54bcc to
2b0a600
Compare
and move certificate_details query to `query::certificate::get_certificate_details`
imported from mithril-signer client
This is useful for context when the `APIVersionProvider` don't need to return response that may evolve at runtime, such as with the aggregator-client in the mithril-client. This also enable the new shared aggregator-client to have a default api version provider, that will always use the base `openapi.yml`, but provide a way to use another provider in its builder (for the aggregator and the client that will fetch the version discriminant based on the chain).
2b0a600 to
fc997ea
Compare
To enable testing with canned `openapi.yml` version or setting up a always failing provider
…ersions mismatch the test suite is copied and adapted from `mithril-client`
Make one test for shared features on http method instead of testing them on both get and post
…t latest genesis imported and adapted from the associated route tests in the `mithril-aggregator` aggregator client.
* mithril-aggregator from `0.7.77` to `0.7.78` * mithril-common from `0.6.12` to `0.6.13` * mithril-signer from `0.2.262` to `0.2.263`
fc997ea to
70c6c51
Compare
Content
This PR add a new
mithril-aggregator-clientinternal crate that define a api allowing to fetch and send data through Http to a Mithril Aggregator.This is a first step that define the internal mechanism but all available queries are yet to be implemented in order to replace existing clients in the aggregator, signer and client lib.
Details
mithril-aggregator-clientinternal crateAggregatorClientstruct that expose one method:sendAggregatorQuerytraitAggregatorClientis meant to be built using a builder pattern accessible from eitherAggregatorClient::builderor directly though the builder:AggregatorClientBuilder::new(aggregator_url)CertificateDetailsQueryto retrieve a certificate with a given hash or the latest genesismithril-common- makeApiVersionProviderTestExtensionsthat defines three test utilities:update_open_api_versions(test method moved fromApiVersionProviderpublic api),new_with_default_version({Version}), andnew_failingPre-submit checklist
Comments
Todo
A TODO is added because of an
unwrapinAggregatorClient.send:This unwrap is a strict copy from all three existing aggregator client behavior, this raise a question if a mithril api version, and by extension sending the
mithril-api-versionheader, is mandatory or not. Especially since the behavior ofwarn_if_api_version_mismatch, that is used afterward when handling the response, in all three clients is to not crash if an error is raised when computing the version.Api design choice
The choice have been made of making the
AggregatorQuerytrait internal to the crate, this means childs crates can't define custom queries.The idea behind this choice is that the public api of the crate avoid exposing types from third party, non-mithril, crates, but the
AggregatorQuerymechanism need to pass around areqwest::Response, so making this trait public would be exposing a "leaking" abstraction.We may decide instead to take that upfront if we think that defining queries on a child crate is useful, but since all messages that are available from a mithril aggregator are defined in the shared
mithril-commoncrate right now there's no difficulty to define all queries in themithril-aggregator-clientcrate.Issue(s)
Relates to #2640