-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
Conversation
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- if the object was in the input of the transaction
- 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).
There was a problem hiding this 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() |
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
c5aad55
to
34ae29a
Compare
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:
Specifically, this PR does the following:
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).wrapped
andunwrapped
to signed effects, create them properly.Closes #588.