Skip to content

Conversation

@Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 29, 2025

Content

This PR includes a refactor of the mithril-aggregator and mithril-signer to make them use the shared AggregatorHttpClient defined in the internal/mithril-aggregator-client crate.

Details

mithril-aggregator-client

  • Add query needed by aggregator, signer, and mithril-protocol-config : GetAggregatorFeaturesQuery, GetCertificatesListQuery, GetEpochSettingsQuery, GetProtocolConfigurationQuery, PostRegisterSignatureQuery, and PostRegisterSignerQuery
  • rename CertificateDetailsQuery to GetCertificateQuery so all queries names follow a {Verb}{Subject}Query scheme
  • rename AggregatorClient to AggregatorHttpClient, same for the error and result types: this makes clears that those types are fully dedicated to http and can't be easily generalized.
  • reorganize queries in get and post submodules instead of domain based submodules (like the previous certificate) as domain based naming would have result in a lot of submodules with two or three files maximum
  • light refactor of the client buider to accommodate real usage in code: with_api_version_provider now take an Arc as parameter, with_relay_endpoint now take an option.
  • add a test_http_compression_is_enabled that generate to easily add a test in child crates that check that http compression is enabled

mithril-protocol-config

  • Refactor HttpMithrilNetworkConfigurationProvider to use a mithril-aggregator-client::AggregatorHttpClient

mithril-aggregator

  • replace the locally defined AggregatorClient with a direct implementation of the business traits on the mithril-aggregator-client::AggregatorHttpClient

mithril-signer

  • replace the locally defined AggregatorClient with a direct implementation of the business traits on the mithril-aggregator-client::AggregatorHttpClient
  • simplify the AggregatorClient trait to use StdResult instead of AggregatorClientError

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
    • No new TODOs introduced

Issue(s)

Relates to #2640

@Alenar Alenar self-assigned this Oct 29, 2025
@Alenar Alenar added the refactoring 🛠️ Code refactoring and enhancements label Oct 29, 2025
@github-actions
Copy link

github-actions bot commented Oct 30, 2025

Test Results

    4 files  ± 0    168 suites  ±0   24m 40s ⏱️ +42s
2 207 tests  - 49  2 207 ✅  - 49  0 💤 ±0  0 ❌ ±0 
6 887 runs   - 54  6 887 ✅  - 54  0 💤 ±0  0 ❌ ±0 

Results for commit fcd8891. ± Comparison against base commit 0625621.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview October 30, 2025 08:51 — with GitHub Actions Inactive
Copy link
Collaborator

@turmelclem turmelclem left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Since there will be arround ~20 requests maximum, making a module by
subject will result on a lots of modules with one or two files, not that
helpful.
+ rename `CertificateDetailsQuery` to `GetCertificateQuery` to have
an uniform convention: `{Verb}{Subject}Query`.
…in follower aggregator

- use an arc for the `ApiVersionProvider` so it fit with our current DI
  systems
… `Http`

This makes clear that this client is Http focused only and can't be
extended as is to support other protocols.
@Alenar Alenar force-pushed the djo/2640/shared-aggregator-client-for-signer-aggregrator branch from 65a5b51 to c5822d4 Compare October 31, 2025 14:42
@Alenar Alenar temporarily deployed to testing-preview October 31, 2025 14:52 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-preview October 31, 2025 15:06 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the djo/2640/shared-aggregator-client-for-signer-aggregrator branch from 883789a to 6bda57a Compare November 3, 2025 11:59
* mithril-aggregator-client from `0.1.1` to `0.1.2`
* mithril-protocol-config from `0.1.1` to `0.1.2`
* mithril-aggregator from `0.7.92` to `0.7.93`
* mithril-signer from `0.2.274` to `0.2.275`
@Alenar Alenar temporarily deployed to testing-preview November 3, 2025 12:10 — with GitHub Actions Inactive
@Alenar Alenar merged commit d1a8a24 into main Nov 3, 2025
41 of 43 checks passed
@Alenar Alenar deleted the djo/2640/shared-aggregator-client-for-signer-aggregrator branch November 3, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring 🛠️ Code refactoring and enhancements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants