[move] Added support for passing vectors of objects to entry functions #4349

awelc · 2022-08-28T19:29:18Z

This PR implements passing vectors of objects to owned functions and resolves #492. One somewhat tricky part to handle was the fact that the adapter was taking advantage of the fact that one object was corresponding to one parameter - checking of various properties of both objects (based on object ID) and parameters (based on their index IDX) was happening in the same spots. This PR is attempting to refactor and re-use this code but it may result in some (minor) computational redundancy. Another somewhat tricky part was how to combine existing objects in to bcs-encoded vector that can be used as an argument - I am not sure if there is a better way than what this PR does, that is hand-encoding vector length and appending all objects to the result.

sblackshear · 2022-08-28T22:38:15Z

crates/sui-types/src/messages.rs

@@ -49,7 +49,8 @@ pub enum CallArg {
    Pure(Vec<u8>),
    // an object
    Object(ObjectArg),
-    // TODO support more than one object (object vector of some sort)
+    // a vector of objects
+    ObjVec(Vec<ObjectArg>),


From an expressivity standpoint, I think what we really want here is Vec<CallArg>--folks have also asked to pass in vectors of addresses, integers, strings, and object ID's. Is there a reason not to opt for this more generic approach?

I understand that this schema can also represent bad heterogenous vectors (e.g., a vector containing a mix of object args and ints), but I think that will be caught in a later phase.

folks have also asked to pass in vectors of addresses, integers, and object ID's. Is there a reason not to opt for this more generic approach?

I did not put that much thought into supporting other types of vectors (yet), but since vectors of integers are already supported actually (I added a test for that in this PR), I thought that perhaps other vectors could be supported the same way, that it is via the Pure variant. The schema using Vec<Box<CallArg>> (I don't think Vec<CallArg> would work) also implies nested vectors, which we probably won't support. In summary, I thought of this PR as exclusively attacking the vector-of-objects problem and doing a necessary refactoring when working on PRs for other types of vectors. as I wasn't 100% sure which way it would go.

Makes sense--I didn't realize pure vectors were already supported!

I do think some kinds nested vectors (e.g., vector<vector<u8>> are important to represent (e.g.) the public keys in a multisig. But arbitrary nested vectors are probably a bit much.

tnowacki · 2022-08-29T21:25:32Z

crates/sui-types/src/messages.rs

+                                    // ObjVec is guaranteed to never contain shared objects
+                                    debug_assert!(false);


Why do we have this restriction?

I assumed that this PR is only meant to provide support for vectors of owned objects. I was not sure if there are any additional implications of supporting shared objects. On the surface there are not, but shared objects support is a bit newer (and a bit more complicated) so I did not want to make too many assumptions, so it would be good to get some advice on that.

oxade · 2022-08-31T21:18:49Z

crates/sui-adapter/src/adapter.rs

-                Some((*id, state_view.read_object(id)?))
+                Some(vec![(*id, state_view.read_object(id)?)])
+            }
+            CallArg::ObjVec(vec) => {


This is not an issue we need to solve now, but we're specializing the CallArg types here and I fear we might need to do it in future for other types.
Not sure but is it even possible to create a general container type that prevents over-specialization of the adapter and sui types?
So we can decouple the inner Move type impl from sui types.
If this is not a real concern, please disregard.

I share your concerns, but I am failing to think of anything that much more general.

I have calmed myself with the following line of reasoning:

We can safely add new variants to this if needed, it will not be a breaking change (just will need to be epoch gated or gated by some similar mechanism)

With that if we have something new in the future, like a map or something, we can do that

Or similarly, if we come up with the supreme general approach, we can add that later while still supporting this special case (we did that with Move args in Diem fwiw. As we did not originally support general BCS arguments, only a few specific hand rolled cases)

We can always interpret this differently in the adapter. Much like CallArg::Object can be used to populate &Obj, &mut Obj and Obj, we can have more than one meaning for ObjVec

With this it feels fine enough to just be specific for vectors right now.

oxade · 2022-08-31T21:45:57Z

@awelc will you also handle exposing this in SuiJson so apps/SDK can use it?
If not, let's create an issue for it so someone can take care of it.

awelc · 2022-08-31T22:10:58Z

@awelc will you also handle exposing this in SuiJson so apps/SDK can use it? If not, let's create an issue for it so someone can take care of it.

I am not going to take care of this in the current PR. If you can, please create the issue (I can look into it to see if I can do it myself, particularly if I get some pointers :-)). I would do it myself but I am traveling for the next two days.

tnowacki

I will need to take a closer look at the adapter code, just wanted to leave some comments. I should be able to take a look at this first thing in the morning!

tnowacki · 2022-09-01T02:24:08Z

crates/sui-adapter/src/adapter.rs

+                if vec.is_empty() {
+                    return None;
+                }


Fine here, but should we ban this in the transaction input checker?

It should be OK to pass an empty vector (of internal object type) to an entry function (I even have a test for this) so I am not sure we should ban it.

Ah sure, I could see it being useful

crates/sui-core/src/unit_tests/data/entry_point_vector/sources/objects_vector.move

tnowacki · 2022-09-01T02:29:40Z

crates/sui-verifier/src/entry_points_verifier.rs

+        return Ok(());
+    }
+    if is_object_vector(view, function_type_args, param)? {


Took me a second staring at this. Maybe just if is_object(view, function_type_args, param)? || is_object_vector(view, function_type_args, param)? ?

I generally really like the if-return style of guards. Just confused me as its immediately followed by an if that "returns"

tnowacki · 2022-09-01T02:31:42Z

crates/sui-core/src/unit_tests/move_integration_tests.rs

@@ -445,6 +447,323 @@ async fn test_object_owning_another_object() {
    assert_eq!(effects.deleted.len(), 2);
 }

+#[tokio::test]
+async fn test_entry_point_vector() {


We also have the sui-adapter-transactional-tests

That might be easier for these, but maybe it's easier to do it programmatically in Rust?
Or is there some other reason you chose for these to be in Rust?

Didn't know how to use vectors there (as we discussed elsewhere) but now that I do, I will add at least some tests there.

tnowacki · 2022-09-01T02:32:35Z

crates/sui-core/src/unit_tests/move_integration_tests.rs

+        matches!(effects.status, ExecutionStatus::Failure { .. }),
+        "{:?}",
+        effects.status


This would be a good one for sui-adapter-transactional-tests, as we would get to see the error here.

OK, I tried adding the tests but trying the vector[object(106) syntax resulted in:

thread '<unnamed>' panicked at 'nested sui objects are not yet supported in args', crates/sui-transactional-test-runner/src/args.rs:92:36

If this is a transactional testing framework (temporary) limitation, my suggestion is to stick with integration tests for now and add the transactional ones (again, it should be quick) once the remaining testing framework kinks are ironed out.

tnowacki · 2022-09-01T18:45:57Z

crates/sui-adapter/src/adapter.rs

+                    // write length of the vector as uleb128 as it is encoded in BCS and then append
+                    // all (already serialized) object content data
+                    let mut res = vec![];
+                    leb128::write::unsigned(&mut res, vec.len() as u64).unwrap();


bit funky, but seems fine for now :)

bit funky, but seems fine for now :)

It is a little, isn't it? :-) Could not think of a better/simpler way, though...

tnowacki · 2022-09-01T18:46:23Z

crates/sui-adapter/src/adapter.rs

+                        element_type = Some(arg_type);
+                        inner_vec_type = Some(param_type);


maybe a debug assert that if there was a previous value there, it should be the same?

This reminded me that I wanted to add corresponding tests but didn't... Also, this should be handled by more than an assertion I think. In fact, we should be checking if each element's type is consistent with vector's signature and report and error (rather than panic) if it's not.

…oblem

tnowacki

We should look into adding transactional test support for vectors of objects for some more tests, but I think this should be good to go for now. Thanks!

tnowacki · 2022-09-06T23:19:19Z

crates/sui-adapter/src/adapter.rs

+                if vec.is_empty() {
+                    return None;
+                }


Ah sure, I could see it being useful

tnowacki · 2022-09-06T23:20:14Z

crates/sui-adapter/src/adapter.rs

+                            &mut by_value_objects,
+                            &mut object_type_map,
+                        )?;
+                        type_check_struct(view, type_args, idx, arg_type, param_type)?;


sblackshear reviewed Aug 28, 2022

View reviewed changes

tnowacki reviewed Aug 29, 2022

View reviewed changes

oxade self-requested a review August 29, 2022 23:36

awelc marked this pull request as ready for review August 31, 2022 21:06

oxade reviewed Aug 31, 2022

View reviewed changes

tnowacki reviewed Sep 1, 2022

View reviewed changes

awelc added 5 commits September 2, 2022 09:06

[move] Added support for passing vectors of objects to entry functions

256b9d0

Adjusted test file template

44af14d

Added more tests and reinstated shared objects support in vectors

696f615

Linting issues fixed

b703cf8

Removed unused function and fixed more linting problems

97aefc4

awelc force-pushed the aw/obj-vector branch from 7b0a63b to 97aefc4 Compare September 2, 2022 07:17

awelc added 3 commits September 2, 2022 09:39

Fixed compilation problems plus a minor refactoring

fb6b50b

Updated dependencies

cd9bde0

Addressed review feedback and fixed a vector element type checking pr…

50dbd30

…oblem

awelc self-assigned this Sep 2, 2022

tnowacki approved these changes Sep 6, 2022

View reviewed changes

awelc merged commit d03bcb1 into main Sep 7, 2022

awelc deleted the aw/obj-vector branch September 7, 2022 08:25

awelc mentioned this pull request Sep 8, 2022

[Move] Added support for object vectors to Sui JSON #4526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move] Added support for passing vectors of objects to entry functions #4349

[move] Added support for passing vectors of objects to entry functions #4349

awelc commented Aug 28, 2022 •

edited

Loading

sblackshear Aug 28, 2022 •

edited

Loading

awelc Aug 28, 2022

sblackshear Aug 29, 2022 •

edited

Loading

tnowacki Aug 29, 2022

awelc Aug 29, 2022

oxade Aug 31, 2022 •

edited

Loading

tnowacki Sep 1, 2022

oxade commented Aug 31, 2022

awelc commented Aug 31, 2022

tnowacki left a comment

tnowacki Sep 1, 2022

awelc Sep 2, 2022

tnowacki Sep 6, 2022

tnowacki Sep 1, 2022

tnowacki Sep 1, 2022

awelc Sep 2, 2022

tnowacki Sep 1, 2022

awelc Sep 2, 2022

awelc Sep 2, 2022

tnowacki Sep 1, 2022

awelc Sep 2, 2022

tnowacki Sep 1, 2022

awelc Sep 2, 2022

tnowacki left a comment

tnowacki Sep 6, 2022

tnowacki Sep 6, 2022

		// ObjVec is guaranteed to never contain shared objects
		debug_assert!(false);

		element_type = Some(arg_type);
		inner_vec_type = Some(param_type);

[move] Added support for passing vectors of objects to entry functions #4349

[move] Added support for passing vectors of objects to entry functions #4349

Conversation

awelc commented Aug 28, 2022 • edited Loading

sblackshear Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxade Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxade commented Aug 31, 2022

awelc commented Aug 31, 2022

tnowacki left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc commented Aug 28, 2022 •

edited

Loading

sblackshear Aug 28, 2022 •

edited

Loading

sblackshear Aug 29, 2022 •

edited

Loading

oxade Aug 31, 2022 •

edited

Loading