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

A3p test zcf bundle cap #8911

Merged
merged 4 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ module.exports = {
'./packages/*/tsconfig.json',
'./packages/*/tsconfig.json',
'./packages/wallet/*/tsconfig.json',
'./a3p-integration/proposals/*/tsconfig.json',
'./tsconfig.json',
],
tsconfigRootDir: __dirname,
Expand Down
11 changes: 6 additions & 5 deletions a3p-integration/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Dockerfile
docker-bake.*
upgrade-test-scripts
*submission/

# Yarn (https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored)
.pnp.*
Expand All @@ -14,8 +15,8 @@ upgrade-test-scripts
# same for each proposal, an independent project
proposals/*/.pnp.*
proposals/*/.yarn/*
proposals/*/!.yarn/patches
proposals/*/!.yarn/plugins
proposals/*/!.yarn/releases
proposals/*/!.yarn/sdks
proposals/*/!.yarn/versions
!proposals/*/.yarn/patches
!proposals/*/.yarn/plugins
!proposals/*/.yarn/releases
!proposals/*/.yarn/sdks
!proposals/*/.yarn/versions
1 change: 1 addition & 0 deletions a3p-integration/proposals/a:upgrade-next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"ava": {
"concurrency": 1,
"serial": true,
"timeout": "1m",
"files": [
"!submission"
]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import test from 'ava';

import {
evalBundles,
getIncarnation,
getVatDetails,
} from '@agoric/synthetic-chain';

const SUBMISSION_DIR = 'probe-submission';

test('upgrade Zoe to verify ZcfBundleCap endures', async t => {
await null;
t.assert((await getIncarnation('zoe')) === 1, 'zoe incarnation must be one');

// Before the test, the Wallet Factory should be using the legacy ZCF
const detailsBefore = await getVatDetails('walletFactory');
t.true(detailsBefore.incarnation >= 2, 'wf incarnation must be >= 2');

await evalBundles(SUBMISSION_DIR);

const detailsAfter = await getVatDetails('walletFactory');
t.is(
detailsAfter.incarnation,
detailsBefore.incarnation + 2,
'wf incarnation must increase by 2',
);

// The test restarts the WalletFactory, so it'll use the recently assigned
// ZCF bundle. It then restarts Zoe, so it'll revert to whichever ZCF bundle
// made it to persistent store. We then restart the Wallet Factory and see if
// it's gone back to the ZCF that Zoe initially knew about. If we could get the
// ZCF bundleID here from the probe, we'd explicitly check for that. Instead,
// we have to be content that it did indeed use the newly assigned bundle in
// manual tests.
t.not(detailsAfter.bundleID, detailsBefore.bundleID);
});
4 changes: 3 additions & 1 deletion a3p-integration/proposals/a:upgrade-next/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@
# Place here any test that should be executed using the executed proposal.
# The effects of this step are not persisted in further proposal layers.

yarn ava
yarn ava post.test.js
GLOBIGNORE=post.test.js
yarn ava *.test.js
22 changes: 22 additions & 0 deletions packages/builders/scripts/vats/probe-zcf-bundle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { makeHelpers } from '@agoric/deploy-script-support';

/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').ProposalBuilder} */
export const defaultProposalBuilder = async ({ publishRef, install }) =>
harden({
sourceSpec: '@agoric/vats/src/proposals/probeZcfBundle.js',
getManifestCall: [
'getManifestForProbeZcfBundleCap',
{
zoeRef: publishRef(install('@agoric/vats/src/vat-zoe.js')),
zcfRef: publishRef(install('@agoric/zoe/src/contractFacet/vatRoot.js')),
Copy link
Member

Choose a reason for hiding this comment

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

Why can't zcfRef be the only ref, with a dummy implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WalletRef: The fix in startInstance below wasn't present the previous time that the walletFactory was upgraded, so Zoe didn't remember the bundleId as it should have.

ZoeRef: adminNode.upgrade() requires a bundleCap. I'd be happy to re-use the already known one. This seems to be the most straightforward way to get it.

walletRef: publishRef(
install('@agoric/smart-wallet/src/walletFactory.js'),
),
},
],
});

export default async (homeP, endowments) => {
const { writeCoreProposal } = await makeHelpers(homeP, endowments);
await writeCoreProposal('probeZcfBundle', defaultProposalBuilder);
};
72 changes: 72 additions & 0 deletions packages/vats/src/proposals/probeZcfBundle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { E } from '@endo/far';
import { makeStorageNodeChild } from '@agoric/internal/src/lib-chainStorage.js';

// verify that Zoe remembers the zcfBundleCap across upgrades
export const probeZcfBundleCap = async (
{
consume: {
vatAdminSvc,
walletFactoryStartResult,
provisionPoolStartResult,
chainStorage,
walletBridgeManager: walletBridgeManagerP,
vatStore,
},
},
options,
) => {
const { zoeRef, zcfRef, walletRef } = options.options;
Copy link
Member

Choose a reason for hiding this comment

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

I'm still confused how these options can get passed in. The way things are structured now, the submission is prepared before a3p proposals are run. However I think this proposal does not actually need to generate any bundle, so the submission could be manually written, with the template mechanism used to fill the probed bundle refs?

Copy link
Contributor Author

@Chris-Hibbert Chris-Hibbert Feb 20, 2024

Choose a reason for hiding this comment

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

I think you're right. My current belief is that I want to cite the Zoe and ZCF bundles that will have been included in upgrade-15. I want to do a null upgrade of Zoe to trigger reversion to a stored ZCF bundle. If I can then do a null upgrade to a contract vat (e.g. WalletFactory) then I can compare the ZCF bundle used to the ZCF bundleID just used in upgrade-15.
So I'm looking for how I can find out those bundleIDs to use with the new (very clean!) template mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turadg added a check so if the bundle has already been installed, it doesn't do it again. Including these bundle IDs in the manifest is necessary so I can have them to request an upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how that's relevant. I actually don't understand why this test needs to install any new bundle ?

Copy link
Member

Choose a reason for hiding this comment

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

I mean I understand how one option is to do a full generation of core eval scripts and bundles, but I thought from the description we just wanted to "restart" the contracts. I suppose we may want to upgrade ZCF since the test checks it uses the new version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

restarting the contracts would be fine.

When I used restartContract() without a bundleID, in place of E(walletAdminFacet).upgradeContract(walletRef.bundleID, privateArgs);, I got an error from the kernel that implied that it was trying to upgrade to an earlier version of the contract. checkAndUpdateFacetiousness() complained that I was trying to remove a facet, which would only make sense if walletFactory was trying to upgrade from a relatively modern version to the version on chain. I'll file a bug to investigate restartContract().

Since the only productive way to upgrade the wallet factory was using upgradeContract(), I needed a bundleId.

With ZCF, I need to pass the bundleID to E(zoeConfigFacet).updateZcfBundleId(zcfRef.bundleID); to tell Zoe to use a particular version of ZCF when upgrading the contracts. The point of this test is to validate that #8807 correctly fixes Zoe to store the ZCFBundleID. Before that PR, it clearly was not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the fix in commit 5a30b2d877, I was able to revert to restartContract for the second restart, but the first still needs to be an upgrade (in the test) so that we can tell whether it is using the original bundleId or the newly provided one.


const { adminNode, root: zoeRoot } = await E(vatStore).get('zoe');
const zoeConfigFacet = await E(zoeRoot).getZoeConfigFacet();

// STEP 1: restart WF; it'll use the newest ZCF known to Zoe ////////////////

const WALLET_STORAGE_PATH_SEGMENT = 'wallet';
const [walletBridgeManager, walletStorageNode, ppFacets] = await Promise.all([
walletBridgeManagerP,
makeStorageNodeChild(chainStorage, WALLET_STORAGE_PATH_SEGMENT),
provisionPoolStartResult,
]);
const walletReviver = await E(ppFacets.creatorFacet).getWalletReviver();
const privateArgs = {
storageNode: walletStorageNode,
walletBridgeManager,
walletReviver,
};

const { adminFacet: walletAdminFacet } = await walletFactoryStartResult;
await E(walletAdminFacet).upgradeContract(walletRef.bundleID, privateArgs);

// STEP 2: Set the ZCF bundle ////////////////////////
await E(zoeConfigFacet).updateZcfBundleId(zcfRef.bundleID);

// STEP 3: Upgrade Zoe again ////////////////////////
// ////// See if Zoe forgets ZcfBundleCap //////////

const zoeBundleCap = await E(vatAdminSvc).getBundleCap(zoeRef.bundleID);
await E(adminNode).upgrade(zoeBundleCap, {});

// STEP 4: restart WF ////////////////////////
await E(walletAdminFacet).restartContract(privateArgs);

// ////// See which zcf bundle was used //////////
};
harden(probeZcfBundleCap);

export const getManifestForProbeZcfBundleCap = (_powers, options) => ({
manifest: {
[probeZcfBundleCap.name]: {
consume: {
vatAdminSvc: true,
vatStore: true,
walletBridgeManager: true,
walletFactoryStartResult: true,
provisionPoolStartResult: true,
chainStorage: true,
},
},
},
options,
});
harden(getManifestForProbeZcfBundleCap);
8 changes: 6 additions & 2 deletions packages/zoe/src/contractFacet/zcfZygote.js
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,12 @@ export const makeZCFZygote = async (

await null;
if (!zcfBaggage.has('repairedContractCompletionWatcher')) {
await E(zoeInstanceAdmin).repairContractCompletionWatcher();
console.log(`Repaired contract completion watcher`);
// We don't wait because it's a cross-vat call (to Zoe) that can't be
// completed during this vat's start-up
E(zoeInstanceAdmin)
.repairContractCompletionWatcher()
.catch(() => {});
Comment on lines -472 to +476
Copy link
Member

Choose a reason for hiding this comment

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

This is not strictly a testing change anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct.

I'd guess you're hinting that I should change something, presumably the commit logs, but I don't understand their impact. Is it sufficient that the latest commit has a feat tag, or do I need to change the PR's title or something else? Do I need to eventually need to check it in as a merge rather than a squash, or is there something else to be careful of?

Copy link
Member

Choose a reason for hiding this comment

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

The main problem is that this change is in a commit titled test:. This change should be in a separate commit, before the 'test' commit, describing this change in behavior.

The PR title says "test" but it's normal for other changes to be made to support a testing requirement. The problem is when the changelog leaves those changes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit makes two changes. One is the fix to startInstance.js. The other is a change to the test probe to take advantage of the fix and demonstrate that it fixes the problem. That second change has to update the test, so it can't precede it. I think it's important that the fix and the revelation of its effectiveness stay together.

If I amend the commit to call it a fix, and remember to not squish, would that be sufficient to update the changelog? If not how would you recommend I address this?

Copy link
Member

@turadg turadg Feb 27, 2024

Choose a reason for hiding this comment

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

I think it's important that the fix and the revelation of its effectiveness stay together.

Up to you.

If I amend the commit to call it a fix, and remember to not squish, would that be sufficient to update the changelog?

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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


zcfBaggage.init('repairedContractCompletionWatcher', true);
}

Expand Down
1 change: 1 addition & 0 deletions packages/zoe/src/zoeService/startInstance.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ export const makeStartInstance = (
contractBundleCap: newContractBundleCap,
privateArgs: newPrivateArgs,
};
state.contractBundleCap = newContractBundleCap;
return E.when(getFreshZcfBundleCap(), bCap =>
E(state.adminNode).upgrade(bCap, { vatParameters }),
);
Expand Down
10 changes: 7 additions & 3 deletions scripts/generate-a3p-submission.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ cd "$sdkroot"
buildSubmission() {
proposalName=$1
a3pProposal=$2
output=${3:-$proposalName}
submissionName=${4:-submission}

yarn agoric run "packages/builders/scripts/vats/$proposalName.js"

submissionDir="a3p-integration/proposals/$a3pProposal/submission"

submissionDir="a3p-integration/proposals/$a3pProposal/$submissionName"
mkdir -p "$submissionDir"
cp $(grep -oh '/.*b1-.*.json' "$proposalName"*) "$submissionDir"
mv "$proposalName"* "$submissionDir"
cp $(grep -oh '/.*b1-.*.json' "$output"*) "$submissionDir"
mv "$output"* "$submissionDir"
}

buildSubmission probe-zcf-bundle "a:upgrade-next" probeZcfBundle probe-submission
buildSubmission test-localchain "b:localchain"
Loading