fix: contractGovernor can change params after contract is upgraded #10163
Conversation
Chris-Hibbert
added
bug
|
What's the upgrade / deployment story? |
My hope is that we can approve it before the priceFeed coreEval goes out, so the change will be included in any contractGovernors that get upgraded or replaced. Hmm. that means I'll need a follow-on PR to add installing the code to the priceFeed coreEval. Thanks for asking. |
does that mean "closes: #9982" should be changed to "refs"? |
I don't think so. AIUI, we close bug reports when the code that fixes the issue is merged, not when it gets to the chain. |
|
My understanding is that merging to master is supposed to include whatever code / tests are necessary to get it on chain. |
Deploying agoric-sdk with
|
| Latest commit: |
25d3578
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40a50467.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9982-gov-upgrade-parammgr.agoric-sdk.pages.dev |
added. |
| governorRef: publishRef( | ||
| install( | ||
| '@agoric/governance/src/contractGovernor.js', | ||
| '../bundles/bundle-contractGovernor.js', |
There was a problem hiding this comment.
Please add a test that this works.
I just ran into at least one case where it does not while working on dapp-orchestration-basics deployment testing. And now I see why. Note the lack of .reset() in...
agoric-sdk/packages/deploy-script-support/src/coreProposalBehavior.js
Lines 168 to 170 in 442f07c
Perhaps an easy 1-line fix. IOU I just made issue. Fixing it in this same PR seems fine.
There was a problem hiding this comment.
Please add a test that this works.
Ah, now that I understand #10172, I get that by "works", you mean that it saves the installation to promise space. That had never been part of my mental model of what publishRef(install() did, and so most of the time, I manually populated produce.instance.contract.
Now that I get what it's supposed to be doing, I'll ensure that the key matches the one used for the governance installation.
There was a problem hiding this comment.
commit 7b9e75f uses restoreRef() to replace a bunch of manual fiddling with bundles and install() calls. The line in coreProposalBehavior.js is part of the solution.
|
Also, please include an Upgrade Considerations section in the PR description that goes into the merge commit. @ivanlei recently asked for a list of all the vats that we're upgrading in u17. When u18 or whatever comes around, I want to be able to show that we thought about that list ahead of time. Thanks for adding the contract-upgrade label; that will make this easier to find. |
| contractGovernorRef: publishRef( | ||
| install( | ||
| '@agoric/governance/src/contractGovernor.js', | ||
| '../bundles/bundle-contractGovernor.js', | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Looks like we're installing the governor from two places. So we'll get 2 installation handles.
Does it matter whether the governor for VaultFactory has an identical installation handle to the one from the auctioneer instance? Does it matter that only one of them can be checked against agoricNames.installation to see whether it's the "right" one?
There was a problem hiding this comment.
We discussed the possibility of only using this mechanism in 1 place and waiting for it in the other. (take care to not just wait for consume.installation.contractGovernor, since that's already resolved)
It doesn't seem that either option is catastrophic, so I'll leave it at your discretion.
There was a problem hiding this comment.
I made a separate issue: #10248. I'd like to fix it before this gets to the chain, but I think it's second priority, and there's other work to get done.
| @@ -149,7 +139,7 @@ export const upgradeVaults = async ( | |||
| }); | |||
|
|
|||
| const upgradeResult = await E(kit.adminFacet).upgradeContract( | |||
| bundleID, | |||
| VaultFactoryBundle.bundleID, | |||
There was a problem hiding this comment.
that name is kinda misleading.
I went "whoa! we're putting the whole text of the vault factory contract in there??!?" but then I realized no, it's just a small data structure with a .bundleID. The relevant type is ManifestBundleRef. A name based on that might be nice: vaultsBundleRef.
Chris-Hibbert
force-pushed
the
9584-upgradePriceFeeds
branch
from
bf1bd0c to
eb941d4
Compare
Rather than holding the paramManager retriever, paramGovernance holds an accessor that asks the contract for the paramManager each time it needs it.
priceFeeds don't have governed params, so they don't need the upgrade of their governor, and they are started by startGovernedUpgradeable(), which grabs the old governor installation before these coreEvals can change it.
Chris-Hibbert
force-pushed
the
9982-gov-upgrade-paramMgr
branch
from
7b9e75f to
25d3578
Compare
closes: #10248 ## Description #10163 (priceFeed coreEval) installed the contractGovernor twice, unnecessarily. This installs it once, and passes the reference from the auction proposal to the vaultFactory proposal. ### Security Considerations Repairs a minor issue with legibility of upgraded contracts. ### Scaling Considerations N/A ### Documentation Considerations N/A ### Testing Considerations priceFeed update tests continue to pass. ### Upgrade Considerations This will be included with the priceFeed coreEval, which is expected to be added to upgrade 18.
closes: #9982
closes: #10172
Description
When a governed contract is upgraded, its paramManager (which was ephemeral) is replaced, and the contractGovernor needs to get a fresh copy.
Security Considerations
This bug caused us to need to cut RC2 when preparing the vaultFactory release. That time, the fix was to upgrade the contractGovernor when upgrading a governed contract.
Scaling Considerations
No scaling impacts.
Testing Considerations
added a new test, which fails without the fix.
Upgrade Considerations
This is staged on top of #10074, and the goal is to include it with the priceFeed coreEval. Recent commits have updated the coreEval to include upgrading and installing the contractGovernor code.