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

Publish Reserve econ data: mint & burn #5753

Merged
merged 3 commits into from
Jul 13, 2022
Merged

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #4651

Description

Publish key economic data (minting and burning) from the Reserve contract.

Security Considerations

publishing data that should be public. No security concerns.

Documentation Considerations

None.

Testing Considerations

Tested that the values were published.

@Chris-Hibbert Chris-Hibbert added enhancement New feature or request Inter-protocol Overarching Inter Protocol labels Jul 12, 2022
@Chris-Hibbert Chris-Hibbert added this to the Mainnet 1 milestone Jul 12, 2022
@Chris-Hibbert Chris-Hibbert requested a review from turadg as a code owner July 12, 2022 21:54
@Chris-Hibbert Chris-Hibbert self-assigned this Jul 12, 2022
Comment on lines 28 to 29
* @property {Amount<'nat'>} mintedRUN total RUN minted to date
* @property {Amount<'nat'>} burnedRUN total RUN burned to date
Copy link
Member

Choose a reason for hiding this comment

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

let's use Fee instead of RUN per #4800

and per

// Metrics naming scheme: nouns are present values; past-participles are accumulative.
minted should be second.

"total" helps convey that this is "minted ever" not any other time window.

Suggested change
* @property {Amount<'nat'>} mintedRUN total RUN minted to date
* @property {Amount<'nat'>} burnedRUN total RUN burned to date
* @property {Amount<'nat'>} totalFeeMinted total Fee minted to date
* @property {Amount<'nat'>} totalFeeBurned total Fee burned to date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 227 to 228
let mintedRUN = AmountMath.makeEmpty(runBrand);
let burnedRUN = AmountMath.makeEmpty(runBrand);
Copy link
Member

Choose a reason for hiding this comment

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

just noting the variable names should continue to match the metric names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 105 to 110
export const reserveInitialState = emptyRun => ({
allocations: {},
shortfallBalance: emptyRun,
burnedRUN: emptyRun,
mintedRUN: emptyRun,
});
Copy link
Member

Choose a reason for hiding this comment

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

this is valid only for test-reserve.js so please keep it there

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 hoisted it when the vault test needed it as well. I didn't want the vault test to have to be updated if we add more to the record. I didn't see a better place to put it, and metrics.js is at least under test.

Copy link
Member

Choose a reason for hiding this comment

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

ah, my mistake that it's used only in test-reserve.js.

@@ -316,6 +325,7 @@ const start = async (zcf, privateArgs) => {
}),
);
zcf.reallocate(offerToSeat, collateralSeat);
updateMetrics();
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed here but not after burnedRUN change?

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 is already called in burnRUNToReduceShortfall, because that calls increaseLiquidationShortfall. addLiquiditytoAmmPool mints, but doesn't adjust the shortfall. The shortfall is increased by other contracts that get a shortfallReportingInvitation.

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label Jul 12, 2022
@mergify mergify bot merged commit 39fe092 into master Jul 13, 2022
@mergify mergify bot deleted the 4651-reserveNotification branch July 13, 2022 01:01
turadg pushed a commit that referenced this pull request Jul 14, 2022
* feature: publish Reserve econ data: mint & burn

closes: #4651

* chore: standardize names

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge enhancement New feature or request Inter-protocol Overarching Inter Protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose key Reserve Contract economic data via subscription
2 participants