feat: ballot counter for two-outcome elections #3233

Chris-Hibbert · 2021-06-01T22:24:00Z

doesn't handle Quorum requirements

dtribble

WIP

packages/governance/src/binaryBallotCounter.js

dtribble · 2021-06-15T00:46:15Z

packages/governance/src/binaryBallotCounter.js

+      }
+    });
+
+    const stats = {


seems to me like the tally should be really actionable. Otherwise how do enforcement mechanisms work?

I'm not sure what you mean by "actionable". My expectation was that clients would wait on the outcome. Most things should be able to decide what to do based on that. If they need the extra data in stats, they can examine it. What would be more "actionable"?

It has the counts. It doesn't have the voting decision. Where would the code be that implements a required 2/3rds vote for success?

It has the counts. It doesn't have the voting decision.

The outcome is published with a notifier in outcomePromise. If you think tally should also include that, I can add that. Would you do that by modifying the existing stats object, or constructing a new one to return once the quorum check (line 79) and decision (lines 83-89) have been made?

Where would the code be that implements a required 2/3rds vote for success?

The quorumChecker (line 79) has access to all the info in the tally. In the tests, I only have cases where it checks for a strict number, but it's as easy to compare to a fraction.

packages/governance/src/binaryBallotCounter.js

dtribble

Soem items that need discussion and an await that must be fixed

dtribble · 2021-06-15T16:02:10Z

packages/governance/src/binaryBallotCounter.js

+      ],
+    };
+
+    if (!(await E(quorumChecker).check(stats))) {


await needs to be top-level

dtribble · 2021-06-15T20:35:08Z

packages/governance/src/ballotBuilder.js

+  WEIGHT: 'weight',
+};
+
+const buildBallot = (method, question, positions, maxChoices = 1) => {


is there a maxChoices value to specify "no limit"? Should that be the default instead of 1? (e.g., if you don't provide a limit, there isn't one)

That's reasonable. I changed the default to 0.

packages/governance/src/ballotBuilder.js

packages/governance/src/binaryBallotCounter.js

dtribble · 2021-06-16T05:00:27Z

packages/governance/src/binaryBallotCounter.js

+  });
+
+  const creatorFacet = Far('adminFacet', {
+    closeVoting: () => (isOpen = false),


(isOpen = false) seems a bad pattern; it's assignment in an expression, which we typically don't allow. It's not obvious that the result is intended to be returned. That should be { isOpen = false; }

Ooo, thanks! I blame WebStorm. It offered to "drop the extraneous braces", and then insisted that I add parens around the resulting bare assignment. I'll watch for this in the future.

dtribble · 2021-06-16T05:11:44Z

packages/governance/src/binaryBallotCounter.js

+      }
+    });
+
+    const stats = {


It has the counts. It doesn't have the voting decision. Where would the code be that implements a required 2/3rds vote for success?

katelynsills

A few things:

I think returning a string if it's a tie is going to make things difficult for downstream clients. We should return undefined or a pre-provided default.
I don't understand why we are asking asynchronously whether we have a quorum. Seems like something the ballotCounter should know ahead of time, in the same ways as the question and positions.
We need to be very clear about what is a user-defined value that hasn't been validated yet, and what has already been validated. A few things are never validated.
We need to add types since that will catch a lot that reviewing might miss, and makes reviewing easier.

katelynsills · 2021-06-17T01:18:20Z

packages/governance/src/binaryBallotCounter.js

+import { E } from '@agoric/eventual-send';
+import { ChoiceMethod, buildBallot } from './ballotBuilder';
+
+const TIE_VOTE = "It's a tie!";


If the outcome of the vote is going to be used directly to change a parameter, I think we really do not want to return a string indicating that there was a tie.

Two alternatives: we return undefined in the case of a tie, which requires the downstream client of this to handle ties directly, or we allow the creator of the ballot to pick a default when creating the ballot counter. This default is returned in the case of a tie. I think this might be really useful, especially if binaryBallots are mostly used for YES/NO votes and we want to have a preference of no change (i.e. NO) in the case of a tie.

I really like the idea of specifying the default when the choice is action/no action, but it can't be required because there are both binary questions without a fallback (choosing one of two candidates) and cases with multiple positions where there is no default.

I think I have to go with undefined. That is a better signal than my string.

katelynsills · 2021-06-17T01:30:52Z

packages/governance/src/binaryBallotCounter.js

+
+const start = zcf => {
+  const { question, positions } = zcf.getTerms();
+  return makeBinaryBallotCounter(question, positions[0], positions[1]);


What if positions.length > 2? Can we fail fast with an assert here?

I put the check as the first thing inside makeBinaryBallotCounter(), so it'll fail fast here, too.

katelynsills · 2021-06-17T01:37:39Z

packages/governance/src/ballotBuilder.js

+// WEIGHT: voter lists their choices, each with a numerical weight. High
+//   numbers are most preferred.
+
+const ChoiceMethod = {


This needs types

katelynsills · 2021-06-17T01:37:46Z

packages/governance/src/ballotBuilder.js

+  WEIGHT: 'weight',
+};
+
+const buildBallot = (method, question, positions, maxChoices = 0) => {


This needs types

katelynsills · 2021-06-17T01:39:50Z

packages/governance/src/ballotBuilder.js

+  WEIGHT: 'weight',
+};
+
+const buildBallot = (method, question, positions, maxChoices = 0) => {


In all of these files, question is never validated. If binaryBallotCounter is the origin of question, we need to clearly add assertions there, and then add a comment here that this function is past the validation.

I've added an assertion in binaryBallotCounter, but that won't be the only client of this method, so I added it here as well.

katelynsills · 2021-06-17T01:53:26Z

packages/governance/src/binaryBallotCounter.js

+    closeVoting: () => {
+      isOpen = false;
+    },
+    countVotes,


Shouldn't closing the vote count the votes? Why a separate call here?

you convinced me.

katelynsills · 2021-06-17T01:55:46Z

packages/governance/src/binaryBallotCounter.js

+};
+
+// Exported for testing purposes
+const makeBinaryBallotCounter = (question, aName, bName) => {


Is there a reason to not use the positions argument here too? It seems like the flow is positions in terms => aName, bName in makeBinaryBallotCounter => positions in makeBinaryBallot. Seems better to just pass it through after validating.

katelynsills · 2021-06-17T01:57:00Z

packages/governance/src/ballotBuilder.js

+  };
+
+  return {
+    getMethod: () => method,


It seems strange to make these getters when the data doesn't change. Why not return a record with all the information?

I hope I haven't misunderstood. I added a method that returns a record containing all the details. I think since we want to return choose() it has to be all methods.

katelynsills · 2021-06-17T02:41:43Z

packages/governance/src/ballotBuilder.js

+
+    for (const position of chosenPositions) {
+      assert(
+        positions.includes(position),


How would this work with WEIGHT? Seems like it only works if the chosenPositions is a list of strings. But types would help find cases like this.

Binary ballot only needs unweighted ballots. I've added some type declarations and some indications of where the unimplemented code would go. I don't think ORDER and CHOOSE_N ballots should use the same representation as WEIGHT.

I also changed the variable in binaryBallotCounter for the strength of a ballot from weight to shares since it's a completely different concept.

katelynsills · 2021-06-17T02:44:17Z

packages/governance/src/binaryBallotCounter.js

+
+    // ballot template has position choices; Each ballot in allBallots should
+    // match. count the valid ballots and report results.
+    const [positionA, positionB] = template.getPositions();


Are we getting these from the template because we don't trust aName and bName? Can we make that explicit?

katelynsills

All comments are small, and the only thing that I think is necessary is to keep @agoric/nat as a dependency and use it to check BIGINT (should have been in previous PR but wasn't caught)

katelynsills · 2021-06-18T23:32:57Z

packages/governance/package.json

@@ -31,11 +31,12 @@
  "homepage": "https://github.com/Agoric/agoric-sdk#readme",
  "dependencies": {
    "@agoric/assert": "^0.3.0",
+    "@agoric/captp": "^1.7.15",


I don't think this is used anywhere

katelynsills · 2021-06-18T23:49:03Z

packages/governance/package.json

    "@agoric/marshal": "^0.4.13",
-    "@agoric/nat": "^4.1.0",


Ah, we need this. I realized paramManager ParamType NAT needs to ensure it is both a bigint and a Nat (not a negative bigint)

katelynsills · 2021-06-19T00:01:30Z

packages/governance/src/types.js

+ */
+
+/**
+ * @typedef {Object} QurorumCounter


Typo here that prevents it from being used

katelynsills · 2021-06-19T00:03:42Z

packages/governance/src/binaryBallotCounter.js

+  assert.typeof(positionBName, 'string');
+  positions.push(positionAName, positionBName);
+
+  return buildBallot(ChoiceMethod.CHOOSE_N, question, positions, 1n);


It looks like we're still splitting up positions into positionAName and positionBName, and then re-making an array called positions. Why?

katelynsills · 2021-06-19T00:05:11Z

packages/governance/src/binaryBallotCounter.js

+  question,
+  positions,
+  threshold,
+  tieOutcome = undefined,


katelynsills · 2021-06-19T00:08:15Z

packages/governance/src/binaryBallotCounter.js

+const makeQuorumCounter = quorumThreshold => {
+  const check = stats => {
+    const votes = stats.results.reduce(
+      (runningTotal, { total }) => runningTotal + total,
+      0n,
+    );
+    return votes >= quorumThreshold;
+  };
+  /** @type {QuorumCounter} */
+  return Far('checker', { check });
+};


Can this just be:

Suggested change

const makeQuorumCounter = quorumThreshold => {

const check = stats => {

const votes = stats.results.reduce(

(runningTotal, { total }) => runningTotal + total,

0n,

);

return votes >= quorumThreshold;

};

/** @type {QuorumCounter} */

return Far('checker', { check });

};

const checkQuorum = (threshold, stats) => {

const votes = stats.results.reduce(

(runningTotal, { total }) => runningTotal + total,

0n,

);

return votes >= threshold;

};

That would work, but it wouldn't be compatible with the pluggable QuorumCounter that I intend to use across different counters.

Not objecting, but I would be surprised if the quorum checker was remote. That seems more like a module at most rather than a separate remote contract

It's plausible; I'm moving in that direction with my implementation of closure rules, which is next. I'm just not positive that we can foresee every quorum rule, which means it has to be an external contract/component.

katelynsills · 2021-06-19T00:08:51Z

packages/governance/src/binaryBallotCounter.js

+      X`The default outcome on a tie must be one of the positions, not ${tieOutcome}`,
+    );
+  }
+  const template = makeBinaryBallot(question, aName, bName);


Suggested change

const template = makeBinaryBallot(question, aName, bName);

const template = makeBinaryBallot(question, positions);

katelynsills · 2021-06-19T00:11:54Z

packages/governance/src/binaryBallotCounter.js

+
+    if (!makeQuorumCounter(threshold).check(stats)) {
+      outcomePromise.reject('No quorum');
+      return;


Does this mean that the tally promise never resolves?

good catch. thanks. moved the tally resolution up.

katelynsills · 2021-06-19T00:18:01Z

packages/governance/src/types.js

+ * @param {ChoiceMethod} method
+ * @param {string} question
+ * @param {string[]} positions
+ * @param {bigint} maxChoices


Do we want this to be a bigint, if we are comparing it to .length, a number? I think none of the code will error as is, but it might be nice for consistency

katelynsills · 2021-06-19T00:19:14Z

packages/governance/src/types.js

+/**
+ * @typedef {Object} CompleteOrderedBallot
+ * @property {string} question
+ * @property {string[]} ordered - ordered list of position from most prefered to


typo in preferred and in line 114

doesn't handle Quorum requirements see: #3185

Add types flesh out different ballot types a tiny bit moved quorum counting inside binaryBallotCounter More assertions dropped externally visible countVotes(), getQuestionPositions() dependency updates Made tie result be a default or undefined

simplify ballotCounter creating ballots resolve tally promise earlier typos

dtribble

A few comments but nothing blocking.

dtribble · 2021-06-20T18:38:17Z

packages/governance/src/ballotBuilder.js

+      X`only ${maxChoices} position(s) allowed`,
+    );
+
+    for (const position of chosenPositions) {


This might be better with a single assert of a compound check; e.g.,

assert(chosenPositions.every( p => positions.includes(p), details`Some positions in ${chosenPositions} are not valid in ${positions}`)

packages/governance/src/ballotBuilder.js

dtribble · 2021-06-20T18:41:38Z

packages/governance/src/ballotBuilder.js

+    case ChoiceMethod.ORDER:
+      throw Error(`choice method ${ChoiceMethod.ORDER} is unimplemented`);
+    case ChoiceMethod.WEIGHT:
+      throw Error(`choice method ${ChoiceMethod.WEIGHT} is unimplemented`);


Very minor: I would share the unimplemented code by putting both labels on the same unimplemented case.

done. yarn lint didn't object.

dtribble · 2021-06-20T18:45:52Z

packages/governance/src/binaryBallotCounter.js

+const makeQuorumCounter = quorumThreshold => {
+  const check = stats => {
+    const votes = stats.results.reduce(
+      (runningTotal, { total }) => runningTotal + total,
+      0n,
+    );
+    return votes >= quorumThreshold;
+  };
+  /** @type {QuorumCounter} */
+  return Far('checker', { check });
+};


Not objecting, but I would be surprised if the quorum checker was remote. That seems more like a module at most rather than a separate remote contract

Chris-Hibbert added Small Governance Governance labels Jun 1, 2021

Chris-Hibbert added this to the Beta Phase 4: Governance milestone Jun 1, 2021

Chris-Hibbert self-assigned this Jun 1, 2021

Chris-Hibbert requested a review from dtribble June 1, 2021 23:11

Chris-Hibbert assigned dtribble Jun 1, 2021

Chris-Hibbert marked this pull request as draft June 7, 2021 22:52

Chris-Hibbert force-pushed the buildBallot-3188 branch from 7995c4e to f620353 Compare June 8, 2021 20:38

Chris-Hibbert marked this pull request as ready for review June 8, 2021 22:23

Chris-Hibbert force-pushed the buildBallot-3188 branch 2 times, most recently from d00b094 to e0f4f4a Compare June 11, 2021 00:22

Chris-Hibbert changed the base branch from gov-lib-3186 to master June 11, 2021 16:55

Chris-Hibbert force-pushed the buildBallot-3188 branch from e0f4f4a to 1e25870 Compare June 11, 2021 17:51

dtribble reviewed Jun 15, 2021

View reviewed changes

Chris-Hibbert requested review from katelynsills and dtribble June 15, 2021 20:02

Chris-Hibbert force-pushed the buildBallot-3188 branch from 1c9ea98 to a696c43 Compare June 15, 2021 20:42

dtribble requested changes Jun 16, 2021

View reviewed changes

Chris-Hibbert requested a review from dtribble June 16, 2021 16:24

katelynsills suggested changes Jun 17, 2021

View reviewed changes

Chris-Hibbert force-pushed the buildBallot-3188 branch 3 times, most recently from ee7c381 to e943891 Compare June 18, 2021 22:54

Chris-Hibbert requested a review from katelynsills June 18, 2021 23:08

katelynsills approved these changes Jun 19, 2021

View reviewed changes

Chris-Hibbert force-pushed the buildBallot-3188 branch 2 times, most recently from 26b31ce to c297a97 Compare June 20, 2021 02:52

Chris-Hibbert added 2 commits June 20, 2021 10:40

feat: ballot counter for two-outcome elections

45c427b

doesn't handle Quorum requirements see: #3185

refactor: switch to CHOOSE_N. update functions to arrow functions

87e42eb

Chris-Hibbert added 7 commits June 20, 2021 10:40

refactor: cleanup after moving code into vats

a5be3a8

refactor: move submitVote() to a facet for the voter.

25635b5

chore: update release versions in package.json

4bc42e2

refactor: move voterFacet to be inside creatorFacet

0b21763

chore: review cleanups: default:1, sharedFacet 1st, top-level await

b60c604

chore: cleanups

b2c0331

simplify ballotCounter creating ballots resolve tally promise earlier typos

Chris-Hibbert force-pushed the buildBallot-3188 branch from c297a97 to b2c0331 Compare June 20, 2021 17:40

dtribble approved these changes Jun 20, 2021

View reviewed changes

chore: very minor cleanups: single assert for list; combine cases

6c1d063

Chris-Hibbert merged commit 6dddaa6 into master Jun 20, 2021

Chris-Hibbert deleted the buildBallot-3188 branch June 20, 2021 21:33

Chris-Hibbert mentioned this pull request Jun 22, 2021

Committee Binary Vote (Governance) #3188

Closed

	const template = makeBinaryBallot(question, aName, bName);
	const template = makeBinaryBallot(question, positions);

feat: ballot counter for two-outcome elections #3233

feat: ballot counter for two-outcome elections #3233

Conversation

Chris-Hibbert commented Jun 1, 2021

dtribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment