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

Ensemble/668/refacto crypto entities #1075

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Ensemble/668/refacto crypto entities #1075

merged 5 commits into from
Jul 24, 2023

Conversation

ghubertpalo
Copy link
Collaborator

@ghubertpalo ghubertpalo commented Jul 21, 2023 •
edited
Loading

Content

This PR includes a refactoring of the Verification key. This used to be stored as string internally and format changes occurred all over the place. It is now handled by a type using binary memory storage and owns its type exchanging methods.

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

Comments

There is still a refactoring about the message parts to be done after this PR.

Issue(s)

Relates to #668

@ghubertpalo ghubertpalo requested review from Alenar and jpraynaud and removed request for Alenar July 21, 2023 13:23
@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited
Loading

Test Results

    3 files  ±0    16 suites  ±0   6m 5s ⏱️ +39s
639 tests +4  639 ✔️ +4  0 💤 ±0  0 ❌ ±0 
677 runs  +4  677 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit b125a2b. ± Comparison against base commit 9fe2a48.

♻️ This comment has been updated with latest results.

@ghubertpalo ghubertpalo temporarily deployed to testing-preview July 21, 2023 13:31 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Jul 24, 2023
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 👍

Few typos and ideas below!

mithril-common/src/crypto_helper/mod.rs
@@ -19,6 +20,7 @@ pub use era::{
};
pub use genesis::{ProtocolGenesisError, ProtocolGenesisSigner, ProtocolGenesisVerifier};
pub use types::*;
pub use verification_key::*;
Copy link
Member

Choose a reason for hiding this comment

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

Idea: we could create a types module for all the wrapped types we will implement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! IMHO that should be in a separate PR.

mithril-common/src/crypto_helper/verification_key.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/verification_key.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/verification_key.rs Outdated Show resolved Hide resolved
mithril-common/src/entities/certificate_metadata.rs
vec![
SignerWithStake::new(
"1".to_string(),
"7b22766b223a5b3134332c3136312c3235352c34382c37382c35372c3230342c3232302c32352c3232312c3136342c3235322c3234382c31342c35362c3132362c3138362c3133352c3232382c3138382c3134352c3138312c35322c3230302c39372c39392c3231332c34362c302c3139392c3139332c38392c3138372c38382c32392c3133352c3137332c3234342c38362c33362c38332c35342c36372c3136342c362c3133372c39342c37322c362c3130352c3132382c3132382c39332c34382c3137362c31312c342c3234362c3133382c34382c3138302c3133332c39302c3134322c3139322c32342c3139332c3131312c3134322c33312c37362c3131312c3131302c3233342c3135332c39302c3230382c3139322c33312c3132342c39352c3130322c34392c3135382c39392c35322c3232302c3136352c39342c3235312c36382c36392c3132312c31362c3232342c3139345d2c22706f70223a5b3136382c35302c3233332c3139332c31352c3133362c36352c37322c3132332c3134382c3132392c3137362c33382c3139382c3230392c34372c32382c3230342c3137362c3134342c35372c3235312c34322c32382c36362c37362c38392c39372c3135382c36332c35342c3139382c3139342c3137362c3133352c3232312c31342c3138352c3139372c3232352c3230322c39382c3234332c37342c3233332c3232352c3134332c3135312c3134372c3137372c3137302c3131372c36362c3136352c36362c36322c33332c3231362c3233322c37352c36382c3131342c3139352c32322c3130302c36352c34342c3139382c342c3136362c3130322c3233332c3235332c3234302c35392c3137352c36302c3131372c3134322c3131342c3134302c3132322c31372c38372c3131302c3138372c312c31372c31302c3139352c3135342c31332c3234392c38362c35342c3232365d7d".try_into().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Idea: we could avoid hard coding these cumbersome values here and generally in the tests?

mithril-common/src/entities/signer.rs Outdated Show resolved Hide resolved
@ghubertpalo ghubertpalo force-pushed the ensemble/668/refacto_crypto_entities branch from 4bb71b6 to 0d3f410 Compare July 24, 2023 14:21
@ghubertpalo ghubertpalo marked this pull request as ready for review July 24, 2023 14:21
@ghubertpalo ghubertpalo force-pushed the ensemble/668/refacto_crypto_entities branch from 0d3f410 to b125a2b Compare July 24, 2023 14:54
@ghubertpalo ghubertpalo temporarily deployed to testing-preview July 24, 2023 15:02 — with GitHub Actions Inactive
@ghubertpalo ghubertpalo merged commit c2e8686 into main Jul 24, 2023
24 checks passed
@ghubertpalo ghubertpalo deleted the ensemble/668/refacto_crypto_entities branch July 24, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alenar Alenar Alenar approved these changes

@jpraynaud jpraynaud jpraynaud 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.
3 participants
@ghubertpalo @Alenar @jpraynaud