Skip to content
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.

Sign up for GitHub

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

chore: update installations when restarting or upgrading #10999

Merged
merged 7 commits into from
Feb 13, 2025
Merged

chore: update installations when restarting or upgrading #10999

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5666c30
chore: update installations when restarting or upgrading
Chris-Hibbert Feb 12, 2025
eca2d40
chore: correct contract names for updated installations
Chris-Hibbert Feb 13, 2025
63f4706
chore: drop update of installation for non-contracts
Chris-Hibbert Feb 13, 2025
83a6001
chore: revert chagne for registry; use the correct name for reserve
Chris-Hibbert Feb 13, 2025
8372897
refactor: get installation names from aoricNamesReserved
Chris-Hibbert Feb 13, 2025
2d369bc
chore: give up on enforcing correct installation names
Chris-Hibbert Feb 13, 2025
d1241a6
Merge branch 'master' into cth-recordInstallations
mergify[bot] Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/builders/scripts/inter-protocol/replace-feeDistributor-combo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const { keys } = Object;
const knownVariants = keys(configurations);

/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').CoreEvalBuilder} */
export const defaultProposalBuilder = async (_, opts) => {
export const defaultProposalBuilder = async ({ publishRef, install }, opts) => {
const config = opts.config || configurations[opts.variant];
console.log('feeDist OPTS', opts, config);
if (!config) {
Expand All @@ -58,7 +58,15 @@ export const defaultProposalBuilder = async (_, opts) => {
return harden({
sourceSpec:
'@agoric/inter-protocol/src/proposals/replace-fee-distributor.js',
getManifestCall: [getManifestForReplaceFeeDistributor.name, { ...params }],
getManifestCall: [
getManifestForReplaceFeeDistributor.name,
{
feeDistributorRef: publishRef(
install('@agoric/inter-protocol/src/feeDistributor.js'),
),
...params,
},
],
});
};

Expand Down
12 changes: 10 additions & 2 deletions packages/builders/scripts/inter-protocol/replace-feeDistributor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@ import { getManifestForReplaceFeeDistributor } from '@agoric/inter-protocol/src/
import { SECONDS_PER_HOUR } from '@agoric/inter-protocol/src/proposals/econ-behaviors.js';

/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').CoreEvalBuilder} */
export const defaultProposalBuilder = async (_, opts) => {
export const defaultProposalBuilder = async ({ publishRef, install }, opts) => {
console.log('feeDist OPTS', opts);
return harden({
sourceSpec:
'@agoric/inter-protocol/src/proposals/replace-fee-distributor.js',
getManifestCall: [getManifestForReplaceFeeDistributor.name, { ...opts }],
getManifestCall: [
getManifestForReplaceFeeDistributor.name,
{
feeDistributorRef: publishRef(
install('@agoric/inter-protocol/src/feeDistributor.js'),
),
...opts,
},
],
});
};

Expand Down
7 changes: 4 additions & 3 deletions packages/deploy-script-support/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,10 @@ are theirselves (recursive) permits. See `BootstrapManifiest` in
[lib-boot.js](../vats/src/core/lib-boot.js).

The manifest object returned from a "getManifestCall" invocation may also
include "installations" to register in `agoricNames` and/or "options" to be
provided as the "options" property of the second argument for each call of the
manifest's functions:
include "installations" (they'll be registered with `agoricNames` and in the
bootstrap promise space) and/or "options" (they'll be provided as the "options"
property of the second argument for each call of the manifest's functions):

```js
/** @type {import('@agoric/vats/src/core/lib-boot').BootstrapManifest} */
const gameManifest = harden({
Expand Down
7 changes: 5 additions & 2 deletions packages/inter-protocol/src/proposals/replace-fee-distributor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ harden(replaceFeeDistributor);

const t = 'replaceFeeDistributor';
export const getManifestForReplaceFeeDistributor = async (
_,
feeDistributorOptions,
{ restoreRef },
{ feeDistributorRef, ...feeDistributorOptions },
) => ({
manifest: {
[replaceFeeDistributor.name]: {
Expand Down Expand Up @@ -191,5 +191,8 @@ export const getManifestForReplaceFeeDistributor = async (
},
},
},
installations: {
feeDistributor: restoreRef(feeDistributorRef),
},
options: { ...feeDistributorOptions },
});
5 changes: 4 additions & 1 deletion packages/smart-wallet/src/proposals/upgrade-wallet-factory2-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const upgradeWalletFactory = async (
console.log(`Successfully upgraded WalletFactory`);
};

export const getManifestForUpgradeWallet = (_powers, { walletRef }) => ({
export const getManifestForUpgradeWallet = ({ restoreRef }, { walletRef }) => ({
manifest: {
[upgradeWalletFactory.name]: {
consume: {
Expand All @@ -56,5 +56,8 @@ export const getManifestForUpgradeWallet = (_powers, { walletRef }) => ({
},
},
},
installations: {
walletFactory: restoreRef(walletRef),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create a new installation handle, which is a little tricky...

This upgrade-wallet-factory2-proposal.js is designed to upgrade the wallet factory contract instance. That instance's installation handle will still be the installation handle that it was started with. I'm not sure we want to make that installation handle harder to find by removing it from agoricNames. It's not like we plan to start more smartWallet contract instances.

I suppose we can get the original handle back using E(Zoe).getInstallationForInstance(instance), so maybe there's not much harm here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create a new installation handle, which is a little tricky...

I think that's what I intend. Without this change, the installation registered (in AoricNames and bootstrap space) would remain the old one, and if someone used that to start a new instance, they'd get the old one. I think each contract name in those tables should lead to the most recent installation, i.e. the one you would want to use.

Admittedly this doesn't make as much sense for singleton vats like walletFactory, but if I wanted to do a null-upgrade, this is the installation I'd want to use.

This upgrade-wallet-factory2-proposal.js is designed to upgrade the wallet factory contract instance. That instance's installation handle will still be the installation handle that it was started with.

That seems like a bug to me. I think @erights and I talked about this at some point and agreed that E(Zoe).getInstallationForInstance(instance) should return info about the current installation. It is currently never updated. Perhaps it should return an ordered list of installations, but the current one should be there, as that's the one that someone might want to verify.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I wanted to do a null-upgrade, this is the installation I'd want to use.

upgrade doesn't use installations. it uses bundle hashes.
I suppose you can get a bundle hash from an installation, so I suppose that argues for updating the installation

options: { walletRef },
});
5 changes: 4 additions & 1 deletion packages/vats/src/proposals/upgrade-agoricNames-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const upgradeAgoricNames = async (
};

export const getManifestForUpgradingAgoricNames = (
_powers,
{ restoreRef },
{ agoricNamesRef },
) => ({
manifest: {
Expand All @@ -43,5 +43,8 @@ export const getManifestForUpgradingAgoricNames = (
produce: {},
},
},
installations: {
walletFactory: restoreRef(agoricNamesRef),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

walletFactory? I suppose that's a copy-and-paste-o

but more to the point: agoricNames is not a contract vat. There's no relevant installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

options: { agoricNamesRef },
});
5 changes: 4 additions & 1 deletion packages/vats/src/proposals/upgrade-asset-reserve-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const upgradeAssetReserve = async (
};

export const getManifestForUpgradingAssetReserve = (
_powers,
{ restoreRef },
{ assetReserveRef },
) => ({
manifest: {
Expand All @@ -89,5 +89,8 @@ export const getManifestForUpgradingAssetReserve = (
},
},
},
installations: {
walletFactory: restoreRef(assetReserveRef),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
walletFactory: restoreRef(assetReserveRef),
reserve: restoreRef(assetReserveRef),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

},
options: { assetReserveRef },
});
5 changes: 4 additions & 1 deletion packages/vats/src/proposals/upgrade-bank-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const upgradeBank = async (
await E(adminNode).upgrade(bankBundleCap, {});
};

export const getManifestForUpgradingBank = (_powers, { bankRef }) => ({
export const getManifestForUpgradingBank = ({ restoreRef }, { bankRef }) => ({
manifest: {
[upgradeBank.name]: {
consume: {
Expand All @@ -37,6 +37,9 @@ export const getManifestForUpgradingBank = (_powers, { bankRef }) => ({
},
produce: {},
},
installations: {
walletFactory: restoreRef(bankRef),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a contract vat. there are no relevant installations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

},
options: { bankRef },
});
5 changes: 4 additions & 1 deletion packages/vats/src/proposals/upgrade-mintHolder-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export const upgradeMintHolder = async (
};

export const getManifestForUpgradingMintHolder = (
_powers,
{ restoreRef },
{ contractRef, labelList },
) => ({
manifest: {
Expand All @@ -70,5 +70,8 @@ export const getManifestForUpgradingMintHolder = (
},
},
},
installations: {
mintHolder: restoreRef(contractRef),
},
options: { contractRef, labelList },
});
8 changes: 7 additions & 1 deletion packages/vats/src/proposals/upgrade-paRegistry-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ export const upgradePriceAuthorityRegistry = async (
};

const par = 'paRegistry';
export const getManifestForUpgradingRegistry = (_powers, { registryRef }) => ({
export const getManifestForUpgradingRegistry = (
{ restoreRef },
{ registryRef },
) => ({
manifest: {
[upgradePriceAuthorityRegistry.name]: {
consume: {
Expand All @@ -51,6 +54,9 @@ export const getManifestForUpgradingRegistry = (_powers, { registryRef }) => ({
brand: { consume: { [Stable.symbol]: par } },
},
},
installations: {
walletFactory: restoreRef(registryRef),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

walletFactory looks like a copy-and-paste-o

These changes don't seem to get tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected.

I don't know how to test these. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of a contrived way to test for this particular problem:

  1. run some/all of these upgrades
  2. for each entry in agoricNames.installation, get the bundleId from the installation; extract the bundle from the kernel db; get the entry module of the bundle
  3. check that the entry module makes sense compared to the key of agoricNames.installation

But it seems like a lot of work compared to the gain. Especially since the harm from this goofy code is minimal: we don't intend to start another walletFactory. z:acceptance tests all the things we do plan to do (or: it's supposed to).

},
options: {
registryRef,
},
Expand Down
5 changes: 4 additions & 1 deletion packages/vats/src/proposals/upgrade-provisionPool-proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export const upgradeProvisionPool = async (
};

export const getManifestForUpgradingProvisionPool = (
_powers,
{ restoreRef },
{ provisionPoolRef },
) => ({
manifest: {
Expand All @@ -128,5 +128,8 @@ export const getManifestForUpgradingProvisionPool = (
produce: {},
},
},
installations: {
walletFactory: restoreRef(provisionPoolRef),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

walletFactory again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

},
options: { provisionPoolRef },
});
Loading