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

Refactor the Account state #462

Merged
merged 75 commits into from
Nov 17, 2021

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Oct 29, 2021

Description of change

This PR implements part of what was discussed in #433, namely the refactoring of the state representation in the Account.

The main changes:

  • Remove the event & commit system of the account and use a dual-document state. One document, the current on-tangle representation, is cached locally in storage, while all updates are applied to an in-memory document that is part of the account's state.
  • Remove the parts of the IdentityState that were essentially duplicates of the IotaDocument, e.g. TinyMethod, TinyService, etc. The state type now holds a document directly and only adds some state on top.
  • Remove diff and int generation from the state, and replace it with a single generation field. These generations are used for detecting whether an identity is fresh (generation == 0) and used in the location of keys to differentiate between keys with the same fragment, that were created in different generations.
    • The current solution is temporary until Enable moving keys stronghold.rs#283 lands, which will allow us to implement key locations based on the public key and the fragment, and remove all extra state such as generations.
    • Once the above is implemented, the IdentityState will be removed completely and the account can hold the IotaDocument directly.
    • In anticipation of the complete removal of IdentityState, the chain state was factored out of that type into a dedicated ChainState type. This keeps track of the previous integration and diff chain message id, which is required state for implementing automatic int or diff publishing.
    • The check for whether an identity is new is based on whether the previous integration message id is MessageId::null(), since the generation is anticipated to be removed in future updates.
  • Introduce MethodRelationship next to MethodScope, in order to disallow attaching relationships to a method with MethodScope::VerificationMethod.

This PR does not address:

  • Moving the existence and duplication checks for insertion and removal of methods to the IotaDocument.
  • Purging/Deleting keys in storage, this will be addressed with the key location refactor, although deleting keys as soon as they are no longer needed is slightly risky (if an update isn't included in the tangle) and hasn't been addressed in the account so far, so this might need some discussion.
  • Saving every update as a diff to storage. The state is currently only saved to storage on identity creation and after a successful publication. Dropping the account, with several unpublished updates in-memory, would get rid of these updates. A future PR will implement storing the diffs of each update in storage for later publication, or possible re-application on a changed base document (e.g. one that was resolved/synced from the tangle). One of the light blockers on including it in this PR, is that diffing metadata is currently hard, because identity_iota::did::doc::properties::Properties does not implement Diff. [Task] Implement metadata structures #486 will presumably make diffing metadata easier than it currently is, which should make it easier to implement this feature.

Since this is a large PR, and anyone would like to focus on a specific area of it, my suggestion would be to carefully review the publication logic around whether to publish as an integration or diff update, and whether the previous_message_id is always set correctly.

Finally, while this PR does not address every part of the planned refactor, it should be good enough to get started with the low-level access API implementation and the synchronization.

Links to any relevant issues

#433

Type of change

Add an x to the boxes that are relevant to your changes.

  • 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

Several tests were added or extended, namely the updates test, Publish tests, and adding or detaching relationships on an IotaDocument.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • 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 the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Nov 3, 2021
@PhilippGackstatter
Copy link
Contributor Author

Can't we store the altered document instead of the diffs?

That should also work, good idea!

I didn't notice any glaring issues with setting the message ids

Main reason I'm confused is that I didn't find where the previous message ids were set in the current code, specifically for integration updates.

I addressed most comments, except for the attach_method_relationship discussion (which I'm thinking of pushing to a later PR, when also moving more checks to the lower level for remove_method and insert_method) and the publish type with the UPDATE_METHOD_TYPES which I still want to add, to reduce the number of cases that require integration updates. Will work on that on Monday (presumably).

Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

I like the smaller crate level Error introduced in this PR. In general I would prefer to always return a named error instead of using an alias for the entire Result which seems to be mostly the case in this crate, but that is something we can think about when we get round to refactoring more of our error handling.

@PhilippGackstatter PhilippGackstatter changed the title Account state refactor Refactor the Account state Nov 16, 2021
@HenriqueNogara HenriqueNogara self-requested a review November 16, 2021 10:03
Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the changes!

Minor comments. I could have missed something between all the changes to be honest.

identity-did/src/verification/method_scope.rs Show resolved Hide resolved
identity-iota/src/tangle/publish.rs Show resolved Hide resolved
identity-iota/src/tangle/publish.rs Outdated Show resolved Hide resolved
identity-account/src/identity/chain_state.rs Outdated Show resolved Hide resolved
identity-account/src/identity/chain_state.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
@PhilippGackstatter PhilippGackstatter merged commit 24de8d8 into epic/account-refactor Nov 17, 2021
@PhilippGackstatter PhilippGackstatter deleted the chore/state-refactor branch November 17, 2021 07:52
PhilippGackstatter added a commit that referenced this pull request Nov 18, 2021
* Refactor `Account` to handle just one identity (#436)

* Partial impl to use did as storage identifier

* Move `CreateIdentity` into a separate type

Update Account API to return IdentityState from find_identity and create_identity (#414)

* Update Account::create_identity() to return IdentityState.

* Update Account::find_identity() to return IdentityState.

Update `IdentityUpdater` to only use account ref

Remove resolve, find and list

Replace `IdentityId` with `IotaDID` in account

Use `IotaDID` over `IdentityId` in stronghold

More replacement of id with did

Remove index from account

Fix stream impl

Fix stronghold impl, migrate `MemStore`

Re-add `resolve_identity` so tests can run

Fix Wasm network calls, pin reqwest to 0.11.4 (#439)

Rename `Command` -> `Update`

Rearrange `process_update`

* Fix clippy lints in memstore

* Let `RemoteKey` take a `dyn Storage`

* Implement `AccountConfig`

* Impl create_identity and load_identity in builder

* Rename JSON serialization field name to match spec. (#412)

* Rename JSON serialization field name to match spec.

* Rename notSupported -> representationNotSupported

* Reduce WASM build size (#427)

* reduce wasm build size

* Enable lto for Wasm release

Add `build-dev` task for Wasm for debugging.

Co-authored-by: Craig Bester <craig.bester@iota.org>

* Chore/combine examples (#420)

* Merged Stronghold with basic examples in account

* Updated explorer to identity resolver

* Added explorer URL after basic example

* Renamed examples

* Completed manipulate did example

* Fixed suggestions, removed replaced examples.

* Improved example readme

* Consistently use Stronghold and Resolver URL

* Fix examples

* Merged config example with private tangle

* low-level-api private-network example runs

* cargo fmt

* cargo fmt with nightly

* Impl suggestions

* Refactor config once more

* Update examples to use single-id account

* Don't pass did into `load_snapshot`

* Fix clippy lints

* Partially update `update` tests

* Fix update tests

* Ensure did exists in storage in `load`

* Remote `State` in account

* Rename `load` -> `load_identity`

* Remove `Identity{Id,Index,Key,Name,Tag}`

* Let `update_identity` take `&mut self`

* Document new and existing types

* Update top-level README example

* Fix visibility on `CreateIdentity`

* Add try_from implementation for ed25519 keypair

* Combine account tests

* Rename updates > commands for better reviewability

* Also rename in mod.rs

* Impl `AccountBuilder` without pub async fns

* Update README example with latest builder change

* Change visibility of setup, config, constructors

* Revert to the old `create_did` example

* Save to disk when creating identity, fix doc fmt

Co-authored-by: Craig Bester <craig.bester@iota.org>

* Use `authority` instead of `tag` in stronghold

* Rename `Config` -> `AccountConfig`

* Prevent two accounts from managing the same did

* Impl `IdentityBuilder`

* Only impl `IdentityCreate` under `#[cfg(test)]`

* Use `IdentityBuilder` in examples

* Remove unused `name` field

* Run workflow for `epic/*` branches

* Relase the lease in account tests

* Also run format, clippy and coverage workflows

* Revert "Use `IdentityBuilder` in examples"

This reverts commit 3d1b716.

* Revert "Only impl `IdentityCreate` under `#[cfg(test)]`"

This reverts commit 08ada09.

* Revert "Impl `IdentityBuilder`"

This reverts commit 1a5d645.

* Rename `IdentityCreate` -> `IdentitySetup`

* Add multi-identity example

* Simplify README example

* Rename `identity_setup` module

* Implement the `IdentityLease` newtype

* Rename `IdentityLease` -> `DIDLease`

* Update `resolve_identity` docstring

Co-authored-by: Matt Renaud <matt@m-renaud.com>
Co-authored-by: Thoralf-M <46689931+Thoralf-M@users.noreply.github.com>
Co-authored-by: Craig Bester <craig.bester@iota.org>
Co-authored-by: Jelle Femmo Millenaar <jelle.millenaar@iota.org>

* Apply new fmt granularity

* Refactor the `Account` state (#462)

* Get rid of `IdentitySnapshot`

* Start replacing parts of `IdentityState`

* Create identity document from core structures

* Impl int/diff update determination

* Remove `Deref` impls for IdentityState

* Implement `CreateMethod` update

* Impl attach/detach `MethodRelationship`

* Update to cap inv refactor

* Use `IotaDocument` directly to construct the doc

* Update tests for account updates

* Remove unused types & variables

* Return to unified publish method

* Implement & test attaching relationships

* Fix attach/detach tests post-merge

* Implement detach relationship test

* Implement insert and remove service

* Move `Publish` to low-level API

* Remove did field from account

* Refactor publishing & storing

* Rename int/diff generations

* Change fresh id detection & fix `store_state` call

* Factor out `ChainState` from `IdentityState`

* Remove `this_message_id` from chain state

* Test the chain state

* Fix some error TODOs

* Fix some TODOs

* Fix some storage todos/errors

* Make `IotaDocument::remove_method` fallible again

* Use name instead of identifier to call from_did

* Move state loading into the else clause

* Remove `persist` flag on `process_update`

* Remove unnecessary cloning

* Rename `as_document` -> `document`

* Improve `ChainState` docs

* Fix post-merge things & clippy lint

* Remove `Tiny*` and other unused state types

* `InvalidMethodTarget` -> `InvalidTargetMethod`

* `as_document_mut` -> `document_mut`

* Expand lazy test

* Rename impl_command_builder to update

* Remove event context

* Rename `events` module to `updates`

* Apply fmt

* Rename `commands` -> `updates`

* Remove `UnixTimestamp`

* Remove unused errors in account & update

* Change `fromDID` method in Wasm

* Return `Option<Publish>` from constructor

* Use strum, add todo!(), rename `PublishType`

Co-authored-by: Craig Bester <craig.bester@iota.org>

* Move attach/detach relationship to `CoreDocument`

* Return bool from `detach_method_relationship`

* Use `MethodQuery` in attach/detach

Co-authored-by: Craig Bester <craig.bester@iota.org>

* Revert "Change `fromDID` method in Wasm"

This reverts commit cb0cf35.

* Nest `MethodRelationship` in `MethodScope`

* Move some chain state logic to publish_diff_change

* Don't expect, bubble up

* Return early if there's nothing to publish

* Add todo about error handling

* Remove mut state methods

* Move account constructors to the top

* Use `try_resolve_method_with_scope` in tests

* Remove todos, inline code, explicitly ignore res.

* Remove todo comments, `unwrap`

* Rename chain state fields to `last_*`

* Improve `InvalidTargetMethod` Error name

* Increment actions when updating

* check for updated referenced method in publish

* Enable diff updates for merkle cap. inv. methods

* Only attach relationship on non-embedded methods

* Add proper type annotations in publish

* Improve chain state docs & variable naming

Co-authored-by: Craig Bester <craig.bester@iota.org>

* Fix READMEs

* Deduplicate setup code in builder

* Remove tangle dependency in autopublish test

* Add docs for attach/detach relationship

* Deduplicate signing key extraction

* Fix documentation & annotate types in examples

Co-authored-by: Matt Renaud <matt@m-renaud.com>
Co-authored-by: Thoralf-M <46689931+Thoralf-M@users.noreply.github.com>
Co-authored-by: Craig Bester <craig.bester@iota.org>
Co-authored-by: Jelle Femmo Millenaar <jelle.millenaar@iota.org>
@PhilippGackstatter PhilippGackstatter added Enhancement New feature or improvement to an existing feature Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Dec 3, 2021
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 Enhancement New feature or improvement to an existing feature 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.

4 participants