Splitting Store implementation into separate crates

[gnunicorn]

This change attempts to move the concrete state & crypto-store implementations from the core crates into separate crates to allow for easier plugability.

This requires:

• move IndexedDB Stores into separate crate
• move Sled Stores into a separate crate
• redo client builder infrastructure
• document all newly exposed functions
• update examples
• clean up testing-infrastructure

[jsparber]

clean up testing-infrastructure

Can we drop the test macro, because with the macro it's pretty annoying to figure out which test fails where. Not sure what the best option here is but we could move it to a test crate.

[gnunicorn]

@jsparber Can we drop the test macro, because with the macro it's pretty annoying to figure out which test fails where. Not sure what the best option here is but we could move it to a test crate.

I'd love to drop the macro, but afaik, it is the only way to have the same tests replicated over multiple crates - most problematic is for the wasm-pack tests, which only ever runs tests defined directly in the crate. So not even moving to a test-crate works.

The only other way to achieve this would be to duplicate the tests over all crates, with the obvious disadvantages of running out of sync over time and not having the new features tested until everyone copied them over ... Considering that we don't do proper coverage reports on PRs yet, this is something that will likely be forgotten often.

While I agree it isn't a great solution and agree the problems suck, I do not know about any better option that allows us to achieve a single test suite that is run over all implementations through cargo test (or similar)... if you have proposals I am all open for it!

[poljar]

Couple of nits, benchmarks have changed semantics and a dead link needs to be removed.

[poljar]

Suggested change

	# State Key is a helper feature for state store implementations
	# Store Key is a helper feature for state store implementations

FYI, I mean to rework this as part of the vodozemac work. Create a new crate which implements a StoreKey struct which then enables keys and values to be encrypted by stores in a uniform way.

This would remove duplication between the crypto crate and the base crate as well.

[gnunicorn]

happy to move it out of here - it felt weird to have a standalone feature like this in here, but didn't really want to create more single-type-crates ...

[poljar]

We had a Ruma re-export in the common crate, now we only re-export from the main crate. @jplatte preferred if we directly depend on Ruma in each crate, actually AFAIR he preferred no rust-sdk re-exports of Ruma at all.

[jplatte]

I definitely think the current re-export has some value even though I expect myself to stop using it in my one SDK-dependent project once I no longer use it as a git dep.

For a re-export from -common I'm definitely wondering what the benefit would be.

[gnunicorn]

My main take on re-exports is, that they must be considered if the API of the given crate export them directly (either as param or return value), one must consider exporting the given type directly for ease of use from the lowest point. The background is that if you have multiple dependencies in your try depending on different (rather than a common, re-exported) versions of the same dependency the highest level will end up in an unresolvable incompatible state. This is particularly annoying when mixing git and release-versions.

Example:

Client
 +- agent-smith
 |  +- ruma 0.4
 |  +- matrix-common
 |
 +- matrix-sdk
 |  +- matrix-crypto
 |  |  +- ruma 0.5
 |  +- matrix-common

Now any ruma-type returned by agent-smith will be incompatible with the matrix-sdk input ruma-types - even if they aren't any different at all. This leads to a rust build error on incompatible types, that can't be resolved by Client but only by agent-smith releasing another version of the library now depending on ruma 0.5.

If you are leaking types from a dependency through your API, you are already exporting them (and they are often forgotten or ignored when it comes to SemVer). Not re-exporting the crate as a whole just makes it a lot harder for someone to build upon them and keep things up to date in the longer-term, especially once other dependencies are involved.

Through the lowest API exporting the used version, everything building upon can just build upon the export and thus be sure to always be compatible, rather than looking up the version used in the dependency and adding it to their own Cargo.toml. Arguably, whenever that lowest crate changes the dependency in a non-semver-compatible way, it must release a new major version, too, to avoid the problem mentioned above (which is rarely done properly). The alternative is to not export types of dependencies, but that seems inconvenient for us for now.

I know re-exports are controversial, but I wanted to explain the reasoning behind it and why I think in particular ruma makes a lot of sense for us to export (we depend highly on it for various types).

[jplatte]

By this argument, every library should re-exports every public dependency. This is not at all how things work in the overall Rust ecosystem and I think the Matrix SDK shouldn't stray too far from that.

I just don't think that the problem being solved is big enough to go for such an "ugly" and incomplete solution¹, with the error messages for that kind of type mismatch being pretty good these days and Cargo has support for explicitly marking dependencies as public or private which should help as well (though there doesn't seem to be any recent movement towards stabilization unfortunately – rust-lang/cargo#6129).

¹ Why I say that is:

• I like to nest my imports and with re-exports you get extra unnecessary rightwards drift there
• Boundaries between crates / projects can get less clear
• It's also unclear where you import things from when multiple crates re-export them, a real-world example I've come across is reqwest's re-exports of http crate types and the whole-crate axum::http re-export (though I was actually in favor of that particular one being added)
• You can still have multiple dependencies to different versions of one lower-level crate

[gnunicorn]

By this argument, every library should re-exports every public dependency.

If they are fundamental, in that they themselves export them via their API, yes, probably. The argument still holds water and I don't think you pointing out its final consequence changes much about that.

The user of the API would need it anyways to use the API, it would have to resolve to the exact same version or you can't use the API, why would you not?

This is not at all how things work in the overall Rust ecosystem and I think the Matrix SDK shouldn't stray too far from that.

Just pointing out that below you are naming the popular reqwest-library as an example doing it and I'd also add the widely popular parking_lot, exporting the lower-level lock_api to that list. So I reject the premise that "this is not how things work in overall Rust ecosystem" - and even if, this might be because this problem hasn't been run into as much yet. "It is not being done much yet" is, by itself, not really an argument...

I like to nest my imports and with re-exports you get extra unnecessary rightwards drift there

What do you mean with rightwards drift?

Boundaries between crates / projects can get less clear

Arguably, they already are because the API requires and is exporting types external to its crate

(Which makes navigating the docs pretty annoying, btw, as you can't click half the params and return types but instead have to navigate to the ruma-api yourself every time, ensure you are looking at the exact same version and find the type manually. I am not sure if the re-export fixes that (especially for higher level APIs, like if matrix-sdk used a ruma-type), but it seems to do so on parking_lot.)

You can still have multiple dependencies to different versions of one lower-level crate

You mean despite re-exporting, you can still have different versions? Yeah, sure, that's true either way, and thus not really an argument against it though. Never said, it would solve that issue.

It's also unclear where you import things from when multiple crates re-export them, a real-world example I've come across is reqwest's re-exports of http crate types

That's the only argument, why I usually don't like re-exporting and abstain from it as well. It makes things a bit more vague... but in practice this leads only to a problem if different things re-export the same thing - and that would cause the same issue of incompatible ruma types or if things named the same are exported from different places (which we already see just by named types and you can simply solve as a developer by importing with use ... as MyThing...)

One must be aware of that, but that's different from what I am proposing here: In what I am proposing only common would re-export ruma, the others won't (but refer to common to be the place it comes from or re-export pub use matrix_common::ruma only, which might be more convenient for docs) and we'd generally recommend using common::ruma as the entry point when using the lib. In practice any other import of ruma won't be compatible with matrix-sdk anyways, so I don't see the point of making people jump through the hoops of maintaining their own Crates.toml dependency.

with the error messages for that kind of type mismatch being pretty good these days and Cargo has support for explicitly marking dependencies as public or private which should help as well (though there doesn't seem to be any recent movement towards stabilization unfortunately – rust-lang/cargo#6129).

Agreed, it would be nice (though not fixing the problem entirely), if this was merged. But it hasn't moved in over two years so... Not really an argument to wait for that instead.

"ugly" and incomplete solution

This is what the argument boils down at the end though, that you/some consider this ugly on principle - and I agree it doesn't feel right to do that generally. But especially considering there isn't any complete solution (as of now, nor on the horizon) and other the icky-ness there isn't much of any drawbacks (I can see), I'd rather take a practical approach and that is: one way is a lot easier for the users of the API and one is a lot harder. I'd rather take the easier one, even if it feels likely ugly or icky, if that doesn't come with any clear drawbacks. And I don't see that this does.

[gnunicorn]

Btw, not a hill I am willing to die on. I care 4/10 about this, just made it for convenience and am fine to change it back, but don't have seen any good argument why that would be better.

[jplatte]

Just pointing out that below you are naming the popular reqwest-library as an example doing it

Actually, reqwest doesn't do a whole-crate re-export like here but I guess it doesn't matter that much.

Honestly overall it sounds like we're almost on a similar page, as I said I see some value in the existing re-export. I just saw -common as more of an implementation detail and not something users would interact with directly, so if ruma then becomes a pub use matrix_sdk_common::ruma; re-export in the docs rather than just pub use ruma;, that seems like it would just add confusion and not really help users in any way.

Sorry if my previous reply was a bit intense, I really don't want to die on that hill either ;)

[poljar]

This looks fine, though CI seems to disagree. Feel free to merge once CI is happy.