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 3 commits
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-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: {
assetReserve: 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.

Again, I suggest reserve, not assetReserve.

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

We can see this in published.agoricNames.installation on mainnet (or any of our running networks, I'm pretty sure).

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to import agoricNamesReserved from @agoric/vats/src/core/utils.js to verify that I'm using actual installation names.

},
options: { assetReserveRef },
});
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: {
priceAuthority: 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.

Suggested change
priceAuthority: restoreRef(registryRef),
priceAggregator: 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.

wait... now I'm getting things mixed up.

priceAuthorityRegistry is not a contract vat.

https://github.com/Agoric/agoric-sdk/blob/a92cf556a0c806df293027458883f2a81a2bfade/packages/vats/src/vat-priceAuthority.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped. I hate the names for priceAuthority/Registry/fluxAggregator!

},
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: {
provisionPool: restoreRef(provisionPoolRef),
},
options: { provisionPoolRef },
});
Loading