Skip to content
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

[Task] Find a replacement for feature gated trait methods #923

Open
2 tasks
olivereanderson opened this issue Jun 24, 2022 · 1 comment
Open
2 tasks

[Task] Find a replacement for feature gated trait methods #923

olivereanderson opened this issue Jun 24, 2022 · 1 comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.

Comments

@olivereanderson
Copy link
Contributor

olivereanderson commented Jun 24, 2022

Description

Currently the Storage trait contains certain methods (such as for instance data_encrypt) that are feature gated. We should consider finding an alternative way of allowing users to opt in to implementing these methods on their storage backends. See the Motivation section below for why an alternative is desirable.

The Implementation suggestions section discusses a few ways to proceed.

Motivation

Feature gating trait methods is problematic for the following reasons:

  1. It is not additive meaning that it can break compilation for downstream crates. Here is an example: crate foo depends on identity_account_storage without the encryption feature and implements Storage for one of its types. Another crate bar depends on identity_account_storage with the encryption feature. Then if ever bar becomes part of foo's dependency tree, foo will no longer compile.
  2. If a library consumer wants to implement said feature gated trait methods for at least one of its types, then said methods need to be implemented on every type implementing the trait regardless of whether its needed. As an example someone might have an application with different kinds of Accounts some using fancy storage backends and others less so. If the encryption flag is set they are also forced to implement these methods for their more rudimentary storage types.

Implementation suggestions

The alternative that is easiest to implement would be to simply always include these methods on the Storage trait and provide a default implementation that simply returns a "operation not supported" error that the user my overwrite themselves. Although simple this approach is perhaps not the most elegant.

Another way to approach this would be via generics. Roughly speaking we could do something like: introduce a feature gated sub-trait StorageEncryption: Storage which contains encrypt/decrypt_data methods and add an additional generic parameter to the Account. So it would be (something like) Account<C = Arc<Client>, T: Storage> and make the encrypt/decrypt methods available in an impl block :

#[cfg(feature = "encryption")]
impl <C: SharedPtr, T: StorageEncryption> Account<C,T > {
    pub fn encrypt_data; 
    pub fn decrypt_data;
}  

. Note that this is just a rough sketch and doing this properly would require some thought. In my understanding the bindings require dynamic dispatch and this would have to be supported somehow (I am pretty sure it can be done). However even if we manage to implement this approach "correctly" we might still have to force our API to require more type annotations (turbofish) which is not ideal.

Resources

Blocked by #920

To-do list

  • Implement an alternative to feature gating methods in the Storage trait.
  • Investigate whether there are more instances of feature gated methods in traits throughout the code base and open issues to address them.
@olivereanderson olivereanderson added Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Jun 24, 2022
@olivereanderson olivereanderson added this to the v0.7 Features milestone Jun 24, 2022
@olivereanderson olivereanderson self-assigned this Jul 4, 2022
@olivereanderson
Copy link
Contributor Author

olivereanderson commented Jul 4, 2022

I've looked into the generics approach and I could not find a satisfactory solution. If we are to introduce generics for the storage parameter we would also want to be able to use Rc<dyn Storage> as storage in the Account in the JS bindings. As @cycraig pointed out here: #707 we cannot abstract over the pointer kind directly. Despite this we could however implement Storage for Arc<dyn Storage> and Rc<dyn Storage>, but (to the best of my knowledge) this forces us to split the Storage trait into a Send and !Send version. Despite offering better performance in the bindings and generics also opening up for avoiding dynamic dispatch in the Rust library the cost to ergonomics and readability of the docs is in my opinion too high to go the generic route. I hope that this will become more possible in the future once Rust gets a proper solution for async methods in traits, but for now I think we should go with the simpler solution not involving generics.

@eike-hass eike-hass removed this from the v0.7 Features milestone Dec 11, 2023
@eike-hass eike-hass moved this to Product Backlog in IOTA Trust Framework Developments Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
Status: Product Backlog
Development

No branches or pull requests

2 participants