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

Jump to bottom

7562 proposal restart #7660

Merged
merged 10 commits into from
May 11, 2023
Merged

7562 proposal restart #7660

merged 10 commits into from
May 11, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented May 9, 2023 •
edited
Loading

Description

Part of #7562

A new CORE_EVAL proposal to restart all contracts that should be. It's called "restart-vats" because it will restart all vats that should be, but it doesn't do that yet. I'm going to stack that work on top of this PR to get this landed.

Security Considerations

The privateArgs used in some contract starts in bootstrap are saved in bootstrap.

Scaling Considerations

--

Documentation Considerations

No API changes.

Testing Considerations

The proposal will run in testnet. A new bootstrap test runs it in CI.

@turadg turadg force-pushed the 7562-proposal-restart branch 3 times, most recently from a73a40c to 9a5f69b Compare May 10, 2023 18:33
@turadg turadg force-pushed the 7562-proposal-restart branch 3 times, most recently from e0a2711 to ca27439 Compare May 11, 2023 00:33
@turadg turadg mentioned this pull request May 11, 2023
Merged
@turadg turadg force-pushed the 7562-proposal-restart branch 2 times, most recently from 75688b9 to ce2ed6c Compare May 11, 2023 14:59
@turadg turadg changed the base branch from master to 7562-misc May 11, 2023 17:24
@turadg turadg requested review from dckc and Chris-Hibbert May 11, 2023 17:28
@turadg turadg marked this pull request as ready for review May 11, 2023 17:30
Base automatically changed from 7562-misc to master May 11, 2023 18:37
@turadg turadg marked this pull request as draft May 11, 2023 19:23
@turadg turadg marked this pull request as ready for review May 11, 2023 20:34
@turadg turadg enabled auto-merge May 11, 2023 20:35
dckc
dckc reviewed May 11, 2023
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

we agreed to rework diagnostics a bit

packages/inter-protocol/src/proposals/startEconCommittee.js
'economicCommittee',
);
const { creatorFacet, instance } = startResult;

economicCommitteeKit.resolve(startResult);
economicCommitteeKit.resolve(
// XXX should startInstance return its label?
Copy link
Member

Choose a reason for hiding this comment

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

I can see it both ways

packages/inter-protocol/src/price/fluxAggregatorContract.js
M.remotable('prioritySenders manager'),
M.null(),
),
// only necessary on first invocation, not subsequent
Copy link
Member

Choose a reason for hiding this comment

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

interesting pattern, this

packages/inter-protocol/src/proposals/committee-proposal.js
economicCommitteeCreatorFacet,
highPrioritySendersManager,
},
consume: { namesByAddressAdmin, economicCommitteeCreatorFacet, ...consume },
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I typically leave a comment mentioning things like highPrioritySendersManager that are consumed in the function body, to make aid in keeping consistency with the permit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figure the permit is the SOT so if this strays it will fail quickly

packages/inter-protocol/src/proposals/core-proposal.js
@@ -59,6 +59,7 @@ const SHARED_MAIN_MANIFEST = harden({
consume: {
board: 'board',
chainStorage: true,
diagnostics: true,
Copy link
Member

Choose a reason for hiding this comment

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

The name diagnostics suggests it's innocuous, but it includes all sorts of private args. Let's talk about that.

packages/inter-protocol/src/proposals/price-feed-proposal.js

trace('awaiting startInstance');
const faKit = await E(startGovernedUpgradable)({
governedParams: {},
privateArgs: {
highPrioritySendersManager,
highPrioritySendersManager: await highPrioritySendersManager,
Copy link
Member

Choose a reason for hiding this comment

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

this passes lint? wild.

packages/vats/src/core/basic-behaviors.js
const instancePA = await E.get(diagnostics).instancePrivateArgs;
instancePA.set(kit.instance, privateArgs);
instancePA.set(kit.governor, {
economicCommitteeCreatorFacet: await economicCommitteeCreatorFacet,
Copy link
Member

Choose a reason for hiding this comment

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

is lint ok with that? (again)

packages/vats/src/proposals/restart-vats-proposal.js

const trace = makeTracer('RV');

const HR = '----------------';
Copy link
Member

Choose a reason for hiding this comment

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

flashback to 1991 libwww code :)

packages/vats/src/proposals/restart-vats-proposal.js
* @param {ERef<AdminFacet>} adminFacet
*/
const tryRestart = async (debugName, instance, adminFacet) => {
// TODO document that privateArgs cannot contain promises
Copy link
Member

Choose a reason for hiding this comment

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

good idea...

Copy link
Member Author

Choose a reason for hiding this comment

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

#7695

packages/vats/src/proposals/restart-vats-proposal.js
Comment on lines +81 to +84
const debugName =
kit.label || getInterfaceOf(kit.publicFacet) || 'UNLABELED';
if (debugName !== kit.label) {
console.warn('MISSING LABEL:', kit);
Copy link
Member

Choose a reason for hiding this comment

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

ouch. that looks like tracking down labels was more trouble than I expected.

packages/vats/src/proposals/restart-vats-proposal.js
}
};

// iterate over the two contractKits and use the adminFacet to restartContract
Copy link
Member

Choose a reason for hiding this comment

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

two? we don't need psmKit here too? getting the psmKit stuff into governedContractKits looked like it was still TODO.
(I'm only reviewing this PR for regressions, so it's not critical to landing this, but seems like an omission)

Copy link
Member Author

Choose a reason for hiding this comment

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

this is describing the behavior. the TODO about including psmKits is up top

dckc
dckc approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

👍

packages/vats/src/core/basic-behaviors.js
Comment on lines +128 to +129
produce.diagnostics.resolve({ savePrivateArgs });
produce.instancePrivateArgs.resolve(instancePrivateArgs);
Copy link
Member

Choose a reason for hiding this comment

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

👏

@turadg turadg mentioned this pull request May 11, 2023
Open
@turadg turadg added this pull request to the merge queue May 11, 2023
Merged via the queue into master with commit 0a1dfc6 May 11, 2023
@turadg turadg deleted the 7562-proposal-restart branch May 11, 2023 22:49
@turadg turadg mentioned this pull request May 13, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erights erights erights left review comments

@dckc dckc dckc approved these changes

@Chris-Hibbert Chris-Hibbert Awaiting requested review from Chris-Hibbert

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@turadg @dckc @erights