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

SPIKE: rotate EC membership #10105

Closed
wants to merge 14 commits into from
Closed

SPIKE: rotate EC membership #10105

wants to merge 14 commits into from

Conversation

rabi-siddique
Copy link
Contributor

closes: #XXXX
refs: #XXXX

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Copy link

cloudflare-workers-and-pages bot commented Sep 18, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: cfe20f4
Status: ✅  Deploy successful!
Preview URL: https://f371c254.agoric-sdk.pages.dev
Branch Preview URL: https://rs-ec-changes.agoric-sdk.pages.dev

View logs

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 can make these changes less invasive...

packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
@dckc
Copy link
Member

dckc commented Sep 24, 2024

For the title, consider "SPIKE: rotate EC membership"

This exploratory work is great, but I expect we'll want to break it into a number of PRs when we add tests and such.

packages/governance/src/committee.js Outdated Show resolved Hide resolved
Comment on lines 182 to 184
shutdown() {
zcf.shutdown('EC Charter Shutdown');
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add this now. The Committee is the sole holder of the set of questions that have been voted on, and I'd prefer we find a way to write this to vstorage before killing the vats.

The reason to do something like this is to prevent ex-members from voting, so let's do that directly. Try adding a method like stopAddingVotes() that sets a flag that blocks use of addQuestion() or getPoserInvitation().

Copy link
Member

Choose a reason for hiding this comment

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

noted as a design choice in

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to add this method, it should be tested in unit tests and some test that does an upgrade. Either a bootstrap test or a3p-integration would do.

packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
packages/inter-protocol/test/psm/gov-replace-committee.js Outdated Show resolved Hide resolved
consume: { economicCommitteeCreatorFacet, econCharterKit },
}) => {
console.log('Shutting down old EC Committee');
await E(economicCommitteeCreatorFacet).shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is available on the new committee, but not the old one, unless the old one gets upgraded.

@@ -179,6 +180,9 @@ export const start = async (zcf, privateArgs, baggage) => {
},
makeCharterMemberInvitation: () =>
zcf.makeInvitation(charterMemberHandler, INVITATION_MAKERS_DESC),
shutdown: () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to add this method, it should be tested in unit tests and some test that does an upgrade. Either a bootstrap test or a3p-integration would do.

Comment on lines 182 to 184
shutdown() {
zcf.shutdown('EC Charter Shutdown');
},
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to add this method, it should be tested in unit tests and some test that does an upgrade. Either a bootstrap test or a3p-integration would do.


console.log('Shutting down old EC Charter');
const { creatorFacet } = E.get(econCharterKit);
await E(creatorFacet).shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is available on the new committee, but not the old one, unless the old one got upgraded.

@rabi-siddique rabi-siddique changed the title EC Changes SPIKE: rotate EC membership Sep 25, 2024
@@ -179,6 +179,9 @@ export const start = async (zcf, privateArgs, baggage) => {
},
makeCharterMemberInvitation: () =>
zcf.makeInvitation(charterMemberHandler, INVITATION_MAKERS_DESC),
shutdown: () => {
Copy link
Member

Choose a reason for hiding this comment

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

adding shutdown methods to both contracts in 1 commit is awkward - I expect them to land in separate PRs, since the testing considerations are different.

Copy link
Contributor Author

@rabi-siddique rabi-siddique Sep 25, 2024

Choose a reason for hiding this comment

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

Good point. I will create two separate PRs along with the tests.

re: I've created #10151 and #10150 for this purpose.

@dckc
Copy link
Member

dckc commented Oct 3, 2024

This SPIKE served the purpose of exploring feasibility. Nice work!

Product development proceeds in other PRs.

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.

3 participants