feat(math): add mutative api for LegacyDec.BigInt() #18421
Conversation
Walkthrough<details>
<summary>Walkthrough</summary>
# Walkthrough
The changes introduce a new mutative method `BigIntMut` to the `LegacyDec` type in the `cosmos-sdk` repository. This method converts `LegacyDec` to `*big.Int` mutatively. A corresponding test function `TestConvertToBigIntMutativeForLegacyDec` is also added to ensure the correct behavior of the new method. The `CHANGELOG.md` file is updated to reflect these additions and other mutative APIs for various methods.
# Changes
| File | Change Summary |
| --- | --- |
| `math/dec.go` | Introduced a new method `BigIntMut` to the `LegacyDec` type, which converts `LegacyDec` to `*big.Int` mutatively. |
| `math/dec_test.go` | Added a new test function `TestConvertToBigIntMutativeForLegacyDec` to the `decimalTestSuite` struct to test the new `BigIntMut` method. |
| `math/CHANGELOG.md` | Updated with the addition of mutative APIs for various methods such as `LegacyDec.BigInt()`, `Uint.BigInt()`, `Int.BigInt()`, and `NewIntFromBigInt`. |
</details>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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- math/dec.go (1 hunks)
- math/dec_test.go (1 hunks)
Additional comments: 1
math/dec_test.go (1)
- 758-776: The test case is well written and covers the expected behavior of the
BigIntMutmethod. It checks both the case where theBigIntMutpointer is modified and the case where theBigIntpointer is modified, ensuring that the behavior is as expected in both cases.
| // BigIntMut converts LegacyDec to big.Int, mutative the input | ||
| func (d LegacyDec) BigIntMut() *big.Int { | ||
| if d.IsNil() { | ||
| return nil | ||
| } | ||
|
|
||
| return d.i | ||
| } |
There was a problem hiding this comment.
The method BigIntMut is not actually mutating the LegacyDec instance. It's returning the internal big.Int instance which can be mutated, but the changes won't reflect back in the LegacyDec instance because big.Int is not a pointer type in LegacyDec. If you want to make it mutable, you should change the LegacyDec type to hold a pointer to big.Int instead of a value.
- type LegacyDec struct { i big.Int }
+ type LegacyDec struct { i *big.Int }And then adjust all methods accordingly. Be aware that this is a breaking change and will require updates throughout the codebase where LegacyDec is used.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| // BigIntMut converts LegacyDec to big.Int, mutative the input | |
| func (d LegacyDec) BigIntMut() *big.Int { | |
| if d.IsNil() { | |
| return nil | |
| } | |
| return d.i | |
| } | |
| // BigIntMut converts LegacyDec to big.Int, mutative the input | |
| func (d LegacyDec) BigIntMut() *big.Int { | |
| if d.IsNil() { | |
| return nil | |
| } | |
| return d.i | |
| } | |
| type LegacyDec struct { i *big.Int } |
There was a problem hiding this comment.
Yes our api naming is weird, but this is consistent.
There was a problem hiding this comment.
@julienrbrt Thank you for your response. I understand that the naming is consistent with the existing API. However, the concern here is not just about the naming but the functionality of the BigIntMut method. As it stands, the method does not mutate the LegacyDec instance as the name suggests. Instead, it returns the internal big.Int instance which can be mutated, but the changes won't reflect back in the LegacyDec instance. This could lead to confusion and potential bugs in the future.
If the intention is to allow mutation of the LegacyDec instance through this method, I would recommend changing the LegacyDec type to hold a pointer to big.Int instead of a value. Please note that this would be a breaking change and would require updates throughout the codebase where LegacyDec is used.
Alternatively, if the intention is to simply return the internal big.Int instance without affecting the LegacyDec instance, it might be better to rename the method to something like BigIntCopy to avoid confusion.
I hope this clarifies the concern. Please let me know how you would like to proceed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- math/CHANGELOG.md (1 hunks)
Additional comments: 1
math/CHANGELOG.md (1)
- 40-46: The changelog correctly reflects the addition of the mutative APIs for
LegacyDec.BigInt(),Uint.BigInt(),Int.BigInt(), andNewIntFromBigInt. Ensure that the documentation for these methods is updated to reflect the mutative behavior.
math/CHANGELOG.md
Outdated
| @@ -40,6 +40,7 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j | |||
|
|
|||
| ### Features | |||
|
|
|||
| * [#18421](https://github.com/cosmos/cosmos-sdk/pull/18421) Add mutative api for `LegacyDec.BigInt()`. | |||
There was a problem hiding this comment.
This should be moved under unreleased features.
I think there should also be an API for LegacyDec before improving the API for math/v2
Similar to #18066
Summary by CodeRabbit
The existing bullet-point list is still valid based on the provided instructions.