Skip to content
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

test(zoe): switch to E(zoe).installBundleID() #4684

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Conversation

warner
Copy link
Member

@warner warner commented Feb 27, 2022

The new preferred way to install a contract is by its bundleID, using
E(zoe).installBundleID(id) instead of E(zoe).install(bundle). The bundle
itself must first be installed/registered with the VatAdmin service. In a
real swingset, this happens out-of-band, via
controller.validateAndInstallBundle. In a non-swingset unit test, the
fakeVatAdmin has a new vatAdminState.installBundle(id, bundle).

This changes most Zoe unit tests to perform vatAdminState.installBundle and
then use Zoe's installBundleID method. This requires some mechanical
changes to the way many tests get access to their fakeVatAdmin. I took the
opportunity to switch all tests to importing makeFakeVatAdmin and building
their own local copy, rather than relying upon the shared singleton created
and exported by fakeVatAdmin.js itself.

This leaves one test in unitTests/ using the old E(zoe).install(bundle),
to ensure it still works until we decide to remove it entirely. All tests in
swingsetTests/ still use install(bundle), those will be changed later.

It also updates all swingset-based zoe tests to install contracts by BundleID
instead of a full bundle. In all cases, we add the bundles by name to
config.bundles, from which you can obtain the BundleID with:

const bcap = await E(vatAdminService).getNamedBundleCap(name);
const id = D(bcap).getBundleID();

In a real chain, the bundles would be installed into the kernel at runtime,
and zoe would be given a BundleID by the (off-chain) user doing the
install (or their deploy script).

refs #4565

@warner warner added the Zoe package: Zoe label Feb 27, 2022
@warner warner requested a review from erights February 27, 2022 10:11
@warner warner self-assigned this Feb 27, 2022
@warner warner force-pushed the 4565-update-zoe-tests branch 5 times, most recently from 7ccf2a6 to 4b5242f Compare March 3, 2022 23:26
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +92 to +101
const installations = {};
const installedPs = vatParameters.contractNames.map(name =>
E(vatAdminSvc)
.getNamedBundleCap(name)
.then(bcap => E(zoe).installBundleID(D(bcap).getBundleID()))
.then(installation => {
installations[name] = installation;
}),
);
await Promise.all(installedPs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG was the old code doing a ton of unnecessary awaits. Good to see these reduced to one? Just curious: did you notice a speedup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh I didn't look. But also a handful of intra-kernel roundtrips would be completely swamped by the overhead of creating all the contract bundles, which takes on the order of 50 seconds, so I think I'd have to build a special test to be able to measure a difference.

@warner warner force-pushed the 4565-update-zoe-tests branch from 4b5242f to f97220b Compare March 4, 2022 03:56
@warner warner added the automerge:rebase Automatically rebase updates, then merge label Mar 4, 2022
warner added 2 commits March 4, 2022 04:23
The new preferred way to install a contract is by its bundleID, using
`E(zoe).installBundleID(id)` instead of `E(zoe).install(bundle)`. The bundle
itself must first be installed/registered with the VatAdmin service. In a
real swingset, this happens out-of-band, via
`controller.validateAndInstallBundle`. In a non-swingset unit test, the
`fakeVatAdmin` has a new `vatAdminState.installBundle(id, bundle)`.

This changes most Zoe unit tests to perform `vatAdminState.installBundle` and
then use Zoe's `installBundleID` method. This requires some mechanical
changes to the way many tests get access to their fakeVatAdmin. I took the
opportunity to switch all tests to importing `makeFakeVatAdmin` and building
their own local copy, rather than relying upon the shared singleton created
and exported by `fakeVatAdmin.js` itself.

This leaves one test in `unitTests/` using the old `E(zoe).install(bundle)`,
to ensure it still works until we decide to remove it entirely. All tests in
`swingsetTests/` still use `install(bundle)`, those will be changed later.

refs #4565
This updates all swingset-based zoe tests to install contracts by BundleID
instead of a full bundle. In all cases, we add the bundles by name to
`config.bundles`, from which you can obtain the BundleID with:

```js
const bcap = await E(vatAdminService).getNamedBundleCap(name);
const id = D(bcap).getBundleID();
```

In a real chain, the bundles would be installed into the kernel at runtime,
and zoe would be given a BundleID by the (off-chain) user doing the
install (or their deploy script).

refs #4565
@turadg turadg force-pushed the 4565-update-zoe-tests branch from f97220b to 96e5512 Compare March 4, 2022 04:23
@mergify mergify bot merged commit d989e62 into master Mar 4, 2022
@mergify mergify bot deleted the 4565-update-zoe-tests branch March 4, 2022 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants