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

Unbork Emerynet: Cleanup Auctioneers #10725 #10861

Open
wants to merge 9 commits into
base: dev-upgrade-18-emerynet-fix
Choose a base branch
from

Conversation

siarhei-agoric
Copy link
Contributor

@siarhei-agoric siarhei-agoric commented Jan 17, 2025

refs: #10725

Description

Create a single-file core-eval proposal-builder script to terminate a set of governed contracts passed as arguments as a list of boardID:label pairs.

Security Considerations

This can be [mis]used to kill any contract for which boardID is known.

Scaling Considerations

This script is intended to run as a core-eval, so there is no scaling issues for the script itself. However, it may take long time to process termination of some contracts with large state footprint.

Documentation Considerations

Not intended for wide use outside of Agoric eng.

Testing Considerations

Bootstrap test is included.
A3P testing had been considered but abandoned due to complications with retrieval of boardIDs for target contract instances.

Upgrade Considerations

This should run as part of next Emerynet upgrade.

siarhei-agoric and others added 5 commits January 15, 2025 10:38
refs: #10725

Initial set of changes to get auctioneer governors terminated.
 - contractGovernorExecutor: define ContractGovernorKit kind
 - upgrade-governor-instance:
   - fix SELF
   - some docs
   - rename to governorExecutorBundleId
     - get .bundleID out of bundleRef
 - move governor termination from self.shutdown to core eval
refs: #10725

This change includes:
* A single-file core-eval builder-proposal to terminate governed contracts and their
governors by boardID.
* A bootstrap test which:
** creates an instance of a simple governed contract via its own core-eval
** confirms that the contract accessible and operational
** kill the contract instance by boardID
** confirms that the contract is no longer operational
@siarhei-agoric siarhei-agoric changed the title Unbork emerynet #10725 Unbork Emerynet: Cleanup Auctioneers #10725 Jan 17, 2025
@siarhei-agoric siarhei-agoric changed the base branch from master to dev-upgrade-18 January 17, 2025 23:07
@Chris-Hibbert
Copy link
Contributor

Uhh. Do you want a review at this point? Reviewers have been added, but it's marked Draft, and many parts look unfinished.

@siarhei-agoric siarhei-agoric changed the base branch from dev-upgrade-18 to dev-upgrade-18-emerynet-fix January 21, 2025 19:54
Copy link

cloudflare-workers-and-pages bot commented Jan 22, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1964597
Status:🚫  Build failed.

View logs

@siarhei-agoric siarhei-agoric marked this pull request as ready for review January 22, 2025 17:11
@siarhei-agoric siarhei-agoric requested a review from a team as a code owner January 22, 2025 17:11
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

LGTM. (I paired on some of this code.)

@@ -78,4 +79,4 @@
"typeCoverage": {
"atLeast": 89.49
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is json happy with files that don't end with a linefeed?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but git and editor software definitely prefer its presence.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Lots of style suggestions, and a few substantial ones. Feel free to push back on any that you disagree with.

golang/cosmos/app/upgrade.go Show resolved Hide resolved
@@ -0,0 +1,24 @@
import { readFile } from 'node:fs/promises';
import { fileURLToPath } from 'node:url';
import { search } from '@endo/compartment-mapper';
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to add "@endo/compartment-mapper": "^1.4.0" to packages/internal/package.json dependencies.

packages/internal/src/module-utils.js Outdated Show resolved Hide resolved
packages/internal/src/module-utils.js Outdated Show resolved Hide resolved
packages/internal/src/module-utils.js Show resolved Hide resolved
Comment on lines +42 to +47
if (!m) {
badTargets.push(arg);
} else {
// @ts-expect-error cast
targets.push(m.groups);
}
Copy link
Member

Choose a reason for hiding this comment

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

Indent-reduction suggestion:

Suggested change
if (!m) {
badTargets.push(arg);
} else {
// @ts-expect-error cast
targets.push(m.groups);
}
if (!m) {
badTargets.push(arg);
continue;
}
// @ts-expect-error cast
targets.push(m.groups);

Comment on lines +49 to +53
if (badTargets.length !== 0) {
throw makeError`malformed target(s): ${badTargets}`;
} else if (targets.length === 0) {
throw makeError`no target(s)`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (badTargets.length !== 0) {
throw makeError`malformed target(s): ${badTargets}`;
} else if (targets.length === 0) {
throw makeError`no target(s)`;
}
if (badTargets.length) throw makeError`malformed target(s): ${badTargets}`;
if (!targets.length) throw makeError`no target(s)`;

packages/governance/package.json Outdated Show resolved Hide resolved
@@ -41,7 +42,9 @@ const start = async (zcf, privateArgs) => {
getNum: () => params.getMalleableNumber(),
getApiCalled: () => governanceAPICalled,
}),
creatorFacet: makeGovernorFacet({}, { governanceApi }),
creatorFacet: makeFarGovernorFacet(Far('governedContract creatorFacet'), {
Copy link
Member

Choose a reason for hiding this comment

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

Opened #10879 to document the general problem that masked this bug until now.

Comment on lines +65 to +68
// confirm the contract is no longer there
await t.throwsAsync(() => EV(publicFacet).getNum(), {
message: 'vat terminated',
});
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add something to prove that the governor has been terminated as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

The governor's public facet has getElectorate(), for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants