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

feat(osmomath): mutative and efficient BigDec truncations with arbitrary decimals #6257

Closed
p0mvn opened this issue Aug 31, 2023 · 4 comments · Fixed by #6261
Closed

feat(osmomath): mutative and efficient BigDec truncations with arbitrary decimals #6257

p0mvn opened this issue Aug 31, 2023 · 4 comments · Fixed by #6261
Assignees

Comments

@p0mvn
Copy link
Member

p0mvn commented Aug 31, 2023

Background

Currently, our SDKDec() method of BigDec does a lot of reallocations. Additionally, this function only truncates to sdk.Dec with precision of 18.

Suggested Design

We should have a way to truncate an arbitrary number of digits. The number of reallocations should be minimized. Mutative and non-mutative versions should be implemented.

See chopPrecisionAndTruncate function for reference. Also, see how chopPrecisionAndRoundUpNonMutative and chopPrecisionAndRoundUp are implemented. Clean up and enhance abstractions where applicable.

Acceptance Criteria

@pysel
Copy link
Member

pysel commented Sep 4, 2023

hey @p0mvn. I have a question on this: wdym mutative and non-mutative versions? do you mean SDKDec should mutate it's receiver parameter?

@p0mvn
Copy link
Member Author

p0mvn commented Sep 4, 2023

Hey. For reference, please see Mul and MulMut methods defined on the sdk.Decimal

The benefit of the mutative version is that it doesn't reallocate and mutates the receiver directly. However, it cannot be used always. As a result, we need two versions for most math methods

@pysel
Copy link
Member

pysel commented Sep 4, 2023

in cases of mutative Mul, Quo, Add, and etc, both receiver and the return value have same types (sdk.Dec in case of Mul, for example). in the case of SDKDec function, the receiver is of type BigDec and the output is sdk.Dec, hence, we cannot set the latter to the former.

we could set the underlying *big.Int of the latter to the former, but I suppose that it breaks the point of osmomath.BigDec -> sdk.Dec conversion. if this is the desired approach, please, lmk! thanks

cc: @p0mvn

@p0mvn
Copy link
Member Author

p0mvn commented Sep 4, 2023

osmomath.BigDec is 36 decimals. sdk.Dec is 18 decimals.

To make the mutative SDKDec version defined on the osmomath.BigDec, we need to make it reuse the first 18 decimals of the original buffer and truncate the rest.

There should be a warning made that it makes the original osmomath.BigDec unusable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants