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.

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

fix: contractGovernor can change params after contract is upgraded #10163

Merged
merged 5 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions packages/boot/test/bootstrapTests/price-feed-replace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ test.serial('setupVaults; run updatePriceFeeds proposals', async t => {
const SOME_GUI = 'someGUIHASH';
await updateVaultDirectorParams(t, gd, SOME_GUI);

const { EV } = t.context.runUtils;
const agoricNames = await EV.vat('bootstrap').consumeItem('agoricNames');
const oldVaultInstallation = await EV(agoricNames).lookup(
'installation',
'VaultFactory',
);

t.log('building all relevant CoreEvals');
const coreEvals = await Promise.all([
buildProposal(priceFeedBuilder, ['main']),
Expand All @@ -99,6 +106,16 @@ test.serial('setupVaults; run updatePriceFeeds proposals', async t => {
t.not(instancePre, instancePost);

await priceFeedDrivers[collateralBrandKey].refreshInvitations();

const newVaultInstallation = await EV(agoricNames).lookup(
'installation',
'VaultFactory',
);

t.notDeepEqual(
newVaultInstallation.getKref(),
oldVaultInstallation.getKref(),
);
});

test.serial('1. place bid', async t => {
Expand Down
132 changes: 132 additions & 0 deletions packages/boot/test/bootstrapTests/updateUpgradedVaultParams.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/**
* @file The goal of this test is to show that #9982 is fixed
* We change a parameter so that provideParamGovernance() is called once, and
* paramGoverance has been set. Then upgrade vaultFactory, so any ephemeral
* objects from the contract held by the governor are gone, then try to change
* param again, to show that the bug is fixedd.
*/
import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';

import type { TestFn } from 'ava';
import { makeAgoricNamesRemotesFromFakeStorage } from '@agoric/vats/tools/board-utils';
import { Fail } from '@endo/errors';

import { makeSwingsetTestKit } from '../../tools/supports.js';
import {
makeGovernanceDriver,
makeWalletFactoryDriver,
} from '../../tools/drivers.js';
import { updateVaultManagerParams } from '../tools/changeVaultParams.js';

const makeDefaultTestContext = async t => {
console.time('DefaultTestContext');
const swingsetTestKit = await makeSwingsetTestKit(t.log);

const { runUtils, storage } = swingsetTestKit;
console.timeLog('DefaultTestContext', 'swingsetTestKit');
const { EV } = runUtils;

// Wait for ATOM to make it into agoricNames
await EV.vat('bootstrap').consumeItem('vaultFactoryKit');
console.timeLog('DefaultTestContext', 'vaultFactoryKit');

// has to be late enough for agoricNames data to have been published
const agoricNamesRemotes = makeAgoricNamesRemotesFromFakeStorage(
swingsetTestKit.storage,
);
agoricNamesRemotes.brand.ATOM || Fail`ATOM missing from agoricNames`;
console.timeLog('DefaultTestContext', 'agoricNamesRemotes');

const walletFactoryDriver = await makeWalletFactoryDriver(
runUtils,
storage,
agoricNamesRemotes,
);
console.timeLog('DefaultTestContext', 'walletFactoryDriver');

console.timeEnd('DefaultTestContext');

const gd = await makeGovernanceDriver(
swingsetTestKit,
agoricNamesRemotes,
walletFactoryDriver,
[
'agoric1ldmtatp24qlllgxmrsjzcpe20fvlkp448zcuce',
'agoric140dmkrz2e42ergjj7gyvejhzmjzurvqeq82ang',
'agoric1w8wktaur4zf8qmmtn3n7x3r0jhsjkjntcm3u6h',
],
);

return { ...swingsetTestKit, agoricNamesRemotes, gd };
};

const test = anyTest as TestFn<
Awaited<ReturnType<typeof makeDefaultTestContext>>
>;

test.before(async t => {
t.context = await makeDefaultTestContext(t);
});
test.after.always(t => {
return t.context.shutdown && t.context.shutdown();
});

const outcome = {
bids: [{ payouts: { Bid: 0, Collateral: 1.800828 } }],
};

test('restart vaultFactory, change params', async t => {
const { runUtils, gd, agoricNamesRemotes } = t.context;
const { EV } = runUtils;
const vaultFactoryKit =
await EV.vat('bootstrap').consumeItem('vaultFactoryKit');

const { ATOM } = agoricNamesRemotes.brand;
ATOM || Fail`ATOM missing from agoricNames`;

const reserveKit = await EV.vat('bootstrap').consumeItem('reserveKit');
const bootstrapVat = EV.vat('bootstrap');
const electorateCreatorFacet = await bootstrapVat.consumeItem(
'economicCommitteeCreatorFacet',
);

const poserInvitation = await EV(electorateCreatorFacet).getPoserInvitation();
const creatorFacet1 = await EV.get(reserveKit).creatorFacet;
const shortfallInvitation =
await EV(creatorFacet1).makeShortfallReportingInvitation();

const zoe: ZoeService = await EV.vat('bootstrap').consumeItem('zoe');
const brands = await EV(zoe).getBrands(vaultFactoryKit.instance);
const getDebtLimitValue = async () => {
const params = await EV(vaultFactoryKit.publicFacet).getGovernedParams({
collateralBrand: brands.ATOM,
});

// @ts-expect-error getGovernedParams doesn't declare these fields
return params.DebtLimit.value.value;
};

// Change the value of a param before the upgrade so paramGovernance is set
t.is(await getDebtLimitValue(), 1_000_000_000n);
await updateVaultManagerParams(t, gd, ATOM, 50_000_000n);

t.is(await getDebtLimitValue(), 50_000_000n);

const privateArgs = {
// @ts-expect-error cast XXX missing from type
...vaultFactoryKit.privateArgs,
initialPoserInvitation: poserInvitation,
initialShortfallInvitation: shortfallInvitation,
};

const vfAdminFacet = await EV(
vaultFactoryKit.governorCreatorFacet,
).getAdminFacet();

t.log('awaiting VaultFactory restartContract');
const upgradeResult = await EV(vfAdminFacet).restartContract(privateArgs);
t.deepEqual(upgradeResult, { incarnationNumber: 1 });

await updateVaultManagerParams(t, gd, ATOM, 150_000_000n);
t.is(await getDebtLimitValue(), 150_000_000n);
});
14 changes: 13 additions & 1 deletion packages/boot/test/bootstrapTests/vaults-upgrade.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,11 +286,16 @@ test.serial('restart vaultFactory', async t => {
const vaultFactoryKit =
await EV.vat('bootstrap').consumeItem('vaultFactoryKit');

const reserveKit = await EV.vat('bootstrap').consumeItem('reserveKit');
const bootstrapVat = EV.vat('bootstrap');
const reserveKit = await bootstrapVat.consumeItem('reserveKit');
const electorateCreatorFacet = await bootstrapVat.consumeItem(
'economicCommitteeCreatorFacet',
);
const agoricNames = await EV.vat('bootstrap').consumeItem('agoricNames');
const oldVaultInstallation = await EV(agoricNames).lookup(
'installation',
'VaultFactory',
);

const poserInvitation = await EV(electorateCreatorFacet).getPoserInvitation();
const creatorFacet1 = await EV.get(reserveKit).creatorFacet;
Expand Down Expand Up @@ -319,6 +324,13 @@ test.serial('restart vaultFactory', async t => {
const upgradeResult = await EV(vfAdminFacet).restartContract(privateArgs);
t.deepEqual(upgradeResult, { incarnationNumber: 1 });
t.like(readCollateralMetrics(0), keyMetrics); // unchanged

const newVaultInstallation = await EV(agoricNames).lookup(
'installation',
'VaultFactory',
);

t.notDeepEqual(newVaultInstallation, oldVaultInstallation);
});

test.serial('restart contractGovernor', async t => {
Expand Down
8 changes: 7 additions & 1 deletion packages/builders/scripts/vats/add-auction.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,18 @@ export const defaultProposalBuilder = async ({ publishRef, install }) => {
getManifestCall: [
'getManifestForAddAuction',
{
auctionsRef: publishRef(
auctioneerRef: publishRef(
install(
'@agoric/inter-protocol/src/auction/auctioneer.js',
'../../inter-protocol/bundles/bundle-auctioneer.js',
),
),
contractGovernorRef: publishRef(
install(
'@agoric/governance/src/contractGovernor.js',
'../bundles/bundle-contractGovernor.js',
),
),
},
],
});
Expand Down
8 changes: 7 additions & 1 deletion packages/builders/scripts/vats/upgradeVaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,18 @@ export const defaultProposalBuilder = async ({ publishRef, install }) =>
getManifestCall: [
'getManifestForUpgradeVaults',
{
vaultsRef: publishRef(
VaultFactoryRef: publishRef(
install(
'@agoric/inter-protocol/src/vaultFactory/vaultFactory.js',
'../bundles/bundle-vaultFactory.js',
),
),
contractGovernorRef: publishRef(
install(
'@agoric/governance/src/contractGovernor.js',
'../bundles/bundle-contractGovernor.js',
),
),
Comment on lines +16 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're installing the governor from two places. So we'll get 2 installation handles.

Does it matter whether the governor for VaultFactory has an identical installation handle to the one from the auctioneer instance? Does it matter that only one of them can be checked against agoricNames.installation to see whether it's the "right" one?

Copy link
Member

Choose a reason for hiding this comment

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

We discussed the possibility of only using this mechanism in 1 place and waiting for it in the other. (take care to not just wait for consume.installation.contractGovernor, since that's already resolved)

It doesn't seem that either option is catastrophic, so I'll leave it at your discretion.

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 made a separate issue: #10248. I'd like to fix it before this gets to the chain, but I think it's second priority, and there's other work to get done.

},
],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export const makeCoreProposalBehavior = ({
const installAdmin = E(agoricNamesAdmin).lookupAdmin('installation');
await Promise.all(
installationEntries.map(([key, value]) => {
produceInstallations[key].reset();
produceInstallations[key].resolve(value);
return E(installAdmin).update(key, value);
}),
Expand Down
5 changes: 3 additions & 2 deletions packages/governance/src/contractGovernance/governParam.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ const assertBallotConcernsParam = (paramSpec, questionSpec) => {
};

/**
* @param {ERef<ParamManagerRetriever>} paramManagerRetriever
* @param {() => ERef<ParamManagerRetriever>} paramManagerRetrieverAccessor
* @param {Instance} contractInstance
* @param {import('@agoric/time').TimerService} timer
* @param {() => Promise<PoserFacet>} getUpdatedPoserFacet
* @returns {ParamGovernor}
*/
const setupParamGovernance = (
paramManagerRetriever,
paramManagerRetrieverAccessor,
contractInstance,
timer,
getUpdatedPoserFacet,
Expand All @@ -76,6 +76,7 @@ const setupParamGovernance = (
deadline,
paramSpec,
) => {
const paramManagerRetriever = paramManagerRetrieverAccessor();
const paramMgr = await E(paramManagerRetriever).get(paramSpec.paramPath);
/** @type {import('@endo/marshal').Passable} */
const changePs = {};
Expand Down
2 changes: 1 addition & 1 deletion packages/governance/src/contractGovernorKit.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export const prepareContractGovernorKit = (baggage, powers) => {
const { timer } = powers;
const { creatorFacet, instance } = this.state;
paramGovernance = setupParamGovernance(
E(creatorFacet).getParamMgrRetriever(),
() => E(creatorFacet).getParamMgrRetriever(),
instance,
timer,
() => this.facets.helper.getUpdatedPoserFacet(),
Expand Down
Loading
Loading