7502 followup: factor out factory #7519

turadg · 2023-04-25T23:51:57Z

Description

#7502 fixed an error in zcfMint kind restoration during reincarnation. While investigating it became clear that kind redefinitions could be simplified.

This turns the singleton into an Exo class, sharing the behavior across instances.

Using that prepare it also factors out the ZCFMintFactory and uses the maker directly.

Finally it has some types and helpers to support the change.

Of note: it switches some function declarations back to the @param style because I learned that tsc doesn't check the arguments at the callsite without it.

Security Considerations

noop

Scaling Considerations

noop

Documentation Considerations

noop

Testing Considerations

CI. Incidentally, while iterating there were some bugs that none of the Zoe package tests caught. Only the vats bootstrapTests did.

Chris-Hibbert

LGTM, with questions.

If merging this isn't urgent, I'd like to wait to give @erights a chance to say why we thought using singletons was a better choice for this case, and whether things have changed so it makes sense to switch to prepareDurable.

Chris-Hibbert · 2023-04-26T18:46:00Z

packages/zoe/src/contractFacet/zcfMint.js

+        mintGains: M.call(AmountKeywordRecordShape, M.any()).returns(SeatShape),
+        burnLosses: M.call(AmountKeywordRecordShape, M.any()).returns(


Lets use M.remotable('zcfSeat') instead of M.any().

Oh yeah I had postponed expressing the optionalality of the seat arg:

agoric-sdk/packages/zoe/src/contractFacet/types.js

Lines 142 to 146 in 0a11269

* @callback ZCFMintMintGains

* @param {AmountKeywordRecord} gains

* @param {ZCFSeat} [zcfSeat]

* @returns {ZCFSeat}

*/

Turns out ZCFMintI has this same issue. I'll fix that and re-use it.

Chris-Hibbert · 2023-04-26T18:53:56Z

packages/zoe/src/contractFacet/zcfMint.js

-   * baggage for the state of the zcfMint, makes a durableZcfMint from that
-   * baggage, and registers that baggage to be revived with the factory.
-   */
+  // TODO refactor zcfMintFactory uses to makeZcfMint directly


was this supposed to be "zcfMintFactory used to makeZcfMint directly"? Otherwise I'm not able to make sense of it.

That was an interim state in the refactoring

Chris-Hibbert · 2023-04-26T19:04:22Z

packages/zoe/src/contractFacet/zcfMint.js

+            /** @type {Amount<AssetKind>} */ total,
+            /** @type {Amount<AssetKind>} */ amountToAdd,


I don't see how these types help when add's only use is to be passed to .reduce(). gains is already typed because it is externally visible.

Are they here out of habit, or do they improve maintainability somehow?

interim state. a subsequent commit had,

/** @type {(total: Amount, amountToAdd: Amount) => Amount} */ const add = (total, amountToAdd) => AmountMath.add(total, amountToAdd, mintyBrand);

the type declaration doesn't add much but imo it's still better

erights

I used the opportunity to see if it reminded me of why I may once have objected. I found one possible reason, and explain in the comments why I am not concerned in this case. I got through the whole review without being reminded of any other reason. With @Chris-Hibbert also not remembering or having any reason to object...

LGTM, thanks!

erights · 2023-04-27T02:34:29Z

packages/zoe/src/contractFacet/zcfMint.js

+ * @param {(exit?: undefined) => { zcfSeat: any; userSeat: Promise<UserSeat> }} makeEmptySeatKit
+ * @param {ZcfMintReallocator} reallocator
+ */
+export const prepareDurableZcfMint = (


When a function is already named prepare* and takes a baggage as first argument, the "Durable" part of the name is implied, and so redundant. My naming habit is to say "durable" explicitly only when the name does not start with "prepare".

Suggested change

export const prepareDurableZcfMint = (

export const prepareZcfMint = (

erights · 2023-04-27T02:35:37Z

packages/zoe/src/contractFacet/zcfMint.js

-  /** @type {ZcfMintReallocator} */ reallocator,
+/**
+ *
+ * @param {import('@agoric/vat-data').Baggage} zcfBaggage


Thanks for restoring the @param convention, even if for a different reason. With the two side by side, I again find the @param convention more readable. YMMV

erights · 2023-04-27T02:50:03Z

packages/zoe/src/contractFacet/zcfMint.js

-    const add = (total, amountToAdd) =>
-      AmountMath.add(total, amountToAdd, mintyBrand);


This curried add is an example of an issue that pushed me towards lexical state. When there are a lot of these curried functions, that curry over per-instance state, then having to recreate them per method invocation(!) is oppressive.

In this case, it is one function whose creation code only needs to be repeated twice. It does happen on each invocation of the relevant methods, rather than per singleton kind creation. But these are low frequency methods. So in this case, neither the readability cost nor the runtime cost should hold us back from switching to an exoClass.

(Continuing the review, looking for other red flags against using an exoClass for these.)

erights · 2023-04-27T02:55:45Z

packages/zoe/src/contractFacet/zcfMint.js

+        mintyDisplayInfo.assetKind,
+      );
+
+      return { empty, keyword, mintyBrand, zoeMint, mintyIssuerRecord };


It's funny that, as lexical state, the redundancy of mintyBrand vs mintyIssuerRecord.brand would not bother me. As persistent state, it itches, because I think of exoClass instance state as something with potentially high cardinality. In this case, of course, the cardinality is not actually any higher.

Another reason to avoid the redundancy is because it is more persistent state that could be inconsistent. The prior baggage form did not have the redundancy, but (like add) it is because it only needed to restore the lexical variable per kind, not per invocation.

So I'm on the fence. No change suggested. But if the above changed your mind, that change would be fine with me too.

Agree. PTAAL at b200a59

patches/@endo+patterns+0.2.2.patch

Chris-Hibbert

Here's a left-over nit that didn't get sent with the rest of my review. As it says: "if convenient".

Chris-Hibbert · 2023-04-27T16:28:59Z

packages/zoe/src/contractFacet/zcfMint.js

-  /** @type { (exit?: undefined) => { zcfSeat: any; userSeat: Promise<UserSeat> }} */ makeEmptySeatKit,
-  /** @type {ZcfMintReallocator} */ reallocator,
+/**
+ *


drop blank line if convenient.

ok will do in my rebase cleanup

got 'em all 0160e2f

turadg force-pushed the 7502-refactor branch from 967b3b4 to 0a11269 Compare April 26, 2023 15:10

turadg changed the title ~~7502 followup: refactor factory~~ 7502 followup: factor out factory Apr 26, 2023

turadg requested review from erights and Chris-Hibbert April 26, 2023 15:15

turadg marked this pull request as ready for review April 26, 2023 15:15

Chris-Hibbert approved these changes Apr 26, 2023

View reviewed changes

erights approved these changes Apr 27, 2023

View reviewed changes

turadg force-pushed the 7502-refactor branch 3 times, most recently from bb91d3e to f3c4bf7 Compare April 27, 2023 17:26

gibson042 reviewed Apr 27, 2023

View reviewed changes

patches/@endo+patterns+0.2.2.patch Outdated Show resolved Hide resolved

Chris-Hibbert reviewed May 2, 2023

View reviewed changes

turadg added 7 commits May 2, 2023 10:15

docs: spelling fix

c270220

fix(zoe): ZcfMintI mintGains interface

392a6c4

chore(types): more generics

26bdf95

feat(zoe): assertFullIssuerRecord

dacaa74

refactor(zcf): simplify kinds

2cd31a2

refactor(zcfMint): store only precious state

b647129

style: drop blank first line of jsdoc

0160e2f

turadg force-pushed the 7502-refactor branch from b200a59 to 0160e2f Compare May 2, 2023 17:18

turadg added the automerge:no-update (expert!) Automatically merge without updates label May 2, 2023

Merge branch 'master' into 7502-refactor

5f64ce9

mergify bot merged commit 3a98338 into master May 2, 2023

mergify bot deleted the 7502-refactor branch May 2, 2023 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7502 followup: factor out factory #7519

7502 followup: factor out factory #7519

turadg commented Apr 25, 2023 •

edited

Loading

Chris-Hibbert left a comment

Chris-Hibbert Apr 26, 2023

turadg Apr 26, 2023

Chris-Hibbert Apr 26, 2023

turadg Apr 26, 2023

Chris-Hibbert Apr 26, 2023

turadg Apr 26, 2023

erights left a comment

erights Apr 27, 2023

erights Apr 27, 2023

erights Apr 27, 2023

erights Apr 27, 2023

turadg May 2, 2023

Chris-Hibbert left a comment

Chris-Hibbert Apr 27, 2023

turadg May 2, 2023

turadg May 2, 2023

		mintGains: M.call(AmountKeywordRecordShape, M.any()).returns(SeatShape),
		burnLosses: M.call(AmountKeywordRecordShape, M.any()).returns(

	* @callback ZCFMintMintGains
	* @param {AmountKeywordRecord} gains
	* @param {ZCFSeat} [zcfSeat]
	* @returns {ZCFSeat}
	*/

		/** @type {Amount<AssetKind>} */ total,
		/** @type {Amount<AssetKind>} */ amountToAdd,

	export const prepareDurableZcfMint = (
	export const prepareZcfMint = (

		const add = (total, amountToAdd) =>
		AmountMath.add(total, amountToAdd, mintyBrand);

7502 followup: factor out factory #7519

7502 followup: factor out factory #7519

Conversation

turadg commented Apr 25, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

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

erights 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

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

turadg commented Apr 25, 2023 •

edited

Loading