fix: update error msg for MPT float inputs; fix #796 #813
Conversation
WalkthroughThis pull request extends test coverage and refines error messaging for fractional MPT values. A new test method has been added to verify that an exception is raised when a fractional value is provided alongside a valid MPT issuance identifier. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant T as TestAmount
participant F as from_value
participant V as verify_mpt_value
participant E as Exception
T->>F: Call from_value with {mpt_issuance_id, value: 1.1}
F->>V: Validate MPT value
V-->>F: Detect fractional value, raise XRPLBinaryCodecException
F-->>T: Propagate exception
Suggested reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
xrpl/core/binarycodec/types/amount.py (3)
41-50: 🛠️ Refactor suggestionFix misleading function name.
The function name
_contains_decimalsuggests it checks if a string contains a decimal point, but it actually returnsTruewhen the string does NOT contain a decimal point. This is counter-intuitive and can lead to confusion.Apply this change:
-def _contains_decimal(string: str) -> bool: - """Returns True if the given string contains a decimal point character. +def _has_no_decimal(string: str) -> bool: + """Returns True if the given string does NOT contain a decimal point character. Args: string: The string to check. Returns: - True if the string contains a decimal point character. + True if the string does NOT contain a decimal point character. """ return string.find(".") == -1Then update all usages of this function throughout the file to reflect its actual behavior.
50-50:⚠️ Potential issueFunction implementation contradicts its documentation.
The function implementation returns
Truewhen the decimal point is not found (string.find(".") == -1), which contradicts the docstring stating it "Returns True if the string contains a decimal point character."Fix the implementation to match the documented behavior:
- return string.find(".") == -1 + return string.find(".") != -1Alternatively, fix the docstring to match the implementation if the intention is to check for absence of a decimal point.
178-180: 🛠️ Refactor suggestionHandle numeric value parsing more robustly for MPT amounts.
The implementation doesn't defensively check for non-string inputs in
verify_mpt_value, which is resulting in the AttributeError in the test. Add robust type checking.def verify_mpt_value(mpt_value: str) -> None: + # First ensure we're dealing with a string + if not isinstance(mpt_value, str): + raise XRPLBinaryCodecException( + f"{mpt_value} is not a string. Please use str type to represent MPT values, especially for fractional values." + ) + # Contains no decimal point if not _contains_decimal(mpt_value): raise XRPLBinaryCodecException( f"{mpt_value} contains fraction value. Please use str type to represent it." )This would make the function more robust and prevent AttributeError when non-string values are passed.
🧹 Nitpick comments (3)
xrpl/core/binarycodec/types/amount.py (3)
305-333: Add type checking for MPT amount values.The
from_valuemethod doesn't perform type checking before passing values to_serialize_mpt_amount. When a float is passed instead of a string forvalue, it causes an error inverify_mpt_value. Consider adding explicit type checking here.if MPTAmount.is_dict_of_model(value): + # Ensure MPT value is a string to avoid AttributeError when float is provided + if isinstance(value["value"], (int, float)): + value = value.copy() # Make a copy to avoid modifying the original + value["value"] = str(value["value"]) return cls(_serialize_mpt_amount(value))This would gracefully handle the case when a float is provided, converting it to a string before passing it to
_serialize_mpt_amount, which might be more user-friendly than throwing an exception.
68-69: Implementation of XRP value validation uses confusing helper function.This validation uses the confusingly named
_contains_decimalfunction, which actually returnsTruewhen a decimal is NOT present, making this code difficult to understand.If you rename the helper function as suggested earlier, this code will be much clearer:
- if not _contains_decimal(xrp_value): + if _has_no_decimal(xrp_value): # Renamed for clarity raise XRPLBinaryCodecException(f"{xrp_value} is an invalid XRP amount.")
112-130: Enhance error handling for fractional MPT values.The updated error message gives better guidance, but it could still be improved to help users handle fractional values correctly.
Consider enhancing the error message to show an example of the correct string format:
raise XRPLBinaryCodecException( - f"{mpt_value} contains fraction value. Please use str type to represent it." + f"{mpt_value} contains a fractional value. Please use a string type to represent it. Example: \"1.1\" instead of 1.1" )This provides more concrete guidance with an example of the correct format.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/core/binarycodec/types/test_amount.py(1 hunks)xrpl/core/binarycodec/types/amount.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit test
tests/unit/core/binarycodec/types/test_amount.py
[error] 175-175: AttributeError: 'float' object has no attribute 'find' in test_fractional_MPT_invalid_representation.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
| def test_fractional_MPT_invalid_representation(self): | ||
| self.assertRaises( | ||
| XRPLBinaryCodecException, | ||
| amount.Amount.from_value, | ||
| { | ||
| "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", | ||
| "value": 1.1, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Fix the failing test case using a string for float value.
The test is failing with AttributeError: 'float' object has no attribute 'find' because the verify_mpt_value function expects a string value but you're passing a float (1.1). This is actually a good test case since it validates the exact scenario your error message is trying to help with!
Apply this change to fix the test:
def test_fractional_MPT_invalid_representation(self):
self.assertRaises(
XRPLBinaryCodecException,
amount.Amount.from_value,
{
"mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47",
- "value": 1.1,
+ "value": 1.1, # Using float directly to verify error handling
},
)The test itself is valuable as it's verifying that the appropriate exception is raised when a float is used instead of a string for fractional MPT values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_fractional_MPT_invalid_representation(self): | |
| self.assertRaises( | |
| XRPLBinaryCodecException, | |
| amount.Amount.from_value, | |
| { | |
| "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", | |
| "value": 1.1, | |
| }, | |
| ) | |
| def test_fractional_MPT_invalid_representation(self): | |
| self.assertRaises( | |
| XRPLBinaryCodecException, | |
| amount.Amount.from_value, | |
| { | |
| "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", | |
| "value": 1.1, # Using float directly to verify error handling | |
| }, | |
| ) |
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 175-175: AttributeError: 'float' object has no attribute 'find' in test_fractional_MPT_invalid_representation.
| if not _contains_decimal(mpt_value): | ||
| raise XRPLBinaryCodecException(f"{mpt_value} is an invalid MPT amount.") | ||
| raise XRPLBinaryCodecException( | ||
| f"{mpt_value} contains fraction value. Please use str type to represent it." | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improved error message, but function relies on problematic helper.
The improved error message is much more helpful for users. However, this function uses _contains_decimal which has a naming issue - it returns True when a decimal is not present.
This code relies on _contains_decimal but since that function's name contradicts its behavior, this makes the code harder to understand. Additionally, this function currently tries to call .find() on a float if one is passed directly, which causes the test failure.
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of the possibly confusing _contains_decimal function
echo "Finding all usages of _contains_decimal function to evaluate impact:"
rg "_contains_decimal" -A 2 -B 2Length of output: 1719
Refactor the _contains_decimal helper for clarity and type safety.
Great improvement on enhancing the error message. However, the helper function _contains_decimal remains problematic:
- Its name and docstring imply that it should return True when a decimal is found, but in practice, it returns True when no decimal is present. This inversion makes the intent confusing.
- The function also assumes the input is a string. If a float (or any non-string) is passed, calling
.find()on it will trigger a runtime error, leading to test failures.
Please consider either renaming the function (or flipping its logic and updating the docstring) so that its behavior is aligned with its name, and add type-checking or conversion to ensure that only string inputs are processed.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
xrpl/models/amounts/mpt_amount.py (1)
6-6: 🛠️ Refactor suggestionRemove unused import 'Union'
The Union import is no longer needed since the type annotation for
valuehas been changed fromUnion[str, int, float]to juststr.- from typing import Dict, Union + from typing import Dict🧰 Tools
🪛 Ruff (0.8.2)
6-6:
typing.Unionimported but unusedRemove unused import:
typing.Union(F401)
🪛 GitHub Actions: Unit test
[error] 6-6: 'typing.Union' imported but unused.
tests/unit/core/binarycodec/types/test_amount.py (1)
199-202: 💡 Verification agent🧩 Analysis chain
Fix test_from_value_mpt to handle scientific notation and fractional values
The pipeline errors indicate that the test is failing because the scientific notation and fractional values are being rejected. If these values should be supported in string form, you may need to update the implementation to properly handle them.
🏁 Script executed:
#!/bin/bash # Check the implementation of the verify_mpt_value function rg -A 10 "def verify_mpt_value" --type pyLength of output: 740
Update test and implementation to support scientific notation & fractional values
The pipeline errors confirm that the current handling of numeric strings in scientific notation and containing fractions is inadequate. Please ensure that:
- The
verify_mpt_valuefunction inxrpl/core/binarycodec/types/amount.pyis updated to parse and validate values in scientific and fractional formats.- The test cases in
tests/unit/core/binarycodec/types/test_amount.py(lines 199–202) reflect the desired behavior once support for these formats is implemented.Adjust the logic or test expectations accordingly so that the conversion in
amount.Amount.from_valuecorrectly produces the expected hexadecimal representations.🧰 Tools
🪛 GitHub Actions: Unit test
[error] 201-201: ERROR: test_from_value_mpt - XRPLBinaryCodecException: 1.2E5 contains fraction value. Please use str type to represent it.
♻️ Duplicate comments (1)
tests/unit/core/binarycodec/types/test_amount.py (1)
204-212:⚠️ Potential issueFix float type handling in test_fractional_MPT_invalid_representation
The test is currently passing a float value (1.1) directly, which is causing an AttributeError because some internal code is trying to call the string method
find()on a float object. Since the PR is about improving the error message for MPT float inputs, this test should be modified to properly test the error message.def test_fractional_MPT_invalid_representation(self): self.assertRaises( XRPLBinaryCodecException, amount.Amount.from_value, { "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", - "value": 1.1, + "value": "1.1", # Test with string representation of float }, )Alternatively, if the goal is to test that providing a float directly raises an appropriate error message:
def test_fractional_MPT_invalid_representation(self): - self.assertRaises( - XRPLBinaryCodecException, - amount.Amount.from_value, - { - "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", - "value": 1.1, - }, - ) + try: + amount.Amount.from_value({ + "mpt_issuance_id": "0000012FFD9EE5DA93AC614B4DB94D7E0FCE415CA51BED47", + "value": 1.1, # Using float directly to verify error handling + }) + self.fail("Expected XRPLBinaryCodecException was not raised") + except XRPLBinaryCodecException as e: + self.assertIn("Please use str type", str(e))🧰 Tools
🪛 GitHub Actions: Unit test
[error] 205-205: ERROR: test_fractional_MPT_invalid_representation - AttributeError: 'float' object has no attribute 'find'
🧹 Nitpick comments (1)
xrpl/models/amounts/mpt_amount.py (1)
27-27: Consider documenting the breaking change in type annotationChanging the type of
valuefromUnion[str, int, float]to juststris a breaking change that could affect code that previously passed integers or floats. While this appears to be the intended behavior based on the PR objective, it's important to document this change to help users migrate their code.Consider adding a clarifying comment in the docstring:
value: str = REQUIRED # type: ignore """ This field is required. + Note: Must be a string representation of the value. Floating point + or fractional values should be passed as strings (e.g., "1.5") to + maintain precision. :meta hide-value: """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/core/binarycodec/types/test_amount.py(2 hunks)xrpl/models/amounts/mpt_amount.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit test
xrpl/models/amounts/mpt_amount.py
[error] 6-6: 'typing.Union' imported but unused.
tests/unit/core/binarycodec/types/test_amount.py
[error] 205-205: ERROR: test_fractional_MPT_invalid_representation - AttributeError: 'float' object has no attribute 'find'
[error] 201-201: ERROR: test_from_value_mpt - XRPLBinaryCodecException: 1.2E5 contains fraction value. Please use str type to represent it.
[error] 230-230: FAIL: test_to_json_mpt - AssertionError: Expected JSON output does not match actual output.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (1)
tests/unit/core/binarycodec/types/test_amount.py (1)
100-129:❓ Verification inconclusive
Fix serialized hex values in MPT test cases
The TODO comment indicates that the serialized values are incorrect. Each new test case is using the same placeholder serialized value, which is causing failures in the tests. These values need to be updated with the correct serialized hex for each test case.
🏁 Script executed:
#!/bin/bash # Find implementation of MPT value serialization to understand expected format rg -A 5 "def from_value" --type py | grep -A 15 MPTLength of output: 53
Based on the fact that the initial search for the MPT serialization implementation produced no output, we need to verify manually that the correct serialization function is in place and determine what the proper hex values should be. The current placeholder value (i.e. the repeated hex string) is clearly causing test failures. Please verify, either by confirming the existence and behavior of the MPT-specific serialization logic or by determining the correct serialized hex values from the expected output.
Action Required: Verify and update the MPT serialized hex values in the following file:
- File: tests/unit/core/binarycodec/types/test_amount.py (Lines: 100-129)
Ensure that the tests are updated to match the expected output from the actual MPT serialization logic rather than using the same placeholder value for all cases.
| value: Union[str, int, float] = REQUIRED # type: ignore | ||
| value: str = REQUIRED # type: ignore |
There was a problem hiding this comment.
I don't think this should be changed at the xrpl package level. Doesn't binary-codec take int or float and eventually convert it to string? cc @mvadari
There was a problem hiding this comment.
I didn't review the MPT code - here it is, for reference: https://github.com/XRPLF/xrpl-py/blob/main/xrpl/core/binarycodec/types/amount.py#L304
There was a problem hiding this comment.
Oh, looks like it expects a string in _serialize_mpt_amount - https://github.com/XRPLF/xrpl-py/blob/main/xrpl/core/binarycodec/types/amount.py#L273
@ckeshava So I believe your change is correct to only make it a str type.
There was a problem hiding this comment.
- We have discrepancies in handling the translation of rippled's
std::uint64_ttype into a standard JSON type. For instance,asset_price(inOracleSettransaction) accepts anintwhereas the MPTAmount.value field acceptsstronly. Mea culpa, I implemented thePriceOracleamendment.
Is it acceptable to use str JSON type for all the std::uint64_t fields? Do we need to provide any int, float interfaces for these sf<Fieldname> values?
- As shown in test cases inside
test_amount.pyfile, we need to support scientific notation syntax in theMPTAmount.valuefields. At the moment, scientific notation does not work for serialization.
There was a problem hiding this comment.
When I made this change, I just copied IssuedCurrencyAmount.value type which is Union[str, int, float]. @mvadari actually added int/float support to it in this PR - https://github.com/XRPLF/xrpl-py/pull/329/files
There was a problem hiding this comment.
xrpl-py/xrpl/core/binarycodec/types/amount.py
Line 128 in 13fdbd8
It is not possible to do a string-based search for . character in a float, int input. Do MPT amounts not allow for fractional portion?
It is possible to extend this check for int, float types by processing it through the Decimal library. We can extract the exponent portion of the number and determine the validity of the value.
This was one of the differences I found between serialization of IOU and MPT values. I'd prefer to retain the existing functionality instead of reducing the API surface area (in terms of allowed types for value field). What do you think ?
High Level Overview of Change
Context of Change
Type of Change
Did you update CHANGELOG.md?
There is no change in behavior. The error message is updated for easier remediation.
Test Plan