fix: simplify singletons by erights · Pull Request #5674 · Agoric/agoric-sdk

erights · 2022-06-27T06:09:54Z

We've already noticed that singletons always have empty state records, and so their behavior methods never need to use their state. @dtribble noticed that we can simply use JS's const scoping (temporal dead zone) to make the self or facets arguments unnecessary. At that point the context is unnecessary and we just need the "normal" methods. Without common state, there's also little reason to group these into a cohort. Thus, our support for singletons can be much closer to Far. To emphasize this, at this time this PR used the term ProvideFar, but we should bikeshed.

dtribble · 2022-06-27T06:14:19Z

For VaultManager, my singleton conversion still has a state record. What happens to everything in that state record?

Chris-Hibbert · 2022-06-27T17:54:45Z

I thought I pushed a review earlier, but it's disappeared, sorry. This seems like it might be too simple. It looks much more like our normal JS code, and not much like our durable code at this point. I think that might be a point against it.

erights · 2022-06-27T17:57:29Z

I thought I pushed a review earlier, but it's disappeared, sorry. This seems like it might be too simple. It looks much more like our normal JS code, and not much like our durable code at this point. I think that might be a point against it.

That is exactly why I am so excited ;)
Why a point against?

Chris-Hibbert · 2022-06-27T17:58:57Z

Why a point against?

Just that it doesn't look like the other durable objects, It's not recognizably durable at a glance.

erights · 2022-06-27T18:00:36Z

Github mystery: I thought I had originally listed @Chris-Hibbert as a reviewer. When I try to add him (back), Github does not add him. I have never seen this before. Any idea?

Chris-Hibbert · 2022-06-27T18:01:24Z

You definitely had listed me. I remember seeing it. I don't know what's going on.

Chris-Hibbert

I misunderstood. This is a significant improvement.

turadg

👌

packages/ERTP/src/paymentLedger.js

turadg · 2022-06-28T14:19:34Z

packages/vat-data/src/kind-utils.js

-  behavior,
-  options = undefined,
-) => {
+export const ProvideFar = (baggage, kindName, methods, options = undefined) => {


Suggested change

export const ProvideFar = (baggage, kindName, methods, options = undefined) => {

/**

* @template T

* @param {import('@agoric/store').MapStore<string, unknown>} baggage

* @param {string} kindName

* @param {T} methods

* @param {import('./types.js').DefineKindOptions<unknown>} [options]

* @returns {import('@endo/far').ERef<T>}

*/

export const ProvideFar = (baggage, kindName, methods, options) => {

I did something similar but different at 98a96e9

I left the optional parameter syntax on the options argument. I think we should in general.

I replaced the ERef<T> with simply T. However, I see at https://github.com/endojs/endo/blob/8077e2134615d6a1cbe8c8f5350f664b56f0a17b/packages/marshal/src/make-far.js#L88 the similar T & RemotableBrand<{}, T>. Please let me know if I should use this instead.

Attn @michaelfig who authored RemotableBrand

PTAL

I'm not sure. I need to better understand the remote call and marshalling systems. I do know the types are a loose approximation so far.

turadg · 2022-06-28T14:21:45Z

packages/vat-data/src/kind-utils.js

+  const behavior = fromEntries(
+    entries(methods).map(([k, m]) => [k, dropContext(m)]),
+  );


mapping object values at the existing keys is a common need so in run-protocol package we have a helper

agoric-sdk/packages/run-protocol/src/collect.js

Lines 6 to 8 in 68d27f3

export const mapValues = (obj, f) =>

// @ts-expect-error entries() loses the K type

harden(fromEntries(entries(obj).map(([p, v]) => [p, f(v, p)])));

should we have it in a more shared package? @agoric/utils?

Yes, I have a similar one, yes with an extensive comment, at

agoric-sdk/packages/zoe/src/objArrayConversion.js

Lines 45 to 48 in 86d58b9

* By analogy with how `Array.prototype.map` will map the elements of

* an array to transformed elements of an array of the same shape,

* `objectMap` will do likewise for the string-named own enumerable

* properties of an object.

Which of these two do you prefer?

Like mapValues it is not reusable only because of where it is. However, @agoric/utils would imply that it is in agoric-sdk and so not reusable from the endo repository. Attn @kriskowal is there a natural endo package to migrate such things to? Should there be?

In the meantime, vat-data should clearly not depend on run-protocol or zoe. But I can at least make it a copy of one of these with a note to depend on a common one once a suitable place is found. In the absence of other feedback, I'll copy objectMap. Thanks.

@turadg I copied objectMap but let me know if I should change it to mapValues.

Despite the two PTALs above, I'm going to merge this PR into #5283 . But still, please do PTAL and comment either here or (preferred) on #5283 . Thanks.

Agree we should find a place for utilities like this and it should be somewhere in @endo. I'd like to see something analogous to Lodash but SES-smart. @kriskowal WDYT of that? I can make a ticket and I'd be very happy to work on it after MN-1.

As to whether mapValues or objectMap goes in the utility package, I think mapValues fits the common use case better: mapping values without regard to the keys. objectMap takes a function that allows keyword transformations so it's not doing much abstracting over Object.fromEntries(Object.entries.map(mapPairFn));

erights requested review from Chris-Hibbert, FUDCo, dtribble and warner June 27, 2022 06:09

erights self-assigned this Jun 27, 2022

erights mentioned this pull request Jun 27, 2022

feat(ertp): durability #5283

Merged

erights force-pushed the markm-durable-ertp branch from 96bb0bc to 767e937 Compare June 27, 2022 19:17

erights requested a review from turadg as a code owner June 27, 2022 19:17

erights force-pushed the markm-simplify-singletons branch from ab2192f to 68d27f3 Compare June 27, 2022 19:17

Chris-Hibbert approved these changes Jun 27, 2022

View reviewed changes

turadg approved these changes Jun 28, 2022

View reviewed changes

erights force-pushed the markm-durable-ertp branch from 767e937 to 4ba2c9e Compare June 28, 2022 19:27

erights force-pushed the markm-simplify-singletons branch from 68d27f3 to 03476ed Compare June 28, 2022 19:28

Chris-Hibbert force-pushed the markm-durable-ertp branch from 4ba2c9e to 3552107 Compare June 28, 2022 19:52

Chris-Hibbert force-pushed the markm-simplify-singletons branch from 98a96e9 to c6e251a Compare June 28, 2022 19:53

erights force-pushed the markm-durable-ertp branch from 3552107 to 4ba2c9e Compare June 28, 2022 20:18

erights force-pushed the markm-simplify-singletons branch from c6e251a to 1c13422 Compare June 28, 2022 20:19

erights force-pushed the markm-durable-ertp branch from 4ba2c9e to 3c6f10c Compare June 28, 2022 20:22

erights added 3 commits June 28, 2022 13:24

fix: simplify singletons

748efb2

fix: based on review comment but different

eecdb63

fix: copy and use objectMap with note

3a856b6

erights force-pushed the markm-simplify-singletons branch from 1c13422 to 3a856b6 Compare June 28, 2022 20:26

erights requested review from turadg and removed request for Chris-Hibbert June 28, 2022 20:32

erights changed the title ~~simplify singletons~~ fix: simplify singletons Jun 28, 2022

erights merged commit fe8a202 into markm-durable-ertp Jun 28, 2022

erights deleted the markm-simplify-singletons branch June 28, 2022 20:34

erights added a commit that referenced this pull request Jun 29, 2022

fix: simplify singletons (#5674)

c25c063

-export const ProvideFar = (baggage, kindName, methods, options = undefined) => {
+/**
+ * @template T
+ * @param {import('@agoric/store').MapStore<string, unknown>} baggage
+ * @param {string} kindName
+ * @param {T} methods
+ * @param {import('./types.js').DefineKindOptions<unknown>} [options]
+ * @returns {import('@endo/far').ERef<T>}
+ */
+export const ProvideFar = (baggage, kindName, methods, options) => {

	export const mapValues = (obj, f) =>
	// @ts-expect-error entries() loses the K type
	harden(fromEntries(entries(obj).map(([p, v]) => [p, f(v, p)])));

	* By analogy with how `Array.prototype.map` will map the elements of
	* an array to transformed elements of an array of the same shape,
	* `objectMap` will do likewise for the string-named own enumerable
	* properties of an object.

Conversation

erights commented Jun 27, 2022

Uh oh!

dtribble commented Jun 27, 2022

Uh oh!

Chris-Hibbert commented Jun 27, 2022

Uh oh!

erights commented Jun 27, 2022

Uh oh!

Chris-Hibbert commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erights commented Jun 27, 2022

Uh oh!

Chris-Hibbert commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Uh oh!

turadg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

turadg Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

erights Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

turadg Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

turadg Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

erights Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

erights Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

turadg Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Chris-Hibbert commented Jun 27, 2022 •

edited

Loading

Chris-Hibbert commented Jun 27, 2022 •

edited

Loading