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

Implement multifaceted virtual objects #4742

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Mar 5, 2022

These changes implement the multifaceted virtual objects API per our current design.

Closes #3834

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Mar 5, 2022
@FUDCo FUDCo requested review from warner and erights March 5, 2022 00:12
@FUDCo FUDCo self-assigned this Mar 5, 2022
@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from 43c7d08 to 2f50f8a Compare March 5, 2022 00:41
@FUDCo
Copy link
Contributor Author

FUDCo commented Mar 5, 2022

Probably worth calling out for the record one small change to the code as implemented from the implementation design in #3834: this implementation uses a different separator character (in this case, comma) between the main portion of the vref and the facet identifier than the separator character (slash) between the base kind vref and the instance identifier. The design proposed the new multi-facet virtual objects would get o+NN/AA/BB, where NN is the kind ID, AA is the instance (cohort) ID, and BB is the facet ID. I found it highly advantageous to use o+NN/AA,BB instead, as there are numerous cases where I needed to strip off the facetID while leaving the rest intact, and having a separate separator meant not having to count separators. While I don't think this bit of minor pragmatism is likely to be terribly controversial, it warrants mention.

@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from 2f50f8a to 838a7c7 Compare March 5, 2022 01:00
erights
erights previously requested changes Mar 7, 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.

I reviewed what I understood. But even after it looks good to me, you should get @warner's approval before merging. There too much I simply cannot evaluate.

packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved
packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved
packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualReferences.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualReferences.js Outdated Show resolved Hide resolved
@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from 838a7c7 to c85e18b Compare March 7, 2022 21:53
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.

I'm submitting a partial review to start the discussion, I'll re-request myself for review and continue the process tomorrow. I've only gotten as far as virtualReferences.js.

packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved
packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved

Additional important details:

- The set of state properties is fully determined by the `init` function. That is, the set of properties that exist on `state` is completely determined by the enumerable properties of the object that `init` returns. State properties cannot thereafter be added or removed.
- The set of state properties of an instance is fully determined by the `init` function. That is, the set of properties that exist on `state` is completely determined by the enumerable properties of the object that `init` returns. State properties cannot thereafter be added or removed.
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention (maybe with a TODO) that this set might vary between instances? Or that init() "SHOULD" return the same set of properties for all instances, even though maybe we don't catch this yet, and that a future implementation will be strict about enforcing it?

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 think we need to make a decision whether we are even going to allow init functions to return different object shapes within a kind.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I think we have opinions (we're in favor of fixed-shape, because give us more flexibility to make a real DB schema, right?), so we should make it clear that authors ought to use a fixed shape, even if we aren't enforcing it yet. "All instances of a given kind ought to have the same set of properties" might be enough.

packages/SwingSet/src/lib/parseVatSlots.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/liveslots.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualReferences.js Outdated Show resolved Hide resolved
@warner warner self-requested a review March 8, 2022 02:04
@erights erights self-requested a review March 9, 2022 02:57
@erights
Copy link
Member

erights commented Mar 9, 2022

Hi @FUDCo I asked myself for another review. Please don't squash any further changes yet.

@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from c85e18b to deeab24 Compare March 10, 2022 02:57
@FUDCo FUDCo requested a review from erights March 10, 2022 08:52
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.

  • cohort record needs retention
  • type naming comments


Additional important details:

- The set of state properties is fully determined by the `init` function. That is, the set of properties that exist on `state` is completely determined by the enumerable properties of the object that `init` returns. State properties cannot thereafter be added or removed.
- The set of state properties of an instance is fully determined by the `init` function. That is, the set of properties that exist on `state` is completely determined by the enumerable properties of the object that `init` returns. State properties cannot thereafter be added or removed.
Copy link
Member

Choose a reason for hiding this comment

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

Right, but I think we have opinions (we're in favor of fixed-shape, because give us more flexibility to make a real DB schema, right?), so we should make it clear that authors ought to use a fixed shape, even if we aren't enforcing it yet. "All instances of a given kind ought to have the same set of properties" might be enough.

packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualReferences.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/liveslots.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/liveslots/virtualReferences.js Outdated Show resolved Hide resolved
@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch 5 times, most recently from cd6ca3a to e090593 Compare March 16, 2022 07:26
@FUDCo FUDCo requested a review from warner March 16, 2022 07:42
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.

ceaseRecognition needs discussion, the rest is minor, we can land once we figure out that one.

@warner
Copy link
Member

warner commented Mar 17, 2022

My remaining requested changes are cosmetic: comments and assert description strings. Feel free to land after you've added those (and rebased/squashed/etc).

@erights said he was satisfied but his review says "Changes requested", so I'm going to dismiss his review so it won't prevent you from landing this (and so we don't have to bother him with a re-review request).

@warner warner dismissed erights’s stale review March 17, 2022 23:22

@erights said he was satisified in the comment

@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from e090593 to a644780 Compare March 18, 2022 06:18
@FUDCo FUDCo force-pushed the 3834-multifaceted-virtual-objects branch from a644780 to 0696866 Compare March 18, 2022 06:29
@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Mar 18, 2022
@mergify mergify bot merged commit 661687f into master Mar 18, 2022
@mergify mergify bot deleted the 3834-multifaceted-virtual-objects branch March 18, 2022 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple facets for each virtual object
3 participants