cosmos-sdk: Decimal Multiplication/Quo Bug

@alexanderbez @ValarDragon using another library would not have helped!

Oh, yes, we’re losing precision because accum is less than 1. The cited example in the original post is misleading… the error is in the least significant decimal digit, not the integer digit.

  
import "fmt"
import "github.com/cosmos/cosmos-sdk/types"

func main() {
    a := types.NewDecWithPrec(7470, 10)
    b := types.NewDecWithPrec(4222968745, 10)
    c := types.NewDecWithPrec(4222968745, 10)
    d1 := a.Mul(b)
    d := a.Mul(b).Quo(c)
    fmt.Println(a, b, c, d1, d)
}

>> 0.0000007470 0.4222968745 0.4222968745 0.0000003155 0.0000007471

In retrospect this isn’t surprising… the error happens upon multiplication since we’re clipping decimal digits, and then when we divide by a number ~1 or < 1, the error doesn’t get “buried”, and if << 1 it’ll get amplified.

Makes sense but scary!

I imagine we should probably build this into a function within the decimal library and this may come up again.

Seconded, this is not obvious at all just reviewing code, we should encapsulate this kind of protection directly in the library rather than case-by-case in callers.

I do agree with in general leveraging more open source software. However just because something exists, doesn’t mean its good. In this case, I actually don’t find this library great for our use case, especially relative to what we have. We’d have to fork it anyway to add MarshalAmino functions 😕 I do think it would have been a good idea to use if we didn’t already build the more tailored decimal impl.

It has one global variable for precision: https://github.com/shopspring/decimal/blob/master/decimal.go#L45. Theres no benchmarks on any of the meaningful functions. (w.r.t. to what we’d use) It is fundamentally designed for decimals with variable precision, our use case is many decimals with the same precision. I am fairly confident ours will be more efficient for that reason. In the future, I think we should strive for less re-allocation (e.g. big int style) as we re-allocate all over the place, a decision this library chose to not do for ease of usage. (I’d ideally like to aim for a big-int pool / decimal pool) I also think utilizing SIMD would be cool for the operations. (Our 256 bit decimals fit perfectly into AVX2 SIMD registers, the underlying golang big int takes a perf hit by not getting to know max word size before the computation.)

Back to this library, I fail to see what it does that our implementation doesn’t already do. We already have something thats been tailored to our use case, I don’t see a reason to switch to the more general case. IMO, that libraries core benefit would be implementations of functions we don’t use atm. (Different rounds, math funcs, etc.) but that seems like bloat for the sdk type impl.

Thats an interesting edge case rigel! I’ll take a look tomorrow.

@rigelrozanski That list is the #prelaunch tag, so its in the right place.

I think we should move the relevant code to the library instead of requiring that callers handle it.

I was just listing an example @ValarDragon…I can’t recall us actually looking at other libraries.