-
Notifications
You must be signed in to change notification settings - Fork 607
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
Convert sdk.Int to BigDec #6409
Conversation
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change, please see comment
func BigDecFromSDKInt(i Int) BigDec { | ||
return NewBigDecFromBigIntWithPrec(i.BigInt(), 0) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not mutative. See that BigInt()
reinitializes the underlying buffer.
Let's still keep this, please. However, I'm also wondering if we can make a mutative version that would reuse the input's buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see, currently it's hard to access Int.i
directly without BigInt()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make an issue to consider this in the future and potentially talk to the SDK team about exposing such API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue for tracking #17772
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried returning Int.i in my local, performance is improved but the potential risks are unclear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! left a small nit, lgtm when Roman's comment is addressed
@@ -582,6 +582,12 @@ func BigDecFromDec(d Dec) BigDec { | |||
return NewBigDecFromBigIntWithPrec(d.BigInt(), PrecisionDec) | |||
} | |||
|
|||
// BigDecFromSDKInt returns the BigDec representation of an sdkInt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we do not reference sdk anymore, Int is defined in osmomath package
// BigDecFromSDKInt returns the BigDec representation of an sdkInt. | |
// BigDecFromSDKInt returns the BigDec representation of an Int. |
@hieuvubk please add a changelog entry and create an issue. Thanks! |
Marking as draft until the requests are resolved |
@hieuvubk could you investigate the e2e failure please? I tried rerunning it a few times, and it didn't work. From logs, unclear what the issue is so might require investigations |
I think it's due to a version conflict of |
Please run:
Should update the tag to the one taken from a commit on your branch. You should see updates in |
thanks @p0mvn it works! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more request before merge please @hieuvubk
// TODO (perf): consider better conversion helpers to minimize reallocations. | ||
amountBigDec := osmomath.BigDecFromDec(amount.ToLegacyDec()) | ||
amountBigDec := osmomath.BigDecFromSDKInt(amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep these comments please.
Do you mint creating an issue for this osmosis side and linking it to cosmos/cosmos-sdk#17772?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…-labs/osmosis into hieu/bigdec_from_sdk_int
* refactor/test(CL): Stricter rounding behavior in CL math methods; unit tests at low price level * comment updates * convert int => bigdec instead of int => dec => bigdec * tests * Liquidity1 lack * add changelog endpoint * update go mod * keep comments * go mod * fix * Update CHANGELOG.md --------- Co-authored-by: Roman <[email protected]> (cherry picked from commit e2b521d) # Conflicts: # CHANGELOG.md # go.mod # go.sum # osmomath/decimal_test.go # x/concentrated-liquidity/math/math.go
* Convert sdk.Int to BigDec (#6409) * refactor/test(CL): Stricter rounding behavior in CL math methods; unit tests at low price level * comment updates * convert int => bigdec instead of int => dec => bigdec * tests * Liquidity1 lack * add changelog endpoint * update go mod * keep comments * go mod * fix * Update CHANGELOG.md --------- Co-authored-by: Roman <[email protected]> (cherry picked from commit e2b521d) # Conflicts: # CHANGELOG.md # go.mod # go.sum # osmomath/decimal_test.go # x/concentrated-liquidity/math/math.go * resolve conflict * go mod tidy * update go mod * Update CHANGELOG.md --------- Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: roman <[email protected]>
Closes: #6370
What is the purpose of the change
Create a func
BigDecFromSDKInt
to convert sdk.Int => BigDec instead of converting sdk.Int => sdk.Dec => BigDecBenchmark:
Testing and Verifying
(Please pick one of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)