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

[MoveosStd] Refactor ObjectRef, context:new_object and context::remove_object #948

Merged
merged 5 commits into from
Oct 14, 2023

Conversation

jolestar
Copy link
Contributor

Summary

[MoveosStd] Refactor ObjectRef, context:new_object and context::remove_object

  1. Make ObjectRef can not copy.
  2. context::new_object<T> auto add Object to storage, and return ObjectRef
  3. context::remove_object<T> auto unpack the Object, and return (id, owner, T)
  4. Refactor examples to use ObjectRef.

Continue #935

Part of #901

@geometryolife Some documents and example readme need to be updated.
@wubuku the code generator needs to be updated with new Object and ObjectRef API.

@vercel
Copy link

vercel bot commented Oct 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
rooch ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2023 1:19am

@jolestar jolestar requested review from WGB5445 and pause125 October 11, 2023 06:13
new_object_with_id(self, object_id, owner, value)
}

public(friend) fun new_object_with_id<T: key>(self: &mut Context, id: ObjectID, owner: address, value: T) : ObjectRef<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If new an object and return ObjectRef by default, need to additionally call the object_ref::id method to get the object_id. Looking at the example, only need the object_id in many cases, which will increase the burden to the developer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The return value here is actually to save the Ref. In this way, we can save the creator's permissions, so that anyone who needs to can operate the object anywhere. If the return value is ObjectID, we can only add a field in to determine whether the user has permission to operate the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, developers needed to call object::id(&obj) to get ObjectID, now, replaced by object_ref::id(&obj_ref).

@WGB5445
Copy link
Collaborator

WGB5445 commented Oct 11, 2023

ObjectRef is a way to divide Object permissions. ObjectRef allows anyone to operate on the Object body. Should we abstract the permissions of sending and extending Objects, so that Objects are more flexible?

@jolestar
Copy link
Contributor Author

ObjectRef is a way to divide Object permissions. ObjectRef allows anyone to operate on the Object body. Should we abstract the permissions of sending and extending Objects, so that Objects are more flexible?

Do you have some ideas about this?

@WGB5445
Copy link
Collaborator

WGB5445 commented Oct 11, 2023

ObjectRef is a way to divide Object permissions. ObjectRef allows anyone to operate on the Object body. Should we abstract the permissions of sending and extending Objects, so that Objects are more flexible?

Do you have some ideas about this?

Here, we limit the sending of Objects of type to the module of T. We can modify Objects so that they can be sent anywhere.

https://github.com/rooch-network/rooch/blob/aa85fd70355e8b9ad77a2004f22301185fe74489/moveos/moveos-stdlib/moveos-stdlib/sources/object.move#L56-#L60

We can limit the sending permissions of Objects by adding some fields. Only users who have TransferRef can send this Object.

struct Object {
    allow_ungated_transfer: bool,
}

As a result, all Objects can be sent arbitrarily, and the creator can close the sending of some Objects. Here is the implementation of Aptos, which I think is very good.

https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/object.move#L400-L410

@jolestar
Copy link
Contributor Author

Here, we limit the sending of Objects of type to the module of T. We can modify Objects so that they can be sent anywhere.

https://github.com/rooch-network/rooch/blob/aa85fd70355e8b9ad77a2004f22301185fe74489/moveos/moveos-stdlib/moveos-stdlib/sources/object.move#L56-#L60

We can limit the sending permissions of Objects by adding some fields. Only users who have TransferRef can send this Object.

struct Object {
    allow_ungated_transfer: bool,
}

As a result, all Objects can be sent arbitrarily, and the creator can close the sending of some Objects. Here is the implementation of Aptos, which I think is very good.

https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/object.move#L400-L410

We can provide a public_transfer to Object, if needed. The public_transfer requires the T has key + store ability but does not require private_generics(T) like Sui.

@WGB5445
Copy link
Collaborator

WGB5445 commented Oct 11, 2023

Here, we limit the sending of Objects of type to the module of T. We can modify Objects so that they can be sent anywhere.
https://github.com/rooch-network/rooch/blob/aa85fd70355e8b9ad77a2004f22301185fe74489/moveos/moveos-stdlib/moveos-stdlib/sources/object.move#L56-#L60
We can limit the sending permissions of Objects by adding some fields. Only users who have TransferRef can send this Object.

struct Object {
    allow_ungated_transfer: bool,
}

As a result, all Objects can be sent arbitrarily, and the creator can close the sending of some Objects. Here is the implementation of Aptos, which I think is very good.
https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/object.move#L400-L410

We can provide a public_transfer to Object, if needed. The public_transfer requires the T has key + store ability but does not require private_generics(T) like Sui.

I believe it is necessary, As a special type, Object should have some differences from ordinary structs, such as inheritance, polymorphism, and extensibility.
And I think it is essential that Object be extensible. A built-in typetable or other extension mechanism would increase Object's flexibility.

@jolestar jolestar force-pushed the object_ref_refactor branch from aa85fd7 to 34b56aa Compare October 12, 2023 05:03
@jolestar jolestar force-pushed the object_ref_refactor branch from 34b56aa to e79f988 Compare October 12, 2023 15:03
public fun add_object<T: key>(self: &mut Context, obj: Object<T>) {
storage_context::add<T>(&mut self.storage_context, obj)
/// Remove object from object store, and unpack the Object
public fun remove_object<T: key>(self: &mut Context, object_id: ObjectID): (ObjectID, address, T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

After remove object, the original ObjectRef reference still exists. If call object_ref::borrrow or object_ref::borrrow_mut, the underlying raw_table will throw a NotFound error code. The Rust life cycle management mechanism fails in this dimension.

Should we consider adding track ObjectRef?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This problem does exist, So is it better to only handle the Object life cycle through Ref?
Only read using ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Record a TODO about this

public fun new<T: key>(object: &mut Object<T>) : ObjectRef<T> {
//TODO should we track the reference count?
new_internal(object)
}

Currently, before using ObjectRef, the dev needs to call object_ref::exist_object to ensure the object exists.

@jolestar jolestar merged commit 7046836 into main Oct 14, 2023
4 checks passed
@jolestar jolestar deleted the object_ref_refactor branch October 14, 2023 03:29
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.

4 participants