feat(math): add mutative api for Uint.BigInt() #18247
Conversation
WalkthroughThis change introduces mutative APIs for Changes
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/uint.go (1 hunks)
- math/uint_test.go (1 hunks)
Additional comments: 1
math/uint_test.go (1)
- 102-120: The new test
TestConvertToBigIntMutativeForUintis well-structured and covers the expected behavior of theBigIntMut()method. It checks that theBigIntMut()andBigInt()methods return the same initial value, and that modifying theBigIntMut()pointer updates bothBigIntMut()andBigInt(), while modifying theBigInt()pointer does not affect either. This is a good practice to ensure the mutative behavior of theBigIntMut()method is working as expected. However, it would be beneficial to add a few more test cases to cover edge cases, such as when theUintis uninitialized.+ // Add test case for uninitialized Uint + func (s *uintTestSuite) TestConvertToBigIntMutativeForUninitializedUint() { + var i sdkmath.Uint + s.Require().Nil(i.BigIntMut()) + }Committable suggestion (Beta)
func (s *uintTestSuite) TestConvertToBigIntMutativeForUint() { r := big.NewInt(30) i := sdkmath.NewUintFromBigInt(r) // Compare value of BigInt & BigIntMut s.Require().Equal(i.BigInt(), i.BigIntMut()) // Modify BigIntMut() pointer and ensure i.BigIntMut() & i.BigInt() change p1 := i.BigIntMut() p1.SetInt64(40) s.Require().Equal(big.NewInt(40), i.BigIntMut()) s.Require().Equal(big.NewInt(40), i.BigInt()) // Modify big.Int() pointer and ensure i.BigIntMut() & i.BigInt() don't change p2 := i.BigInt() p2.SetInt64(50) s.Require().NotEqual(big.NewInt(50), i.BigIntMut()) s.Require().NotEqual(big.NewInt(50), i.BigInt()) } // Add test case for uninitialized Uint func (s *uintTestSuite) TestConvertToBigIntMutativeForUninitializedUint() { var i sdkmath.Uint s.Require().Nil(i.BigIntMut()) }
| // BigInt converts Uint to big.Int, mutative the input | ||
| func (u Uint) BigIntMut() *big.Int { | ||
| if u.IsNil() { | ||
| return nil | ||
| } | ||
| return u.i | ||
| } |
There was a problem hiding this comment.
The new method BigIntMut() allows direct access to the underlying big.Int of the Uint struct. This can lead to potential issues with data integrity and encapsulation, as it allows external code to modify the internal state of the Uint struct. This is generally not a good practice in object-oriented programming, as it can lead to bugs and inconsistencies.
If you need to allow mutation of the big.Int value, consider providing methods on the Uint struct to perform these mutations, rather than exposing the internal state directly. This way, you can control how the internal state is modified and ensure that it remains consistent.
- // BigInt converts Uint to big.Int, mutative the input
- func (u Uint) BigIntMut() *big.Int {
- if u.IsNil() {
- return nil
- }
- return u.i
- }Committable suggestion (Beta)
| // BigInt converts Uint to big.Int, mutative the input | |
| func (u Uint) BigIntMut() *big.Int { | |
| if u.IsNil() { | |
| return nil | |
| } | |
| return u.i | |
| } | |
| // Remove the BigIntMut method to maintain encapsulation |
|
@testinginprod is working on refactoring the API in #18099 to avoid a cluttered API full of |
|
Great |
|
Hi, we discussed internally, and we will wait for the API improvement mentioned here for math/v2. |
|
I merged with the main branch |
Similar to #18066, I think there should also be an api for Uint
Summary by CodeRabbit