feat: Value Renderers for number and coin #10038
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10038 +/- ##
==========================================
+ Coverage 63.72% 64.13% +0.40%
==========================================
Files 573 576 +3
Lines 53766 54751 +985
==========================================
+ Hits 34263 35115 +852
- Misses 17555 17659 +104
- Partials 1948 1977 +29
|
|
Visit https://dashboard.github.orijtech.com?back=0&pr=10038&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
cyberbono3
force-pushed
the
cyberbono3/value-renderer
branch
from
0328356
to
36ae1e2
Compare
cyberbono3
force-pushed
the
cyberbono3/value-renderer
branch
from
d4d9471
to
4d9e3bd
Compare
cyberbono3
force-pushed
the
cyberbono3/value-renderer
branch
from
4f1b05a
to
ab7d6fb
Compare
|
I suggest the next implementation plan.
Does it sound good to you, @clevinson ? |
|
I'm not sure I understand why we need a CLI method for value renderers. Was that discussed in one of the meetings? I can't seem to see any mention of it in the issue #9955, and don't really understand the use case. From my understanding, all we need to implement in this PR would a Looking at the existing code a few other thoughts- If want to pass in to |
|
This pull request introduces 1 alert when merging 24ae2f4 into 37dbfec - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b1d15ff into 37dbfec - view on LGTM.com new alerts:
|
types/valuerenderer/valuerenderer.go
Outdated
| Parse(context.Context, string) (interface{}, error) | ||
| } | ||
|
|
||
| // denomQuerierFunc takes a context and a denom as arguments and returns metadata, error |
There was a problem hiding this comment.
let's update a comment to something more meaningful. We know the types, instead, describe what the function should do.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
no need to repeat the package name in the struct name.
There was a problem hiding this comment.
does DefaultRenderer instead of DefaultValueRenderer sound better to you?
| return sb.String(), nil | ||
| } | ||
|
|
||
| // TODO address the cass where denom starts with "u" |
There was a problem hiding this comment.
is this TODO still relevant?
There was a problem hiding this comment.
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".
types/valuerenderer/valuerenderer.go
Outdated
| return denom | ||
| // e.g. mregen => uregen | ||
| case strings.HasPrefix(denom, "m"): | ||
| return "u" + denom[1:] |
There was a problem hiding this comment.
m stands for mili. Why are you overwriting this?
There was a problem hiding this comment.
I have to convert it to banktypes.Metadata.Base which represents the minimum denom and always starts with "u" if I am not mistaken.
types/valuerenderer/valuerenderer.go
Outdated
| } | ||
| // remove all commas | ||
| str := strings.ReplaceAll(s, ",", "") | ||
| re := regexp.MustCompile(`(\d+)(\w+)`) |
There was a problem hiding this comment.
we should compile the regexp once, and store it as a module private variable.
There was a problem hiding this comment.
I would rather make it global since valuerenderer_test package imports that.
| } | ||
|
|
||
| // Parse parses a string and takes a decision whether to convert it into Coin or Uint | ||
| func (dvr DefaultValueRenderer) Parse(ctx context.Context, s string) (interface{}, error) { |
There was a problem hiding this comment.
returning an empty interface is a bad idea. Why do we need a single function to work both with numbers and coins?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Yes, that would be a better idea.
| @@ -14,6 +14,8 @@ import ( | |||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | |||
| ) | |||
|
|
|||
| var RegExp = regexp.MustCompile(`(\d+)(\w+)`) | |||
There was a problem hiding this comment.
I have made RegExp global variable since valuerenderer_test package imports that. Does it sound good ?
cyberbono3
force-pushed
the
cyberbono3/value-renderer
branch
from
f0b2b45
to
1264277
Compare
|
|
||
| // TODO address the cass where denom starts with "u" | ||
| // baseFromDenom converts denom to banktypes.Metadata.Base that is used in banktypes.QueryDenomMetadataRequest. queryClient.DenomMetadata method takes Banktypes.QueryDenomMetadataRequest and context as arguments and returns resp.Metadata. | ||
| func baseFromDenom(denom string) (string, error) { |
|
|
||
| // TODO address the cass where denom starts with "u" | ||
| // baseFromDenom converts denom to banktypes.Metadata.Base that is used in banktypes.QueryDenomMetadataRequest. queryClient.DenomMetadata method takes Banktypes.QueryDenomMetadataRequest and context as arguments and returns resp.Metadata. | ||
| func baseFromDenom(denom string) (string, error) { |
There was a problem hiding this comment.
Does name baseFromDenom sound good to you , @robert-zaremba ?
| // TODO address the cass where denom starts with "u" | ||
| // baseFromDenom converts denom to banktypes.Metadata.Base that is used in banktypes.QueryDenomMetadataRequest. queryClient.DenomMetadata method takes Banktypes.QueryDenomMetadataRequest and context as arguments and returns resp.Metadata. | ||
| func baseFromDenom(denom string) (string, error) { | ||
| if len(denom) == 0 { |
There was a problem hiding this comment.
If len(denom) == 0 I yield an error
|
Closing, and let's re-prioritize this work wrt #10251. |
|
@AmauryM - the value render will still be needed for the sign mode textual, so maybe we should still keep the PR open and update once there will be more requirements? |
|
Sure, but i'll put as draft. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@AmauryM can this be worked on or is it blocked? |
|
It's blocked, happy to close this. |
Part of TX working group.
Description
Closes: #9955
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 changeCHANGELOG.mdReviewers 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 change