support changes to governed APIs #10822
Conversation
Chris-Hibbert
force-pushed
the
9990-upgradeAPIs
branch
from
cc5512a to
4d3a323
Compare
Deploying agoric-sdk with
|
| Latest commit: |
d1ec616
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://994d62e6.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9990-upgradeapis.agoric-sdk.pages.dev |
Chris-Hibbert
force-pushed
the
9990-upgradeAPIs
branch
2 times, most recently
from
5d8e282 to
0edc4ac
Compare
Chris-Hibbert
force-pushed
the
9990-upgradeAPIs
branch
from
0edc4ac to
d1ec616
Compare
There was a problem hiding this comment.
Yes, I agree we should merge this.
How to validate it on a testnet in an upcoming release is sort of... interesting. It will have no impact until some governed contract gets upgraded. That might be in a later release. At that point, there would be a testing burden to verify that this fix works. But this issue won't be listed in the things we fixed for that later release. Seems like an acceptable risk.
The rest of my comments are sort of thinking out loud. Maybe I should keep them to myself... but I suppose I'll share anyway.
| ]; | ||
|
|
||
| const setUpGovernedContract = async (zoe, timer, EV, controller) => { | ||
| const installBundle = contractBundle => EV(zoe).install(contractBundle); |
There was a problem hiding this comment.
Is it important to use .install(...) rather than .installBundleID(...)?
| add2: { outgoing: 'add2' }, | ||
| }; | ||
|
|
||
| let governedContract; |
There was a problem hiding this comment.
module level mutable state. hm. consider using a promise(kit) in t.context something like t.context.governedContractKit.promise and .resolver
| const agoricNamesAdmin = | ||
| await EV.vat('bootstrap').consumeItem('agoricNamesAdmin'); | ||
| const instanceAdmin = await EV(agoricNamesAdmin).lookupAdmin('instance'); | ||
| await EV(instanceAdmin).update('governedContract', instance); |
There was a problem hiding this comment.
I wonder why we're adding governedContract to agoricNames.instance
| // Far side-effects the object, and can only be applied once | ||
| const farGovernedApis = Far('governedAPIs', governedApis); |
There was a problem hiding this comment.
after thinking about this a bit, it's probably OK as is. But I'll leave some breadcrumbs...
can we make it the caller's responsibility to mark it Far? Making the record in one place and marking it Far in another is odd.
One possibility is to make a new record...
| // Far side-effects the object, and can only be applied once | |
| const farGovernedApis = Far('governedAPIs', governedApis); | |
| // Far side-effects the object, and can only be applied once | |
| const farGovernedApis = Far('governedAPIs', { ... governedApis }); |
| } catch (e) { | ||
| Fail`restartContract failed: original contract facets were not durable`; | ||
| } |
There was a problem hiding this comment.
Is it really the case that the only exception possible is that the facets were not durable? I suppose so... zcfBaggage.get(...) is the only thing in the try.
But if get(...) had a bug, we might lose diagnostic info here. It feels weird to throw away e completely.
log it?
There was a problem hiding this comment.
Do we support something like this?
| } catch (e) { | |
| Fail`restartContract failed: original contract facets were not durable`; | |
| } | |
| } catch (e) { | |
| throw assert.error(`restartContract failed: original contract facets were not durable`, { cause: e }); | |
| } |
| @@ -0,0 +1,311 @@ | |||
| import { test as anyTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js'; | |||
There was a problem hiding this comment.
it's a little hard to see the forest for the trees.
Part of me would like the uninteresting stuff moved to a separate file, but then I'd have to jump back and forth.
closes: #9990
Description
When a contract is upgraded, we'll read the governed APIs afresh.
Security Considerations
Nothing special. The point of governed APIs is that contracts can make access to calling specific APIs legible in the sense used by governance.
Scaling Considerations
Not an issue.
Documentation Considerations
If we had detailed documentation on governance, this would be worth adding to the docs.
Testing Considerations
Added a bootstrap test showing that a contract can be upgraded and the new APIs will be available after upgrade.
Upgrade Considerations
AssetReserve, fluxAggregator, and VaultFactory use
apiGovernanceand might need this support if any governed APIs were added.