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

ValidatorName->ValidatorPublicKey #2827

Closed
wants to merge 1 commit into from

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Nov 6, 2024

Motivation

ValidatorName seems confusing, because we have actual names for validators now (azurenode, lavanderfive, etc), and this is not it. It's the public key of the validator.

Proposal

Change ValidatorName to ValidatorPublicKey.
If you disagree with this, speak now or forever hold your peace :)
Fixes #2820

Test Plan

CI

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Nov 6, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ndr-ds and the rest of your teammates on Graphite Graphite

@ndr-ds ndr-ds force-pushed the 11-06-validatorname-_validatorpublickey branch 3 times, most recently from cb23746 to d239f58 Compare November 6, 2024 03:49
@ndr-ds ndr-ds marked this pull request as ready for review November 6, 2024 04:16
Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

This is a breaking change (APIs, schemas, CLI args, etc change). I don't know if we want to add this - potential - integration issues at the eve of release.

Twey
Twey previously approved these changes Nov 6, 2024
@Twey Twey dismissed their stale review November 6, 2024 11:21

Revoked pending internal discussion

Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

needs discussion

@@ -11,7 +11,7 @@ use crate::test::{make_first_block, BlockTestExt};
fn test_signed_values() {
let key1 = KeyPair::generate();
let key2 = KeyPair::generate();
let name1 = ValidatorName(key1.public());
let name1 = ValidatorPublicKey(key1.public());
Copy link
Contributor

Choose a reason for hiding this comment

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

These renamings are always work work because we forget places like here, the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! Let me do a pass and see if I forgot anywhere else

@ndr-ds ndr-ds force-pushed the 11-06-validatorname-_validatorpublickey branch 2 times, most recently from 8640ab0 to 5b738ea Compare November 6, 2024 14:10
@ndr-ds ndr-ds requested review from ma2bd, deuszx and Twey November 6, 2024 14:50
@ndr-ds ndr-ds force-pushed the 11-06-validatorname-_validatorpublickey branch from 5b738ea to 7cfe386 Compare November 13, 2024 16:09
@ndr-ds ndr-ds closed this Nov 13, 2024
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.

Consider renaming ValidatorName into ValidatorPublicKey
4 participants