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: Value Renderers for number and coin #10038

Closed
wants to merge 38 commits into from
Closed
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
2a08eb1
first draft
cyberbono3 Aug 23, 2021
599fadf
second draft
cyberbono3 Aug 24, 2021
4b4ee3a
TestFormatInt pass
cyberbono3 Aug 25, 2021
4231817
WIP
cyberbono3 Aug 26, 2021
f6f698f
WIP
cyberbono3 Aug 30, 2021
00824fb
WIP TestFormatCoin
cyberbono3 Aug 30, 2021
53003ed
WIP all tests pass
cyberbono3 Aug 31, 2021
36ae1e2
WIP debugging TestFormatCoin
cyberbono3 Sep 1, 2021
f32a13c
add arg to DenomQuerierFunc
cyberbono3 Sep 1, 2021
4d9e3bd
WIP
cyberbono3 Sep 1, 2021
06d9ae5
getMetadata
cyberbono3 Sep 1, 2021
ab7d6fb
all tests pass
cyberbono3 Sep 1, 2021
24ae2f4
dvr.QueryDenomMetadata
cyberbono3 Sep 8, 2021
b1d15ff
TestFormatCoin first test case pass
cyberbono3 Sep 8, 2021
0c5e2b1
WIP handle slice of metadatas
cyberbono3 Sep 10, 2021
812bcd2
WIP fix first test case in testFormatCoin
cyberbono3 Sep 10, 2021
5090714
WIP all tests pass
cyberbono3 Sep 11, 2021
700fab9
cleanup
cyberbono3 Sep 12, 2021
ec13baf
r4r
cyberbono3 Sep 15, 2021
b8953f7
fix merge conflict
cyberbono3 Sep 15, 2021
41394fc
add TODOs
cyberbono3 Sep 16, 2021
fa8c8e3
Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into cy…
cyberbono3 Sep 16, 2021
f0b5a1d
WIP
cyberbono3 Sep 18, 2021
784eee9
tests pass
cyberbono3 Sep 20, 2021
ccf9b4d
cleanup
cyberbono3 Sep 20, 2021
dcefbf5
WIP
cyberbono3 Sep 20, 2021
c57115c
fix Parse and tests
cyberbono3 Sep 21, 2021
15992db
update comments
cyberbono3 Sep 21, 2021
03f6041
fix TestFormatDec
cyberbono3 Sep 21, 2021
ed27445
cleanup
cyberbono3 Sep 21, 2021
fa9b039
Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into cy…
cyberbono3 Sep 21, 2021
5c0a6cd
add godoc
cyberbono3 Sep 21, 2021
c8fc3a0
update denomQuerierFunc godoc
cyberbono3 Sep 21, 2021
6860bde
remove float64 from computeExponentSubtraction
cyberbono3 Sep 22, 2021
fbe7e94
apply reviewer comments
cyberbono3 Sep 22, 2021
8c29035
make RegExp global variable
cyberbono3 Sep 22, 2021
1264277
update Parse godoc
cyberbono3 Sep 22, 2021
28b5890
update baseFromDenom
cyberbono3 Sep 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ require (
github.com/cosmos/go-bip39 v1.0.0
github.com/cosmos/iavl v0.17.1
github.com/cosmos/ledger-cosmos-go v0.11.1
github.com/dustin/go-humanize v1.0.0 // indirect
github.com/enigmampc/btcutil v1.0.3-0.20200723161021-e2fb6adb2a25
github.com/gogo/gateway v1.1.0
github.com/gogo/protobuf v1.3.3
Expand Down
206 changes: 206 additions & 0 deletions types/valuerenderer/valuerenderer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
package valuerenderer

import (
"context"
"errors"
"math"
"regexp"
"strconv"
"strings"

"github.com/dustin/go-humanize"
Copy link
Contributor Author

@cyberbono3 cyberbono3 Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found humanize library with 4k github stars.


"github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
)

// ValueRenderer defines an interface to produce formated output for Int,Dec,Coin types as well as parse a string to Coin or Uint.
type ValueRenderer interface {
Format(context.Context, interface{}) (string, error)
Parse(context.Context, string) (interface{}, error)
}

// denomQuerierFunc takes a context and a denom as arguments and returns metadata, error
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's update a comment to something more meaningful. We know the types, instead, describe what the function should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it sounds better: denomQuerierFunc should take context and baseDenom to invoke DenomMetaData method on queryClient. This method will returnbanktypes.Metadata.

type denomQuerierFunc func(context.Context, string) (banktypes.Metadata, error)

// DefaultValueRenderer defines a struct that implements ValueRenderer interface
type DefaultValueRenderer struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to repeat the package name in the struct name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does DefaultRenderer instead of DefaultValueRenderer sound better to you?

denomQuerier denomQuerierFunc
}

var _ ValueRenderer = &DefaultValueRenderer{}

// NewDefaultValueRenderer initiates denomToMetadataMap field and returns DefaultValueRenderer struct
func NewDefaultValueRenderer(denomQuerier denomQuerierFunc) DefaultValueRenderer {
return DefaultValueRenderer{
denomQuerier: denomQuerier,
}
}

// Format converts an empty interface into a string depending on interface type.
func (dvr DefaultValueRenderer) Format(c context.Context, x interface{}) (string, error) {
var sb strings.Builder

switch v := x.(type) {
case types.Dec:
s := v.String()
if len(s) == 0 {
return "", errors.New("empty string")
}

i := strings.Index(s, ".")
first, second := s[:i], s[i+1:]
first64, err := strconv.ParseInt(first, 10, 64)
if err != nil {
return "", err
}

sb.WriteString(humanize.Comma(first64))
sb.WriteString(".")
sb.WriteString(removeTrailingZeroes(second))

case types.Int:
s := v.String()
if len(s) == 0 {
return "", errors.New("empty string")
}

sb.WriteString(humanize.Comma(v.Int64()))

case types.Coin:
metadata, err := dvr.denomQuerier(c, convertToBaseDenom(v.Denom))
if err != nil {
return "", err
}

expSub := computeExponentSubtraction(v.Denom, metadata)

formatedAmount := dvr.ComputeAmount(v.Amount.Int64(), expSub)

sb.WriteString(formatedAmount)
sb.WriteString(metadata.Display)

default:
panic("type is invalid")
}

return sb.String(), nil
}

// TODO address the cass where denom starts with "u"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this TODO still relevant?

Copy link
Contributor Author

@cyberbono3 cyberbono3 Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there is a case, where denom might start with "u", e.g. urobert. In this case this is problematic to convert urobert to baseDenom by invoking convertToBaseDenom(denom string) string, cause baseDenom always start with "u".

func convertToBaseDenom(denom string) string {
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved
switch {
// e.g. uregen => uregen
case strings.HasPrefix(denom, "u"):
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved
return denom
// e.g. mregen => uregen
case strings.HasPrefix(denom, "m"):
return "u" + denom[1:]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m stands for mili. Why are you overwriting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to convert it to banktypes.Metadata.Base which represents the minimum denom and always starts with "u" if I am not mistaken.

// has no prefix regen => uregen
default:
return "u" + denom
}
}

// computeExponentSubtraction iterates over metadata.DenomUnits and computes the subtraction of exponents
func computeExponentSubtraction(denom string, metadata banktypes.Metadata) float64 {
var coinExp, displayExp int64
for _, denomUnit := range metadata.DenomUnits {
if denomUnit.Denom == denom {
coinExp = int64(denomUnit.Exponent)
}

if denomUnit.Denom == metadata.Display {
displayExp = int64(denomUnit.Exponent)
}
}

return float64(coinExp - displayExp)
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved
}

// removeTrailingZeroes removes trailing zeroes from a string
func removeTrailingZeroes(str string) string {
index := len(str)-1
for i := len(str) - 1; i > 0; i-- {
if rune(str[i]) == rune('0') {
continue
}

index = i
break
}

return str[:index+1]
}
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved

// countTrailingZeroes counts the amount of trailing zeroes in a string
func countTrailingZeroes(str string) int {
counter := 0
for i := len(str) - 1; i > 0; i-- {
if rune(str[i]) == rune('0') {
counter++
} else {
break
}
}
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved
return counter
}

// ComputeAmount calculates an amount to produce formated output
func (dvr DefaultValueRenderer) ComputeAmount(amount int64, expSub float64) string {

switch {
// negative , convert mregen to regen less zeroes 23 => 0,023, expSub -3
case math.Signbit(expSub):

stringValue := strconv.FormatInt(amount, 10)
count := countTrailingZeroes(stringValue)
if count >= int(math.Abs(expSub)) {
// case 1 if number of zeroes >= Abs(expSub) 23000, -3 => 23 (int64)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 1 : stringValue : "23000", expSub = -3 => 23 (int64)

x := amount / int64(math.Pow(10, math.Abs(expSub)))
return humanize.Comma(x)
} else {
// case 2 number of trailing zeroes < abs(expSub) 23, -3,=> 0.023(float64)
x := float64(float64(amount) / math.Pow(10, math.Abs(expSub)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 2 : stringValue : "23", expSub = -3 => 0.023 (float64)

return humanize.Ftoa(x)
}
// positive, e.g.convert mregen to uregen
case !math.Signbit(expSub):
x := amount * int64(math.Pow(10, expSub))
return humanize.Comma(x)
// == 0, convert regen to regen, amount does not change
default:
return humanize.Comma(amount)
}
}

// Parse parses a string and takes a decision whether to convert it into Coin or Uint
cyberbono3 marked this conversation as resolved.
Show resolved Hide resolved
func (dvr DefaultValueRenderer) Parse(ctx context.Context, s string) (interface{}, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning an empty interface is a bad idea. Why do we need a single function to work both with numbers and coins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueRenderer interface has been designed by @aaronc

type ValueRenderer interface {
  Format(ctx context.Context, interface{}) (string, error)
  Parse(ctx context.Context, s string) (interface{}, error)
}

Can we split Parse into 2 functions ?

ParseCoin(ctx context.Context, s string) (types.Coin, error)
ParseUint(ctx context.Context, s string) (types.Uint, error)

cc @aaronc @AmauryM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be a better idea.

if s == "" {
return nil, errors.New("unable to parse empty string")
}
// remove all commas
str := strings.ReplaceAll(s, ",", "")
re := regexp.MustCompile(`(\d+)(\w+)`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should compile the regexp once, and store it as a module private variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather make it global since valuerenderer_test package imports that.

// case 1: "1000000regen" => Coin
if re.MatchString(str) {
var amountStr, denomStr string
s1 := re.FindAllStringSubmatch(str, -1) // [[1000000regen 1000000 regen]]
amountStr, denomStr = s1[0][1], s1[0][2]

amount, err := strconv.ParseInt(amountStr, 10, 64)
if err != nil {
return nil, err
}

return types.NewInt64Coin(denomStr, amount), nil
}

// case2: convert it to Uint
i, err := strconv.ParseUint(str, 10, 64)
if err != nil {
return nil, err
}

return types.NewUint(i), nil
}
Loading