Rename vat-chainStorage to vat-bridge and make it upgradable
#7107
Conversation
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
d44a416
to
19a4ead
Compare
Datadog ReportBranch report: ✅ |
packages/vats/src/vat-bridge.js
Outdated
| const provideManagerForBridge = zoneBridgeManager( | ||
| zone.subZone('BridgeManager'), | ||
| D, | ||
| ); | ||
| const makeChainStorageNode = zoneChainStorageNode( | ||
| zone.subZone('ChainStorageNode'), | ||
| ); |
There was a problem hiding this comment.
Given that zoneBridgeManager starts with zone.subZone('ScopedManager').exoClass('BridgeScopedManager', …), zone.exoClassKit('BridgeManagerKit', …), and zone.mapStore('bridgeToManagerKit'); and that zoneChainStorageNode starts with zone.exoClass('ChainStorageNode', …), isn't this redundant nesting? I'd much rather see an arrangement in which all zone keys appear at the same (high) level. If we're not comfortable with representing specific MapStore keys as objects, perhaps instead we need a way to attenuate the zones?
| const provideManagerForBridge = zoneBridgeManager( | |
| zone.subZone('BridgeManager'), | |
| D, | |
| ); | |
| const makeChainStorageNode = zoneChainStorageNode( | |
| zone.subZone('ChainStorageNode'), | |
| ); | |
| const view = zone.attenuate({ | |
| keys: ['BridgeScopedManager', 'BridgeManagerKit', 'bridgeToManagerKit'], | |
| }); | |
| const provideManagerForBridge = zoneBridgeManager(view, D); | |
| const makeChainStorageNode = zoneChainStorageNode(view); |
There was a problem hiding this comment.
The mechanism for attenuating the zones is to create a subzone. Attenuation based on filtering keys by name is discouraged because it is tightly coupled to the library using the zone, for which the specific key names are an implementation detail.
Your critique about redundant nesting for the subZone('ScopedManager') is a good one, though. I'll fix that.
There was a problem hiding this comment.
Attenuation based on filtering keys by name is discouraged because it is tightly coupled to the library using the zone, for which the specific key names are an implementation detail.
Understood, but this cure seems worse than the disease. If I understand correctly, we're going to end up with baggage full of deeply nested but mostly empty maps, which will be unnecessarily difficult to debug:
MapStore{
BridgeManager: MapStore{
ScopedManager: MapStore{
BridgeScopedManager_kindHandle: Far('kind', {}),
},
BridgeManagerKit_kindHandle: Far('kind', {}),
bridgeToManagerKit: MapStore{…},
},
ChainStorageNode: MapStore{
ChainStorageNode_kindHandle: Far('kind', {}),
},
…
}
while I think we should strive for something more like
MapStore{
BridgeChannel_kindHandle: Far('kind', {}),
BridgeManagerKit_kindHandle: Far('kind', {}),
bridgeToManagerKit: MapStore{…},
ChainStorageNode_kindHandle: Far('kind', {}),
…
}
There was a problem hiding this comment.
Just a minor correction: I've already changed this so that there is no ScopedManager MapStore, which leaves the general structure 2 levels deep (first is controlled by the vat/contract initialization code with keys corresponding to packages, second is keys controlled by the individual package). Using subZones in this way is a straightforward (once documented) way to protect against one package stealing keys from another package.
If having 2 levels is still too difficult to debug, it is worth mentioning that the concept of a subZone doesn't inherently need to be implemented as deeply-nested maps. Another way forward could be to use a flat map with key prefixes to represent deep keys, somewhat like how you did with chainStorageNode.
MapStore{
"BridgeManager:BridgeManagerKit_kindHandle": Far('kind', {}),
"BridgeManager:BridgeScopedManager_kindHandle": Far('kind', {}),
"BridgeManager:bridgeToManagerKit": MapStore{...},
"ChainStorageNode:ChainStorageNode_kindHandle": Far('kind', {}),
...
}
Is either of these alternatives acceptable to you? Do they inspire you to propose something else?
There was a problem hiding this comment.
I do not understand the problem
- with nested sub-zones as attenuation. If another mechanism would not replace sub-zones, then it would seem to be unnecessary additional complexity. And I can't see how it could replace sub-zones.
- with nested sub-zones resulting in nested baggage map-stores. Seems more straightforward than imposing a prefixing convention on flat names. Also, any string mangling system would need to worry about quoting confusion and hilbert, which we simply avoid by using nested maps.
We should probably have a verbal discussion.
There was a problem hiding this comment.
@erights We should keep in mind that the current state of sub-zones is insufficient to handle things that will definitely be needed, like limits for key counts, key length, entry depth, value size, and total data usage, and possibly even for things like key pattern. We can probably add all of that to sub-zones, but they may not turn out to be the right abstraction (for example, in a filesystem, directories are generally quite distinct from regular files and sometimes control is delegated at the level of a directory but other times it is at the level of one or more specific files—there's a reason why e.g. mktemp supports a --directory option rather than always creating a directory, even though single-file behavior is possible to implement on top of a directory-only foundation).
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
17b330d
to
2c35ee4
Compare
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
2c35ee4
to
7648f44
Compare
Datadog ReportBranch report: ✅ |
michaelfig
force-pushed
the
mfig-zones
branch
2 times, most recently
from
9b2697b
to
46aae49
Compare
michaelfig
force-pushed
the
mfig-vat-bridge
branch
2 times, most recently
from
f04c118
to
c119eb2
Compare
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
c119eb2
to
db21963
Compare
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
db21963
to
63e0b89
Compare
There was a problem hiding this comment.
Thanks for the reviews, @gibson042! I've responded to or incorporated all your comments.
packages/internal/src/callback.js
Outdated
| * @param {Parameters<I>} args | ||
| * @returns {Promise<Awaited<ReturnType<I>>>} | ||
| */ | ||
| export const esend = (callback, ...args) => { |
There was a problem hiding this comment.
I understand this intuition, and in fact went out of my way to explore it, but there is a security boundary at play here.
If I use call(callback), I'm running the callback synchronously and so without proper care, it may interfere with my plans. If I use esend(callback), I'm protected by the async isolation guarantees that E(x) provides, regardless of the target of the callback.
Having said that, I acknowledge that the name call is terrible at conveying this. I think I will change it to callSync. I'm also thinking of changing esend to callE so that it rhymes and is reliant on E(target).
| * one-off objects created with `Far()` or `makeExo()` | ||
| * instances created with a "make" function from `defineExoClass()` | ||
| * multifaceted kit instances created with a "makeKit" function from `defineExoClassKit()` | ||
|
|
||
| * Virtual objects in disk-based storage | ||
| * zone API found at `import { virtualZone } from '@agoric/zone/virtual.js';` | ||
| * instances created with a "make" function from `defineVirtualExoClass()` | ||
| * multifaceted kit instances created with a "makeKit" function from `defineVirtualExoClassKit()` |
There was a problem hiding this comment.
New section to collect all the Zone documentation that was scattered around the previous section.
Datadog ReportBranch report: ✅ |
There was a problem hiding this comment.
LGTM, but I am increasingly of the opinion that callSync is a footgun and should be removed. The typing in callback.js is quite a feat!
packages/zoe/tools/internal-types.js
Outdated
| * @property {(msg?: string) => ERef<void>} tick Advance the timer by one tick. | ||
| * DEPRECATED: use `await tickN(1)` instead. `tick` function errors might be | ||
| * thrown synchronously, even though success is signaled with `void` or | ||
| * `Promise<void>`. |
There was a problem hiding this comment.
I'm convinced by your desire to reserve ERef for inputs, and this can use JSDoc @deprecated.
| * @property {(msg?: string) => ERef<void>} tick Advance the timer by one tick. | |
| * DEPRECATED: use `await tickN(1)` instead. `tick` function errors might be | |
| * thrown synchronously, even though success is signaled with `void` or | |
| * `Promise<void>`. | |
| * @deprecated Use `await tickN(1)` instead. `tick` function errors might be | |
| * thrown synchronously, even though success is signaled by returning anything | |
| * other than a rejected promise. | |
| * @property {(msg?: string) => void | Promise<void>} tick Advance the timer by one tick. |
There was a problem hiding this comment.
Unfortunately, @deprecated only displays its doc string for a declaration it decorates, not a @property.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7107 +/- ##
==========================================
- Coverage 79.40% 70.62% -8.78%
==========================================
Files 390 447 +57
Lines 73961 85077 +11116
Branches 3 3
==========================================
+ Hits 58729 60088 +1359
- Misses 15231 24923 +9692
- Partials 1 66 +65
... and 55 files with indirect coverage changes |
michaelfig
force-pushed
the
mfig-vat-bridge
branch
from
c1f757c
to
4b1a425
Compare
michaelfig
added
the
automerge:no-update
refs: #6553
Description
Based on #7057.
Both chain storage and the bridge manager are made durable, living in an upgradable
vats/src/vat-bridge.js.SwingSet changes should be reviewed by somebody from the kernel team.
Security Considerations
Usual security concerns and benefits with upgradability.
Scaling Considerations
All the usual durability concerns with disk vs. memory vs. cpu consumption.
Documentation Considerations
n/a
Testing Considerations
Null upgrade tests now also include basic functionality unit tests for the bridge and chain storage.