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

make Zoe durable #5997

Merged
merged 14 commits into from
Dec 20, 2022
Merged

make Zoe durable #5997

merged 14 commits into from
Dec 20, 2022

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Aug 18, 2022

closes: #4383
refs: #6502

Description

Make Zoe durable.

The prior version passed around many bundles of functions. Those had to be converted to durable facets to be passable.

Security Considerations

Pervasive

Documentation Considerations

seat notifiers (which notified on interim allocations) have been dropped and replaced with a subscriber that only notifies when the seat exits.

The way feeMintAccess is obtained changed.

The methods that allowed requests for internal allocation state (getCurrentAllocationJig) has been dropped. Many of the clients can use getFinalAllocation() instead.

zoe.bindDefaultFeePurse, zoe.makeFeePurse, and zcf.assert() were dropped.

Testing Considerations

Test timing changed in some cases, since Promises can no longer be used.

@Chris-Hibbert Chris-Hibbert self-assigned this Aug 18, 2022
@Chris-Hibbert Chris-Hibbert marked this pull request as draft August 18, 2022 19:37
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch from 575fd50 to e989f7f Compare December 1, 2022 01:36
@Chris-Hibbert Chris-Hibbert changed the base branch from master to gibson-4567-notifier-package-durability December 1, 2022 17:33
@Chris-Hibbert Chris-Hibbert added Zoe package: Zoe far Distributed object helpers security Zoe Contract Contracts within Zoe vaults-release labels Dec 1, 2022
@Chris-Hibbert Chris-Hibbert marked this pull request as ready for review December 1, 2022 17:57
@Chris-Hibbert Chris-Hibbert requested review from erights and removed request for turadg December 1, 2022 17:57
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch from e989f7f to 350b919 Compare December 1, 2022 22:10
@Chris-Hibbert Chris-Hibbert changed the title For examination: make Zoe instanceAdmin durable make Zoe instanceAdmin durable Dec 1, 2022
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch 2 times, most recently from e63d04d to c3010a4 Compare December 2, 2022 00:36
@Chris-Hibbert Chris-Hibbert changed the title make Zoe instanceAdmin durable make Zoe durable Dec 2, 2022
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch 2 times, most recently from 58d46cf to 86dcfcc Compare December 2, 2022 23:57
@turadg turadg mentioned this pull request Dec 3, 2022
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Review so far. I got through all of zcf and everything but zoe itself. Of course, zoe is where the action is, so it will still take significant time to complete.

High level comment so far: I don't understand why zcf needed such radical surgery, since coveredCall-durable.js already demonstrates that zcf is upgradable. If we really were saving all the precious semantic state that the successor needs to revivify a working zcf, then the additional durable things seem like a distraction.

packages/zoe/src/contractFacet/exit.js Show resolved Hide resolved
packages/zoe/src/contractFacet/zcfSeat.js Outdated Show resolved Hide resolved
packages/zoe/src/contractFacet/zcfMint.js Show resolved Hide resolved
packages/zoe/src/contractFacet/zcfZygote.js Show resolved Hide resolved
packages/zoe/src/contractFacet/zcfZygote.js Outdated Show resolved Hide resolved
packages/zoe/src/makeHandle.js Show resolved Hide resolved
packages/zoe/src/typeGuards.js Outdated Show resolved Hide resolved
packages/zoe/src/typeGuards.js Show resolved Hide resolved
packages/zoe/src/typeGuards.js Show resolved Hide resolved
packages/zoe/src/typeGuards.js Outdated Show resolved Hide resolved
@Chris-Hibbert
Copy link
Contributor Author

High level comment so far: I don't understand why zcf needed such radical surgery, since coveredCall-durable.js already demonstrates that zcf is upgradable. If we really were saving all the precious semantic state that the successor needs to revivify a working zcf, then the additional durable things seem like a distraction.

This is an important question.

The original Zoe and ZCF shared some state using Promises and bags of functions. It's no longer possible for Zoe to hold onto a loose function, so some things that didn't have to be durable object for the ZCF upgrade case have to be durable In order for Zoe to hold onto them.

@gibson042 gibson042 force-pushed the gibson-4567-notifier-package-durability branch 2 times, most recently from eac2982 to dc22548 Compare December 5, 2022 23:31
packages/zoe/src/typeGuards.js Outdated Show resolved Hide resolved
packages/zoe/src/zoeService/zoeSeat.js Outdated Show resolved Hide resolved
@erights
Copy link
Member

erights commented Dec 19, 2022

This review pass done.

@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch from 7ebfd2d to 8137369 Compare December 20, 2022 01:08
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch from 8137369 to 2c41f06 Compare December 20, 2022 01:27
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Just that one remaining issue in the declaration of BundleShape.

LGTM!

Comment on lines 234 to 237
export const BundleShape = M.and(
M.partial({ moduleFormat: M.string() }),
M.recordOf(M.string(), M.string()),
);
Copy link
Member

@erights erights Dec 20, 2022

Choose a reason for hiding this comment

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

I think this was more correct before literally taking @kriskowal 's advice.

  • @Chris-Hibbert , We still need the { stringLengthLimit: Infinity } to evade the limit check, right?
  • @kriskowal , M.partial is deprecated in favor of M.splitArray and M.splitRecord.
  • @kriskowal , By using M.partial you're saying that moduleFormat is optional, as did the old code when the first argument to splitRecord was harden({}). But your prose says it is mandatory. Assuming it is mandatory, I removed that first argument to splitRecord below.

So, does the following look right?

export const ModuleFormatBundleShape = M.splitRecord(
  harden({ moduleFormat: M.any() }),
);

export const BundleShape = M.and(ModuleFormatBundleShape, SourceBundleShape);

Copy link
Member

Choose a reason for hiding this comment

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

(just edited my suggested code from M.or to M.and)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need the { stringLengthLimit: Infinity } to evade the limit check, right?

I believe so. It's still in SourceBundleShape. I don't think it's necessary anywhere else.

export const SourceBundleShape = M.recordOf(
  M.string(),
  M.string({ stringLengthLimit: Infinity }),
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed this change, and tests are passing now.

@kriskowal with your agreement that this change is correct, I'll merge this PR.

@Chris-Hibbert Chris-Hibbert removed the automerge:squash Automatically squash merge label Dec 20, 2022
Comment on lines 230 to 237
export const SourceBundleShape = M.recordOf(
M.string(),
M.string({ stringLengthLimit: Infinity }),
);
export const ModuleFormatBundleShape = M.splitRecord(
harden({ moduleFormat: M.any() }),
);
export const BundleShape = M.and(ModuleFormatBundleShape, SourceBundleShape);
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
export const SourceBundleShape = M.recordOf(
M.string(),
M.string({ stringLengthLimit: Infinity }),
);
export const ModuleFormatBundleShape = M.splitRecord(
harden({ moduleFormat: M.any() }),
);
export const BundleShape = M.and(ModuleFormatBundleShape, SourceBundleShape);
export const BundleShape = M.and(
M.splitRecord(harden({ moduleFormat: M.string() })),
M.recordOf(
M.string(),
M.string(harden({ stringLengthLimit: Infinity })),
),
);

How does splitRecord differ from partial? Is harden necessary there in a way that would apply equally to M.string’s arg?

The bundle shape described in this way applies equally well to source bundles and hash bundles, so the intermediate names are not useful. If we wish to cut finer distinctions, we’ll need shapes for SourceBundle and HashBundle that are the union of supported formats in each case. HashBundle is of the form { moduleFormat: 'endoZipBase64Sha512', endoZipBase64Sha512: string } and SourceBundle is one of { moduleFormat: 'getExports', source: string }, { moduleFormat: 'nestedEvaluate', source: string }, or { moduleFormat: 'endoZipBase64', endoZipBase64: string, endoZipBase64Sha512?: string }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to this simplified BundleShape, and used it in all the places BundleShape or SourceBundleShape appeared. I'll merge like this.

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label Dec 20, 2022
@Chris-Hibbert Chris-Hibbert force-pushed the 4383-durable-InstanceAdmin branch from b0864f0 to ad36f3c Compare December 20, 2022 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge far Distributed object helpers security Zoe Contract Contracts within Zoe Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

store Zoe state in durable/virtual objects/collections
4 participants