Skip to content

Conversation

@richvdh
Copy link
Member

@richvdh richvdh commented Sep 10, 2025

PrivateCrossSigningIdentity::with_account is called in lots of places where we end up throwing away most of its work. Indeed, there is only one place that actually uses the results (olm::Account::bootstrap_cross_signing).

It makes more sense to me to replace it with something similar, and hoist the remaining functionality up to bootstrap_cross_signing.

@richvdh richvdh requested review from a team as code owners September 10, 2025 17:49
@richvdh richvdh requested review from Hywan and andybalaam and removed request for a team September 10, 2025 17:49
@richvdh richvdh force-pushed the rav/identity_test_cleanups branch 2 times, most recently from ba1c321 to f38695e Compare September 10, 2025 17:57
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 10, 2025

CodSpeed Performance Report

Merging #5654 will not alter performance

Comparing rav/identity_test_cleanups (0002ea4) with main (c2bc465)

Summary

✅ 49 untouched

@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.34%. Comparing base (4cc1cd1) to head (0002ea4).
⚠️ Report is 33 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5654      +/-   ##
==========================================
- Coverage   88.34%   88.34%   -0.01%     
==========================================
  Files         354      354              
  Lines       97098    97093       -5     
  Branches    97098    97093       -5     
==========================================
- Hits        85780    85772       -8     
- Misses       7265     7268       +3     
  Partials     4053     4053              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

I was just starting to write a rant about how with_account should not exist and then I read your second commit. Enthusiastic support here.

Comment on lines 11 to 13
- [**breaking**] Remove `matrix_sdk_crypto::olm::PrivateCrossSigningIdentity::with_account`. Applications should instead use `matrix_sdk_crypto::olm::Account::bootstrap_cross_signing` which has identical behaviour.
`PrivateCrossSigningIdentity::for_account` is added for test situations where the upload requests are not required.
([#5654](https://github.com/matrix-org/matrix-rust-sdk/pull/5654))
Copy link
Contributor

Choose a reason for hiding this comment

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

This was not a public method, as such this can't be a breaking change.

Do we now need to expose a public function for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was not a public method, as such this can't be a breaking change.

ohhh it was pub(crate), I missed that. And no, we don't need a public function. Let me have another go at this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@poljar I've made the new method pub(crate), and removed the changelog entry.

@richvdh richvdh requested a review from poljar September 11, 2025 17:10
It turns out that creating a cross-signing identity *without* the upload
requests is a very common thing to do, especially in tests. We can simplify
some code by factoring it out as a new helper.
This was now only used in one place, and I think it makes more sense to inline
it into olm::Account than leave it in `PrivateCrossSigningIdentity`.
@richvdh richvdh force-pushed the rav/identity_test_cleanups branch from d09ecb4 to 0002ea4 Compare September 12, 2025 13:04
@richvdh richvdh enabled auto-merge September 12, 2025 13:04
@richvdh richvdh merged commit b8cbd6c into main Sep 12, 2025
52 checks passed
@richvdh richvdh deleted the rav/identity_test_cleanups branch September 12, 2025 13:21
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.

4 participants