refactor(run-protocol): move config of charging and recording periods to vault factory #4394

turadg · 2022-01-26T19:49:48Z

closes: #4185

Description

Moves the loan calculation periods (charging and recording) from governance params on each vault manager (collateral type) to the vault factory as a whole.

Security Considerations

See #4185

Documentation Considerations

Needs documenting

Testing Considerations

Period governance probably needs testing. Will be addressed in #4432

turadg · 2022-01-26T19:58:52Z

packages/assert/src/assert.js

+ * @param {string} [message]
+ * @returns {T}
+ */
+export function assertDefined(val, message) {


@erights WDYT of this? similar to Nat but for a nullish check. See below for an example in action.

mhofman · 2022-01-26T21:56:45Z

IMO the problem here is that getLoanParams is not sufficiently typed, and is a simple indexed access, which means the type may come back as undefined.

I don't disagree that an assertNonNullish would be valuable, but it should be reserved for places where not just used for satisfying the type checker.

erights · 2022-01-26T23:41:49Z

packages/assert/src/assert.js

+/**
+ * @template T
+ * @param {T | null | undefined} val
+ * @param {string} [message]
+ * @returns {T}
+ */
+export function assertDefined(val, message) {
+  if (val != null) {
+    return val;
+  }
+  throw Error(message !== undefined ? message : `unexpected ${val}`);
+}


I like the idea of it, but have some questions. The assert module is migrating from agoric-sdk to endo anyway. But wherever it is, it should be written in our normal assertFoo style. The following is how I might write it.

Also, please don't write function functions. We have essentially adopted an arrow-only style. We have not yet taken the time to convert old code. But new code should be in arrow-only style unless there's a good stated reason otherwise.

Also, you're exporting a non-hardened value.

Suggested change

/**

* @template T

* @param {T | null | undefined} val

* @param {string} [message]

* @returns {T}

*/

export function assertDefined(val, message) {

if (val != null) {

return val;

}

throw Error(message !== undefined ? message : `unexpected ${val}`);

}

/**

* @template T

* @param {T | null | undefined} val

* @param {string} [optDetails]

* @returns {T}

*/

export const assertDefined = (val, optDetails = X`unexpected ${q(val)}`) => {

if (val != null) {

// This `!= null` idiom checks that `val` is not `null` nor `undefined`.

return val;

}

assert.fail(optDetails);

};

harden(assertDefined);

Also, I'm not sure I like "defined" as the opposite of "nullish". But I cannot think of a better one.

Apropos naming, is there a better term than "assert"? In SES, global assert mostly return void but this one returns the argument.

I could see someone being surprised by one or the other. For consistency, would it be better to return the argument when asserting a type or to have a different verb for this version that returns its argument? cc @kriskowal

This is a good point. I agree that we should not have a function named assertFoo that returns its argument. This is indeed inconsistent with both precedents: The assertFoo functions we have don't return their argument, and the validating or coercing functions we have that do return their argument, or a coerced-to-validated form of their argument, like Nat, and written simply naming the property they assure about their return result. This one at least is written with an initial upper case, mirroring similar usage of String, BigInt etc.

How about simply NotNullish ?

Went with slight tweak, NonNullish, similar to https://www.typescriptlang.org/docs/handbook/utility-types.html#nonnullabletype

I have in hobby projects found it useful to distinguish assert* (returns void) from assume* (returns argument), though one could argue that all assume functions could just be assert and ignore the return.

https://github.com/borkshop/js/blob/main/cube/assert.js

Not having to name an intermediate variable in an expression like this is useful:

const location = assumeDefined(locations.get(entityId), X`No entity for identifier ${q(entityId)}`);

With some refactoring I don't need this anymore but I still think it would be useful. Pulling out to #4422 and removing you from reviewers for this PR.

dckc · 2022-01-27T00:21:30Z

packages/run-protocol/src/vaultFactory/vault.js

+import {
+  CHARGING_PERIOD,
+  RECORDING_PERIOD,
+} from '@agoric/zoe/src/contracts/loan';
+


That's an example contract that, according to #1656, should move out of Zoe. Let's not use that as the source of truth for this.

the NonNullish utility for which changes were requested has been pulled out (#4422)

dckc

I don't see a test for the fix / feature / design change. I suggest a test that changes the recording period and checks that it affects vaults of multiple collateral brands.

dckc · 2022-01-31T19:16:02Z

packages/governance/src/contractHelper.js

@@ -49,6 +51,8 @@ const handleParamGovernance = (zcf, paramManager) => {
      ...originalPublicFacet,
      getSubscription: () => paramManager.getSubscription(),
      getContractGovernor: () => electionManager,
+      /** @type {GetGovernedVaultParams} */
+      // @ts-expect-error we know this ParamManagerFull is really a GetGovernedVaultParams


Not for this PR, but...

I suspect there's a way to avoid type conflicts like this by parameterizing types such as ParamManagerFull.

It looks like I didn't file an issue, but I did some related work in https://github.com/Agoric/agoric-sdk/commits/claims-registar-dc-review esp 88b28e9 .

dckc · 2022-01-31T19:18:35Z

packages/governance/src/paramGovernance/paramMakers.js

+/**
+ * @param {bigint} value
+ */


In the interest of vertical code density, for single-parameter functions, I usually do:

Suggested change

/**

* @param {bigint} value

*/

/** @param {bigint} value */

This reduces consistency somewhat. And if the function later takes more than one parameter, there is some churn. So it's a matter of taste.

packages/governance/src/paramGovernance/paramManager.js

dckc · 2022-01-31T19:22:44Z

packages/run-protocol/src/vaultFactory/params.js

-  // @ts-ignore It's a VaultParamManager
+/**
+ * @param {LoanTiming} initialValues
+ * @returns {ParamManagerFull & {


dckc · 2022-01-31T19:28:14Z

packages/run-protocol/src/vaultFactory/params.js

+};
+
+/**
+ * XXX maybe call this "loan terms"?


to whom is this XXX comment directed? Why call it "loan terms"? Is there something wrong with the current name? if so, what?

The XXX is directed at whoever is reading this code, but I think this one can be resolved before merging.

I thought "terms" instead of "rates" because while it includes interestRate it also has initialMargin, liquidationMargin and loanFee. The latter don't seem like rates to me but they are part of the terms of the loan.

I didn't make the change because I thought maybe the charging and recording period could be considered terms of the loan… but since those are parameters of the whole vault factory they aren't part of any particular loan.

So I think Terms is better than Rates. But I'm not as sure about the "Loan" term (pun intended) because vaults, while they share some mechanics with loans, are explicitly not loans. Maybe ChargingTerms?

I have some hesitation about changing this name, as it's not required to address #4185 . It's a bit of a "drive-by." Plus, I feel a little like when my spouse asks me whether I like the new paint color; I just don't have an opinion.

OTOH, @dtribble did ask you to look into naming issues such as this. See if you can grab him briefly?

For somewhat arbitrary issues like this, I'm kinda inclined to fall back on an executive hierarchy (#3382); that is: talk about it with @Chris-Hibbert .

I guess if it's between you and me: I'll agree to a change if you think it's sufficiently worthwhile to add a test :)
Is it part of the package's external API? If so, that makes it a breaking change, yes? So it would need its own conventional commit to get it in the CHANGELOG. (or do we use NEWS.md in this package?)

Good call that it's out of scope. I've cut it and will look into it separately.

Good question about breaking change… changing the TS type wouldn't break API but this refactoring does change how debts are calculated. However I don't think it breaks any external assumptions because Agoric controls the governance of both the vault factory and vault managers (the destination and source for timing governance)

dckc · 2022-01-31T19:31:34Z

packages/governance/src/types.js

 */

 /**
 * @typedef {Object} ParamManagerBase
- * @property {GetParams} getParams
+ * @property {() => Record<Keyword,ParamShortDescription>} getParams


Suggested change

* @property {() => Record<Keyword,ParamShortDescription>} getParams

* @property {() => Record<Keyword, ParamShortDescription>} getParams

Thanks. Among the downside of type defs being in jsdoc comments. In .ts file Prettier would autoformat.

We have 95 Record with space and 18 without it.

dckc · 2022-01-31T19:33:04Z

packages/run-protocol/src/vaultFactory/params.js

+/**
+ * @param {Amount} electorateInvitationAmount
+ */


again, please consider a more dense/concise form:

Suggested change

/**

* @param {Amount} electorateInvitationAmount

*/

/** @param {Amount} electorateInvitationAmount */

I'm convinced.

As you said above, there's a trade-off with comment churn when another param is added. But on balance I think it's better to have the concise form. Comments have less need for git blame.

dckc · 2022-01-31T19:34:48Z

packages/run-protocol/src/vaultFactory/types.js

@@ -16,7 +16,7 @@
 */

 /**
- * @typedef {Object} Rates
+ * @typedef {Object} Rates - XXX loan terms?


another XXX. I'm not sure what to do with these.

If they are intended to land on master, ideally they would have corresponding issue #s.

dckc · 2022-01-31T19:37:28Z

packages/run-protocol/src/vaultFactory/types.js

- */
-
-/**
- * @callback MakeVaultParamManager


Good riddance to the use of @callback for something that isn't actually passed around as a callback.

dckc · 2022-01-31T19:39:08Z

packages/run-protocol/src/vaultFactory/vaultManager.js

+ *  'ChargingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },
+ *  'RecordingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },


again: why quote these keys?

Suggested change

* 'ChargingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },

* 'RecordingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },

* ChargingPeriod: ParamRecord<'relativeTime'> & { value: RelativeTime },

* RecordingPeriod: ParamRecord<'relativeTime'> & { value: RelativeTime },

vestiges of refactoring I guess. will remove.

Doesn't seem to have been cleaned up before merge?

mea culpa. I'll include in another PR

addressed

…eed for casts

… type

turadg · 2022-02-01T20:16:41Z

I don't see a test for the fix / feature / design change. I suggest a test that changes the recording period and checks that it affects vaults of multiple collateral brands.

Good suggestion, @dckc . Given the state of swingset testing in run-protocol, I discussed with Dean in standup and we agreed to punt to a more comprehensive look at test coverage #4432

PTAAL

dckc

postponing testing is very much not my style, but let's give it a try (#4432).

Chris-Hibbert · 2022-02-07T21:33:44Z

packages/governance/src/types.js

- * @callback GetParams - getParams() retrieves a Record containing
- *   keyword pairs with descriptions of parameters under governance.
- * @returns {Record<Keyword,ParamShortDescription>}
+ * @callback GetGovernedVaultParams


@turadg governance shouldn't know about vaults.

This looks like it's actually about Vaults, so I'd like to move it. Is there a reason it needs to be here?

This was a best effort to get the types consistent. Feel free to improve!

I sorta moved it back in #4437

Chris-Hibbert · 2022-02-07T21:35:04Z

packages/governance/src/types.js

@@ -539,7 +544,7 @@
 * @typedef {Object} GovernedPublicFacet
 * @property {() => Subscription<ParamDescription>} getSubscription
 * @property {VoteOnParamChange} getContractGovernor
- * @property {GetParams} getGovernedParams - get descriptions of
+ * @property {GetGovernedVaultParams} getGovernedParams - get descriptions of


@turadg

It may be the case that the only actual use of this is in Vaults, but the support in Governance should be more generic. Is there a way to have a generic GetParams here, and override it with something more specific in Vaults?

Yes, if in the vaults we use something more specific instead of GovernedPublicFacet. Happy to work together on the specifics.

Chris-Hibbert · 2022-02-07T21:40:00Z

packages/run-protocol/src/vaultFactory/vaultManager.js

+ *  'ChargingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },
+ *  'RecordingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },


Doesn't seem to have been cleaned up before merge?

turadg commented Jan 26, 2022

View reviewed changes

turadg force-pushed the ta/vault-periods-immutable branch from 18cd189 to a914f32 Compare January 26, 2022 20:42

erights previously requested changes Jan 26, 2022

View reviewed changes

dckc previously requested changes Jan 27, 2022

View reviewed changes

turadg force-pushed the ta/vault-periods-immutable branch from a914f32 to 5537785 Compare January 31, 2022 16:15

turadg requested a review from dckc January 31, 2022 18:24

turadg marked this pull request as ready for review January 31, 2022 18:58

dckc reviewed Jan 31, 2022

View reviewed changes

turadg changed the title ~~move config of charging and recording periods to chain startup~~ refactor: move config of charging and recording periods to vault factory Feb 1, 2022

turadg mentioned this pull request Feb 1, 2022

Plan work to improve test design and coverage of run-protocol #4432

Closed

turadg added 13 commits February 1, 2022 12:13

chore(governance): more complete type for getLoanParams() to remove n…

cf799ae

…eed for casts

inline more types

04a6dbb

LoanParams --> LoanTiming

8a86d8f

money

47d8cc7

more specific name for GetParams (GetGovernedVaultParams) and tighter…

a999b6b

… type

fix governance types

4a2aa4e

cleanup

1a5c319

test support

2bd8ceb

refactor: test-governance before()

1882e02

test support

7f52d08

trying to understand SwingSet test setup

1e2ee6d

test refactoring

526eee1

update comment on buildVatController

1c04948

turadg force-pushed the ta/vault-periods-immutable branch from fc86778 to 1c04948 Compare February 1, 2022 20:13

turadg requested a review from dckc February 1, 2022 20:16

dckc approved these changes Feb 1, 2022

View reviewed changes

turadg added 2 commits February 1, 2022 13:28

suppress TS errors

1ed7b99

another rev on the buildVatController comment

374266f

turadg changed the title ~~refactor: move config of charging and recording periods to vault factory~~ refactor(run-protocol): move config of charging and recording periods to vault factory Feb 1, 2022

turadg added the automerge:squash Automatically squash merge label Feb 1, 2022

Merge branch 'master' into ta/vault-periods-immutable

698d588

mergify bot merged commit d7b69bd into master Feb 1, 2022

mergify bot deleted the ta/vault-periods-immutable branch February 1, 2022 22:40

Chris-Hibbert reviewed Feb 7, 2022

View reviewed changes

	* @property {() => Record<Keyword,ParamShortDescription>} getParams
	* @property {() => Record<Keyword, ParamShortDescription>} getParams

		* 'ChargingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },
		* 'RecordingPeriod': ParamRecord<'relativeTime'> & { value: RelativeTime },

refactor(run-protocol): move config of charging and recording periods to vault factory #4394

refactor(run-protocol): move config of charging and recording periods to vault factory #4394

Uh oh!

Conversation

turadg commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Documentation Considerations

Testing Considerations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dckc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg commented Feb 1, 2022

Uh oh!

dckc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg commented Jan 26, 2022 •

edited

Loading

turadg Jan 31, 2022 •

edited

Loading

turadg Jan 31, 2022 •

edited

Loading