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): Add a test for contracts surviving Zoe vat upgrade #6715

Merged
merged 13 commits into from
Jan 13, 2023

Conversation

gibson042
Copy link
Member

@gibson042 gibson042 commented Dec 22, 2022

closes: #6713

Description

Creates a test that instantiates covered-call contracts, null-upgrades Zoe, and then finishes the contracts.

Security Considerations

n/a

Documentation Considerations

n/a

}

// Null-upgrade Zoe.
const incarnationNumber = await run('upgradeVat', [zoeVatConfig]);
Copy link
Member Author

Choose a reason for hiding this comment

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

@Chris-Hibbert I think I will need some help after all.

Error: vat-upgrade failure

error during vat dispatch() of startVat,[object Object] (Error#1)
Error#1: defineDurableKind not called for tags: [ZDEFAULT brand,ZDEFAULT payment,ZDEFAULT Purse,ZDEFAULT issuer,ZDEFAULT mint,FeeMint,Zoe Invitation brand,Zoe Invitation payment,Zoe Invitation Purse,Zoe Invitation issuer,Zoe Invitation mint,InstanceAdmin,BundleIDInstallation,BundleInstallation,InstallationStorage,ZoeMint,ZoeStorageManager,InstanceHandle,zoe Seat publisher,ZoeSeatKit,SeatHandle,instanceAdmin,zoeInstanceAdmin,emptyCreatorFacet,emptyPublicFacet,adminFacet,ZoeService,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager]
Error: defineDurableKind not called for tags: [ZDEFAULT brand,ZDEFAULT payment,ZDEFAULT Purse,ZDEFAULT issuer,ZDEFAULT mint,FeeMint,Zoe Invitation brand,Zoe Invitation payment,Zoe Invitation Purse,Zoe Invitation issuer,Zoe Invitation mint,InstanceAdmin,BundleIDInstallation,BundleInstallation,InstallationStorage,ZoeMint,ZoeStorageManager,InstanceHandle,zoe Seat publisher,ZoeSeatKit,SeatHandle,instanceAdmin,zoeInstanceAdmin,emptyCreatorFacet,emptyPublicFacet,adminFacet,ZoeService,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager,InstanceStorageManager]
 at apply ()
 at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:54)
 at insistAllDurableKindsReconnected (/bundled-source/.../packages/SwingSet/src/liveslots/virtualObjectManager.js:994)
 at startVat (/bundled-source/.../packages/SwingSet/src/liveslots/liveslots.js:1437)
 at ()
 at ()

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

Thanks, @gibson042, I'll take it from here for now. This is a great framework for verifying upgradeability, and I'll use it to figure out what I didn't set up correctly for upgrade.

import { vivifyErtpService } from '@agoric/ertp/test/swingsetTests/ertpService/vat-ertp-service.js';

export const buildRootObject = async (vatPowers, _vatParams, baggage) => {
const exportableAmountMath = Far('AmountMath', { ...AmountMath });
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need an exportableAmountMath? AmountMath was designed to be usable locally in every vat and produce copy objects that can be sent around seamlessly.

Comment on lines +52 to +55
const messageVat = (name, methodName, args) =>
run('messageVat', [{ name, methodName, args }]);
const messageObject = (presence, methodName, args) =>
run('messageVatObject', [{ presence, methodName, args }]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these a replacement for E() calls? What role do they play?

@gibson042
Copy link
Member Author

Are these a replacement for E() calls? What role do they play?

Why do we need an exportableAmountMath? AmountMath was designed to be usable locally in every vat and produce copy objects that can be sent around seamlessly.

The answer to both questions is that dependent code exists outside of the vats under test and is thus limited to pass-by-copy data as supported by encoding and decoding in packages/SwingSet/test/bootstrap-relay.js (which does use E directly).

@Chris-Hibbert Chris-Hibbert changed the base branch from master to 5997-complete-ZoeDurability December 28, 2022 23:16
@Chris-Hibbert
Copy link
Contributor

@gibson042, I pushed one teensy correction. Otherwise, the test worked great! #6726 contains the changes to make Zoe pass the test. This test is now stacked on top of that.

@Chris-Hibbert Chris-Hibbert force-pushed the 5997-complete-ZoeDurability branch 2 times, most recently from 78cad8e to 45e3ed6 Compare December 29, 2022 19:10
@erights
Copy link
Member

erights commented Dec 30, 2022

Are these two CI dapp failures due to the API change not yet being propagated to other repos? If so, why didn't they already fail in #6726 ?

@Chris-Hibbert
Copy link
Contributor

I hadn't bothered to add the markup below to this one, since it's a known fix. Once the base PR is merged, this will get it.

#dapp-otc-branch: 5997-integration
#documentation-branch: 5997-integration

I'll add it above, and if the tests run from scratch, they'll get it.

@gibson042
Copy link
Member Author

The incarnationNumber change should not be necessary given the current state of upgradeVat in master, but I'm happy to let that shake out as appropriate. I'll review #6726 today.

Chris-Hibbert and others added 6 commits January 4, 2023 17:40
Refactored instanceRecord storage to serve Zoe and ZCF better

initialize issuerStorage only once

revive feeMint, invitationIssuer on restart

save bundleCap vatAdminSvc in zoe for re-use

define makeInvitation inline in instanceStorageManager, so pass in InvitationIssuer

vat-zoe has to define durable kinds on start & restart, but vatAdminService should be
re-used on restart. This necessitated a change in the return value of makeZoeKit

instanceStorageManager holds its state directly rather than lexically.
* fix: suggested simplification of 6726

* fix: more
No need for feeMintAccessRetriever.

setVatAdminService is a far function

also cleaned up some asserts
closes: #6748

This method shouid only ever have been accessible on ZCF.
@Chris-Hibbert
Copy link
Contributor

I remove the cross-repo imports in the top comment. I'll rebase this on the latest #6726 to see that the tests still pass.

Base automatically changed from 5997-complete-ZoeDurability to master January 13, 2023 18:15
@Chris-Hibbert
Copy link
Contributor

The code this is a test for has been merged. @gibson042, please merge this.

@Chris-Hibbert Chris-Hibbert added Zoe package: Zoe automerge:squash Automatically squash merge labels Jan 13, 2023
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 876c738 into master Jan 13, 2023
@mergify mergify bot deleted the gibson-6713-test-zoe-upgrade branch January 13, 2023 20:05
@dckc dckc mentioned this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing tests for contracts surviving Zoe vat upgrade
3 participants