chore: update installations when restarting or upgrading #10999

Chris-Hibbert · 2025-02-12T23:37:42Z

Description

I've known for a while that there was something magic about the installations keyword in getManifestForFoo in coreEval proposals, but I was never able to track it down. Now I've found the relevant code. In coreProposalBehavior (which is copied into generated proposals), if the manifest contains named installations, then they are added to both agoricNames and the bootstrap promiseSpace. When upgrading a contract to a new installation, the installation should nearly always be updated this way.

This PR updates all the proposals/scripts currently slated for upgrade-19 to include their upgraded bundles in installations so future references will automatically pick them up.

Security Considerations

related to security and reliability of the contracts, not the chain.

Scaling Considerations

not relevant.

Documentation Considerations

I updated the README in deploy-script-support.

Testing Considerations

The n:upgrade-next and z:acceptance tests continue to pass.

Upgrade Considerations

PSM is the only contract being upgraded in Upgrade19 that starts up multiple copies, and it was already doing this correctly.

dckc

Making mintHolder work like PSM seems good.

There are various problems with the rest.

dckc · 2025-02-13T00:13:58Z

packages/smart-wallet/src/proposals/upgrade-wallet-factory2-proposal.js

+  installations: {
+    walletFactory: restoreRef(walletRef),
+  },


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.

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.

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

dckc · 2025-02-13T00:15:06Z

packages/vats/src/proposals/upgrade-agoricNames-proposal.js

+  installations: {
+    walletFactory: restoreRef(agoricNamesRef),
+  },


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.

dckc · 2025-02-13T00:15:58Z

packages/vats/src/proposals/upgrade-asset-reserve-proposal.js

@@ -89,5 +89,8 @@ export const getManifestForUpgradingAssetReserve = (
      },
    },
  },
+  installations: {
+    walletFactory: restoreRef(assetReserveRef),


Suggested change

walletFactory: restoreRef(assetReserveRef),

reserve: restoreRef(assetReserveRef),

dckc · 2025-02-13T00:16:30Z

packages/vats/src/proposals/upgrade-bank-proposal.js

+    installations: {
+      walletFactory: restoreRef(bankRef),
+    },


not a contract vat. there are no relevant installations

dckc · 2025-02-13T00:17:30Z

packages/vats/src/proposals/upgrade-paRegistry-proposal.js

@@ -51,6 +54,9 @@ export const getManifestForUpgradingRegistry = (_powers, { registryRef }) => ({
      brand: { consume: { [Stable.symbol]: par } },
    },
  },
+  installations: {
+    walletFactory: restoreRef(registryRef),


walletFactory looks like a copy-and-paste-o

These changes don't seem to get tested.

corrected.

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

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

run some/all of these upgrades

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

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).

dckc · 2025-02-13T00:17:48Z

packages/vats/src/proposals/upgrade-provisionPool-proposal.js

@@ -128,5 +128,8 @@ export const getManifestForUpgradingProvisionPool = (
      produce: {},
    },
  },
+  installations: {
+    walletFactory: restoreRef(provisionPoolRef),


walletFactory again

cloudflare-workers-and-pages · 2025-02-13T01:04:23Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`d1241a6`
Status:	✅ Deploy successful!
Preview URL:	https://28628f55.agoric-sdk.pages.dev
Branch Preview URL:	https://cth-recordinstallations.agoric-sdk.pages.dev

View logs

dckc · 2025-02-13T04:59:13Z

packages/vats/src/proposals/upgrade-asset-reserve-proposal.js

@@ -89,5 +89,8 @@ export const getManifestForUpgradingAssetReserve = (
      },
    },
  },
+  installations: {
+    assetReserve: restoreRef(assetReserveRef),


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).

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

dckc · 2025-02-13T04:59:54Z

packages/vats/src/proposals/upgrade-paRegistry-proposal.js

@@ -51,6 +54,9 @@ export const getManifestForUpgradingRegistry = (_powers, { registryRef }) => ({
      brand: { consume: { [Stable.symbol]: par } },
    },
  },
+  installations: {
+    priceAuthority: restoreRef(registryRef),


Suggested change

priceAuthority: restoreRef(registryRef),

priceAggregator: restoreRef(registryRef),

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

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

dckc · 2025-02-13T18:50:10Z

packages/smart-wallet/src/proposals/upgrade-wallet-factory2-proposal.js

@@ -56,5 +57,8 @@ export const getManifestForUpgradeWallet = (_powers, { walletRef }) => ({
      },
    },
  },
+  installations: {
+    [agoricNamesReserved.installation.walletFactory]: restoreRef(walletRef),


interesting idea, but that expression evaluates to 'multitenant smart wallet'

agoric-sdk/packages/vats/src/core/utils.js

Line 44 in 729fe8e

walletFactory: 'multitenant smart wallet',

dckc · 2025-02-13T18:53:22Z

packages/smart-wallet/src/proposals/upgrade-wallet-factory2-proposal.js

+  installations: {
+    [agoricNamesReserved.installation.walletFactory]: restoreRef(walletRef),


maybe...

Suggested change

installations: {

[agoricNamesReserved.installation.walletFactory]: restoreRef(walletRef),

/** @satisfies {Record<keyof typeof agoricNamesReserved.installation, unknown>} */

installations: {

'walletFactory': restoreRef(walletRef),

It looks good, and I'd prefer it to no check, but it didn't enforce correct names.

dckc · 2025-02-13T18:53:35Z

packages/vats/src/proposals/upgrade-asset-reserve-proposal.js

@@ -89,5 +90,8 @@ export const getManifestForUpgradingAssetReserve = (
      },
    },
  },
+  installations: {
+    [agoricNamesReserved.installation.reserve]: restoreRef(assetReserveRef),


same problem

likewise the 2 below

dckc

I'm still uneasy that so many ways to get it sort of wrong are not detected by tests.
But I can't think of any cost-effective ways to improve the situation.

Chris-Hibbert · 2025-02-13T21:13:46Z

I'm still uneasy that so many ways to get it sort of wrong are not detected by tests.
But I can't think of any cost-effective ways to improve the situation.

I agree. I thought of trying to impose your suggestion of keyof typeof agoricNamesReserved.installation at the manifest level, but I couldn't see a place to do that. If we had a type declaration on the value returned by getManifestForFoo, it might be worth the payoff to add that type in all the proposals.

chore: update installations when restarting or upgrading

5666c30

Chris-Hibbert self-assigned this Feb 12, 2025

Chris-Hibbert requested a review from a team as a code owner February 12, 2025 23:37

Chris-Hibbert requested a review from AgoricTriage February 12, 2025 23:37

Chris-Hibbert added deployment Chain deployment mechanism (e.g. testnet) Zoe Contract Contracts within Zoe contract-upgrade labels Feb 12, 2025

Chris-Hibbert requested a review from dckc February 12, 2025 23:58

dckc requested changes Feb 13, 2025

View reviewed changes

Chris-Hibbert added 2 commits February 12, 2025 17:03

chore: correct contract names for updated installations

eca2d40

chore: drop update of installation for non-contracts

63f4706

dckc reviewed Feb 13, 2025

View reviewed changes

Chris-Hibbert added 2 commits February 13, 2025 09:43

chore: revert chagne for registry; use the correct name for reserve

83a6001

refactor: get installation names from aoricNamesReserved

8372897

Chris-Hibbert requested a review from dckc February 13, 2025 18:47

dckc reviewed Feb 13, 2025

View reviewed changes

chore: give up on enforcing correct installation names

2d369bc

Chris-Hibbert requested a review from dckc February 13, 2025 19:56

dckc approved these changes Feb 13, 2025

View reviewed changes

Chris-Hibbert added the automerge:squash Automatically squash merge label Feb 13, 2025

Merge branch 'master' into cth-recordInstallations

d1241a6

mergify bot merged commit 9db2909 into master Feb 13, 2025
83 checks passed

mergify bot deleted the cth-recordInstallations branch February 13, 2025 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update installations when restarting or upgrading #10999

chore: update installations when restarting or upgrading #10999

Chris-Hibbert commented Feb 12, 2025

dckc left a comment

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

cloudflare-workers-and-pages bot commented Feb 13, 2025 •

edited

Loading

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025

dckc Feb 13, 2025

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025 •

edited

Loading

dckc Feb 13, 2025 •

edited

Loading

Chris-Hibbert Feb 13, 2025

dckc Feb 13, 2025 •

edited

Loading

dckc left a comment

Chris-Hibbert commented Feb 13, 2025

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

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

	priceAuthority: restoreRef(registryRef),
	priceAggregator: restoreRef(registryRef),

		installations: {
		[agoricNamesReserved.installation.walletFactory]: restoreRef(walletRef),

chore: update installations when restarting or upgrading #10999

chore: update installations when restarting or upgrading #10999

Conversation

Chris-Hibbert commented Feb 12, 2025

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 13, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

dckc Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Feb 13, 2025

cloudflare-workers-and-pages bot commented Feb 13, 2025 •

edited

Loading

dckc Feb 13, 2025 •

edited

Loading

dckc Feb 13, 2025 •

edited

Loading

dckc Feb 13, 2025 •

edited

Loading