Skip to content
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

refactor(aggregator): simplify dependency builder #2288

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Feb 7, 2025

Content

This PR includes a extensive splitting of the aggregator dependency builder, a 2200 lines file, in order to make it more manageable.

Main changes:

  • the dependency/builder.rs is now a module with several submodules:
    • protocol: contains the core services that run the Mithril protocol (generation of signables artefacts => aggregations of signatures into a certificates => building of artefacts and generation of proofs).
    • enablers: contains the services related to sub domain cases that enables the protocol, such as communicating with the node.
    • support: contains low level services such as stores or cross-cutting concerns such as observability.

Additional change:

  • some methods related to configuration access have been streamlined either by removing them and using the configuration directly or by moving said method to the configuration structure.

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
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Comments

This methods only re-dispatch existing code without reducing the existing get_/build_ boilerplate, an attempt to do that will be done in another pull request.

Issue(s)

Relates to #2254

@Alenar Alenar added the refactoring 🛠️ Code refactoring and enhancements label Feb 7, 2025
@Alenar Alenar self-assigned this Feb 7, 2025
@Alenar Alenar force-pushed the djo/2254/aggregator/simplify-dependency-builder branch from f6f0e06 to f6ee221 Compare February 7, 2025 17:31
@Alenar Alenar force-pushed the djo/2254/aggregator/simplify-dependency-builder branch from f6ee221 to 2b13644 Compare February 7, 2025 17:39
Copy link

github-actions bot commented Feb 7, 2025

Test Results

    4 files  ±0     56 suites  ±0   20m 30s ⏱️ ±0s
1 596 tests ±0  1 596 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 900 runs  ±0  1 900 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b379808. ± Comparison against base commit 2627f17.

This pull request removes 3 and adds 3 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ dependency_injection::builder::tests::build_cardano_database_artifact_builder::if_local_uploader_creates_all_cardano_database_subdirs
mithril-aggregator ‑ dependency_injection::builder::tests::build_cardano_database_artifact_builder::if_not_local_uploader_create_cardano_database_immutable_dirs
mithril-aggregator ‑ dependency_injection::builder::tests::cardano_transactions_preloader_activated_with_cardano_transactions_signed_entity_type_in_configuration
mithril-aggregator ‑ dependency_injection::builder::enablers::cardano_node::tests::cardano_transactions_preloader_activated_with_cardano_transactions_signed_entity_type_in_configuration
mithril-aggregator ‑ dependency_injection::builder::protocol::artifacts::tests::if_local_uploader_creates_all_cardano_database_subdirs
mithril-aggregator ‑ dependency_injection::builder::protocol::artifacts::tests::if_not_local_uploader_create_cardano_database_immutable_dirs

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview February 7, 2025 17:51 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet February 7, 2025 17:51 — 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 👍

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

…o a directory

In order to split it afterward.
…uilders

Without moving the existing methods yet.
- move `get_epoch_settings_configuration` to the Configuration struct as
  it have no other dependencies
- use directly `Configuration::compute_allowed_signed_entity_types_discriminants`
  instead of `get_allowed_signed_entity_types_discriminants` as the only
  thing that the latter do is calling the former.
* mithril-aggregator from `0.7.1` to `0.7.2`
@Alenar Alenar force-pushed the djo/2254/aggregator/simplify-dependency-builder branch from 2b13644 to b379808 Compare February 10, 2025 14:31
@Alenar Alenar temporarily deployed to testing-preview February 10, 2025 14:42 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet February 10, 2025 14:42 — with GitHub Actions Inactive
@Alenar Alenar merged commit f9b2d69 into main Feb 10, 2025
37 of 41 checks passed
@Alenar Alenar deleted the djo/2254/aggregator/simplify-dependency-builder branch February 10, 2025 17:03
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.

3 participants