Skip to content

Conversation

cedric-cordenier
Copy link
Contributor

Requires

Supports

Copy link

github-actions bot commented Jul 1, 2025

👋 @cedric-cordenier, thanks for creating this pull request!

To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team.

Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks!

SecretIdentifier id = 1;
string encrypted_value = 2;
repeated EncryptedShares encrypted_decryption_key_shares = 3;
string error = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: proto allows us to tag some fields as optional, to indicate they can be omitted, and easily checked in code with logic.

Example :
message SecretResponse {
.
.
optional string encrypted_value = 2;
optional string error = 4;
}

Then in go code:
secretResponse.has_error() OR
secretResponse.has_encryptedValue()

This is instead of directly checking if these are empty strings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My preference would be to split this into



message SecretResponse {
  SecretIdentifier id = 1;

  oneof result {
    SecretData success_data = 2;
    string error = 3;
  }
}

message SecretData {
  string encrypted_value = 1;
  repeated EncryptedShares encrypted_decryption_key_shares = 2;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the oneof :)

@cedric-cordenier cedric-cordenier enabled auto-merge (squash) July 2, 2025 09:57
@cedric-cordenier cedric-cordenier merged commit 455da70 into main Jul 2, 2025
14 of 16 checks passed
@cedric-cordenier cedric-cordenier deleted the PRIV-97-CRUDL-protos branch July 2, 2025 10:09
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.

5 participants