[DONE] Should Account Handle a Single Identity?

[HenriqueNogara]

Currently, the Account API allows for multiple identities. In order to allow this, Account uses a HashMap to match each identity to their tags. See the structures below:

#[derive(Debug)]
pub struct Account {
  config: Arc<Config>,
  state: State,
  store: Box<dyn Storage>,
  index: RwLock<IdentityIndex>,
}

// An mapping between [IdentityTag]s and [IdentityId]s.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[serde(transparent)]
pub struct IdentityIndex {
  data: HashMap<IdentityTag, IdentityId>,
  #[serde(skip)]
  locks: HashMap<IdentityId, IdentityLock>,
}

Although this might be convenient for certain use cases, this may also introduce unwanted complexity. Common operations would be more convenient when calling without a parameter. See the example below:

// Add a new Ed25519 (default) verification method to the identity - the
// verification method is included as an embedded authentication method.
account
    .update_identity(did)
    .create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .apply()
    .await?;

If Account were to handle a single identity update_identity, delete_identy, and resolve_identity could be called without passing a parameter, we could also get rid of methods such as list_identities and find_identity. Other methods could also be simplified as well.

For this change to be effective we should also make it easier to create new accounts. One idea would be to create a AccountConfig struct that would be configured with a Storage, DidMethod, and a ClientMap and that could be reused to create as many accounts as needed.

[PhilippGackstatter]

Generally, I like the direction. It certainly simplifies the API of the Account. However, it also pushes some responsibility out of the Account. One question that arises is how an account is created. With this new approach, the account is a manager for a single identity. Thus, an account should only ever represent a valid identity (with the exception of the state an account is in after account.delete_identity has been called).

I think we want to continue to have one stronghold, or more generally a Storage, that stores multiple identities. We probably don't want one stronghold for one identity. Two obvious APIs would then be:

Factory pattern

Create an AccountFactory from a Storage and an AccountConfig, so:

let account_config = /* ... */
let account_storage = AccountStorage::Stronghold(snapshot, Some(password));
let account_factory = AccountFactory::new(account_config, account_storage);

and then instantiate account instances from the factory. The account instances inherit the storage and config from the factory. But how do we specify which identity an account manages?

Creation

Since I defined that an account should only represent a valid identity, the creation should happen on the factory, and return an Account. For example:

let account = account_factory.create_identity(IdentityCreate::default());

Update

We want to update an identity that exists and that we have the keys for in our local stronghold. Now we need to somehow create an account that associates the new instance with the right keys in the storage. Something like

let account = account_factory.load_account(did)

This could be called loading, instantiating or anything else, but the point is we need some way to create an account that manages an existing identity.

No-Factory approach

It's less obvious to me how this would work nicely without a factory. We want to instantiate new accounts directly from an AccountConfig and Storage, and at the same time create an identity or load one.

let account = Account::create_identity(account_config, account_storage, IdentityCreate::default());
let account2 = Account::load_identity(account_config, account_storage, did);

Seems like a mouthful to me, so the factory approach seems cleaner here. Was the thinking along the lines of such a factory or has the discussion gone in a different direction so far?

Edit: At this point we could even rename the Account to Identity 😁.

[eike-hass]

I can't speak on how idiomatic the first approach is, but it feels weird to me, to reuse the builder. The second approach looks clean to me, although I don't understand where account_config comes from

[PhilippGackstatter]

I've linked the builder pattern explanation in Rust, and while I think it's not an "official source", they do say:

// If we can get away with not consuming the Builder here, that is an
// advantage. It means we can use the FooBuilder as a template for constructing
// many Foos.

So I think it's okay, perhaps even idiomatic.

[olivereanderson]

If the AccountConfig mentioned under "Good old constructors" does not have any other purpose than to be supplied as an argument to the constructor of Account then it is de facto a builder, in which case the AccountBuilder as described under "The builder pattern" mentioned above conveys the reason for its existence in a more clear way.

Using constructors would perhaps be more appealing if methods/functions with default arguments were possible in Rust, but this is not the case, at least not at this time.

I do understand and appreciate the concern of @eike-hass regarding the reuse of the builder. This is perhaps not how the builder pattern was originally intended or how it has been in use in other programming languages. What we have described above is perhaps more like a combination of the Builder pattern and the Prototype pattern, but it would be hard for us to come up with a good name for that combo, so it is perhaps better to go with the source @PhilippGackstatter linked above and just call this approach a flavour of the Builder pattern in Rust.

[HenriqueNogara]

I really don't think reusing the builder is a problem, I think it is actually preferred (or at least it used to be). https://doc.rust-lang.org/1.0.0/style/ownership/builders.html#non-consuming-builders-(preferred): I would go for the builder, personally.

[PhilippGackstatter]

Agreed with the points above, especially that the AccountConfig is a de facto builder. I want to add that these approaches are not mutually exclusive, but can actually nicely live alongside each other. We can set it up in a way such that AccountBuilder::create_identity calls Account::create_identity, at which point the only question is whether the latter should be public or not. It's probably still a good idea to settle on one approach - currently the builder seems favored - build that out, and once we had time to test drive the approach we might also want to make the methods on the Account public.