drop internalCreatorFacet from contract governance (#4164)

* refactor: drop internalCreatorFacet from contract governance

I added the internalCreatorFacet as a result of my confusion.

The intent was that the limitedCreatorFacet would be the creatorFacet
provided by the contract with access to the contractGovernor added. I
forgot to interpolate the methods of the original creatorFacet into
the limitedCreatorFacet.

When I needed access to those methods later, I added the
internalCreatorFacet.

I've updated the contractGovernance diagram to show the methods.

contractGovernor starts the governedContract, and gets the powerful
creatorFacet. It uses the powerful methods itself, and passes only the
limited facet on to its caller.

* docs: clarify some governance requirements

a comment in contractGovernor said governed contract in place of contractGovernor

in contractHelper.js add API doc saying that the governed contract
shouldn't use the powerful facets

in AMM: reduce scope of creatorFacet by creating it after publicFacet

also some other cleanups
 * rename make*Facet to wrap*Facet

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

packages/governance/README.md

@@ -65,7 +65,7 @@ voterHandle.
 ## ContractGovernor
 
 We want some contracts to be able to make it visible that their internal
-parameters can be controlled by a public process, and allow observers to see who
+parameters are controlled by a public process, and allow observers to see who
 has control, and how those changes can happen. To do so, the contract would
 use a ParamManager to hold its mutable state. ParamManager has facets for
 accessing the param values and for setting them. The governed contract would use
@@ -75,7 +75,7 @@ values, is only accessible to a visible ContractGovernor. The ContractGovernor
 makes the Electorate visible, while tightly controlling the process of
 creating new questions and presenting them to the electorate.
 
-The governor starts up the Contract and can see what params are subject to
+The governor starts up the Contract and can see what parameters are subject to
 governance. It provides a private facet that carries the ability to request
 votes on changing particular parameters. Some day we may figure out how to make
 the process and schedule of selecting parameter changes to vote on also subject
@@ -102,6 +102,16 @@ contract would also make the read-only facet visible, so others can see the
 current values. The initial values of the parameters, along with their types
 remain visible in the contract's terms.
 
+By using ContractHelper, the governed contract returns a (powerful) creatorFacet
+with two methods: `getParamMgrRetriever` (which provides access to read and
+modify parameters), and `getLimitedCreatorFacet`, which has the creator facet
+provided by the governed contract and doesn't include any powerful governance
+capabilities. ContractGovernor starts the governed contract, so it gets the
+powerful creatorFacet. ContractGovernor needs access to the paramManager, but
+shouldn't share it. So the contractGovernor's creatorFacet provides access to
+the governed contract's publicFacet, creatorFacet, instance, and
+`voteOnParamChange()`, which the contract's owner should treat as powerful. 
+
 ### ParamManager
 
 `ContractGovernor` expects to work with contracts that use `ParamManager` to
@@ -149,7 +159,7 @@ governor's public facet will also refer to the contract it governs. Once you
 have the instance you can retrieve the installation from Zoe which allows you to
 examine the source.
 
-The governedContract will provide the electorate, which allows you to check the
+The governed contract will provide the electorate, which allows you to check the
 electorate, and retrieve a list of open questions. (We should add closed
 questions and their resolution as well.) Each question refers to the
 voteCounter it uses.

packages/governance/docs/contractGovernance.puml

@@ -24,13 +24,15 @@ class "ContractGovernor\n(an ElectionManager)" as ContractGovernor {
     <i>verifiable</i>: governedInstance, electorateInstance
     --
     +getElectorate()
-    +getGovernedContract()
+    +getGovernedContract() - the governed instance
     +validateVoteCounter()
     +validateElectorate()
     +validateTimer()
-    -startGovernedInstance(electorate, governed, ...)
+    --
+    -voteOnParamChange()
+    -getCreatorFacet() - The unrestricted part of the governed contract's creatorFacet
 }
-note left : ContractGovernor starts GovernedContract\nstartGovernedInstance() returns a tightly held facet\n   with voteOnParamChange() for the creator.
+note left : ContractGovernor starts GovernedContract.\nvoteOnParamChange() should be tightly held.\ngetCreatorFacet() is for the contract's creator.
 
 class Electorate {
     Questions

packages/governance/src/contractGovernor.js

@@ -114,7 +114,7 @@ const start = async (zcf, privateArgs) => {
     X`questionPoserInvitation didn't match supplied Electorate`,
   );
 
-  // CRUCIAL: only governedContract should get the ability to update params
+  // CRUCIAL: only contractGovernor should get the ability to update params
   /** @type {Promise<LimitedCreatorFacet>} */
   const limitedCreatorFacet = E(governedCF).getLimitedCreatorFacet();
 
@@ -153,7 +153,6 @@ const start = async (zcf, privateArgs) => {
   const creatorFacet = Far('governor creatorFacet', {
     voteOnParamChange,
     getCreatorFacet: () => limitedCreatorFacet,
-    getInternalCreatorFacet: () => E(governedCF).getInternalCreatorFacet(),
     getInstance: () => governedInstance,
     getPublicFacet: () => governedPF,
   });

packages/governance/src/contractHelper.js

@@ -11,9 +11,14 @@ const { details: X, quote: q } = assert;
  * Helper for the 90% of contracts that will have only a single set of
  * parameters. In order to support managed parameters, a contract only has to
  *   * define the parameter template, which includes name, type and value
- *   * call handleParamGovernance() to get makePublicFacet and makeCreatorFacet
+ *   * call handleParamGovernance() to get wrapPublicFacet and wrapCreatorFacet
  *   * add any methods needed in the public and creator facets.
  *
+ *   It's also crucial that the governed contract not interact with the product
+ *   of wrapCreatorFacet(). The wrapped creatorFacet has the power to change
+ *   parameter values, and the governance guarantees only hold if they're not
+ *   used directly by the governed contract.
+ *
  *  @type {HandleParamGovernance}
  */
 const handleParamGovernance = (zcf, governedParamsTemplate) => {
@@ -30,7 +35,7 @@ const handleParamGovernance = (zcf, governedParamsTemplate) => {
   );
   const paramManager = buildParamManager(governedParams);
 
-  const makePublicFacet = (originalPublicFacet = {}) => {
+  const wrapPublicFacet = (originalPublicFacet = {}) => {
     return Far('publicFacet', {
       ...originalPublicFacet,
       getSubscription: () => paramManager.getSubscription(),
@@ -42,24 +47,26 @@ const handleParamGovernance = (zcf, governedParamsTemplate) => {
     });
   };
 
-  /** @type {LimitedCreatorFacet} */
-  const limitedCreatorFacet = Far('governedContract creator facet', {
-    getContractGovernor: () => electionManager,
-  });
+  const makeLimitedCreatorFacet = originalCreatorFacet => {
+    return Far('governedContract creator facet', {
+      ...originalCreatorFacet,
+      getContractGovernor: () => electionManager,
+    });
+  };
 
-  const makeCreatorFacet = (originalCreatorFacet = Far('creatorFacet', {})) => {
+  const wrapCreatorFacet = (originalCreatorFacet = Far('creatorFacet', {})) => {
+    const limitedCreatorFacet = makeLimitedCreatorFacet(originalCreatorFacet);
     return Far('creatorFacet', {
       getParamMgrRetriever: () => {
         return Far('paramRetriever', { get: () => paramManager });
       },
-      getInternalCreatorFacet: () => originalCreatorFacet,
       getLimitedCreatorFacet: () => limitedCreatorFacet,
     });
   };
 
   return harden({
-    makePublicFacet,
-    makeCreatorFacet,
+    wrapPublicFacet,
+    wrapCreatorFacet,
     getParamValue: name => paramManager.getParam(name).value,
   });
 };

packages/governance/src/types.js

@@ -501,8 +501,6 @@
  *   facet of the governed contract, without the tightly held ability to change
  *   param values.
  * @property {() => any} getPublicFacet - public facet of the governed contract
- * @property {() => any} getInternalCreatorFacet - powerful creator facet of the
- *   governed contract
  * @property {() => Promise<Instance>} getInstance - instance of the governed
  *   contract
  */
@@ -519,27 +517,30 @@
 
 /**
  * @typedef {Object} GovernedCreatorFacet
- * @property {() => ParamManagerRetriever} getParamMgrRetriever
- * @property {() => any} getInternalCreatorFacet
- * @property {() => LimitedCreatorFacet} getLimitedCreatorFacet
+ * @property {() => ParamManagerRetriever} getParamMgrRetriever - allows accessing
+ *   and updating governed parameters. Should only be directly accessible to the
+ *   contractGovernor
+ * @property {() => LimitedCreatorFacet} getLimitedCreatorFacet - the creator
+ *   facet of the governed contract. Doesn't provide access to any governance
+ *   functionality
  */
 
 /**
- * @callback MakePublicFacet
+ * @callback WrapPublicFacet
  * @param {any} originalPublicFacet
  * @returns {GovernedPublicFacet}
  */
 
 /**
- * @callback MakeCreatorFacet
+ * @callback WrapCreatorFacet
  * @param {any} originalCreatorFacet
  * @returns {GovernedCreatorFacet}
  */
 
 /**
  * @typedef {Object} ParamGovernorBundle
- * @property {MakePublicFacet} makePublicFacet
- * @property {MakeCreatorFacet} makeCreatorFacet
+ * @property {WrapPublicFacet} wrapPublicFacet
+ * @property {WrapCreatorFacet} wrapCreatorFacet
  * @property {(name: string) => ParamValue} getParamValue
  */
 

packages/governance/test/swingsetTests/contractGovernor/governedContract.js

@@ -7,14 +7,14 @@ const MALLEABLE_NUMBER = 'MalleableNumber';
 /** @type {ContractStartFn} */
 const start = async zcf => {
   const { main: initialValue } = zcf.getTerms();
-  const { makePublicFacet, makeCreatorFacet } = handleParamGovernance(
+  const { wrapPublicFacet, wrapCreatorFacet } = handleParamGovernance(
     zcf,
     harden(initialValue),
   );
 
   return {
-    publicFacet: makePublicFacet({}),
-    creatorFacet: makeCreatorFacet({}),
+    publicFacet: wrapPublicFacet({}),
+    creatorFacet: wrapCreatorFacet({}),
   };
 };
 

packages/treasury/test/test-stablecoin.js

@@ -185,7 +185,7 @@ async function setupAmm(
     electorateCreatorFacet: committeeCreator,
   });
 
-  const ammCreatorFacetP = E(ammGovernorCreatorFacet).getInternalCreatorFacet();
+  const ammCreatorFacetP = E(ammGovernorCreatorFacet).getCreatorFacet();
   const ammPublicP = E(ammGovernorCreatorFacet).getPublicFacet();
 
   const [ammCreatorFacet, ammPublicFacet] = await Promise.all([

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

@@ -114,8 +114,8 @@ const start = zcf => {
   assert(centralBrand !== undefined, X`centralBrand must be present`);
 
   const {
-    makePublicFacet,
-    makeCreatorFacet,
+    wrapPublicFacet,
+    wrapCreatorFacet,
     getParamValue,
   } = handleParamGovernance(zcf, makeInitialValues(poolFeeBP, protocolFeeBP));
   const getPoolFeeBP = () => getParamValue(POOL_FEE_KEY);
@@ -211,15 +211,10 @@ const start = zcf => {
     protocolSeat,
     centralBrand,
   );
-  const creatorFacet = makeCreatorFacet(
-    Far('AMM Fee Collector facet', {
-      makeCollectFeesInvitation,
-    }),
-  );
 
   /** @type {XYKAMMPublicFacet} */
-  // @ts-ignore makePublicFacet includes all the methods that are passed in.
-  const publicFacet = makePublicFacet(
+  // @ts-ignore wrapPublicFacet includes all the methods that are passed in.
+  const publicFacet = wrapPublicFacet(
     Far('AMM public facet', {
       addPool,
       getPoolAllocation,
@@ -240,6 +235,11 @@ const start = zcf => {
     }),
   );
 
+  const creatorFacet = wrapCreatorFacet(
+    Far('AMM Fee Collector facet', {
+      makeCollectFeesInvitation,
+    }),
+  );
   return harden({ publicFacet, creatorFacet });
 };
 

packages/zoe/test/unitTests/contracts/vpool-xyk-amm/test-amm-governance.js

@@ -134,7 +134,7 @@ const setupServices = async (
     }),
   );
 
-  const ammCreatorFacetP = E(governorCreatorFacet).getInternalCreatorFacet();
+  const ammCreatorFacetP = E(governorCreatorFacet).getCreatorFacet();
   const ammPublicP = E(governorCreatorFacet).getPublicFacet();
 
   const [ammCreatorFacet, ammPublicFacet] = await Promise.all([

packages/zoe/test/unitTests/contracts/vpool-xyk-amm/test-xyk-amm-swap.js

@@ -155,7 +155,7 @@ const setupServices = async (
     }),
   );
 
-  const ammCreatorFacetP = E(governorCreatorFacet).getInternalCreatorFacet();
+  const ammCreatorFacetP = E(governorCreatorFacet).getCreatorFacet();
   const ammPublicP = E(governorCreatorFacet).getPublicFacet();
 
   const [ammCreatorFacet, ammPublicFacet] = await Promise.all([