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

refactor(crypto): CRP-2597 move MasterPublicKeyId protobuf from registry/crypto to types #2406

Merged

Conversation

fspreiss
Copy link
Member

@fspreiss fspreiss commented Nov 4, 2024

Moves the MasterPublicKeyId Protocol Buffers message definition, and subsequently also the contained EcdsaCurve, EcdsaKeyId, SchnorrAlgorithm, SchnorrKeyId, VetKdCurve, VetKdKeyId definitions from the registry.crypto.v1 package to the types.v1 package, without making any changes to the types in the process.

This step is necessary so that we can later add an optional MasterPublicKeyId key_id = 6; field to the types.v1.NiDkgId message. Without the move, this would create a circular dependency between registry/crypto/v1/crypto.proto and types/v1/types.proto because registry/crypto/v1.crypto.proto imports types/v1/types.proto (so as to use types.v1.SubnetId in the ChainKeySigningSubnetList);

Given that all message definitions remain unchanged and we only change the package, the wire format of the messages is unchanged, which should make this change safe. However, changes like this lead to the //pre-commit:buf-breaking test to fail (even though buf.yaml is configured to use WIRE), which is a known deficiancy in the underlying buf compatibility-check library: bufbuild/buf#2318. Because of this, we set the CI_OVERRIDE_BUF_BREAKING flag to override this.

@fspreiss fspreiss added CI_OVERRIDE_BUF_BREAKING Skip buf-breaking (protobuf) check (explain in PR description why) and removed CI_OVERRIDE_BUF_BREAKING Skip buf-breaking (protobuf) check (explain in PR description why) labels Nov 6, 2024
@fspreiss fspreiss changed the base branch from master to basvandijk/run-buf-breaking-on-proto-changes November 6, 2024 12:06
Base automatically changed from basvandijk/run-buf-breaking-on-proto-changes to master November 6, 2024 12:35
@fspreiss fspreiss added the CI_OVERRIDE_BUF_BREAKING Skip buf-breaking (protobuf) check (explain in PR description why) label Nov 6, 2024
Copy link
Contributor

@dragoljub-duric dragoljub-duric left a comment

Choose a reason for hiding this comment

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

/rs/replicated_state and /rs/types/management_canister_types LGTM.

@fspreiss fspreiss added this pull request to the merge queue Nov 9, 2024
Merged via the queue into master with commit 3c3d9cd Nov 9, 2024
24 checks passed
@fspreiss fspreiss deleted the franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types branch November 9, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants