fix(math)!: make LegacyDec zero values marshaled equal

[facundomedica]

Description

Would like to get comments regarding why this was like this in the first place and if it's something we should fix or not.
The issue was that MarshalJSON(LegacyDec{}) != MarshalJSON(LegacyNewDec(0)) as one produces "0" and the other one "0.00000...".

Closes: #17606

---

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...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• run make lint and make test
• confirmed all CI checks have passed

Reviewers 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...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[wojtek-coreum]

Later on, please backport it to v0.47

[samricotta]

I think it makes sense, aligns more with our migration to math and cleaning up code

[facundomedica]

Later on, please backport it to v0.47

I have some doubts wether or not we can backport this (although I don't think we use this marshalling for state anywhere), to avoid delaying your work I'd suggest you define all the Dec fields, at least for now.

[wojtek-coreum]

@facundomedica

although I don't think we use this marshalling for state anywhere

I guess you do, because this is v0.47 where we found the bug

to avoid delaying your work I'd suggest you define all the Dec fields, at least for now

yes, this is current workaround but we would like to not force our users to set fields which are not required

[julienrbrt]

We cannot backport it for the reason Facu mentioned and the issue is worse than that actually because Math is actually its own go mod (not the fix but the deployment).

Given that old chains/sdk version can use math and that it can be state breaking; I do not think we can fix this in math/v1.

cc @tac0turtle @facundomedica

[wojtek-coreum]

@julienrbrt

I don't really understand the reason because @facundomedica didn't explain it, despite saying that it might not be possible.

From my perspective, the case is really simple. Somewhere in Cosmos SDK v0.47, or inside its dependencies, there is a bug causing wrong serialization of sdk.Dec. As far as I understand this PR, the place to fix has been identified. Now you just need to fix it in v0.47 branch directly or inside the dependency, release new patched version(s) and that's it. I don't see why it might be impossible.

[julienrbrt]

There is anyway no backport needed as math is tracked separately from the Cosmos SDK releases. See the math/CHANGELOG.md.

The issue is that if this bug is relied upon in the state machine, then that is a state machine breaking change to fix the behavior, which cannot be introduced in patch releases. Bugs that are state machine breaking, unless critical, are fixed in the next major version.
This could be state machine breaking, depending on how chains are using math, so it cannot be included in a patch version (of the SDK and math). Additionally because math is separately tracked from the SDK which poses issue if we want to include this in math v1.

[wojtek-coreum]

@julienrbrt Thanks for explanation, now I see your point.

[julienrbrt]

Blocking this until we discuss about deployment.

[julienrbrt]

As discussed in our standup, we will not address this issue in math v1.
We will track this, for fixing in math v2 (if it ever exists), and mark this as a won't fix known bug for math v1.

Correct me if I am wrong @facundomedica

[wojtek-coreum]

@julienrbrt @facundomedica Could you elaborate more, on why you made this decision? It seems weird that bugs are not fixed even when you know how to fix them.

[julienrbrt]

@julienrbrt @facundomedica Could you elaborate more, on why you made this decision? It seems weird that bugs are not fixed even when you know how to fix them.

The issue is the deployment of such bug fix. We have tracked the bug in a separate issue, and we will fix it in a breaking version of math.

Given than math is its own go.mod, feel free to fork it, and use it in your chain if you are sure this won't cause for you any state break.