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

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

Closed
wants to merge 2 commits into from

Conversation

alnoki
Copy link
Contributor

@alnoki alnoki commented May 4, 2023

@0xjinn @gregnazario @davidiw

Summary of changes

  • Add JSON file input support for Move public entry, script, and view functions
  • Abstract out assorted TryInto and TryFrom implementations in accordance with DRY design principles
  • Add docs site tutorial with representative Move example package

JSON syntax

As described in the docs site tutorial preview for this PR, a public entry function can now be invoked with arguments passed from either the CLI or from a JSON file:

Arguments passed via CLI

aptos move run \
    --function-id <test_account>::cli_args::set_vals \
    --private-key-file <test_account.key> \
    --type-args \
        0x1::account::Account \
        0x1::chain_id::ChainId \
    --args \
        u8:123 \
        "bool:[false, true, false, false]" \
        'address:[["0xace", "0xbee"], ["0xcad"], []]'

Arguments passed via JSON file

Command:

aptos move run \
    --private-key-file <test_account.key> \
    --json-file entry_function_arguments.json

JSON file:

{
    "function_id": "<test_account>::cli_args::set_vals",
    "type_args": [
        "0x1::account::Account",
        "0x1::chain_id::ChainId"
    ],
    "args": [
        {
            "arg_type": "u8",
            "arg_value": 123
        },
        {
            "arg_type": "bool",
            "arg_value": [false, true, false, false]
        },
        {
            "arg_type": "address",
            "arg_value": [
                [
                    "0xace",
                    "0xbee"
                ],
                [
                    "0xcad"
                ],
                []
            ]
        }
    ]
}

The docs site tutorial also includes examples for view functions and script functions.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

lots of good things here, one thing I am curious about...

Since this can already be written in a shell script, what is the additional ROI of adding another layering here? I'd be curious your use case / motivation. As this adds additional ways of interacting, it increases the scope of support 😬

@alnoki
Copy link
Contributor Author

alnoki commented May 7, 2023

lots of good things here, one thing I am curious about...

Since this can already be written in a shell script, what is the additional ROI of adding another layering here? I'd be curious your use case / motivation. As this adds additional ways of interacting, it increases the scope of support 😬

@davidiw a couple thoughts:

  1. JSON file support from this PR enables modular isolation of Move-specific arguments, e.g. so that people can share JSON files on GitHub or similar without having to specify keyfile vs private key, gas limit options etc. This will enable repeatability especially when coordinating specific transaction calls from different parties (like in a multisig).
  2. It will be easier for bespoke tooling solutions like project-specific Python/Rust to output Move calls to JSON, which has ubiquitous library support across languages, than it would be to output to shell scripts (especially given that the JSON spec in the current PR is transaction-option-agnostic per note 1).
  3. The JSON parsing tools establishe a composable infrastructure within typical workflows that can be easily adapted for additional use cases outside of aptos move function invocation support, for example:
  4. A primary goal for JSON file support in this manner, per [Feature Request][CLI] JSON input file support #7709, is the verification of multisig operations, in particular for comparing on-chain payloads/hashes versus a locally-generated value. Basically once someone has proposed a function invocation in a v2 multisig via the aptos CLI, there is not a straightforward method for a non-proposer signatory to validate the contents of the proposal they want to vote on. Here the idea would be to generate a JSON file with the payload and hash corresponding to an entry function call, such that it can be used as an input to verify the on-chain proposal contents.
  5. In particular there is the special case of publishing code via a multisig, where a JSON file per note 4 could be generated from local compilation of a Move package. Basically the workflow would be that a signatory proposes publication of a package from a specific commit, then the rest of the signatories in the multisig compile the package locally and derive the corresponding payload/hash JSON output file. This file could be checked against the on-chain proposal so everyone can verify what is actually in transaction purgatory. Essentially this would allow for more secure and trustless multisig operations.

Your thoughts?

@gregnazario
Copy link
Contributor

lots of good things here, one thing I am curious about...
Since this can already be written in a shell script, what is the additional ROI of adding another layering here? I'd be curious your use case / motivation. As this adds additional ways of interacting, it increases the scope of support 😬

@davidiw a couple thoughts:

  1. JSON file support from this PR enables modular isolation of Move-specific arguments, e.g. so that people can share JSON files on GitHub or similar without having to specify keyfile vs private key, gas limit options etc. This will enable repeatability especially when coordinating specific transaction calls from different parties (like in a multisig).
  2. It will be easier for bespoke tooling solutions like project-specific Python/Rust to output Move calls to JSON, which has ubiquitous library support across languages, than it would be to output to shell scripts (especially given that the JSON spec in the current PR is transaction-option-agnostic per note 1).
  3. The JSON parsing tools establishe a composable infrastructure within typical workflows that can be easily adapted for additional use cases outside of aptos move function invocation support, for example:
  4. A primary goal for JSON file support in this manner, per [Feature Request][CLI] JSON input file support #7709, is the verification of multisig operations, in particular for comparing on-chain payloads/hashes versus a locally-generated value. Basically once someone has proposed a function invocation in a v2 multisig via the aptos CLI, there is not a straightforward method for a non-proposer signatory to validate the contents of the proposal they want to vote on. Here the idea would be to generate a JSON file with the payload and hash corresponding to an entry function call, such that it can be used as an input to verify the on-chain proposal contents.
  5. In particular there is the special case of publishing code via a multisig, where a JSON file per note 4 could be generated from local compilation of a Move package. Basically the workflow would be that a signatory proposes publication of a package from a specific commit, then the rest of the signatories in the multisig compile the package locally and derive the corresponding payload/hash JSON output file. This file could be checked against the on-chain proposal so everyone can verify what is actually in transaction purgatory. Essentially this would allow for more secure and trustless multisig operations.

Your thoughts?

This was common in the AWS CLI as well. The CLI allowed JSON input, but would also take a whole JSON file that defined all the inputs. Usually providing a JSON input into the CLI even if scripted tends to be messy around escaping.

It is a little unfortunate that full JSON is a very verbose path, and this provides yet another level of support needed for it.

@gregnazario gregnazario requested a review from 0xmigo May 10, 2023 01:12
@alnoki
Copy link
Contributor Author

alnoki commented May 15, 2023

@0xjinn have you been able to successfully run the new functionality using the docs site tutorial preview for this PR?

@0xmigo
Copy link
Contributor

0xmigo commented May 16, 2023

@0xjinn have you been able to successfully run the new functionality using the docs site tutorial preview for this PR?

Tested from my side using both --private-key and --private-key-file. Confirmed working on my end.

@alnoki Shall we include the testing json file in the cli_args example directory?

Use literal file include for maintainability

Update pnpm package spec for literal file include

Update dictionary for literal file include

See aptos-labs#8054 (comment)
@alnoki
Copy link
Contributor Author

alnoki commented May 16, 2023

@0xjinn have you been able to successfully run the new functionality using the docs site tutorial preview for this PR?

Tested from my side using both --private-key and --private-key-file. Confirmed working on my end.

@0xjinn Nice, thanks for confirming both styles.

@alnoki Shall we include the testing json file in the cli_args example directory?

28cb64d adds JSON files to the example directory per your request.

Is there anything else needed to land this?

Copy link
Contributor

@0xmigo 0xmigo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Will let @gregnazario and @davidiw to take a final look on this.

@alnoki
Copy link
Contributor Author

alnoki commented May 24, 2023

@0xjinn #8346 extends this PR with additional multisig tooling and a comprehensive multisig governance tutorial

Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

I like the idea, particularly of JSON input from a file. Where I'm less convinced is using JSON for args directly on the CLI, but I don't see the harm of landing this support alongside the existing parsing logic. Saying that, I notice that with this CLI the old way of providing vector arguments no longer works.

Before:

$ aptos move run --function-id 0x1::cli_args::set_vals --args u8:123 'vector<address>:0x1,0x2'
{
  "Error": "Simulation failed with status: Transaction Executed and Committed with Error LINKER_ERROR"
}

After:

$ ~/a/core/target/debug/aptos move run --function-id 0x1::cli_args::set_vals --args u8:123 'vector<address>:0x1,0x2'
error: Invalid value "vector<address>:0x1,0x2" for '--args <ARGS>...': Invalid arguments: Invalid arg type 'vector<address>'.  Must be one of: ['address','bool','hex','string','u8','u16','u32','u64','u128','u256','raw']

For more information try --help

In my opinion we can't land such a breaking change, the old behavior would have to continue to work in addition to the new behavior.

@alnoki
Copy link
Contributor Author

alnoki commented May 25, 2023

I like the idea, particularly of JSON input from a file. Where I'm less convinced is using JSON for args directly on the CLI, but I don't see the harm of landing this support alongside the existing parsing logic. Saying that, I notice that with this CLI the old way of providing vector arguments no longer works.

Before:

$ aptos move run --function-id 0x1::cli_args::set_vals --args u8:123 'vector<address>:0x1,0x2'
{
  "Error": "Simulation failed with status: Transaction Executed and Committed with Error LINKER_ERROR"
}

After:

$ ~/a/core/target/debug/aptos move run --function-id 0x1::cli_args::set_vals --args u8:123 'vector<address>:0x1,0x2'
error: Invalid value "vector<address>:0x1,0x2" for '--args <ARGS>...': Invalid arguments: Invalid arg type 'vector<address>'.  Must be one of: ['address','bool','hex','string','u8','u16','u32','u64','u128','u256','raw']

For more information try --help

In my opinion we can't land such a breaking change, the old behavior would have to continue to work in addition to the new behavior.

@banool note that the breaking syntax is necessary to support multi-nested vectors and has already been merged per #7790

This PR just extends the functionality with JSON input files

@banool
Copy link
Contributor

banool commented May 25, 2023

Hummm okay I see, all good then! Looks like the docs have been updated correctly too.

@alnoki alnoki closed this May 27, 2023
@alnoki alnoki reopened this May 27, 2023
@alnoki
Copy link
Contributor Author

alnoki commented May 27, 2023

Closed by #8346

@alnoki alnoki closed this May 27, 2023
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.

5 participants