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

Specify a bundle to use when upgrading auctions #9937

Merged
merged 5 commits into from
Aug 21, 2024
Merged

Specify a bundle to use when upgrading auctions #9937

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6e0739
feat: specify a bundle to use when upgrading auctions
Chris-Hibbert Aug 21, 2024
37505e3
chore: use installation.produce.CONTRACT rather than agoricNames
Chris-Hibbert Aug 21, 2024
cbcc513
refactor: remove outdated dependence of vault upgrade on priceFeeds
Chris-Hibbert Aug 21, 2024
cdebfe1
fixup! refactor: remove outdated dependence of vault upgrade on price…
Chris-Hibbert Aug 21, 2024
b0df701
Merge branch 'master' into 9936-auctionBundle
mergify[bot] Aug 21, 2024
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
16 changes: 16 additions & 0 deletions a3p-integration/proposals/a:vaults-auctions/upgradeVaults.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,25 @@ const triggerAuction = async t => {
t.is(atomOut, '+5200000');
};

// contract vat names are based on bundleID
const ORIGINAL_AUCTION_VAT_NAME = 'zcf-b1-a5683-auctioneer';

const newAuctioneerFromNewBundle = details => {
for (const detail of details) {
if (
!detail.vatName.includes('governor') &&
detail.vatName !== ORIGINAL_AUCTION_VAT_NAME
) {
return true;
}
}
return false;
};

const checkAuctionVat = async t => {
const details = await getDetailsMatchingVats('auctioneer');

t.true(newAuctioneerFromNewBundle(details));
// This query matches both the auction and its governor, so double the count
t.true(Object.keys(details).length > 2);
};
Expand Down
14 changes: 12 additions & 2 deletions packages/builders/scripts/vats/add-auction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import { makeHelpers } from '@agoric/deploy-script-support';

/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').CoreEvalBuilder} */
export const defaultProposalBuilder = async () => {
export const defaultProposalBuilder = async ({ publishRef, install }) => {
return harden({
sourceSpec: '@agoric/inter-protocol/src/proposals/add-auction.js',
getManifestCall: ['getManifestForAddAuction'],
getManifestCall: [
'getManifestForAddAuction',
{
auctionsRef: publishRef(
install(
'@agoric/inter-protocol/src/auction/auctioneer.js',
'../../inter-protocol/bundles/bundle-auctioneer.js',
),
),
},
],
});
};

Expand Down
77 changes: 50 additions & 27 deletions packages/inter-protocol/src/proposals/add-auction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,35 +15,47 @@ const trace = makeTracer('NewAuction', true);
/**
* @param {import('./econ-behaviors.js').EconomyBootstrapPowers &
* interlockPowers} powers
* @param {{ options: { auctionsRef: { bundleID: string } } }} options
*/
export const addAuction = async ({
consume: {
zoe,
board,
chainTimerService,
priceAuthority,
chainStorage,
economicCommitteeCreatorFacet: electorateCreatorFacet,
auctioneerKit: legacyKitP,
},
produce: { newAuctioneerKit, auctionsUpgradeComplete },
instance: {
consume: { reserve: reserveInstance },
},
installation: {
export const addAuction = async (
{
consume: {
auctioneer: auctionInstallation,
contractGovernor: contractGovernorInstallation,
zoe,
board,
chainTimerService,
priceAuthority,
chainStorage,
economicCommitteeCreatorFacet: electorateCreatorFacet,
auctioneerKit: legacyKitP,
},
produce: { newAuctioneerKit, auctionsUpgradeComplete },
Copy link
Member

Choose a reason for hiding this comment

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

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

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

instance: {
consume: { reserve: reserveInstance },
},
installation: {
consume: { contractGovernor: contractGovernorInstallation },
produce: { auctioneer: produceInstallation },
},
issuer: {
consume: { [Stable.symbol]: stableIssuerP },
},
},
issuer: {
consume: { [Stable.symbol]: stableIssuerP },
},
}) => {
trace('addAuction start');
{ options },
) => {
trace('addAuction start', options);
const STORAGE_PATH = 'auction';
const { auctionsRef } = options;

const poserInvitationP = E(electorateCreatorFacet).getPoserInvitation();
const bundleID = auctionsRef.bundleID;
/**
* @type {Promise<
* Installation<import('../../src/auction/auctioneer.js')['start']>
* >}
*/
const installationP = E(zoe).installBundleID(bundleID);
produceInstallation.reset();
produceInstallation.resolve(installationP);

const [
initialPoserInvitation,
Expand Down Expand Up @@ -89,10 +101,12 @@ export const addAuction = async ({
},
);

const installation = await installationP;

const governorTerms = await deeplyFulfilledObject(
harden({
timer: chainTimerService,
governedContractInstallation: auctionInstallation,
governedContractInstallation: installation,
governed: {
terms: auctionTerms,
issuerKeywordRecord: { Bid: stableIssuer },
Expand All @@ -103,7 +117,7 @@ export const addAuction = async ({
}),
);

/** @type {GovernorStartedInstallationKit<typeof auctionInstallation>} */
/** @type {GovernorStartedInstallationKit<typeof installationP>} */
const governorStartResult = await E(zoe).startInstance(
contractGovernorInstallation,
undefined,
Expand Down Expand Up @@ -179,14 +193,23 @@ export const ADD_AUCTION_MANIFEST = harden({
auctioneer: true,
Copy link
Member

Choose a reason for hiding this comment

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

don't need to consume the auctioneer 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

contractGovernor: true,
},
produce: { auctioneer: true },
},
issuer: {
consume: { [Stable.symbol]: true },
},
},
});

/* Add a new auction to a chain that already has one. */
export const getManifestForAddAuction = async () => {
return { manifest: ADD_AUCTION_MANIFEST };
/**
* Add a new auction to a chain that already has one.
*
* @param {object} _ign
* @param {any} addAuctionOptions
*/
export const getManifestForAddAuction = async (_ign, addAuctionOptions) => {
return {
manifest: ADD_AUCTION_MANIFEST,
options: addAuctionOptions,
};
};
34 changes: 14 additions & 20 deletions packages/inter-protocol/src/proposals/upgrade-vaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ const any = promises =>
* interlockPowers} powers
* @param {{ options: { vaultsRef: { bundleID: string } } }} options
*/
export const upgradeVaults = async (powers, { options }) => {
const {
export const upgradeVaults = async (
{
consume: {
agoricNamesAdmin,
newAuctioneerKit: auctioneerKitP,
Expand All @@ -58,10 +58,15 @@ export const upgradeVaults = async (powers, { options }) => {
newAuctioneerKit: tempAuctioneerKit,
auctionsUpgradeComplete: auctionsUpgradeCompleteProducer,
},
installation: {
produce: { VaultFactory: produceVaultInstallation },
},
instance: {
produce: { auctioneer: auctioneerProducer },
},
} = powers;
},
{ options },
) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const { vaultsRef } = options;
const kit = await vaultFactoryKit;
const auctioneerKit = await auctioneerKitP;
Expand All @@ -76,27 +81,13 @@ export const upgradeVaults = async (powers, { options }) => {
* Installation<import('../../src/vaultFactory/vaultFactory.js')['start']>
* >}
*/
let installationP;
const installationP = E(zoe).installBundleID(bundleID);
produceVaultInstallation.reset();
produceVaultInstallation.resolve(installationP);
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

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

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


await auctionsUpgradeComplete;
auctionsUpgradeCompleteProducer.reset();
Copy link
Member

Choose a reason for hiding this comment

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

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.

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


if (vaultsRef) {
if (bundleID) {
installationP = E(zoe).installBundleID(bundleID);
await E.when(
installationP,
installation =>
E(E(agoricNamesAdmin).lookupAdmin('installation')).update(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

yes.

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

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

const readCurrentDirectorParams = async () => {
const { publicFacet: directorPF } = kit;

Expand Down Expand Up @@ -280,6 +271,9 @@ export const getManifestForUpgradeVaults = async (
newAuctioneerKit: uV,
auctionsUpgradeComplete: uV,
},
installation: {
produce: { VaultFactory: true },
},
instance: { produce: { auctioneer: uV, newAuctioneerKit: uV } },
},
},
Expand Down
Loading