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

liquidation penalty proceeds to reserve #5387

Merged
merged 3 commits into from
May 24, 2022
Merged

liquidation penalty proceeds to reserve #5387

merged 3 commits into from
May 24, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented May 17, 2022

closes: #5367

Description

Upon completion of liquidation, push penalty proceeds to the reserve.

Security Considerations

Reserve can now accept RUN anytime from anywhere.

Documentation Considerations

--

Testing Considerations

New test

@turadg turadg requested a review from Chris-Hibbert May 17, 2022 23:20
@turadg turadg linked an issue May 17, 2022 that may be closed by this pull request
Comment on lines 99 to 100
await userSeatPromise;
const deposits = await deposited;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await userSeatPromise;
const deposits = await deposited;
const [deposits] = await Promise.all([userSeatPromise, deposited]);

@@ -45,6 +45,7 @@ const contractRoots = {
liquidate: './src/vaultFactory/liquidateIncrementally.js',
VaultFactory: './src/vaultFactory/vaultFactory.js',
amm: './src/vpool-xyk-amm/multipoolMarketMaker.js',
reserve: './src/reserve/assetReserve.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test that notices that the penalties have been drained to the reserve.

Copy link
Member Author

@turadg turadg May 18, 2022

Choose a reason for hiding this comment

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

Did you see 513f944? (it was 16min before the review posted but I'm guessing the browser window hadn't updated)

@@ -102,6 +102,10 @@ const start = async (zcf, privateArgs) => {

/** @type {ZCFMint} */
const runMint = await zcf.registerFeeMint('RUN', feeMintAccess);
const centralBrand = runMint.getIssuerRecord().brand;
// FIXME second term of tuple must be the liquidity brand
brandForKeyword.init('Central', [centralBrand, centralBrand]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it would be constructive to add it this way. The liquidity keyword can't be replaced once this runs. Should it call addIssuer in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree there's got to be a better way. addIssuer fails however. I'll wait to see what options might work after #5377

@turadg turadg force-pushed the 5367-liq-to-reserve branch from 222b748 to b431916 Compare May 24, 2022 21:08
@turadg turadg marked this pull request as ready for review May 24, 2022 21:09
@turadg turadg requested a review from dtribble as a code owner May 24, 2022 21:09
@turadg turadg requested a review from Chris-Hibbert May 24, 2022 21:09
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label May 24, 2022
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.

LGTM


const makeLiquidityKeyword = keyword => `${keyword}Liquidity`;

const RunKW = 'RUN';
Copy link
Member

Choose a reason for hiding this comment

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

Interesting... I have put my toe in the water with constants for keywords... I'm not entirely happy with the results. let's see how it goes here...

Copy link
Member

Choose a reason for hiding this comment

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

Did you consider something like StableKW where only the value would change when we go to IST?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea and it didn't occur to me. I wouldn't want to do it in just this PR but I'll think about it for #4800

IST seems so central to the package that my intuition is abstracting it would just add cognitive load and provide no benefit. I see it could be beneficial in making the transition easier, but that's a one-shot change that shouldn't burden regular development.

await waitForPromisesToSettle();

const { reserveCreatorFacet } = t.context;
const reserveAllocations = await E(reserveCreatorFacet).getAllocations();
Copy link
Member

Choose a reason for hiding this comment

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

getAllocations is on the creator facet? odd. I wonder why that is.

@turadg turadg dismissed Chris-Hibbert’s stale review May 24, 2022 21:45

confident I addressed the requests

@mergify mergify bot merged commit 0f4e0b5 into master May 24, 2022
@mergify mergify bot deleted the 5367-liq-to-reserve branch May 24, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribute liquidation penalties to the reserve
3 participants