fix(vm-config): vats/init-core.js relies on econCommittee
#11056
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
272bd32
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9e3787ab.agoric-sdk.pages.dev |
| Branch Preview URL: | https://mfig-devnet-config-steps.agoric-sdk.pages.dev |
michaelfig
force-pushed
the
mfig-devnet-config-steps
branch
from
8e33f9c to
4c363ab
Compare
michaelfig
changed the title
init-core.js relies on inter-protocolvats/init-core.js relies on econCommittee
michaelfig
force-pushed
the
mfig-devnet-config-steps
branch
from
4c363ab to
ef30b2a
Compare
michaelfig
force-pushed
the
mfig-devnet-config-steps
branch
from
ef30b2a to
272bd32
Compare
| @@ -50,10 +50,10 @@ export const makeStoreHooks = (store, log = noop) => { | |||
| return; | |||
| } | |||
| if (store.has(name)) { | |||
| console.warn('cannot save duplicate:', name); | |||
| return; | |||
| store.set(name, value); | |||
There was a problem hiding this comment.
how would this change get to mainnet?
"n/a" doesn't seem to suffice for upgrade considerations.
There was a problem hiding this comment.
How about adding a test in test-promise-space.js for this fix?
There was a problem hiding this comment.
"How do we validate this change in a testnet?" will eventually be asked. If the answer is "it's not cost-effective" then that should go in Testing Considerations.
| if (!nameToState.has(name)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
How about adding a test in test-promise-space.js for this fix too?
| @@ -223,32 +225,60 @@ export const startWalletFactory = async ( | |||
| privateArgs: { | |||
| storageNode: walletStorageNode, | |||
| walletBridgeManager, | |||
| walletReviver: E(ppFacets.creatorFacet).getWalletReviver(), | |||
| walletReviver: E(E.get(ppFacetsP).creatorFacet).getWalletReviver(), | |||
There was a problem hiding this comment.
we pass a promise in privateArgs? I wonder how that works with upgrade... Do settled promises stay settled across upgrades?
| @@ -184,7 +184,11 @@ export const startWalletFactory = async ( | |||
|
|
|||
| const marshaller = await E(board).getPublishingMarshaller(); | |||
| const poolBank = E(bankManager).getBankForAddress(poolAddr); | |||
| const ppFacets = await E(startGovernedUpgradable)({ | |||
|
|
|||
| // We cannot await the start of the provisionPool, since | |||
There was a problem hiding this comment.
We did await when we rebooted the VM in the vaults upgrade, right? Any idea what changed?
| instanceProduce.provisionPool.resolve(ppFacets.instance); | ||
|
|
||
| await Promise.all([ | ||
| E(ppFacets.creatorFacet).addRevivableAddresses(oldAddresses), |
There was a problem hiding this comment.
I would dearly like to get rid of the whole reviving business.
someday...
| if (present) { | ||
| void E.sendOnly(nameAdmin).delete(name); |
There was a problem hiding this comment.
seems worth a unit test in test-name-hub-published.js
There was a problem hiding this comment.
but again... how do we deploy this change to mainnet?
Maybe the answer is "we don't" but that merits something explicit in upgrade considerations.
| old.reject(Error(`Value has been deleted`)); | ||
| old.reject(Error(`Value for ${key} has been deleted`)); |
There was a problem hiding this comment.
Is the agoricNames vat being upgraded in u19?
Is nameHub used in any other vats? Oh yeah... provisioning holds namesByAddress. Are we upgrading that one?
closes: #11053
Description
packages/builders/scripts/vats/init-core.jshas a dependency onpackages/builders/scripts/inter-protocol/init-core.js. When they were moved to different coreProposal steps,startWalletFactory.jswasn't able to initialize correctly. This PR moves theinter-protocol/init-core.jscommitteeProposalBuilderto the same first step asvats/init-core.jsto reenable smart wallet provisioning.Security Considerations
n/a
Scaling Considerations
No additional resource consumption.
Documentation Considerations
n/a
Testing Considerations
Currently manually tested as specified in #11053 , but it would be good to test in CI if it can be done without significantly affecting CI duration.
Upgrade Considerations
n/a