feat(math): fix and test LegacyToBigInt #18576
Conversation
WalkthroughWalkthroughThe changes involve updating the Changes
Assessment against linked issues
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- math/dec.go (2 hunks)
- math/dec_test.go (1 hunks)
Additional comments: 3
math/dec.go (2)
- 234-234: Both
BigIntandBigIntMutmethods now involve division byprecisionReuse, which could potentially lead to precision loss or unexpected behavior. Verify that the precision and value implications of these changes have been thoroughly considered and tested.Also applies to: 243-243
- 231-237: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [231-245]
Overall, the changes to the
BigIntandBigIntMutmethods are significant and could impact the existing functionality. It is crucial to ensure that these changes are well-documented and that all dependent code has been reviewed and updated accordingly.math/dec_test.go (1)
- 766-780: The test
TestConvertToBigIntMutativeForLegacyDeccorrectly verifies the behavior ofBigIntandBigIntMutmethods, ensuring that the mutative method affects the originalLegacyDecvalue as expected.
| @@ -231,8 +231,7 @@ func (d LegacyDec) BigInt() *big.Int { | |||
| return nil | |||
| } | |||
|
|
|||
| cp := new(big.Int) | |||
| return cp.Set(d.i) | |||
| return new(big.Int).Quo(d.i, precisionReuse) | |||
There was a problem hiding this comment.
The BigInt method now returns a new big.Int by dividing d.i by precisionReuse, which changes the behavior of the method. Ensure that this change is intentional and that all usages of this method have been updated to handle the new behavior correctly.
There was a problem hiding this comment.
ditto, that is a pretty significant change, for a behavior that has been there for 3+ years: https://github.com/cosmos/cosmos-sdk/blame/fc52ae5fdbf10f6780f02fec591f01dcb5c294b6/math/dec.go#L234
There was a problem hiding this comment.
@julienrbrt Indeed, the change in behavior for the BigInt method is significant. The method previously returned a copy of the internal big.Int representation, but with the new change, it now returns a new big.Int that is the result of dividing the internal big.Int by precisionReuse. This effectively changes the scale of the number from a decimal representation to an integer representation.
Given the longevity of the previous behavior, it's crucial to review all instances where BigInt is used to ensure that the new behavior does not introduce bugs or unexpected results. It may also be necessary to update documentation and communicate these changes to all developers who rely on this method, as they may need to adjust their code to accommodate the new behavior.
If this change is intentional and all usages have been accounted for, it would be good to proceed. However, if there's any uncertainty, it might be worth considering additional validation or even reverting to the previous behavior to maintain backward compatibility.
| @@ -241,7 +240,7 @@ func (d LegacyDec) BigIntMut() *big.Int { | |||
| return nil | |||
| } | |||
|
|
|||
| return d.i | |||
| return d.i.Quo(d.i, precisionReuse) | |||
There was a problem hiding this comment.
The BigIntMut method now mutates the internal d.i by dividing it by precisionReuse. This is a significant change that alters the state of the LegacyDec instance. Ensure that this change is intentional and that all usages of this method have been updated to handle the new behavior correctly.
|
Hey, I reopened with a new PR. Because I'm no longer at Notional, I can't commit to that branch. |
|
To: #18658 |
|
|
Description
Closes: #18575
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.mdmake lintandmake testReviewers 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 changeSummary by CodeRabbit
Refactor
big.Intformat, ensuring more predictable and safer operations.Tests
big.Intconversions to validate new behavior and ensure accuracy.