Merge pull request #7668 from Agoric/7632-cleanup-virtual-collection-…

…api-surfaces

Correct infelicities in virtual collection API implementation

packages/store/src/index.js

@@ -56,7 +56,7 @@ export { makeScalarSetStore } from './stores/scalarSetStore.js';
 export { makeScalarWeakMapStore } from './stores/scalarWeakMapStore.js';
 export { makeScalarMapStore } from './stores/scalarMapStore.js';
 
-export { provideLazy } from './stores/store-utils.js';
+export { provideLazy, isCopyMap, isCopySet } from './stores/store-utils.js';
 
 // /////////////////////// Deprecated Legacy ///////////////////////////////////
 

packages/store/src/stores/scalarWeakMapStore.js

@@ -58,8 +58,12 @@ export const makeWeakMapStoreMethods = (
     },
 
     addAll: entries => {
-      if (isCopyMap(entries)) {
-        entries = getCopyMapEntries(entries);
+      if (typeof entries[Symbol.iterator] !== 'function') {
+        if (Object.isFrozen(entries) && isCopyMap(entries)) {
+          entries = getCopyMapEntries(entries);
+        } else {
+          Fail`provided data source is not iterable: ${entries}`;
+        }
       }
       for (const [key, value] of /** @type {Iterable<[K, V]>} */ (entries)) {
         // Don't assert that the key either does or does not exist.

packages/store/src/stores/scalarWeakSetStore.js

@@ -42,8 +42,12 @@ export const makeWeakSetStoreMethods = (
     },
 
     addAll: keys => {
-      if (isCopySet(keys)) {
-        keys = getCopySetKeys(keys);
+      if (typeof keys[Symbol.iterator] !== 'function') {
+        if (Object.isFrozen(keys) && isCopySet(keys)) {
+          keys = getCopySetKeys(keys);
+        } else {
+          Fail`provided data source is not iterable: ${keys}`;
+        }
       }
       for (const key of /** @type {Iterable<K>} */ (keys)) {
         assertKeyOkToAdd(key);

packages/swingset-liveslots/NEWS.md

@@ -0,0 +1,41 @@
+NEXT VERSION
+
+This version introduces three changes to the virtual collections API
+implementation.  These changes bring the `ScalarBigXXXStore` collection types
+into compliance with the API as defined in the `store` package, but they _do_
+introduce potential compatibility issues that should be considered.  The changes
+are:
+
+* Removal of the `addToSet` method from the `ScalarBigSetStore` and
+  `ScalarBigWeakSetStore` types.  This was an internal implementation method
+  that should never have been present.  Since it was internal method that nobody
+  knew about it, we believe it highly unlikely to be in use.  Furthermore, we
+  have verified by inspection that there appears to be no existing contract or
+  vat code that made use of it.  However, its removal _is_ an incompatible
+  change that people should be aware of.  If anyone had been using it (in, say,
+  test code), they should replace calls to it with calls to `add`, which
+  performs the same operation.
+
+* Removal of the `entries` method from the `ScalarBigSetStore` type.  This was
+  implemented in a mistaken attempt to be compatible with the JavaScript `Set`
+  type, but stores have a slightly different API and this method should not have
+  been present.  As with `addToSet`, we have inspected existing code to verify
+  that it is not in actual use; however, while extreme care was taken during
+  this inspection, due to the ubiquity of the method name `entries` on other
+  types, we cannot be quite as confident in the inspection's correctness as we
+  are with the `addToSet` method.  One source of assurance on this score is that
+  it is common practice for tests of contract code to substitue the `SetStore`
+  implementation from the `stores` package, which lacks the offending method
+  already.  Existing uses, if any, should be replaced with calls to either the
+  `keys` or `values` method (they are equivalent), with suitable alterations to
+  account for the fact that these return a single value rather than a pair.
+
+* Addition of the `addAll` method to all the virtual collection types, which was
+  an omission from the original implementation.  Care should be taken to ensure
+  that developmental vat or contract code using this method is not deployed
+  prior to the deployment of this version of Liveslots.
+
+PRIOR VERSIONS
+
+Liveslots version `@agoric/[email protected]` was deployed to the
+mainnet1B chain as part of `@agoric/[email protected]`

packages/swingset-liveslots/src/collectionManager.js

@@ -15,7 +15,10 @@ import {
   makeCopySet,
   makeCopyMap,
   getRankCover,
+  getCopyMapEntries,
+  getCopySetKeys,
 } from '@endo/patterns';
+import { isCopyMap, isCopySet } from '@agoric/store';
 import { makeBaseRef, parseVatSlot } from './parseVatSlots.js';
 import {
   enumerateKeysStartEnd,
@@ -624,6 +627,36 @@ export function makeCollectionManager(
     const snapshotMap = (keyPatt, valuePatt) =>
       makeCopyMap(entries(keyPatt, valuePatt));
 
+    const addAllToSet = elems => {
+      if (typeof elems[Symbol.iterator] !== 'function') {
+        if (Object.isFrozen(elems) && isCopySet(elems)) {
+          elems = getCopySetKeys(elems);
+        } else {
+          Fail`provided data source is not iterable: ${elems}`;
+        }
+      }
+      for (const elem of elems) {
+        addToSet(elem);
+      }
+    };
+
+    const addAllToMap = mapEntries => {
+      if (typeof mapEntries[Symbol.iterator] !== 'function') {
+        if (Object.isFrozen(mapEntries) && isCopyMap(mapEntries)) {
+          mapEntries = getCopyMapEntries(mapEntries);
+        } else {
+          Fail`provided data source is not iterable: ${mapEntries}`;
+        }
+      }
+      for (const [key, value] of mapEntries) {
+        if (has(key)) {
+          set(key, value);
+        } else {
+          doInit(key, value, true);
+        }
+      }
+    };
+
     return {
       has,
       get,
@@ -635,6 +668,8 @@ export function makeCollectionManager(
       keys,
       values,
       entries,
+      addAllToSet,
+      addAllToMap,
       snapshotSet,
       snapshotMap,
       sizeInternal,
@@ -647,12 +682,23 @@ export function makeCollectionManager(
     const hasWeakKeys = storeKindInfo[kindName].hasWeakKeys;
     const raw = summonCollectionInternal(initial, collectionID, kindName);
 
-    const { has, get, init, addToSet, set, delete: del } = raw;
+    const {
+      has,
+      get,
+      init,
+      addToSet,
+      addAllToMap,
+      addAllToSet,
+      set,
+      delete: del,
+    } = raw;
     const weakMethods = {
       has,
       get,
       init,
       addToSet,
+      addAllToSet,
+      addAllToMap,
       set,
       delete: del,
     };
@@ -764,63 +810,84 @@ export function makeCollectionManager(
   }
 
   function collectionToMapStore(collection) {
-    const { snapshotSet: _, snapshotMap, ...rest } = collection;
-    return Far('mapStore', { snapshot: snapshotMap, ...rest });
+    const {
+      has,
+      get,
+      init,
+      set,
+      delete: del,
+      addAllToMap,
+      keys,
+      values,
+      entries,
+      snapshotMap,
+      getSize,
+      clear,
+    } = collection;
+    const mapStore = {
+      has,
+      get,
+      init,
+      set,
+      delete: del,
+      addAll: addAllToMap,
+      keys,
+      values,
+      entries,
+      snapshot: snapshotMap,
+      getSize,
+      clear,
+    };
+    return Far('mapStore', mapStore);
   }
 
   function collectionToWeakMapStore(collection) {
-    return Far('weakMapStore', collection);
+    const { has, get, init, set, delete: del, addAllToMap } = collection;
+    const weakMapStore = {
+      has,
+      get,
+      init,
+      set,
+      delete: del,
+      addAll: addAllToMap,
+    };
+    return Far('weakMapStore', weakMapStore);
   }
 
   function collectionToSetStore(collection) {
     const {
       has,
       addToSet,
       delete: del,
+      addAllToSet,
       keys,
-      getSize,
       snapshotSet,
+      getSize,
       clear,
     } = collection;
-    function* entries(patt) {
-      for (const k of keys(patt)) {
-        yield [k, k];
-      }
-    }
-    function addAll(elems) {
-      for (const elem of elems) {
-        addToSet(elem, null);
-      }
-    }
 
     const setStore = {
       has,
       add: addToSet,
-      addAll,
       delete: del,
+      addAll: addAllToSet,
       keys: patt => keys(patt),
       values: patt => keys(patt),
-      entries,
-      getSize: patt => getSize(patt),
       snapshot: snapshotSet,
+      getSize: patt => getSize(patt),
       clear,
     };
     return Far('setStore', setStore);
   }
 
   function collectionToWeakSetStore(collection) {
-    const { has, addToSet, delete: del } = collection;
-    function addAll(elems) {
-      for (const elem of elems) {
-        addToSet(elem);
-      }
-    }
+    const { has, addToSet, delete: del, addAllToSet } = collection;
 
     const weakSetStore = {
       has,
       add: addToSet,
-      addAll,
       delete: del,
+      addAll: addAllToSet,
     };
     return Far('weakSetStore', weakSetStore);
   }

packages/swingset-liveslots/test/test-collections.js

@@ -3,6 +3,7 @@ import '@endo/init/debug.js';
 
 import { Far } from '@endo/marshal';
 import { M } from '@agoric/store';
+import { makeCopyMap, makeCopySet } from '@endo/patterns';
 import { makeFakeCollectionManager } from '../tools/fakeVirtualSupport.js';
 
 const {
@@ -191,6 +192,89 @@ test('basic weak set operations', t => {
   );
 });
 
+function exerciseSetAddAll(t, weak, testStore) {
+  const allThatStuff = stuff.map(entry => entry[0]);
+
+  testStore.addAll(allThatStuff);
+  for (const elem of allThatStuff) {
+    t.truthy(testStore.has(elem));
+    testStore.delete(elem);
+  }
+  if (!weak) {
+    t.is(testStore.getSize(), 0);
+  }
+
+  testStore.addAll(makeCopySet(allThatStuff));
+  for (const elem of allThatStuff) {
+    t.truthy(testStore.has(elem));
+    testStore.delete(elem);
+  }
+  if (!weak) {
+    t.is(testStore.getSize(), 0);
+  }
+
+  t.throws(
+    () => testStore.addAll({ bogus: 47 }),
+    m(/provided data source is not iterable/),
+  );
+}
+
+test('set addAll', t => {
+  exerciseSetAddAll(t, false, makeScalarBigSetStore('test set'));
+});
+
+test('weak set addAll', t => {
+  exerciseSetAddAll(t, true, makeScalarBigWeakSetStore('test weak set'));
+});
+
+test('set snapshot', t => {
+  const testStore = makeScalarBigSetStore('test set');
+  const allThatStuff = stuff.map(entry => entry[0]);
+  testStore.addAll(allThatStuff);
+  t.deepEqual(testStore.snapshot(), makeCopySet(allThatStuff));
+});
+
+function exerciseMapAddAll(t, weak, testStore) {
+  testStore.addAll(stuff);
+  for (const [k, v] of stuff) {
+    t.truthy(testStore.has(k));
+    t.is(testStore.get(k), v);
+    testStore.delete(k);
+  }
+  if (!weak) {
+    t.is(testStore.getSize(), 0);
+  }
+
+  testStore.addAll(makeCopyMap(stuff));
+  for (const [k, v] of stuff) {
+    t.truthy(testStore.has(k));
+    t.is(testStore.get(k), v);
+    testStore.delete(k);
+  }
+  if (!weak) {
+    t.is(testStore.getSize(), 0);
+  }
+
+  t.throws(
+    () => testStore.addAll({ bogus: 47 }),
+    m(/provided data source is not iterable/),
+  );
+}
+
+test('map addAll', t => {
+  exerciseMapAddAll(t, false, makeScalarBigMapStore('test map'));
+});
+
+test('weak map addAll', t => {
+  exerciseMapAddAll(t, true, makeScalarBigWeakMapStore('test weak map'));
+});
+
+test('map snapshot', t => {
+  const testStore = makeScalarBigMapStore('test map');
+  testStore.addAll(stuff);
+  t.deepEqual(testStore.snapshot(), makeCopyMap(stuff));
+});
+
 test('constrain map key shape', t => {
   const stringsOnly = makeScalarBigMapStore('map key strings only', {
     keyShape: M.string(),
@@ -766,13 +850,6 @@ test('set queries', t => {
     symbolKrusty,
     undefined,
   ]);
-
-  // @ts-expect-error our BigSetStore has .entries, but not the SetStore type
-  t.deepEqual(Array.from(testStore.entries(M.number())), [
-    [-29, -29],
-    [3, 3],
-    [47, 47],
-  ]);
 });
 
 test('remotable sort order', t => {