Skip to content

Conversation

@Alenar
Copy link
Collaborator

@Alenar Alenar commented Nov 10, 2025

Content

This PR includes a refactor to the mithril-signer inner architecture splitting its AggregatorClient into several small business service or removing unused method.

Details

  • Implement SignaturePublisher directly on mithril_aggregator_client::AggregatorHttpClient, this allow for the removal of register_signature from the signer AggregatorClient
  • Remove AggregatorClient::retrieve_aggregator_features which is no longer used since the introduction of the MithrilNetworkConfigurationProvider
  • Add a new services::signer_registration module, with two submodules based on the architecture of the services::signature_publisher module:
    • publisher which extract the AggregatorClient::register_signer into a new SignerRegistrationPublisher trait
    • retriever which extract the AggregatorClient::retrieve_aggregator_features into a new SignersRegistrationRetriever
    • both new traits are implemented directly over the mithril_aggregator_client::AggregatorHttpClient
  • streamline naming (rename certificate_handler to appropriate types)

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are 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)

Closes #2759

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

github-actions bot commented Nov 10, 2025

Test Results

    4 files  ±0    168 suites  ±0   24m 6s ⏱️ +36s
2 204 tests  - 3  2 204 ✅  - 3  0 💤 ±0  0 ❌ ±0 
6 881 runs   - 6  6 881 ✅  - 6  0 💤 ±0  0 ❌ ±0 

Results for commit 951dd5a. ± Comparison against base commit 161e0fc.

This pull request removes 9 and adds 6 tests. Note that renamed tests count towards both.
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::register_signer
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_aggregator_features
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_epoch_settings
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::register_signer
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_aggregator_features
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::certificate_handler::tests::retrieve_epoch_settings
mithril-signer::era_switch ‑ test_extensions::certificate_handler::tests::register_signer
mithril-signer::era_switch ‑ test_extensions::certificate_handler::tests::retrieve_aggregator_features
mithril-signer::era_switch ‑ test_extensions::certificate_handler::tests::retrieve_epoch_settings
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::fake_aggregator::tests::register_signer
mithril-signer::create_cardano_transaction_single_signature ‑ test_extensions::fake_aggregator::tests::retrieve_epoch_settings
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::fake_aggregator::tests::register_signer
mithril-signer::create_immutable_files_full_single_signature ‑ test_extensions::fake_aggregator::tests::retrieve_epoch_settings
mithril-signer::era_switch ‑ test_extensions::fake_aggregator::tests::register_signer
mithril-signer::era_switch ‑ test_extensions::fake_aggregator::tests::retrieve_epoch_settings

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview November 10, 2025 18:28 — with GitHub Actions Inactive
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 👍

…tpClient`

Instead of implementing the publisher on the signer `AggregatorClient`,
removing an intermediate layer
…SignerRegistrationPublisher` trait

- Based on the same architecture than the `SignaturePublisher`
- with an http implementation directly on the shared `AggregatorHttpClient`
- add a spy implementation for the tests needs
…res`

As it was supersed by the mithril network configuration and have now no
usage even in tests.
…to `retrieve_all_signer_registrations`

And `SignerEpochSettings` to `RegisteredSigners`
Since the former have been supersed with multiple traits which are
implemented on the `FakeAggregator` test utils.
…in the same parent module

and split the retriever in several modules like the publisher
* mithril-signer from `0.2.277` to `0.2.278`
@Alenar Alenar force-pushed the djo/2759/signer/refactor-aggregator-client branch from 1501736 to 951dd5a Compare November 13, 2025 10:44
@Alenar Alenar temporarily deployed to testing-preview November 13, 2025 10:58 — with GitHub Actions Inactive
@Alenar Alenar merged commit a735684 into main Nov 13, 2025
41 checks passed
@Alenar Alenar deleted the djo/2759/signer/refactor-aggregator-client branch November 13, 2025 11:05
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.

Refactor AggregatorClient trait in signer

3 participants