Manage parameters for a contract #3208

Chris-Hibbert · 2021-05-28T22:38:21Z

outstanding issues:

should this validate types?
is there a clean way to assert something is an amount?
should managed amounts (and ratios) only set the value and leave
the brand to the managed contract?
are there others types to add or remove?

dtribble

It doesn't have actions against these changes or notification of the changes. I need to see the context to comment further.

dtribble · 2021-06-09T17:05:22Z

packages/governance/src/param-manager.js

+  STRING: 'string',
+  RATIO: 'ratio',
+  AMOUNT: 'amount',
+  BRAND: 'brand',


do we need an "instance" or "installation" reference? We could just use boardID for that, but might want direct support for "boardID" usage.

based on @katelynsills' comments, I think these will be handles.

I'll add a case for handles and one for untyped ANY.

Yes, we should only ever use boardIds where we can't pass objects. So anything on-chain should not be using a boardId and should use the handle instead.

added instance and installation as (currently unverified) types. No sign of boardIDs.

dtribble · 2021-06-09T17:48:58Z

packages/governance/src/param-manager.js

+  const types = makeStore('name');
+
+  paramDesc.forEach(({ name, value, type }) => {
+    assert.typeof(name, 'string');


Seems unnecessary given that looking it up will do the same assert.

katelynsills

Some small suggestions, most of which are optional. Looks really good!

katelynsills · 2021-06-16T01:28:19Z

packages/governance/package.json

+  "parsers": {
+    "js": "mjs"
+  },
+  "main": "src/missing.js",


Can we add the actual file here?

I'll update it to param-manager.js for now, but it should eventually be the registrar or something else. This won't be the most important thing for long.

katelynsills · 2021-06-16T01:29:14Z

packages/governance/package.json

+  },
+  "main": "src/missing.js",
+  "engines": {
+    "node": ">=11.0"


I think this should be "node": ">=14.15.0", since 11 won't actually work, as far as I know.

Suggested change

"node": ">=11.0"

"node": ">=14.15.0"

done. Both treasury and zoe still have 11.0. I didn't check anywhere else.

katelynsills · 2021-06-16T01:33:30Z

packages/governance/package.json

+    "@agoric/notifier": "^0.3.14",
+    "@agoric/promise-kit": "^0.2.13",
+    "@agoric/store": "^0.4.14",
+    "@agoric/swingset-vat": "^0.17.2",


I don't think this is a dependency. Could you check the rest to make sure they are used?

I removed everything that isn't currently in use. I'll add them individually as I introduce dependencies.

katelynsills · 2021-06-16T01:33:38Z

packages/governance/package.json

+  "dependencies": {
+    "@agoric/assert": "^0.3.0",
+    "@agoric/bundle-source": "^1.3.7",
+    "@agoric/captp": "^1.7.13",


I don't think this is a dependency

katelynsills · 2021-06-16T01:34:05Z

packages/governance/package.json

+    "@agoric/install-ses": "^0.5.13",
+    "ava": "^3.12.1",
+    "esm": "^3.2.25",
+    "ses": "^0.12.7"


Is this a dependency? I don't see it used anywhere

katelynsills · 2021-06-16T01:59:19Z

packages/governance/src/param-manager.js

+    },
+  };
+
+  return { publicFacet, manager };


I'm torn on this name, since it's potentially confused with the contract instance publicFacet. But on the other hand, it might be nice for people to be familiar with the pattern.

I was confused by it, worrying that it would need to be accessed async. So I advocate read or params.

I changed to params.

katelynsills · 2021-06-16T02:00:22Z

packages/governance/src/types.js

+
+/**
+ * @typedef  {Object} ParamManager
+ * @property {(name: string, value: any) => void} update


If we restrict types, we could tighten up this any to be the types we support. This would help developers a lot because they could find bugs statically before deploying (or testing locally, I suppose).

Suggested change

* @property {(name: string, value: any) => void} update

* @property {(name: string, value: ParamValue) => void} update

If I thought we had gotten everything that devs might want to put in a contract, I would be in favor of this, but I think devs are going to need an escape hatch. Having the explicit ParamType.ANY lets them clearly escape our limits. We should try to be welcoming to new suggestions, but there's clearly going to be a release cycle before we can get them support for new types.

katelynsills · 2021-06-16T02:01:25Z

packages/governance/src/types.js

+
+/**
+ * @typedef  {Object} ParamManagerPublic
+ * @property {(name: string) => any} lookup


Suggested change

* @property {(name: string) => any} lookup

* @property {(name: string) => ParamValue} lookup

katelynsills · 2021-06-16T02:01:56Z

packages/governance/src/types.js

+ *             }} ParamDescription
+ */
+
+// type: string above should be type: ParamType


Huh, why not just use ParamType as the type rather than string?

typescript was fighting with me. I got it this time.

katelynsills · 2021-06-16T02:02:09Z

packages/governance/src/types.js

+
+/**
+ * @typedef  {{name: string,
+ *             value: any,


Suggested change

* value: any,

* value: ParamValue,

dtribble

I think publicFacet needs to be changed, but the rest of discussion.

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

packages/governance/src/param-manager.js

+  ANY: 'any',
+  BIGINT: 'bigint',
+  BRAND: 'brand',
+  HANDLE: 'handle',


Those seems plausible, assuming Zoe is the manager of such things. It does make the test async, though, which introduces a lot of complexity.

dtribble · 2021-06-16T05:50:26Z

packages/governance/src/param-manager.js

+    case ParamType.AMOUNT:
+      // TODO(hibbert): is there a clean way to assert something is an amount?
+      // An alternate approach would be to say the contract knows the brand and
+      // the managed parameter only needs to supply the value.


I agree. We should stick with Amounts, and just e.g., assert that they are of a particular Brand if that's relevant.

dtribble · 2021-06-16T05:51:34Z

packages/governance/src/param-manager.js

+      // An alternate approach would be to say the contract knows the brand and
+      // the managed parameter only needs to supply the value.
+      assert(
+        AmountMath.isEqual(value, value),


I prefer the non-allocaating version in asserts :). a coerce often allocates

dtribble · 2021-06-16T05:55:33Z

packages/governance/src/param-manager.js

+    },
+  };
+
+  return { publicFacet, manager };


I was confused by it, worrying that it would need to be accessed async. So I advocate read or params.

dtribble · 2021-06-16T06:00:10Z

packages/governance/test/test-param-manager.js

+      type: ParamType.NAT,
+    },
+  ]);
+  t.is(publicFacet.lookup('number'), 13);


yeah params looks like a good name for that thing :)

katelynsills · 2021-06-16T23:36:39Z

Adding my feedback from the discussion this morning based on what @dtribble brought up:

We should change the manager API to allow for POLA patterns by default. Instead of holding the manager meaning that you can change every parameter that you know of (or can guess the name of), it should be easy to attenuate the manager object such that only one parameter or groups of parameters can be changed.

During the meeting, we discussed a few ways to do this. The first option was return one manager object per parameter. I might be misunderstanding this, but this seems like it would create a ton of objects unnecessarily, and it would be difficult to recombine the objects to allow groups of parameters to be changed at once. I would suggest a second option, which is to return one manager object, but instead of update(name, value), the manager object has methods for updating each name:

const manager = {
   updateFeeRatio: value => ...,
   updateContractName: value => ...,
}

Then in the documentation we can explain how to attenuate:

const attenuatedManager = {
  updateFeeRatio: manager.updateFeeRatio,
};

We should optimize for the least number of lookups when reading the values, so that UI and other downstream clients don't need to query the chain to look up a value. This could mean having a function that returns a JavaScript object:

publicFacet.getParams();

// returns
{ 
    myName: {
        name: 'myName',
        type: 'string',
        value: 'Alice',
    },
   anotherName: {
....

rather than definedNames or lookup

Chris-Hibbert

I didn't remove ParamType.ANY. LMK if we don't need to leave this escape hatch for 3rd party devs.

Chris-Hibbert · 2021-06-16T16:33:37Z

packages/governance/jsconfig.json

+    "strictNullChecks": true,
+    "moduleResolution": "node",
+  },
+  "include": ["src/**/*.js", "test/**/*.js", "exported.js", "globals.d.ts"],


thanks, good catch.

Chris-Hibbert · 2021-06-16T16:36:12Z

packages/governance/package.json

+  "parsers": {
+    "js": "mjs"
+  },
+  "main": "src/missing.js",


I'll update it to param-manager.js for now, but it should eventually be the registrar or something else. This won't be the most important thing for long.

Chris-Hibbert · 2021-06-16T16:36:41Z

packages/governance/package.json

+  },
+  "main": "src/missing.js",
+  "engines": {
+    "node": ">=11.0"


done. Both treasury and zoe still have 11.0. I didn't check anywhere else.

Chris-Hibbert · 2021-06-16T22:22:03Z

packages/governance/package.json

+    "@agoric/notifier": "^0.3.14",
+    "@agoric/promise-kit": "^0.2.13",
+    "@agoric/store": "^0.4.14",
+    "@agoric/swingset-vat": "^0.17.2",


I removed everything that isn't currently in use. I'll add them individually as I introduce dependencies.

Chris-Hibbert · 2021-06-16T22:23:28Z

packages/governance/package.json

+    "ses": "^0.12.7"
+  },
+  "files": [
+    "bundles/",


Chris-Hibbert · 2021-06-16T23:05:15Z

packages/governance/src/param-manager.js

+    },
+  };
+
+  return { publicFacet, manager };


I changed to params.

Chris-Hibbert · 2021-06-16T23:06:23Z

packages/governance/src/types.js

+
+/**
+ * @typedef  {Object} ParamManagerPublic
+ * @property {(name: string) => any} lookup


Chris-Hibbert · 2021-06-16T23:06:29Z

packages/governance/src/types.js

+
+/**
+ * @typedef  {{name: string,
+ *             value: any,


Chris-Hibbert · 2021-06-16T23:35:49Z

packages/governance/src/param-manager.js

+  STRING: 'string',
+  RATIO: 'ratio',
+  AMOUNT: 'amount',
+  BRAND: 'brand',


added instance and installation as (currently unverified) types. No sign of boardIDs.

Chris-Hibbert · 2021-06-16T23:36:10Z

packages/governance/src/param-manager.js

+    case ParamType.AMOUNT:
+      // TODO(hibbert): is there a clean way to assert something is an amount?
+      // An alternate approach would be to say the contract knows the brand and
+      // the managed parameter only needs to supply the value.


updated comment

katelynsills · 2021-06-17T00:02:16Z

packages/governance/src/paramManager.js

+harden(ParamType);
+
+const assertType = (type, value, name) => {
+  if (!type) {


Just pointing to this directly because I think the comment got lost.

I think this is unnecessary since we already have the ParamType.ANY which is handled below.

katelynsills · 2021-06-17T00:09:48Z

One last comment - instead of ParamType.ANY, should it be UNKNOWN? https://stackoverflow.com/a/57356103 A JSDoc/TypeScript type of unknown requires the recipient to type it before using, but any does not.

closes #3186 outstanding issues: * should this validate types? * is there a clean way to assert something is an amount? * should managed amounts (and ratios) only set the value and leave the brand to the managed contract? * are there others types to add or remove?

dependency reduction support instance and installation rather than handle support NAT type which must be a bigint rename publicFacet to params

change 'any' to 'unknown' drop an unneeded guard clause a single getParams() describes all parameters

Thanks to Michael Fig.

katelynsills

The most important changes are changing the dependencies, moving the keyword check up, and not creating new describer functions.

katelynsills · 2021-06-17T21:10:37Z

yarn.lock

@@ -2,6 +2,13 @@
 # yarn lockfile v1


+"@agoric/assert@^0.3.1":


Hmm, most of the changes in this yarn.lock shouldn't be happening. Before we merge this, can we make sure we've gotten the package dependencies right? For instance, an old version of Nat is being installed, and multiple Agoric packages are being added to the root yarn.lock even though they are already widely used.

katelynsills · 2021-06-17T21:17:30Z

packages/governance/test/test-param-manager.js

+  };
+  // @ts-ignore  illegal value for testing
+  t.throws(() => buildParamManager([stuffDescription]), {
+    message: 'unknown type guard "quote"',


This error message is kind of strange. Do we mean "unrecognized type"?

Suggested change

message: 'unknown type guard "quote"',

message: 'unrecognized type "quote"',

katelynsills · 2021-06-17T21:21:28Z

packages/governance/test/test-param-manager.js

+    value: 314159n,
+    type: ParamType.NAT,
+  };
+  const { getParams, updateNat: _updateNat, updateStuff } = buildParamManager([


Suggested change

const { getParams, updateNat: _updateNat, updateStuff } = buildParamManager([

const { getParams, updateStuff } = buildParamManager([

Seems to be unused. Is this intentional? If so, let's add a comment

It's unused, but I wanted it to be visible that updateNat is defined. Perhaps I'll just use it instead of making a big deal of it.

katelynsills · 2021-06-17T21:26:42Z

packages/governance/src/types.js

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


This type doesn't seem to be used at all. Seems like ParamDescription is the same thing and is the one used.

katelynsills · 2021-06-17T21:28:31Z

packages/governance/src/types.js

+ * @typedef {Object} ParamManagerBase
+ * @property {() => Record<Keyword,ParamDescription>} getParams
+ *
+ * @typedef {{ [updater: string]: (arg: any) => void }} ParamManagerUpdaters


Can this be:

Suggested change

* @typedef {{ [updater: string]: (arg: any) => void }} ParamManagerUpdaters

* @typedef {{ [updater: string]: (arg: ParamValue) => void }} ParamManagerUpdaters

katelynsills · 2021-06-17T21:31:43Z

packages/governance/src/paramManager.js

+    case ParamType.AMOUNT:
+      // It would be nice to have a clean way to assert something is an amount.
+      assert(
+        AmountMath.isEqual(value, value),


Is there a reason to use isEqual? AmountMath.coerce is the clean way to do it. We just don't have a brand to test against objectively, so the call becomes:

Suggested change

AmountMath.isEqual(value, value),

AmountMath.coerce(value.brand, value),

katelynsills · 2021-06-17T21:39:23Z

packages/governance/src/paramManager.js

+  INSTANCE: 'instance',
+  INSTALLATION: 'installation',


Could we test INSTANCE and INSTALLATION?

katelynsills · 2021-06-17T21:45:37Z

packages/governance/src/paramManager.js

+    assertType(type, value, name);
+    // we want to create function names like updateFeeRatio(), so we insist that
+    // the name has Keyword-nature.
+    assertKeywordName(name);


Nice! Can we move this up to line 86? I think we don't want to use the name as a property accessor into values until after this check.

katelynsills · 2021-06-17T21:48:23Z

packages/governance/src/paramManager.js

+    const describer = () => ({
+      name,
+      type,
+      value: values[name].value,
+    });
+    describers.push(describer);


It seems very strange to have a function per value for making descriptions. Is there a reason we need this? Or could we have a single function that knows how to turn values into what should be returned from getParams?

katelynsills · 2021-06-17T21:49:20Z

packages/governance/src/paramManager.js

+    describers.forEach(d => {
+      const description = d();
+      descriptions[description.name] = description;
+    });


I think we don't need describers and can just iterate over values directly. This is making and using a ton of functions.

good point. Done

dependency trimming better test for AMOUNT. improve some error messages narrow some type declarations add tests for INSTANCE and INSTALLATION

katelynsills

Approved, but please change esm to use the Agoric version before merging. Looks great!

katelynsills · 2021-06-18T00:11:51Z

yarn.lock

-esm@agoric-labs/esm#Agoric-built:
+esm@^3.2.25, esm@agoric-labs/esm#Agoric-built:


I think this should only be esm@agoric-labs/esm#Agoric-built

katelynsills · 2021-06-18T00:12:35Z

packages/governance/package.json

+  "devDependencies": {
+    "@agoric/install-ses": "^0.5.13",
+    "ava": "^3.12.1",
+    "esm": "^3.2.25"


Suggested change

"esm": "^3.2.25"

"esm": "agoric-labs/esm#Agoric-built",

Chris-Hibbert added enhancement New feature or request Small Governance Governance labels May 28, 2021

Chris-Hibbert added this to the Beta Phase 4: Governance milestone May 28, 2021

Chris-Hibbert requested a review from dtribble May 28, 2021 22:38

Chris-Hibbert assigned Chris-Hibbert and dtribble May 28, 2021

dtribble reviewed Jun 9, 2021

View reviewed changes

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

Chris-Hibbert force-pushed the gov-lib-3186 branch from 912ec10 to df28867 Compare June 14, 2021 20:41

Chris-Hibbert requested a review from katelynsills June 15, 2021 20:05

Chris-Hibbert force-pushed the gov-lib-3186 branch from 964877b to 9ffd163 Compare June 16, 2021 00:57

katelynsills suggested changes Jun 16, 2021

View reviewed changes

dtribble reviewed Jun 16, 2021

View reviewed changes

Chris-Hibbert commented Jun 16, 2021

View reviewed changes

katelynsills reviewed Jun 17, 2021

View reviewed changes

Chris-Hibbert requested a review from katelynsills June 17, 2021 17:12

Chris-Hibbert added 9 commits June 17, 2021 10:50

chore: convert functions to arrow functions

4563d42

chore: add types for handle and any.

5ce2ecf

chore: drop an inadvertant import

e1dc132

feat: add lookup for param names and details

07ec824

chore: update version info

ce7ef0b

chore: review suggestions

b07a981

dependency reduction support instance and installation rather than handle support NAT type which must be a bigint rename publicFacet to params

refactor: paramManager separates each param updater

bf2dc9e

change 'any' to 'unknown' drop an unneeded guard clause a single getParams() describes all parameters

chore: typescript corrections

842a5f1

Thanks to Michael Fig.

Chris-Hibbert force-pushed the gov-lib-3186 branch from 83fe828 to 842a5f1 Compare June 17, 2021 17:51

katelynsills suggested changes Jun 17, 2021

View reviewed changes

chore: review cleanups

edf7dac

dependency trimming better test for AMOUNT. improve some error messages narrow some type declarations add tests for INSTANCE and INSTALLATION

Chris-Hibbert force-pushed the gov-lib-3186 branch from 717eba1 to edf7dac Compare June 17, 2021 23:34

Chris-Hibbert requested a review from katelynsills June 18, 2021 00:01

katelynsills approved these changes Jun 18, 2021

View reviewed changes

chore: correct esm import

569a01e

Chris-Hibbert merged commit 8447993 into master Jun 18, 2021

Chris-Hibbert deleted the gov-lib-3186 branch June 18, 2021 00:48

	* @property {(name: string, value: any) => void} update
	* @property {(name: string, value: ParamValue) => void} update

	* @property {(name: string) => any} lookup
	* @property {(name: string) => ParamValue} lookup

	message: 'unknown type guard "quote"',
	message: 'unrecognized type "quote"',

	const { getParams, updateNat: _updateNat, updateStuff } = buildParamManager([
	const { getParams, updateStuff } = buildParamManager([

	* @typedef {{ [updater: string]: (arg: any) => void }} ParamManagerUpdaters
	* @typedef {{ [updater: string]: (arg: ParamValue) => void }} ParamManagerUpdaters

	AmountMath.isEqual(value, value),
	AmountMath.coerce(value.brand, value),

		esm@agoric-labs/esm#Agoric-built:
		esm@^3.2.25, esm@agoric-labs/esm#Agoric-built:

Manage parameters for a contract #3208

Manage parameters for a contract #3208

Conversation

Chris-Hibbert commented May 28, 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

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

Choose a reason for hiding this comment

katelynsills commented Jun 16, 2021

Chris-Hibbert 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

katelynsills commented Jun 17, 2021

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