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

Replace ClientMap with Client in Account #582

Merged
merged 11 commits into from
Jan 13, 2022
Merged

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Jan 12, 2022

Description of change

Refactors the Account to use a single Client rather than a ClientMap, since each Account only manages a single identity (which can only live on a single network/Tangle). This change slightly simplifies the Account and AccountBuilder.

This also means an Account will not be able to create both mainnet and devnet identities by default, which was a side-effect of ClientMap automatically creating clients for both those networks by default.

A minor downside of this refactor is that AccountSetup always needs a Client on creation, which is an async operation. Previously the Clients were constructed lazily by the ClientMap. However, this change only affects our internal tests as far as I can see.

Minor change: the private Tangle examples now set the primary_node rather than just node since the behaviour for node differs between Rust and Wasm: on Wasm the node is considered "synchronised", while in Rust the node is considered "un-synchronised" until a background task (from iota-client) marks it as "synchronised". This causes the Rust private Tangle examples to throw an error (when pointing to a running private Tangle or devnet or mainnet) since it considers all nodes to be "un-synchronised". We use primary_node since it bypasses this check.

Added

  • Add AccountBuilder::client_builder (in addition to AccountBuilder::client to allow building a single Client or sharing an existing Arc<Client>).

Changed

  • Update AccountBuilder::client to take Arc<Client> instead of a ClientBuilder closure.
  • Update AccountBuilder::create_identity to use the network from the configured Client rather than IdentitySetup.
  • Update AccountBuilder::load_identity to throw an error if the DID network does not match the Client network, which would throw an error when publishing the document anyway.
  • Update Client::publish_document to throw an error if the DID network differs from the Client network.
  • Update Client::publish_diff to throw an error if the DID network differs from the Client network.

Removed

  • Remove Account::set_client.
  • Remove IdentitySetup::network.

Links to any relevant issues

Fixes issue #531

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

All 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

@cycraig cycraig 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 Jan 12, 2022
@cycraig cycraig linked an issue Jan 12, 2022 that may be closed by this pull request
7 tasks
@cycraig cycraig marked this pull request as ready for review January 12, 2022 13:11
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, looks good overall. I'm wondering whether we still need IdentitySetup::network, though.

identity-account/src/error.rs Outdated Show resolved Hide resolved
identity-account/src/account/account.rs Outdated Show resolved Hide resolved
identity-account/src/account/account.rs Outdated Show resolved Hide resolved
identity-account/src/account/builder.rs Outdated Show resolved Hide resolved
identity-account/src/account/builder.rs Outdated Show resolved Hide resolved
@cycraig
Copy link
Contributor Author

cycraig commented Jan 13, 2022

Should we expose the Client in the Account by adding an Account::client() method?

@PhilippGackstatter
Copy link
Contributor

Should we expose the Client in the Account by adding an Account::client() method?

What is the advantage of exposing the client? The network can be obtained through account.did().network() so I guess it's not because of that?

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! Looks good to me.

@cycraig
Copy link
Contributor Author

cycraig commented Jan 13, 2022

Should we expose the Client in the Account by adding an Account::client() method?

What is the advantage of exposing the client? The network can be obtained through account.did().network() so I guess it's not because of that?

To be able to access the Client to pass it to e.g. another AccountBuilder or Resolver is what I had in mind. I don't think it's necessary at this point so I'll leave the API as-is.

@cycraig cycraig merged commit f695052 into dev Jan 13, 2022
@cycraig cycraig deleted the feat/account-client-refactor branch January 13, 2022 15:00
@PhilippGackstatter
Copy link
Contributor

To be able to access the Client to pass it to e.g. another AccountBuilder or Resolver is what I had in mind. I don't think it's necessary at this point so I'll leave the API as-is.

I see, that makes sense. In #574 a getter for the storage was added, so maybe something similar will be required for the client as well.

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
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Refactor Account to use a single Client
3 participants