-
Notifications
You must be signed in to change notification settings - Fork 329
feat(base): Add EncryptionState::StateEncrypted
#5523
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(base): Add EncryptionState::StateEncrypted
#5523
Conversation
697aab6
to
751f635
Compare
CodSpeed Performance ReportMerging #5523 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5523 +/- ##
=======================================
Coverage 88.59% 88.59%
=======================================
Files 340 340
Lines 93836 93836
Branches 93836 93836
=======================================
+ Hits 83133 83135 +2
+ Misses 6572 6571 -1
+ Partials 4131 4130 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch!
I believe these is a design issue with the new StateEncrypted
variant. See my comment.
751f635
to
7345c02
Compare
7345c02
to
cfd8166
Compare
Looks like this breaks the Go FFI bindings in |
EncryptionState::StateEncrypted
EncryptionState::Encrypted { encrypt_state_events }
Yes, you may need to update Complement Crypto. Ideally, you can open a branch on Complement Crypto with the same name as your branch here, to trigger a special build that will help tests both PR at the same time. |
Signed-off-by: Skye Elliot <[email protected]>
cfd8166
to
18b5679
Compare
EncryptionState::Encrypted { encrypt_state_events }
EncryptionState::StateEncrypted
Reverted back to the enum variant after the discussion with @richvdh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continue to believe the changes I've recommended before are the way to go, but @richvdh brought a pretty relevant argument in favor of the current solution I'm about the approve. My solution is a breaking change, the new solution isn't a breaking change. Knowing this is an experimental feature, avoiding a breaking change is definitely better. We can still refactor later once it's stablised.
EncryptionState::StateEncrypted
to represent the encryption state of rooms supporting state event encryption.