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

Implement BigDec binary search in osmoutils #2802

Merged
merged 10 commits into from
Sep 22, 2022
Merged

Implement BigDec binary search in osmoutils #2802

merged 10 commits into from
Sep 22, 2022

Conversation

AlpinYukseloglu
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu commented Sep 20, 2022

Progress towards: #2794 and #2702

What is the purpose of the change

This PR implements a BigDec binary search function for our general osmoutils package.

Brief Changelog

  • Add BinarySearchBigDec to osmoutils
  • Add CompareBigDec to osmoutils
  • Refactor existing and new tests to be more readable (string map names, t.Run etc.)

Testing and Verifying

This change added tests for both new functions in osmoutils/binary_search_test.go

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
  • How is the feature or change documented? (not documented)

osmoutils/binary_search.go Outdated Show resolved Hide resolved
osmoutils/binary_search.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM. What are our plans for the binary search solver in stableswap? Is it going to be used anywhere?

osmoutils/binary_search.go Outdated Show resolved Hide resolved
osmoutils/binary_search_test.go Outdated Show resolved Hide resolved
@AlpinYukseloglu
Copy link
Contributor Author

Yes! After we merge this and #2697 (which has most of our stableswap CFMM tests in it) I will migrate our core solvers to use this implementation @p0mvn

Co-authored-by: Roman <roman@osmosis.team>
AlpinYukseloglu and others added 2 commits September 22, 2022 09:42
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@mergify mergify bot merged commit b6995ab into main Sep 22, 2022
@mergify mergify bot deleted the binary-search-cfmm branch September 22, 2022 17:02
AlpinYukseloglu added a commit that referenced this pull request Sep 22, 2022
@AlpinYukseloglu AlpinYukseloglu restored the binary-search-cfmm branch September 22, 2022 17:04
@AlpinYukseloglu AlpinYukseloglu deleted the binary-search-cfmm branch September 22, 2022 17:10
mergify bot pushed a commit that referenced this pull request Sep 22, 2022
…rch (#2816)

Progress towards: #2794 

## What is the purpose of the change

This PR replaces our stableswap two-asset CFMM solver to use the `BigDec` binary search implementation in #2802 

Note: most of the diff is tests merged from #2697 and the core binary search logic from #2802, both of which had to be merged into this branch

## Brief Changelog

- Replace binary search solver logic with our `BigDec` binary search implementation

## Testing and Verifying

- All two-asset CFMM test cases were set up to test this implementation in `amm_test.go`

## Documentation and Release Note

  - Does this pull request introduce a new feature or user-facing behavior changes? (no)
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? (no)
  - How is the feature or change documented? (not documented)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants