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

Change getSender method of SenderCertificate to always return the UUID #492

Closed
wants to merge 1 commit into from

Conversation

AsamK
Copy link
Contributor

@AsamK AsamK commented Oct 16, 2022

Currently the only user of this method is the ProtcolException constructor, when an UnidentifiedSenderMessageContent is present. All other instances of ProtocolException already use the sender's UUID as sender. So it would be good to have this consistent.

Also brings this in line with similar methods, like getSourceIdentifier on SignalServiceEnvelope in the Android app.

Currently the only user of this method is the ProtcolException constructor,
when a UnidentifiedSenderMessageContent is present.
All other instances of ProtocolException use the sender's UUID as sender.
So it would be good to have this consistent.

Also brings this in line with similar methods, like `getSourceIdentifier` on
SignalServiceEnvelope.
Copy link
Contributor

@jrose-signal jrose-signal left a comment

Choose a reason for hiding this comment

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

I checked with the Android team and they concur: while the current behavior made sense during the migration to UUIDs, when local recipient info was potentially in terms of just e164s (or preferring e164s), it's no longer our source of truth, and we don't need to keep this behavior from libsignal-metadata-java moving forward.

I'll cherry-pick this into our private branch and it will go out with the next release.

@jrose-signal
Copy link
Contributor

Released in v0.21.1 as ad1fabb.

@AsamK AsamK deleted the protocol-exception-uuid branch October 20, 2022 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants