how to expose the Virtual/Durable Collection API? global? module import?

[warner]

What is the Problem Being Solved?

How should we make the new Collections API (#2004) available to user-level code?

Imagine a library that uses our new VirtualMap or DurableSet or whatever. Should this library start with import { VirtualMap } from '@agoric/collections' ? Or should it look for globalThis.VirtualMap? Or should it require VirtualMap to be passed in as an argument?

There are two drivers here. The first is whether we're comfortable having all pieces of this API be ambiently available. @erights has expressed concern that the Durable subset represents sufficiently more authority than the merely Virtual or Ephemeral portions, since Durable data can (in fact must) survive an upgrade, whereas Ephemeral is entirely in RAM and Virtual is just like RAM but bigger. I'm still of the opinion that basically everything should be Durable, or at least that any program which uses Virtual instead of Durable is running the risk of making the wrong decision and being in trouble when it comes to upgrade. So I'm less worried about withholding Durable from user code.

If we decide Durable should not be ambient, then it must be passed as an argument. That would still leave the question of how Virtual should be delivered.

The other driver is that we have two environments where these features might be used. The main one is inside a swingset vat, where both Virtual and Durable collections will be wired to DB access managed by liveslots and the virtual object manager. Code that runs here is loaded into a Compartment, which gives us some measure of control over how it imports things.

But an equally important one is in unit tests, run under Node.js, outside of any Compartment (to be precise it runs in the start compartment). It is critical that libraries should be unit testable without too much fuss, without building an entire swingset kernel to host it.

We have three rough approaches we could take.

globals

The simplest is to provide the collections as a global, perhaps globalThis.collections or globalThis.swingsetCollectionsAPI. Under swingset, liveslots prepares the collections globals at the same time it prepares the #2724 modified WeakMap/WeakSet. It would add these to the globalThis of the new vat Compartment before freezing it. Then we either modify ZCF to copy the globals into the contract's Compartment (importBundle(contractBundle, { globals: { ...globalThis } })), or we modify importBundle to add a feature that automatically copies some globals into new child compartments (similar to how we currently force-inherit inescapableGlobalLexicals). Or we put the collections API objects on inescapableGlobalLexicals, although that seems unsatisfying.

Under unit tests, library developers are obligated to import a @agoric/fake-collection-shim from their test program, before their library gets a chance to look at the global. This shim would modify the global to add the necessary API objects. This looks just like what we're already doing with SES/lockdown and eventual-send/HandledPromise. So we'd publish just one module to NPM: @agoric/fake-collection-shim.

module import (plus globals)

The second approach is to provide the collections on a global, but ask users to import { VirtualMap } from '@agoric/collections' anyways. This package (which we would publish to NPM) would contain a trivial re-export of the globals. Unit tests would import @agoric/fake-collection-shim in their test driver file, and import @agoric/collections in their library file. When a vat or contract is bundled for the chain, the archive would include a copy of @agoric/collections. Swingset would load that copy as expected, but would provide the necessary globals first.

The upside (in my mind) is discoverability. Library code that uses collections would have an obvious import statement, users/code-readers could look up @agoric/collections on NPM and find documentation (even if the implementation is elsewhere).

The (well, one) downside is that the global would still be present, and users might come to rely upon it rather than using the imported form. A library could use c = new swingsetCollections.VirtualSet() without any import, and it would work by-accident.

module import (without globals)

To address the last problem, we could have swingset prepare two separate Compartments. In the first one, swingset provides the collections API as a global, imports @agoric/collections, and collects the result as a Module Namespace Object. Then, swingset evaluates the vat code in a second Compartment which lacks the globals, but provides that MNO as a compartment-map override, so when the vat code performs the import, it gets the swingset-provided MNO instead of evaluating the contents of @agoric/collections itself.

The result is that the vat code doesn't see the globals, partially removing the hazard that people might use the API from the wrong place and come to rely upon it being there. However this doesn't help the unit test case: the global is still present, and unit test code could come to rely upon it being on the global without error. And unit tests are really the one opportunity we have to educate users about what is supposed to work and what isn't.

(note: the global name might be sufficiently different than the imported one to reduce this hazard)

Also, this approach is more complicated, because the compartment-map override is new territory (I think it'd be the first time this feature would be used in anger). In particular, the code that creates the bundle needs to be aware of the fact that @agoric/collections is a "hole" to be filled in later, by the swingset-time loading code. If the bundler doesn't agree with the loader about where the holes are, bad things will happen. One possibility is that the archive contains a copy of @agoric/collections even though the loader is told to replace it, and we think the loader would honor the archive over the override. Another possibility is that the bundler omits a module and the loader doesn't replace it, in which case the loader will fail outright.

What to do?

@michaelfig suggested the third approach to me (assuming I understood our conversation correctly). @kriskowal recommended the first approach (just use a global) and has talked me out of the third approach (import override). I'm still hoping for the documentation/discovery aspects of the second approach (import), but I'm coming around to using the first (global).

I'm inclined to allow ambient access to Durable, but if we really don't want that, we might put Virtual on the global, and pass Durable through vatPowers. Since "Durable" is closely tied to upgrade planning, and since upgrade planning also involves the "baggage" index (to pass pointers to Durable collections through to the new version), it might make a lot of sense to pass the Durable constructors through the same object that provides access to the baggage.

Security Considerations

• should Durable be ambient?
• if we use module overrides, do auditors need to be worried about what other modules appeared in the original source but have been replaced by the runtime?
  • correspondingly, in the use-a-global approach, they need to be aware of what globalThis contains and where it comes from
• can these APIs provide a resource-exhaustion vector, or is that sufficiently covered by the Metering we do elsewhere?

Test Plan

[michaelfig]

@michaelfig suggested the third approach to me (assuming I understood our conversation correctly). @kriskowal recommended the first approach (just use a global) and has talked me out of the third approach (import override). I'm still hoping for the documentation/discovery aspects of the second approach (import), but I'm coming around to using the first (global).

If you're steering away from the third approach, I'd much prefer the second approach.

Regardless of whether you do the first or the second, please do make a container for all SwingSet-specific globals, rather than pooping all over the globalThis. For that purpose, I had once suggested globalThis.Vat, with properties like globalThis.Vat.VirtualSet, etc. It's weird enough (and to this day still bothers me) that running the SES shim gives you globalThis.assert. globalThis.harden seems more justifiable.

Please Keep the Global Namespaces Clean! When we don't, the aftermath will live on much longer than any of us.

[warner]

Current approach is to add globalThis.VatData, which will be a hardened record containing the various permutations of makeVirtualMap/etc. We'll move makeKind from globalThis.makeKind to globalThis.VatData.makeKind, to achieve conservation of poop. At wednesday's kernel meeting we can sort out global-vs-module and the right name for this.

I'm thinking that these virtual/durable collections are "owned by X or "provided by X" or "provided as part of the X environment" for some value of X on a spectrum from "agoric" to "swingset" to "vat" to "endo", but more towards the vat/endo side of that spectrum. Just like "the ability to send messages" is somewhere there, and E, and harden, and maybe assert. Vats may or may not be supported by swingset (we can imagine some other hosting technology that provides the same sort of environment), so calling it VatData rather than SwingsetData feels better. But if we pull in E and some others, then maybe globalThis.Vat should have E too.

const { E, makeKind, harden } = globalThis.VatStuff;

[Tartuffo]

@erights needs time to re-absorb the Virtual Object stuff to decide if this is good enough, or needs a new API. Mark and Chris need to try this stuff out in Zoe and standard contracts.

[erights]

Although I am not finished reviewing #4316 , I am far enough to settle the question raised in this issue. The way this PR does it, a compartment-global VatData, is not my favorite but it is fine. We can certainly go to MN-1 with it. If there are no rude surprises (I expect none), then we can just keep doing it that way.

I think this should close this issue, so I'm closing it. If I'm missing something, please reopen and let me know.

[erights]

I figured out what I'm missing. Reopening and assigning to @FUDCo and @michaelfig

This VatData global only works for us when we've given it static scoping and static typing plumbing like we've done for harden and assert. When I hovered over makeScalarBigWeakMapStore it said any. Then I noticed this example file was missing a // @ts-check, so I tried adding one. The any is still there. In addition, it now complains that VatData is not defined. Oh of course! So I switched the // @ts-check with the /* global VatData */ and got

Poking around, the /* global foo */ declarations talk to eslint, which does otherwise complain about VatData being undefined. But with the // @ts-check, as you can see in the popup, this time the error comes from TypeScript.

[erights]

@turadg , you seem interested in getting such matters straightened out, so assigning to you too. Thanks!

[erights]

Damn. In the swingset-runner package with the modification shown above, yarn lint doesn't complain. It looks like it is only doing an eslint check, not a ts check. I only know about the typing problem because of vscode. This of course means it is missed in CI as well.

[kriskowal]

The inconsistent results between eslint and vscode can probably be addressed by upgrading @endo/eslint-config, but this will require a PR that holistically fixes eslint errors throughout the monorepo on uncovered files.

The relevant change to eslint-config can be surgically added to individual package.json eslintConfig to gradually tighten coverage https://github.com/endojs/endo/blob/master/packages/eslint-config/eslint-config.json#L85-L100

[michaelfig]

I'd love to have a module that's a thin wrapper around globalThis.VatData, and that provides typing information. That's by far the easiest way to support the need for types and makes dependencies explicit in the source code.

Especially since globalThis.VatData won't be standardised as part of SES (unlike harden and assert).

[warner]

Ok I think we've converged on globalThis.VataData within the vat environment, and an as-yet-unnamed and unhoused module to re-export and provide types.

@FUDCo has implemented this module, feel free to use it during immediate development (cc @turadg): a deep import of packages/SwingSet/src/storeModule.js (which I think means import { makeKind, makeScalarBigMapStore } from '@agoric/swingset-vat/src/storeModule.js'). I don't know if it provides enough type information to do the right thing: patches welcome.

Obviously we don't want keep such a deep import around (eww unstructured rummaging about in other package's file hierarchy), and the product-naming questions abound. I'd like someone else to figure out what package this re-export+types should live in. I think @agoric/store is not an option because of the circularity it would introduce, so I'm guessing it'll need to be a new package, unfortunately.

[mhofman]

FYI, I believe we'll need some kind of compartment override for proxy stamping and marshal usage in the contract compartment (see #3905)

[dtribble]

Right now, makeKind gets imported from a deep import. It should likely be something like:

import { makeKind } from '@agoric/storage';

[FUDCo]

We are currently debating the right place for the package to live. One of the possibilities is to just create a package of its own, which is basically what you propose. I think that would be a fine approach. The concern I'd have with the specific proposal there is people geting confused between @agoric/storage and @agoric/store, since the names don't suggest which of the two you should use.

[erights]

@agoric/virtualStore ?

[michaelfig]

@agoric/virtualStore ?

Maybe @agoric/virtual-store? Uppercase is not allowed in package names.

[turadg]

Since VatData is the name for the object containing them, wouldn't it be good to keep that?

More generally the problem here is resolving a global. It's either there or it isn't, so something like this would give that guarantee,

import { VatData } from '@agoric/globals';

And that module would have something like,

/* global VatData */
assert(VatData, 'VatData not defined');
export { VatData };

In what environments should this global be available? If's exactly vats, then perhaps,

import { VatData } from `@agoric/vats';

[FUDCo]

I think the idea is that we want use an imported module to hide the fact that this functionality is being injected into the environment via a global, because the test scaffolding and the actual SwingSet runtime set things up at different times and in different ways, and we'd like code that uses this functionality to be able to be agnostic as to which environment it's running in. In particular, we'd like some environments to be able to acquire the functionality via an import without the global ever being involved.

[erights]

@turadg and I were just discussing the package that exists for the sole purpose of re-exporting the properties of VatData as named exports. As part of that, we discussed the naming of that package. @turadg suggested the "obvious" one we all missed: @agoric/vat-data. If VatData was the right choice for the temporarily exposed global namespace object, then it should also be the right choice for the corresponding module.

[erights]

Please see #4618 (comment)