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

change price request parameter order in multipoolAutoswap for consistency #2267

Closed
Chris-Hibbert opened this issue Jan 26, 2021 · 5 comments
Closed
Assignees
Labels
hygiene Tidying up around the house Zoe Contract Contracts within Zoe

Comments

@Chris-Hibbert
Copy link
Contributor

What is the Problem Being Solved?

#2166 adds two methods to multipoolAutoswap's publicFacet for getting prices: getPriceGivenRequiredOutput, getPriceGivenAvailableInput. The order of their parameters is inconsistent with the pre-existing methods: getInputPrice, getOutputPrice. The original methods were consistent about amount followed by brand, while the new ones are consistent on input then output. We'd like to migrate the old methods to be consistent with the new ones. We'll never have fewer users to migrate than we do now.

  getOutputForGivenInput(amountIn, brandOut) => Amount
  getInputForGivenOutput(amountOut, brandIn) => Amount
  getPriceGivenRequiredOutput(brandIn, amountOut) => { AmountIn, AmountOut }
  getPriceGivenAvailableInput(amountIn, brandOut) => { AmountIn, AmountOut }

Description of the Design

The end state is two methods that return just an amount, and two that return { AmountIn, AmountOut }. It will take more steps to end up with the original methods having the same names and different signatures. If we can find reasonable names to replace them, then the migration path is

1 add new methods, deprecate old names
2 wait
3 remove the old methods

My suggestion would be getOutputGivneInput() and getInputGivenOutput().

Security Considerations

The goal is to reduce confusion. There shouldn't be other security considerations.

Test Plan

Duplicate some tests during the migration period, then remove them before Beta.

@Chris-Hibbert Chris-Hibbert added enhancement New feature or request Beta hygiene Tidying up around the house Zoe Contract Contracts within Zoe and removed enhancement New feature or request labels Jan 26, 2021
@Chris-Hibbert Chris-Hibbert added this to the Beta Launch milestone Jan 26, 2021
@Chris-Hibbert Chris-Hibbert self-assigned this Jan 26, 2021
@dtribble
Copy link
Member

dtribble commented Jan 27, 2021

We also overload some offer attributes as parameters. In particular, vault liquidation requires "sell collateral up to the amount needed to pay the debt", but if that's not enough, it should accept the lower amount. Because that's also the want, it must reject the offer if the collateral isn't worth enough. (ping @katelynsills and @erights for the general Zoe api pattern question)

@katelynsills
Copy link
Contributor

katelynsills commented Jan 27, 2021

Hi @dtribble, can you restate the general Zoe api pattern question? I'm not sure which question that was.

@rowgraus rowgraus removed this from the Beta Initial Launch milestone Mar 29, 2021
@rowgraus
Copy link

@dtribble @Chris-Hibbert

Is this issue still relevant after the work that went in prior to Beta release? I am moving it to Product Backlog for now but lmk if we can close it.

@Chris-Hibbert
Copy link
Contributor Author

We also overload some offer attributes as parameters. In particular, vault liquidation requires "sell
collateral up to the amount needed to pay the debt", but if that's not enough, it should accept the lower amount.

See also #1673 (comment)

In order for the vault to liquidate in one request (the current approach requires two requests), we'd need a new request type in the AMM. That would require disregarding offer safety, since the fallback transaction is to sell whatever remains for whatever we can get.

"The general Zoe API pattern question" is about how to pass parameters into contracts with requests like this. The only inputs one gets with an offer are the proposal and the terms, but those are both binding. In order to create the new AMM liquidation request proposed above, the seller has to be able to say "I want at least this much payout, but if the collateral isn't worth that much, then sell everything." That doesn't fit in the proposal, since that's the basis for enforcing offer safety. We could add something to the request for the invitation that would insert the desired proceeds into the terms, so the contract could know that parameter. It wouldn't be enforced with offer safety, but that's what we want in this case.

@katelynsills I think @dtribble has a notion that there might be a more general way to support this kind of parameter in constructing or exercising offers.

Is this issue still relevant after the work that went in prior to Beta release? I am moving it to Product Backlog for now but lmk if we can close it.

Separately from the issues above, no this hasn't been handled yet. I would categorize it as refactoring for developer ease-of-use. It doesn't add new functionality.

@Chris-Hibbert
Copy link
Contributor Author

  • The problem in the title is now moot since multipoolAutoswap was removed in the migration to RUN-protocol.
  • The API issue was resolved with the addition of private args in Zoe
  • We haven't solved the problem of "sell collateral up to the amount needed to pay the debt unless that's not enough, when it should accept the lower amount"

erights added a commit that referenced this issue May 6, 2024
erights added a commit that referenced this issue May 6, 2024
erights added a commit that referenced this issue May 7, 2024
erights added a commit that referenced this issue May 8, 2024
erights added a commit that referenced this issue May 8, 2024
erights added a commit that referenced this issue May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hygiene Tidying up around the house Zoe Contract Contracts within Zoe
Projects
None yet
Development

No branches or pull requests

4 participants