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

Make aggregator advertises constant signing configurations for an epoch #1964

Merged
merged 18 commits into from
Oct 1, 2024
Merged

Make aggregator advertises constant signing configurations for an epoch #1964

merged 18 commits into from
Oct 1, 2024

Conversation

dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Sep 27, 2024 •
edited
Loading

Content

This PR updates the aggregator to ensure the /epoch-settings route consistently exposes constant signing configurations for each epoch.

The Cardano signing configurations are now stored in the aggregator's epoch_setting table, ensuring they remain unchanged throughout an epoch.

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
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Issue(s)

Closes #1924

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 27, 2024 •
edited
Loading

Test Results

    4 files  ±0     54 suites  ±0   9m 47s ⏱️ +15s
1 310 tests +3  1 310 ✅ +3  0 💤 ±0  0 ❌ ±0 
1 521 runs  +3  1 521 ✅ +3  0 💤 ±0  0 ❌ ±0 

Results for commit 239e18a. ± Comparison against base commit c4e0f0f.

This pull request removes 7 and adds 10 tests. Note that renamed tests count towards both. 
mithril-aggregator ‑ database::repository::epoch_settings_store::tests::save_protocol_parameters_prune_older_epoch_settings
mithril-aggregator ‑ runtime::runner::tests::test_update_protocol_parameters
mithril-aggregator ‑ services::epoch_service::tests::update_protocol_parameters_insert_future_protocol_parameters_in_the_store
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_get_protocol_parameters_do_not_exist
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_get_protocol_parameters_exist
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_save_protocol_parameters_already_exist
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_save_protocol_parameters_do_not_exist_yet

mithril-aggregator ‑ database::repository::epoch_settings_store::tests::save_epoch_settings_prune_older_epoch_settings
mithril-aggregator ‑ database::repository::epoch_settings_store::tests::save_epoch_settings_stores_in_database
mithril-aggregator ‑ http_server::routes::epoch_routes::tests::get_epoch_settings_message_retrieves_protocol_parameters_from_epoch_service
mithril-aggregator ‑ http_server::routes::epoch_routes::tests::get_epoch_settings_message_retrieves_signing_configuration_from_epoch_service
mithril-aggregator ‑ runtime::runner::tests::test_update_epoch_settings
mithril-aggregator ‑ services::epoch_service::tests::update_epoch_settings_insert_future_epoch_settings_in_the_store
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_get_epoch_settings_do_not_exist
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_get_epoch_settings_exist
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_save_epoch_settings_already_exist_return_previous_epoch_settings_stored
mithril-aggregator ‑ store::epoch_settings_storer::tests::test_save_epoch_settings_do_not_exist_yet_return_none

♻️ This comment has been updated with latest results.

@dlachaume dlachaume force-pushed the ensemble/1924/store-signing-config branch 2 times, most recently from 141df96 to 7084f0e Compare September 27, 2024 14:55
@dlachaume dlachaume force-pushed the ensemble/1924/store-signing-config branch from f444c62 to 8d25e1a Compare September 30, 2024 09:32
@dlachaume dlachaume self-assigned this Sep 30, 2024
@dlachaume dlachaume marked this pull request as ready for review September 30, 2024 10:11
jpraynaud
jpraynaud approved these changes Sep 30, 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 left minor comments below.

mithril-aggregator/src/database/repository/epoch_settings_store.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/containers.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/containers.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/epoch_routes.rs Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/epoch_routes.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/services/epoch_service.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/services/epoch_service.rs Show resolved Hide resolved
mithril-aggregator/src/services/epoch_service.rs Show resolved Hide resolved
mithril-aggregator/src/store/epoch_settings_storer.rs Outdated Show resolved Hide resolved
mithril-common/src/entities/protocol_parameters.rs Outdated Show resolved Hide resolved
sfauvel and others added 7 commits September 30, 2024 16:50
@sfauvel @dlachaume
Add CardanoTransactionsSigningConfigto the AggregatorEpochSettings
e8b01d2
@sfauvel @dlachaume
refacto: ẁith_data in FakeEpochService takes AggregatorEpochSetti…
18d233c 
…ngs` instead of `ProtocolParameters`
@sfauvel @dlachaume
Plug epoch route with cardano transactions signing config from EpochS…
990fcbb 
…ervice

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume @sfauvel
Fix typos
2d50170 
Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume
Adapt test that save future epoch settings in store by retrieving Epo…
24f62d1 
…ch settings instead of Protocol Parameters
@dlachaume @sfauvel
Minor changes after PR review
849ae86 
- fix functions renaming
- simplify `init_epoch_settings_storer` parameters
- remove unused function `upcoming_cardano_transactions_signing_config`

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume
Remove inappropriate Default implementation on structs
00fea10
@dlachaume dlachaume force-pushed the ensemble/1924/store-signing-config branch from 4f279b4 to 69ca68f Compare September 30, 2024 14:52
Alenar
Alenar approved these changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

mithril-aggregator/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
@dlachaume
Add missing test cases when accessing to cardano signing configuratio…
a81fa85 
…ns before calling `inform_epoch`
@dlachaume
Change get_epoch_settings_configuration result type to dedicated `R…
058f6ed 
…esult`
@dlachaume
Enrich ExpectedEpochData with cardano signing config fields
d0ff532
@dlachaume
chore: bump crates versions
239e18a 
* mithril-aggregator from `0.5.70` to `0.5.71`
* mithril-common from `0.4.60` to `0.4.61`
* mithril-signer from `0.2.188` to `0.2.189`
@dlachaume dlachaume force-pushed the ensemble/1924/store-signing-config branch from 51a0e2b to 239e18a Compare October 1, 2024 09:57
@dlachaume dlachaume merged commit e60e0c5 into main Oct 1, 2024
45 checks passed
@dlachaume dlachaume deleted the ensemble/1924/store-signing-config branch October 1, 2024 10:15
@dlachaume dlachaume mentioned this pull request Oct 1, 2024
Merged
12 tasks
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

@Alenar Alenar Alenar approved these changes

@sfauvel sfauvel sfauvel approved these changes

Assignees

@dlachaume dlachaume

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregator advertises constant signing configurations for an epoch
4 participants
@dlachaume @Alenar @sfauvel @jpraynaud