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.

Sign up for GitHub

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

Jump to bottom

Reorganize signer dependencies #1908

Merged
merged 14 commits into from
Aug 29, 2024
Merged

Reorganize signer dependencies #1908

merged 14 commits into from
Aug 29, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Aug 29, 2024 •
edited
Loading

Content

This PR reorganize mithril-signer source directory based on mithril-aggregator current structure:

  • runtime/signer_services:
    • Moved to a dependency_injection module
    • Renamed types: ServiceBuilder -> DependenciesBuilder, SignerService -> SignerDependencyContainer)
    • Types are split into a builder and container submodule
  • Introduce a services module:
    • Contains aggregator client, transactions importer, single signer and upkeep service
    • Since transactions importer are split in several files (because of the three decorators) it's itself a submodule directory in the services module.
  • Introduce a store module that contains the MKTreeStoreSqlite and the ProtocolInitializerStore

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

Issue(s)

Closes #1906

@Alenar Alenar requested review from sfauvel and jpraynaud August 29, 2024 11:02
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 29, 2024 •
edited
Loading

Test Results

    4 files  ±0     53 suites  ±0   9m 13s ⏱️ -13s
1 245 tests ±0  1 245 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 456 runs  ±0  1 456 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 8362c9b. ± Comparison against base commit 29586c4.

This pull request removes 62 and adds 62 tests. Note that renamed tests count towards both. 
mithril-signer ‑ aggregator_client::tests::test_aggregator_features_ko_412
mithril-signer ‑ aggregator_client::tests::test_aggregator_features_ko_500
mithril-signer ‑ aggregator_client::tests::test_aggregator_features_ko_json_serialization
mithril-signer ‑ aggregator_client::tests::test_aggregator_features_ok_200
mithril-signer ‑ aggregator_client::tests::test_aggregator_features_timeout
mithril-signer ‑ aggregator_client::tests::test_certificate_pending_ko_412
mithril-signer ‑ aggregator_client::tests::test_certificate_pending_ko_500
mithril-signer ‑ aggregator_client::tests::test_certificate_pending_ok_200
mithril-signer ‑ aggregator_client::tests::test_certificate_pending_ok_204
mithril-signer ‑ aggregator_client::tests::test_epoch_settings_ko_412
…

mithril-signer ‑ dependency_injection::builder::tests::test_auto_create_stores_directory
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_ko_412
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_ko_500
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_ko_json_serialization
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_ok_200
mithril-signer ‑ services::aggregator_client::tests::test_aggregator_features_timeout
mithril-signer ‑ services::aggregator_client::tests::test_certificate_pending_ko_412
mithril-signer ‑ services::aggregator_client::tests::test_certificate_pending_ko_500
mithril-signer ‑ services::aggregator_client::tests::test_certificate_pending_ok_200
mithril-signer ‑ services::aggregator_client::tests::test_certificate_pending_ok_204
…

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview August 29, 2024 11:10 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet August 29, 2024 11:10 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Aug 29, 2024
View reviewed changes
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

I just left few suggestions

mithril-signer/src/services/cardano_transactions_importer/mod.rs Outdated Show resolved Hide resolved
mithril-signer/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
Alenar added 11 commits August 29, 2024 14:50
@Alenar
Scaffold a dependency_injection module in Signer
40a8a86 
Based on the aggregator equivalent module.
@Alenar
Align Signer dependency injection components with aggregator naming
c28cd91 
* `**ServiceBuilder` -> `**DependenciesBuilder`
* `SignerService` -> `SignerDependencyContainer`
@Alenar
Move Signer Dependencies builder & container to `dependency_injection…
44bd0bf 
…` module

And remove `signer_services.rs` from the `runtime` module.
@Alenar
Scaffold a services module in Signer
9c7c349
@Alenar
Move several Signer services to services mod
23791da 
While keeping import paths identical for now.
@Alenar
Scope signer services import to the services module
556acef 
Instead of reexporting them directly at the lib root.
@Alenar
Promote Signer services::cardano_transactions_importer mod to a dir…
141d68c 
…ectory

And move existing code to a `service.rs` mod to avoid putting everything
in the `mod.rs`.

This is a ground work to move all importers to this directory
@Alenar
Move tx importer decorators to the new module
8c07973
@Alenar
Scaffold a store module in Signer
01e9188
@Alenar
Move Signer stores to their new dedicated mod
8b6511d
@Alenar
Scope signer store import to the store module
6fe0220
@Alenar Alenar force-pushed the djo/1906/reorganize_signer_dependencies branch from 76daf5f to 6fe0220 Compare August 29, 2024 12:51
@Alenar Alenar had a problem deploying to testing-sanchonet August 29, 2024 12:58 — with GitHub Actions Failure
@Alenar Alenar temporarily deployed to testing-preview August 29, 2024 13:30 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet August 29, 2024 13:30 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the djo/1906/reorganize_signer_dependencies branch from b57768d to c9abaa5 Compare August 29, 2024 14:35
@Alenar
Rename ProductionDependenciesBuilder to DependenciesBuilder
afd8865 
And remove the trait of the same name.

The idea behind the `DependenciesBuilder` trait was to define a common
api to build dependencies, but two years after its introduction it only
have one implementor.
@Alenar Alenar force-pushed the djo/1906/reorganize_signer_dependencies branch from c9abaa5 to afd8865 Compare August 29, 2024 14:37
@Alenar Alenar temporarily deployed to testing-preview August 29, 2024 14:45 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet August 29, 2024 14:45 — with GitHub Actions Inactive
sfauvel
sfauvel approved these changes Aug 29, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

@Alenar Alenar temporarily deployed to testing-preview August 29, 2024 14:59 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet August 29, 2024 14:59 — with GitHub Actions Inactive
@Alenar Alenar merged commit bf52aa3 into main Aug 29, 2024
40 checks passed
@Alenar Alenar deleted the djo/1906/reorganize_signer_dependencies branch August 29, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpraynaud jpraynaud jpraynaud approved these changes

@sfauvel sfauvel sfauvel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reorganize signer dependencies
3 participants
@Alenar @sfauvel @jpraynaud