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 timepoint retrieval & conversion to signed entities #1736

Merged
merged 16 commits into from
Jun 7, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jun 6, 2024

Content

Following changes introduced in #1727 the conversion of time points to signed entities became more cumbersome. Instead of only using a CardanoNetwork it also needed now the newly introduced CardanoTransactionsSigningConfig.

This PR make this easier by delegating the configuration to a new in memory type: SignedEntityConfig, that is created at configuration time and is in the application dependency container.

This type also takes responsibility, previously in the aggregator Configuration, to list allowed signed entity types from a time point. It also hold statically the list of default types that should always be allowed.

Also in this PR: Rename TimePointProvider to TickerService and remove the conflicting service, named the same, from the aggregator.
Those two were doing the same things and comments were in the code to remove the TimePointProvider in favor of the TickerService, but the former continued to evolved even if the later api & naming were preferred. So this PR fixes this contradiction by "fusing" them together.

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)

Relates to #1697 and #1727

Copy link

github-actions bot commented Jun 6, 2024

Test Results

    3 files  ±0     43 suites  ±0   8m 36s ⏱️ -1s
1 028 tests +6  1 028 ✅ +6  0 💤 ±0  0 ❌ ±0 
1 126 runs  +6  1 126 ✅ +6  0 💤 ±0  0 ❌ ±0 

Results for commit 179f66d. ± Comparison against base commit 2ebb8c4.

This pull request removes 13 and adds 19 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_discriminant_should_not_duplicate_a_signed_entity_discriminant_type_already_in_default_ones
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_discriminant_should_not_return_unknown_signed_entity_types_in_configuration
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_discriminant_without_specific_configuration
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_discriminants_should_add_signed_entity_types_in_configuration_at_the_end
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_discriminants_with_multiple_identical_signed_entity_types_in_configuration_should_not_be_added_several_times
mithril-aggregator ‑ configuration::test::test_list_allowed_signed_entity_types_with_specific_configuration
mithril-aggregator ‑ services::ticker::tests::get_epoch
mithril-aggregator ‑ services::ticker::tests::get_immutable_beacon
mithril-aggregator ‑ services::ticker::tests::no_beacon_error
mithril-common ‑ entities::signed_entity_type::tests::computing_block_number_to_be_signed
…
mithril-common ‑ entities::signed_entity_config::tests::computing_block_number_to_be_signed
mithril-common ‑ entities::signed_entity_config::tests::computing_block_number_to_be_signed_round_step_to_a_block_range_start
mithril-common ‑ entities::signed_entity_config::tests::given_discriminant_convert_to_signed_entity
mithril-common ‑ entities::signed_entity_config::tests::test_list_allowed_signed_entity_types_discriminant_should_not_duplicate_a_signed_entity_discriminant_type_already_in_default_ones
mithril-common ‑ entities::signed_entity_config::tests::test_list_allowed_signed_entity_types_discriminant_without_specific_configuration
mithril-common ‑ entities::signed_entity_config::tests::test_list_allowed_signed_entity_types_discriminants_should_add_configured_discriminants
mithril-common ‑ entities::signed_entity_config::tests::test_list_allowed_signed_entity_types_discriminants_with_multiple_identical_signed_entity_types_in_configuration_should_not_be_added_several_times
mithril-common ‑ entities::signed_entity_config::tests::test_list_allowed_signed_entity_types_with_specific_configuration
mithril-common ‑ entities::signed_entity_type::tests::parse_list_error_format_to_an_useful_message
mithril-common ‑ entities::signed_entity_type::tests::parse_signed_entity_types_discriminants_discriminant_without_values
…

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview June 6, 2024 15:37 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet June 6, 2024 15:37 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-preview June 6, 2024 15:59 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet June 6, 2024 15:59 — with GitHub Actions Inactive
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

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 few comments below.

mithril-aggregator/src/configuration.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/containers.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/configuration.rs Show resolved Hide resolved
Alenar and others added 14 commits June 7, 2024 12:38
Note: there's still a conflicting `TickerService` defined in the
aggregator, this will be solved in a later commit.

Co-authored-by: Sébastien Fauvel <[email protected]>
and remove it's own version of the service.

Co-authored-by: Sébastien Fauvel <[email protected]>
A shortcut method for the cases were only the epoch is needed

Co-authored-by: Sébastien Fauvel <[email protected]>
To `ticker_service`.

Co-authored-by: Sébastien Fauvel <[email protected]>
This static configuration contains all what's needed to convert a
`TimePoint` to a `SignedEntityType` (given a discriminant).

It's hold by the TickerService and copied into each created time point
so they don't need anything more for conversions (no additional services
needed at a price of a more complex struct).

Also add `SignedEntityTypeDiscriminants::all` to list all available
discriminent using `StrumIterm` (so they're always updated).

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: Damien Lachaume <[email protected]>
For signers, even if they have no use right now, they are configured as:
- allowed discriminants: all
- cardano_transactions_signing_config: security parameter '3000' and
  step '120' (same as aggregator).
…ormer

Since the `TimePoint` now hold everything needed for that conversion.
Copied & adapted from aggregator `Configuration`.

Co-authored-by: Damien Lachaume <[email protected]>
Instead of putting everything in the `TimePoint`, this made the
timepoint cumbersome to use in the numerous cases when we did not needs
those functionnalities.

Co-authored-by: Damien Lachaume <[email protected]>
As this responsability is now handled by the `SignedEntityConfig`.

Co-authored-by: Damien Lachaume <[email protected]>
* Remove now unused `network` field from `AggregatorConfig`.
* Make builder `get_signed_entity_config` pub like its other 'get'
  methods.
* fix the default `CardanoImmutableFilesFull` signed entity construction
  in the `/register-signatures` root handler.

Co-authored-by: Damien Lachaume <[email protected]>
From the aggregator configuration.

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: Damien Lachaume <[email protected]>
Before the were simply ignored

Co-authored-by: Sébastien Fauvel <[email protected]>
@Alenar Alenar force-pushed the ensemble/1697/timepoint_to_signed_entity branch from 1087a20 to bd0311e Compare June 7, 2024 10:38
@Alenar Alenar temporarily deployed to testing-preview June 7, 2024 10:45 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet June 7, 2024 10:45 — with GitHub Actions Inactive
Clarify some documentations and naming.

Co-authored-by: Sébastien Fauvel <[email protected]>
* Mithril-aggregator from `0.5.17` to `0.5.18`
* Mithril-signer from `0.2.141` to `0.2.142`
* Mithril-common from `0.4.13` to `0.4.14`
@Alenar Alenar force-pushed the ensemble/1697/timepoint_to_signed_entity branch from bd0311e to 179f66d Compare June 7, 2024 12:13
@Alenar Alenar temporarily deployed to testing-preview June 7, 2024 12:20 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet June 7, 2024 12:20 — with GitHub Actions Inactive
@Alenar Alenar merged commit f390dfb into main Jun 7, 2024
41 of 63 checks passed
@Alenar Alenar deleted the ensemble/1697/timepoint_to_signed_entity branch June 7, 2024 12:29
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