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: RelativePow now returns 1 when 0^0 #18211

Merged
merged 2 commits into from
Oct 23, 2023
Merged

fix: RelativePow now returns 1 when 0^0 #18211

merged 2 commits into from
Oct 23, 2023

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Oct 23, 2023

Description

I've checked current usages and it doesn't look like there are people misusing this enough to ever get the wrong result. The issue is that when x and n are 0 then the result is whatever being passed as b, even though there's a comment stating // 0^0 = 1.

This is not being used in the SDK btw.

https://github.com/search?q=RelativePow+language%3AGo&type=code&l=Go


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: Corrected the behavior of the RelativePow function to return 1 when raising 0 to the power of 0, enhancing the mathematical accuracy of the software.
  • Bug Fix: Reverted a change in the ApproxRoot function's rounding behavior, ensuring consistent and expected results across all calculations.

These changes improve the reliability of mathematical operations within the software, providing users with more accurate and predictable results.

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

coderabbitai bot commented Oct 23, 2023

Walkthrough

This pull request addresses two key bug fixes in the cosmos-sdk repository. The first fix ensures that 0^0 is evaluated as 1 in the RelativePow function. The second fix restores the banker's rounding behavior in the ApproxRoot function.

Changes

File Summary
math/uint.go Modified RelativePow function to correctly handle 0^0 case.
math/uint_test.go Updated test case for RelativePow function to reflect the correct expected result.
math/CHANGELOG.md Documented the bug fixes in RelativePow and ApproxRoot functions.

🐇

In the realm of numbers, where zero and one play,

A rabbit hopped and found a better way.

With a twitch of the nose and a flick of the ear,

Bugs in the code, they disappear! 🎉


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-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 cc97cf0.
Files selected for processing (2)
  • 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)
  • 274-274: The test case has been updated to reflect the new expected behavior of the RelativePow function when both x and n are zero. The expected result has been changed from sdkmath.NewUint(10) to sdkmath.NewUint(1), which is the correct mathematical result for 0^0. This change is consistent with the updated implementation of the RelativePow function.

@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 cc97cf0 and 2f28796.
Files selected for processing (1)
  • math/CHANGELOG.md (1 hunks)

@tac0turtle
Copy link
Member

dont think we can backport this to 0.47 but 0.50 is safe

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Agree with Marko that we shouldn't backport to v0.47.

@julienrbrt julienrbrt removed the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Oct 23, 2023
@tac0turtle tac0turtle added this pull request to the merge queue Oct 23, 2023
Merged via the queue into main with commit 442c3c5 Oct 23, 2023
55 checks passed
@tac0turtle tac0turtle deleted the facu/fix-relativepow branch October 23, 2023 13:23
mergify bot pushed a commit that referenced this pull request Oct 23, 2023
(cherry picked from commit 442c3c5)

# Conflicts:
#	math/CHANGELOG.md
#	math/uint.go
tac0turtle added a commit that referenced this pull request Oct 23, 2023
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
@julienrbrt
Copy link
Member

julienrbrt commented Oct 23, 2023

I forgot this is math. So this is the same version used in v0.47, v0.50, etc.. this means the backported wasn't necessary and that it will be in v0.47 available.

@facundomedica can you check on source graph that no one depends on it? Otherwise I'll check myself once im no more afk.

@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants