-
Notifications
You must be signed in to change notification settings - Fork 59
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: add support to dec34 #1100
base: main
Are you sure you want to change the base?
Conversation
…acyDec to work with decimal places less than -18
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1100 +/- ##
==========================================
- Coverage 48.13% 47.63% -0.50%
==========================================
Files 892 897 +5
Lines 35069 35744 +675
==========================================
+ Hits 16880 17028 +148
- Misses 16931 17439 +508
- Partials 1258 1277 +19 |
regenmath "github.com/regen-network/regen-ledger/types/v2/math" | ||
) | ||
|
||
type Dec34 regenmath.Dec |
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.
https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/x/group/internal/math/dec.go#L14
Considering the comments above, we shall use wrapper?
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.
@avkr003 regenmath.Dec
is already a wrapper around apd.Decimal
here we are essentially defining a new type that is a regenmath.Dec
and therefore a wrapper of apd.Decimal
as well
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.
@cosmic-vagabond instead of using their library lets copy paste their whole file. I think they did the same from the cosmos SDK / group module or cosmos copy pasted from them. I am guessing there must some reasoning they didn't import but created own data structures (can't think of though, may be easier to migrate later or easier auditing as less exposure to external libraries)
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.
@avkr003 this seems to be a minor change we can come back to based on our priorities, I would be happy to work on it in another PR as this one is quite big and becomes extremely difficult to maintain consider the other changes that gets added to the repo.
import ( | ||
"cosmossdk.io/math" | ||
"github.com/cockroachdb/apd/v2" | ||
regenmath "github.com/regen-network/regen-ledger/types/v2/math" |
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.
necessary to use regen network code? Directly use apd? cosmos in group module (example)
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.
FYI #1100 (comment)
regenmath "github.com/regen-network/regen-ledger/types/v2/math" | ||
) | ||
|
||
type Dec34 regenmath.Dec |
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.
@cosmic-vagabond instead of using their library lets copy paste their whole file. I think they did the same from the cosmos SDK / group module or cosmos copy pasted from them. I am guessing there must some reasoning they didn't import but created own data structures (can't think of though, may be easier to migrate later or easier auditing as less exposure to external libraries)
sdkmath "cosmossdk.io/math" | ||
) | ||
|
||
func OneTokenUnit(decimal uint64) sdkmath.Int { |
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.
The function name isn't obvious. It's returning 10^6 or 10^18 or based on right? Rename to GetPowerOfTen (basically exactly what it does)
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.
Description
What has Changed?
Add support to Dec34 type to handle up to 34 decimal places for price calculation using asset with high decimal precision (e.g. WETH 18 decimals)
Test results
EVM token price (0.004)
Swap estimation retrieves correct price and amount:
Inverse price accurate:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeDeployment Notes
Are there any specific considerations to take into account when deploying these changes? This may include new dependencies, scripts that need to be executed, or any aspects that can only be evaluated in a deployed environment.
Screenshots and Videos
Please provide any relevant before and after screenshots by uploading them here. Additionally, demo videos can be highly beneficial in demonstrating the process.