Grant proposing rights to new charter members #10166

rabi-siddique · 2024-09-27T13:27:39Z

Description

The PR builds on #10164, introducing code changes to initiate a new charter committee instance and send invitations to the members.

Security Considerations

The EC Charter is important for deciding who can suggest changes to governed contract parameters. If the wrong people are given access or if expired members aren't removed, it can lead to security risks by letting unauthorized individuals propose changes.

Since we're starting a new contract, old charter members can still propose questions, even though they can't vote. This still creates a potential security issue, as people who are no longer active can still influence the process. These people might fill the system with too many unnecessary or harmful proposals, which can make it difficult to focus on the important ones and slow down the decision-making process.

Scaling Considerations

Documentation Considerations

Testing Considerations

Same as specified in #10164

Upgrade Considerations

cloudflare-workers-and-pages · 2024-09-27T13:31:18Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`78581b1`
Status:	✅ Deploy successful!
Preview URL:	https://d90e66bd.agoric-sdk.pages.dev
Branch Preview URL:	https://rs-faraz-replace-charter.agoric-sdk.pages.dev

View logs

dckc · 2024-09-27T14:53:28Z

packages/builders/scripts/inter-protocol/replace-charter-core.js

@@ -0,0 +1,54 @@
+import { makeHelpers } from '@agoric/deploy-script-support';


hm. I wasn't expecting a separate builder script for each issue. I suppose it could work, but it seems kinda awkward.

I guess I was expecting this to use the same builder script from #10164 and just expand the core eval code. What do you think about stacking the PRs?

…er (#10178)   refs: #10134 #10133 ## Description This pull request builds on #10166 and #10164. It updates the core eval code to utilize the `governorCreatorFacet` for the governed contracts via the `governedContractKit`. Initially, the aim was to use the `governedContractKit` to access facets from price feed contracts. But since it also includes facets for other governed contracts, I expanded the code to incorporate these as well. ### Security Considerations Same as #10166 and #10164 ### Scaling Considerations  ### Documentation Considerations  ### Testing Considerations Same as specified in #10164 ### Upgrade Considerations

dckc · 2024-10-07T14:25:23Z

packages/boot/test/bootstrapTests/ec-replace-charter.test.ts

+
+    const oracleInvitationAfterProposal =
+      await governanceDriver.ecMembers[0].getOracleInvitation();
+    t.not(oracleInvitationAfterProposal, undefined);


Suggested change

t.not(oracleInvitationAfterProposal, undefined);

t.truthy(oracleInvitationAfterProposal);

or better:

Suggested change

t.not(oracleInvitationAfterProposal, undefined);

t.is(passStyleOf(oracleInvitationAfterProposal), 'remotable');

That's the most info you can get without a remote call to the zoe invitation issuer.

dckc

gotta go to a meeting; this is what i have so far...

dckc · 2024-10-08T18:57:25Z

packages/boot/test/bootstrapTests/ec-replace-charter.test.ts

@@ -0,0 +1,475 @@
+import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';


There's a substantial overlap between ec-membership-update.test.ts and ec-replace-charter.test.ts. How about putting everything in ec-membership-update.test.ts to eliminate duplication?

dckc · 2024-10-08T19:04:58Z

packages/boot/tools/drivers.ts

@@ -281,6 +281,17 @@ export const makeGovernanceDriver = async (
        proposal: {},
      });
    },
+    getOracleInvitation: async () => {


we have method naming conventions and "get" usually throws when it doesn't find the thing... at least in the store API... so consider...

Suggested change

getOracleInvitation: async () => {

findOracleInvitation: async () => {

dckc

This is getting pretty close, but I think some of these are worth fixing.

The test.macro thing is not critical.

Here's hoping I find a moment to go over these comments and clarify which ones are ciritical.

dckc · 2024-10-08T22:45:31Z

packages/boot/test/bootstrapTests/ec-membership-update.test.ts

+test.serial('successful proposal by outgoing member', async t => {
+  // Ability to propose by outgoing member should still exist


hm. it's not so much that it should. It shouldn't, but postponed addressing that, for now.

Perhaps write this as a test that they cannot propose, but mark it test.failing, and cite issue #10136 ?

dckc · 2024-10-08T22:46:46Z

packages/boot/test/bootstrapTests/ec-membership-update.test.ts

+  t.assert(lastOutcome.outcome === 'win');
+});
+
+test.serial('successful proposal and vote for auctioneer', async t => {


I think of this more as...

Suggested change

test.serial('successful proposal and vote for auctioneer', async t => {

test.serial('EC can govern auctioneer parameter', async t => {

dckc · 2024-10-08T22:48:37Z

packages/boot/test/bootstrapTests/ec-membership-update.test.ts

+  t.assert(lastOutcome.outcome === 'win');
+});
+
+test.serial('successful proposal and vote for provisionPool', async t => {


Suggested change

test.serial('successful proposal and vote for provisionPool', async t => {

test.serial('EC can govern provisionPool parameter', async t => {

This starts to get repetitive. You might try a test.macro parameterized on contract instance and governed parameter.

The issue calls for checking all instanced of 6 contract installations. I see only 5. Reserve seems to be missing.

But maybe it's there and I don't see it. It would be nice to see all 6 on one screen using invocations of a macro. For example:

const ecCanGovern = test.macro({ title:(_t, label) => `EC can govern ${label}`, exec(t, label, findInstances, parameters) => { ... }, }); test('auctioneer', ecCanGovern, instance => [instance.auctioneer], { LowestRate: 100_000_000n }); test('reserve', ecCanGovern, instance => [instance.reserve], { ... }); test('PSM', ecCanGovern, instance => instance.map(...).filter(...), { ... });

I hope that's enough of a suggestion. What do you think?

added the test for reserve. missed that accidentally. thanks for the catch

regarding test.macro, i tried it out. personally it didn't improve readability a lot since there are a lot of minor variations between the tests due to which I have to pass a ton of parameters to the macro. reading the tests at first glance, it isnt immediately obvious what those parameters do

thanks for trying it out.

dckc · 2024-10-08T23:05:22Z

packages/boot/tools/drivers.ts

@@ -322,6 +333,28 @@ export const makeGovernanceDriver = async (
    });
  };

+  const proposeApiCall = async (


dckc · 2024-10-08T23:05:37Z

packages/inter-protocol/src/proposals/replaceElectorate.js

@@ -18,6 +18,7 @@ import {
 import { reserveThenDeposit } from './utils.js';

 /** @import {EconomyBootstrapPowers} from './econ-behaviors.js' */
+/** @import {EconCharterStartResult} from './econ-behaviors.js') */


Suggested change

/** @import {EconCharterStartResult} from './econ-behaviors.js') */

/** @import {EconCharterStartResult} from './econ-behaviors.js' */

dckc · 2024-10-08T23:07:32Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+ *   options: {
+ *     voterAddresses: Record<string, string>;
+ *     econCharterKit: {
+ *       creatorFacet: {


IIUC, the config is plain data. How are we getting an object with a method in there?

oh... this isn't an exported function referred to by a behavior.

Still.. the name config threw me off. Maybe opts instead?

dckc · 2024-10-08T23:09:22Z

packages/inter-protocol/src/proposals/replaceElectorate.js

+ * Starts a new Economic Committee Charter by creating an instance with the
+ * provided committee specifications.
+ *
+ * - @param {EconomyBootstrapPowers} powers - The resources and capabilities


Suggested change

* - @param {EconomyBootstrapPowers} powers - The resources and capabilities

* @param {EconomyBootstrapPowers} powers - The resources and capabilities

It's odd that this doesn't fail the lint check in ci.

Github says

Missing JSDoc @param "root0.installation.consume.econCommitteeCharter" declaration

dckc · 2024-10-09T15:48:06Z

packages/boot/tools/drivers.ts

-        proposal: {},
-      });
+      const invitation = findInvitation(w, 'Voter');
+      if (invitation) {


If there's no such invitation, this seems to fail silently. Silent failures drive me buggy. How about assert(invitation);?

this check was added because this function is also triggered from the liquidationDriverKit. since im now using that along with my regular tests, it results in the invitation being accepted twice.
since we dont want that, and it would be kind of unnecessary complicated to update the liquidationDriverKit (and as a result, the tests that use it), i thought it would be easier to have this check present

rename it to acceptOutstandingInvitation()? and add an else case to console.log('no outstanding Voter invitation')?

For this sort of thing, I prefer an early return:

if (!invitation) { console.log(...); return; }

It avoids indenting the code for the main execution path.

dckc

this should work!

please do edit the price feed test name

dckc · 2024-10-09T21:20:34Z

packages/boot/test/bootstrapTests/ec-membership-update.test.ts


 import { makeSwingsetTestKit } from '../../tools/supports.js';
 import {
  makeGovernanceDriver,
  makeWalletFactoryDriver,
 } from '../../tools/drivers.js';
+import { makeLiquidationTestKit } from '../../tools/liquidation.js';
+import { NonNullish } from '@agoric/internal';


Does this pass lint? don't all the intra-package imports (../) have to go after all the inter-package imports?

Try optimize imports?

not critical, unless our lint rules require it

its required. for some reason the lint issues are not actively appearing in my IDE.
thanks for the catch

dckc · 2024-10-09T21:23:07Z

packages/boot/test/bootstrapTests/ec-membership-update.test.ts

+  }
+});
+
+test.serial('EC can govern price feed parameter', async t => {


this case isn't about a parameter.

Suggested change

test.serial('EC can govern price feed parameter', async t => {

test.serial('EC can make calls to price feed governed APIs', async t => {

not a critical correctness issue, but misleading

rabi-siddique requested a review from a team as a code owner September 27, 2024 13:27

rabi-siddique requested a review from AgoricTriage September 27, 2024 13:27

rabi-siddique changed the title ~~Core eval for replacing charter members~~ Grant proposing rights to new charter members Sep 27, 2024

dckc reviewed Sep 27, 2024

View reviewed changes

rabi-siddique marked this pull request as draft September 27, 2024 15:50

rabi-siddique force-pushed the rs-faraz-replace-charter branch from ac34a12 to caeddfd Compare September 30, 2024 03:04

rabi-siddique changed the base branch from master to rs-rotate-voting-rights September 30, 2024 03:05

rabi-siddique force-pushed the rs-faraz-replace-charter branch 8 times, most recently from c486362 to 502343e Compare September 30, 2024 06:27

frazarshad force-pushed the rs-faraz-replace-charter branch from 502343e to cd7809e Compare September 30, 2024 12:26

rabi-siddique requested a review from dckc October 1, 2024 03:40

rabi-siddique marked this pull request as ready for review October 1, 2024 03:40

rabi-siddique force-pushed the rs-faraz-replace-charter branch from cd7809e to e993aeb Compare October 1, 2024 06:55

frazarshad force-pushed the rs-faraz-replace-charter branch from e993aeb to cd7809e Compare October 1, 2024 08:47

rabi-siddique mentioned this pull request Oct 1, 2024

using governedContractKits in core eval replacing committee and charter #10178

Merged

rabi-siddique requested a review from Chris-Hibbert October 2, 2024 12:00

Chris-Hibbert approved these changes Oct 2, 2024

View reviewed changes

rabi-siddique mentioned this pull request Oct 3, 2024

rotate voting rights for EC committee #10164

Merged

dckc reviewed Oct 7, 2024

View reviewed changes

frazarshad force-pushed the rs-rotate-voting-rights branch from cafe7fd to ca27d2f Compare October 8, 2024 17:01

Base automatically changed from rs-rotate-voting-rights to master October 8, 2024 17:39

frazarshad force-pushed the rs-faraz-replace-charter branch from ffce1a7 to aef84d0 Compare October 8, 2024 18:05

dckc reviewed Oct 8, 2024

View reviewed changes

frazarshad requested a review from dckc October 8, 2024 21:28

dckc requested changes Oct 8, 2024

View reviewed changes

dckc reviewed Oct 9, 2024

View reviewed changes

frazarshad requested a review from dckc October 9, 2024 21:15

dckc approved these changes Oct 9, 2024

View reviewed changes

frazarshad added the automerge:rebase Automatically rebase updates, then merge label Oct 10, 2024

rabi-siddique and others added 9 commits October 10, 2024 05:48

chore: use startUpgradable

c72ee89

test: bootstrap test for replaceElectorate

20a0b18

chore: change buildProposal definition to accept args

dc546f9

feat: core eval for replacing charter members

100acf0

test: test for replacing instance of charter contract

95ebf02

chore: changes to incorporate governedContractKits

40eec81

test: added more tests for each individual contract

6226b30

docs: add JSDoc comments to enhance function documentation

6ebf525

chore: use startUpgradable

78581b1

rabi-siddique force-pushed the rs-faraz-replace-charter branch from 6a3afcb to 78581b1 Compare October 10, 2024 05:48

mergify bot merged commit fc1eb01 into master Oct 10, 2024
80 checks passed

mergify bot deleted the rs-faraz-replace-charter branch October 10, 2024 06:24

rabi-siddique mentioned this pull request Oct 10, 2024

The newly elected EC must be able to govern (propose + vote) #10185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grant proposing rights to new charter members #10166

Grant proposing rights to new charter members #10166

rabi-siddique commented Sep 27, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 27, 2024 •

edited

Loading

dckc Sep 27, 2024

dckc Oct 7, 2024

frazarshad Oct 8, 2024

dckc left a comment

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc left a comment

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

frazarshad Oct 9, 2024

frazarshad Oct 9, 2024

dckc Oct 9, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 8, 2024

dckc Oct 9, 2024

frazarshad Oct 9, 2024

dckc Oct 9, 2024

dckc left a comment

dckc Oct 9, 2024

frazarshad Oct 10, 2024

dckc Oct 9, 2024

		@@ -0,0 +1,54 @@
		import { makeHelpers } from '@agoric/deploy-script-support';

	t.not(oracleInvitationAfterProposal, undefined);
	t.truthy(oracleInvitationAfterProposal);

	t.not(oracleInvitationAfterProposal, undefined);
	t.is(passStyleOf(oracleInvitationAfterProposal), 'remotable');

		@@ -0,0 +1,475 @@
		import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';

	getOracleInvitation: async () => {
	findOracleInvitation: async () => {

		test.serial('successful proposal by outgoing member', async t => {
		// Ability to propose by outgoing member should still exist

	test.serial('successful proposal and vote for auctioneer', async t => {
	test.serial('EC can govern auctioneer parameter', async t => {

	test.serial('successful proposal and vote for provisionPool', async t => {
	test.serial('EC can govern provisionPool parameter', async t => {

	/** @import {EconCharterStartResult} from './econ-behaviors.js') */
	/** @import {EconCharterStartResult} from './econ-behaviors.js' */

	* - @param {EconomyBootstrapPowers} powers - The resources and capabilities
	* @param {EconomyBootstrapPowers} powers - The resources and capabilities

	test.serial('EC can govern price feed parameter', async t => {
	test.serial('EC can make calls to price feed governed APIs', async t => {

Grant proposing rights to new charter members #10166

Grant proposing rights to new charter members #10166

Conversation

rabi-siddique commented Sep 27, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Sep 27, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabi-siddique commented Sep 27, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 27, 2024 •

edited

Loading