Manage treasury 3189 #3310

Chris-Hibbert · 2021-06-14T20:40:09Z

dtribble

There's a bunch of code here that seems redundant with another pull request soI'm not sure what's up. I also have a more friendly API for params to discuss.

packages/governance/src/param-manager.js

packages/treasury/src/stablecoinMachine.js

dtribble · 2021-06-16T06:09:01Z

packages/treasury/src/stablecoinMachine.js

+    const {
+      publicFacet: rateManagerPublic,
+      manager: rateManager,
+    } = buildParamManager([


this has got to go into a different file or something. Or we need a much more abbreviated syntax for this.

I'm thinking each param should be a separate object, rather than havign them all looked up through essentially a map.

const chargingPeriodParam = gov.natParam(CHARGING_PERIOD_KEY, loanParams.chargingPeriod); const recordingPeriodParam = gov.natParam(RECORDING_PERIOD_KEY, loanParams.recordingPeriod);

and then usage would be chargingPeriodParam.value() or some such.

moved to a separate file.

They're not separate objects, but there are separate updateFoo() functions.

packages/treasury/src/vaultManager.js

katelynsills

In this PR, someone who is only looking at the treasury contract has no way of knowing how governance can change the parameters. I realize that this PR might have just been just part of the overall concept (just a piecemeal use of the governance params only), but I think for this PR to be a useful test of the system, we should have a test that models the user of the treasury. The user might ask:

What are the terms that can change? Can I know which terms can change just by looking at the invitation? What if the contract is not reporting the mutable terms correctly?
For the governed parameters, how are they governed? Under what rules can they change? Who makes decisions? How can I verify what the documentation says?

In order for the user to answer # 2, the whole governance infrastructure needs to be within this contract and the paramManager should not escape the contract. Adding getParamManager: () => paramManager, to the creatorFacet would be wrong.

packages/treasury/src/stablecoinMachine.js

@dckc

comments have been addressed. Responsibility for review transferred to @dckc

dckc

I tried just jumping in as if the PR were self-explanatory, but I'm quickly getting in over my head. I better talk with @Chris-Hibbert and maybe @michaelfig .

dckc · 2021-10-04T18:24:26Z

packages/treasury/src/vault.js

@@ -58,6 +58,8 @@ export function makeVaultKit(
    assert(vaultState === VaultState.ACTIVE, 'vault must still be active');
  }

+  console.log(`VALT   ${q(manager)}`);


This looks like debug logging. Any particular reason to leave it turned on in production?

Nope. Thanks.

dckc · 2021-10-04T18:36:10Z

packages/treasury/bundles/install-on-chain.js

+import contractGovernorBundle from './bundle-contractGovernor.js';
+import committeeBundle from './bundle-committee.js';
+import binaryVoteCounterBundle from './bundle-binaryVoteCounter.js';
+import { governedParameterTerms } from '../src/params';


I struggled to find a test where I could walk through this code... it looks like nothing in the tests in this package uses install-on-chain.js. But I see the cosmic-swingset integration test does.

That was how I discovered that I needed to update this was that the integration test failed. It looks like it may be used in bringing up a chain, so I tried to make the settings as realistic as possible.

packages/treasury/test/swingsetTests/governance/vat-zoe.js

dckc · 2021-10-04T18:48:55Z

packages/treasury/bundles/install-on-chain.js

    ['INSTALLATION_BOARD_ID', stablecoinMachineInstall],
    ['RUN_ISSUER_BOARD_ID', centralIssuer],
    ['RUN_BRAND_BOARD_ID', centralBrand],
    ['AMM_INSTALLATION_BOARD_ID', autoswapInstall],
    ['LIQ_INSTALLATION_BOARD_ID', liquidationInstall],
+    ['BINARY_COUNTER_INSTALLATION_BOARD_ID', binaryCounterInstall],
+    ['COMMITTEE_INSTALLATION_BOARD_ID', committeeInstall],
+    ['CONTRACT_GOVERNOR_INSTALLATION_BOARD_ID', contractGovernorInstall],


I'm struggling to judge whether these board / naming updates are correct. I suspect there are some external constraints... from dapp-treasury?

installOnChain has no doc comment.

vs-code is complaining that it can't find the NameHub type.

I was mostly guessing here. I should ask @michaelfig to review this PR and opine on the changes to `install-on-chain.js

This list is specifically for the UI config that is returned to dapp-treasury/ui. So, unless the Treasury UI is going to use those board IDs, please don't bother with them.

Rather than a 5-member committee that has no active members, I would prefer a "don't do anything except unilaterally replace my electorate" facet that could be handed to the bootstrap vat, so that the bootstrap could install the actual initial electorate.

It's fine if that's done in a future PR, but please explicitly mark all the inescapable committee governance in this file as a temporary hack just to allow the treasury not to crash during installation.

The way governance works is that the electorate has to exist before the contract is instantiated, in order to make the governance visible to all. I'm fine with saying there's no governance during testNet, but the way I'd do that is by building a do-nothing electorate. I think that's straightforward, and may be useful in other, similar circumstances.

It would be visible that there's a place for governance, but no one can initiate governance actions in the current installation.

This list is specifically for the UI config that is returned to dapp-treasury/ui. So, unless the Treasury UI is going to use those board IDs, please don't bother with them.

I'm not going to add code to the Treasury UI at this point, but it ought to be able to help the user review invitations. It should be able to identify the installations. All these installations are relevant to the user.

dckc · 2021-10-04T18:51:24Z

packages/treasury/bundles/install-on-chain.js

@@ -74,42 +88,65 @@ export async function installOnChain({
    }),
  );

+  const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 5 };


Where did we get 5 for the size of the treasury board? Was that specified somewhere? Or is it arbitrary?

It was arbitrary. Can you think of a good place to put a parameter/manifest constant?

I'm not sure a manifest constant would help me; then I'd just wonder why that is 5. I'm trying to learn the story behind these numbers and structures. Which ones stay fixed and which ones change and how and why? I'm struggling to pick it up from the code alone.

a do-nothing electorate would make that constant unnecessary.

Chris-Hibbert · 2021-10-04T20:48:17Z

@michaelfig would you take a look at the changes in install-on-chain.js? I made an educated guess as to what was needed, but I may not be familiar with enough of the context.

michaelfig · 2021-10-05T00:00:05Z

packages/vats/src/bootstrap.js

-    const treasuryCreator = await installEconomy(bootstrapPaymentValue);
+    // NOTE: no use of the voteCreator. We'll need it to initiate votes on
+    // changing Treasury parameters.
+    const { treasuryCreator, _voteCreator } = await installEconomy(


If you keep the committee in place, maybe you could make it just 1 member, and send the voteCreator to the some place as the treasuryCreator is exposed (look for powerFlagConfig) and name it treasuryVoteCreator.

I added a no-action electorate instead, as it's easier to validate that it's not doing anything. We can add an active electorate when we know what we want it to do.

dckc

calling it a day... I think I'll share what I've got so far.

Getting the types to work with dynamic parameter names is tricky. I haven't gotten all of the suggestions I make here work with yarn lint at the same time. Feel free to pick the ones you like and disregard the others. I suspect we'll have to change ParamDescs from an array ParamDesc[] to Record<string, ParamSomething> to get them to line up right. That's a substantive change, but it's something that can happen in a separate PR.

dckc · 2021-10-04T21:21:33Z

packages/treasury/bundles/install-on-chain.js

@@ -74,42 +88,65 @@ export async function installOnChain({
    }),
  );

+  const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 5 };


Until we work out how anybody should get access to the voter invitations, let's make it 1; that's sort of more obviously not a real committee.

Suggested change

const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 5 };

const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 1 };

I've drafted a no-action electorate. I think that's more clear that it does nothing.

packages/treasury/src/stablecoinMachine.js

dckc · 2021-10-05T00:57:37Z

packages/treasury/src/params.js

+};
+
+export const makePoolParamManager = (loanParams, rates) => {
+  /** @type {PoolParamManager} */


this is perhaps useful as documentation, but tsc doesn't grok type annotations on return statements.

I tried to work out appropriate type declarations, but with dynamic parameter names, it gets complicated real quick, so I'm setting it aside for now.

I can declare the return type of makePoolParamManager.

packages/treasury/src/stablecoinMachine.js

dckc · 2021-10-05T01:18:28Z

packages/treasury/test/test-stablecoin.js

  const {
    aethKit: { mint: aethMint, issuer: aethIssuer, brand: aethBrand },
  } = setupAssets();

-  const priceAuthorityPromiseKit = makePromiseKit();
-  const priceAuthorityPromise = priceAuthorityPromiseKit.promise;
+  const manualTimer = buildManualTimer(console.log);


I tried to get some of these timers to be quiet unless ava sees -v by doing...

Suggested change

const manualTimer = buildManualTimer(console.log);

const manualTimer = buildManualTimer(t.log);

but sometimes t is not in scope... I'm not sure whether it's worth doing just some of the time.

What problem are you trying to solve?

The the problem of lots of unwanted console logs when running tests.

The t.log function is silent unless the -v command-line flag is given.

packages/treasury/src/types.js

packages/treasury/src/stablecoinMachine.js

dckc · 2021-10-05T02:37:00Z

packages/treasury/src/stablecoinMachine.js

@@ -151,6 +192,7 @@ export async function start(zcf, privateArgs) {
        want: { Governance: _govOut }, // ownership of the whole stablecoin machine
      } = seat.getProposal();
      assert(!collateralTypes.has(collateralBrand));
+      // initialPrice is in rates, but only used at creation, so not in governor


I see keywords joined with _ a few lines down:

`${collateralKeyword}_Liquidity`

Is that right? As of landing the Constant Product PR, we have:

const addPool = async (secondaryIssuer, keyword) => { const liquidityKeyword = `${keyword}Liquidity`;

https://github.com/Agoric/agoric-sdk/blob/master/packages/zoe/src/contracts/vpool-xyk-amm/pool.js#L233

This is still tied to multipoolAutoswap. I'd like to convert it to use constantProduct, but that hasn't happened yet. I think that should be a separate PR.

multipoolAutoswap and constantProduct have different keyword patterns? on purpose?

packages/treasury/src/types.js

dckc

The ${keyword}_Liquidation vs ${keyword}Liquidation conflict seems like a real bug, i.e. incompatibility with the xyk stuff.

And install-on-chain simplifications suggested by MF seem worth doing.

packages/treasury/src/stablecoinMachine.js

packages/treasury/test/swingsetTests/governance/test-governance.js

dckc · 2021-10-05T19:19:32Z

packages/treasury/test/swingsetTests/governance/vat-voter.js

+          // I'd like to validate Installations, but there doesn't seem to be a
+          // way to get it from an Instance. I'd verify the Electorate,
+          // ballotCounter, and contractGovernor.


What about E(zoe).getInstallationForInstance(instance)?

Thanks. This comment predates both the implementation of that feature, and I think the issue that tracked it.

dckc · 2021-10-05T19:21:37Z

packages/treasury/test/swingsetTests/governance/vat-zoe.js

+      const { zoeService, feeMintAccess } = makeZoeKit(
+        vatAdminSvc,
+        shutdownZoeVat,


There's nothing specific to treasury governance in this vat-zoe.js, is there? This looks like boilerplate as a consequence of metering changes.

IOU an issue to consolidate it in zoe/tools or some such.

that would be nice. Yes, it's boilerplate, used repeatedly in many swingset tests.

michaelfig · 2021-10-07T04:53:25Z

@Chris-Hibbert if you want:

a file in a directory (like bundles/install-on-chain.js) to
import individual files from a second directory (like '../src/params.js')
and that second directory uses ambient types (like types.js),
you must import those type files explicitly from that imported individual file.

The following solves your TS conundrum:

diff --git a/packages/treasury/src/params.js b/packages/treasury/src/params.js
index a3aa8958f..6d25c9658 100644
--- a/packages/treasury/src/params.js
+++ b/packages/treasury/src/params.js
@@ -2,6 +2,8 @@
 
 import { buildParamManager, ParamType } from '@agoric/governance';
 
+import './types.js';
+
 export const POOL_FEE_KEY = 'PoolFee';
 export const PROTOCOL_FEE_KEY = 'ProtocolFee';

extract params to a separate file integrate contract governance into treasury swingset test for treasury governance closes #3189 closes #3473

Chris-Hibbert · 2021-10-07T18:08:11Z

@dckc, @dtribble Please take another look. I think I've addressed all your comments, but github says there are still changes requested from you.

dckc

good stuff.

dtribble

The cleanup for the param implementation will be a separate PR.

dckc · 2021-10-13T07:43:10Z

p.s. I reviewed a slog from test/swingsetTests/governance/test-governance.js and it looks good:

https://gist.github.com/8f79071f74a07605499ef7bc2380d699

In particular, all 4 votes go in one round trip:

Chris-Hibbert added enhancement New feature or request Small Inter-protocol Overarching Inter Protocol Governance Governance labels Jun 14, 2021

Chris-Hibbert added this to the Testnet: Stress Test Phase milestone Jun 14, 2021

Chris-Hibbert self-assigned this Jun 14, 2021

Chris-Hibbert changed the base branch from master to gov-lib-3186 June 14, 2021 20:40

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

Chris-Hibbert requested review from katelynsills and dtribble June 15, 2021 20:02

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

dtribble requested changes Jun 16, 2021

View reviewed changes

katelynsills previously requested changes Jun 17, 2021

View reviewed changes

packages/treasury/src/stablecoinMachine.js Show resolved Hide resolved

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

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 5766e66 to cda95b1 Compare June 17, 2021 22:19

Base automatically changed from gov-lib-3186 to master June 18, 2021 00:48

katelynsills removed this from the Testnet: Stress Test Phase milestone Jun 18, 2021

Chris-Hibbert force-pushed the manageTreasury-3189 branch from cda95b1 to 41f66c5 Compare June 21, 2021 23:53

Chris-Hibbert changed the base branch from master to TreasuryGovernance June 21, 2021 23:54

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 41f66c5 to e478297 Compare June 22, 2021 00:25

Chris-Hibbert force-pushed the manageTreasury-3189 branch from e478297 to a9ad9ab Compare July 7, 2021 19:48

Chris-Hibbert changed the base branch from TreasuryGovernance to contractGovernanceMgr-3185-outside July 7, 2021 19:51

Chris-Hibbert mentioned this pull request Jul 7, 2021

ContractGovernor manages parameter updating for a contract #3448

Merged

Chris-Hibbert force-pushed the manageTreasury-3189 branch 2 times, most recently from 06a9e90 to 3b4bc67 Compare July 16, 2021 00:28

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 3b4bc67 to 4396a30 Compare August 3, 2021 22:23

Chris-Hibbert changed the base branch from contractGovernanceMgr-3185-outside to contractGovernanceMgr-3185 August 3, 2021 22:28

Chris-Hibbert marked this pull request as draft August 3, 2021 22:42

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 4396a30 to 1c82754 Compare August 3, 2021 22:44

Chris-Hibbert requested a review from dtribble October 4, 2021 16:59

dckc reviewed Oct 4, 2021

View reviewed changes

Chris-Hibbert requested a review from michaelfig October 4, 2021 20:47

Chris-Hibbert closed this Oct 4, 2021

Chris-Hibbert reopened this Oct 4, 2021

michaelfig reviewed Oct 5, 2021

View reviewed changes

dckc reviewed Oct 5, 2021

View reviewed changes

dckc requested changes Oct 5, 2021

View reviewed changes

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 1a5d557 to 3ca89e8 Compare October 6, 2021 18:41

Chris-Hibbert requested a review from dckc October 6, 2021 20:14

Chris-Hibbert added 5 commits October 7, 2021 09:20

feat!: use contractGovernor to govern Treasury using ParamManager

57aad11

extract params to a separate file integrate contract governance into treasury swingset test for treasury governance closes #3189 closes #3473

chore: minor cleanups: drop extra logs, standardize asserts, tsc fix

e01c968

chore: improve typescript declarations

937a5a1

feat: add noAction electorate for assurance of no governance changes

fc1e381

chore: validate() in test checks the installations

ebe1f36

Chris-Hibbert force-pushed the manageTreasury-3189 branch from e7c329e to 97f1d53 Compare October 7, 2021 16:20

fix: import types.js into params so bundle is usable in tests

6ce7f17

Chris-Hibbert force-pushed the manageTreasury-3189 branch from 97f1d53 to 6ce7f17 Compare October 7, 2021 16:40

fix: remove spurious distinction in naming of Liquidity keyword

2052b0a

dckc approved these changes Oct 7, 2021

View reviewed changes

dtribble approved these changes Oct 8, 2021

View reviewed changes

Chris-Hibbert added the automerge:squash Automatically squash merge label Oct 8, 2021

Merge branch 'master' into manageTreasury-3189

cbc1df0

mergify bot merged commit c19f141 into master Oct 8, 2021

mergify bot deleted the manageTreasury-3189 branch October 8, 2021 16:32

	const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 5 };
	const electorateTerms = { committeeName: 'TreasuryBoard', committeeSize: 1 };

	const manualTimer = buildManualTimer(console.log);
	const manualTimer = buildManualTimer(t.log);

Manage treasury 3189 #3310

Manage treasury 3189 #3310

Conversation

Chris-Hibbert commented Jun 14, 2021 • edited Loading

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

katelynsills left a comment

Choose a reason for hiding this comment

dckc 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

Chris-Hibbert Oct 5, 2021 • edited Loading

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

Chris-Hibbert commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc 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

dckc 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

michaelfig commented Oct 7, 2021

Chris-Hibbert commented Oct 7, 2021

dckc left a comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

dckc commented Oct 13, 2021

Chris-Hibbert commented Jun 14, 2021 •

edited

Loading

Chris-Hibbert Oct 5, 2021 •

edited

Loading