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: Modernize usage of Tink #266

Closed
wants to merge 3 commits into from

Conversation

tholenst
Copy link
Contributor

KeysetReader/Writer objects will go away. We use TinkProtoKeysetFormat instead.

This is a pure refactoring.

KeysetReader/Writer objects will go away. We use TinkProtoKeysetFormat
instead.

This is a pure refactoring.
@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas self-requested a review August 28, 2024 19:23
@SanjayVas SanjayVas changed the title Modernize usage of Tink. refactor: Modernize usage of Tink Aug 28, 2024
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

KeysetReader/Writer objects will go away.

Can you link to any public documentation on this? I don't see any deprecation warnings on the interfaces.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tholenst)

a discussion (no related file):
Approved pending lint fixes.

Note that my approval is not sufficient to merge this PR. This is because we have a two-organization change policy, where every code change must involve at least two member organizations. Since we're both Googlers, there's no other organization yet involved. I can add an external reviewer for you and shepherd this through to the downstream repos once the linter errors are fixed.



src/main/kotlin/org/wfanet/measurement/common/crypto/tink/KeyHandle.kt line 36 at r1 (raw file):

  fun toByteString(): ByteString {
    return ByteString.copyFrom(TinkProtoKeysetFormat.serializeKeysetWithoutSecret(keysetHandle));

It's unfortunate that the new interface doesn't offer a way to avoid the extra copies :(

@tholenst
Copy link
Contributor Author

KeysetReader/Writer objects will go away.

Can you link to any public documentation on this? I don't see any deprecation warnings on the interfaces.

The Tink team currently has the policy that all Google internal usages need to be removed before we add deprecation warnings. This is to avoid spamming deprecation warnings on things which we never remove anyhow.

This is the last instance of this API within Google.

It's unfortunate that the new interface doesn't offer a way to avoid the extra copies :(

In a sense yes. The problem is that we don't want to require a Protobuf dependency on the main part of Tink, so we opted for byte[]. But yes, it has disadvantages too :(

@tholenst
Copy link
Contributor Author

tholenst commented Sep 4, 2024

@SanjayVas: would you mind adopting this PR instead? I think if one has everything set up (with the linting) and if one is familiar with git things become much more efficient. Since I don't have the linting set up locally and it seems I cannot run the check myself and since I'm not too familiar with git things become a bit more difficult.

WDYT?

@SanjayVas
Copy link
Member

@tholenst I was going to suggest that as it seemed like you were having some difficulty.

@SanjayVas
Copy link
Member

Created #269 to replace this.

@SanjayVas SanjayVas closed this Sep 4, 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.

3 participants