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.

Sign up for GitHub

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

Jump to bottom

osmomath: mutative and efficient BigDec truncations with arbitrary decimals #6261

Merged
merged 25 commits into from
Sep 20, 2023
Merged

osmomath: mutative and efficient BigDec truncations with arbitrary decimals #6261

merged 25 commits into from
Sep 20, 2023

Conversation

pysel
Copy link
Member

@pysel pysel commented Aug 31, 2023 •
edited
Loading

Closes: #6257

What is the purpose of the change

This PR:

  1. Modifies SDKDec() (now just Dec()) conversion function to be less resource-consuming
  2. Allows for arbitrary number of decimals truncations

Performance

The new version of SDKDec is approximately 1.84 times better than the old version in terms of time, and 1.75 times better in terms of the number of allocations:

NEW:
BenchmarkSDKRealloc-8           12195942                90.67 ns/op          224 B/op          4 allocs/op

OLD:
BenchmarkSDKRealloc-8            6852703               166.7 ns/op           328 B/op          7 allocs/op

Testing and Verifying

Tests added

Documentation and Release Note

changelog misc section

@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023 •
edited
Loading

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

@osmosis-labs osmosis-labs deleted a comment from devbot-wizard Aug 31, 2023
@pysel pysel added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Aug 31, 2023
@pysel pysel force-pushed the pysel/bigdec-sdkdec branch 3 times, most recently from 756288c to 0b8dac1 Compare September 4, 2023 14:13
@pysel
Copy link
Member Author

pysel commented Sep 4, 2023

devbot add changelog misc osmomath: mutative and efficient BigDec truncations with arbitrary decimals.

@pysel pysel marked this pull request as ready for review September 5, 2023 14:10
@pysel pysel requested a review from p0mvn September 5, 2023 14:10
@p0mvn
Copy link
Member

p0mvn commented Sep 5, 2023

@pysel please see the last A/C on the issue:

Optimize this to be the first client of efficient variable truncation: #6256 (comment)

Could you try addressing it, please?

p0mvn
p0mvn requested changes Sep 7, 2023
View reviewed changes
osmomath/decimal.go Outdated Show resolved Hide resolved
osmomath/decimal.go Outdated Show resolved Hide resolved
osmomath/decimal.go Outdated Show resolved Hide resolved
osmomath/decimal.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C:simulator Edits simulator or simulations label Sep 11, 2023
@p0mvn p0mvn mentioned this pull request Sep 11, 2023
Closed
@hieuvubk hieuvubk mentioned this pull request Sep 16, 2023
Merged
6 tasks
mattverse
mattverse approved these changes Sep 18, 2023
View reviewed changes
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.

Nice work! Overall LGTM

osmomath/decimal.go Show resolved Hide resolved
x/concentrated-liquidity/math/tick.go Show resolved Hide resolved
osmomath/decimal.go Show resolved Hide resolved
@p0mvn
Copy link
Member

p0mvn commented Sep 18, 2023

This PR is LGTM after the remaining comments are addressed.

Note to self: try running tests before approving

This was referenced Sep 20, 2023
Closed
Open
@p0mvn p0mvn mentioned this pull request Sep 20, 2023
Closed
@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v19.x backport patches to v19.x branch and removed V:state/compatible/no_backport State machine compatible PR, depends on prior breaks labels Sep 20, 2023
p0mvn
p0mvn approved these changes Sep 20, 2023
View reviewed changes
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.

Nice work!

In the future, please add changelog and create issues for unaddressed threads

@p0mvn p0mvn merged commit d5c8abc into main Sep 20, 2023
1 check passed
@p0mvn p0mvn deleted the pysel/bigdec-sdkdec branch September 20, 2023 19:37
mergify bot pushed a commit that referenced this pull request Sep 20, 2023
@pysel @mergify
osmomath: mutative and efficient BigDec truncations with arbitrary de…
2dbd3cb 
…cimals (#6261)

* merge main

* Auto: update go.mod after push to pysel/bigdec-sdkdec that modified dependencies locally

* better doc

* mutative/non-mutative custom precision truncators

* selfrev: better variable name

* use better type

* selfrev: better docs

* selfrev: better error in case of failed test

* use new truncators in CalculatePriceToTick

* use osmomath.PrecisionDec

* supposedly fix e2e

* sdk.Precision -> osmomath.PrecisionDec

* pre-compute precision factors

* add edge case

* fix go mod after merge

* panic if converting to Dec with precsiion bigger than PrecisionDec

* bring back old case, expect panic

* add example to DecWithPrecision

* updates

* godoc updates

* changelog

* uint

* updates

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: roman <roman@osmosis.team>
(cherry picked from commit d5c8abc)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
#	x/epochs/go.mod
#	x/epochs/go.sum
@mergify mergify bot mentioned this pull request Sep 20, 2023
Merged
p0mvn added a commit that referenced this pull request Sep 20, 2023
@p0mvn
osmomath: mutative and efficient BigDec truncations with arbitrary de…
3eba9e9 
…cimals (backport #6261)
p0mvn added a commit that referenced this pull request Sep 20, 2023
@mergify @p0mvn
osmomath: mutative and efficient BigDec truncations with arbitrary de…
992b5b2 
…cimals (backport #6261) (#6470)

* osmomath: mutative and efficient BigDec truncations with arbitrary decimals (backport #6261)

* go.mod

---------

Co-authored-by: roman <roman@osmosis.team>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse mattverse approved these changes

@p0mvn p0mvn p0mvn approved these changes

@nicolaslara nicolaslara Awaiting requested review from nicolaslara nicolaslara is a code owner

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic is a code owner

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu is a code owner

Assignees
No one assigned
Labels
A:backport/v19.x backport patches to v19.x branch C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/epochs V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(osmomath): mutative and efficient BigDec truncations with arbitrary decimals
3 participants
@pysel @p0mvn @mattverse