-
Notifications
You must be signed in to change notification settings - Fork 90
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
[Task] Refactor the Account state #433
Comments
State representationTo implement this issue I think we first need to come to a conclusion about how we want to represent the state of a document in the account. The Another approach is to have two The current approach is hard to reconcile with the low-level API access, since the state is not naturally represented as a document, and we cannot simply return a mutable The events are used to determine whether a collection of events should be published as a diff message or an integration message. However, iiuc, the logic is basically that it's an integration message if the document was just created, and a diff otherwise. This logic can be used with documents as well. So are there any reasons for keeping the event system? Where is the state?A second discussion is around where that state is stored. With #436 an account handles only one identity. Loading the The first question here is whether we want to prevent this or leave it up to downstream developers. I think some in the team have previously argued for the latter. I'm not quite convinced yet that this would be a great API, but can also see that the alternative is more complex. Just to have it as an alternative, one other approach is to have the state live in the Any thoughts on whether we should synchronize access to the same identity or leave it up to the developer? |
Regarding the state representation: I agree that although elegant, if not used as the source of truth, the event source is nor very useful. Is the logic for integration messages really that simplistic? I remember discussion where there were other cases mentioned but maybe they never made it into code, @JelleMillenaar what do you think? |
If the
I think #436 took a stance on this to prevent concurrent access to a DID from more than one (local) tl;dr I'm in favour of using two documents to represent the internal state in the Edit: synchronising from the Tangle might actually be more difficult with an |
So how the tangle sync works is one of the primary concerns for the state representation, but I think we don't have a clear picture of how that's going to work, yet? This feels like a prerequisite to make a good decision here. |
Jelle and I discussed this a bit. The documents approach should work, if the sync works roughly like this: Whenever The local update is essentially lost. If we store updates as events in the store, then we'd have to remove that specific update from the store, during the next sync. It seems non-trivial to convert the on-tangle state to events and figure out if it should be removed or not. Having just a document as state, means on the next sync, the local doc is simply overwritten with the latest on-tangle state. Then apply the update again. Essentially it means: If you have a multi-device setup, you should call
The tangle is the source of truth. Any updates applied to a local state that didn't have that service removed, are ignored on-tangle, and a sync is required before a valid update can be published. Not entirely sure if that answers your point, though. Aside: Would be nice to have |
Since writing the above message, I felt like we should do better than just say "you can publish updates, but they might just be lost". This is mainly an elaboration on Craig's idea with the document and the set of updates. State representationAs state, we keep in the storage (or in account, see open questions):
SynchronizationSynchronization here is defined as the process of exchanging messages between account and Tangle, such that afterwards, both local and on-tangle state are the same. To that end, we first do a lighter-weight resolution: As an additional local state, we keep two sets of message ids for the int (
Publish UpdatesThe publish process itself is also modified, such that updates that have originally been applied to a local state that was older than what was on-tangle, are tried to be applied to the newer state. Reason being, that if we publish updates without first synchronizing, we risk our updates being ignored (because of
Low-Level AccessEdit: I mistook With this state representation, granting low-level access is not quite straightforward. With normal updates, each update produces an To make this easier, we can also give out a copy of the current account state, let them make changes to it, and resubmit it, in which case we can easily make a diff. This raises other problems such as updating the account in the meantime, which is not a problem with the previous approach, since the document is mutably borrowed and so the account cannot be mutably borrowed to make updates at the same time. To suffer from neither problem, we could take a closure and let users update the document through it. For example: account.update_identity_unchecked(|document: &mut IotaDocument| {
// Do some unchecked things
document.methods.append(...)
}).await?; In Notes & Open Questions
Overall thoughts on proceeding this way? What did I miss? |
Thank you for the writeup. I largely agree with most points. 1. State representation:I agree that storing the latest document and a set of updates seems necessary to prevent lost updates while retaining local changes. 2. Synchronization:The approach for lighter-weight resolution is a neat optimisation for frequent synchronisations (might be useful in the resolver too) but I would not implement it until milestone ordering is used for deterministic resolution. I'd rather be slow and correct than fast, but it's still a discussion. 3. Publish Updates:I fully agree that synchronisation should be optional to avoid the resolution overhead. I imagine the range of applications which do not encounter distributed document updates is larger, so ensuring they are not negatively impacted is important.
I envisioned that synchronisation would simply replace the local version of the document with whatever is on the Tangle, then a subsequent 4. Low-Level Access:This seems to be where we diverge.
Update: After thinking about this more, I agree the
E.g. let mut updated_document: IotaDocument = account.previous_document().clone();
updated_document.insert_method(...);
updated_document.insert_service(...);
updated_document.remove_method(...);
let diff: DocumentDiff = account.previous_document().diff(updated_document)?;
account.update_identity_unchecked(diff);
account.publish_updates().await?; My philosophy with low-level/direct access is that the
That seems like a decent way of tackling it, they may be on an epic branch but could also be merged into dev separately. I honestly prefer incremental updates to dev to prevent major changes from epic branches compounding and breaking everything at once with large merge conflicts. |
Alternative Solution ExplorationThe entire idea for the proposed solution (representing the state as a document with update events) revolves around merging local changes with remote changes. What if we disregard that requirement and simply discard local changes on synchronisation? This is basically playing devil's advocate for a bit. Why disregard merging local updates with the remote document? Local changes are likely to be short-lived: are there any use-cases for making one or more document updates and not publishing them to the Tangle almost immediately? Any new/updated verification methods cannot/should not be used to sign anything since if they're not published the signature is meaningless, and services are similarly only useful when publicly viewable on the Tangle, so an update would naturally be published as soon as feasible. In that case, a typical workflow would synchronise first and look something like: account.synchronise().await?; // fetch latest document
account.update_identity()... // make some changes
account.update_identity()...
account.publish_changes().await?; // publish immediately The timespan between synchronising and publishing, which is where a conflicting document update to the Tangle from a different agent could occur, is going to be relatively small. I imagine the time between synchronising and publishing here is similar to the proposed workflow of applying updates then synchronising just before publishing, since proof-of-work and retrying takes some time in addition to confirming the message. account.update_identity()... // make some changes
account.update_identity()...
account.synchronise().await?; // fetch latest document
account.publish_changes().await?; // merge updates and publish, still takes time due to PoW/confirmation To immediately counter this argument, however, the timespan can be greater when dealing with e.g. lost internet connection on a mobile SSI wallet, or a data centre outage causing updates to be stuck on local storage before being published to the Tangle. Dual Document State:This is my previous proposal where two documents represent the Advantages:
Disadvantages:
NOTEI believe both solutions suffer from the possibility for "lost" messages where:
Coordination in distributed systems is a difficult problem; without some sort of locking mechanism we cannot eliminate these problems entirely. To be clear: I am still partially in favour of the proposed solution (compared to the alternative solution with two documents), this was just to explore what would change if we dropped a certain requirement and could simplify the |
Synchronization & StateYou raise some excellent points there, thanks a lot! I agree that the assumption for a need to merge changes is moot, considering the timespan in the general case. In that case I no longer see a good reason to keep the event system, and am in favour of using only documents as state. I'm glad you brought this up, since we might be able to get rid of the event system completely and work with diffs only. I also like that your alternative solution is easier to understand from an outside perspective, as synchronizing, updating and publishing are their own methods, with the slight cost of potentially forgetting them. In terms of where the state lives, we may want to have it in the account and storage at the same time. We write to storage on every update, but that doesn't necessarily gets propagated to disk, iirc. To be more specific, we'd have two documents in storage as state: the tangle doc and the updated one, but the account only needs to hold the latest doc. In the account we can then grant access to the latest document, as you mentioned, and we can apply the unchecked updates directly to that, too. Nit: Not sure if I like the name "synchronization" in that case, since that implies for me that it's a bidirectional exchange of messages, not just one-way. Fetch or pull might be more appropriate. With these method names I would be less surprised if my local state was overwritten. Not too strong of an opinion, though :).
Absolutely. It's just a tiny bit less likely to happen when we can guarantee that synchronizing and publishing happen one after the other, rather than giving users the responsibility of synchronizing and applying updates in time. But as you've shown, it doesn't matter for the majority of cases.
I feel like this is less of a problem with your solution, actually. Since you just synchronized, it's very unlikely that an update you applied has to be dropped. In the merge solution, this is more likely to happen, and I had not thought about how to deal with this. I'm not sure we need to fix this, given how unlikely it is. Having a few stray private keys in a stronghold isn't a big deal, I think. Low-Level AccessI will skip over some of the points you raised for Overall, the document-based state makes low-level access easier so that's an improvement. If we do want to write the latest unchecked update to storage though, we have the same issues as I described in the latest of my proposals. I still think the closure approach is not too bad.
The closure can return an error too, which in turn propagates out of fn update_idenity_unchecked<F: Fn(doc: &mut IotaDocument) -> identity_account::errors::Result<()>>(&mut self, f: F) -> identity_account::errors::Result<()> so
Honestly, I don't know. The only example I know of is setting the previous message id as you mentioned, but it'd be really helpful to have a few more examples of what unchecked updates should and shouldn't allow. That being said, if we allow arbitrarily modifying a document and add a
While we want to allow access to low-level, unsafe APIs, I would still argue we should do what we can to make it as easy to use as possible. Diffs are an API devs need to learn, and it seems like something we should be able to wrap. Rather than going in this direction, I would still prefer the other options, like returning a copy of a document, let developers mutate it arbitrarily and publish it unchecked (which just does the diff internally). Here, we just need to be very clear about what invariants we expect the caller to uphold (because simultaneous updates to the account can be bad). To summarize, I'm in favor of the dual document state. I would like to get started with the implementation, so if there is no disagreement, I would go ahead. We can still continue to discuss the synchronization and the low-level API. |
Synchronization & State
We could still have updates publish immediately, but honestly I think that's just a convenience and explicitly choosing when to publish isn't that much of a hassle. I feel like "forgetting to publish" is just a bug by any other name and synchronising is based on the application.
I think
Maybe we could purge any key locations for methods no longer in the new document? I think "just leave them there" is a valid option since discovering my Stronghold keys were deleted automatically may be a bit of a shock. Low-Level Access
I'm still nervous about going all-in on simplifying the
In the dual-document approach we don't need to create the
Yes, I think
I don't think we need to go that far, we'll have to see on a case-by-case basis. On a related note, we probably need more options for the regular ConclusionI think we're mostly aligned now?
Edit:
We are essentially saying this now. However, as previously mentioned, the possibility for lost updates due to distributed updates to a DID document is something developers will need to deal with in any case. |
Yes, I'm perfectly fine with that.
I'm not sure actually, since there is merge functionality in the lower-level API, too. We could still hold a list of diffs that may have been applied to another document, and try to reapply (
Yes, but now I understand your point about only needing to do that in Tos summarize: Just returning a
👍
I know. Your point about the timespans showed that choosing the more complex system with merging has almost the same problems as that without, so I'm okay with going the simpler route. |
Note: This issue used to be about low-level API access, but this task has moved to #475.
Description
Refactor the
Account
state to enable lower-level access (#475) to the document stored inside. This should refactor the account snapshots with its event system to useIotaDocument
s to represent the state of an identity. The stored document represent the last resolved tangle state of the document. When an account is created, this document is loaded and any updates to the identity are applied to that in-memory document.Whether these updates are written into storage or stay temporary until published needs to be figured out. Doing so would enable updates to be applied to the identity locally, and publish them at a later point. The downside is the higher complexity involved, since the low-level access to the document isn't explicit to the account, so it cannot write them into storage immediately. Some caveats may apply in that case.
Finally, the refactor should introduce an abstraction that allows for easier implementation of future did methods.
Motivation
Simplify the internal state representation and take advantage of the account only having to manage one identity now.
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.
IdentitySnapshot
and the event/commit systemIotaDocument
with any better APIs fromIdentityState
IotaDocument
and apply all updates to that documentThe text was updated successfully, but these errors were encountered: