10248 install contractGovernor once #10256
Conversation
Chris-Hibbert
added
Governance
Deploying agoric-sdk with
|
| Latest commit: |
e319cd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fd07db08.agoric-sdk.pages.dev |
| Branch Preview URL: | https://10248-installonce.agoric-sdk.pages.dev |
| @@ -9,40 +9,51 @@ const trace = makeTracer('NewAuction', true); | |||
| /** | |||
| * @typedef {PromiseSpaceOf<{ | |||
| * auctionUpgradeNewInstance: Instance; | |||
| * newContractGovBundleId: string; | |||
There was a problem hiding this comment.
hm. "new" with respect to what? Are we certain we'll never do this again? If not, what would we call it next time?
There was a problem hiding this comment.
My theory is that since I reset() it at the end, it doesn't stick around to clutter the name space. We can call it "new" again next time or something more specific.
I had "priceFeed" in the name originally, but when I decided it would be better to reset() when complete, I went for a shorter name that I thought was clearer.
| @@ -9,40 +9,51 @@ const trace = makeTracer('NewAuction', true); | |||
| /** | |||
| * @typedef {PromiseSpaceOf<{ | |||
| * auctionUpgradeNewInstance: Instance; | |||
| * newContractGovBundleId: string; | |||
There was a problem hiding this comment.
And this promise space key is shared across the whole bootstrap vat, right?
At some point, it belongs in ChainBootstrapSpaceT. I wonder when we should do that. hm.
There was a problem hiding this comment.
Only if it sticks around, as far as I'm concerned.
| electorateCreatorFacet, | ||
| newContractGovBundleIdP, | ||
| ]); | ||
| newContractGovBundleIdErasor.reset(); |
There was a problem hiding this comment.
IIRC, a reset() like this made something more difficult last time. Testing or something. Here's hoping I remember more detail.
There was a problem hiding this comment.
I think there was 3-way communication that time, and it was conceivable that the second reader would reset before the first reader had read. There are only two parties this time.
| @@ -179,6 +190,7 @@ export const addAuction = async ({ | |||
| ); | |||
|
|
|||
| auctionUpgradeNewInstance.resolve(governedInstance); | |||
| newContractGovBundleId.resolve(contractGovernorBundle.bundleID); | |||
There was a problem hiding this comment.
correctness problem
When we make the governor for the auctioneer that we're adding, we don't seem to use an installation based on this ID.
Is there a way to test that we got that part right?
There was a problem hiding this comment.
When we make the governor for the auctioneer that we're adding, we don't seem to use an installation based on this ID.
startInstance() wants an Installation, while upgradeContract() wants a BundleId. The cleanest way of tying those together seems to be in getManifestForAddAuction().
That takes the bundleRef (contractGovernorRef) from the proposalBuilder script and passes both it and the installation produced from it (installations: { contractGovernor: restoreRef(contractGovernorRef) }) to the auction proposal. The addAuction proposal uses the installation to start the new auction, and passes the bundleId through promise space to the upgrade-vaults proposal.
I could verify that both governors have the same bundle, but I don't see how the Installation and bundleId could become dissociated.
Chris-Hibbert
added
automerge:squash
Chris-Hibbert
added
the
automerge:rebase
Pass just the bundleId through from the first proposal to the second, and then reset() the value. Also cleaned up some leftover permissions
Chris-Hibbert
force-pushed
the
10248-installOnce
branch
from
b6efbfe to
e319cd1
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.