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

feat: save the outgoing EC Charter instance and kit #10749

Merged
merged 2 commits into from
Dec 20, 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
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,18 @@ export const testRecordedRetiredInstances = async ({
);
assert(committeeIDs);
assert(committeeIDs.length === 1);
trace('found committeeIDs', committeeIDs);

const committeeInstance = retiredContractInstances.get(committeeIDs[0]);
assert(await E(contractKits).get(committeeInstance));

const charterIDs = [...retiredContractInstances.keys()].filter(k =>
k.startsWith('econCommitteeCharter'),
);
assert(charterIDs);
Copy link
Member

Choose a reason for hiding this comment

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

log it/them? I'd feel better seeing positive evidence in a ci log.

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

assert(charterIDs.length === 1);
trace('found charterID', charterIDs);

trace('done');
};
harden(testRecordedRetiredInstances);
Expand Down
83 changes: 71 additions & 12 deletions packages/inter-protocol/src/proposals/replaceElectorate.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,33 +289,87 @@ const startNewEconomicCommittee = async (
* Starts a new Economic Committee Charter by creating an instance with the
* provided committee specifications.
*
* @param {EconomyBootstrapPowers} powers - The resources and capabilities
* required to start the committee.
* @param {EconomyBootstrapPowers &
* PromiseSpaceOf<{ retiredContractInstances: MapStore<string, Instance> }>} powers
* - The resources and capabilities required to start the committee.
Comment on lines +293 to +294
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if/when retiredContractInstances should be added in vats/src
/core/types-ambient.d.ts

*
* @returns {Promise<EconCharterStartResult>} A promise that resolves to the
* charter kit result.
*/
const startNewEconCharter = async ({
consume: { startUpgradable },
produce: { econCharterKit },
consume: {
board,
startUpgradable,
contractKits: contractKitsP,
econCharterKit: econCharterKitP,
retiredContractInstances: retiredContractInstancesP,
},
produce: {
econCharterKit: produceEconCharterKit,
retiredContractInstances: produceRetiredInstances,
},
installation: {
consume: { binaryVoteCounter: counterP, econCommitteeCharter: installP },
},
instance: {
produce: { econCommitteeCharter },
consume: { econCommitteeCharter: previousInstanceP },
},
}) => {
const [charterInstall, counterInstall] = await Promise.all([
const [
charterInstall,
counterInstall,
previousInstance,
contractKits,
econCharterKit,
retiredInstances,
] = await Promise.all([
installP,
counterP,
previousInstanceP,
contractKitsP,
econCharterKitP,
provideRetiredInstances(retiredContractInstancesP, produceRetiredInstances),
Copy link
Member

Choose a reason for hiding this comment

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

still slightly weirded out by our approach for this, but it looks sound

Copy link
Member

Choose a reason for hiding this comment

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

in case it makes you feel any better, the approach goes back to the vaults release.

// Initialize the periodic collectors list if we don't have one.
periodicFeeCollectorsP.resolve(
makeScalarBigMapStore('periodicCollectors', { durable: true }),
);
const periodicCollectors = await periodicFeeCollectors;

or even further (mid 2022) using []:

// Initialize the periodic collectors list if we don't have one.
periodicFeeCollectorsP.resolve([]);
const periodicCollectors = await periodicFeeCollectors;

]);
const terms = await harden({
binaryVoteCounterInstallation: counterInstall,
});

const label = 'econCommitteeCharter';
const previousCharterKit = { ...econCharterKit, label };

const boardID = await E(board).getId(previousCharterKit.instance);
const identifier = `${label}-${boardID}`;
Copy link
Member

Choose a reason for hiding this comment

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

are we relying on this identifier syntax somewhere? It seems like there should be helper functions to construct and parse it. perhaps near provideRetiredInstances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far there's no plan for code to rely on it. People may construct scripts to make use of it (like Richard's vat killer), but the boardID has to be looked up manually at this point to know what name to make.

trace('Saving previous EC Charter Instance', label);

// save the old charter instance kit so we can manage it later
if (retiredInstances.has(identifier)) {
// bootstrap tests start having already run this upgrade. Actual upgrades on
// mainNet or testnets should start with the promiseSpace post upgrade-17,
// which doesn't have this entry in the map.
trace(
'⚠️ WARNING: collision on storing Charter in retireInstances not' +
' expected during chain upgrade. It IS normal during bootstrap tests',
);
} else {
retiredInstances.init(identifier, previousCharterKit.instance);
}
if (contractKits.has(previousInstance)) {
// bootstrap tests start having already run this upgrade. Actual upgrades on
// mainNet or testnets should start with the promiseSpace post upgrade-17,
// which doesn't have this entry in the map.
trace(
'⚠️ WARNING: collision on storing Charter in contractKits not' +
' expected during chain upgrade. It IS normal during bootstrap tests',
);
} else {
contractKits.init(previousInstance, previousCharterKit);
}

trace('Starting new EC Charter Instance');

const terms = harden({
binaryVoteCounterInstallation: counterInstall,
});
const startResult = await E(startUpgradable)({
label: 'econCommitteeCharter',
label,
installation: charterInstall,
terms,
});
Expand All @@ -325,8 +379,8 @@ const startNewEconCharter = async ({
econCommitteeCharter.reset();
econCommitteeCharter.resolve(E.get(startResult).instance);

econCharterKit.reset();
econCharterKit.resolve(startResult);
produceEconCharterKit.reset();
produceEconCharterKit.resolve(startResult);
return startResult;
};

Expand Down Expand Up @@ -515,6 +569,8 @@ export const getManifestForReplaceAllElectorates = async (
auctionUpgradeNewGovCreator: true,
auctionUpgradeNewInstance: true,
psmKit: true,
contractKits: true,
econCharterKit: true,
governedContractKits: true,
chainStorage: true,
highPrioritySendersManager: true,
Expand Down Expand Up @@ -543,7 +599,10 @@ export const getManifestForReplaceAllElectorates = async (
economicCommittee: true,
econCommitteeCharter: true,
},
consume: { economicCommittee: true },
consume: {
economicCommittee: true,
econCommitteeCharter: true,
},
},
},
},
Expand Down
Loading