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

Upgrade ed25519-dalek to v2 #1189

Merged
merged 4 commits into from Sep 1, 2023
Merged

Upgrade ed25519-dalek to v2 #1189

merged 4 commits into from Sep 1, 2023

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Aug 29, 2023 •
edited by jpraynaud

Content

This PR upgrades Mithril common ed25519-dalek dependency from version 1.0.1 to 2.0.0, adapting to the new api where needed.

This will solve the following dependabot issue: https://github.com/input-output-hk/mithril/security/dependabot/55

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

Should we rename types to match their updated wrapped inner type ? ie: ProtocolGenesisSecretKey refer to a ProtocolKey<ed25519_dalek::SigningKey> and could be renamed to ProtocolGenesisSigningKey.

Issue(s)

Closes #1188

@Alenar Alenar requested a review from jpraynaud August 29, 2023 17:22
@github-actions
Copy link

github-actions bot commented Aug 29, 2023 •
edited

Test Results

    3 files  ±0    17 suites  ±0   6m 25s ⏱️ +59s
671 tests ±0  671 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
727 runs  ±0  727 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit be9fdce. ± Comparison against base commit c590faa.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview August 29, 2023 17:31 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Aug 30, 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 comments on renaming the Keypairs aliases

mithril-common/src/crypto_helper/cardano/cold_key.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/cardano/opcert.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/cardano/opcert.rs Outdated Show resolved Hide resolved
@Alenar
Copy link
Collaborator Author

Alenar commented Aug 30, 2023

Can one of you take a look @iquerejeta @curiecrypt ?

@Alenar Alenar temporarily deployed to testing-preview August 30, 2023 16:21 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-preview August 31, 2023 09:15 — with GitHub Actions Inactive
curiecrypt
curiecrypt approved these changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

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

It seems OK with the renaming of the Keypairs aliases suggested by @jpraynaud. LGTM 👍

distributedstatemachine
distributedstatemachine reviewed Aug 31, 2023
View reviewed changes
Copy link

@distributedstatemachine distributedstatemachine left a comment

Choose a reason for hiding this comment

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

Started working on this before realising this PR was out. Putting thoughts here

mithril-common/src/crypto_helper/cardano/cold_key.rs Show resolved Hide resolved
mithril-common/src/crypto_helper/cardano/opcert.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/era.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/era.rs Outdated Show resolved Hide resolved
mithril-common/src/crypto_helper/era.rs Outdated Show resolved Hide resolved
@Alenar Alenar temporarily deployed to testing-preview August 31, 2023 14:29 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-preview August 31, 2023 14:47 — with GitHub Actions Inactive
@Alenar Alenar merged commit a6caa1c into main Sep 1, 2023
25 checks passed
@Alenar Alenar deleted the djo/upgrade_dalek_to_v2 branch September 1, 2023 08:19
@jpraynaud jpraynaud mentioned this pull request Sep 1, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abailly-iohk abailly-iohk abailly-iohk left review comments

@distributedstatemachine distributedstatemachine distributedstatemachine left review comments

@jpraynaud jpraynaud jpraynaud approved these changes

@curiecrypt curiecrypt curiecrypt approved these changes

@iquerejeta iquerejeta Awaiting requested review from iquerejeta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update ed25519-dalek to 2.0.0
5 participants
@Alenar @jpraynaud @curiecrypt @abailly-iohk @distributedstatemachine