Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(math): NewUintFromBigInt argument mutation #18214

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Oct 23, 2023

Description

same as #17352 but for NewUintFromBigInt


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)

Summary by CodeRabbit

  • Bug Fix: Enhanced the reliability of the NewUIntFromBigInt function to ensure that modifying the input does not affect the returned value, providing a more stable and predictable user experience.
  • Bug Fix: Updated the RelativePow function to return 1 when raising 0 to the power of 0, improving mathematical consistency.
  • Bug Fix: Reverted the rounding behavior in the ApproxRoot function to use banker's rounding universally, ensuring more accurate results.
  • Test: Added new test cases for the NewUintFromBigInt and TestParseUint functions, increasing the robustness of our testing suite.

@facundomedica facundomedica requested a review from a team as a code owner October 23, 2023 12:41
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2023

Walkthrough

The changes primarily focus on enhancing the robustness and correctness of the cosmos-sdk math package. They ensure that the NewUIntFromBigInt function doesn't mutate the returned value, update the behavior of RelativePow, and revert the rounding behavior of ApproxRoot. Additionally, new test cases have been added to validate these changes.

Changes

File Summary
math/uint.go The checkNewUint function is modified to create a new big.Int instance, ensuring the original value is not externally modified.
math/uint_test.go A new test function TestNewUintFromBigInt is added to verify the behavior of NewUintFromBigInt. The TestParseUint function is also updated with additional test cases.
math/CHANGELOG.md The changelog includes bug fixes related to NewUIntFromBigInt, RelativePow, and ApproxRoot functions in the cosmos-sdk repository.

"In the land of code, where logic is king, 🤴
Bugs are banished, and tests do sing. 🎵
With each new change, we hop with delight, 🐇
For in the realm of software, correctness is might!" 💪


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

@github-prbot github-prbot requested review from a team, kocubinski and tac0turtle and removed request for a team October 23, 2023 12:41
@github-actions

This comment has been minimized.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0
Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f2f47ff and 53d9050.
Files selected for processing (2)
  • math/uint.go (1 hunks)
  • math/uint_test.go (1 hunks)
Additional comments (Suppressed): 2
math/uint.go (1)
  • 238-239: The change here is to prevent mutation of the original big.Int value that is passed as an argument to the NewUintFromBigInt function. This is achieved by creating a new big.Int instance and setting its value to the input argument. This is a good practice as it ensures that the function does not have any side effects on its input arguments.
-    return Uint{i}, nil
+    return Uint{new(big.Int).Set(i)}, nil
math/uint_test.go (1)
  • 264-272: The new test TestNewUintFromBigInt is well written and covers the expected behavior of the NewUintFromBigInt function. It checks both the initial conversion from big.Int to sdkmath.Uint and the immutability of the sdkmath.Uint instance when the original big.Int is modified. This test will help ensure that the NewUintFromBigInt function behaves as expected and does not introduce any unexpected side effects.

@facundomedica facundomedica added backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Oct 23, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0
Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 53d9050 and 4efd5e9.
Files selected for processing (2)
  • math/CHANGELOG.md (1 hunks)
  • math/uint.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • math/uint.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0
Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4efd5e9 and 2b4c78d.
Files selected for processing (3)
  • math/CHANGELOG.md (1 hunks)
  • math/uint.go (1 hunks)
  • math/uint_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • math/uint.go
Additional comments (Suppressed): 1
math/uint_test.go (1)
  • 264-272: The new test TestNewUintFromBigInt is well written and covers the expected behavior of the NewUintFromBigInt function. It checks that the original big.Int and the sdkmath.Uint have the same value after conversion, and that modifying the original big.Int does not affect the sdkmath.Uint. This test ensures that the NewUintFromBigInt function does not mutate the original big.Int value, which is a crucial aspect of its functionality.

@facundomedica facundomedica added this pull request to the merge queue Oct 23, 2023
Merged via the queue into main with commit bbf2faa Oct 23, 2023
54 checks passed
@facundomedica facundomedica deleted the facu/fix-uint-mut branch October 23, 2023 14:14
mergify bot pushed a commit that referenced this pull request Oct 23, 2023
(cherry picked from commit bbf2faa)

# Conflicts:
#	math/CHANGELOG.md
mergify bot pushed a commit that referenced this pull request Oct 23, 2023
(cherry picked from commit bbf2faa)

# Conflicts:
#	math/CHANGELOG.md
@julienrbrt julienrbrt removed backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Oct 23, 2023
mpoke pushed a commit that referenced this pull request Jan 31, 2024
mattverse pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request Feb 6, 2024
stana-miric added a commit to informalsystems/cosmos-sdk that referenced this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants