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

ContractGovernor manages parameter updating for a contract #3448

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Jul 2, 2021

ContractGovernor is an ElectionManager that knows a particular
Registrar, and can manage multiple contracts. It queries the public
facet of the contract to find which params are supposed to be maanged,
and can create new Ballot questions to ask its electorate to change
the values of those parameters.

There's a README.md that gives on overview of the interdependent
APIs.

The swingsetTest exercise the ContractGovernor over a trivial
governedContract that shows how a contract can configure its
parameters, and the methods that have to be available in order for
everythign to be found and verified. The tests include a verification
step that cross-checks the Registrar,
ElectionManager (i.e. ContractGovernor in this case), and Ballots to
see that they are consistent.

The API os Ballot and others is changed to bundle the details of a
question into BallotSpec. This affects some of the unit tests.

One open issue: I suspect that the question name should be a
structured object. It's currently a string, which is required to be
unique. I'd prefer to leave that to a refactoring to clean up.

closes #3189
closes #3783

@Chris-Hibbert Chris-Hibbert added this to the Beta Phase 4: Governance milestone Jul 2, 2021
@Chris-Hibbert Chris-Hibbert self-assigned this Jul 2, 2021
Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

Below are comments mostly about small details. This is only a partial review, and I didn't dig into everything.

I think I would prefer a model where the governing is outside the governed contract, similar to #3453

I think it would also help to have a write up for the following two scenarios:

Scenario 1: As a user, I want to use contract instance x, but only if it is well governed. How can I check that it is well governed, if I only start with knowledge of the instance object for x? (We can assume a method in Zoe to get installation from instance).

Scenario 2: I want to participate in a vote to change the y parameter of contract instance x. How do I do it? How do I know that I can vote or not? What information do I have to start? (For instance, in the case of a committee vote, I might have the instance for x, the governed contract, and an invitation that represents the ability to vote.)


const MALLEABLE_NUMBER = 'MalleableNumber';

export const governedParameterTerms = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, why this particular structure? It seems to have an unnecessary level. Could it just be:

export const governedParameterTerms = [MALLEABLE_NUMBER];

Also, why export this? Is that for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #3310 the treasury declares two paramManagers for the parameters that relate to the AMM and to the Pools. Each Pool has a separate set of parameters, so the number of parameters being managed grows with the number of Pools.

The description of the parameters is exported so that the contract can refer to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example still has an unnecessary level to it. I'm not understanding why two sets of parameters requires the additional level.

packages/governance/src/contractGovernor.js Outdated Show resolved Hide resolved
packages/governance/src/contractGovernor.js Outdated Show resolved Hide resolved
// read and trust the contract to enforce that the instance in its terms was
// for the same instance as the private facet it uses. Having it public
// means anyone can ask the governor to manage other compatible contracts.
governContract,
Copy link
Contributor

Choose a reason for hiding this comment

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

Who pays for this contract instance? My guess is that they will not want to make the governContract method public, since its probably pretty costly to call, let alone to create new BallotCounters through createQuestion. So having governContract be public doesn't seem like a usable solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in the context where the governor was made inside the governedContract. In governedContract.js, this was only accessible via the creatorFacet, so we have the same instincts about keeping it closely held.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this method no longer exists, but the type for it still exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I removed it.

Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

This is a partial review. There's a few large security problems:

  • There's a security vulnerability in the contractGovernor where the creatorFacet of the governedContract (which is how you get the paramManager) is returned to the creator of the contractGovernor. That means the creator can change the values willy-nilly, which is exactly what we must ensure we prevent, since only the governance process in code should be able to change the values.
  • There's a potential vulnerability in the contractGovernor where the creator of the contractGovernor instance could trick the user into thinking that a different registrar is used. The registrar instance is gotten from the registrarCreatorFacet that is supplied by the creator. The user is given this registrar instance. How does the user know this registrar is actually part of the governance of this contract?

Because of the two problems above, can I have a step-by-step walkthrough (with code would be even better) of the two scenarios I mentioned earlier? I think this will force both of us to be explicit about what the user can prove to themselves and where they might be tricked.

Scenario 1: As a user, I want to use contract instance x, but only if it is well governed. How can I check that it is well governed, if I only start with knowledge of the instance object for x? (We can assume a method in Zoe to get installation from instance).

Scenario 2: I want to participate in a vote to change the y parameter of contract instance x. How do I do it? How do I know that I can vote or not? What information do I have to start? (For instance, in the case of a committee vote, I might have the instance for x, the governed contract, and an invitation that represents the ability to vote.)

Here's one that I started for Scenario 1 that includes the changes I would make:


Scenario 1: As a user, I want to use contract instance x, but only if it is well governed. How can I check that it is well governed, if I only start with knowledge of the instance object for x? (We can assume a method in Zoe to get installation from instance).

  1. I start knowing contract instance X.

  2. I call E(zoe).getInstallation(X) to get the installation.

  3. I get the code (??) (TODO: fill in)

  4. I see that the code for contract instance X includes the standard
    helper "handleGovernance". This means that the terms must have the
    governedParams in this particular format: [TODO: fill in]. It also means
    there's a paramManager that manages them. The paramManager is code
    I know and trust. I know that the paramManager is returned as part
    of the creatorFacet. Lastly, this means that the "governor"
    contract instance is in the terms of contract instance X.

  5. I do const terms = E(zoe).getTerms(X);

  6. I get the "governor" contract instance, Y from the terms of
    contract instance X: const { governor } = terms;

  7. I call E(zoe).getInstallation(Y) to get the installation for the
    governor contract instance.

  8. The contract governor installation is a widely known and audited
    one. If I am familiar with it, I skip to step [TODO: fill in] Otherwise I continue.

  9. I get the code for the governor installation TODO: fill in??

  10. In the code, I can see that the publicFacet has a method
    getGovernedContract which returns the governedContract instance
    that the governor creates.

  11. I need to confirm that governedContract and the governor mutually
    agree on their relationship. I use the helper validateGoverning:

      const validateGoverning = async (zoe, allegedGoverned, allegedGovernor) => {
        const allegedGovernorPF = E(zoe).getPublicFacet(allegedGovernor);
        const allegedGovernedTermsP = E(zoe).getTerms(allegedGoverned);
        const realGovernedP = E(allegedGovernorPF).getGovernedInstance();
        const [{ governor: realGovernor }, realGoverned] = await Promise.all([
          allegedGovernedTermsP,
          realGovernedP,
        ]);
        assert(
          allegedGovernor === realGovernor,
          X`The alleged governor did not match the governor gotten from the governed contract`,
        );
    
        assert(
          allegedGoverned === realGoverned,
          X`The alleged governed did not match the governed contract gotten from the governor`,
        );
      };
  12. I must also confirm that the creatorFacet of the governedContract is never released to the
    creator of the governor. I can do this by visually inspecting the
    code of the governor and ensuring that both the param manager and
    the creatorFacet for the governedContract never escape.

  13. A method voteOnParamChange is available to the caller of
    startGovernedInstance on the creatorFacet of the governor. The
    voteOnParamChange method takes as arguments:

    paramMgrName
    paramName,
    proposedValue,
    closingTime

    This means that these are entirely up to the whims of the holder
    of the object with the voteOnParamChange method, and are not baked
    into the smart contract.

    This seems to be the bare minimum required to specify when
    specifying a parameter to change, which makes sense. Everything
    else is either specified as a term or as code in the governor
    contract.

  14. How can I find the electorate that makes decisions for this
    contract? The registrar contract instance is listed... [TODO: complete]


const creatorFacet = Far('creator facet of governed contract', {
getContractGovernor: () => electionManager,
getParamMgrAccessor,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this just be a function that returns the paramManager itself? Why return an object with a function?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to be able to return multiple paramManagers, I suggest returning an object like:

{ ammManager: paramManager1, treasuryManager: paramManager2 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the treasury, the multiple treasury paramManagers are selected by collateralType. Passing the collateralType to getParameterMgrAccessor() seemed clearer than any structure I was able to think of. I would prefer a structure if there were a simple way to do that.

/** @type {ContractStartFn} */
const start = async zcf => {
const {
/** @type {Instance} */ electionManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

These types aren't being applied
Screen Shot 2021-07-15 at 11 22 27 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type-screenShot-210720

Fascinating! WebStorm shows it, for once.

I don't think it would be a productive use of my time to open VS Code and hover over every type to find things like this. Can you help me figure out how/why yarn lint isn't complaining and VS Code isn't seeing it? The syntax looks right to me, and WebStorm seems happy.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can you help me figure out how/why yarn lint isn't complaining

We don't have "Typescript noImplicitAny" enabled.

and VS Code isn't seeing it?

We don't have the JSDoc linting rules cranked up high enough to diagnose this questionable syntax. Since this syntax of typing the destructuring isn't supported, VSCode doesn't complain (which is a major feature of using JSDoc).

The syntax looks right to me

It doesn't look right to me, but that's probably because I'm conditioned by using VSCode, and this attempt to assign types to destructured properties has never worked.

I think the portable syntax you're reaching for is:

  /** @type {{ electionManager: Instance, governedParams: ParameterNameList }} */
  const { electionManager, governedParams } = zcf.getTerms();

Even better would be to have a regularly-defined @typedef to name the terms for this contract, and use that defined name directly in /** @type {MyKindOfTerms} */.

and WebStorm seems happy.

I don't know why WebStorm is happy. It must not be using the Typescript Language Server which so many other systems rely on. Or maybe your IDE is somehow using a different version of Typescript?

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 couldn't get /** @type {{ electionManager: Instance, governedParams: ParameterNameList }} */ to work (standalone or as a typedef), and I couldn't find any contracts that declared the types of the values returned from zcf.getTerms(), so I dropped the declaration with disappointment.

const paramManager = buildParamManager(paramDesc);

assert(
sameStructure(governedParams, harden(governedParameterTerms)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if governedParams is of type ParamDescriptions, I don't see how these can be the same structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the declaration of governedParameterTerms was wrong. I don't understand why yarn lint wasn't complaining.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

the declaration of governedParameterTerms was wrong. I don't understand why yarn lint wasn't complaining.

How would you expect it to complain? Can you please be more specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If yarn lint and WebStorm have different understandings of typescript, then that explains why there were no complaints.

packages/governance/src/contractGovernor.js Show resolved Hide resolved

const {
publicFacet: counterPublicFacet,
instance: ballotCounter,
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the ballotCounter used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used in governParams directly. It is returned to the caller of voteOnParamChange, who wants it to do cross checks and wait for the outcome.

packages/governance/src/contractGovernor.js Outdated Show resolved Hide resolved
Comment on lines 19 to 22
* its creatorFacet. getParamMgrAccessor() takes a paramDesc, which identifies
* the parameter to be voted on. If the contract has a single paramMgr, the
* paramDesc can be just the parameter name. If the contract has more than one,
* then it must accept enough details to identify the paramMgr and parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to identify the paramMgr by the paramName doesn't seem right. What if there are multiple paramNames that are the same? I think it would be better for the governed contract to return an object with string keys and ParamManagerFull values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to identify the paramMgr by the paramName doesn't seem right.

That's correct.

Each paramName needs to be unique for each paramManager. If there's only one paramManager, the paramManagerAccessor doesn't have to require a specifier that includes which paramManager. When there are multiple paramManagers that might have overlapping paramNames, then the paramDesc has to specify both.

I think it would be better for the governed contract to return an object with string keys and ParamManagerFull values.

Yeah, that would be better if it would work. The Treasury distinguishes paramManagers for the vault params by the collateralType. Using the collateralType as a parameter to a function seemed better than a string key for a record.

*/
const start = async zcf => {
const zoe = zcf.getZoeService();
let registrar;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the registrar instance be in the terms, and can we confirm that the registrar public facet passed in, is associated with that instance going both directions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use a tightly-held facet of the Registrar to limit who can create new questions, so it can't be included in the terms.

I'm now getting the registrar instance from an invitation, so we have Zoe's guarantee for that connection.

(https://github.com/Agoric/agoric-sdk/issues/3449), which allows you to examine
the souce.

The governedContract will provide the registrar, which allows you to check the
Copy link
Contributor

Choose a reason for hiding this comment

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

How does a user check the electorate? As far as I can tell, the governedContract gives me an instance for a registrar that may or may not actually be the instance associated with the passed-in registrarCreatorFacet. So I could be misled to another instance altogether. Putting that aside, let's say I get the installation for the registrar instance, and that it's for the committee registrar. How do I know who is voting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a correct analysis.

The governedContract (or other things) can indicate an alleged Registrar. The validation has to come from the other end.

The committeeRegistrar is a particular Registrar that doesn't itself provide any validation for who the committee members are. A contract that was started knowing the Registrar will trust it. Others might have access to the bootstrap.js that created it, or find it in a public registry that they have reason to trust. More often, we'll use other kinds of Registrars that provide more legibility for how they were constituted.

Once you know a registrar that you trust (or that you are reliant on), you can follow it back to its installation and review the code or check with a trusted reviewer. The Registrar itself will tell you about elections it manages, which gives you the ballot details and the ballot counter, so you can tell how the election will be run and outcome determined.

@erights erights self-requested a review July 23, 2021 00:36
@katelynsills katelynsills self-requested a review July 23, 2021 00:37
Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

Whoops, realized I had intended to "request changes" rather than comment on my previous review. This review is just fixing that status.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

I just added myself as a reviewer in order to give feedback about the Fred problem. When we were first discussing governance, it was immediately clear to me that the Fred problem was the central challenge in designing a governance system, so that invites would still be meaningful assets even when issued by a governed contract. Unfortunately we have not established clear methodology about how to evaluate whether a contract has adequately addressed the Fred problem. The methodology here, using prose descriptions to establish that it is possible for Fred to reliably examine an invitation, should always have been suspect. But when the Fred issue gets as deep as this, "solving" it in prose is clearly inadequate.

Back in the days of the covered call running on the contractHost, you introduced an essential observation: The author of a contract is the one who is in the best position to author some helpful predicates for Fred to use, to approve or disapprove an invite. Let's call these "fredicates". (Ok, I don't expect anyone to call these that but me ;).) Each fredicate represents the contract author's theory about what Fred might want, such that Fred can express a desire-that-fits-the-theory by parameterizing the fredicate that embodies that theory. You wrote such a fredicate for that old covered call contract.

My first feedback on this PR is to request at least one such fredicate. With a fredicate, we do not need prose describing what Fred would check. The code itself expresses that. What we do need is a description of what Fred wants, such that

  • Fred expresses this want by a parameterization of a fredicate, and
  • Alice cannot construct an invite to send to Fred that simultaneously passes the fredicate and fails to satisfy Fred's desire that this parameterization was supposed to represent.

In other words, the fredicate represents a good attack target. Attacking the contract will often start with an attack on the fredicate.

@Chris-Hibbert Chris-Hibbert marked this pull request as draft July 23, 2021 21:54
Copy link
Contributor Author

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I addressed the comments. I'd be happy to see reactions to my responses, but I've changed the PR state to Draft, as it's not ready for final review at this point.

packages/governance/src/contractGovernor.js Outdated Show resolved Hide resolved
packages/governance/src/contractGovernor.js Show resolved Hide resolved
const voteOnParamChange = async (
paramName,
proposedValue,
ballotCounterInstallation,
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'm hoping/expecting contractGovernor can be canonical, but I expect there will be multiple ballotCounters. It's true that contractGovernor expects a binary counter, but I don't think it's unreasonable for there to be improvements on the first one. Leaving this to be set when each governed contract starts up makes it easier to migrate.

packages/governance/src/governParam.js Outdated Show resolved Hide resolved
paramName,
proposedValue,
ballotCounterInstallation,
closingRule,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

*/
const start = async zcf => {
const zoe = zcf.getZoeService();
let registrar;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use a tightly-held facet of the Registrar to limit who can create new questions, so it can't be included in the terms.

I'm now getting the registrar instance from an invitation, so we have Zoe's guarantee for that connection.

packages/governance/src/governParam.js Outdated Show resolved Hide resolved
Comment on lines 96 to 98
...binaryBallotDetails,
instance: ballotCounter,
details,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The details result is the place where the ballot creator learns the ballot handle and the counterInstance when they care about that. All the voters will be notified of the new question, but this is the creator's chance to find out.

The distinction for BinaryBallotDetails evaporated. It's just BallotDetails now.

ballotCounterInstance it is. I'll look for more places to add that kind of clarity.

packages/governance/src/governParam.js Show resolved Hide resolved
packages/governance/src/governParam.js Show resolved Hide resolved
@Chris-Hibbert Chris-Hibbert force-pushed the contractGovernanceMgr-3185 branch 2 times, most recently from 16af4da to cb88451 Compare August 3, 2021 22:23
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Just some preliminary comments while I absorb orienting material. Since I am not yet oriented, some may be based on my misunderstandings.

packages/governance/README.md Outdated Show resolved Hide resolved
packages/governance/README.md Outdated Show resolved Hide resolved
packages/governance/README.md Outdated Show resolved Hide resolved
packages/governance/README.md Outdated Show resolved Hide resolved
packages/governance/docs/contracts.puml Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Still working my way through. Sorry it is taking so long.

packages/governance/src/ballotBuilder.js Outdated Show resolved Hide resolved
packages/governance/src/types.js Outdated Show resolved Hide resolved
packages/governance/src/paramManager.js Outdated Show resolved Hide resolved
packages/governance/src/paramManager.js Outdated Show resolved Hide resolved
packages/governance/src/ballotBuilder.js Outdated Show resolved Hide resolved
packages/governance/src/governParam.js Show resolved Hide resolved
Chris-Hibbert pushed a commit that referenced this pull request Aug 25, 2021
erights added a commit that referenced this pull request Aug 25, 2021
Copy link
Contributor Author

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I responded to some of your comments. I continue to proceed through them.

packages/governance/docs/contracts.puml Outdated Show resolved Hide resolved
packages/governance/src/ballotBuilder.js Outdated Show resolved Hide resolved
@Chris-Hibbert Chris-Hibbert force-pushed the contractGovernanceMgr-3185 branch 2 times, most recently from e16874f to 3badae2 Compare August 26, 2021 00:26
@Chris-Hibbert Chris-Hibbert force-pushed the contractGovernanceMgr-3185 branch 2 times, most recently from 8ebabda to 20e1ec6 Compare September 16, 2021 00:51
@Chris-Hibbert Chris-Hibbert force-pushed the contractGovernanceMgr-3185 branch 2 times, most recently from afd01b5 to e3fa924 Compare September 16, 2021 22:28
@Chris-Hibbert
Copy link
Contributor Author

@dckc following a recent discussion, I added a few CRUCIAL markers to areas of code that are handling delicate capabilities, or carefully ensuring important properties.

Comment on lines +271 to 272
* @property {ERef<Timer>} timer
* @property {Timestamp} deadline
Copy link
Member

Choose a reason for hiding this comment

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

as we just discovered, deadlines are RelativeTimes.

Suggested change
* @property {ERef<Timer>} timer
* @property {Timestamp} deadline
* @property {ERef<Timer>} timer
* @property {RelativeTime} deadline

* @param {ParamSpecification} paramSpec
* @param {ParamValue} proposedValue
* @param {Installation} voteCounterInstallation
* @param {bigint} deadline
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {bigint} deadline
* @param {RelativeTime} deadline

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

This looks ready to be the next step in development of our governance framework.

@katelynsills , @erights , over to you to conclude your reviews.

The design looks solid (the diagrams are quite helpful) and the argument for correctness is reasonably clear. I look forward to the integration into the treasury etc.

During my review I locally added static types to the tests, which helped me understand a bunch of details such as QuestionSpec vs QuestionDetails and how they flow. @Chris-Hibbert and I agreed that's cost effective to keep and maintain, so I plan to land it in a follow-on PR.

p.s. The Verifying Governance in ContractGovernor discussion document was crucial to my understanding of the argument for correctness. The README here has much of it, such as Examining a Contract before use, but I have read it all too many times by now to be sure that nothing worth preserving in agoric-sdk wasn't copied over.

Comment on lines 27 to 29
counting contract. The QuestionSpec consists of `{ choiceMethod, issue,
positions, electionType, maxChoices }`. The issue and positions can be
strings or structured objects. ChoiceMethod is one of UNRANKED and ORDER, which
Copy link
Member

Choose a reason for hiding this comment

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

The code uses method, not choiceMethod.

Suggested change
counting contract. The QuestionSpec consists of `{ choiceMethod, issue,
positions, electionType, maxChoices }`. The issue and positions can be
strings or structured objects. ChoiceMethod is one of UNRANKED and ORDER, which
counting contract. The QuestionSpec consists of `{ method, issue,
positions, electionType, maxChoices }`. The issue and positions can be
strings or structured objects. Method is one of UNRANKED and ORDER, which

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

connected to so that voters can verify that their votes mean what they say and
will be tabulated as expected.

Any occasion of governance starts with the creation of a Registrar. Two kinds
Copy link
Member

Choose a reason for hiding this comment

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

@Chris-Hibbert writes in #3869 (comment)

I wrote a note-to-self on finding something public-facing to rename to be the electorate.

I'm putting it here so that maybe github will help us track it.

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 couldn't find a way to distinguish between Registrar and Electorate, so I just renamed everything. The committeeRegistrar became just committee

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Hi @Chris-Hibbert most sorry this took so long. AFAICT all my issues that needed to be resolved are. LGTM!!!!

@dckc I wanted to mention to you how extraordinarily valuable your UI was for deepening my understanding. After many hours of reading the code, talking to @Chris-Hibbert (recording somewhere) and looking at abstraction diagrams, within three minutes of interacting with your UI mockup, suddenly all these distinction were tangible and made complete sense. The many hours were not unusual or unique to this subject area. The three minutes of immediate clarification were. It was an important lesson.

In light of that, @Chris-Hibbert I now see that the particular abstraction boundaries and composition choices you made have a rightness to them that I'm sorry I was struggling to appreciate when you were explaining them to me. Starting from this division into compositional concepts, we are in a more powerful position to assemble novel governance arrangements than I expected. Not that they're perfect ;) . But they are a great starting point to iterate from. I'm happy to see them merged now. Thanks!

In order to make the management of parameters visible to clients of the
contract, it should
* validate that the declaration of the parameters is included in its terms,
* publish the accessor facet of the paramManager in its publicFacet, and
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this "accessor facet" terminology is now stale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@Chris-Hibbert Chris-Hibbert force-pushed the contractGovernanceMgr-3185 branch 2 times, most recently from 8554897 to 8b17382 Compare September 30, 2021 22:12
ContractGovernor is an ElectionManager that knows its Registrar, and
starts up and manages one contract. The managed contract returns a
facet that describes the managed parameters to the governor. The
ContractGovernor returns a facet that allows the creation of new Ballot
questions to ask the electorate to approve changing the values of
those parameters.

There's a README.md that gives on overview of the interdependent
APIs.

The swingsetTest exercise the ContractGovernor over a trivial
governedContract that shows how a contract can configure its
parameters, and the methods that have to be available in order for
everything to be found and verified. The tests include a verification
step that cross-checks the Registrar, ElectionManager (i.e.
ContractGovernor in this case), and questions to see that they are
consistent.

add a helper for governedContracts
add index.js
add comments for CRUCIAL governance security attributes
@Chris-Hibbert Chris-Hibbert dismissed katelynsills’s stale review September 30, 2021 23:22

Dean said that he and @erights agreed that Kate's review is no longer blocking this merge.

@Chris-Hibbert Chris-Hibbert merged commit 59ebde2 into master Sep 30, 2021
@Chris-Hibbert Chris-Hibbert deleted the contractGovernanceMgr-3185 branch September 30, 2021 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update a contract to rely on managed parameters (Governance)
5 participants