Mdjakovic/update curve #767
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request addresses a query error in the Curve ADO related to Float data type handling. The changes primarily involve modifying the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)contracts/math/andromeda-curve/src/contract.rs(3 hunks)contracts/math/andromeda-curve/src/testing/mock.rs(1 hunks)contracts/math/andromeda-curve/src/testing/tests.rs(4 hunks)packages/andromeda-math/src/curve.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Contract Tests (ubuntu-latest, unit-test)
- GitHub Check: Contract Tests (ubuntu-latest, integration-test)
- GitHub Check: Contracts
- GitHub Check: Schemas
- GitHub Check: Summary
🔇 Additional comments (6)
contracts/math/andromeda-curve/src/testing/mock.rs (1)
81-81: Update tests to reflect thex_valuetype changeThe parameter
x_valueinquery_plot_y_from_xwithin the mock has been updated fromf64tou64. Please ensure all test cases and mock implementations are adjusted accordingly to prevent type mismatches and test failures.contracts/math/andromeda-curve/src/testing/tests.rs (4)
Line range hint
157-172: LGTM! Test cases for base 2 growth curve are well-structured.The test cases correctly verify exponential growth with base 2, and the type change from
f64tou64aligns with the PR objectives to fix float-related issues.
Line range hint
175-190: LGTM! Test cases for base 3 growth curve are well-structured.The test cases correctly verify exponential growth with base 3, and the type change from
f64tou64aligns with the PR objectives.
193-209: Great addition! Test cases for base 4 growth curve with constant value enhance coverage.The new test cases correctly verify exponential growth with base 4 and constant value 2, providing better coverage for the curve configurations.
Line range hint
211-226: LGTM! Test cases for base 2 decay curve handle decimals correctly.The test cases correctly verify exponential decay with base 2, and the string comparison ensures precise handling of decimal values.
CHANGELOG.md (1)
42-42: LGTM! Changelog entry is clear and properly placed.The entry accurately describes the fix for the Curve ADO's query error related to float data type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/math/andromeda-curve/src/contract.rs (1)
185-190: Consider using Decimal::from_ratio for cleaner initialization.Instead of using
Decimal::percent, consider usingDecimal::from_ratiofor a more direct and cleaner way to initialize the decimal values.- let base_value_decimal = Decimal::percent(base_value * 100); - let constant_value_decimal = - Decimal::percent(constant_value.unwrap_or(DEFAULT_CONSTANT_VALUE) * 100); - let multiple_variable_value_decimal = Decimal::percent( - multiple_variable_value.unwrap_or(DEFAULT_MULTIPLE_VARIABLE_VALUE) * 100, - ); + let base_value_decimal = Decimal::from_ratio(base_value, 1u64); + let constant_value_decimal = + Decimal::from_ratio(constant_value.unwrap_or(DEFAULT_CONSTANT_VALUE), 1u64); + let multiple_variable_value_decimal = + Decimal::from_ratio(multiple_variable_value.unwrap_or(DEFAULT_MULTIPLE_VARIABLE_VALUE), 1u64);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/math/andromeda-curve/src/contract.rs(3 hunks)contracts/math/andromeda-curve/src/mock.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Contract Tests (ubuntu-latest, unit-test)
- GitHub Check: Contract Tests (ubuntu-latest, integration-test)
- GitHub Check: Contract Tests (ubuntu-latest, unit-test)
- GitHub Check: Contract Tests (ubuntu-latest, integration-test)
- GitHub Check: Contracts
- GitHub Check: Schemas
- GitHub Check: Summary
🔇 Additional comments (7)
contracts/math/andromeda-curve/src/mock.rs (1)
83-86: LGTM! Parameter type change aligns with contract implementation.The change from
f64tou64forx_valueparameter matches the contract implementation and helps resolve the float data type issue.contracts/math/andromeda-curve/src/contract.rs (6)
18-19: LGTM! Added Decimal import for precise calculations.The addition of
Decimaltype from cosmwasm_std is appropriate for handling precise numerical calculations.
174-174: LGTM! Parameter type change matches mock implementation.The change from
f64tou64forx_valueparameter aligns with the mock implementation and helps resolve the float data type issue.
192-199: LGTM! Proper error handling for decimal conversion.The code properly handles potential errors during decimal conversion and multiplication operations.
201-208: LGTM! Overflow check for u32 conversion.The code properly checks if the exponent value exceeds u32::MAX before attempting the conversion.
211-218: LGTM! Safe decimal operations with overflow checks.The code uses checked operations (
checked_mul,checked_pow) to safely handle potential overflows in decimal calculations.
220-225: LGTM! Proper handling of curve types with underflow check.The code correctly handles both Growth and Decay curve types, with proper error handling for potential underflow in division operations.
| @@ -60,7 +60,7 @@ pub enum QueryMsg { | |||
| #[returns(GetCurveConfigResponse)] | |||
| GetCurveConfig {}, | |||
| #[returns(GetPlotYFromXResponse)] | |||
| GetPlotYFromX { x_value: f64 }, | |||
| GetPlotYFromX { x_value: u64 }, | |||
There was a problem hiding this comment.
I think we want to use ::cosmwasm_std::Decimal type here rather than u64
There was a problem hiding this comment.
BTW, Decimal's checked_pow() function must have u32 type parameter.
So it can only be calculated by u32 exponent.
In this case, the usage of u64 and Decimal are the same.
But if you want to Decimal as x_value's input type, I will do that.
There was a problem hiding this comment.
Should I use the Decimal as x_value?
|
Looks good! |
Co-authored-by: Connor Barr <[email protected]> (cherry picked from commit fa3aa1d)
Co-authored-by: Mitar Djakovic <[email protected]>
Motivation
We have to resolve the query error caused by
floatdata type.The chain does not support that type.
Implementation
To resolve this issue, I used
u64instead offloat64as the input type, and implemented theDecimalas the calculation.Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
f64tou64.Decimaltype for curve computations.Testing