types: Dec.MarshalJSON now marshals as a normal decimal string #2506
Conversation
ValarDragon
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
|
This is failing because we for some reason are MarshalJSON'ing decimals when used as parameter keys. That should be fixed in a separate PR. |
types/decimal.go
Outdated
| bzWDec[len(bz)-10] = byte('.') | ||
| copy(bzWDec[len(bz)-9:], bz[len(bz)-10:]) | ||
| } | ||
| return json.Marshal(string(bzWDec)) |
There was a problem hiding this comment.
All this logic already exists using Decimal.String() - why are we not reusing it?
There was a problem hiding this comment.
Oh woops, didn't see that lol
There was a problem hiding this comment.
Actually I think this implementation is more efficient (albeit harder to grok). I'd actually propose making a follow-up issue to compare efficiencies via benchmarks and converge on using one or the other.
There was a problem hiding this comment.
Yo - If I had to guess I'd say you're correct why don't we:
- Replace the
.String()code with your new updated logic - Use
.String()in JSON Marshalling here - copy the old code into a #postlaunch issue for later benchmarking
:)
how does that sound?
|
Is this R4R? |
|
Its ready to merge once I replace the current string implementation with my (more efficient looking) implementation written here. |
Codecov Report
@@ Coverage Diff @@
## develop #2506 +/- ##
========================================
Coverage 60.28% 60.28%
========================================
Files 150 150
Lines 8589 8589
========================================
Hits 5178 5178
Misses 3067 3067
Partials 344 344 |
…-sdk into dev/change_dec_marshaljson
types/decimal.go
Outdated
| copy(bzWDec[2+(10-len(bz)):], bz) | ||
| } else { | ||
| // len(bz) + 1 to account for the decimal point that is being added | ||
| bzWDec = make([]byte, len(bz)+1) |
There was a problem hiding this comment.
Store len(bz) in a variable
types/decimal.go
Outdated
| bzWDec[0] = byte('0') | ||
| bzWDec[1] = byte('.') | ||
| // set relevant digits to 0 | ||
| for i := 0; i < 10-len(bz); i++ { |
There was a problem hiding this comment.
Store len(bz) in a variable
Closes #2475
This implementation has sub-optimal efficiency, should re-use memory allocations in unmarshalling. However this is only called for UI purports, so not really a big deal if its a tiny bit slower.
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/) - no encoding docs atmAdded entries in
PENDING.mdwith issue #rereviewed
Files changedin the github PR explorerFor Admin Use: