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

Support immutable objects #140

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Support immutable objects #140

merged 1 commit into from
Jan 11, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Jan 8, 2022

This PR introduces immutable objects for #96.
It does so by:

  1. A read_only field is added to Object. Module objects will set it to true. Move objects will set it to false by default, but can be set to true through to_immutable().
  2. Added a ToImmutable module in Move framework, with two public functions: to_immutable() and transfer_and_to_immutable(). Note that we cannot name it feeze() as it is restricted keyword in Move. Since to_immutable() consumes the entire object, we need to provide a way for developers to set a newly created object to immutable too. To do so, we must provide transfer_and_to_immutable() so that the object is set to immutable and transfer to someone at the same time. to_immutable() has the recipient field as empty, while transfer_and_to_immutable() has the valid recipient field.
  3. To distinguish this event and the transfer event (both have object and potentially recipient), we take advantage of the seq_num field in the event.
  4. In adapter, we then process the event based on seq_num.

Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

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

Nice!

@lxfind
Copy link
Contributor Author

lxfind commented Jan 10, 2022

Addressed the comments with the following changes:

  1. Deleted "Immutable.move" as it's no longer needed.
  2. I ended up only adding a "transfer_and_freeze" method in Transfer module. The caller must provide a recipient, even if the recipient is the current owner of the object (i.e. only freeze, but no transfer).
  3. Added a should_freeze parameter to the transfer_internal native function to indicate whether we want to freeze the object after transfer.
  4. When processing the "transfer_internal" event during execution, we don't charge gas based on object size (but only charge a constant gas instead), because we would charge it based on the size in the adapter if we end up creating a new object (for transfers that don't create new objects, my thought is that we should charge based on object size, but that's open to iterate).

Also some notes on formally supporting Move events. It seems really difficult to do given what we have right now.
If we define a native function like this:

native fun emit_event<E>(e: E);

where E is typically a custom Event struct that wraps an object type T.
In the adapter, we would need to some how be able to figure out the inner type T, which seems really difficult to do.

@sblackshear
Copy link
Collaborator

sblackshear commented Jan 11, 2022

where E is typically a custom Event struct that wraps an object type T.

For user-defined events, I think we must define native fun emit_event<E: drop>(e: E). This will prevent events from containing objects (which is what we want, since we don't want users to accidentally pack tokens into an even + want custom object destruction policies to be respected), but also means we can't use this native function for transfers.

This is unfortunate, but since transfers are the only "system" event type, I think having a separate native function for it is ok.

@lxfind lxfind merged commit eda23a1 into MystenLabs:main Jan 11, 2022
mwtian pushed a commit that referenced this pull request Sep 12, 2022
* Add simple grpc server to primary & add test to hit embedded grpc server in primary

* Checkpointing gRPC Server + get_collections endpoint

* Switch from build.rs to bootstrap method

* Completed get_collections endpoint

* Cleanup for review

* small typo

* fix license headers

* update generated narwhal.rs

* Refactoring/Changes based on review comments

* Only spawn grpc server if external consensus is used & fix broken tests

* fix benchmark_client: missed field name update

* Add port picker for benchmarking and address other review comments

* Fix small typo

* rename proto bytes fields with more descriptive names
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
* Add simple grpc server to primary & add test to hit embedded grpc server in primary

* Checkpointing gRPC Server + get_collections endpoint

* Switch from build.rs to bootstrap method

* Completed get_collections endpoint

* Cleanup for review

* small typo

* fix license headers

* update generated narwhal.rs

* Refactoring/Changes based on review comments

* Only spawn grpc server if external consensus is used & fix broken tests

* fix benchmark_client: missed field name update

* Add port picker for benchmarking and address other review comments

* Fix small typo

* rename proto bytes fields with more descriptive names
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.

3 participants