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

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

Draft
wants to merge 31 commits into
base: franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types
Choose a base branch
from

Conversation

fspreiss
Copy link
Member

@fspreiss fspreiss commented Nov 5, 2024

Extends the NiDkgTag with a new variant NiDkgTag::HighThresholdForKey that holds a MasterPublicKeyId.

Notes:

  • Makes the representation for NiDkgTag explicit with #[repr(isize)] because a #[repr(inttype)] must be provided on an enum if it has a non-unit variant with a discriminant (E0732). The representation should be the same as it was before, given that isize is the default representation for enums.

    Note that casting NiDkgTag with as, e.g., to i32 or even isize is no longer possible because "an as expression can be used to convert enum types to numeric types only if the enum type is unit-only or field-less" (see Enumeration casting and E0605).

  • The conversion impl TryFrom<i32> for NiDkgTag had to be removed, because it is no longer possible, because an i32 itself is no longer sufficient for instantiating an NiDkgTag.

    This conversion was used in two places: (1) impl TryFrom<NiDkgIdProto> for NiDkgId and (2) build_tagged_transcripts_map. There, the conversion is now implemented directly.

  • No longer Implements EnumIter for NiDkgTag because this would require a Default implementation for MasterPublicKeyId, which does not make sense, at least in production. Instead implements EnumCount.

TODOs:

  • Address the issue that //rs/tests/consensus/orchestrator:cup_compatibility_test fails
  • Naming: NiDkgTag::HighThresholdForKey vs NiDkgTag::HighThresholdForKeyId
  • use optional in new protobuf fields?: Rust struct definitions are the same
  • decide how to deal with #[allow(clippy::result_large_err)] in crypto code
  • add tests for changes in rs/crypto/src/sign/threshold_sig/store.rs

@github-actions github-actions bot added the feat label Nov 5, 2024
@fspreiss fspreiss changed the base branch from franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types-ontopof-non-copy-ni-dkg-id to franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types November 6, 2024 08:38
@fspreiss fspreiss changed the title feat(crypto): CRP-2597-extend-nidkg-tag-with-high-threshold-for-keyid-variant feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant