dfinity / quill

Governance & ledger toolkit for cold wallets
Apache License 2.0
81 stars 29 forks source link

Add New Command for Submitting Upgrade-Asset-Canister-Proposal Without Bash Script Payload Encoding #243

Open ClankPan opened 2 months ago

ClankPan commented 2 months ago

Hi Dfinity devs, I'm working on KinicDAO. This PR adds a new command to quill sns that allows submitting an upgrade-asset-canister-proposal without payload encoding in a bash script.

Description

I have added a new command file, src/commands/sns/make_commit_proposed_batch_proposal.rs, and modified sns.rs to integrate the new command.

This new command accepts evidence in hex format directly and uses a function ID as an argument. This simplifies the process by eliminating the need for complex Candid encoding in bash scripts, as discussed in this thread.

This PR is related to issue #242.

Testing

We tested the command locally using sns-testing and on our SNS asset canister. The proposal here was submitted using the custom command.

To test the command in your local environment, follow the instructions in issue #242.

Checklist

sesi200 commented 2 months ago

Also, Clippy fails with

  error: useless conversion to the same type: `std::vec::Vec<u8>`
    --> src/commands/sns/make_commit_proposed_batch_proposal.rs:85:19
     |
  85 |         evidence: hex::decode(evidence)?.into(),
     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `hex::decode(evidence)?`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
     = note: `-D clippy::useless-conversion` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(clippy::useless_conversion)]`
ClankPan commented 2 months ago

Hi @sesi200. Thank you for the review. I fixed them.

sesi200 commented 2 months ago

Looking great, thank you very much, @ClankPan!

In the PR template you have the checklist what still needs to be done: documentation, and a small test. Would you mind adding these as well? Then this is ready to merge. For prior art, have a look over here: https://github.com/dfinity/quill/pull/171/files, files docs/cli-reference/quill-neuron-manage.md, tests/commands/neuron-manage-disburse-somewhat-to-someone.sh, and tests/outputs/neuron-manage-disburse-somewhat-to-someone.txt. Should be pretty straightforward

sesi200 commented 2 months ago

CI complaints (not sure if you can see it as an external contributor):

cargo fmt -- --check

Diff in /home/runner/work/quill/quill/src/commands/sns/make_commit_proposed_batch_proposal.rs at line 12:
 use candid::Encode;
 use clap::Parser;
 use ic_sns_governance::pb::v1::{
-    manage_neuron, proposal, ExecuteGenericNervousSystemFunction, ManageNeuron, Proposal
+    manage_neuron, proposal, ExecuteGenericNervousSystemFunction, ManageNeuron, Proposal,
 };

 use super::{ParsedSnsNeuron, SnsCanisterIds};
Diff in /home/runner/work/quill/quill/src/commands/sns/make_commit_proposed_batch_proposal.rs at line 59:

 #[derive(candid::CandidType, candid::Deserialize)]
 pub struct CommitProposedBatchArguments {
-  pub batch_id: u128,
-  pub evidence: Vec<u8>,
+    pub batch_id: u128,
+    pub evidence: Vec<u8>,
 }

 pub fn exec(
Diff in /home/runner/work/quill/quill/src/commands/sns/make_commit_proposed_batch_proposal.rs at line 76:
         summary_path,
         function_id,
         evidence,
-        batch_id
+        batch_id,
     } = opts;

     let commit_proposed_batch_arguments = CommitProposedBatchArguments {
Diff in /home/runner/work/quill/quill/src/commands/sns/make_commit_proposed_batch_proposal.rs at line 102:
             ExecuteGenericNervousSystemFunction {
                 function_id,
                 payload,
-            }
+            },
         )),
     };

Diff in /home/runner/work/quill/quill/src/commands/sns/make_commit_proposed_batch_proposal.rs at line 1[25](https://github.com/dfinity/quill/actions/runs/10813866616/job/30038408701?pr=243#step:6:26):

     Ok(vec![msg])
 }
+
Diff in /home/runner/work/quill/quill/src/commands/sns.rs at line 25:
 mod get_sale_participation;
 mod get_swap_refund;
 mod list_deployed_snses;
+mod make_commit_proposed_batch_proposal;
 mod make_proposal;
 mod make_upgrade_canister_proposal;
-mod make_commit_proposed_batch_proposal;
 mod neuron_id;
 mod neuron_permission;
 mod new_sale_ticket;
Diff in /home/runner/work/quill/quill/src/commands/sns.rs at line 72:
     ListDeployedSnses(list_deployed_snses::ListDeployedSnsesOpts),
     MakeProposal(make_proposal::MakeProposalOpts),
     MakeUpgradeCanisterProposal(make_upgrade_canister_proposal::MakeUpgradeCanisterProposalOpts),
-    MakeCommitProposedBatchProposal(make_commit_proposed_batch_proposal::MakeCommitProposedBatchProposalOpts),
+    MakeCommitProposedBatchProposal(
+        make_commit_proposed_batch_proposal::MakeCommitProposedBatchProposalOpts,
+    ),
     NeuronId(neuron_id::NeuronIdOpts),
     NeuronPermission(neuron_permission::NeuronPermissionOpts),
     NewSaleTicket(new_sale_ticket::NewSaleTicketOpts),
sesi200 commented 2 months ago

I don't follow GH notifications reliably. If you want me to have another look please ping me on the forum