Remove AccountStorage

[cycraig]

Description of change

Removes AccountStorage to simplify passing of Storage interfaces to AccountBuilder.

Note that there are now two storage setters on AccountBuilder: storage, which takes any S: Storage implementation, and storage_shared, which takes Arc<dyn Storage>. The reason for this is that I could not find a way to allow a generic parameter to take either Arc<dyn Storage> or impl Storage. The closest I got was the following:

pub fn storage<A: Into<Arc<S>>, S: 'static + Storage>(mut self, value: A)

Unfortunately the above function does not work when passed an Arc<storage>:

error[E0282]: type annotations needed
  --> examples\account/manipulate_did.rs:34:6
   |
34 |     .storage(Arc::new(stronghold))
   |      ^^^^^^^ cannot infer type for type parameter `S` declared on the associated function `storage`

and the simpler pub fn storage<A: Into<Arc<dyn Storage>>>(mut self, value: A) does not work for non-Arc values.

We could force users to always pass in an Arc<dyn Storage> as we did with the AccountStorage::Custom(Arc<dyn Storage>) variant previously, but I felt this was easier to work with in general.

Note also that I did not change the Stronghold constructor nor add a StrongholdOptions struct mentioned in #552. There are upcoming changes to the Stronghold interface anyway where this change may make more sense, and until there are more options than dropsave it may be fine for now.

Added

• Add AccountBuilder::storage_shared to take Arc<dyn Storage>.

Changed

• Change AccountBuilder::storage to take any Storage interface.

Removed

• Remove AccountStorage

Links to any relevant issues

Fixes #552.

Type of change

• Bug fix (a non-breaking change which fixes an issue)
• Enhancement (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Fix

How the change has been tested

Tests and examples pass locally.

Change checklist

• I have followed the contribution guidelines for this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[PhilippGackstatter]

I suppose we could make storage a required field to prevent accidentally using the MemStore. Perhaps by throwing an error at build time, similar to the IdentityUpdater builders. The build steps of the builder already return Result so that wouldn't be too bad.

Preventing it statically would mean taking it in the AccountBuilder constructor, but then we'd need to add two constructors for impl Storage and Arc<dyn Storage>. That's nicer than throwing a runtime error, but is less conventional for builders, I suppose. Then again, the AccountBuilder is already unconventional as builders go, since it is not consumed when creating an Account from it.

[cycraig]

Still undecided whether we should make storage mandatory at this point. Requiring passing MemStore also means exposing it to the Wasm bindings explicitly or using the custom Typescript MemStore implementation, which I believe still needs some changes in #719.

We have mandatory fields in quite a few builders that cause runtime errors if omitted, so I don't think it would be necessary to add storage as a constructor parameter to AccountBuilder if we choose to make it mandatory.

... unconventional as builders go, since it is not consumed when creating an Account from it.

I think non-consumption is considered an advantage to be fair.

[PhilippGackstatter]

The Wasm bindings are a good point. In light of that, I'm happy with leaving it as-is currently, so we don't have to explicitly expose the MemStore now.

[PhilippGackstatter]

I think we could already make the password required for the next release. This is optional here because there used to be set_password through which it could be set after instantiation. But from looking at the new stronghold interface, a Vec<u8> will be required to instantiate the KeyProvider, and I don't think we want to map None to an empty Vec. It doesn't really make sense to use stronghold without a password, so we might as well enforce it and give direct control to users. For testing, one can always pass an empty or random string.

Whether to take a String or Vec<u8> I'm not sure about, but I think a string might be more convenient?

[cycraig]

Ah, thanks for the information. I've changed password to be required.

I think String may be more convenient for now? Alternatively I would suggest &[u8] but we cannot zeroize the parameter ourselves then, and String appears slightly better than Vec<u8> in my opinion.

[PhilippGackstatter]

Sorry for the delay, looks good to me!