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.

Sign up for GitHub

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 support for range voting e.g. "70802-9" #136

Merged
merged 8 commits into from
Sep 27, 2022
Merged

Add support for range voting e.g. "70802-9" #136

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
43d86e5
Add support for range voting e.g. "70802-9".
jplevyak Sep 21, 2022
de0d2ae
Add test.
jplevyak Sep 21, 2022
1f4d8dc
Fixup docs.
jplevyak Sep 21, 2022
c7d3b30
Make test coverage better.
jplevyak Sep 21, 2022
3c91880
Fix clippy issues.
jplevyak Sep 21, 2022
aeb89ec
Pull out range function, add comments and tests and update error
jplevyak Sep 23, 2022
f0774df
Update src/commands/neuron_manage.rs
jplevyak Sep 26, 2022
ffecac6
Address comments.
jplevyak Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/cli-reference/quill-neuron-manage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,5 @@ quill neuron-manage [option] <neuron id>
| `--merge-maturity <MERGE_MATURITY>` | Merge the percentage (between 1 and 100) of the maturity of a neuron into the current stake. |
| `--remove-hot-key <REMOVE_HOT_KEY>` | Principal hot key to be removed. |
| `--split <SPLIT>` | Split off the given number of ICP from a neuron. |
| `--register-vote <PROPOSAL_ID>...` [--reject] | Vote to approve (default) or reject proposal(s). |
| `--register-vote (<PROPOSAL_ID>|<PROPOSAL_ID>-<RANGE_END_SUFFIX>)...` [--reject] | Vote to approve (default) or reject proposal(s). |

50 changes: 40 additions & 10 deletions src/commands/neuron_manage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ pub struct ManageOpts {

/// Vote on proposal(s) (approve by default).
#[clap(long, multiple_values(true))]
register_vote: Option<Vec<u64>>,
register_vote: Option<Vec<String>>,

/// Reject proposal(s).
#[clap(long)]
Expand Down Expand Up @@ -407,15 +407,45 @@ pub fn exec(auth: &AuthInfo, opts: ManageOpts) -> AnyhowResult<Vec<IngressWithRe

if let Some(proposals) = opts.register_vote {
for proposal in proposals {
let args = Encode!(&ManageNeuron {
id,
command: Some(Command::RegisterVote(RegisterVote {
vote: if opts.reject { 2 } else { 1 },
proposal: Some(ProposalId { id: proposal }),
})),
neuron_id_or_subaccount: None,
})?;
msgs.push(args);
let mut proposals = Vec::new();
if proposal.contains('-') {
let pieces: Vec<&str> = proposal.split('-').collect();
if pieces.len() == 2 {
if let Ok(mut first) = pieces[0].parse::<u64>() {
let mut last = pieces[0].to_string();
last.replace_range(
pieces[0].chars().count() - pieces[1].chars().count()..,
pieces[1],
);
if let Ok(last) = last.parse::<u64>() {
if ((last - first) as usize) < 100 {
proposals.resize_with((last - first + 1) as usize, || {
first += 1;
first - 1
});
}
}
}
}
} else if let Ok(proposal) = proposal.parse::<u64>() {
proposals.push(proposal);
}
if proposals.is_empty() {
return Err(anyhow!(
"Proposal ranges must be less than 100 and in the form XXX-YY."
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is confusing at best: it claims the format HAS to be XXX-YY, but if the XXX part was as strictly enforced as the YY part it wouldn't allow me to specify any proposals larger than 999.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions? Perhaps just X-Y ?

Copy link
Member

@ericswanson-dfinity ericswanson-dfinity Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's start with <a number>-<a number greater than the first number> as valid. So these are ok:

  • 465-475
  • 11033-11270
    We can consider forms of shorthand once those are supported, but I think we should be sure that it's worth it / likely to be used. While one might see shorthand like "349-51" on a piece of paper, I'm not sure how many would expect that to be valid input for a command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use 78825-8 for example all the time. We often have small ranges in sets of upgrade or replace proposals. I think that X-Y encourages the natural interpretation which is the unambiguous suffix semantics e.g. 123-5 123-25 123-125 are all the same and 123-1 is the empty range i.e. there is no natural interpretation that it might be say 131 based on some notion carry. For documentation like this I think less is more sometimes. If someone doesn't use the additional suffix capability it isn't the worst thing.

));
}
for proposal in proposals {
let args = Encode!(&ManageNeuron {
id,
command: Some(Command::RegisterVote(RegisterVote {
vote: if opts.reject { 2 } else { 1 },
proposal: Some(ProposalId { id: proposal }),
})),
neuron_id_or_subaccount: None,
})?;
msgs.push(args);
}
}
};

Expand Down
1 change: 1 addition & 0 deletions tests/commands/neuron-manage-vote-range.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
${CARGO_TARGET_DIR:-../target}/debug/quill neuron-manage 2313380519530470538 --register-vote 349-51 --pem-file - | ${CARGO_TARGET_DIR:-../target}/debug/quill send --dry-run -
54 changes: 54 additions & 0 deletions tests/outputs/neuron-manage-vote-range.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
Sending message with

Call type: update
Sender: fdsgv-62ihb-nbiqv-xgic5-iefsv-3cscz-tmbzv-63qd5-vh43v-dqfrt-pae
Canister id: rrkah-fqaaa-aaaaa-aaaaq-cai
Method name: manage_neuron
Arguments: (
record {
id = opt record { id = 2_313_380_519_530_470_538 : nat64 };
command = opt variant {
RegisterVote = record {
vote = 1 : int32;
proposal = opt record { id = 349 : nat64 };
}
};
neuron_id_or_subaccount = null;
},
)
Sending message with

Call type: update
Sender: fdsgv-62ihb-nbiqv-xgic5-iefsv-3cscz-tmbzv-63qd5-vh43v-dqfrt-pae
Canister id: rrkah-fqaaa-aaaaa-aaaaq-cai
Method name: manage_neuron
Arguments: (
record {
id = opt record { id = 2_313_380_519_530_470_538 : nat64 };
command = opt variant {
RegisterVote = record {
vote = 1 : int32;
proposal = opt record { id = 350 : nat64 };
}
};
neuron_id_or_subaccount = null;
},
)
Sending message with

Call type: update
Sender: fdsgv-62ihb-nbiqv-xgic5-iefsv-3cscz-tmbzv-63qd5-vh43v-dqfrt-pae
Canister id: rrkah-fqaaa-aaaaa-aaaaq-cai
Method name: manage_neuron
Arguments: (
record {
id = opt record { id = 2_313_380_519_530_470_538 : nat64 };
command = opt variant {
RegisterVote = record {
vote = 1 : int32;
proposal = opt record { id = 351 : nat64 };
}
};
neuron_id_or_subaccount = null;
},
)