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

7918 upgrade15 update zoe zcf #8793

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #7918
refs: #7918
refs: #6678
refs: #7946

Description

Add to the release branch the code to allow updating the version of ZCF to be used on contract upgrades. (#7918)

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

Has been tested in swingSetTests, and in docker. Once the release branch is ready, we'll add a3p-integration tests, too.

Upgrade Considerations

Allows contract vats to be upgraded.

@Chris-Hibbert Chris-Hibbert added Zoe package: Zoe Zoe Contract Contracts within Zoe contract-upgrade labels Jan 23, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Jan 23, 2024
Base automatically changed from 8453-release-Zoe to tmp-upgrade-15 January 23, 2024 19:25
@Chris-Hibbert Chris-Hibbert changed the base branch from tmp-upgrade-15 to 8453-release2-Zoe January 23, 2024 22:40
@@ -50,6 +50,12 @@ export const makeStartInstance = (
seatHandleToZoeSeatAdmin,
);

const getFreshZcfBundleCap = async () => {
const settledBundleCap = await getZcfBundleCapP();
settledBundleCap !== undefined || Fail`the ZCF bundle cap was broken`;
Copy link
Member

Choose a reason for hiding this comment

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

When might settledBundleCap be undefined? What I'm wondering is if "broken" is the right diagnosis to report.

Copy link
Member

Choose a reason for hiding this comment

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

When a fresh ZcfBundleCap is gotten, what if anything updates state.zcfBundleCap? Does getZcfBundleCapP do that? Where is getZcfBundleCapP defined anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zcfBundleCap is defined in zoe.js. It starts out undefined, and is set when setVatAdminSvc() is called. Zoe ensures that it's set early, but I think the type system can't tell, and is happier when we check and throw. "unset" might be more apt, but if it happens, I want whoever sees it to realize it's a big deal.

When a fresh ZcfBundleCap is gotten, what if anything updates state.zcfBundleCap?

Ooh! good catch! There's no state.zcfBundleCap, it's held in baggage. (We can't change what's in state in a new incarnation.) A fresh one would be set in a call to updateZcfBundleId, which does not update zoeBaggage. Created an issue: #8805.

Where is getZcfBundleCapP defined anyway?

In the call to makeStartInstance it's set to () => zcfBundleCap. (i.e. just dynamically access the local version held by Zoe.)

zcfBundleCap = bundleCap;
},
e => {
console.error(`'🚨 unable to update ZCF Bundle: `, e);
Copy link
Member

Choose a reason for hiding this comment

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

I see

image

What's the weird character within the literal string after the open single quote? Why is there no close single quote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's (unnecessarily) a backquoted string. The singlequote within (between the backquote and flashing light) is unintentional. Since it's withing a backquoted string, it doesn't need a close quote.

I don't want to fix it only here. If you think it's worth the trouble, I will fix it first on master and then here.

@@ -117,7 +122,7 @@ export const makeZoeStorageManager = (
'zoeMintBaggageSet',
);
for (const issuerBaggage of zoeMintBaggageSet.values()) {
zoeMintBaggageSet(issuerBaggage);
prepareIssuerKit(issuerBaggage);
Copy link
Member

@erights erights Jan 23, 2024

Choose a reason for hiding this comment

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

Surprised to see zoeMintBaggageSet become prepareIssuerKit. Digging, I see that zoeMintBaggageSet just calls provideDurableSetStore. Neither sounds like it is doing a job similar to prepareIssuerKit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zoeMintBaggageSet(issuerBaggage) was always a typo here. zoeMintBaggageSet is a collection. I don't know why lint didn't complain about it, but it's been fixed on master for a while.

As @warner noticed earlier today, it only didn't fail because zoeMintBaggageSet was empty till recently.

@erights erights self-requested a review January 23, 2024 23:52
Base automatically changed from 8453-release2-Zoe to dev-upgrade-14 February 8, 2024 01:04
@mhofman mhofman added the force:integration Force integration tests to run on PR label Feb 8, 2024
@mhofman mhofman merged commit 408fffa into dev-upgrade-14 Feb 8, 2024
71 checks passed
@mhofman mhofman deleted the 7918-upgrade15-updateZoeZcf branch February 8, 2024 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contract-upgrade force:integration Force integration tests to run on PR Zoe Contract Contracts within Zoe Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants