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

refactor: add passStyleOf to VatData global #8051

Closed
wants to merge 1 commit into from

Conversation

erights
Copy link
Member

@erights erights commented Jul 14, 2023

See endojs/endo#1676

#9431 currently staged on this PR

Harmless for now. Eventually to be picked up by @endo/pass-style as imported after liveslots, so it will use the one from liveslots. This makes the determinism of user code depend on passStyleOf really being deterministic. Otherwise, user code could observe GC.

@erights erights requested a review from warner July 14, 2023 23:36
@erights erights self-assigned this Jul 14, 2023
@erights erights changed the title refactor: add passStyleOf to VatData global refactor: add passStyleOf to VatData global Jul 14, 2023
@erights erights requested a review from mhofman July 14, 2023 23:37
@erights erights marked this pull request as draft July 14, 2023 23:43
@erights erights marked this pull request as ready for review July 14, 2023 23:56
@erights erights requested a review from kriskowal July 15, 2023 02:37
@mhofman
Copy link
Member

mhofman commented Jul 17, 2023

I still have concerns about using VatData to plumb powers from agoric-sdk into the endo layers, as I raised in endojs/endo#1676 (comment)

I actually do like @gibson042's suggestion in endojs/endo#1676 (comment) to use a registered symbol.

@erights erights force-pushed the markm-vatdata-pass-style branch 3 times, most recently from 731829d to 3f0afb1 Compare July 29, 2023 23:44
@erights erights force-pushed the markm-vatdata-pass-style branch 3 times, most recently from f07d107 to d9bb678 Compare August 11, 2023 04:45
@erights erights force-pushed the markm-vatdata-pass-style branch 2 times, most recently from 6a0a373 to 2c91f28 Compare August 22, 2023 02:35
@erights erights changed the base branch from master to markm-8231-interfaces-be-passable August 22, 2023 02:36
@erights erights changed the base branch from markm-8231-interfaces-be-passable to master August 28, 2023 05:53
@erights erights force-pushed the markm-vatdata-pass-style branch 3 times, most recently from f6f3203 to f6e0b5e Compare August 30, 2023 01:31
@erights erights force-pushed the markm-vatdata-pass-style branch 2 times, most recently from b36c686 to c3d92b1 Compare September 16, 2023 03:44
@erights erights force-pushed the markm-vatdata-pass-style branch 2 times, most recently from c193da3 to 1505dc6 Compare September 27, 2023 23:02
@erights
Copy link
Member Author

erights commented Sep 27, 2023

I actually do like @gibson042's suggestion in endojs/endo#1676 (comment) to use a registered symbol.

Reviewers,
Could we agree on exactly that suggestion?

If so, we can deal with the inter-repo version skew issue as follows:

This PR place passStyleOf in both places for now, but mark the VatData placement as deprecated. We should get the full performance benefit at this step. The further steps below should have no further effect.

An Endo PR look for passStyleOf in both places, marking the VatData placement as deprecated.

Endo release / agoric-sdk upgrade ritual

agoric-sdk PR that stops putting it into VatData.

and endo PR that stops looking for it in VatData.

@mhofman
Copy link
Member

mhofman commented Sep 27, 2023

I actually do like @gibson042's suggestion in endojs/endo#1676 (comment) to use a registered symbol.

Reviewers, Could we agree on exactly that suggestion?

If so, we can deal with the inter-repo version skew issue as follows:

Ok I refreshed my mind on this topic. Let's do the registered symbol approach.

I however do not understand the need for the migration dance. Why do we need agoric-sdk to place it in both places? Let's just have agoric-sdk place it with the registered symbol, and update endo to only look there. I don't believe there's been any actual use of the VatData location, and at the next Endo sync, it'll just start working.

@erights
Copy link
Member Author

erights commented Sep 28, 2023

Let's do the registered symbol approach.

Excellent!

I however do not understand the need for the migration dance.

So we can get the benefit before the next Endo sync.

@erights
Copy link
Member Author

erights commented Sep 28, 2023

Based on a private conversation with @kriskowal (thanks!), I'm satisfied we can do the endo+agoric-sdk versioning dance fast enough that I should just remove the VatData option from endo, and from this PR.

@erights
Copy link
Member Author

erights commented Sep 28, 2023

In light of endojs/endo#1794 I’m trying to figure out how VatData gets from liveslots onto the relevant globals. That journey landed me at

https://github.com/endojs/endo/blob/52aba3137cb60204bcfc2af54393edef9cb65b96/packages/import-bundle/src/compartment-wrapper.js#L45

where an Object.keys will skip symbol-named properties. But I don’t know whether this code is actually relevant anyway. Help?

@mhofman
Copy link
Member

mhofman commented Oct 3, 2023

As mentioned in Slack, my understanding is that:

So it should work out of the box? Is it failing to appear in the compartment created by liveslots, or were you trying to propagate automatically to any nested compartment (e.g. to avoid having to modify ZCF as well, and how is ZCF handling VatData today?). Arguably the inescapable properties should propagate both symbol and string keys.

@erights
Copy link
Member Author

erights commented Sep 2, 2024

Closing in favor of recent changes

@erights erights closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants