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

Add wrapped and unwrapped to authority store and signed effects #658

Merged
merged 3 commits into from
Mar 8, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Mar 4, 2022

In #406, we added support to tell when an object is being deleted in a transaction (through a DeleteObjectID event). But we were not processing this event.
This PR uses that event, so that we could tell which object is being deleted and which is being wrapped (similarly, which is being created and which is being unwrapped).
There are two reasons we want to distinguish them:

  1. Clarify to the users. Users will find it confusing to see their objects deleted but in fact they were just wrapped; similarly it's confusing to see an object being created, in fact they were being unwrapped.
  2. More accurate information on the authorities, which will allow us to return more accurate information regarding the latest state of an object (wrapped instead of deleted and etc.). It will also allow us to delete objects from the store if they were really deleted.

Specifically, this PR does the following:

  1. In adapter.rs, we now process the object id deletion events, and add them to a list. Based on the list we can construct a more accurate story on object deletions. There are 3 types of deletions (defined in DeleteKind): ExistInInput means objects that were in the input and being actively deleted; Wrap means objects that were in the input and being wrapped; NotExistInInput means objects that weren't in the inputs, but seems to be actively deleted (reasons on why this could happen are in the comments).
  2. The addition of these deleted ids that are not in the input has some consequences and need to be dealt with while patching up the unwrapped object versions.
  3. Add wrapped and unwrapped to signed effects, create them properly.
  4. Update the authority store with the accurate digest.
  5. Added a detailed test on the version and digest for wrapping and unwrapping.

Closes #588.

Copy link
Contributor

@velvia velvia left a comment

Choose a reason for hiding this comment

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

Will have to come back later on this, it's a big review and most of the rest of the code I'm not familiar with. :-p

// We need to call get() on objects because some object that were just deleted may not
// be in the objects list. This can happen if these deleted objects were wrapped in the past,
// and hence will not show up in the input objects.
.filter_map(|(id, _)| objects.get(id).and_then(Object::get_single_owner_and_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

This objects is the temp store, that would have a copy of objects that are deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't. The temp store gets objects from two ways:

  1. if the object was in the input of the transaction
  2. if the object was written during the transaction

For an object that was wrapped in the past (i.e. embedded in another object), and got deleted in this transaction, we will not have the object content anywhere (except inside the object it was embedded in).

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This LGTM, I left comments to improve the rustdoc. I wouldn't be that nitpicky usually, but I think they would help here in letting the reader jump to parts of the code that define invariants that work with each other.

Another way to address this need to "read it all before you modify one part" in metadata is to use stg like pre, but the rustdoc is perhaps more reasonable in this instance.

// VersionedID { id: UniqueID { id: ID { bytes: address } } .. }
// ```
versioned_id
.copy_value()
Copy link
Contributor

Choose a reason for hiding this comment

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

I must admit this is a bit terrifying, but I don't have much of an alternative, except for an imbricated pattern match that the unpack makes unwieldy, so ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will clean this up in #675.

@lxfind lxfind force-pushed the add-wrapped-to-effects branch from c5aad55 to 34ae29a Compare March 8, 2022 03:48
@lxfind lxfind merged commit 945dd9d into main Mar 8, 2022
@lxfind lxfind deleted the add-wrapped-to-effects branch March 8, 2022 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wrapped to Signed effects
3 participants