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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ClankPan
Copy link

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

  • I have made corresponding changes to the documentation in docs/cli-reference.
  • I have added corresponding integration tests.

@sesi200
Copy link
Contributor

sesi200 commented Sep 11, 2024

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
Copy link
Author

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

@sesi200
Copy link
Contributor

sesi200 commented Sep 12, 2024

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
Copy link
Contributor

sesi200 commented Sep 12, 2024

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
Copy link
Contributor

sesi200 commented Sep 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants