[CLI] Add nested vector arg support, docs (#7709) #7790

alnoki · 2023-04-15T15:22:04Z

Addresses #7709

Summary of changes

Implement recursive vector parsing for ArgWithType
Abstract out ArgWithTypeVec field in accordance with DRY design principles
Add Move example with multiple nested vector permutations
Add CLI docs for running example Move code

Vector syntax

Previously, the CLI was restricted to vector arguments with a maximum depth of 1, per the below syntax:

vector<u8>:2,5,3

Notably, this schema does not support doubly nested vectors of the form vector<vector<u8>>, as there is no way to delimit inner vectors.

However, since Move vectors must all use the same inner type, the new schema employs JSON array syntax of the form:

"u8:[[1, 2], [], [4, 5, 6]]"

Note here the use of quotes to escape spaces and square braces, as required for typical shell interpreters like zsh.

Hence as described in the docs example from this PR, a Move function can be invoked via syntax of the form:

aptos move run \
    --function-id <deploy_address>::cli_args::set_vals \
    --private-key-file <keyfile> \
    --args \
        u8:123 \
        "bool:[false, true, false, false]" \
        'address:[["0xace", "0xbee"], ["0xcad"], ["0xdee"], []]'

Additional example

See also testnet transaction 499639189 for a public entry function invocation with a vector<vector<u64>> argument, run from the CLI via:

aptos move run \
    --function-id $testnet_address::incentives::update_incentives \
    --private-key-file tesnet_account.key \
    --type-args 0x1::aptos_coin::AptosCoin \
    --args \
        u64:1 \
        u64:1 \
        u64:1 \
        u64:2000 \
        "u64:[[10000, 0, 7], [8333, 1, 6], [7692, 2, 5], [7143, 3, 4], [6667, 4, 3], [6250, 5, 2], [5882, 6, 1]]"

Local checks

The following commands were run in the below directories:

aptos-core/

pre-commit run --all-files
scripts/rust_lint.sh

aptos-core/crates/aptos/
```
cargo test
```
aptos-core/developer-docs-site/
```
pnpm spellcheck
pnpm lint
```

gregnazario

Pretty nice, I'll have to test it out and all, been very busy. Noting that this is a breaking change. @0xjinn can take a look as well

aptos-move/move-examples/cli_args/Move.toml

gregnazario · 2023-04-20T21:35:32Z

crates/aptos/src/move_tool/mod.rs

+    /// Vectors may be specified using JSON array literal syntax (you may need to escape this with
+    /// quotes based on your shell interpreter)
+    ///
+    /// Example: `address:0x1 bool:true u8:0 u256:1234 "bool:[true, false]" 'address:[["0xace", "0xbee"], []]'`


This is pretty clean, might have to make a version bump and note that it's a breaking change in the CHANGELOG.md

Can do, I'm glad to add these unless you would rather handle it.

gregnazario · 2023-04-20T21:36:27Z

crates/aptos/src/move_tool/mod.rs

+/// Does not support string arguments that contain the following characters:
+///
+/// * `,`
+/// * `[`
+/// * `]`


We'll need to add escaping soon

alnoki · 2023-04-21T00:23:18Z

@gregnazario I've addressed your comments above and resolved merge conflicts, so this should be ready to land!

Let me know if you or @0xjinn have anything to discuss

gregnazario

I'm going to approve this @0xjinn can you take another look at this.

developer-docs-site/docs/tools/aptos-cli-tool/use-aptos-cli.md

crates/aptos/src/move_tool/mod.rs

gregnazario · 2023-04-26T18:57:20Z

crates/aptos/src/test/mod.rs

+                arg_vec: ArgWithTypeVec {
+                    args: vec![
+                        ArgWithType::address(self.account_id(operator_index)),
+                        ArgWithType::address(self.account_id(voter_index)),
+                        ArgWithType::u64(amount),
+                        ArgWithType::u64(commission_percentage),
+                        ArgWithType::bytes(vec![]),
+                    ],


We should probably add a unit test for this function now (I feel like I've avoided it too long), that checks the multi-nested BCS, and ensures it can be deserialized as the type expected.

Agreed. I am also working on a PR that will extend this functionality to include JSON input files, and it looks like it'll involve some minor refactors to struct nesting. Once we can get that in perhaps it'll be appropriate to add more unit testing?

0xmigo · 2023-04-26T19:22:30Z

crates/aptos/src/move_tool/mod.rs

+    }
+
+    /// Recursively parse argument JSON into BCS representation.
+    pub fn parse_arg_json(&self, arg: &serde_json::Value) -> CliTypedResult<ArgWithType> {


nit: Would be great if we can unit test and safeguard the parser

Agreed. As I mention in #7790 (comment) I'm working on a separate PR that will extend support to JSON input files as well. Perhaps once both input types are supported an end-to-end test will be in order?

alnoki · 2023-04-26T21:20:42Z

@gregnazario I believe I've addressed all of your comments/suggestions.

@0xjinn I believe I've addressed your proposition.

Is there anything else needed to get an approving review from @0xjinn and land in main?

0xmigo · 2023-04-26T21:37:10Z

I think this looks good on my end. I did manual tested it and ensured the example transaction works.

I can follow up with PR to update the changelog.

cc: @gregnazario for final look

davidiw · 2023-04-30T06:10:56Z

claims it needs to be rebased

alnoki · 2023-05-01T17:08:39Z

claims it needs to be rebased

@davidiw I just merged in main without conflicts, perhaps the notice was due to the time since branching?

Either way @gregnazario this should be ready to land

alnoki · 2023-05-03T22:38:12Z

@davidiw @gregnazario I've further squashed the PR so it reflects as a single commit against main

There still are no merge conflicts, so I'm not sure if there's anything else I can do to massage the diff history

github-actions · 2023-05-04T00:07:25Z

✅ Forge suite `land_blocking` success on `5cedefa0dd673fa9efd6d64c422b35127556169c`

performance benchmark : 5423 TPS, 7303 ms latency, 26500 ms p99 latency,(!) expired 162 out of 2316080 txns
Test Ok

github-actions · 2023-05-04T00:08:42Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `5cedefa0dd673fa9efd6d64c422b35127556169c`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 5cedefa0dd673fa9efd6d64c422b35127556169c (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7857 TPS, 4843 ms latency, 7000 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 5cedefa0dd673fa9efd6d64c422b35127556169c
compatibility::simple-validator-upgrade::single-validator-upgrade : 4916 TPS, 8039 ms latency, 10800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 5cedefa0dd673fa9efd6d64c422b35127556169c
compatibility::simple-validator-upgrade::half-validator-upgrade : 4895 TPS, 8315 ms latency, 11300 ms p99 latency,no expired txns
4. upgrading second batch to new version: 5cedefa0dd673fa9efd6d64c422b35127556169c
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6452 TPS, 5916 ms latency, 10400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 5cedefa0dd673fa9efd6d64c422b35127556169c passed
Test Ok

github-actions · 2023-05-04T00:12:55Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60` ==> `5cedefa0dd673fa9efd6d64c422b35127556169c`

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 5cedefa0dd673fa9efd6d64c422b35127556169c (PR)
Upgrade the nodes to version: 5cedefa0dd673fa9efd6d64c422b35127556169c
framework_upgrade::framework-upgrade::full-framework-upgrade : 6248 TPS, 6267 ms latency, 9900 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 5cedefa0dd673fa9efd6d64c422b35127556169c passed
Test Ok

lightmark · 2023-05-17T22:15:59Z

love this @alnoki

alnoki · 2023-05-18T14:16:53Z

love this @alnoki

@lightmark see also #8054

alnoki requested review from clay-aptos, saharct and gregnazario as code owners April 15, 2023 15:22

alnoki changed the title ~~[CLI] Add nested vector arg support, docs (#7709)~~ [CLI] Add nested vector arg support, docs #7709 Apr 15, 2023

alnoki changed the title ~~[CLI] Add nested vector arg support, docs #7709~~ [CLI] Add nested vector arg support, docs (#7709) Apr 15, 2023

alnoki mentioned this pull request Apr 15, 2023

[CLI] Fix empty vector, and cleanup vec<u8> parsing #7789

Closed

gregnazario reviewed Apr 20, 2023

View reviewed changes

gregnazario approved these changes Apr 26, 2023

View reviewed changes

0xmigo reviewed Apr 26, 2023

View reviewed changes

0xmigo approved these changes Apr 26, 2023

View reviewed changes

[CLI] Add nested vector arg support, docs (aptos-labs#7709)

5cedefa

alnoki force-pushed the nested-vectors-cli branch from 2ca933a to 5cedefa Compare May 3, 2023 22:36

gregnazario enabled auto-merge (squash) May 3, 2023 23:34

This comment has been minimized.

Sign in to view

gregnazario merged commit c2af641 into aptos-labs:main May 4, 2023

alnoki mentioned this pull request May 25, 2023

[CLI] Add JSON input file support, docs (#7709) #8054

Closed

alnoki mentioned this pull request May 27, 2023

Extend multisig tutorial/SDK with automation tool #7297

Closed

0xmigo mentioned this pull request Jul 16, 2023

[CLI] fix u64 to u256 input args #9111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI] Add nested vector arg support, docs (#7709) #7790

[CLI] Add nested vector arg support, docs (#7709) #7790

alnoki commented Apr 15, 2023 •

edited

Loading

gregnazario left a comment

gregnazario Apr 20, 2023

alnoki Apr 21, 2023

gregnazario Apr 20, 2023

alnoki Apr 21, 2023

alnoki commented Apr 21, 2023

gregnazario left a comment

gregnazario Apr 26, 2023

alnoki Apr 26, 2023

0xmigo Apr 26, 2023

alnoki Apr 26, 2023

alnoki commented Apr 26, 2023

0xmigo commented Apr 26, 2023

davidiw commented Apr 30, 2023

alnoki commented May 1, 2023

alnoki commented May 3, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented May 4, 2023

github-actions bot commented May 4, 2023

github-actions bot commented May 4, 2023

lightmark commented May 17, 2023

alnoki commented May 18, 2023

[CLI] Add nested vector arg support, docs (#7709) #7790

[CLI] Add nested vector arg support, docs (#7709) #7790

Conversation

alnoki commented Apr 15, 2023 • edited Loading

Summary of changes

Vector syntax

Additional example

Local checks

gregnazario 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

alnoki commented Apr 21, 2023

gregnazario 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

alnoki commented Apr 26, 2023

0xmigo commented Apr 26, 2023

davidiw commented Apr 30, 2023

alnoki commented May 1, 2023

alnoki commented May 3, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented May 4, 2023

✅ Forge suite land_blocking success on 5cedefa0dd673fa9efd6d64c422b35127556169c

github-actions bot commented May 4, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 5cedefa0dd673fa9efd6d64c422b35127556169c

github-actions bot commented May 4, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 5cedefa0dd673fa9efd6d64c422b35127556169c

lightmark commented May 17, 2023

alnoki commented May 18, 2023

alnoki commented Apr 15, 2023 •

edited

Loading

✅ Forge suite `land_blocking` success on `5cedefa0dd673fa9efd6d64c422b35127556169c`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `5cedefa0dd673fa9efd6d64c422b35127556169c`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60` ==> `5cedefa0dd673fa9efd6d64c422b35127556169c`