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

feat: durability for governance #8157

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

Chris-Hibbert
Copy link
Contributor

Not done yet

closes: #8123
refs: #8125
refs: #4340
closes: #5200
closes: #7118
closes: #6138

Description

Make Governance durable, and support upgrade of contracts

Security Considerations

Maintain all governance guarantees

Scaling Considerations

N/A

Documentation Considerations

This probably deserves some new documentation.

Testing Considerations

Tests should show upgrade of a contract

Upgrade Considerations

Yep, that's what it's for.

@Chris-Hibbert Chris-Hibbert self-assigned this Aug 7, 2023
@Chris-Hibbert Chris-Hibbert marked this pull request as draft August 7, 2023 18:03
@Chris-Hibbert Chris-Hibbert force-pushed the 8123-durGovernance branch 2 times, most recently from e7c8050 to 1d5a5d9 Compare August 7, 2023 18:16
@Chris-Hibbert Chris-Hibbert force-pushed the 8123-durGovernance branch 4 times, most recently from e6889e4 to 1781a02 Compare August 23, 2023 17:30
getVisibleValue: M.call(M.any()).returns(M.any()),
prepareToUpdate: M.call(M.any()).returns(M.any()),
update: M.call(M.any()).returns(M.recordOf(KeywordShape, M.any())),
getGetterElement: M.call().returns([M.string(), M.any()]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The M.any() in the return value is a Function. Is there a type for that?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -399,6 +399,12 @@ export const start = async (zcf, privateArgs, baggage) => {
);
},
...publicMixin,
getPublicTopics() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could return another helper from handleParamGovernance that would build getPublicTopics() from something provided by the caller, so getPublicTopics() could just be included in publicMixin. Would that be better?

@Chris-Hibbert Chris-Hibbert force-pushed the 8123-durGovernance branch 2 times, most recently from 8908203 to e84855a Compare August 23, 2023 23:42
const [preparedAmount] = await Promise.all([
E(E(zoe).getInvitationIssuer()).getAmountOf(invite),
]);
assertInvitation(invite);
Copy link
Member

Choose a reason for hiding this comment

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

that's an async function so much be awaited to throw upon failure. I think this is a bug in master too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Chris-Hibbert
Copy link
Contributor Author

@turadg This is my current state. There are a small number (8ish?) of lint errors to be cleaned up. All tests in Zoe, governance, and inter-protocol are passing. Other than checking on what other tests are broken in CI, my next step is working on deployment/docker tests. I haven't done a thorough pass ensuring I cleaned up all the refactorings, but the code is doing the right thing currently, AFAICT.

@Chris-Hibbert Chris-Hibbert force-pushed the 8123-durGovernance branch 3 times, most recently from 8f9233f to b666c7c Compare September 6, 2023 17:20
const [preparedAmount] = await Promise.all([
E(E(zoe).getInvitationIssuer()).getAmountOf(invite),
]);
assertInvitation(invite);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's gone!

All existing tests in governance, zoe, boot, and inter-protocol pass.

deployment tests are next
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants