Feature/basic move call arg linter #508

oxade · 2022-02-21T17:47:54Z

Related to: #451
WIP: simplify serialization if possible

Why we need this:

Folks have expressed difficulties in calling Move functions due to the various nuances.

What this is:

Simplifies calling from JSON or other loose-typed formats
Primary aim is to avoid having to specify args like this for example

Example fn signature:

    public fun create_player(
        player_name: vector<u8>, farm_name: vector<u8>, farm_img_id: u64, total_monster_slots: u64, ctx: &mut TxContext
    )

    "pure_args": [
        "x\"abcdef\"",
        "x\"1234\"", 
        "1u64",
        "8u64",
        "x\"a295ff8e49bb3f934db9ba68237a9dd0bf8c7154c47d7eedd2d0ddaef3d09b71\""
    ]

but instead use something like this:

    "pure_args": [
        "0xabcdef",
        [1,2,3,4,5], 
        1,
        8,
        "0xa295ff8e49bb3f934db9ba68237a9dd0bf8c7154c47d7eedd2d0ddaef3d09b71"
    ]

Supports nested vectors, and other features. See tests
Out input format is confusing

What this is not:

It is not a full linter currently and it does not check some deeper nuances of callable Move functions. If we need more functionality, we can add them
This is not pre-execution, but it can be extended to be
This does not make it impossible to make mistakes. For example we know Sui addresses are fixed length, but this library has no way to know what is an address based on the function signature since addresses are simple called Vec.

Next steps (future PR):

Implement is_primitive in move_binary_format::Type:: rather than here
Deeper checks for object args
Check TypeTags
Use with REST API
This logic should be shared with adapter ideally

sblackshear

Love seeing this, will be a huge UX improvement! And really like the way you have made it work for both the CLI and the REST API's.

Suggested changes I feel strongly about:

Sharing function signature validation code and function resolution code with the adapter, even if it requires doing some refactoring here or in another PR that we land first. The validation code especially is very intricate, and we're going to be making some changes there. I want to make sure we only have to maintain one copy of it.
Refactoring the BCS conversion to remove the vector length prefix hack
Making some strides toward a SuiJSON type (or similar) that represents the input language for Sui API users + can be specified later.

sui/src/utils.rs

sblackshear · 2022-02-22T01:30:08Z

sui/src/utils.rs

+const HEX_PREFIX: &str = "0x";
+
+#[derive(Debug)]
+pub struct MoveFunctionComponents {


Suggested change

pub struct MoveFunctionComponents {

pub struct MoveFunctionCall {

sblackshear · 2022-02-22T01:33:03Z

sui/src/utils.rs

+    let function_signature = get_expected_fn_signature(package, module.clone(), function.clone())?;
+
+    // Now we check that the args are proper
+    // More checks are done in the adapter


CC @awelc to see if there's a way to reuse the adapter's implementation of the checks here. E.g., it looks like adapter::resolve_and_type_check could probably be refactored to separate the function signature checks from the arg value checks. Would be nice to have only one copy of this logic if we can swing it!

sblackshear · 2022-02-22T01:40:18Z

sui/src/utils.rs

+    pub function: Identifier,
+    pub object_args: Vec<ObjectID>,
+    pub pure_args_serialized: Vec<Vec<u8>>,
+    // TODO: add type args checks


Is there a blocker that makes it difficult to implement this?

sblackshear · 2022-02-22T01:45:08Z

sui/src/utils.rs

+
+        // We can encode U8 Vector as string in 2 ways
+        // 1. If it starts with 0x, we treat it as hex strings, where each pair is a byte
+        // 2. If it does not start with 0x, we treat each character as an ASCII endoced byte


Suggested change

// 2. If it does not start with 0x, we treat each character as an ASCII endoced byte

// 2. If it does not start with 0x, we treat each character as an ASCII encoded byte

sblackshear · 2022-02-22T02:13:04Z

sui/src/utils.rs

+                // If starts with 0x, treat as hex vector?
+                hex::decode(s.trim_start_matches(HEX_PREFIX))?
+            } else {
+                check_if_ascii(s.to_string())?;


We don't know that a Move call is going to turn a string into ASCII. It might turn it into UTF8, or (very commonly for immutable content like NFT metadata) not even look at the bytes. Thus, I think we shouldn't assume or enforce that the string is ASCII here--just converting it to bytes with no validation is fine/preferred.

@sblackshear does this not conflict with (1b) in your requirements here? #451
It says to Fail if the string contains non-ASCII bytes.

It does--apologies for that! Please ignore the advice of past me; did not think this all the way through.

sblackshear · 2022-02-22T02:20:09Z

sui/src/utils.rs

+        let transformed = match arg {
+            Value::String(s) => {
+                let mut s = s.trim().to_lowercase();
+                if !s.starts_with(HEX_PREFIX) {


I think it would be simpler to pick one of "always need the prefix" or "never need the prefix" instead of supporting both.

Because object ID's can also appear in the pure args (e.g., if the sig is foo(o: Object, object_id: vector<u8>)) "always need the prefix" seems a bit clearer/more consistent? It seems simple enough to explain that

every time you write an object ID, you need a 0x prefix

if you want to write some hex encoded bytes that are not an object ID, you also need a 0x prefix

sblackshear · 2022-02-22T02:22:38Z

sui/src/utils.rs

+}
+
+/// Get the expected function signature from the package, module, and identifier
+fn get_expected_fn_signature(


I think this should be in impl MovePackage (or at least in object.rs) rather than here--finding a function in a package will be very common.

Seems like another opportunity for code sharing with the adapter as well.

Agreed.
impl MovePackage requires making MovePackage a NewType (currently its aliasing BTreeMap), which I want to do, but expands the scope of this PR.
For now I can just make this a function in object.rs and improve MovePackage later

sblackshear · 2022-02-22T02:23:12Z

sui/src/utils.rs

+fn resolve_pure_arg(curr_val: &Value, expected_type: &Type) -> Result<Vec<u8>> {
+    if !is_primitive(expected_type) {
+        return Err(anyhow!(
+            "Unexpected arg type {:?}. Only primitive types are allowed",


Suggested change

"Unexpected arg type {:?}. Only primitive types are allowed",

"Unexpected arg type {}. Only primitive types are allowed",

This will give you Display printing, which is much nicer. As a general principle, if we're returning an error that users will see (not just ones that Sui devs will see), we probably want to implement the Display trait and use display printing so they don't have to see the ugliness of Rust debug printing.

sblackshear · 2022-02-22T02:26:24Z

sui/src/utils.rs

+    pub module: Identifier,
+    pub function: Identifier,
+    pub object_args: Vec<ObjectID>,
+    pub pure_args_serialized: Vec<Vec<u8>>,


Suggested change

pub pure_args_serialized: Vec<Vec<u8>>,

pub pure_args_bcs: Vec<Vec<u8>>,

oxade · 2022-02-22T19:08:40Z

@sblackshear based on our discussions here I think I should start with the following (in no order) before merging this PR, otherwise I'll have to come back around and delete code:

Separate/repurpose/share type and function check logic from adapter
impl MovePackage
Disambiguate/remove utils.rs by creating specific modules

With these changes, adding the parser should be cleaner.

oxade · 2022-02-25T19:55:56Z

Closing in favor of
#566

oxade added 9 commits February 18, 2022 22:15

Add basic args linter

8f386a6

Added more tests

8821927

Merge branch 'main' into feature/basic-move-call-arg-linter

fb77ffd

Added comments

4365b4f

Added comments

7447e19

Licence header

b7e04e7

Merge branch 'main' into feature/basic-move-call-arg-linter

8837771

Synced to main

496904f

clippy updates

3c33a08

sblackshear requested changes Feb 22, 2022

View reviewed changes

This was referenced Feb 22, 2022

Remove utils module #522

Merged

Lint args inputs #566

Merged

oxade closed this Feb 25, 2022

huitseeker deleted the feature/basic-move-call-arg-linter branch April 22, 2022 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/basic move call arg linter #508

Feature/basic move call arg linter #508

oxade commented Feb 21, 2022 •

edited

Loading

sblackshear left a comment

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

oxade Feb 22, 2022

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

oxade Feb 22, 2022 •

edited

Loading

sblackshear Feb 22, 2022

sblackshear Feb 22, 2022

oxade commented Feb 22, 2022 •

edited

Loading

oxade commented Feb 25, 2022

	pub struct MoveFunctionComponents {
	pub struct MoveFunctionCall {

	// 2. If it does not start with 0x, we treat each character as an ASCII endoced byte
	// 2. If it does not start with 0x, we treat each character as an ASCII encoded byte

	"Unexpected arg type {:?}. Only primitive types are allowed",
	"Unexpected arg type {}. Only primitive types are allowed",

	pub pure_args_serialized: Vec<Vec<u8>>,
	pub pure_args_bcs: Vec<Vec<u8>>,

Feature/basic move call arg linter #508

Feature/basic move call arg linter #508

Conversation

oxade commented Feb 21, 2022 • edited Loading

sblackshear 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

oxade Feb 22, 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 commented Feb 22, 2022 • edited Loading

oxade commented Feb 25, 2022

oxade commented Feb 21, 2022 •

edited

Loading

oxade Feb 22, 2022 •

edited

Loading

oxade commented Feb 22, 2022 •

edited

Loading