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

Feat: use stored signing configuration in aggregator when creating signed entity type #1988

Merged
merged 15 commits into from
Oct 10, 2024
Merged

Feat: use stored signing configuration in aggregator when creating signed entity type #1988

merged 15 commits into from
Oct 10, 2024

Conversation

dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Oct 8, 2024 •
edited
Loading

Content

This PR includes the use of the constant stored signing configurations for creating the signed entity type in the aggregator. This information is exposed by the epoch service.
Additionally, it removes the signed entity configuration from the aggregator’s dependency container.

The testing tools for the epoch service have also been simplified to improve ease of use and understanding.

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 #1961

@dlachaume dlachaume self-assigned this Oct 8, 2024
@dlachaume dlachaume changed the title Ensemble/1961/use stored signing config in aggregator Feat: use stored signing config in aggregator Oct 8, 2024
@dlachaume dlachaume changed the title Feat: use stored signing config in aggregator Feat: use stored signing configuration in aggregator when creating signed entity type Oct 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024 •
edited
Loading

Test Results

    4 files  ±0     54 suites  ±0   9m 27s ⏱️ +8s
1 338 tests +6  1 338 ✅ +6  0 💤 ±0  0 ❌ ±0 
1 546 runs  +6  1 546 ✅ +6  0 💤 ±0  0 ❌ ±0 

Results for commit 91f748e. ± Comparison against base commit ae674b6.

♻️ This comment has been updated with latest results.

@dlachaume dlachaume temporarily deployed to testing-sanchonet October 8, 2024 13:03 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-sanchonet October 8, 2024 14:02 — with GitHub Actions Inactive
@dlachaume dlachaume requested review from jpraynaud and Alenar October 8, 2024 14:07
@dlachaume dlachaume marked this pull request as ready for review October 8, 2024 14:07
jpraynaud
jpraynaud reviewed Oct 9, 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.

Overall looks good 👍 I left some comments below.

IMO the PR is too dense, which makes the review complicated and could have been split 😅

mithril-common/src/entities/signed_entity_config.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/middlewares.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/root_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 Outdated Show resolved Hide resolved
mithril-aggregator/src/services/epoch_service.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
Alenar
Alenar approved these changes Oct 9, 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/http_server/routes/root_routes.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/http_server/routes/root_routes.rs Outdated Show resolved Hide resolved
@dlachaume dlachaume requested a review from jpraynaud October 9, 2024 16:18
@dlachaume dlachaume temporarily deployed to testing-sanchonet October 9, 2024 16:27 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Oct 10, 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 👍

dlachaume and others added 12 commits October 10, 2024 10:25
@dlachaume @sfauvel
feat: add network and allowed_discriminants as parameters for `Mi…
b307ab0 
…thrilEpochService`

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume
feat: implement Signed entity configuration in Epoch Service
ac100a6
@sfauvel @dlachaume
Move outdated computation from state_machine to the runner
e54cd9a
@dlachaume @sfauvel
Remove cardano signing configuration in init_state_from_fixture par…
a678a92 
…ameters.

Instead, retrieve it from the aggregator configuration.

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume
Use SignedEntityConfig from the EpochService to create the signed…
3294864 
… entity type
@dlachaume @sfauvel @Alenar
Remove the signed entity config in the dependency container of the ag…
477f84e 
…gregator

- Add `allowed_discriminants` in the dependencies builder
- Remove `signed_entity_config` in `AggregatorConfig`

Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
Co-authored-by: DJO <Alenar@users.noreply.github.com>
@dlachaume @sfauvel @Alenar
Remove test function build_service in favor of EpochServiceBuilder
8f5ca53 
Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
Co-authored-by: DJO <Alenar@users.noreply.github.com>
@dlachaume
Refactor epoch service test tools
26be0d6
@dlachaume @sfauvel
Renaming and remove comments
65e8a7d 
Co-authored-by: Sébastien Fauvel <sfauvel@users.noreply.github.com>
@dlachaume
Refactor signed entity types construction in route route tests
0244426
@dlachaume
append_allowed_signed_entity_types_discriminants returns a new inst…
8d1d4e4 
…ance of the list instead of updating the given one
@dlachaume
Remove allowed_discriminants from dependencies builder
61c48e8
@dlachaume
Change the way to test is_open_message_outdated cases when there is…
2def8ad 
… a newer open message.

Now modifying the epoch message instead of the signed entity config parameter. We are not testing anymore that the function calls the epoch service.
@dlachaume
Refactor EpochServiceBuilder simplifying its inner parameters
85fa75b
@dlachaume
chore: upgrade crate versions
91f748e 
* mithril-aggregator from `0.5.77` to `0.5.78`
* mithril-common from `0.4.66` to `0.4.67`
@dlachaume dlachaume force-pushed the ensemble/1961/use-stored-signing-config-in-aggregator branch from f35cf39 to 91f748e Compare October 10, 2024 08:30
@dlachaume dlachaume temporarily deployed to testing-sanchonet October 10, 2024 08:38 — with GitHub Actions Inactive
@dlachaume dlachaume merged commit 6849c3a into main Oct 10, 2024
43 checks passed
@dlachaume dlachaume deleted the ensemble/1961/use-stored-signing-config-in-aggregator branch October 10, 2024 08:40
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

Assignees

@sfauvel sfauvel

@dlachaume dlachaume

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregator uses stored signing configurations when creating signed entity type
4 participants
@dlachaume @Alenar @jpraynaud @sfauvel