Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make vatstore optionally available to vats as a vat power #3304

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Jun 12, 2021

Implements #3292

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Jun 12, 2021
@FUDCo FUDCo requested a review from warner June 12, 2021 02:44
@FUDCo FUDCo self-assigned this Jun 12, 2021
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.

Implementation looks good, modulo the recommended comments and test addition.

I'm tempted to suggest you remove the enable/disable switch, since it's a fairly big delta compared to the small (in my perspective, qv #3292 comments) need. It's a bummer that the process of adding vat options takes so much effort (those keys are carried through so many files before they make it to liveslots). But I'll leave that decision up to you.

send('get', 'foo');
send('get', 'zot');
await c.run();

Copy link
Member

Choose a reason for hiding this comment

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

Let's add another check here, peek into the kvStore and confirm the surviving foo key is being stored with the userspace-specific prefix, not something which could possibly overlap with e.g. the virtual object manager's keyspace.

@@ -916,6 +918,23 @@ function build(
if (enableDisavow) {
vpow.disavow = disavow;
}
if (enableVatstore) {
vpow.vatstore = harden({
Copy link
Member

Choose a reason for hiding this comment

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

let's add a comment here to describe how the vat's keyspace is carved up, and what code is responsible:

  • userspace vatstore: vvs.${userKey}
  • VOM makeWeakStore: ws${storeID}.${vobjID} (virtualObjectManager.js / makeWeakStore / virtualObjectKey)
  • VOM virtual objects: o+${kindID}/${instanceID} (virtualObjectManager.js / makeKind / makeNewInstance)

Or, put this comment up at the top of liveslots.js, and add pointers to it from the three places that currently compete for keyspace.

My biggest concern is that we might accidentally add a new feature in the future, and the implementor doesn't realize the importance of keeping the keyspaces distinct, and we wind up with a breach enabled by userspace intentionally colliding with the keyspace of something more powerful.

Copy link
Member

Choose a reason for hiding this comment

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

I'd feel slightly better if the virtual object manager used vom. in front of everything it manages (so vom.ws${storeID}.${vobjID} and vom.o+${kindID}/${instanceID}). On one hand, that feels overkill because those three keyspaces are already obviously non-overlapping. On the other hand, then a third feature can just choose a different prefix (foo.) instead of also needing to worry about how the VOM might change in the future and consume more of the keyspace.

@FUDCo FUDCo force-pushed the 3292-reify-vatstore-as-a-vat-power branch from 29d11b9 to 229da78 Compare June 14, 2021 17:52
@FUDCo FUDCo enabled auto-merge June 14, 2021 17:54
@FUDCo FUDCo merged commit a74e826 into master Jun 14, 2021
@FUDCo FUDCo deleted the 3292-reify-vatstore-as-a-vat-power branch June 14, 2021 18:20
@FUDCo
Copy link
Contributor Author

FUDCo commented Jun 15, 2021

Closes #3292

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants