-
Notifications
You must be signed in to change notification settings - Fork 60
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 47.97% 47.49% -0.49%
==========================================
Files 895 900 +5
Lines 35132 35800 +668
==========================================
+ Hits 16854 17002 +148
- Misses 17016 17518 +502
- Partials 1262 1280 +18 |
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)
x/amm/types/one_token_unit.go
Outdated
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.
} | ||
|
||
func (k Keeper) GetAssetPriceFromDenom(ctx sdk.Context, denom string) sdkmath.LegacyDec { | ||
func (k Keeper) GetAssetPriceFromDenom(ctx sdk.Context, denom string) (elystypes.Dec34, uint64) { |
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.
Why sending decimal and divide by 10 to the power of that decimal later. Why can't we multiply just price x 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.
@avkr003 can you clarify your thought here, lets take this function as an example:
func (k Keeper) GetTokenPrice(ctx sdk.Context, tokenInDenom, baseCurrency string) (elystypes.Dec34, uint64) {
oraclePrice, decimals := k.oracleKeeper.GetAssetPriceFromDenom(ctx, tokenInDenom)
if oraclePrice.IsPositive() {
return oraclePrice, decimals
}
// Calc tokenIn / uusdc rate
tokenUsdcRate := k.EstimatePrice(ctx, tokenInDenom, baseCurrency)
usdcDenomPrice, decimals := k.oracleKeeper.GetAssetPriceFromDenom(ctx, baseCurrency)
if usdcDenomPrice.IsZero() {
usdcDenomPrice = elystypes.OneDec34()
}
return tokenUsdcRate.Mul(usdcDenomPrice), decimals
}
how would you change the signature of GetAssetPriceFromDenom
to make it work with this function?
thanks
x/tier/keeper/portfolio.go
Outdated
edenDenomPrice := k.amm.GetEdenDenomPrice(ctx, baseCurrency) | ||
totalVesting = totalVesting.Mul(edenDenomPrice) | ||
edenDenomPrice, decimals := k.amm.GetEdenDenomPrice(ctx, baseCurrency) | ||
totalVesting = totalVesting.Mul(edenDenomPrice).QuoInt(ammtypes.OneTokenUnit(decimals)) |
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.
id eden price has enough decimals to hold the accuracy, then why can't we just do totalVesting.Mul(edenDenomPrice). At some places we are not dividing by ammtypes.OneTokenUnit(decimals)
Trying to understand why
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 the division
operation was moved away from GetAssetPriceFromDenom
that’s you can see this division happening here, the division
operation was moved because in some places the function GetAssetPriceFromDenom
was used where there is division that is expected
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.