feat(sdk): Poll encryption state on sync event for up to 3 seconds. #5559
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5559 +/- ##
=======================================
Coverage 88.63% 88.64%
=======================================
Files 340 340
Lines 95102 95112 +10
Branches 95102 95112 +10
=======================================
+ Hits 84298 84308 +10
- Misses 6616 6617 +1
+ Partials 4188 4187 -1 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5559 will not alter performanceComparing Summary
|
Fixes Room::enable_encryption_inner to break out of polling the encryption state when it becomes known, rather than just encrypted.
|
Okay, now this makes sense to me but I would still like somebody from the crypto team to have a quick look. |
stefanceriu
requested review from
a team and
richvdh
and removed request for
a team
kaylendog
force-pushed
the
kaylendog/encrypt-poll-sync
branch
from
b4a0220 to
24502d2
Compare
| // If encryption was enabled, return. | ||
| #[cfg(not(feature = "experimental-encrypted-state-events"))] | ||
| if res.is_ok() && self.inner.encryption_state().is_encrypted() { | ||
| debug!("room successfully marked as encrypted"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| self.client.state_store().save_changes(&changes).await?; | ||
| self.set_room_info(room_info, RoomInfoNotableUpdateReasons::empty()); | ||
| } else { | ||
| // If encryption with state event encryption was enabled, return. | ||
| #[cfg(feature = "experimental-encrypted-state-events")] | ||
| if res.is_ok() && { | ||
| if encrypted_state_events { | ||
| self.inner.encryption_state().is_state_encrypted() | ||
| } else { | ||
| self.inner.encryption_state().is_encrypted() | ||
| } | ||
| } { | ||
| debug!("room successfully marked as encrypted"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This might belong in its own PR?
There was a problem hiding this comment.
you mean, separate the "poll for 3 seconds" part from the "make it support encrypted state" part? Yes, a separate PR would be optimal, but I think it's fine in practice.
This fixes a small issue encountered while developing the integration test for encrypted state events. If the first sync response received from the server after enabling encryption does not contain the
m.room.encryptionevent, the client will report the encryption state as unknown, even if the next sync response does contain the event.Room::enable_encryption_innerto spin on the room encryption state, rather than bailing after the first sync response is received.Signed-off-by: Skye Elliot