refactor: Move FormatCoins to core
#13306
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13306 +/- ##
==========================================
+ Coverage 54.70% 55.78% +1.07%
==========================================
Files 645 692 +47
Lines 55691 59931 +4240
==========================================
+ Hits 30466 33431 +2965
- Misses 22736 23667 +931
- Partials 2489 2833 +344
|
| // alphabetically by value-rendered denoms. It expects an array of metadata | ||
| // (optionally nil), where each metadata at index `i` MUST match the coin denom | ||
| // at the same index. | ||
| func FormatCoins(coins []*basev1beta1.Coin, metadata []*bankv1beta1.Metadata) (string, error) { |
There was a problem hiding this comment.
autocli actually only needs parsing...
There was a problem hiding this comment.
Ah, ok, let's still get this merged IMO. Then we can add parsing directly here.
|
|
||
| func TestFormatCoins(t *testing.T) { | ||
| var testcases []coinsJsonTest | ||
| raw, err := os.ReadFile("../../tx/textual/internal/testdata/coins.json") |
There was a problem hiding this comment.
This core test imports a tx module's json file, which is not ideal.
We could alternatively:
- test FormatCoins in the
txmodule. Con: unit test not next to its function - move coins.json into core. Con: we'll have some ValueRenderer json files in
tx, others incore
For now the PR's version is my favorite. Any other opinions?
There was a problem hiding this comment.
I would say use go/embed + https://github.com/golang/go/wiki/Modules#can-a-module-depend-on-an-internal-in-another
There was a problem hiding this comment.
What you're doing is also maybe not terrible 🤷
There was a problem hiding this comment.
I am going to keep it like this for now
| coinExp = unit.Exponent | ||
| foundCoinExp = true | ||
| } | ||
| if dispDenom == unit.Denom { |
There was a problem hiding this comment.
Why are we comparing against unit.Denom here? shouldn't it be unit.Display?
There was a problem hiding this comment.
There's no unit.Display, the metadata object looks like
{
"metadata": {
"description": "The native staking token of the Cosmos Hub.",
"denom_units": [
{
"denom": "uatom",
"exponent": 0,
"aliases": [
"microatom"
]
},
{
"denom": "matom",
"exponent": 3,
"aliases": [
"milliatom"
]
},
{
"denom": "atom",
"exponent": 6,
"aliases": [
]
}
],
"base": "uatom",
"display": "atom",
"name": "",
"symbol": ""
}
}This piece of code finds the denom unit object (which contains the exponent) from inside the denom_units array
|
This is R4R again |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if exponentDiff > 0 { | ||
| dispAmount = dispAmount.Mul(math.LegacyNewDec(10).Power(uint64(exponentDiff))) | ||
| } else { | ||
| dispAmount = dispAmount.Quo(math.LegacyNewDec(10).Power(uint64(-exponentDiff))) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
| } | ||
|
|
||
| if exponentDiff > 0 { | ||
| dispAmount = dispAmount.Mul(math.LegacyNewDec(10).Power(uint64(exponentDiff))) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
| return vr + " " + coin.Denom, err | ||
| } | ||
|
|
||
| exponentDiff := int64(coinExp) - int64(dispExp) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
| return vr + " " + coin.Denom, err | ||
| } | ||
|
|
||
| exponentDiff := int64(coinExp) - int64(dispExp) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
| // temporary until we tag a new go module | ||
| replace cosmossdk.io/math => ../math |
There was a problem hiding this comment.
this is kinda annoying...right? make changes, add replace directive, tag, remove replace directive (assuming you don't forget).
Can we think of a better way?
There was a problem hiding this comment.
Agreed. Maybe we can always keep replaces on main, and use tags in release branches? I don't know.
There was a problem hiding this comment.
We shouldn't do separate stuff on release branches. When we have more go.mod's release branches become much less necessary. We should usually be tagging off main.
Ideally we'd have a bot that whenever it sees a merged commit with local replace directives, opens a PR to remove them which we can merge after we tag.
…sdk into am/13155-core-coins
|
I'll go ahead and merge this. @aaronc I'll create the reverse |
|
I'm working on a |
|
Yes! |
Description
ref: #13155
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