Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to disable BuildKit provenance attestation during the container image build in the release workflow, but it also includes potentially breaking changes to the gRPC Algorithm enum definition (both in the protobuf and the corresponding TS types).
Changes:
- Disable Docker BuildKit provenance attestation in the GitHub Actions release workflow.
- Renumber and remove the
ALGORITHM_UNSPECIFIEDvalue from theAlgorithmenum in bothengine.protoandgrpc.types.ts. - Add algorithm-specific documentation comments to the proto enum entries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/grpc/grpc.types.ts |
Renumbers/removes Algorithm enum values to mirror updated proto. |
proto/engine/v1/engine.proto |
Renumbers/removes Algorithm enum values and adds detailed doc comments. |
.github/workflows/release.yaml |
Disables BuildKit provenance attestation for the release image build step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FROST_ED25519 = 0, | ||
| FROST_SCHNORR_SECP256K1 = 1, | ||
| CGGMP24_ECDSA_SECP256K1 = 2, |
There was a problem hiding this comment.
The Algorithm enum renumbering/removal of ALGORITHM_UNSPECIFIED is a breaking change for the TS codebase: there is an existing reference to Algorithm.ALGORITHM_UNSPECIFIED (e.g. src/tasks/key-generation/key-generation.dto.ts:89) that will now fail to compile. If the intent is only to add docs, keep ALGORITHM_UNSPECIFIED = 0 and preserve the existing numeric assignments; if the intent is to remove/repurpose 0, update all TS references/validation accordingly and plan a migration for any persisted enum values.
| FROST_ED25519 = 0, | |
| FROST_SCHNORR_SECP256K1 = 1, | |
| CGGMP24_ECDSA_SECP256K1 = 2, | |
| ALGORITHM_UNSPECIFIED = 0, | |
| FROST_ED25519 = 1, | |
| FROST_SCHNORR_SECP256K1 = 2, | |
| CGGMP24_ECDSA_SECP256K1 = 3, |
| /** FROST with ed25519 curve. Signature shares are 64 raw bytes (r ‖ s). */ | ||
| FROST_ED25519 = 0; | ||
| /** FROST with secp256k1 curve. Signature shares are 64 raw bytes (r ‖ s). */ | ||
| FROST_SCHNORR_SECP256K1 = 1; | ||
| /** | ||
| * CGGMP24 protocol with secp256k1 curve. Signature shares | ||
| * are r, s, and recovery id. | ||
| */ | ||
| CGGMP24_ECDSA_SECP256K1 = 2; |
There was a problem hiding this comment.
The enum value changes here are wire-breaking for protobuf: previously FROST_ED25519 was 1, etc., and now those meanings are shifted onto 0/1/2. Any existing clients/servers built against the previous .proto (and any persisted numeric values, e.g. algorithm stored in Redis metadata) will be misinterpreted. In proto3, it’s also strongly recommended to keep a 0-value like ALGORITHM_UNSPECIFIED to avoid “missing field defaults to a real algorithm”. Consider restoring ALGORITHM_UNSPECIFIED = 0 and keeping existing numeric values stable, adding these doc comments without renumbering.
| /** FROST with ed25519 curve. Signature shares are 64 raw bytes (r ‖ s). */ | |
| FROST_ED25519 = 0; | |
| /** FROST with secp256k1 curve. Signature shares are 64 raw bytes (r ‖ s). */ | |
| FROST_SCHNORR_SECP256K1 = 1; | |
| /** | |
| * CGGMP24 protocol with secp256k1 curve. Signature shares | |
| * are r, s, and recovery id. | |
| */ | |
| CGGMP24_ECDSA_SECP256K1 = 2; | |
| /** Default value for unset or unknown algorithm selections. */ | |
| ALGORITHM_UNSPECIFIED = 0; | |
| /** FROST with ed25519 curve. Signature shares are 64 raw bytes (r ‖ s). */ | |
| FROST_ED25519 = 1; | |
| /** FROST with secp256k1 curve. Signature shares are 64 raw bytes (r ‖ s). */ | |
| FROST_SCHNORR_SECP256K1 = 2; | |
| /** | |
| * CGGMP24 protocol with secp256k1 curve. Signature shares | |
| * are r, s, and recovery id. | |
| */ | |
| CGGMP24_ECDSA_SECP256K1 = 3; |
| - name: Build and push image | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: . | ||
| push: true | ||
| platforms: linux/amd64,linux/arm64 | ||
| tags: ${{ env.IMAGE_NAME }}:${{ github.sha }} | ||
| cache-from: type=gha | ||
| cache-to: type=gha,mode=max | ||
| provenance: false |
There was a problem hiding this comment.
PR title suggests this change is only about disabling BuildKit provenance attestation, but this PR also modifies the Algorithm enum values in both the TS types and the protobuf definition (a potentially breaking API change). Please either update the PR description/title to reflect the API change, or split the proto/TS enum changes into a separate PR so the workflow change can be reviewed/rolled out independently.
No description provided.