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

Add addPool to AMM creatorFacet and reinstate economy boot #5428

Closed
wants to merge 7 commits into from

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented May 25, 2022

refs: #5375

Description

  • Put addPool on the AMM creatorFacet, primarily to un-tie knots in automated bootstrap; not intended for normal production use.
  • Fix economy bootstrap, and add to CI.

Security Considerations

Documentation Considerations

Testing Considerations

@michaelfig michaelfig added the Inter-protocol Overarching Inter Protocol label May 25, 2022
@michaelfig michaelfig requested a review from turadg as a code owner May 25, 2022 05:27
@michaelfig michaelfig force-pushed the dc-addPool-creator branch from 7b457dd to 7f8738e Compare May 25, 2022 05:27
@michaelfig michaelfig requested a review from dckc May 25, 2022 05:27
@@ -291,10 +291,38 @@ const start = async (zcf, privateArgs) => {
}),
);

/** @type {GovernedCreatorFacet<*>} */
const creatorFacet = makeGovernorFacet(
// TODO: pass this into makeAddPoolInvitation
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be done before merge to DRY out the two addPools. They've already diverged in that the other one updates metrics. cc @Chris-Hibbert

zcf.saveIssuer(secondaryIssuer, keyword);
/** @param {ZCFMint} mint */
async mint => {
await zcf.saveIssuer(secondaryIssuer, keyword);
Copy link
Member

Choose a reason for hiding this comment

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

noting #5407 to link this instance to that ticket

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.

This makes it possible to create a new empty pool, violating the requirement for a minimum funding. It also allows/requires the pool to be funded using addLiquidity rather than going through addPoolAndLiquidityHandler, which means that no liquidity will be extracted to the reserve.

@dckc
Copy link
Member

dckc commented May 25, 2022

@Chris-Hibbert , this no-fee addPool method is primarily intended to un-tie knots in automated bootstrap; it's not intended for normal production use. (I clarified the PR description to say as much.)

This makes it possible to create a new empty pool, violating the requirement for a minimum funding.

The motivation for minimum funding was essentially spam prevention, wasn't it? This no-fee addPool method is on the creator facet, which is only available to the BLDer DAO, not to spammers. There's nothing inherently wrong with an empty AMM pool, is there?

It also allows/requires the pool to be funded using addLiquidity rather than going through addPoolAndLiquidityHandler, which means that no liquidity will be extracted to the reserve.

Yes, this is a consideration that anyone with access to the AMM creator facet should be aware of. Is it a reason not to add the feature, though?

@dckc
Copy link
Member

dckc commented May 25, 2022

discussion with @Chris-Hibbert , @dtribble and co suggests:

  • try PSM?
  • try 0 minimum required liquidity?

@dckc
Copy link
Member

dckc commented May 26, 2022

obsolete in favor of #5441

@dckc dckc closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Inter-protocol Overarching Inter Protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants