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

Changes to remove extraneous HandledPromise shim dependencies #1463

Merged
merged 15 commits into from
Aug 14, 2020

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Aug 13, 2020

Closes #1469

We import the HandledPromise shim once-and-for-all in @agoric/install-ses. Then, using a version of SES with endojs/endo#416, it is propagated as a global to all child compartments.

Removing the extraneous HandledPromise plumbing from the code will make it easier to migrate to the new Promise.delegate API once @agoric/eventual-send/shim implements it.

@michaelfig michaelfig added the eventual-send package: eventual-send label Aug 13, 2020
@michaelfig michaelfig self-assigned this Aug 13, 2020
@warner
Copy link
Member

warner commented Aug 13, 2020

I'm surprised the tests pass if all the Compartments are now missing their HandledPromise endowment entirely. Does that work because install-ses is now changing the global Promise to really be HandledPromise?

Take a second look at install-metering-and-ses and check that the ordering of (modify globalThis.Promise) and (instrument globalThis) doesn't leave us with a non-instrumented Promise.. I suspect there might be a problem there but I haven't dived in yet.

I also noticed this downgrades the harden(E) to Object.freeze(E). Does that open up a communication channel via something like E.when.message = 'hello' ?

@michaelfig
Copy link
Member Author

I'm surprised the tests pass if all the Compartments are now missing their HandledPromise endowment entirely. Does that work because install-ses is now changing the global Promise to really be HandledPromise?

It will work with endojs/endo#416

Take a second look at install-metering-and-ses and check that the ordering of (modify globalThis.Promise) and (instrument globalThis) doesn't leave us with a non-instrumented Promise.. I suspect there might be a problem there but I haven't dived in yet.

I've fixed this by ensuring the shim is installed before metering is turned on.

I also noticed this downgrades the harden(E) to Object.freeze(E). Does that open up a communication channel via something like E.when.message = 'hello' ?

I was hoping to split the restructuring changes (needed to disentangle harden from the vetted shim before lockdown) to eventual-send to a different PR, but I could do them in the current PR if that would be better.

@michaelfig michaelfig force-pushed the mfig/handled-promise-shim branch 2 times, most recently from 4f76091 to e505ac4 Compare August 14, 2020 02:44
@michaelfig michaelfig added the tooling repo-wide infrastructure label Aug 14, 2020
@michaelfig
Copy link
Member Author

@erights, this is of lower priority than Zoe RC. I only expect a detailed review from you on the changes to @agoric/eventual-send that were necessary in order to make it a vetted shim. Notably, we couldn't early-bind some global properties since the metering transform replaced them in the course of wrapping all the globals (including HandledPromise).

@michaelfig michaelfig added hygiene Tidying up around the house and removed tooling repo-wide infrastructure labels Aug 14, 2020
@michaelfig michaelfig marked this pull request as ready for review August 14, 2020 04:39
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.

LGTM

packages/dapp-svelte-wallet/package.json Show resolved Hide resolved
packages/eventual-send/shim.js Outdated Show resolved Hide resolved
packages/eventual-send/shim.js Show resolved Hide resolved
packages/eventual-send/src/E.js Outdated Show resolved Hide resolved
packages/eventual-send/src/index.js Outdated Show resolved Hide resolved
packages/eventual-send/src/index.js Show resolved Hide resolved
packages/eventual-send/src/no-shim.js Show resolved Hide resolved
packages/promise-kit/src/promiseKit.js Outdated Show resolved Hide resolved
packages/promise-kit/src/promiseKit.js Show resolved Hide resolved
@michaelfig
Copy link
Member Author

@FUDCo or @warner, would you be able to review the other changes (not packages/eventual-send)? The criterion is whether this is indeed a good cleanup.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Three cheers for taking out the trash!

@michaelfig michaelfig merged commit 6f97075 into master Aug 14, 2020
@michaelfig michaelfig deleted the mfig/handled-promise-shim branch August 14, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eventual-send package: eventual-send hygiene Tidying up around the house
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn off eslint's import/prefer-default-export globally
4 participants