[sui-adapter][sui-framework] Add child_count to Move objects #3730

tnowacki · 2022-08-03T02:18:04Z

Added a count of children to objects
The count is incremented/decremented in the adapter when children are added, transferred away, or deleted
The count must be zero when an object is deleted, frozen, or wrapped
- Wrapped objects with children cannot be supported without adding the child count to the parent_sync. @lxfind seemed unsure if this was a wise idea
- We can always come back and do that later if we want to

lxfind · 2022-08-10T03:33:03Z

crates/sui-framework/sources/transfer.move

@@ -26,7 +26,7 @@ module sui::transfer {
    /// module. The object's module can expose a function that returns a reference to the object's
    /// UID, `&UID`. Which can then be used with this function.
    /// The child object is specified in `obj`, and the parent object id is specified in `owner_id`.
-    public fun transfer_to_object_id<T: key>(obj: T, owner_id: &UID) {
+    public fun transfer_to_object_id<T: key>(obj: T, owner_id: &mut UID) {


Why mut here (doc needs to be updated, and worth explaining there too)?

Sure I can update the doc here, but it makes it easier to block immutable objects from adding children

crates/sui-types/src/object.rs

crates/sui-adapter/src/adapter.rs

lxfind · 2022-08-10T23:33:54Z

crates/sui-adapter/src/adapter.rs

                            state_view.log_event(Event::delete_object(
                                module_id.address(),
                                module_id.name(),
                                ctx.sender(),
                                *obj_id,
                            ));
-                            state_view.delete_object(obj_id, version, DeleteKind::Normal)
+                            if let Owner::ObjectOwner(parent_id) = owner {
+                                let delta = child_count_deltas.entry(parent_id.into()).or_insert(0);


Does 0 defaults to i64? Might be worth annotating the type of child_count_deltas just to make sure we don't accidentally break the type

I added an annotation. Just very nervous about or_default in general. But I can switch to that if you think it makes it more clear

crates/sui-adapter/src/adapter.rs

lxfind · 2022-08-11T01:03:11Z

crates/sui-adapter/src/adapter.rs

                            state_view.log_event(Event::delete_object(
                                module_id.address(),
                                module_id.name(),
                                ctx.sender(),
                                *obj_id,
                            ));
-                            state_view.delete_object(obj_id, version, DeleteKind::Normal)
+                            if let Owner::ObjectOwner(parent_id) = owner {


Why don't we check here whether the object to be deleted has 0 child at this point? Wonder if that would simplify things (always check child count at deletion point)

We might not have processed the children yet. It is so you can delete all children and all parents in the same txn.

We could instead process the transfers children-first, but it would require some sorting here

Shouldn't the events be processed in the order they are created?

Do we want to enforce that the parents must be deleted exactly before the children? It feels like an unnecessary footgun. Especially when a lot of this complexity is still needed for dealing with wrapped objects.

Did you mean "the parents must be deleted after all the children were deleted"?

That seems a reasonable requirement?

First, it doesn't make the code any easier. As we only know if an object has been wrapped at the end. So this code won't get get any less complex.

Second, I don't think it is a great requirement, because there is nothing in the Move layer that indicates you will bump into this. We have the ability to easily make people's lives easier without changing the complexity of the code, so why not?

lxfind · 2022-08-11T01:09:11Z

crates/sui-adapter/src/temporary_store.rs

+                let mut object = self.objects[id].clone();
+                // Active input object must be Move object.
+                object.data.try_as_move_mut().unwrap().increment_version();
+                self.written.insert(*id, object);


I don't quite understand this.
btw any object update now must be done through self.write_object() to make sure previous transaction is updated.

I can try to do that. But it is just trying to mark the object as written if the child_count has changed

Changed this a bit, but I'm still manually inspecting the _written map

hmm actually, this may be an issue. So some objects may get to be written twiece

I'm not sure I understand the concern

lxfind · 2022-08-11T01:12:49Z

crates/sui-adapter/src/temporary_store.rs

+            }
+        }
+        for (id, delta) in deltas {
+            match (self.written.get_mut(&id), self.deleted.get_mut(&id)) {


Having trouble wrapping my head around this match, some comments would help

Added comments

- Added a count of children to objects - The count is incremented/decremented in the adapter when children are added, transferred away, or deleted - The count must be zero when an object is deleted, frozen, or wrapped

…jects

tnowacki requested review from lxfind, awelc, sblackshear, damirka and emmazzz August 3, 2022 02:18

tnowacki mentioned this pull request Aug 3, 2022

[sui-adapter][sui-framework] Add child_count to object::Info #3392

Closed

tnowacki force-pushed the rust-child-count branch 2 times, most recently from 1183fcb to 84f755b Compare August 3, 2022 20:09

tnowacki mentioned this pull request Aug 4, 2022

[docs] Fix "programming with objects" #3767

Merged

lxfind reviewed Aug 11, 2022

View reviewed changes

tnowacki force-pushed the rust-child-count branch from 84f755b to 0492ef2 Compare August 11, 2022 18:04

oxade self-requested a review August 11, 2022 18:26

tnowacki force-pushed the rust-child-count branch from 0492ef2 to 6b5c746 Compare August 11, 2022 21:51

lxfind approved these changes Aug 11, 2022

View reviewed changes

tnowacki force-pushed the rust-child-count branch from 32a3aca to d48adbe Compare August 12, 2022 01:20

tnowacki added 3 commits August 12, 2022 11:34

[sui-adapter][sui-framework] Add child_count to Move objects

2e70239

- Added a count of children to objects - The count is incremented/decremented in the adapter when children are added, transferred away, or deleted - The count must be zero when an object is deleted, frozen, or wrapped

fixup! [sui-adapter][sui-framework] Add child_count to Move objects

bdd7c95

fixup! fixup! [sui-adapter][sui-framework] Add child_count to Move ob…

a500e39

…jects

tnowacki force-pushed the rust-child-count branch from d48adbe to a500e39 Compare August 12, 2022 18:34

tnowacki enabled auto-merge (squash) August 12, 2022 18:34

tnowacki merged commit 8f70bc0 into MystenLabs:main Aug 12, 2022

tnowacki deleted the rust-child-count branch October 4, 2022 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sui-adapter][sui-framework] Add child_count to Move objects #3730

[sui-adapter][sui-framework] Add child_count to Move objects #3730

tnowacki commented Aug 3, 2022 •

edited

Loading

lxfind Aug 10, 2022

tnowacki Aug 11, 2022 •

edited

Loading

lxfind Aug 10, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

lxfind Aug 11, 2022

tnowacki Aug 11, 2022

[sui-adapter][sui-framework] Add child_count to Move objects #3730

[sui-adapter][sui-framework] Add child_count to Move objects #3730

Conversation

tnowacki commented Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

tnowacki Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki commented Aug 3, 2022 •

edited

Loading

tnowacki Aug 11, 2022 •

edited

Loading