Open robin-nitrokey opened 7 months ago
Looks good.
For the re-export of third-party crates I would go to the extent that they should generally be avoided. I don't really see the benefit of re-exporting the rand
traits for example.
I think re-exports do have some benefits. Especially for types like heapless_bytes::Bytes
and littlefs2::Path
that are fundamental to Trussed, it would be less convenient to always have to add heapless-bytes and littlefs2 dependencies to all crates using Trussed (and always having to look up the compatible version). I agree that it is less important for the rand
traits as they are only part of Platform
.
it would be less convenient to always have to add heapless-bytes and littlefs2 dependencies to all crates using Trussed (and always having to look up the compatible version)
We already have to do that for littlefs2 and many other crates because we're using git dependencies.
You're right that for heapless-bytes and other dependencies where we don't use patch releases that much it's a benefit.
I still hope for a future where we don’t need the Git dependencies … or at least only in the runner, not in every crate using Trussed. Actually, it would make sense to move Path
/PathBuf
from littlefs2 into a separate crate. Maybe something like heapless-path
? That would also mean that patches to the littlefs2 implementation would not affect the Trussed API directly.
When working on splitting Trussed into a client API and a backend implementation, I noticed the following:
Currently, many types that are declared in Trussed are re-exported in multiple locations. For example, the
Platform
trait:platform::Platform
Platform
(top-level)types::Platform
External types are also re-exported in multiple locations:
platform
re-exportsrand_core::{CryptoRng, RngCore}
types
re-exports, among others, many heapless and littlefs2 typesAt the same time, we have many modules that only contain one or two public types. For example,
error
only containsError
andResult
.store::certstore
only containsCertstore
andClientCertstore
. While it makes sense to group the implementation into a module, it does not make sense to usetrussed::error::Error
,trussed::store::certstore::Certstore
ortrussed::platform::Platform
to refer to these types.I propose the following guidelines for cleaning up imports and modules:
The goal of these changes would be to improve readability of the Trussed code and of the code using Trussed, to make it easier to import the correct types and to avoid surprising or unexpected imports.
I’m not sure how to deal with re-exports of external types. One option would be to only re-export them in a single location,
types
, or to have a re-export of the entire crate on the top-level and not re-export individual types.What do you think?