[Request] Allow Account to resolve remote DIDs

[cycraig]

Description

Update the Account to allow it to resolve DIDs from the Tangle that do not correspond to a locally-stored IdentityKey.

Motivation

Currently, Account::resolve_identity() throws Error::IdentityNotFound if called with a DID that does not correspond to an IdentityKey stored locally in the Account (e.g. one created with Account::create_identity()).

This is confusing because the low-level API's Client::resolve() function returns any valid DID from the network, and IdentityNotFound implies that the DID does not exist at all, not just locally.

Options

1. Improve the error message to indicate that the identity was just not found in the account/locally.
   E.g. "Identity not found in account" instead of just "Identity not found".
   
   
2. Rename resolve_identity() to try avoid confusion.
   E.g. resolve_account_identity() or resolve_identity_key()
   
   
3. Update resolve_identity() to take an IotaDID / IdentityTag so any DID may be resolved, instead of an IdentityKey (which may be an IdentityId, a local identifier that does not contain a full DID tag).
   
   
4. Leave resolve_identity() as-is (or rename it) and add a new function, e.g. resolve_remote_identity(), which can resolve any DID.
   
   
5. Add a function, e.g. get_client_map(), to expose a reference to the ClientMap in State::clients from the Account. This would allow users to call the low-level API's resolve() manually.
   
   

Overall a combination of the above options may be preferable. Personally I prefer (1) improve error message (in general) and (3) update resolve_identity() signature, since it doesn't seem to be used internally with any IdentityIds. If the Account is only intended to resolve identities managed locally, then the function should be renamed (2).

I am in favour of exposing the ClientMap to avoid the anti-pattern of just calling the low-level API indirectly through the Account using wrapper functions that do nothing else (not that we do this currently). Other than resolve(), there are one or two convenient functions, such as resolve_history(), that are currently inaccessible through the Account.
However, we may want to discourage misuse of the low-level API on DID documents managed by the Account which could result in an inconsistent/corrupt state.

Are you planning to do it yourself in a pull request?

Yes, pending discussions.

[PhilippGackstatter]

With the lock/thread-safety discussion (in #377) a related issue came up. If we didn't use an incrementing integer to identify identities locally in the account (i.e. the IdentityId), but DIDs instead, we could increase the concurrency of the account (@JelleMillenaar's idea). We wouldn't have to lock the index when creating/deleting an identity, and could use a concurrent HashMap (e.g. DashMap) to keep track of the identifiers. More importantly, though, this also makes the account a lot easier to understand, as there could be much fewer types that identify an identity, i.e. Identity{Id, Key, Tag}. That reduces the complexity both for the user and for the developers working on it.

I would suggest removing IdentityKey, IdentityTag and IdentityId. Identities are naturally identified by their did and that would nicely fit with option 3 of your proposal. If we really want to keep referring to identities by name, then I would still remove the IdentityTag from the public interface (currently returned by Account::list_identities), and use strings instead (already allowed wherever an IdentityKey is required). In that case I would also go with option 1 to give a better error if an identity-by-name wasn't found.

I would not add a new function that also resolves identities (option 2) to avoid increasing the complexity. Similarly, I also wouldn't expose a part of the low-level API in the high-level API (option 4) for the same reason.

[cycraig]

I'm not aware of the design decisions behind Identity{Id, Key, Tag}: they appear to be performance optimisations to avoid string repetition (IdentityTag omits the "iota:did:" part of a DID) or requiring string matching/hashing everywhere by using the more lightweight local IdentityId (u32 instead of string). We can probably look into string interning or judicious use of Arc/Cow to prevent duplicates if it becomes an issue.

I agree with the suggestion to remove IdentityKey, IdentityTag, and IdentityId. I believe there will need to be a replacement struct e.g. Identity/IdentityDID, because an IotaDID may contain a fragment, query, path etc. while we need to store only the tag.

Would the IdentityIndex fall away completely or just contain a set, or the concurrent hashmap of locks?

[PhilippGackstatter]

We can actually keep the IdentityIndex together with IdentityId, but the latter only for the implementation, not as part of the public interface. The IdentityId is extensively used in the Storage trait, so it would be good not to change that.

String interning would probably not help in that case, since the array holding the strings would have to be shared among the account and storage implementation, which seems hard to do, unless I'm missing your point?

If we want to keep the naming feature too, then we basically also need to keep the IdentityTag, but only internally. The more I think about it, the more I want to keep the names. It seems quite useful for developers to specify a name and be able to list named identities, which can be directly displayed to users in an application. Towards that end we could add Account::list_named_identities that returns a Vec<String> or similar, and let Account::list_identities return a Vec<IotaDID>.

My main point is just to reduce complexity for the users of our library by removing tag and id from the public interface. If we keep naming, then we also need to keep the IdentityKey in the interface, I guess. Sorry for the rollercoaster of opinions.

[cycraig]

Keeping IdentityId and IdentityKey means the change doesn't really impact this issue then, I imagine the solution would be (3) and implement either From<IotaDID> for IdentityTag or AsRef<IdentityTag> or vice-versa to wrangle the resolve_history parameter to accept an IotaDID (without the fragment, path, or query parts) or IdentityTag but exclude IdentityKey/IdentityId since those seem more like internal constructs.

It was just a suggestion as a potential optimisation to prevent copies, I don't know what technical roadblocks we may encounter and I don't intend to start off with that. I was just under the assumption we were replacing IdentityId with an IdentityDID (or whatever) which would require multiple copies of strings everywhere, e.g. in the lock map HashMap<IdentityDID, RwLock<IdentityDID>> and the index etc.

Naming the identities seems out-of-scope from the Account in my opinion. It is easy enough for an application to maintain a map of names to DIDs; although the idea may have been to store the names in a secure Stronghold storage to prevent leaking which DIDs are stored? I was thinking about having IdentityTag and then something like a NamedIdentityTag if we retain naming identities in the Account, for clarity if nothing else.

[PhilippGackstatter]

Without names, the approach would be to let all methods that currently take an IdentityKey take an IotaDID directly instead, right? Because we don't want to introduce another type in the public interface.

Inside these methods, we can get low-cost access to the tag as a &str through IotaDID::tag. Why not use that as the key into the IdentityIndex? The tag only needs to be cloned when inserting into the HashMap. We might even be able to get rid of the index after all and replace IdentityId with &str.

Without the index and thus without the IdentityId we might need a slightly different approach to locking. At that point, it no longer makes sense to duplicate the tag in the lock itself, but maybe use HashMap<String, Arc<RwLock<()>>> instead. Not really best practice, though. At that point, I can also see us using the IdentityDID type, though not sure what that would look like. Wouldn't we have to convert/map the IotaDID into this new type on every access, so we also need frequent matching?

[cycraig]

Without names, the approach would be to let all methods that currently take an IdentityKey take an IotaDID directly instead, right? Because we don't want to introduce another type in the public interface.

Not really, because an IotaDID can have other parts at the end such as a fragment, path, or query. We may need a new type to make it clear those functions are only interested in the identity itself, like IdentityTag does or some new IdentityDID.

Replacing with a bare &str is not great, we need a wrapping struct (e.g. IdentityDID) to enforce constraints in my opinion, similar to why NetworkName was introduced. I thought we couldn't get rid of IdentityId due to its usage in the storage? I'm a bit lost.

Wouldn't we have to convert/map the IotaDID into this new type on every access, so we also need frequent matching?

Yes but I'm not convinced that's terrible performance-wise, IdentityKey already does string matching on the tag in several places. I also don't want to get trapped by premature optimisation. We could implement a shared interface for IotaDID and IdentityDID (similar to IdentityKey) to prevent conversions from being necessary, or maybe implement AsRef<IdentityDID> for IotaDID or something.

[PhilippGackstatter]

an IotaDID can have other parts at the end such as a fragment, path, or query.

Sure, but then the current implementation would also be broken/ambiguous.

let did: IotaDID = "did:iota:test:BjCGvYHiv3JDVRqdQyzavjC1QTFB46LVSVBeoUfrZt6h?query=xyz#fragment".parse().unwrap();
account.update_identity(did, ...).await?;

This operates only on the did.method_id() which would be test:BjCGvYHiv3JDVRqdQyzavjC1QTFB46LVSVBeoUfrZt6h here, ignoring the other segments. That's your criticism, if I'm understanding correctly?

There is a lot of that ambiguity in the interface, like in Account::sign, which takes any IdentityKey and a fragment. That's a bit awkward if you call it with an IotaDID like above, and having to pass the fragment again. Or in the low-level API:
IotaVerificationMethod::from_did which creates a method from a did and an explicit fragment. I'm not saying that's great, but that seems to have been the choice thus far, and for consistency we should update all of those APIs, if any.

If we continue to have a shared interface, then this wouldn't become clearer, since you can still call update_identity with an IotaDID that has segments. If we want to be clearer, we should go the AsRef route so the "conversion" becomes explicit. Still I'm not convinced that the clarity we gain by making this an explicit type, is worth having that additional type in the interface.

I thought we couldn't get rid of IdentityId due to its usage in the storage? I'm a bit lost.

Sorry for that, I was just exploring different options. It's not that the IdentityId is required, it would just be more work to change it, which I was initially opposed to. It's not a dealbreaker to update that, though.

[cycraig]

This operates only on the did.method_id() which would be test:BjCGvYHiv3JDVRqdQyzavjC1QTFB46LVSVBeoUfrZt6h here, ignoring the other segments. That's your criticism, if I'm understanding correctly?

Yes, using a full IotaDID seems ambiguous in a lot of cases. So having IdentityDID or something which only holds the method_id() seems preferable in my opinion.

I agree we should have a consistent interface but this is may be overlapping with other Account refactors a bit too much at this point.

To summarise, I want to:

1. introduce an IdentityDID struct that holds only the tag/method_id of an IotaDID, similar to IdentityTag.
   E.g. it only holds "did:iota:test:BjCGvYHiv3JDVRqdQyzavjC1QTFB46LVSVBeoUfrZt6h" or "test:BjCGvYHiv3JDVRqdQyzavjC1QTFB46LVSVBeoUfrZt6h".
   
   
2. replace IdentityKey with IdentityDID, and use AsRef<IdentityDID> so &IotaDID also works.
   Edit: this would address the resolve_history problem which is the original issue.
   
   
3. remove IdentityId and the index if possible. We may need to retain the index for fast lookups of which identities are managed by the Account.
   
   
4. remove named identities as a feature in the Account. If we want it I would prefer something like the index which stores name mappings explicitly rather than shoe-horning them into keys. The original intention may have been to "simplify" the interface by allowing one to use the full DID, name, or local u32 id as parameters, which has actually resulted in a more complex and confusing interface.

Not all of these objectives may be possible, but it seems to be what we're converging on, yes?

[PhilippGackstatter]

Regarding the first point, I would perhaps discuss adding IdentityDID to the public interface internally a bit more before making that change. In the interest of explicitness, I agree with the general idea, though. The name introduces some ambiguity with IotaDID, so something including tag might be good, perhaps even reuse IdentityTag or use IotaDIDTag - just ideas.

The other points I fully agree with.

[cycraig]

Looks like this might be made redundant by PR #436 since resolve_identity will only retrieve the DID document managed by the Account?

[PhilippGackstatter]

I think so. Although even in #436 the resolve function is still part of the account, to let tests pass. Once the new resolver API lands, that method can be removed and this issue will no longer be one.

[JelleMillenaar]

Issue has become a non-issue as this feature is going to be removed entirely from the Account in a near-future update.