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.

Sign up for GitHub

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

Jump to bottom

Allow vats under SwingSet to unwrap their promises #674

Merged
merged 2 commits into from
Mar 15, 2020
Merged

Allow vats under SwingSet to unwrap their promises #674

merged 2 commits into from
Mar 15, 2020

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Mar 12, 2020 •
edited
Loading

Refs #74

Closes #682

Use @agoric/make-promise instead of the builtin makePromise.js that doesn't properly unwrap.

warner
warner approved these changes Mar 12, 2020
View reviewed changes
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Sounds ok to me. I'll defer to @erights about whether unwrap is ocap-safe, but I don't think I've heard him object.

katelynsills
katelynsills approved these changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

This looks good. I think there was some previous discussion about reusing the package @agoric/make-promise. Was there a reason not to do that? I can't remember

@michaelfig
Copy link
Member Author

This looks good. I think there was some previous discussion about reusing the package @agoric/make-promise. Was there a reason not to do that? I can't remember

Oh! I didn't remember about @agoric/make-promise. @warner, can we introduce it as a SwingSet dependency?

@warner warner mentioned this pull request Mar 13, 2020
Closed
@warner
Copy link
Member

warner commented Mar 13, 2020

yep, just opened an issue

@DavidBruant DavidBruant mentioned this pull request Mar 13, 2020
Open
16 tasks
@DavidBruant
Copy link
Contributor

The same fix was recently implemented in https://github.com/Agoric/agoric-sdk/pull/623/files#diff-f63ab4d221058b2338f0582969e18090

Oh! I didn't remember about @agoric/make-promise. @warner, can we introduce it as a SwingSet dependency?

+1

@warner
Copy link
Member

warner commented Mar 13, 2020

Huh, ok github's "open related issue" didn't do what I expected. Landing this (#674) PR will not resolve the new (#682) bug.. we need to change SwingSet to depend on make-promise (and use it internally, in lieu of it's own copy) to resolve #682.

Did github edit the top comment of this PR when I used the "open related issue" button?

I think we should edit the top comment to remove the claim that it closes the new ticket.

@DavidBruant
Copy link
Contributor

Did github edit the top comment of this PR when I used the "open related issue" button?

no, i think mfig did (3 hours ago)

image

@michaelfig
Copy link
Member Author

Yep, was in the midst of pushing new state and updated the issue in the wrong order.

@michaelfig
fix: allow vats under SwingSet to unwrap their promises
f2be5c7 
Use @agoric/make-promise to make Promises that can be unwrapped
(currently via the HandledPromise shim, but that may change).
@michaelfig michaelfig force-pushed the 74-swingset-allow-unwrap branch from 439927e to f2be5c7 Compare March 14, 2020 02:56
@warner
Copy link
Member

warner commented Mar 14, 2020 •
edited
Loading

I edited the comment to remove the magic bug-closing words

@michaelfig
Copy link
Member Author

I edited the comment to remove the magic bug-closing words

I put back the bug-closing words since that's now what this PR does. 😉

I should not have updated it preemptively. Too many balls in the air.

@michaelfig michaelfig added the SwingSet package: SwingSet label Mar 15, 2020
warner
warner approved these changes Mar 15, 2020
View reviewed changes
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

looks good, and yeah that closes the other ticket now

@michaelfig michaelfig merged commit eedd68d into master Mar 15, 2020
@michaelfig michaelfig deleted the 74-swingset-allow-unwrap branch March 15, 2020 20:10
@michaelfig michaelfig mentioned this pull request Sep 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@warner warner warner approved these changes

@katelynsills katelynsills Awaiting requested review from katelynsills

Assignees
No one assigned
Labels
SwingSet package: SwingSet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

have swingset import/use make-promise
4 participants
@michaelfig @warner @DavidBruant @katelynsills