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 multisig governance tooling, docs #8346

Merged
merged 8 commits into from
May 27, 2023

Conversation

alnoki
Copy link
Contributor

@alnoki alnoki commented May 24, 2023

@davidiw @lightmark @movekevin @0xjinn

Closes #7709

Linked issue and PR

This PR completes all milestones listed in #7709, adding a single squashed commit on top of PR #8054 (which has a single squashed commit and a second commit addressing a request from a reviewer, who subsequently provided an approving review).

Summary of changes

In addition to the changes from #8054, this PR adds:

  • Support for passing an explicit payload to aptos multisig execute, marked as a TODO by @movekevin in 349cda9
  • An aptos multisig check-transaction subcommand for verifying on-chain multisig transaction proposals
  • A --json-output-file option for aptos move publish that creates a JSON file with publication transaction data, which can be passed to aptos multisig check-transaction to verify on-chain multisig publication transaction propsals
  • An extensive docs website tutorial for multsig governance (preview here), including payload and hash-only transactions, for both package publication and entry function invocation, with assorted multisig view function examples

Implications

By closing #7709, this PR adds functionality required for trustless multisig goverance via the CLI, using practical workflows: developers may now pass multisig governance entry function arguments via JSON input files, check expected transaction data against on-chain hashes or payloads, and publish packages while verifying data along the way.

In particular, this functionality allows for a publication proposer to provide other multisig owners with a git commit for a particular Move package, such that the corresponding publication payload hash can be checked against a locally-compiled bytecode record (via the json-output-file command for aptos multsig publish, combined with the aptos multisig check-transaction command): this is analagous to evaluating a checksum against a locally compiled binary, as is typical in open-source software.

One the package is published, multisig owners can then conduct governance operations by posting a transaction proposal in JSON format in a public forum, and can verify that the JSON file evaluates to an on-chain multisig payload hash.

Hence with the new functionality, multisig-based DAOs can now conduct protocol governance "out in the open" with standard and official tooling.

Select command examples from tutorial

See full tutorial preview here

Creating a publication JSON file:

aptos move publish \
    --named-addresses test_account=$multisig_addr \
    --json-output-file publication.json

Creating a hash-only publication transaction:

aptos multisig create-transaction \
    --multisig-address $multisig_addr \
    --json-file publication.json \
    --hash-only \
    --private-key-file ace.key

Creating a full-payload governance transaction:

aptos multisig create-transaction \
    --multisig-address $multisig_addr \
    --function-id $multisig_addr::cli_args::set_vals \
    --type-args \
        0x1::account::Account \
        0x1::chain_id::ChainId \
    --args \
        u8:123 \
        "bool:[false, true, false, false]" \
        'address:[["0xace", "0xbee"], ["0xcad"], []]' \
    --private-key-file bee.key

Checking against an on-chain payload hash:

aptos multisig check-transaction \
    --multisig-address $multisig_addr \
    --json-file publication.json \
    --sequence-number 1

alnoki added 3 commits May 6, 2023 07:45
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 alnoki changed the title Cli multisig governance [CLI] Add multisig governance tooling, docs May 24, 2023
aptos-move/move-examples/cli_args/Move.toml Outdated Show resolved Hide resolved
aptos-move/move-examples/cli_args/scripts/set_vals.move Outdated Show resolved Hide resolved
aptos-move/move-examples/cli_args/sources/cli_args.move Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
@alnoki
Copy link
Contributor Author

alnoki commented May 25, 2023

@movekevin a4bbabb resolves all of your review comments

@movekevin movekevin requested a review from banool May 25, 2023 08:03
alnoki added a commit to alnoki/aptos-core that referenced this pull request May 25, 2023
@alnoki
Copy link
Contributor Author

alnoki commented May 25, 2023

@movekevin a6d6be7 resolves your nit review comment

@banool
Copy link
Contributor

banool commented May 25, 2023

@alnoki I'm a bit confused, are you planning on landing #8054 first and then landing this PR? Or will you just land this one?

@alnoki
Copy link
Contributor Author

alnoki commented May 25, 2023

@alnoki I'm a bit confused, are you planning on landing #8054 first and then landing this PR? Or will you just land this one?

@banool Based on discussions with @movekevin it looks like the approach here is going to be land this and then just close out #8054

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.

This is awesome. I'm only the requesting changes for some small things like some clap stuff, using Lazy, etc. The rest I want to see what you think.

Some more feedback beyond what I left in the code:

How about verify-proposal instead of check-transaction? The latter feels quite vague.

In the "Multisig governance" section in move/move-on-aptos/cli it would be good to mention which multisig we're talking about.

I'll save further docs comments for after this round of feedback.

crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved
crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved
crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved
crates/aptos/src/move_tool/mod.rs Outdated Show resolved Hide resolved
crates/aptos/src/common/types.rs Show resolved Hide resolved
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.

Also could you please update crates/aptos/CHANGELOG.md?

In addition to these changes, I'd like to see something that makes it obvious that vector args have changed, like:

- **Breaking change**: You can no longer pass in a vector like this: `--arg vector<address>:0x1,0x2`, you must do it like this: `--arg 'address:["0x1", "0x2"]'`

@alnoki
Copy link
Contributor Author

alnoki commented May 26, 2023

This is awesome. I'm only the requesting changes for some small things like some clap stuff, using Lazy, etc. The rest I want to see what you think.

Some more feedback beyond what I left in the code:

How about verify-proposal instead of check-transaction? The latter feels quite vague.

In the "Multisig governance" section in move/move-on-aptos/cli it would be good to mention which multisig we're talking about.

I'll save further docs comments for after this round of feedback.

Also could you please update crates/aptos/CHANGELOG.md?

In addition to these changes, I'd like to see something that makes it obvious that vector args have changed, like:

- **Breaking change**: You can no longer pass in a vector like this: `--arg vector<address>:0x1,0x2`, you must do it like this: `--arg 'address:["0x1", "0x2"]'`

@banool I believe 6aa7447 resolves all items in the two above quote reply blocks

@alnoki
Copy link
Contributor Author

alnoki commented May 26, 2023

@banool I believe I have either resolved or addressed all of your review comments via a combination of 6aa7447 and my above responses

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.

You don't have to do it in this PR but if you're interested, it'd be helpful if you could add end to end tests for this new functionality in crates/aptos/e2e. Whether you do it now or one of us does it later, it'd be helpful if you could open an issue for this.

I'd appreciate if you could resolve that issue with the JSON response in the error case (if it indeed works how I expect right now). If you think it's too much of an overhaul let me know and we can help out.

crates/aptos/CHANGELOG.md Show resolved Hide resolved
crates/aptos/src/account/multisig_account.rs Outdated Show resolved Hide resolved
@alnoki
Copy link
Contributor Author

alnoki commented May 26, 2023

You don't have to do it in this PR but if you're interested, it'd be helpful if you could add end to end tests for this new functionality in crates/aptos/e2e. Whether you do it now or one of us does it later, it'd be helpful if you could open an issue for this.

I'd appreciate if you could resolve that issue with the JSON response in the error case (if it indeed works how I expect right now). If you think it's too much of an overhaul let me know and we can help out.

@banool #8392 opens an issue per your request

See #8346 (comment) re: JSON response

Your thoughts?

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.

Awesome thanks a lot for all your work on this PR and sticking with me with these changes I've requested, I'd say we're good to go!

@alnoki
Copy link
Contributor Author

alnoki commented May 26, 2023

Awesome thanks a lot for all your work on this PR and sticking with me with these changes I've requested, I'd say we're good to go!

@banool @movekevin thanks for all of the collaboration on this!

It looks like there are two approving reviews, so will one of you please land this into main?

@movekevin movekevin enabled auto-merge (squash) May 26, 2023 18:10
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9da54751cda49c265c4f5fd061fa3a17101c8020

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9da54751cda49c265c4f5fd061fa3a17101c8020 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 10016 TPS, 3721 ms latency, 5800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 9da54751cda49c265c4f5fd061fa3a17101c8020
compatibility::simple-validator-upgrade::single-validator-upgrade : 6198 TPS, 6509 ms latency, 8300 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 9da54751cda49c265c4f5fd061fa3a17101c8020
compatibility::simple-validator-upgrade::half-validator-upgrade : 5846 TPS, 6826 ms latency, 8600 ms p99 latency,no expired txns
4. upgrading second batch to new version: 9da54751cda49c265c4f5fd061fa3a17101c8020
compatibility::simple-validator-upgrade::rest-validator-upgrade : 8231 TPS, 4573 ms latency, 7700 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9da54751cda49c265c4f5fd061fa3a17101c8020 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 9da54751cda49c265c4f5fd061fa3a17101c8020

performance benchmark : 5810 TPS, 6823 ms latency, 27100 ms p99 latency,(!) expired 760 out of 2481680 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 9da54751cda49c265c4f5fd061fa3a17101c8020

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

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.

[Feature Request][CLI] JSON input file support
3 participants