Specify a bundle to use when upgrading auctions #9937

Chris-Hibbert · 2024-08-21T18:58:12Z

closes: #9936

Description

Upload new auction code for the coreEval.

Security Considerations

N/A

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

Added a test that the new vat has a different bundle ID. I couldn't find any accessible behavioral differences to test.

Upgrade Considerations

verify that upgrade happened.

cloudflare-workers-and-pages · 2024-08-21T18:59:12Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`b0df701`
Status:	✅ Deploy successful!
Preview URL:	https://94da373a.agoric-sdk.pages.dev
Branch Preview URL:	https://9936-auctionbundle.agoric-sdk.pages.dev

View logs

dckc · 2024-08-21T19:35:20Z

packages/inter-protocol/src/proposals/add-auction.js

+  await E.when(
+    installationP,
+    installation =>
+      E(E(agoricNamesAdmin).lookupAdmin('installation')).update(
+        'auctioneer',
+        installation,
+      ),
+    err => console.error(`🚨 failed to update vaultFactory installation`, err),
+  );


Using the promise space is supposed to take care of agoricNames for you:

Suggested change

await E.when(

installationP,

installation =>

E(E(agoricNamesAdmin).lookupAdmin('installation')).update(

'auctioneer',

installation,

),

err => console.error(`🚨 failed to update vaultFactory installation`, err),

);

produceInstallation.reset();

produceInstallation.resolve(installationP);

combined with:

installation: { produce: { auction: produceInstallation }, },

I'm not 100% sure these are equivalent. thinking it over.

Reviewing the code for makePromiseSpaceForNameHub, I see that

using the promise space updates agoricNames as a side-effect, so they stay in sync, but

updating agoricNames directly does _not _ update the promise space, so if someone later tries to use installation.consume.auctioneer, they'll get the old installation

I will fix the code in upgradeVaults as well.

This turned into a major clean-up.

@dckc this means upgrade-vaults.js needs a fresh review.

dckc · 2024-08-21T19:37:55Z

packages/inter-protocol/src/proposals/upgrade-vaults.js

-  } = powers;
+  },
+  { options },
+) => {


I'm struggling to understand this change. Is it just formatting?

It's mostly formatting. The old code had access to the name powers within the body. That was used in some proposals in order to pass all the powers to sub-proposals. That isn't happening here, so I reverted to the more conventional (for proposals) format of expanding powers in the parameter list, rather than passing it through and expanding it in the body.

It was just (bad?) luck that the differ matches the expansion of powers in the param list with the identical one in the body.

I would have accepted a suggestion that it was unrelated to this PR, but now that I'm changing the expansion of powers in order to get installation.produce.VaultFactory, I'll leave it in as cleanup related to that.

dckc

I think it's worth keeping the installation promise space in sync with agoricNames

dckc · 2024-08-21T19:43:56Z

a3p-integration/proposals/a:vaults-auctions/upgradeVaults.test.js

+function newAuctioneerFromNewBundle(details) {
+  for (const detail of details) {
+    // contract vat names are based on bundleID
+    const originalAuctionVatName = 'zcf-b1-a5683-auctioneer';


seems like this should be hoisted out of the loop.

not critical

good point.

dckc · 2024-08-21T20:52:23Z

packages/inter-protocol/src/proposals/upgrade-vaults.js

-      await E.when(
-        installationP,
-        installation =>
-          E(E(agoricNamesAdmin).lookupAdmin('installation')).update(


I would expect agoricNamesAdmin to disappear from the permit of the auction part. I wonder why I don't see it.

upgradeVaults.js still uses it for the auction instance at line 226. Should that use promise space instead?

add-auction.js added it in b6e0739, but dropped it in 37505e3.

upgradeVaults.js still uses it for the auction instance at line 226. Should that use promise space instead?

yes.

dckc · 2024-08-21T21:24:02Z

packages/inter-protocol/src/proposals/add-auction.js

+      economicCommitteeCreatorFacet: electorateCreatorFacet,
+      auctioneerKit: legacyKitP,
+    },
+    produce: { newAuctioneerKit, auctionsUpgradeComplete },


in discussion, we agreed newAuctioneerKit should go away in favor of the auctionsUpgradeComplete sync point and replacing auctioneerKit ( with reset, resolve ).

dckc · 2024-08-21T21:25:38Z

packages/inter-protocol/src/proposals/add-auction.js

@@ -179,14 +193,23 @@ export const ADD_AUCTION_MANIFEST = harden({
        auctioneer: true,


don't need to consume the auctioneer installation

…Feeds

dckc

looks good.

dckc · 2024-08-21T22:48:42Z

packages/inter-protocol/src/proposals/upgrade-vaults.js

+  const installationP = E(zoe).installBundleID(bundleID);
+  produceVaultInstallation.reset();
+  produceVaultInstallation.resolve(installationP);


The publishRef / install stuff in builders/scripts/*.js is supposed to take care of this installBundleId and reset / resolve.

But the only cost to doing it this way is 1 cross-vat call and a bit of allocation / GC. And there's benefit in clarity.

And it looks like the reset is missing from the other code path:

agoric-sdk/packages/deploy-script-support/src/coreProposalBehavior.js

Lines 169 to 171 in aedfac6

installationEntries.map(([key, value]) => {

produceInstallations[key].resolve(value);

return E(installAdmin).update(key, value);

dckc · 2024-08-21T22:53:10Z

packages/inter-protocol/src/proposals/upgrade-vaults.js

-  let installationP;
+  const installationP = E(zoe).installBundleID(bundleID);
+  produceVaultInstallation.reset();
+  produceVaultInstallation.resolve(installationP);

  await auctionsUpgradeComplete;
  auctionsUpgradeCompleteProducer.reset();


It's a little surprising that the vaults upgrade is producing auctionsUpgradeComplete. Now I remember: this is why. A bit more docs might be nice.

I think your earlier suggestion of a naming convention is good.

My theory here is that names like fooProcessCompletion should be considered to be single-use flags and should be cleaned up once consumed.

refs: #9940 ## Description #9937 unintentionally upgraded the vaultFactory with the instance of the old auction. This change asks the promise space for the auction instance after the new auction startup is complete. ### Security Considerations None. ### Scaling Considerations None. ### Documentation Considerations None. ###Testing Considerations This will be tested on Ollinet and emerynet before deployment. I tested locally on a3p by watching the logs to see that the vaults synced to the new auction schedule. ###Upgrade Considerations upgrade vaults and auctions so they work together.

closes: #9936 ## Description Upload new auction code for the coreEval. ### Security Considerations N/A ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations Added a test that the new vat has a different bundle ID. I couldn't find any accessible behavioral differences to test. ### Upgrade Considerations verify that upgrade happened.

feat: specify a bundle to use when upgrading auctions

b6e0739

Chris-Hibbert added contract-upgrade auction next-release about next agoric-sdk or endo release labels Aug 21, 2024

Chris-Hibbert requested a review from dckc August 21, 2024 18:58

Chris-Hibbert self-assigned this Aug 21, 2024

Chris-Hibbert added the force:integration Force integration tests to run on PR label Aug 21, 2024

dckc reviewed Aug 21, 2024

View reviewed changes

dckc requested changes Aug 21, 2024

View reviewed changes

chore: use installation.produce.CONTRACT rather than agoricNames

37505e3

dckc reviewed Aug 21, 2024

View reviewed changes

refactor: remove outdated dependence of vault upgrade on priceFeeds

cbcc513

Chris-Hibbert requested a review from dckc August 21, 2024 22:31

fixup! refactor: remove outdated dependence of vault upgrade on price…

cdebfe1

…Feeds

dckc approved these changes Aug 21, 2024

View reviewed changes

Chris-Hibbert added the automerge:squash Automatically squash merge label Aug 21, 2024

Merge branch 'master' into 9936-auctionBundle

b0df701

mergify bot merged commit b6379da into master Aug 21, 2024
80 checks passed

mergify bot deleted the 9936-auctionBundle branch August 21, 2024 23:32

This was referenced Aug 22, 2024

get auction instance from promise space after auction startup completes #9940

Closed

get auction instance from promise returned by addAuction #9944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify a bundle to use when upgrading auctions #9937

Specify a bundle to use when upgrading auctions #9937

Chris-Hibbert commented Aug 21, 2024

cloudflare-workers-and-pages bot commented Aug 21, 2024 •

edited

Loading

dckc Aug 21, 2024 •

edited

Loading

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc left a comment

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

dckc left a comment

dckc Aug 21, 2024

dckc Aug 21, 2024

Chris-Hibbert Aug 21, 2024

		@@ -179,14 +193,23 @@ export const ADD_AUCTION_MANIFEST = harden({
		auctioneer: true,

	installationEntries.map(([key, value]) => {
	produceInstallations[key].resolve(value);
	return E(installAdmin).update(key, value);

Specify a bundle to use when upgrading auctions #9937

Specify a bundle to use when upgrading auctions #9937

Conversation

Chris-Hibbert commented Aug 21, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Aug 21, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc Aug 21, 2024 • edited Loading

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

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

cloudflare-workers-and-pages bot commented Aug 21, 2024 •

edited

Loading

dckc Aug 21, 2024 •

edited

Loading