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 Collection module to fastx framework #351

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Add Collection module to fastx framework #351

merged 1 commit into from
Feb 8, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 3, 2022

This PR adds Collection, a heterogeneous object collection that owns a list of objects with potentially different types. This is a modules that demonstrates how object ownership can be used.

There are some other issues preventing us from exposing the add function as entry points: #348

@lxfind
Copy link
Contributor Author

lxfind commented Feb 3, 2022

Made a few changes:

  1. Added remove_and_take to Collection. Now remove will return the object being removed, and it's no longer an entry function; while remove_and_take can be an entry function and always transfer the object to the signer.
  2. Call Collection::transfer instead of Transfer::transfer in the test.

@lxfind lxfind force-pushed the object-owner-tests branch from b6dccc8 to a7cbd8a Compare February 3, 2022 07:02
Base automatically changed from object-owner-tests to main February 3, 2022 07:37
Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

I have a question here: so we allow an owner to add an object to a collection, which is very cool. However I think that it is possible for the owner of the collection object to transfer the object owned to someone else?

    // Remove the object from the collection. After the removal,
    /// the object is still owned by the collection. Caller will need
    /// to decide where to transfer it to.

What prevents the owner of the collection from moving the object in a collection, while it is still in the collection? Would it not be safer to wrap the object in the collection into a special object that denotes items in this collection, and unwrap it when it comes out of the collection, and forbid it moving while it is still in the collection?

@sblackshear
Copy link
Collaborator

I have a question here: so we allow an owner to add an object to a collection, which is very cool. However I think that it is possible for the owner of the collection object to transfer the object owned to someone else?

    // Remove the object from the collection. After the removal,
    /// the object is still owned by the collection. Caller will need
    /// to decide where to transfer it to.

What prevents the owner of the collection from moving the object in a collection, while it is still in the collection? Would it not be safer to wrap the object in the collection into a special object that denotes items in this collection, and unwrap it when it comes out of the collection, and forbid it moving while it is still in the collection?

Every function that can transfer an object requires Move ownership (e.g., a T instead of &mut T) and makes the transferred object unreachable for the rest of the transaction. Because you can't get Move ownership of an item in the collection without also removing it, it either has to be in one place or the other.

I do think the comment is potentially confusing, though... Every object remains owned by whoever owned it at the beginning of the transaction unless it is explicitly passed to a transfer (in which case it will be unreachable for the rest of the transaction, so no ownership change can be observed by the program). Said more concisely, object ownership is invariant within a single transaction, but the comment makes it sound like this might not be true

@lxfind
Copy link
Contributor Author

lxfind commented Feb 5, 2022

Addressed feedback:

  1. Added contains method
  2. Added checks to avoid double add

@lxfind lxfind merged commit 89fca8a into main Feb 8, 2022
@lxfind lxfind deleted the add-collection branch February 8, 2022 04:46
mwtian pushed a commit that referenced this pull request Sep 12, 2022
* Add skeleton for new epoch endpoint

* add method to extract and verify public key from request similar to proposer
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
* Add skeleton for new epoch endpoint

* add method to extract and verify public key from request similar to proposer
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