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

6678 upgrade zoe & zcf #7946

Merged
merged 5 commits into from
Jul 27, 2023
Merged

6678 upgrade zoe & zcf #7946

merged 5 commits into from
Jul 27, 2023

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Jun 16, 2023

closes: #7918
refs: #6678

Description

Changes to Zoe to enable upgrade of ZCF. Zoe needs to have a function in its creatorFacet that updates the ZCF bundle that will be used when starting or upgrading contracts. startInstance has to retrieve that value each time it is going to pass it to the kernel. (It used to be cached.)

Additionally, discovered and fixed an issue with persisting zcfMints.

Security Considerations

Maintain Zoe's guarantees. Make it possible for users to know what version of ZCF will be used for new contracts.

Scaling Considerations

N/A

Documentation Considerations

None

Testing Considerations

Tested in #7966 and #7969

@Chris-Hibbert Chris-Hibbert self-assigned this Jun 16, 2023
@Chris-Hibbert Chris-Hibbert marked this pull request as draft June 16, 2023 23:41
@Chris-Hibbert Chris-Hibbert marked this pull request as ready for review June 21, 2023 18:17
@Chris-Hibbert Chris-Hibbert marked this pull request as draft June 21, 2023 18:17
@Chris-Hibbert Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from b8f88d7 to 14ac202 Compare June 22, 2023 16:37
@Chris-Hibbert Chris-Hibbert requested a review from turadg June 22, 2023 22:39
@Chris-Hibbert Chris-Hibbert marked this pull request as ready for review June 22, 2023 22:39
@turadg turadg mentioned this pull request Jun 23, 2023
@Chris-Hibbert Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from a7a4fed to a16493d Compare June 23, 2023 23:33
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.

Thanks for the commit comments. I needed them to understand the changes.

I still have a couple questions before approving.

zcfBundleCap = bundleCap;
},
e => {
console.warn(`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.

why only warn? This seems severe to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Zoe will continue to operate. The request probably had the wrong format of bundleID/bundleCap/etc. I'm amenable if you think it should be severe because the caller didn't get what they wanted.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, something failed in this case. It's at least console.error and a siren.

If throwing doesn't break anything other than the requested operation (which already failed) then please throw.

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

if (zoeBaggage.has(BUILD_PARAMS_KEY)) {
const { feeIssuerConfig, zcfSpec } = zoeBaggage.get(BUILD_PARAMS_KEY);
makeDurableZoeKit({
({ zoeConfigFacet } = makeDurableZoeKit({
Copy link
Member

Choose a reason for hiding this comment

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

this introduces a new concept, ZoeConfigFacet, which is the return type of makeDurableZoeKit.

Can we reconcile these names?

  1. call this zoeKit and have getZoeKit
  2. define a new type ZoeConfigFacet and declare makeDurableZoeKit to return one of those
  3. ??

I'm partial to #1 so the name matches the maker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makeDurableZoeKit returns { zoeService, zoeConfigFacet, feeMintAccess }, but this call only needs the zoeConfigFacet because the others have already been returned.

I'll add a comment.

Copy link
Member

Choose a reason for hiding this comment

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

I misread. Thanks for the comment.

if (zoeBaggage.has(BUILD_PARAMS_KEY)) {
const { feeIssuerConfig, zcfSpec } = zoeBaggage.get(BUILD_PARAMS_KEY);
makeDurableZoeKit({
({ zoeConfigFacet } = makeDurableZoeKit({
Copy link
Member

Choose a reason for hiding this comment

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

I misread. Thanks for the comment.

zcfBundleCap = bundleCap;
},
e => {
console.warn(`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.

Yeah, something failed in this case. It's at least console.error and a siren.

If throwing doesn't break anything other than the requested operation (which already failed) then please throw.

@Chris-Hibbert Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from a16493d to b9c3ff2 Compare July 17, 2023 23:07
Base automatically changed from 6678-intrinsicAtomicTransfer to master July 27, 2023 19:37
@Chris-Hibbert Chris-Hibbert added this pull request to the merge queue Jul 27, 2023
Merged via the queue into master with commit e6e23eb Jul 27, 2023
@Chris-Hibbert Chris-Hibbert deleted the 6678-upgradeZoeZcf branch July 27, 2023 20:22
mhofman pushed a commit that referenced this pull request Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants