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 epoch mandatory in RegisterSignerMessage #1986

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Make epoch mandatory in RegisterSignerMessage #1986

merged 2 commits into from
Oct 8, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 7, 2024 •
edited
Loading

Content

This PR refactor the RegisterSignerMessage in order to make its epoch field mandatory, allowing to simplify the code that used it.

This is non-breaking since the epoch is sent by all signers since version 0.2.36 released with distribution 2318 more than a year and half ago (no signers are running older versions anymore).

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are 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 #1956

jpraynaud
jpraynaud approved these changes Oct 7, 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 👍

@github-actions GitHub Actions
Copy link

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

Test Results

    4 files  ±0     54 suites  ±0   9m 19s ⏱️ -1s
1 331 tests +1  1 331 ✅ +1  0 💤 ±0  0 ❌ ±0 
1 542 runs  +1  1 542 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 525751e. ± Comparison against base commit 322a8ce.

This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both. 
mithril-common ‑ messages::register_signer::tests::test_v1
mithril-common ‑ messages::register_signer::tests::test_v2

mithril-common ‑ messages::register_signer::tests::test_actual_json_deserialized_into_actual_message
mithril-common ‑ messages::register_signer::tests::test_actual_json_deserialized_into_message_supported_until_open_api_0_1_32
mithril-common ‑ messages::register_signer::tests::test_actual_json_deserialized_into_message_supported_until_open_api_0_1_8

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-sanchonet October 7, 2024 14:24 — with GitHub Actions Inactive
dlachaume
dlachaume approved these changes Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

sfauvel
sfauvel approved these changes Oct 8, 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
refactor: make epoch mandatory in RegisterSignerMessage
a1ba1e4 
This is non-breaking since the epoch is sent by all signers since
version `0.2.36` released with distribution `2318` more than a year and
half ago (no signers are running older versions anymore).
@Alenar
chore: upgrade crate versions
525751e 
* mithril-aggregator from `0.5.74` to `0.5.75`
* mithril-common from `0.4.64` to `0.4.65`
* mithril-signer from `0.2.194` to `0.2.195`
* mithril-end-to-end from `0.4.34` to `0.4.35`
@Alenar Alenar force-pushed the djo/1956/non_optional_epoch_in_register_signer_msg branch from ac75149 to 525751e Compare October 8, 2024 08:36
@Alenar Alenar temporarily deployed to testing-sanchonet October 8, 2024 08:44 — with GitHub Actions Inactive
@Alenar Alenar merged commit e4f8e3c into main Oct 8, 2024
43 checks passed
@Alenar Alenar deleted the djo/1956/non_optional_epoch_in_register_signer_msg branch October 8, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfauvel sfauvel sfauvel approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

@dlachaume dlachaume dlachaume 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.

Make epoch non optional in RegisterSignerMessage
4 participants
@Alenar @sfauvel @jpraynaud @dlachaume