Skip to content

Conversation

@dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Sep 23, 2024

Content

This PR includes refactoring of epoch settings:

  • Moved EpochSettings from common to SignerEpochSettings in the signer
  • Standardized occurrences of EpochSetting to EpochSettings
  • Renamed the ProtocolParametersStorer trait to EpochSettingsStorer

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)

@dlachaume dlachaume self-assigned this Sep 23, 2024
@dlachaume dlachaume requested a review from sfauvel September 23, 2024 15:38
@github-actions
Copy link

github-actions bot commented Sep 23, 2024

Test Results

    4 files  ±0     54 suites  ±0   9m 48s ⏱️ +9s
1 295 tests ±0  1 295 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 506 runs  ±0  1 506 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d8880b4. ± Comparison against base commit a0d6680.

This pull request removes 10 and adds 10 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ database::query::epoch_setting::delete_epoch_setting::tests::test_delete_below_threshold
mithril-aggregator ‑ database::query::epoch_setting::delete_epoch_setting::tests::test_delete_by_epoch
mithril-aggregator ‑ database::query::epoch_setting::get_epoch_setting::tests::test_get_epoch_settings
mithril-aggregator ‑ database::query::epoch_setting::update_epoch_setting::tests::test_update_epoch_setting
mithril-aggregator ‑ database::repository::epoch_setting_store::tests::save_protocol_parameters_prune_older_epoch_settings
mithril-aggregator ‑ store::protocol_parameters_store::tests::test_get_protocol_parameters_do_not_exist
mithril-aggregator ‑ store::protocol_parameters_store::tests::test_get_protocol_parameters_exist
mithril-aggregator ‑ store::protocol_parameters_store::tests::test_handle_discrepancies_at_startup_should_complete_at_least_two_epochs
mithril-aggregator ‑ store::protocol_parameters_store::tests::test_save_protocol_parameters_already_exist
mithril-aggregator ‑ store::protocol_parameters_store::tests::test_save_protocol_parameters_do_not_exist_yet
mithril-aggregator ‑ database::query::epoch_settings::delete_epoch_settings::tests::test_delete_below_threshold
mithril-aggregator ‑ database::query::epoch_settings::delete_epoch_settings::tests::test_delete_by_epoch
mithril-aggregator ‑ database::query::epoch_settings::get_epoch_settings::tests::test_get_epoch_settings
mithril-aggregator ‑ database::query::epoch_settings::update_epoch_settings::tests::test_update_epoch_settings
mithril-aggregator ‑ database::repository::epoch_settings_store::tests::save_protocol_parameters_prune_older_epoch_settings
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_handle_discrepancies_at_startup_should_complete_at_least_two_epochs
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

♻️ This comment has been updated with latest results.

@dlachaume dlachaume force-pushed the ensemble/standardize-epoch-settings-naming branch from 471bc9c to 886393d Compare September 23, 2024 15:44
@dlachaume dlachaume marked this pull request as ready for review September 23, 2024 15:46
@dlachaume dlachaume force-pushed the ensemble/standardize-epoch-settings-naming branch 2 times, most recently from acd39cb to b8c7aba Compare September 23, 2024 16:01
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

@dlachaume dlachaume changed the title Standardize epoch settings naming Refactor: standardize epoch settings naming Sep 24, 2024
@dlachaume dlachaume force-pushed the ensemble/standardize-epoch-settings-naming branch 2 times, most recently from 446bdc3 to 4470fee Compare September 24, 2024 08:50
@dlachaume dlachaume force-pushed the ensemble/standardize-epoch-settings-naming branch from 4470fee to eb2b54d Compare September 24, 2024 09:04
@dlachaume dlachaume requested a review from jpraynaud September 24, 2024 09:15
@dlachaume dlachaume requested a review from Alenar September 24, 2024 09:15
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 👍

@dlachaume dlachaume force-pushed the ensemble/standardize-epoch-settings-naming branch from eb2b54d to d8880b4 Compare September 24, 2024 14:08
@dlachaume dlachaume merged commit 04bd838 into main Sep 24, 2024
45 of 65 checks passed
@dlachaume dlachaume deleted the ensemble/standardize-epoch-settings-naming branch September 24, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants