-
Notifications
You must be signed in to change notification settings - Fork 212
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
chore: rough external governor #3453
chore: rough external governor #3453
Conversation
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.
@@ -29,7 +29,7 @@ const parametersAreGoverned = async (zoe, governedInstance, mgr, name) => { | |||
return termsKeys.every(k => governedKeys.includes(k)); | |||
}; | |||
|
|||
/* | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just needed two ** to work
const { electionManager, governedParams } = zcf.getTerms(); | ||
const governorPublic = E(zcf.getZoeService()).getPublicFacet(electionManager); | ||
/** @type {{ governor: Instance, governedParams: Array<string> }} */ | ||
const { governor, governedParams } = zcf.getTerms(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract that creates this contract would pass in its own instance in the terms here.
}); | ||
|
||
const creatorFacet = Far('creator facet of governed contract', { | ||
getContractGovernor: () => gCreator, | ||
getParamManager: () => paramManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The creator, the contract that creates this instance, would be the only one to get access to the paramManager
X`Terms must include ${MALLEABLE_NUMBER}`, | ||
); | ||
|
||
const gCreator = await E(governorPublic).governContract( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract developer wouldn't have to know anything about governance, just that they should define which parameters are governable and expose the paramManager and the governor instance.
const start = async zcf => { | ||
const zoe = zcf.getZoeService(); | ||
|
||
const startGovernedInstance = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that unlike packages/governance/src/contractGovernor.js, this would only govern the one contract instance.
) => { | ||
const { creatorFacet, instance: governedInstance } = await E( | ||
zoe, | ||
).startInstance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The governed contract is started within this "governor" contract
const paramManager = E(creatorFacet).getParamManager(); | ||
|
||
// connect the vote outcome to the paramManager | ||
const setParamByVoting = setupGovernance(paramManager, registrar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass the paramManager to an imported function that allows us to set a parameter through voting
// connect the vote outcome to the paramManager | ||
const setParamByVoting = setupGovernance(paramManager, registrar); | ||
return Far('setParamByVoting', { | ||
setParamByVoting, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we expose this as the return value of the startGovernedInstance
call
|
||
import { ChoiceMethod } from '../../../src/ballotBuilder'; | ||
|
||
const setupGovernance = (paramManager, registrar) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is intended to be importable and reusable
import { ChoiceMethod } from '../../../src/ballotBuilder'; | ||
|
||
const setupGovernance = (paramManager, registrar) => { | ||
const params = E(paramManager).getParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, needs an await here
* @param {ClosingRule} closingRule | ||
*/ | ||
const setParamByVoting = async (name, proposedValue, closingRule) => { | ||
assert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should reject invalid values and names before any voting happens
); | ||
/** @type {ParamDescription["type"]} */ | ||
// TODO: use paramDescription.type to actually enforce the type | ||
// const paramType = params[name].type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great if the paramManager had importable validating functions per type.
// TODO: use paramDescription.type to actually enforce the type | ||
// const paramType = params[name].type; | ||
|
||
const positivePosition = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the positions and questions need to be records, maybe with text somehow indicated. This should be informed by how the wallet UI would display voting, which should replace presences with the user's petname for that presence.
closingRule, | ||
}; | ||
|
||
const outcomeP = E(registrar).askQuestion(binaryBallotDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fanciful, but I was wondering if we could present a simpler API, such that there's just one object (here, called registrar
) that we call askQuestion
on with all of the ballot details. We have a number of different pieces, and it seems like most of them are being exposed to the user, but can some pieces encapsulate others so that the external interface is simpler?
f22bdb4
to
6e1a3b5
Compare
Here's a quick outline of what the governed contract might look like if it only exposes a paramManager and is instantiated in another contract, the governor.
I'll add comments in this PR explaining certain parts, but this code does not run.