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

Manage parameters for a contract #3208

Merged
merged 11 commits into from
Jun 18, 2021
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"packages/bundle-source",
"packages/import-bundle",
"packages/eventual-send",
"packages/governance",
"packages/promise-kit",
"packages/tame-metering",
"packages/transform-metering",
Expand Down
5 changes: 5 additions & 0 deletions packages/governance/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Change Log

All notable changes to this project will be documented in this file.
See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.

Empty file added packages/governance/NEWS.md
Empty file.
18 changes: 18 additions & 0 deletions packages/governance/jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// This file can contain .js-specific Typescript compiler config.
{
"compilerOptions": {
"target": "esnext",

"noEmit": true,
/*
// The following flags are for creating .d.ts files:
"noEmit": false,
"declaration": true,
"emitDeclarationOnly": true,
*/
"downlevelIteration": true,
"strictNullChecks": true,
"moduleResolution": "node",
},
"include": ["src/**/*.js", "test/**/*.js"],
}
71 changes: 71 additions & 0 deletions packages/governance/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
"name": "@agoric/governance",
"version": "0.1.0",
"description": "Core governance support",
"parsers": {
"js": "mjs"
},
"main": "src/paramManager.js",
"engines": {
"node": ">=14.15.0"
},
"scripts": {
"build": "exit 0",
"test": "ava",
"test:xs": "exit 0",
"lint-fix": "yarn lint:eslint --fix && yarn lint:types",
"lint-check": "yarn lint",
"lint": "yarn lint:types && yarn lint:eslint",
"lint:eslint": "eslint '**/*.js'",
"lint:types": "tsc -p jsconfig.json"
},
"repository": {
"type": "git",
"url": "git+https://github.com/Agoric/agoric-sdk.git"
},
"author": "Agoric",
"license": "Apache-2.0",
"bugs": {
"url": "https://github.com/Agoric/agoric-sdk/issues"
},
"homepage": "https://github.com/Agoric/agoric-sdk#readme",
"dependencies": {
"@agoric/assert": "^0.3.0",
"@agoric/ertp": "^0.11.2",
"@agoric/marshal": "^0.4.13",
"@agoric/nat": "^4.0.0",
"@agoric/notifier": "^0.3.14",
"@agoric/store": "^0.4.15",
"@agoric/zoe": "^0.16.0"
},
"devDependencies": {
"@agoric/install-ses": "^0.5.13",
"ava": "^3.12.1",
"esm": "^3.2.25"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"esm": "^3.2.25"
"esm": "agoric-labs/esm#Agoric-built",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

},
"files": [
"src/",
"NEWS.md"
],
"ava": {
"files": [
"test/**/test-*.js"
],
"require": [
"esm"
],
"timeout": "10m"
},
"eslintConfig": {
"extends": [
"@agoric"
]
},
"prettier": {
"trailingComma": "all",
"singleQuote": true
},
"publishConfig": {
"access": "public"
}
}
133 changes: 133 additions & 0 deletions packages/governance/src/paramManager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// @ts-check

import { assert, details as X } from '@agoric/assert';
import { assertIsRatio } from '@agoric/zoe/src/contractSupport';
import { AmountMath, looksLikeBrand } from '@agoric/ertp';
import { Far } from '@agoric/marshal';
import { assertKeywordName } from '@agoric/zoe/src/cleanProposal';

/**
* @type {{
* AMOUNT: 'amount',
* UNKNOWN: 'unknown',
* BRAND: 'brand',
* INSTANCE: 'instance',
* INSTALLATION: 'installation',
* NAT: 'nat',
* RATIO: 'ratio',
* STRING: 'string',
* }}
*/
const ParamType = {
AMOUNT: 'amount',
BRAND: 'brand',
INSTANCE: 'instance',
INSTALLATION: 'installation',
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we test INSTANCE and INSTALLATION?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

NAT: 'nat',
RATIO: 'ratio',
STRING: 'string',
UNKNOWN: 'unknown',
};
harden(ParamType);

const assertType = (type, value, name) => {
switch (type) {
case ParamType.AMOUNT:
// It would be nice to have a clean way to assert something is an amount.
assert(
AmountMath.isEqual(value, value),
Copy link
Contributor

Choose a reason for hiding this comment

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

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),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

X`value for ${name} must be an Amount, was ${value}`,
);
break;
case ParamType.BRAND:
assert(
looksLikeBrand(value),
X`value for ${name} must be a brand, was ${value}`,
);
break;
case ParamType.INSTALLATION:
// TODO(3344): add a better assertion once Zoe validates installations
assert(
typeof value === 'object' && !Object.getOwnPropertyNames(value).length,
X`value for ${name} must be an empty object, was ${value}`,
);
break;
case ParamType.INSTANCE:
// TODO(3344): add a better assertion once Zoe validates instances
assert(
typeof value === 'object' && !Object.getOwnPropertyNames(value).length,
X`value for ${name} must be an empty object, was ${value}`,
);
break;
case ParamType.NAT:
assert.typeof(value, 'bigint');
break;
case ParamType.RATIO:
assertIsRatio(value);
break;
case ParamType.STRING:
assert.typeof(value, 'string');
break;
case ParamType.UNKNOWN:
break;
default:
assert.fail(X`unknown type guard ${type}`);
}
};

const parse = paramDesc => {
const values = {};
// manager has an updateFoo() for each Foo param. It will be returned.
const manager = {};
// getParams() uses describers to generate descriptions of each param.
const describers = [];

paramDesc.forEach(({ name, value, type }) => {
assert(
!values[name],
X`each parameter name must be unique: ${name} duplicated`,
);
assertType(type, value, name);
// we want to create function names like updateFeeRatio(), so we insist that
// the name has Keyword-nature.
assertKeywordName(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


values[name] = { type, value };
manager[`update${name}`] = newValue => {
assertType(type, newValue, name);
values[name].value = newValue;
};

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

});

const getParams = () => {
/** @type {Record<Keyword,ParamDescription>} */
const descriptions = {};
describers.forEach(d => {
const description = d();
descriptions[description.name] = description;
});
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 we don't need describers and can just iterate over values directly. This is making and using a ton of functions.

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 point. Done

return descriptions;
};

return { getParams, manager };
};

/** @type {BuildParamManager} */
const buildParamManager = paramDesc => {
const { getParams, manager } = parse(paramDesc);

return Far('param manager', {
getParams,
...manager,
});
};
harden(buildParamManager);

export { ParamType, buildParamManager };
41 changes: 41 additions & 0 deletions packages/governance/src/types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// @ts-check

/**
* @typedef { 'amount' | 'unknown' | 'brand' | 'installation' | 'instance' | 'nat' | 'ratio' | 'string' } ParamType
*/

/**
* @typedef { Amount | Brand | Instance| Installation | bigint | Ratio | string | unknown } ParamValue
*/

/**
* @typedef {Object} ParamDetails
Copy link
Contributor

Choose a reason for hiding this comment

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

This type doesn't seem to be used at all. Seems like ParamDescription is the same thing and is the one 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.

done

* @property {string} name
* @property {ParamValue} value
* @property {ParamType} type
*/

/**
* @typedef {Object} ParamDescription
* @property {string} name
* @property {ParamValue} value
* @property {ParamType} type
*/

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

Choose a reason for hiding this comment

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

Can this be:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. Done

* @typedef {ParamManagerBase & ParamManagerUpdaters} ParamManagerPublic
*/

/**
* @typedef {Array<ParamDescription>} ParamDescriptions
*/

/**
* @callback BuildParamManager
* @param {ParamDescriptions} paramDesc
* @returns {ParamManagerPublic}
*/
Loading