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

4968 psm econ data #6013

Merged
merged 11 commits into from
Aug 22, 2022
Merged

4968 psm econ data #6013

merged 11 commits into from
Aug 22, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Aug 19, 2022

closes: #4968

Description

See curated commit history.

Security Considerations

--

Documentation Considerations

Updated README

Testing Considerations

Added unit tests. To cover startPSM I also tested manually with cosmic-swingset chain,

❯ make scenario2-setup scenario2-run-chain-economy

❯ agd query vstorage keys published.psm.IST.AUSD
children:
- governance
- metrics
pagination: null

❯ agoric follow :published.psm.IST.AUSD.metrics
SES Lockdown using options from environment variables "LOCKDOWN_ERROR_TAMING"
{
  anchorPoolBalance: {
    brand: slot(0,"Alleged: AUSD brand"),
    value: 0n,
  },
  feePoolBalance: {
    brand: slot(1,"Alleged: IST brand"),
    value: 0n,
  },
  totalAnchorProvided: {
    brand: slot(0),
    value: 0n,
  },
  totalStableProvided: {
    brand: slot(1),
    value: 0n,
  },
}

@turadg turadg linked an issue Aug 19, 2022 that may be closed by this pull request
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 didn't look closely at the test driver. I'll submit this request while I'm looking that over.

const updateMetrics = () => {
metricsPublication.updateState(
harden({
poolBalance: anchorPool.getAmountAllocated('Anchor', anchorBrand),
Copy link
Contributor

Choose a reason for hiding this comment

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

That's all we need to recover the state, but the econ committee will also want to know about flows through the system. We should track the amounts that passed through giveStable and wantStable as well.

Copy link
Member

Choose a reason for hiding this comment

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

@rowgraus @dtribble @btulloh it would be good to get your input on this.

@turadg turadg requested a review from Chris-Hibbert August 20, 2022 15:28
@turadg turadg force-pushed the 4968-psm-econ-data branch from dba9086 to 34863ab Compare August 20, 2022 15:28
@dtribble
Copy link
Member

Everything up to the test file lgtm

@turadg turadg force-pushed the 4968-psm-econ-data branch from 34863ab to cbb6b62 Compare August 21, 2022 15:45
@turadg turadg force-pushed the 4968-psm-econ-data branch from cbb6b62 to 846017a Compare August 21, 2022 16:24
@turadg turadg force-pushed the 4968-psm-econ-data branch from 846017a to 6a5d862 Compare August 21, 2022 16:26
@turadg turadg marked this pull request as ready for review August 21, 2022 16:26
@turadg turadg requested a review from dckc August 21, 2022 22:49
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.

Code looks good (by-commit organization reads nicely).

Re design/requirements #4639 doesn't mention the PSM, and #6009 still says "TBD". I wonder if @dtribble was reviewing from that angle. The example output in the PR description is quite handy, meanwhile.

harden({
feeMintAccess,
initialPoserInvitation,
storageNode: mockChainStorage.makeChildNode('thisPsm'),
Copy link
Member Author

Choose a reason for hiding this comment

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

in startPSM the node path is psm.<stable>.<anchor>. There was no no unit test of startPSM and this PR doesn't add one. So the test plan includes manually testing on-chain, which is fuller coverage.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; that's nice...

It prompted me to wonder about other places where our code might assume the PSM, as if there were only one. For example, E(home.agoricNames).lookup('instance', 'psm').

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.

with input from @btulloh , this seems good to go

@turadg turadg dismissed Chris-Hibbert’s stale review August 22, 2022 15:42

requested change performed

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 22, 2022
@mergify mergify bot merged commit e2c1d94 into master Aug 22, 2022
@mergify mergify bot deleted the 4968-psm-econ-data branch August 22, 2022 16:53
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.

Expose key PSM economic data via subscription
4 participants