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

refactor(osmomath/cl): avoid using broken power function; panic on broken values #5000

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 25, 2023

Addresses: #4967

What is the purpose of the change

Addresses #4967 in the context of concentrated liquidity. Keeping #4967 open until completely resolved in the future

The osmomath.Power function is broken for base < 1. The reason is that the logarithm function is
negative between zero and 1, and the Exp2(k) is undefined for negative k.
As a result, this function panics if called for base < 1 as it is implemented by Exp2(base.LogBase2().Mul(exponent))

Brief Changelog

  • replace Power function with PowerInteger in `x/concentrated-liquidity module
  • panic in osmomath.Power foe base < 1

Testing and Verifying

  • added test

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? yes
  • How is the feature or change documented? not applicable

@p0mvn p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Apr 25, 2023
@p0mvn p0mvn marked this pull request as ready for review April 25, 2023 12:28
@p0mvn p0mvn requested a review from czarcas7ic as a code owner April 25, 2023 12:28
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

This LGTM, but we still need to replace the osmomath hash in go.mod

Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from roman/fix-inverse-relationship-1 to main April 25, 2023 19:35
@p0mvn p0mvn merged commit 9d3ef25 into main Apr 25, 2023
@p0mvn p0mvn deleted the roman/power-fn-fix branch April 25, 2023 20:11
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants