-
Notifications
You must be signed in to change notification settings - Fork 217
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
fix(swingset): move upgrade() vatParameters into options bag #5365
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing I really think should be tweak, but other than that it looks good to me.
@@ -98,7 +98,13 @@ export function buildRootObject(vatPowers) { | |||
done() { | |||
return doneP; | |||
}, | |||
upgrade(bundlecap, vatParameters) { | |||
upgrade(bundlecap, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
should default to {}
, so existing code that isn't using it don't need to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. On second thought I'm not sure how downstream code will handle a missing vatParameters
parameter. I had been thinking you could get away with leaving it off if you didn't have any parameters, and maybe you could have. If so, you should still be able to do that. If not, maybe you should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
I guess omitting vatParameters
in the upgrade should get you the same value as you'd get omitting it in the original createVat
. That ought to be undefined
, but now that I'm looking at the code, I'm not sure that's what happens. I'll add a test.
62e9bde
to
0725890
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
は、私にはよく見えますよ
0725890
to
d3e2641
Compare
Previously, the parent vat did `E(adminFacet).upgrade(vatParameters)` Now it does `E(adminFacet).upgrade(bundleCap, { vatParameters })` This matches the way `vatParameters` are passed to the original `createVat()` call. All other options are rejected. The `options` argument itself is optional. This changes the `adminFacet` API, but internally continues to send `vatParameters` in their own argument to the underlying device nodes. If/when we send actual options, we'll need to decide whether to change the shape of the device node API by converting `vatParameters` into `options`, or just add additional arguments. closes #5345
d3e2641
to
d26b872
Compare
Previously, the parent vat did
E(adminFacet).upgrade(vatParameters)
Now it does
E(adminFacet).upgrade(bundleCap, { vatParameters })
This matches the way
vatParameters
are passed to the originalcreateVat()
call.All other options are rejected.
This changes the
adminFacet
API, but internally continues to sendvatParameters
in their own argument to the underlying devicenodes. If/when we send actual options, we'll need to decide whether to
change the shape of the device node API by converting
vatParameters
into
options
, or just add additional arguments.closes #5345