feat: add model validation for types #708
Conversation
mvadari
force-pushed
the
model-type-validation
branch
from
cd1e0d0
to
bc3e043
Compare
mvadari
force-pushed
the
model-type-validation
branch
from
bc3e043
to
310363e
Compare
|
thanks, this is very interesting work. I feel we could have used this library to enforce type checks, had it been better maintained: https://pypi.org/project/enforce-typing/ |
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
Warning Rate limit exceeded@mvadari has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 19 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes to the project include the addition of support for the DeliverMax field in Payment transactions and the implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
participant Validator
User->>System: Initiates Payment with DeliverMax
System->>Validator: Validate transaction parameters
Validator-->>System: Parameters valid
System->>User: Transaction successful
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 full review |
Actions performedFull review triggered. |
![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 and nitpick comments (2)
xrpl/models/base_model.py (2)
294-351: Consider adding a docstring to the_check_typemethod for clarity.Adding a docstring to
_check_typewould improve code readability and maintainability by documenting its purpose, parameters, and return value.
307-307: Clarify or remove the comment regarding the mypy issue.The comment
# unsure what the problem with mypy is heremay not be helpful. If there's a mypy warning, consider either resolving it or providing a more detailed explanation to assist future maintainers.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- snippets/paths.py (1 hunks)
- tests/unit/models/test_base_model.py (18 hunks)
- tests/unit/models/transactions/test_check_cash.py (1 hunks)
- tests/unit/models/transactions/test_oracle_set.py (1 hunks)
- tests/unit/models/transactions/test_xchain_claim.py (1 hunks)
- xrpl/models/base_model.py (2 hunks)
- xrpl/models/requests/ledger_entry.py (1 hunks)
- xrpl/models/transactions/oracle_set.py (2 hunks)
- xrpl/models/transactions/pseudo_transactions/enable_amendment.py (2 hunks)
- xrpl/models/transactions/transaction.py (2 hunks)
- xrpl/utils/xrp_conversions.py (3 hunks)
Additional context used
Gitleaks
tests/unit/models/test_base_model.py
448-448: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
471-471: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (22)
tests/unit/models/transactions/test_check_cash.py (1)
9-9: Verify and update other occurrences of_CHECK_IDin the codebase.The change to the
_CHECK_IDconstant, moving from a numeric format to a hexadecimal string format, appears to be valid and aligns with the provided summary.However, please ensure that any other occurrences of
_CHECK_IDin the test files or the main codebase are updated to match this new format for consistency.Run the following script to verify the usage of
_CHECK_ID:Verification successful
Verification complete:
_CHECK_IDformat change is consistentThe change to the
_CHECK_IDconstant, moving from a numeric format to a hexadecimal string format, has been consistently applied across the codebase. The new format is defined once and used in multiple locations without any instances of the old format remaining.
- Definition:
_CHECK_ID = "838766BA2B995C00744175F69A1B11E32C3DBC40E64801A4056FCBD657F57334"- Usage: Found in 4 other locations, likely in function calls or assignments
No further action is required regarding this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `_CHECK_ID` match the new hexadecimal string format. # Test: Search for the `_CHECK_ID` usage. Expect: Only occurrences of the new format. rg --type python -A 5 $'_CHECK_ID'Length of output: 64
Script:
#!/bin/bash # Description: Verify all occurrences of `_CHECK_ID` in Python files # Find Python files and search for _CHECK_ID fd '\.py$' --exec rg -n '_CHECK_ID' {} \; # Search for the old numeric format (assuming it was a number like 19048) echo "Searching for potential old numeric format:" fd '\.py$' --exec rg -n '_CHECK_ID\s*=\s*\d+' {} \; # Search for the new hexadecimal string format echo "Searching for the new hexadecimal string format:" fd '\.py$' --exec rg -n '_CHECK_ID\s*=\s*"[0-9A-F]{64}"' {} \;Length of output: 683
snippets/paths.py (1)
2-2: The addition of a blank line at the beginning of the file is a trivial formatting change that does not affect the code's functionality or logic. No review comment is necessary.tests/unit/models/transactions/test_xchain_claim.py (1)
71-71: LGTM!The change from string to integer for the
destination_tagparameter is correct and aligns with the expected input type. This improves the type safety and robustness of thetest_successful_claim_destination_tagfunction.xrpl/utils/xrp_conversions.py (2)
39-39: LGTM!The change from
type()toisinstance()for type checking is a good refactor. It improves code robustness and maintainability without altering the core functionality.
87-87: LGTM!Similar to the previous function, the change from
type()toisinstance()for type checking is a good refactor. It improves code robustness and maintainability without altering the core functionality.xrpl/models/transactions/pseudo_transactions/enable_amendment.py (2)
5-5: LGTM!The import statement is correct and necessary to support the expanded type definition of the
flagsattribute.
104-104: Verify the impact of the expandedflagstype definition.The change to the
flagsattribute type is logically correct and enhances the flexibility of the attribute. The default value and documentation comment remain valid.However, this change may affect how the
flagsattribute is utilized throughout the codebase. Please ensure that:
- Existing code that interacts with the
flagsattribute has been updated to handle the new dictionary type.- The expanded type definition is properly validated and handled in the relevant methods of the
EnableAmendmentclass.You can use the following script to search for potential areas that may require updates:
Verification successful
Verification complete:
flagsattribute change appears isolatedThe expanded type definition for the
flagsattribute in theEnableAmendmentclass seems to have minimal impact on the codebase. Our analysis shows:
- No direct read or write access to the
flagsattribute was found outside the class definition.- No method definitions were found that use
flagsas a parameter.- The
flagsattribute is defined asUnion[Dict[str, bool], int, List[int]] = 0, consistent with the original review comment.The change appears to be isolated to the
EnableAmendmentclass definition. However, to ensure complete verification:
- Review any code that instantiates or processes
EnableAmendmentobjects to confirm compatibility with the new type definition.- Verify that the expanded type aligns with the intended use of the
flagsattribute as described in the documentation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flags` attribute in the codebase. # Test 1: Search for read access of the `flags` attribute. # Expect: Relevant code should handle the expanded type definition. rg --type python -A 5 $'\.flags' # Test 2: Search for write access to the `flags` attribute. # Expect: Relevant code should conform to the expanded type definition. rg --type python -A 5 $'flags\s*='Length of output: 131
Script:
#!/bin/bash # Description: Verify the usage of the `flags` attribute in the codebase using ast-grep # Test 1: Search for read access of the `flags` attribute echo "Read access of flags attribute:" ast-grep --lang python --pattern '$_.$_flags' # Test 2: Search for write access to the `flags` attribute echo "Write access to flags attribute:" ast-grep --lang python --pattern '$_.flags = $_' # Test 3: Search for method definitions that use the flags attribute as a parameter echo "Method definitions with flags as parameter:" ast-grep --lang python --pattern 'def $_($_, flags: $_, $_): $$$' # Test 4: Search for the EnableAmendment class definition echo "EnableAmendment class definition:" ast-grep --lang python --pattern 'class EnableAmendment($_): $$$'Length of output: 5906
xrpl/models/transactions/oracle_set.py (2)
23-26: LGTM!Explicitly casting
EPOCH_OFFSETto an integer improves type clarity and safety without altering the underlying value or logic.
149-149: LGTM!Updating the error message to use consistent terminology improves clarity and readability without altering the underlying meaning.
xrpl/models/requests/ledger_entry.py (2)
259-259: LGTM!The change to include
intas an additional valid type forxchain_claim_idattribute inLedgerEntryclass looks good. It aligns with the type annotation update in theXChainClaimIDclass and enhances the flexibility of theLedgerEntryclass in handling different data types for claims.
261-261: LGTM!The change to include
intas an additional valid type forxchain_create_account_claim_idattribute inLedgerEntryclass looks good. It aligns with the type annotation update in theXChainCreateAccountClaimIDclass and enhances the flexibility of theLedgerEntryclass in handling different data types for account claim IDs.tests/unit/models/transactions/test_oracle_set.py (1)
329-329: LGTM!The updated error message improves clarity by using consistent terminology for the ripple epoch reference. This change enhances readability without altering the test's functionality.
CHANGELOG.md (1)
21-21: Excellent addition to model validation!Checking parameter types as part of the validation process aligns perfectly with the PR objective. This enhancement can help catch type mismatches early, reducing potential runtime errors and improving overall data integrity.
xrpl/models/transactions/transaction.py (2)
Line range hint
267-267:
Line range hint
368-383: LGTM!The added exception handling for invalid
self.flagstype enhances the method's reliability and error reporting.tests/unit/models/test_base_model.py (7)
87-95: Effective test for invalid types intest_bad_typeThe test correctly verifies that the
Paymentmodel raises anXRPLModelExceptionwhen provided with incorrect types for theaccount,amount, anddestinationfields.
96-104: Validation offlagsfield type intest_bad_type_flagsThis test appropriately checks that passing a string instead of an integer to the
flagsfield results in anXRPLModelException, enhancing type safety.
106-114:test_bad_type_enumaccurately tests invalid enum valuesThe test ensures that providing an invalid value to the
subcommandenum in thePathFindmodel raises anXRPLModelException, which is essential for input validation.
425-437: Detection of nonexistent fields intest_nonexistent_fieldThis test correctly checks that the
TrustSetmodel raises anXRPLModelExceptionwhen an unexpected field (bad_field) is included, reinforcing model strictness.
440-440: Please address previous comment regarding clarity intest_bad_literalA previous reviewer suggested adding a comment to highlight the
bad_literalattribute in this test case for clarity. Please consider incorporating this to improve understandability.
821-843: Tests for input case sensitivity infrom_xrplmethods are comprehensiveThe added test
test_request_input_from_xrpl_accepts_camel_casecorrectly ensures thatBaseModel.from_xrplonly accepts inputs with camelCase or PascalCase keys, raising exceptions for snake_case keys. This reinforces consistency and correctness in input data handling.
845-862: Verification of key case sensitivity inTransaction.from_xrplThe test
test_transaction_input_from_xrpl_accepts_only_camel_caseappropriately verifies thatTransaction.from_xrplrejects inputs with snake_case keys, aligning with expected input formats and preventing potential parsing issues.
xrpl/models/base_model.py
Outdated
| if expected_type_origin is list: | ||
| # expected a List, received a List | ||
| if not isinstance(value, list): | ||
| return {"attr": f"{attr} is {type(value)}, expected {expected_type}"} |
There was a problem hiding this comment.
Fix the typo in error dictionary keys to correctly reference the variable attr.
In the return statements on lines 314, 328, and 348, the key of the error dictionary is mistakenly set to the string "attr" instead of the variable attr. This will result in error dictionaries with the key "attr" instead of the actual attribute name, which could lead to confusion when handling errors.
Apply this diff to correct the error:
- return {"attr": f"{attr} is {type(value)}, expected {expected_type}"}
+ return {attr: f"{attr} is {type(value)}, expected {expected_type}"}Also applies to: 328-328, 348-348
| "issuing_chain_door": issuer, | ||
| "issuing_chain_issue": {"currency": "XRP"}, | ||
| }, | ||
| "public_key": "0342E083EA762D91D621714C394", |
There was a problem hiding this comment.
Ensure public_key values do not expose sensitive information
The hard-coded public_key values at lines 448 and 471 may inadvertently contain sensitive data. Please confirm that these are not actual cryptographic keys. If they are placeholders for testing, consider annotating them as such or using clearly non-sensitive values to prevent any security concerns.
Also applies to: 471-471
Tools
Gitleaks
448-448: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
This is a sample string (that isn't even a valid public key), just used as a placeholder.
There was a problem hiding this comment.
@mvadari, thank you for the clarification. I've noted that the public_key values in this test file are placeholders and do not represent actual cryptographic keys.
(_/)
(•‿•)
Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl-py#708
File: tests/unit/models/test_base_model.py:448-448
Timestamp: 2024-09-19T20:01:37.621Z
Learning: In `tests/unit/models/test_base_model.py`, the `public_key` values are sample strings used as placeholders and do not contain actual cryptographic keys.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
High Level Overview of Change
This PR adds additional validation for models to also check parameter types and ensure they match what is expected. It also fixes issues that were uncovered via adding this additional checking.
Context of Change
It's a common annoyance and improves the user experience.
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added tests, and added a few additional tests to improve code coverage in the
base_model.pyfile. CI passes.Summary by CodeRabbit
Release Notes
New Features
featureRPC.Improvements
Bug Fixes
Documentation
These changes enhance transaction handling, improve error reporting, and increase the robustness of data validation processes.