[Task] Improve Account API by internalizing commands

[JelleMillenaar]

Description

Small refactor on the account API in order to make updating DID Documents from a two step into a one step process. Currently the Account API allows developers to generate update commands that they have to manage and pass on into the update_identity function. These commands can be stored inside the account and can be either published immediately or queued to be published in a single call of update_identity. If more commands are queued they are published in the order they are added. Later, we can add additional optimization code to combine multiple commands into a single DID update.

Motivation

Reduce the complexity of the account even further.

Resources

Example

To-do list

Create a task-specific to-do list . Please link PRs that match the To-do list item behind the item after it has been submitted.

• Internalize Commands
• Update update_identity function to no longer take a command parameter.
• Update all account examples
• Rename document in account examples to DID
• Enable queuing multiple updates before publishing - PR Enable lazy tangle updates in the Account #377.
• Add the ability to create an identity with an existing keypair to account - PR Enable the creation of an identity from a KeyPair in the Account #352

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

• The feature or fix is implemented in Rust and across all bindings whereas possible.
• The feature or fix has sufficient testing coverage
• All tests and examples build and run locally as expected
• Every piece of code has been document according to the documentation guidelines.
• If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
• If the feature is not currently documented, a documentation task Issue has been opened to address this.

[JelleMillenaar]

Added 'Add the ability to create an identity with an existing keypair to account' to the task as it is a minor account API change.

[l1h3r]

@JelleMillenaar what is the intended benefit of reusing an existing keypair here? is this just for convenience or space-saving?

Stronghold doesn't support copying keys (maybe this could be added) and sharing keys across multiple identities would require a fair amount of internal changes. If you wanted to create a "child" identity owned by a "parent" identity wouldn't the controller field be a better fit once we have proper support for it?

[PhilippGackstatter]

In regards to the simplification & optimization of identity updates, I think we have (at least) these three variants to consider.

Aside: I originally thought that we might want to reuse the Command in a future actor handler, but that is not the case. The account types that are re-used in the actor are those in the identity sub module of the account, such as IdentityCreate. Thus we don't have to consider that use case in the following discussion.

Variant 1 (current approach)

 let command = Command::create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .finish()?;

  // Process the command and update the identity state on the tangle.
  account.update_identity(document, command).await?;

Variant 2

let command = Command::create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .finish()?;

let other_command = /* omitted */

// Stores the created command in an internal queue in the account.
account.queue_command(command);
// We can queue multiple commands before applying them.
account.queue_command(other_command);

// Applies all pending commands to the document and updates the tangle with the latest document.
account.update_identity(document).await?;

Variant 3

 account.new_command(document)
    .create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .apply() // <-- Updates the tangle immediately.
    .await?;

 account.new_command(document)
    .create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .queue(); // <-- Queues the command for later.

account.apply_pending_commands(document).await?;

Variant 2 updates the tangle exactly once, even with multiple commands present. It's possible to build up a queue of commands in the account, and then call update_identity once, to apply all queued changes. The major upside of this variant is, that it might be possible to implement optimizations here. We could potentially apply all changes to the latest document locally, and only publish the result once. If multiple commands are typically applied at the same time, then this variant has potential benefits, saving network calls and storage space on (perma)nodes. Given the current set of commands, it seems that creating multiple services or methods at once is a realistic scenario.

identity.rs/identity-account/src/events/command.rs

Lines 28 to 59 in 5a35860

	#[derive(Clone, Debug, PartialEq)]
	pub enum Command {
	CreateIdentity {
	network: Option<String>,
	authentication: MethodType,
	},
	CreateMethod {
	scope: MethodScope,
	type_: MethodType,
	fragment: String,
	},
	DeleteMethod {
	fragment: String,
	},
	AttachMethod {
	fragment: String,
	scopes: Vec<MethodScope>,
	},
	DetachMethod {
	fragment: String,
	scopes: Vec<MethodScope>,
	},
	CreateService {
	fragment: String,
	type_: String,
	endpoint: Url,
	properties: Option<Object>,
	},
	DeleteService {
	fragment: String,
	},
	}

This variant's API is a bit less intuitive and more verbose to use than the others. First-time users might not realize they have to call update_identity for changes to be applied, although good documentation can surely help. It also actually needs three invocations rather than the current two, so it's more verbose in that regard. I would only go with this variant if there is a strong case to be made for the optimizations.

An actual "one-line" update would look something like variant 3. Either apply the command immediately with apply or queue it for later application with queue. A potential problem with this variant is that the new_command method will produce a Command which will have to hold a reference to the Account and IotaDocument. This is because contrary to the other variants, apply is called on the Command and therefore it needs to have a reference to the objects it works with. Thus, we introduce a tight coupling between Command and Account/IotaDocument. Given that Command and Account are only used together anyway, and no future usage outside the account is foreseeable, this seems okay.

The current variant 1 is somewhere in between, requiring two method invocations to update an identity, but offers no way to lazily apply multiple commands.

Any thoughts on this are very welcome - perhaps I've missed a variant that combines the strengths of some of these. Other than that, what are your preferences?

[cycraig]

Variant 3 seems feasible.

A potential problem with this variant is that the new_command method will produce a Command which will have to hold a reference to the Account and IotaDocument.

Small correction: the current update_identity function in the Account takes in a generic IdentityKey, which is essentially an IotaDID identifying a document, not an entire IotaDocument. As an aside, this relates to the "Rename document in account examples to DID" task.
The references to an Account and IdentityKey may not need to be stored in the Command to implement Variant 3. Since each Command implements its own builder pattern, could the references temporarily live in the builder struct instead? If so, that would leave the Command as-is and avoid any tight coupling, so we could potentially retain some version of Variant 1 if that has any benefits?

Implementing a queue can be considered separately as it seems possible with all three of the outlined variants. If I understand correctly, a queue would would need to store both an IdentityKey and the Command? Could it instead store the Events that each Command generates internally before they are processed by the Account? The latter approach has the benefit of catching validation errors early (before lazy evaluation) but I'm unsure if it's possible since there are checks, e.g. to ensure no duplicate key fragments are added, which depend on the current state of the document and hence previous updates.

Nit: I prefer the verb enqueue() to reserve queue() for returning the pending Commands / Events perhaps?

[eike-hass]

@PhilippGackstatter: In Variant 2 we don't need define for which document we want to queue commands, but for applying we do? I see the elegance of Variant 3, but it has same problem of potentially forgetting to actually commiting the updates.
I think I am leaning towards Variant 2, especially if we could save a reference of the correct acount and just call account.update_identity() to commit the changes. Would it also be possible to pass and arrray or list to quene_command?

[cycraig]

@eike-hass Part of the issue is adding the ability to queue commands (to combine multiple updates and reduce network chattiness), which is essentially the only thing Variant 2 introduces.

The other part is "internalising" commands in the Account to make it somewhat easier to apply updates, which is what Variant 3 does and Variant 2 does not address. If that's not as important we can go with Variant 2 since the queue system seems like more of a priority and leave the refactor for another time?

I see the elegance of Variant 3, but it has same problem of potentially forgetting to actually commiting the updates.

That could be addressed with a similar "apply-on-drop" system which is currently used in the Account, where pending updates are committed if the Account is dropped (goes out of scope). The problem with that is any errors are unrecoverable at that point and there are issues with calling async functions on drop (has to block, panics are a problem etc.).

Another comment on Variant 2 is that the queue_command could take a reference to the DID if you're performing multiple updates on different documents? It would also prevent "forgetting" about a previously-queued command and then applying it on the wrong document. I'm not sure what to do with commands like CreateIdentity which don't make sense to be queued.

// Stores the created command in an internal queue in the account.
account.queue_update(command, did);
// We can queue multiple commands before applying them.
account.queue_update(other_command, did);

// Applies all pending commands to the document/s and updates the tangle with the latest document/s.
account.apply_pending_updates().await?;

[PhilippGackstatter]

Small correction: the current update_identity function in the Account takes in a generic IdentityKey, which is essentially an IotaDID

The references to an Account and IdentityKey may not need to be stored in the Command to implement Variant 3.

Good points!

Implementing a queue can be considered separately as it seems possible with all three of the outlined variants.

Not sure how that would be possible with variant 1, unless we add a second method to enqueue items, in which case we have basically variant 2, or using some apply-on-drop mechanism, which has too many problems currently, due to the involved asynchronicity, imho. Generally though, I agree it can be added later, especially with variant 3, where we can do the change to the "internalisation" of the commands first, and add a queue method later.

I prefer the verb enqueue()

Agreed 🙂

I think I am leaning towards Variant 2, especially if we could save a reference of the correct acount

I think with Craig's suggestion, this point no longer stands. It will be stored in the CommandBuilder instead, and the Command will continue to be decoupled.

Would it also be possible to pass and arrray or list to queue_command?

I'm a bit hesitant to add this, unless there is a strong use case where we have many commands to apply at once. In Rust especially, I would rather do a commands.for_each(|command| account.enqueue_command(did, command));. For JS the story might be different though, so we could potentially add it in the Wasm bindings.

Another comment on Variant 2 is that the queue_command could take a reference to the DID if you're performing multiple updates on different documents?

For variant 2, I think that makes sense 👍

CreateIdentity doesn't make sense to be queued.

Why not? Can we not create an identity, then add a method and a service, and only then publish all of it as one document?
As a more efficient alternative to account.create_identity and subsequent updates, that would be nice to have.

Variant 3 [...] has same problem of potentially forgetting to actually commiting the updates.

I think that's the biggest remaining issue with variant 2 and 3. I'm not a fan of the apply-on-drop, as it hides too much from the user and is difficult to implement. Another way we could batch multiple updates, without the problem to forget applying them, would be by building on @eike-hass's array suggestions and combine this with variant 3:

// A single update in one line
account.new_command(did)
  .create_method()
  .scope(MethodScope::Authentication)
  .fragment("my-auth-key")
  .apply() // <-- Updates the tangle immediately.
  .await?;

let command = account.new_command(did).create_method().build();
let other_command = account.new_command(did).create_service().build();

account.apply_batch_updates(&[command, other_command]).await?;

in which case we have both the one line update, and the batch update without possibly forgetting it.

[cycraig]

Implementing a queue can be considered separately as it seems possible with all three of the outlined variants.

Not sure how that would be possible with variant 1, unless we add a second method to enqueue items, in which case we have basically variant 2 [...]

You're right.

CreateIdentity doesn't make sense to be queued.

Why not? Can we not create an identity, then add a method and a service, and only then publish all of it as one document?
As a more efficient alternative to account.create_identity and subsequent updates, that would be nice to have.

Because I think the only way to get an IotaDID of a newly created identity through the Account is to publish it (unless you generate the initial keypair beforehand and can derive the IotaDID from that). Without that we have no IdentityKey to reference when creating/apply any of the update commands, unless I'm mistaken? I agree it would be convenient to have, I just don't see how it would work with any of the suggested approaches since the caller usually needs the result of the create operation returned?

[...] Another way we could batch multiple updates, without the problem to forget applying them, would be by building on @eike-hass's array suggestions and combine this with variant 3: [...] in which case we have both the one line update, and the batch update without possibly forgetting it.

We can call this above approach "Variant 4" for future reference. It neither fully internalizes the commands nor maintains an internal queue in the Account, and so pushes the responsibility to the caller to "queue" commands their side. I somewhat prefer this just because it avoids the overhead and complexity of maintaining a queue internally, and we can still optimise/"squash" multiple updates to a DID.

What if we have some sort of compound command that can be chained instead of queued? E.g.

account.new_command(did)
  .create_method() 
  .chain() // <-- Creates some sort of "compound" command struct/queue to hold multiple commands
  .new_command(did)
  .create_service()
  .chain()
  .apply() // <-- Executes all commands in the chain
  .await?; 

I'm a bit iffy on whether the above syntax is possible, so it will probably have to be something more like:

account.new_command(did)
  .create_method() 
  .chain(||
    account.new_command(did)
    .create_service()
    .build()
  ).apply() // <-- Executes all commands in the chain
  .await?; 

Just exploring other possible styles that avoid an internal queue in the Account.

[PhilippGackstatter]

// Stores the created command in an internal queue in the account.
account.queue_update(command, did);

We can btw. also rename the Command concept to Update, if you prefer that. I think @JelleMillenaar is also in favor of that. The only slight "conceptual clash" we have, is that creating an identity is part of the possible updates. But for that we have a dedicated method anyway (Account::create_identity) and for the rest it makes more sense.

Without that we have no IdentityKey to reference when creating/apply any of the update commands, unless I'm mistaken?

It looks like to call Account::process we only need an IdentityId which is an id of the account's internal identification system. When a new identity is created, the next IdentityId is generated, and is used to process the Command. So it might be possible to use that id to do multiple updates to a fresh identity, even before the did is generated. I'll investigate once I implement this.

What if we have some sort of compound command that can be chained instead of queued? E.g.

The first of your proposals might be possible. I would only implement this if that is possible - the second seems like too much API complexity given the original goal. I imagine it would also be a challenge to port this to Wasm. The first proposal still seems like a mouthful, too, but fulfills the "batch update in one statement" goal.

I think I like the array variant a bit better. For singular updates it has a one-statement solution, but for multiple updates, I find it acceptable to have multiple statements. It then also keeps the CommandBuilder's complexity fairly low.

Curious to hear what others think, though.

[eike-hass]

I feel the apply_on_drop logic should not be a goal. I think it would be best if we could throw or at least log when we detect uncommited changes. In regards to the queue: I think an internal queue is useful if the user wants do add fields in a loop or other control structure. To avoid the internal queue another form of batching should be possible like a function that accepts multiple "transactions".

[PhilippGackstatter]

With the queue out of the way (implemented in #377), I'd like to revive the discussion with a focus on the API, so we can hopefully settle on a design.

  account
    .update_identity(did)
    .create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .apply()
    .await?;

This would be somewhat verbose, but clear in what it does. "Update the identity did by creating a method with this scope and fragment, and apply it". If we care a lot about verbosity, one line can be removed if we supply the did that we want to update in create_method:

  account
    .create_method(did)
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .apply()
    .await?;

Does this confuse first-time users? We're not creating a method on the account, yet that's one way to read it. The former is perhaps clearer here: we create an intermediary object (the return value of update_identity) that somehow represents the did, that we then create the method on.

Does either of these seem like a good choice, or can anyone come up with something concise and clear?

I also still like the current approach for its clarity, which only gets verbose/repetitive if multiple updates are applied subsequently, due to having to call update_identity after every update. I think that's acceptable, though. Just to show that again: achieving the same with the current approach would be:

 let command: Command = Command::create_method()
    .scope(MethodScope::Authentication)
    .fragment("my-auth-key")
    .finish()?;

account.update_identity(document, command).await?;

[PhilippGackstatter]

Prior to this discussion even, we could see if we still want to keep the builder pattern for the commands. To give the full picture, take CreateMethod as an example. It's definition is:

CreateMethod {
    scope: MethodScope,
    type_: MethodType,
    fragment: String,
  }

where type and scope have sane defaults, but the fragment is required. The builder pattern doesn't enforce this, though:

Command::create_method()
  .scope(MethodScope::Authentication)
  .finish()?;

would throw a runtime error, because the fragment isn't specified. However, this could be prevented at compile-time, by making the required parameters (fragment) part of Command::create_method.

This plays into the internalization debate, since option 2 would cobble together the did with the required parameters, which wouldn't be great. In that case, option 1 would be cleaner:

account
    .update_identity(did)
    .create_method("my-auth-key")
    .scope(MethodScope::Authentication)
    .apply()
    .await?;

We can also supply the did in apply. It feels a bit backwards to me. First we create the method, then we apply it to the did - should be the other way around. Still, it's very concise.

account
    .create_method("my-auth-key")
    .scope(MethodScope::Authentication)
    .apply(did)
    .await?;

[eike-hass]

I would prefer the verbose approach of starting the chain with update_identity(did). Are there other commands then create_method that would branch from there?

[PhilippGackstatter]

Are there other commands then create_method that would branch from there?

Yes, all of these commands would have a corresponding method.

identity.rs/identity-account/src/events/command.rs

Lines 32 to 64 in bb6bfa4

	pub enum Command {
	CreateIdentity {
	network: Option<String>,
	method_secret: Option<MethodSecret>,
	authentication: MethodType,
	},
	CreateMethod {
	scope: MethodScope,
	type_: MethodType,
	fragment: String,
	method_secret: Option<MethodSecret>,
	},
	DeleteMethod {
	fragment: String,
	},
	AttachMethod {
	fragment: String,
	scopes: Vec<MethodScope>,
	},
	DetachMethod {
	fragment: String,
	scopes: Vec<MethodScope>,
	},
	CreateService {
	fragment: String,
	type_: String,
	endpoint: Url,
	properties: Option<Object>,
	},
	DeleteService {
	fragment: String,
	},
	}

[eike-hass]

Then I am strongly in favor of update_identity(did).

[PhilippGackstatter]

Per the discussion in standup today, we have reached consensus on:

• Implement option 1, i.e. the account.update_identity(did) style.
• The builder pattern remains for Rust, constructor pattern would become too verbose for create_service, as an example. There are not enough "sane defaults" for a service, unless we start randomly generating fragments.
• Constructor pattern remains an option for bindings, in particular JS where default parameters are an option.
• Method names might need a review as the account has a lot of *_identity method names.