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

A3p test zcf bundle cap #8911

merged 4 commits into from
Feb 28, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Feb 14, 2024

refs: #8868

Description

An a3p-integration test for #8807.

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing/Upgrade Considerations

This is a test that a bug fix had the appropriate effect.

The test takes advantage of the fact that the vatInfo held in swingstore (and returned by getVatDetails() in synthetic-chain) gives the bundleId of the ZCF in a contract vat rather than the bundleID of the contract itself.

@Chris-Hibbert Chris-Hibbert added the Zoe package: Zoe label Feb 14, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Feb 14, 2024
@Chris-Hibbert Chris-Hibbert marked this pull request as draft February 14, 2024 01:23
packages/vats/package.json Outdated Show resolved Hide resolved
{
"agoricProposal": {
"type": "/agoric.swingset.CoreEvalProposal",
"source": "submission"
Copy link
Member

Choose a reason for hiding this comment

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

I think the source needs to be subdir, but I'm not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked just fine this way. Was the "need" you refer a style rule, or is the fact that it works sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

packages/vats/src/proposals/probeZcfBundle.js Outdated Show resolved Hide resolved
@mhofman mhofman added the force:integration Force integration tests to run on PR label Feb 14, 2024
Base automatically changed from 8445-a3p-tests-mastr to master February 17, 2024 23:04
options,
) => {
console.log('probeZcfBundle start');
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.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

I'm still pretty confused by what this test tries to do. We should sync up

Comment on lines -472 to +476
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(() => {});
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.

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 //////////////////
Copy link
Member

Choose a reason for hiding this comment

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

newest is ambiguous here, as it's not the zcf bundle that is being installed just below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, just the most recent known to Zoe. updated.

options,
) => {
console.log('probeZcfBundle start');
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 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

await evalBundles(SUBMISSION_DIR);

const detailsAfter = await getVatDetails('walletFactory');
t.true(detailsAfter.incarnation >= 3, 'wf incarnation after must be >= 3');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
t.true(detailsAfter.incarnation >= 3, 'wf incarnation after must be >= 3');
t.true(detailsAfter.incarnation == detailsBefore.incarnation + 1, 'wf incarnation must increase');

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

'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.

@Chris-Hibbert Chris-Hibbert changed the title WIP: A3p test zcf bundle cap A3p test zcf bundle cap Feb 23, 2024
@Chris-Hibbert Chris-Hibbert marked this pull request as ready for review February 23, 2024 23:18
@Chris-Hibbert Chris-Hibbert mentioned this pull request Feb 24, 2024
14 tasks
a3p-integration/proposals/a:upgrade-next/post.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/b:restart-vats/package.json Outdated Show resolved Hide resolved
a3p-integration/proposals/tsconfig.json Outdated Show resolved Hide resolved
packages/vats/src/proposals/probeZcfBundle.js Show resolved Hide resolved
packages/vats/src/proposals/probeZcfBundle.js Show resolved Hide resolved
Comment on lines -472 to +476
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(() => {});
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.

packages/zoe/src/zoeService/startInstance.js Show resolved Hide resolved
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I left some requests. I'll review again when it's rebased on master or #9004

@Chris-Hibbert
Copy link
Contributor Author

I left some requests. I'll review again when it's rebased on master or #9004

rebased onto #9004

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

The commits can be cleaned up a bit but not a blocker

Copy link
Member

Choose a reason for hiding this comment

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

this should be squashed into the previous.

@@ -0,0 +1,37 @@
/* eslint-disable @jessie.js/safe-await-separator */
Copy link
Member

Choose a reason for hiding this comment

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

I think this isn't doing anything. also the best remedy is await null instead of suppression

@Chris-Hibbert Chris-Hibbert force-pushed the a3p-test-zcfBundleCap branch 2 times, most recently from 75a6159 to bcc0aa6 Compare February 28, 2024 18:19
@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Feb 28, 2024
@mergify mergify bot merged commit 111247f into master Feb 28, 2024
66 checks passed
@mergify mergify bot deleted the a3p-test-zcfBundleCap branch February 28, 2024 19:15
@Chris-Hibbert
Copy link
Contributor Author

@gibson042 commit 72c7574 contains an important fix to ZCF. It should be cherry-picked into upgrade15, and ZCF updated with a call to E(zoeConfigFacet).updateZcfBundleId(zcfRef.bundleID); so that upgrading contracts will get this new version of ZCF.

@mhofman mhofman mentioned this pull request Apr 17, 2024
mergify bot added a commit that referenced this pull request Apr 18, 2024
refs: #9246

## Description

Add a proposal to upgrade the ZCF bundle only, and run it as a core
proposal during chain software upgrade. This is necessary for #9246 but
not sufficient as the release also needs to include the fix from #8911.

### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

None

### Testing Considerations

Will be tested end to end (make sure smart wallet can be upgraded) in
#9206

### Upgrade Considerations

@gibson042 must do a partial cherry-pick of
72c7574#diff-8dcfebb96abffde204086c5d660142e0c4c74a7bac0800f7c18447e0baad1979
(the `zcfZygote.js` change) in the release-15 branch.

Will be part of that chain software upgrade.
Chris-Hibbert added a commit that referenced this pull request Apr 18, 2024
This fix is cherry-picked from #8911 for upgrade 15
Chris-Hibbert added a commit that referenced this pull request Apr 18, 2024
This fix is cherry-picked from #8911 for upgrade 15
Chris-Hibbert added a commit that referenced this pull request Apr 18, 2024
This fix is cherry-picked from #8911 for upgrade 15
Chris-Hibbert added a commit that referenced this pull request Apr 18, 2024
This fix is cherry-picked from #8911 for upgrade 15
gibson042 added a commit that referenced this pull request Apr 20, 2024
Created by following the [pending
MAINTAINERS.md](https://github.com/Agoric/agoric-sdk/pull/9235/files?short_path=39da3bd#diff-39da3bd6270d44ea37b6ed50bd42eeb9d93ac5e1639645871a69cbe08cbe29de),
branching from dev-upgrade-15.

NOTE:
[scripts/have-news](https://github.com/Agoric/agoric-sdk/blob/master/scripts/have-news)
has an undocumented dependency upon src-prefix="a" that was disrupted by
my local
[`diff.mnemonicPrefix=true`](https://git-scm.com/docs/diff-config/2.9.5#Documentation/diff-config.txt-diffmnemonicPrefix)
configuration, resulting in empty output until I tracked down the issue.
I've opened #9269 to track that.

## Packages that have NEWS.md updates

```diff
--- c/golang/cosmos/CHANGELOG.md
+++ w/golang/cosmos/CHANGELOG.md
@@ -3,6 +3,21 @@
 All notable changes to this project will be documented in this file.
 See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.
 
+## [0.35.0-u15.0](https://github.com/Agoric/agoric-sdk/compare/@agoric/cosmos@0.35.0-u14.1...@agoric/cosmos@0.35.0-u15.0) (2024-04-20)
+
+
+### Features
+
+* upgrade zcf: install bundle and call updateZcfBundleId() ([74662d7](74662d7)), closes [#9250](#9250)
+* **cosmos:** Next upgrade is `agoric-upgrade-15` ([6a84bb1](6a84bb1))
+
+
+### Bug Fixes
+
+* various fixes and features for u15 ([21e6f7c](21e6f7c))
+
+
+
 ## [0.35.0-u14.1](https://github.com/gibson042/agoric-sdk/compare/@agoric/cosmos@0.35.0-u14.0...@agoric/cosmos@0.35.0-u14.1) (2024-03-12)
 
 
--- c/packages/smart-wallet/CHANGELOG.md
+++ w/packages/smart-wallet/CHANGELOG.md
@@ -3,6 +3,20 @@
 All notable changes to this project will be documented in this file.
 See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.
 
+### [0.5.4-u15.0](https://github.com/Agoric/agoric/compare/@agoric/smart-wallet@0.5.4-u14.1...@agoric/smart-wallet@0.5.4-u15.0) (2024-04-20)
+
+
+### Features
+
+* **smart-wallet:** tryExitOffer reclaims withdrawn payments ([6f01f63](6f01f63))
+
+
+### Bug Fixes
+
+* in SmartWallet, if invitation is invalid, don't process offer ([29078f7](29078f7))
+
+
+
 ### [0.5.4-u14.1](https://github.com/Agoric/agoric/compare/@agoric/smart-wallet@0.5.4-u14.0...@agoric/smart-wallet@0.5.4-u14.1) (2024-03-12)
 
 **Note:** Version bump only for package @agoric/smart-wallet
--- c/packages/vats/CHANGELOG.md
+++ w/packages/vats/CHANGELOG.md
@@ -3,6 +3,15 @@
 All notable changes to this project will be documented in this file.
 See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.
 
+### [0.15.2-u15.0](https://github.com/Agoric/agoric-sdk/compare/@agoric/vats@0.15.2-u14.1...@agoric/vats@0.15.2-u15.0) (2024-04-20)
+
+
+### Features
+
+* upgrade zcf: install bundle and call updateZcfBundleId() ([74662d7](74662d7)), closes [#9250](#9250)
+
+
+
 ### [0.15.2-u14.1](https://github.com/Agoric/agoric-sdk/compare/@agoric/vats@0.15.2-u14.0...@agoric/vats@0.15.2-u14.1) (2024-03-12)
 
 **Note:** Version bump only for package @agoric/vats
--- c/packages/zoe/CHANGELOG.md
+++ w/packages/zoe/CHANGELOG.md
@@ -3,6 +3,15 @@
 All notable changes to this project will be documented in this file.
 See [Conventional Commits](https://conventionalcommits.org) for commit guidelines.
 
+### [0.26.3-u15.0](https://github.com/Agoric/agoric-sdk/compare/@agoric/zoe@0.26.3-u14.0...@agoric/zoe@0.26.3-u15.0) (2024-04-20)
+
+
+### Bug Fixes
+
+* fix ZCF to not await in first crank ([8e289fd](8e289fd)), closes [#8911](#8911)
+
+
+
 ### [0.26.3-u14.0](https://github.com/Agoric/agoric-sdk/compare/@agoric/zoe@0.26.3-u13.0...@agoric/zoe@0.26.3-u14.0) (2024-02-27)
 
 
```
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 force:integration Force integration tests to run on PR Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants