-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change zoe/etc tests to use bundlecaps, not bundles #4487
Comments
Zoe needs a way to create a new ZCF vat. Inside swingset, zoe uses `vatAdminService`, and this is used from unit tests (zoe and other packages that use zoe) when they are willing to take the time to run a full swingset environment. Unit tests that use zoe, but not swingset, use `tools/fakeVatAdmin.js` as a replacement, which implements `createVat(bundle)` but not `createVatByName(name)`. This is used to evaluate the ZCF bundle in a new Compartment (using `importBundle` and `evalContractBundle`). The primary export of `@agoric/zoe` is `makeZoeKit()`, which is called with `vatAdminService` and an optional `zcfBundleName`. This function runs `setupCreateZCFVat(vatAdminService)` to attenuate the vat-making power into one that can only create ZCF vats. Previously, `setupCreateZCFVat` closed over a copy of the ZCF bundle, and passed it to `vatAdminService~.createVat(zcfBundle)`. This hides the existence of the ZCF bundle from other packages that just want to use Zoe. However, to remove these large code bundles from userspace messages, we need the ZCF bundle to be installed "off to the side", by the code that prepares the swingset environment (usually as `config.bundles.zcf=`). This commit changes `fakeVatAdmin.js` to implement `createVatByName('zcf')`, and to move the copy of the zcfBundle out of `makeZoeKit()` and into `fakeVatAdmin.js` . In addition, it changes `createZCFVat.js` to provide a default bundle name of 'zcf'. Any unit test that uses `fakeVatAdmin.js` can continue to do so without changes, and the fake service will magically know how to create Zoe's ZCF vat without any additional configuration. Unit tests that use swingset, however, need to be updated to install the ZCF bundle into the kernel, with something like: ``` import zcfBundle from `@agoric/zoe/bundles/bundle-contractFacet.js`; ... config.bundles.zcf = { bundle: zcfBundle }; ``` Note: if we used `{ sourceSpec: '@agoric/zoe/contractFacet.js' }`, then we would not depend upon a recent `cd packages/zoe && yarn build`, and each kernel instance would bundle its own copy. This would be slower, but perhaps less prone to stale-bundle surprises, and might be a nicer export commitment). refs #4487
This updates unit tests in (zoe, governance, run-protocol), specifically those which use a swingset environment, to fetch the ZCF contract bundle from the Zoe package, and install it into the kernel config they build. This is necessary now that Zoe's `setupCreateZCFVat` no longer includes a copy. Any new tests which follow this pattern will need to do the same. refs #4487
I have a first step for this in the 4487-zoe-bundlecaps branch (look at the last two commits). The description says: Zoe needs a way to create a new ZCF vat. Inside swingset, zoe uses Unit tests that use zoe, but not swingset, use The primary export of However, to remove these large code bundles from userspace messages, we need This commit changes Unit tests that use swingset, however, need to be updated to install the ZCF
Note: if we used What To Export?I'm always hesitant to use "deep imports": e.g. Currently I think the only deliberate export is So I think we need an alternate export from
I could use some advice on what sort of interface Zoe should be committing to, and whether this approach feels comfortable. Note that this is just the first step. The second step will involve using bundlecaps instead of the hard-coded |
Ok, @Chris-Hibbert and @dckc encouraged me to be ok with committing Zoe to a pre-bundled export at |
Zoe needs a way to create a new ZCF vat. Inside swingset, zoe uses `vatAdminService`, and this is used from unit tests (zoe and other packages that use zoe) when they are willing to take the time to run a full swingset environment. Unit tests that use zoe, but not swingset, use `tools/fakeVatAdmin.js` as a replacement, which implements `createVat(bundle)` but not `createVatByName(name)`. This is used to evaluate the ZCF bundle in a new Compartment (using `importBundle` and `evalContractBundle`). The primary export of `@agoric/zoe` is `makeZoeKit()`, which is called with `vatAdminService` and an optional `zcfBundleName`. This function runs `setupCreateZCFVat(vatAdminService)` to attenuate the vat-making power into one that can only create ZCF vats. Previously, `setupCreateZCFVat` closed over a copy of the ZCF bundle, and passed it to `vatAdminService~.createVat(zcfBundle)`. This hides the existence of the ZCF bundle from other packages that just want to use Zoe. However, to remove these large code bundles from userspace messages, we need the ZCF bundle to be installed "off to the side", by the code that prepares the swingset environment (usually as `config.bundles.zcf=`). This commit changes `fakeVatAdmin.js` to implement `createVatByName('zcf')`, and to move the copy of the zcfBundle out of `makeZoeKit()` and into `fakeVatAdmin.js` . In addition, it changes `createZCFVat.js` to provide a default bundle name of 'zcf'. Any unit test that uses `fakeVatAdmin.js` can continue to do so without changes, and the fake service will magically know how to create Zoe's ZCF vat without any additional configuration. Unit tests that use swingset, however, need to be updated to install the ZCF bundle into the kernel, with something like: ``` import zcfBundle from `@agoric/zoe/bundles/bundle-contractFacet.js`; ... config.bundles.zcf = { bundle: zcfBundle }; ``` Note: if we used `{ sourceSpec: '@agoric/zoe/contractFacet.js' }`, then we would not depend upon a recent `cd packages/zoe && yarn build`, and each kernel instance would bundle its own copy. This would be slower, but perhaps less prone to stale-bundle surprises, and might be a nicer export commitment). refs #4487
This updates unit tests in (zoe, governance, run-protocol), specifically those which use a swingset environment, to fetch the ZCF contract bundle from the Zoe package, and install it into the kernel config they build. This is necessary now that Zoe's `setupCreateZCFVat` no longer includes a copy. Any new tests which follow this pattern will need to do the same. refs #4487
Zoe needs a way to create a new ZCF vat to host each contract instance. Inside swingset, zoe uses `vatAdminService`, and this is used from unit tests (zoe and other packages that use zoe) when they are willing to take the time to run a full swingset environment. Unit tests that use zoe, but not swingset, use `tools/fakeVatAdmin.js` as a replacement, which implements `createVat(bundle)` but not `createVatByName(name)`. This is used to evaluate the ZCF bundle in a new Compartment (using `importBundle` and `evalContractBundle`). The primary export of `@agoric/zoe` is `makeZoeKit()`, which is called with `vatAdminService` and an optional `zcfBundleName`. This function runs `setupCreateZCFVat(vatAdminService)` to attenuate the vat-making power into one that can only create ZCF vats. Previously, `setupCreateZCFVat` closed over a copy of the ZCF bundle, and passed it to `vatAdminService~.createVat(zcfBundle)`. This hides the existence of the ZCF bundle from other packages that just want to use Zoe. However, to remove these large code bundles from userspace messages (#4372), we need the ZCF bundle to be installed "off to the side", by the code that prepares the swingset environment (usually as `config.bundles.zcf=`). This commit changes `fakeVatAdmin.js` to implement `createVatByName('zcf')`, and to move the copy of the zcfBundle out of `makeZoeKit()` and into `fakeVatAdmin.js` . In addition, it changes `createZCFVat.js` to provide a default bundle name of 'zcf'. Any unit test that uses `fakeVatAdmin.js` can continue to do so without changes, and the fake service will magically know how to create Zoe's ZCF vat without any additional configuration. Unit tests that use swingset, however, need to be updated to install the ZCF bundle into the kernel, with something like: ``` import zcfBundle from `@agoric/zoe/bundles/bundle-contractFacet.js`; ... config.bundles.zcf = { bundle: zcfBundle }; ``` Note: if we used `{ sourceSpec: '@agoric/zoe/contractFacet.js' }`, then we would not depend upon a recent `cd packages/zoe && yarn build`, and each kernel instance would bundle its own copy. This would be slower, but perhaps less prone to stale-bundle surprises, and might be a nicer export commitment. refs #4487
This updates unit tests in (zoe, governance, run-protocol), specifically those which use a swingset environment, to fetch the ZCF contract bundle from the Zoe package, and install it into the kernel config they build. This is necessary now that Zoe's `setupCreateZCFVat` no longer includes a copy. Any new tests which follow this pattern will need to do the same. refs #4487
@warner When I first started working on our ZH board, I marked this as being for the Mainnet 1 release because it was already in the Review/QA pipeline. The comment makes it seem like it is should be In Progress or maybe MN-1 Backlog or Product Backlog, not really In Review or In QA. Should this be moved back to the MN-1 backlog, or taken out of the Mainnet 1 release altogether. |
When a PR is marked as referencing a ticket, I think ZH is enthusiastically moving that ticket into the Review/QA pipeline even though the ticket won't be closed by the PR. I expect there will be another 3-ish PRs necessary before this ticket is closed. I'll move it back to the In Progress pipeline. |
Wow, that is pretty annoying! |
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. Previously, Zoe did E(vatAdminSvc).createVatByName('zcf'), and assumed that vatAdminSvc had access to a vat source bundle by that name. Now, whoever calls makeZoeKit() is obligated to provide Zoe with the right bundlecap, and Zoe does E(vatAdminSvc).createVat(zcfBundlecap). This makes things more explicit. This changes the signature of `makeZoeKit()` in a non-backwards-compatible way: the `zcfBundlecap` argument is now mandatory, and thus appears second, replacing the old optional fourth-position `zcfBundleName`. Swingset-based unit tests that use Zoe generally follow a pattern where `vat-zoe.js` holds the `makeZoeKit()` call, and `bootstrap.js` sends vat-zoe a `buildZoe(vatAdminSvc)` message. Those need to change: bootstrap should use `D(devices.bundle).getNamedBundleCap('zcf')` to get the `zcfBundlecap`, and send it through `buildZoe()`. vat-zoe should be changed to pass `zcfBundlecap` into `makeZoeKit()`. A similar pattern should be used in actual deployments. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. Downstream module owners should grep their source code for `makeZoeKit` and `fakeVatAdmin` to find the calls that need updating. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. Code that uses `makeZoeKit()` and provides the optional fourth argument needs to be updated to the new signature. This commit changes all such instances in the current codebase. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. Code that uses `makeZoeKit()` and provides the optional fourth argument needs to be updated to the new signature. This commit changes all such instances in the current codebase. refs #4487
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundlecap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundlecap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundlecap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. Code that uses `makeZoeKit()` and provides the optional fourth argument needs to be updated to the new signature. This commit changes all such instances in the current codebase. refs #4487
@warner Moving this back to In Progress since it was moved to Review/QA as part of @michaelfig closing #4373 |
Zoe needs access to the vatAdminSvc, to create a new ZCF vat to host each contract. `makeZoeKit()` provides an optional argument to control which bundle is used for this purpose. This 4th argument was a string, defaulting to 'zcf', and relied upon `E(vatAdminSvc).createVatByName` (which is going away). To move everything closer to bundlecaps, the `makeZoeKit()` optional argument now takes `{ name }` or `{ id }` or `{ bundleCap }`, instead of only a name. Zoe uses the new `E(vatAdminSvc).getNamedBundleCap()` to convert a name into a bundlecap, but in the future I expect our chain-side bootstrap() to call `makeZoeKit()` with a specific bundlecap. In the long run, I'm trying to make room for Zoe to accomodate multiple versions of ZCF, each indexed by its bundlecap. Non-swingset-based zoe-using unit tests generally use `fakeVatAdmin.js` to build a mock `vatAdminSvc`. This commit changes `fakeVatAdmin.js` to include an additional `zcfBundleCap` export, which is an empty marker object that serves as a stand-in for the bundlecap. The fake `vatAdminSvc` can accept this fake `zcfBundleCap`, and will evaluate the ZCF code appropriately. However the fake `vatAdminSvc` also knows how to convert the default `'zcf'` name into that bundlecap, so downstream code should not need to change. `fakeVatAdmin` no longer has a `createVatByName` method, as Zoe no longer needs it. Code that uses `makeZoeKit()` and provides the optional fourth argument needs to be updated to the new signature. This commit changes all such instances in the current codebase. refs #4487
Reviewing this now:
So I think spawner is the remaining blocker, but of course it would be a drastic API change. And the spawner service is used by code outside of agoric-sdk, so I'm not sure a simple CI run would catch the breakage. I'll push a draft PR to remove the feature from vat-admin, and see which of our existing tests break. |
What is the Problem Being Solved?
#4372 makes it possible to call
E(vatAdminService).createVat(bundlecap)
, and #4486 wants to removeE(vatAdminService).createVat(bundle)
. After #4372 lands, but before #4486 can land, we need to update both Zoe and the unit test approach used by zoe/ertp/governance/etc to use bundlecaps instead of bundles.Description of the Design
TBD
Security Considerations
Test Plan
The text was updated successfully, but these errors were encountered: