refactor: method schemas in Governance contracts #6114

Chris-Hibbert · 2022-09-02T00:19:34Z

closes: #6060

Description

Convert Governance contracts to use method schemas

Can be reviewed as separate commits:

commit 0: basic governance types
commit 1: committee as heapFarClass
commit 2: binaryVoteCounter and questionDetails as heapFarClasses

Security Considerations

This uses method schemas to enforce types in the APIs.

Documentation Considerations

No changes in behavior.

Testing Considerations

Existing tests pass. I didn't add tests for out-of-range parameters.

erights · 2022-09-02T02:20:19Z

packages/governance/src/typeGuards.js

+export const NoParamChangesPositionShape = harden({
+  noChange: M.arrayOf(M.string()),
+});
+export const ParamChangesPositionsShape = [


Suggested change

export const ParamChangesPositionsShape = [

export const ParamChangesPositionsShape = harden([

and several other places

erights · 2022-09-02T16:59:41Z

packages/governance/src/typeGuards.js

+  'offer_filter',
+);
+
+export const TimerShape = M.handle('timer');


For a remotable, the most that the pattern system can enforce is that it is a remotable. The string argument is only for diagnostic purposes. So the following does all the enforcement you have in mind, but by itself fails to provide the diagnostic info that you're expecting a handle.

Suggested change

export const TimerShape = M.handle('timer');

export const TimerShape = M.remotable('timer');

you could just indicate that in the name manually

Suggested change

export const TimerShape = M.handle('timer');

export const TimerShape = M.remotable('timerHandle');

or, if you think it's worth it, create an abstraction that just does the name mangling

Suggested change

export const TimerShape = M.handle('timer');

const makeHandleShape = name => M.remotable(`${name}Handle`);

//...

export const TimerShape = makeHandleShape('timer');

cool! thanks.

erights

Review going well but not done yet.

packages/governance/src/binaryVoteCounter.js

packages/governance/src/typeGuards.js

packages/governance/src/contractGovernance/governParam.js

erights · 2022-09-06T04:19:29Z

packages/governance/src/contractGovernor.js

@@ -27,7 +28,7 @@ const validateQuestionDetails = async (zoe, electorate, details) => {
    counterInstance,
    issue: { contract: governedInstance },
  } = details;
-  validateParamChangeQuestion(details);
+  fit(details, ParamChangesQuestionDetailsShape);


packages/governance/src/typeGuards.js

packages/governance/src/noActionElectorate.js

packages/governance/src/typeGuards.js

erights

LGTM!

erights · 2022-09-06T19:29:57Z

packages/governance/src/committee.js

-  });
-
-  return { publicFacet, creatorFacet };
+  return makeCommitteeKit();


Another one where the kit is instantiated only once, so the objects in the kit are singletons. You should just use makeHeapFarInstance on each.

erights · 2022-09-06T19:44:01Z

packages/governance/src/committee.js

+        getPoserInvitation() {
+          return getPoserInvitation(zcf, async (voteCounter, questionSpec) => {
+            const quorumThreshold = quorumRule => {


AFAICT, the body of the argument function here is identical to the body of the addQuestion method below. Although the switch to inline concise methods admittedly make sharing this function harder, remember that you can send messages to self.

Suggested change

getPoserInvitation() {

return getPoserInvitation(zcf, async (voteCounter, questionSpec) => {

const quorumThreshold = quorumRule => {

getPoserInvitation() {

const { facet: { creatorFacet } } = this;

return getPoserInvitation(zcf, async (voteCounter, questionSpec) =>

creatorFacet.addQuestion(voteCounter, questionSpec),

);

},

The suggestion above is if you stick with the defineHeapFarClassKit. If you switch to multiple makeHeapFarInstances and call the lexical variable creatorFacet, then the thisful line disappears and the rest of the suggestion still applies.

Thank you. I should have noticed the duplication and looked for an appropriate way to get the sharing back.

erights · 2022-09-06T19:54:22Z

packages/governance/src/question.js

+  // XXX It would be nice to enforce this using parameterized types, but that
+  // seems to only enforce type constraints, (i.e. the tieOutcome will be the
+  // same type as any of the positions) unless you can provide the concrete
+  // value at pattern creation time.


yes. No change suggested.

erights · 2022-09-06T19:58:27Z

packages/governance/src/question.js

@@ -43,124 +38,9 @@ const QuorumRule = /** @type {const} */ ({
  ALL: 'all',
 });

-/**


Re the deletions from here (line 46) through line 196:

OMG that's a lot of manual input validation code deleted! Awesome!

It strikes me that this might serve as one of the extremes for our benchmarking, comparing manual input validation to pattern-based. Attn @gibson042 @warner

erights · 2022-09-06T20:18:09Z

packages/governance/src/typeGuards.js

+export const SimpleQuestionDetailsShape = harden({
+  method: ChoiceMethodShape,
+  issue: SimpleIssueShape,
+  positions: SimplePositionsShape,
+  electionType: M.or('election', 'survey'),
+  maxChoices: M.gte(1),
+  closingRule: ClosingRuleShape,
+  quorumRule: QuorumRuleShape,
+  tieOutcome: NoSimplePositionShape,
+  questionHandle: makeHandleShape('Question'),
+  counterInstance: InstanceHandleShape,
+});


AFAICT, the parts in common with SimpleQuestionSpecShape are identical. If this is an intentional part of the design, then

Suggested change

export const SimpleQuestionDetailsShape = harden({

method: ChoiceMethodShape,

issue: SimpleIssueShape,

positions: SimplePositionsShape,

electionType: M.or('election', 'survey'),

maxChoices: M.gte(1),

closingRule: ClosingRuleShape,

quorumRule: QuorumRuleShape,

tieOutcome: NoSimplePositionShape,

questionHandle: makeHandleShape('Question'),

counterInstance: InstanceHandleShape,

});

export const SimpleQuestionDetailsShape = harden({

...SimpleQuestionSpecShape,

questionHandle: makeHandleShape('Question'),

counterInstance: InstanceHandleShape,

});

Wherever interface extension is an intentional design, this is a good way to express it.

Thanks. I was looking for that, and didn't find it.

erights · 2022-09-06T20:32:01Z

packages/governance/src/typeGuards.js

+// XXX I want to add questionHandle and counterInstance to
+// ParamChangesQuestionSpecShape. I don't see any alternative to adding the
+// methods to each member separately
+export const QuestionDetailsShape = M.or(
+  ParamChangesQuestionDetailsShape,
+  ApiInvocationQuestionDetailsShape,
+  OfferFilterQuestionDetailsShape,
+  SimpleQuestionDetailsShape,
+);


Thanks for putting the question inline. Would be good for anything in common to be pulled out of the expensive M.or. If you pull these two common fields out of the branches, then you should also rename the branches to not imply that they're meaningful by themselves, and stop exporting them. For three of these, they are only used here, so no problem. But ParamChangesQuestionDetailsShape is used elsewhere. Assuming that this is dealt with somehow, we could rewrite the factored pattern as

Suggested change

// XXX I want to add questionHandle and counterInstance to

// ParamChangesQuestionSpecShape. I don't see any alternative to adding the

// methods to each member separately

export const QuestionDetailsShape = M.or(

ParamChangesQuestionDetailsShape,

ApiInvocationQuestionDetailsShape,

OfferFilterQuestionDetailsShape,

SimpleQuestionDetailsShape,

);

export const QuestionDetailsShape = M.split({

questionHandle: QuestionHandleShape, // making the handle shape should already be factored out.

counterInstance: InstanceHandleShape,

},

M.or(

ParamChangesQuestionPartialDetailsShape,

ApiInvocationQuestionPartialDetailsShape,

OfferFilterQuestionPartialDetailsShape,

SimpleQuestionPartialDetailsShape,

),

});

Thanks for the explanation. That's a helpful alternative.

Now that I see how ParamChangesQuestionDetailsShape is used, I wouldn't be surprised to find other uses of the other QuestionDetailsShapes. I think I'll leave it as is.

split interfaces in BinaryVoteCounter to separate heapFarInstances include unused parameters in interface methods move some guards to Zoe harden some arrays `1` is better than `M.eq(1)`

split up more makeHeapFarInstances clean up some interface shapes. reduce duplication between farInstance methods

Chris-Hibbert added the Governance Governance label Sep 2, 2022

Chris-Hibbert requested a review from erights September 2, 2022 00:19

Chris-Hibbert self-assigned this Sep 2, 2022

Chris-Hibbert marked this pull request as draft September 2, 2022 00:19

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from a99346b to bcc4bd9 Compare September 2, 2022 00:28

erights reviewed Sep 2, 2022

View reviewed changes

erights force-pushed the markm-heap-far-classes branch 2 times, most recently from 54815db to 3c0bcd7 Compare September 3, 2022 05:00

Chris-Hibbert force-pushed the 6060-governanceSchemas branch 2 times, most recently from ea40db9 to 0e12651 Compare September 3, 2022 21:39

erights force-pushed the markm-heap-far-classes branch 2 times, most recently from f0d2aa5 to a2cff79 Compare September 4, 2022 01:02

Base automatically changed from markm-heap-far-classes to master September 4, 2022 06:10

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from 60a1188 to 711dd62 Compare September 5, 2022 18:02

Chris-Hibbert changed the title ~~chore: basic types for governance~~ refactor: method schemas in Governance contracts Sep 5, 2022

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from 711dd62 to 6612417 Compare September 5, 2022 18:09

Chris-Hibbert requested a review from erights September 5, 2022 18:10

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from 6612417 to 50f9ee4 Compare September 5, 2022 18:26

Chris-Hibbert marked this pull request as ready for review September 5, 2022 19:04

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from 50f9ee4 to 70dd248 Compare September 5, 2022 20:30

erights requested changes Sep 6, 2022

View reviewed changes

Chris-Hibbert requested a review from erights September 6, 2022 20:02

erights approved these changes Sep 6, 2022

View reviewed changes

erights mentioned this pull request Sep 6, 2022

Determine performance impact of enforcing Method Schemas #6098

Closed

Chris-Hibbert added the automerge:squash Automatically squash merge label Sep 7, 2022

Chris-Hibbert added 5 commits September 7, 2022 09:10

chore: basic types and fit patterns for governance

2a79cf1

refactor: turn committee into a heapFarClass

a5b8df8

refactor: make binaryVoteCounter and QuestionDetails heapFarClasses

cdbb65e

refactor: heapFarInstances for voterFacet and noActionElectorate

4bda7f3

chore: changes from code review

675c019

split interfaces in BinaryVoteCounter to separate heapFarInstances include unused parameters in interface methods move some guards to Zoe harden some arrays `1` is better than `M.eq(1)`

chore: refactor based on review comments

6bd1cc4

split up more makeHeapFarInstances clean up some interface shapes. reduce duplication between farInstance methods

Chris-Hibbert force-pushed the 6060-governanceSchemas branch from 8aaeb3d to 6bd1cc4 Compare September 7, 2022 16:24

chore: integration updates

526bea2

mergify bot merged commit d6e382a into master Sep 7, 2022

mergify bot deleted the 6060-governanceSchemas branch September 7, 2022 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: method schemas in Governance contracts #6114

refactor: method schemas in Governance contracts #6114

Chris-Hibbert commented Sep 2, 2022 •

edited

Loading

erights Sep 2, 2022

Chris-Hibbert Sep 2, 2022

erights Sep 2, 2022

Chris-Hibbert Sep 2, 2022

erights left a comment

erights Sep 6, 2022

erights left a comment

erights Sep 6, 2022

Chris-Hibbert Sep 7, 2022

erights Sep 6, 2022 •

edited

Loading

Chris-Hibbert Sep 7, 2022

erights Sep 6, 2022

erights Sep 6, 2022

erights Sep 6, 2022

Chris-Hibbert Sep 7, 2022

erights Sep 6, 2022

Chris-Hibbert Sep 7, 2022

	export const ParamChangesPositionsShape = [
	export const ParamChangesPositionsShape = harden([

	export const TimerShape = M.handle('timer');
	export const TimerShape = M.remotable('timer');

-export const TimerShape = M.handle('timer');
+const makeHandleShape = name => M.remotable(`${name}Handle`);
+//...
+export const TimerShape = makeHandleShape('timer');

refactor: method schemas in Governance contracts #6114

refactor: method schemas in Governance contracts #6114

Conversation

Chris-Hibbert commented Sep 2, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Sep 6, 2022 • edited Loading

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

Chris-Hibbert commented Sep 2, 2022 •

edited

Loading

erights Sep 6, 2022 •

edited

Loading