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

fix: improve dust handling in EstimateTradeBasedOnPriceImpact #6769

Merged

Conversation

migueldingli1997
Copy link
Contributor

@migueldingli1997 migueldingli1997 commented Oct 26, 2023

What is the purpose of the change

We've noticed that the EstimateTradeBasedOnPriceImpact introduced in #6167 can error when we're estimating a dust trade against a balancer pool. This happens especially if the weights of the base and quote assets are equal, meaning that for a small enough input, the pool might only be able to give out <1 tokens, which is rounded to zero and triggers an ErrInvalidMathApprox.

This PR adds logic to look out for ErrInvalidMathApprox in the case of balancer pools, in which case this error is tolerated and a zero output is returned instead of the error. Some refactoring to reduce code duplication was also applied.

Testing and Verifying

This change added tests and can be verified as follows:

  • Added unit test case that specifically checks that we can estimate the trade output from a 1 token input into a balancer pool.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@migueldingli1997 migueldingli1997 changed the title Estimate trades query patch fix: improve dust handling in EstimateTradeBasedOnPriceImpact Oct 26, 2023
@github-actions github-actions bot added C:docs Improvements or additions to documentation C:CLI C:x/poolmanager labels Oct 26, 2023
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Good catch.

I think there are two components of this PR.

  1. Err checking if err returned from calculation is ErrInvalidMathApprox, if so graceful returning zero instead of treating this as error
  2. Refactoring functions in router.go with wrapper functions that create response (e.g ZeroEstimateTradeBasedOnPriceImpactResponseOnErrInvalidMathApprox, ZeroEstimateTradeBasedOnPriceImpactResponseFromRequest)

I am very much in favor of part 1, but I think the new methods introduced with wrapper functions that calls another function to create response struct can be overly confusing compared to directly creating and returning response struct in router.go. Please take a look at let me know what you think! :)

CHANGELOG.md Outdated Show resolved Hide resolved
x/poolmanager/router.go Show resolved Hide resolved
@github-actions github-actions bot removed the C:CLI label Oct 27, 2023
@migueldingli1997
Copy link
Contributor Author

@mattverse I removed the confusing helper functions sir

@mattverse mattverse added the V:state/breaking State machine breaking PR label Oct 27, 2023
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM

Note to other reviewers: This change is state breaking due to us having estimate query for stargate query

@migueldingli1997
Copy link
Contributor Author

@mattverse is this pending another approval or can it be merged?

@mattverse
Copy link
Member

@migueldingli1997 Just waiting to see if any other folks are willing to review this. Planning to merge this by EOW if not

@mattverse mattverse merged commit 5702bc7 into osmosis-labs:main Nov 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Improvements or additions to documentation C:x/poolmanager V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants