-
Notifications
You must be signed in to change notification settings - Fork 207
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
rotate voting rights for EC committee #10164
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
561806e
to
aa185a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic progress, but this part needs to should run in ci too:
To test with a3p, I've been using this bash script. Make sure you've added the gov1 keyring to your local keyring ...
See z:acceptance
in a3p-integration
.
p.s. I see that an a3p test isn't in the test plan for #10134, so that's negotiable. But you evidently found it important to get confidence in this code. Perhaps it's feasible to get a similar level of confidence by adding to the bootstrap test here?
0c1133b
to
396c189
Compare
With an issue as critical as this, I'd prefer that the top sections be more filled out. It's probably the case that the original issue should have indicated that it's not a small deal. Security ConsiderationsThe Economic Committee plays a crucial role in managing the Agoric chain. The current committee doesn't easily support a smooth transition when members need to retire or be replaced. An incomplete transition to a new committee could severely hamper operations of the chain. Accidentally providing access to the wrong parties (or failing to remove parties whose terms have expired) could make it hard to ensure that the right committee members have control. Other chains have been sunk or badly damaged when the stakeholders couldn't restrict some people from making changes. Upgrade considerationsChanging the composition of the committee requires upgrading some of the economic contracts at this point. We need to test and inspect carefully to ensure that a mistake doesn't unintentionally re-use earlier invitations, or reference the wrong version of the committee. |
#10134 is carefully scoped to only rotating voting rights, which does not require upgrading any contracts. Calling
Yes... I only got as far as TBD in #10134. In #10133 I got as far as "many and subtle". :)
I'd say managing The Inter Protocol. I suppose inasmuch as IST is used to pay for contract execution, you could say that applies more widely, though. |
addressesToRemove: [], | ||
}, | ||
}, | ||
BOOTSTRAP_TEST: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updatePriceFeeds.js in #10074 also includes values for main
, and devnet
. I think it's worth adding those so we can review.
#10146 shows how to add platform-specific build instructions to package.json..
t.context = await makeZoeTestContext(t); | ||
}); | ||
|
||
test.serial('normal running of committee', async t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10163 uses the governanceDriver
in bootstrapTests
. It vastly simplifies the setup and interactions in order to propose and vote on parameter changes. Calling updateVaultManagerParams()
does all the work for param changes.
The driver also has an entrypoint for accepting invitations. I don't know whether you'd have to enhance it to support changing electorates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered about that driver... I sort of assumed it was limited to puppet governance, but I guess not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't appear to be. boot/tools/drivers.ts
is definitely enumerating ec members during voting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chris-Hibbert @dckc i used the governance driver, but i had to make a lot of updates to its code. lmk if it looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are fine changes. I really like the way it simplified the test.
E.get(permittedPowers.consume.reserveKit).governorCreatorFacet, | ||
E.get(permittedPowers.consume.auctioneerKit).governorCreatorFacet, | ||
E.get(permittedPowers.consume.vaultFactoryKit).governorCreatorFacet, | ||
...[...psmKitMap.values()].map(psmKit => psmKit.psmGovernorCreatorFacet), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to omit provisionPool and the price_feeds? They're all governed by the EC as well.
Unfortunately, with the priceFeeds, once the priceFeed coreEval goes out (see #10074), there will be old priceFeeds and new ones.It would be better if we could just upgrade the governors for the active contracts, but it's not obvious how best to distinguish them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to omit provisionPool and the price_feeds?
not really, but in the test plan for the issue here, I only called for testing one parameter of one governed contract.
Testing that they all work is explicitly in #10133 ... but you're probably right that it should be in scope here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we could just upgrade the governors for the active contracts, but it's not obvious how best to distinguish them.
I see little down-side in doing replaceElectorate
for all of them. (again, we're not upgrading anything here... oh... but maybe we'll run into #9982. hm.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. good. let's replace them all.
It doesn't appear that all comments have been addressed.
|
Is this task about notifying price feeds of a new committee different from what was implemented in #10178? If yes, could you provide some pointers? |
Will be adding that after confirmation as I am not aware of addresses associated with re: I added values for |
I'll add that in a separate PR. |
…er (#10178) <!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! ✰ ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> <!-- Most PRs should close a specific Issue. All PRs should at least reference one or more Issues. Edit and/or delete the following lines as appropriate (note: you don't need both `refs` and `closes` for the same one): --> 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 <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations Same as specified in #10164 ### Upgrade Considerations <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
Nope, that's what I'm looking for. I can't tell where those changes were merged, but it doesn't seem to be here. |
TODOs are too easy to lose track of. I think that's a blocker for merging this PR. A checkbox in the top-comment here would be prominent enough, or else an issue. |
Please take the manual testing stuff out of Testing Considerations. |
const configurations = { | ||
MAINNET: { | ||
committeeName: 'Economic Committee', | ||
// TODO: Update the addresses after confirmation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO when? before or after merging this PR? If after, please add an issue and cite it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10194 is the issue to cite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the 👍 means you agree to add the issue number to this comment, but I don't see any such change yet. I suppose I'll stand by for you to hit the re-review button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UNTIL form looked good. Did it go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reinstated. Just me messing stuff up by going back and forth between the committee and charter branches :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting close!
I think I should hold off approval until we learn more about the mainnet slate of addresses.
const Usage = `agoric run replace-electorate-core.js ${keys(configurations).join(' | ')}`; | ||
export default async (homeP, endowments) => { | ||
const { scriptArgs } = endowments; | ||
const config = configurations[scriptArgs?.[0]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const config = configurations[scriptArgs?.[0]]; | |
const variant = scriptArgs?.[0]; | |
const config = configurations[variant]; |
|
||
const { writeCoreEval } = await makeHelpers(homeP, endowments); | ||
|
||
await writeCoreEval('replace-committee', (utils, opts) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await writeCoreEval('replace-committee', (utils, opts) => | |
await writeCoreEval(`replace-committee-${variant}`, (utils, opts) => |
if (!highPrioritySendersManager) { | ||
throw Error('highPrioritySendersManager is not defined'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!highPrioritySendersManager) { | |
throw Error('highPrioritySendersManager is not defined'); | |
} | |
highPrioritySendersManager || Fail`highPrioritySendersManager is not defined`; |
If this isn't in Coding-Style, it should be.
for (const addr of addressesToAdd) { | ||
trace(`Adding ${addr} to High Priority Senders list`); | ||
await E(highPrioritySendersManager).add( | ||
HIGH_PRIORITY_SENDERS_NAMESPACE, | ||
addr, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const addr of addressesToAdd) { | |
trace(`Adding ${addr} to High Priority Senders list`); | |
await E(highPrioritySendersManager).add( | |
HIGH_PRIORITY_SENDERS_NAMESPACE, | |
addr, | |
); | |
} | |
await Promise.all(addressesToAdd.map(addr => E(highPrioritySendersManager).add( | |
HIGH_PRIORITY_SENDERS_NAMESPACE, | |
addr, | |
))); |
If the tracing is important, consider:
const traced = (label, x) => {
trace(label, x);
return x;
and then
for (const addr of addressesToAdd) { | |
trace(`Adding ${addr} to High Priority Senders list`); | |
await E(highPrioritySendersManager).add( | |
HIGH_PRIORITY_SENDERS_NAMESPACE, | |
addr, | |
); | |
} | |
await Promise.all(addressesToAdd.map(addr => E(highPrioritySendersManager).add( | |
HIGH_PRIORITY_SENDERS_NAMESPACE, | |
traced('High Priority Senders: adding', addr), | |
))); |
E.get(permittedPowers.consume.reserveKit).governorCreatorFacet, | ||
E.get(permittedPowers.consume.auctioneerKit).governorCreatorFacet, | ||
E.get(permittedPowers.consume.vaultFactoryKit).governorCreatorFacet, | ||
...[...psmKitMap.values()].map(psmKit => psmKit.psmGovernorCreatorFacet), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. good. let's replace them all.
const psmKitMap = await permittedPowers.consume.psmKit; | ||
|
||
const creatorFacets = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get all these from governedContractKits
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were you, I'd push stuff like governedContractKits
that's in scope for this PR down into this PR so we can land this one.
I know I recommended stacking #10166 on this, but I was thinking that #10166 would remain in Draft mode until this one is landed.
Merging #10166 into this one is an option, but it would expand the scope considerably. I wouldn't be comfortable approving until I had looked the whole combined thing over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Made this commit:
5785221
) => ({ | ||
manifest: { | ||
[replaceElectorate.name]: { | ||
consume: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lists such as this one, I'd like to see the closely held powers listed first, then a comment saying "the rest of these are designed to be widely shared."
The widely shared ones are board and zoe. Hm. short list. Maybe not worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I would appreciate it a lot if you could share some helpful links where I could expand on my understanding of widely
and closely
held. Is there some list that widely shared powers and closely shared powers segregated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would appreciate it a lot if you could share some helpful links ...
Silly me. I know better than to use jargon like that without providing such links... we lose a round-trip when you have to ask.
In this case, I can't think of what to point you at. @Chris-Hibbert ? Help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reserveKit: true, | ||
auctioneerKit: true, | ||
vaultFactoryKit: true, | ||
psmKit: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does governedContractKits
subsume all 4 of these for our purposes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Made the update.
hm. then this PR does not close #10134? p.s. no... the "separate PR" is about a3p-integration, which is not needed until we're doing contract upgrade. no contract upgrade in #10134. |
I took out "closes #10194". That can't happen until some social governance processes complete. |
Not for My original "how to add platform-specific build instructions to package.json" was about adding something to the I don't expect most proposals to require that dependency, but when it's present, we should add the appropriate commands as part of getting ready for release. It doesn't have to be a blocker for preparing the |
...
I suppose I'm mentally tracking that sort of thing as #10138 . But I've also been saying that #10138 is zero effort on top of features such as this one. Not quite true. I'm still content to leave it out of scope of #10134 (and hence this PR). |
Where did Upgrade Considerations go? Comparing against the PR template, we have:
I suppose Scaling Considerations are unremarkable. The core eval script is probably O(n) in the number of EC members; given a cap of 12, that makes it more like O(1). And the code runs about once a year, with 3 day notice to all stakers. I suppose Documentation Considerations are mostly a concern for the proposer, so they mostly belong in #10138. We should note somewhere that the old committee is still hanging around and care should be taken to avoid confusion. I wonder where. Upgrade Considerations are also unremarkable. The core-eval materials are in new files, so there's ~no risk of regression in existing materials. The new builder script can be included in any upcoming release. So nothing critical is missing. But in the future, please don't omit these sections. Be explicit that you thought about it and found nothing remarkable. IIUC, the PR description goes into the git history as the description of the merge commit. So I'm reviewing it too. |
The test failure in ci is a known flake. I'm re-running the job.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Looks good.
I have a few suggestions for you to consider. No blockers.
The most straightforward one is probably the extra -
that's messing up the JSDocs for types.
|
||
const { writeCoreEval } = await makeHelpers(homeP, endowments); | ||
|
||
await writeCoreEval(`replace-committee-${variant}`, (utils, opts) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any replace-committee-*
files where I would expect them:
$ cd ~/projects/agoric-sdk/a3p-integration
$ corepack enable
$ yarn
$ yarn build:submissions
$ find . -name 'replace-committee*'
I suppose this is related to Chris's comment.
And I suppose #10138 ensures that we will get to it eventually. So I supposed it's not a blocker for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is related to #10164 (comment).
And I suppose #10138 ensures that we will get to it eventually. So I supposed it's not a blocker for this PR.
Yes
@@ -0,0 +1,326 @@ | |||
// @ts-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a /** @file ... */
comment with a line or two of explanation and a reference to replace-electorate-core.js
.
@@ -0,0 +1,99 @@ | |||
/* global process */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a /** @file ... */
comment including usage?
By way of example: init-orca.js
* Starts a new Economic Committee (EC) by creating an instance with the | ||
* provided committee specifications. | ||
* | ||
* - @param {EconomyBootstrapPowers} powers - The resources and capabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - @param {EconomyBootstrapPowers} powers - The resources and capabilities | |
* @param {EconomyBootstrapPowers} powers - The resources and capabilities |
cafe7fd
to
ca27d2f
Compare
@rabi-siddique, you showed that some PriceFeeds were present in
I'm guessing that was in |
@Chris-Hibbert Yes, it's |
closes: #10133 refs: #10133 ## 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
closes: #10274 ## Description Half the EC were able to pass a motion, when a majority should have been required. ### Security Considerations Ordinary vote counting issue. ### Scaling Considerations N/A ### Documentation Considerations None ### Testing Considerations Added tests for committee. ### Upgrade Considerations The change is in the committee code. This needs to be merged before the proposal in #10164 runs. It [already installs new committee code](https://github.com/Agoric/agoric-sdk/blob/4a33accaeeba27044ab07dd04f64226de1b77759/packages/builders/scripts/inter-protocol/replace-electorate-core.js#L28).
closes: #10134
refs: #10134
Description
This PR implements the following:
Security Considerations
The Economic Committee plays a crucial role in managing the Inter-Protocol. The current committee doesn't easily support a smooth transition when members must retire or be replaced. An incomplete transition to a new committee could severely hamper the chain operations.
Accidentally providing access to the wrong parties (or failing to remove parties whose terms have expired) could make it hard to ensure that the right committee members have control. Other chains have been sunk or badly damaged when the stakeholders couldn't restrict some people from making changes.
Testing Considerations
Bootstrap tests have been added in this PR to run in CI for testing.
Documentation Considerations
Specific documentation concerns belong in #10138. It’s important to ensure the proposal is clearly documented to avoid confusion, especially with the old committee still being around.
Upgrade Considerations:
There is minimal risk of regression because the core-eval materials are introduced in new files. The changes introduced particularly the new builder script, can be seamlessly included in any upcoming release.
Scaling Considerations
The core eval script’s performance depends on the number of Economic Committee (EC) members. Since the committee is limited to a maximum of 12 members, the script’s complexity is treated as constant because it won’t have to handle more than 12 people.
Also, the script is executed approximately once a year, and all stakers receive a 3-day notice before it runs, making the scaling impact negligible.