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

Separate Stake and Stake Pool keys #495

Merged
merged 4 commits into from
Feb 19, 2025
Merged

Separate Stake and Stake Pool keys #495

merged 4 commits into from
Feb 19, 2025

Conversation

LGLO
Copy link
Contributor

@LGLO LGLO commented Feb 17, 2025

Description

Separates StakePoolPublicKey and StakePublicKey and their respective signatures into separate types.
Before there was just MainchainPublicKey, but I argue that Stake key and StakePool key are two different things in the domain, used for different activities.

Please let me know what you think

This PR doesn't separate their hashes, it still is MainchainKeyHash.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages.
  • New tests are added if needed and existing tests are updated.
  • Relevant logging and metrics added
  • CI passes. See note on CI.
  • Any changes are noted in the changelog.md for affected crate
  • Self-reviewed the diff

Copy link
Contributor

@AmbientTea AmbientTea left a comment

Choose a reason for hiding this comment

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

splendid

Copy link
Contributor

@kpinter-iohk kpinter-iohk left a comment

Choose a reason for hiding this comment

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

I really like this change. In some places the variable names were not updated to match the new type, I marked these with suggestions. I got into it and by the time I realized how spammy this could be on GH I was nearly done already, sorry about that. If you prefer I could make these fixes myself and push on your branch.

@LGLO
Copy link
Contributor Author

LGLO commented Feb 18, 2025

I really like this change. In some places the variable names were not updated to match the new type, I marked these with suggestions. I got into it and by the time I realized how spammy this could be on GH I was nearly done already, sorry about that. If you prefer I could make these fixes myself and push on your branch.

I haven't mostly changed variables names. Perhaps some could be changed, but I am not going to break any existing interface that has been released (v1.5.0 or earlier) for sake of matching them. No CLI params changes. No JSON fields name changes. No extrinsic calls changes.

@kpinter-iohk
Copy link
Contributor

kpinter-iohk commented Feb 18, 2025

No CLI params changes

Not even with an alias, preserving compatibility, like I suggested?

Copy link
Contributor

@kpinter-iohk kpinter-iohk left a comment

Choose a reason for hiding this comment

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

🎉

@LGLO LGLO force-pushed the make-some-order-in-keys branch from 58f7c93 to 0b84113 Compare February 19, 2025 10:09
@LGLO LGLO enabled auto-merge (squash) February 19, 2025 10:10
@LGLO LGLO disabled auto-merge February 19, 2025 10:15
@LGLO LGLO enabled auto-merge (squash) February 19, 2025 10:15
@LGLO LGLO merged commit 1ecca19 into master Feb 19, 2025
23 checks passed
@LGLO LGLO deleted the make-some-order-in-keys branch February 19, 2025 10:37
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.

3 participants