cosmos-sdk icon indicating copy to clipboard operation
cosmos-sdk copied to clipboard

[Feature]: Why was the new version of decimal type removed?

Open TraderWithPython opened this issue 7 months ago • 8 comments

Summary

I find the new version of decimal type have been removed. why

Problem Definition

No response

Proposed Feature

I need a useful decimal type where the serialized dictionary order is consistent with the numeric value order.

TraderWithPython avatar Apr 13 '25 18:04 TraderWithPython

@TraderWithPython the new type had a lot of issues with serialization and testing and was not production ready. We removed it until we have a better tested / validated way for doing decimal representations. For now, we have the LegacyDec type which represents a fixed point decimal

aljo242 avatar Apr 14 '25 18:04 aljo242

@TraderWithPython the new type had a lot of issues with serialization and testing and was not production ready. We removed it until we have a better tested / validated way for doing decimal representations. For now, we have the LegacyDec type which represents a fixed point decimal

I didn't encounter any issues with the new type during my testing. Could you elaborate on what specific serialization and testing problems you found? I'd like to help solve these issues if possible. The previous LegacyDec implementation was quite problematic to work with.

TraderWithPython avatar Apr 15 '25 09:04 TraderWithPython

What were the issues with the LegacyDec that you found? Right now we are at a point where the whole codebase and all of our customers are using the LegacyDec. From what I understand people have had string rendering issues and have noted that the type may not have sufficient precision for their needs. Are there other issues you're having?

aljo242 avatar Apr 15 '25 14:04 aljo242

What were the issues with the LegacyDec that you found? Right now we are at a point where the whole codebase and all of our customers are using the LegacyDec. From what I understand people have had string rendering issues and have noted that the type may not have sufficient precision for their needs. Are there other issues you're having?

The first issue I encountered with LegacyDec is that it is prone to overflow during intermediate steps in some of my calculations, which can result in values exceeding its limits.

The second issue is related to serialization: when I use a serialized LegacyDec as a key and iterate over these keys, their dictionary order does not correspond to their numeric order.

TraderWithPython avatar Apr 15 '25 23:04 TraderWithPython

The second issue is related to serialization: when I use a serialized LegacyDec as a key and iterate over these keys, their dictionary order does not correspond to their numeric order.

I wouldn't expect that the new Dec would address these issues. The current LegacyDec sorts things as strings so if you have 900,1000 that will lexically get sorted as 1000,900. The KV store only supports lexical string sorting so in order to create a serialization that maintains numerical sort order we would either need to use a binary format which creates problems when we do proto JSON serialization or carefully construct some string serialization which maintains the numerical sort order. I'm not quite sure how we would do the latter to be honest so I wouldn't actually expect we would create an official decimal type that does this. You, of course, could create a custom type for your application that has this property at the expense of human readable JSON.

Regarding overflow issues, I would expect that the new Dec addresses these. However, because LegacyDec is based on big.Int, I'm a bit surprised that you're running into this. Could you share more details?

aaronc avatar Apr 16 '25 15:04 aaronc

The second issue is related to serialization: when I use a serialized LegacyDec as a key and iterate over these keys, their dictionary order does not correspond to their numeric order.

I wouldn't expect that the new Dec would address these issues. The current LegacyDec sorts things as strings so if you have 900,1000 that will lexically get sorted as 1000,900. The KV store only supports lexical string sorting so in order to create a serialization that maintains numerical sort order we would either need to use a binary format which creates problems when we do proto JSON serialization or carefully construct some string serialization which maintains the numerical sort order. I'm not quite sure how we would do the latter to be honest so I wouldn't actually expect we would create an official decimal type that does this. You, of course, could create a custom type for your application that has this property at the expense of human readable JSON.

Regarding overflow issues, I would expect that the new Dec addresses these. However, because LegacyDec is based on big.Int, I'm a bit surprised that you're running into this. Could you share more details?

I think there are two points that need clarification.

  1. Regarding the overflow issue: Indeed as you noted, LegacyDec defines an upper limit, specifically here: https://github.com/cosmos/cosmos-sdk/blob/main/math/legacy_dec.go#L36 When intermediate calculation values exceed this limit, overflow occurs. Despite using big.Int underneath, this restriction still applies.

  2. Regarding serialization and sorting issues: It's actually possible to achieve both numerical sorting and readability. This requires:

    • Maintaining human-readable JSON serialization (MarshalJSON method: https://github.com/cosmos/cosmos-sdk/blob/main/math/legacy_dec.go#L802)
    • While modifying Protobuf serialization (Marshal method) to output a binary format that sorts numerically

This way, KV store sorting would follow numerical order, while JSON representation remains human-readable. This is one of the greatest advantages of gogoproto's custom type - the ability to implement different serialization strategies for different scenarios.

TraderWithPython avatar May 03 '25 07:05 TraderWithPython

Thanks for pointing out the upper limit restriction.

Yes, we could use gogoproto for custom JSON marshaling, but this is often problematic because it breaks the ProtoJSON spec and other serializers have trouble reproducing the same behavior. But, to the extent that we want the SDK to have custom JSON marshaling (not ProtoJSON compatible), you are correct - we could have human-readable JSON and numerically sorted binary.

aaronc avatar May 05 '25 14:05 aaronc

Thanks for pointing out the upper limit restriction.

Yes, we could use gogoproto for custom JSON marshaling, but this is often problematic because it breaks the ProtoJSON spec and other serializers have trouble reproducing the same behavior. But, to the extent that we want the SDK to have custom JSON marshaling (not ProtoJSON compatible), you are correct - we could have human-readable JSON and numerically sorted binary.

Just need to modify this section of the code. https://github.com/cosmos/cosmos-sdk/blob/f213a2ee3296ae213282621d5fb70b8fb05cb85d/math/legacy_dec.go#L836C1-L885C2

Change it as shown below.

// Marshal implements the gogo proto custom type interface.
func (d LegacyDec) Marshal() ([]byte, error) {
    i := d.i
    if i == nil {
        i = new(big.Int)
    }
    
    // Use the ordered encoding instead of the default MarshalText
    result := new(bytes.Buffer)
    
    // Write sign byte: 1 for positive or zero, 0 for negative
    var sign byte = 1
    if i.Sign() < 0 {
        sign = 0
    }
    result.WriteByte(sign)
    
    // Get the absolute value bytes
    absI := new(big.Int).Abs(i)
    rawBytes := absI.Bytes()
    
    // Write length (fixed 1 byte, supports integers up to 255 bytes)
    if len(rawBytes) > 255 {
        return nil, errors.New("integer too large, exceeds 255 bytes")
    }
    result.WriteByte(byte(len(rawBytes)))
    
    // Write content (bit-inverted for negative numbers)
    if i.Sign() < 0 {
        // For negative numbers, invert each byte
        inverted := make([]byte, len(rawBytes))
        for j, b := range rawBytes {
            inverted[j] = ^b
        }
        result.Write(inverted)
    } else {
        // For positive or zero, write directly
        result.Write(rawBytes)
    }
    
    return result.Bytes(), nil
}

// MarshalTo implements the gogo proto custom type interface.
func (d *LegacyDec) MarshalTo(data []byte) (n int, err error) {
    i := d.i
    if i == nil {
        i = new(big.Int)
    }
    
    // For zero value, we still encode it properly using our ordered encoding
    bz, err := d.Marshal()
    if err != nil {
        return 0, err
    }
    
    if len(bz) > len(data) {
        return 0, errors.New("buffer too small")
    }
    
    copy(data, bz)
    return len(bz), nil
}

// Unmarshal implements the gogo proto custom type interface.
func (d *LegacyDec) Unmarshal(data []byte) error {
    if len(data) == 0 {
        d = nil
        return nil
    }
    
    if d.i == nil {
        d.i = new(big.Int)
    }
    
    if len(data) < 2 {
        return errors.New("invalid encoding: too short")
    }
    
    // Read sign byte
    sign := data[0]
    
    // Read length
    length := int(data[1])
    
    if len(data) < 2+length {
        return errors.New("invalid encoding: content length mismatch")
    }
    
    // Read content
    content := data[2 : 2+length]
    
    // Construct big.Int
    if sign == 0 {
        // Negative number, need to invert bits
        inverted := make([]byte, len(content))
        for i, b := range content {
            inverted[i] = ^b
        }
        d.i.SetBytes(inverted)
        d.i.Neg(d.i)
    } else {
        // Positive or zero
        d.i.SetBytes(content)
    }
    
    if !d.IsInValidRange() {
        return errors.New("decimal out of range")
    }
    return nil
}

TraderWithPython avatar May 05 '25 16:05 TraderWithPython