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

store econ committee questions off-chain #6036

Merged
merged 4 commits into from
Aug 24, 2022
Merged

store econ committee questions off-chain #6036

merged 4 commits into from
Aug 24, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Aug 23, 2022

Description

Publish committee questions to chain storage.

Security Considerations

Gives contracts a Marshaller from the board. It is read-only.

Documentation Considerations

It's a feature of @agoric/governance contracts but also @agoric/inter-protocol. Documented in both READMEs.

Testing Considerations

Unit and swingset tests pass, but they don't cover startEconomicCommittee. So I did a chain test per the "Demo" section of the README. The contract publishes 'undefined' as the first question so the key is present:

❯ agd query vstorage keys 'published.committees'
children:
- Initial_Economic_Committee
pagination: null
❯ agd query vstorage keys 'published.committees.Initial_Economic_Committee'
children:
- latestQuestion
pagination: null
❯ agoric follow :published.committees.Initial_Economic_Committee.latestQuestion
SES Lockdown using options from environment variables "LOCKDOWN_ERROR_TAMING"
undefined

@turadg turadg requested review from dckc and Chris-Hibbert August 23, 2022 19:10
@turadg turadg changed the title ta/store questions store econ committee questions off-chain Aug 23, 2022
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.

I don't see any test output changes showing evidence that questions were ever recorded. Did I miss it somehow? I'm sure there are tests in which questions are asked.

@dckc
Copy link
Member

dckc commented Aug 23, 2022

A test showing a question to the econ committee to pause one side of the PSM would be most welcome!

@Chris-Hibbert
Copy link
Contributor

A test showing a question to the econ committee to pause one side of the PSM would be most welcome!

Well, it turns out that in service of #6007, I'm working on a test showing a governed PSM setting the filter. The existing tests work with a bare PSM that doesn't actually have governance installed AFAICT. (I didn't know that was possible!)

@turadg
Copy link
Member Author

turadg commented Aug 23, 2022

I don't see any test output changes showing evidence that questions were ever recorded. Did I miss it somehow?

Nope. I tried to call this state out in Testing Plan.

I'm sure there are tests in which questions are asked.

If that's sufficient: e91c301

@turadg turadg requested a review from Chris-Hibbert August 23, 2022 22:36
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.

I'm sure there are tests in which questions are asked.

If that's sufficient: e91c301

That's what I was looking for, thanks.

@Tartuffo Tartuffo added the pso label Aug 24, 2022
@turadg turadg force-pushed the ta/store-questions branch from e91c301 to bc336dc Compare August 24, 2022 14:27
@@ -95,16 +95,25 @@ export const startEconomicCommittee = async (
},
{ options: { econCommitteeOptions = {} } = {} },
) => {
const STORAGE_PATH = 'committee';
Copy link
Member

Choose a reason for hiding this comment

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

This suggests it's the only committee. That's true to some extent, but I wonder to what extent we should commit to that. What do you think about published.committee.economicGovernance or just published.committee1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. Updated to be published.committees.Initial_Economic_Committee, sanitizing the committeeName property.

@turadg turadg force-pushed the ta/store-questions branch from bc336dc to e5dc22c Compare August 24, 2022 16:32
@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 24, 2022
@turadg turadg force-pushed the ta/store-questions branch from e5dc22c to 91dec79 Compare August 24, 2022 17:04
@turadg
Copy link
Member Author

turadg commented Aug 24, 2022

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2022

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot merged commit ea1bd99 into master Aug 24, 2022
@mergify mergify bot deleted the ta/store-questions branch August 24, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants