fix: remove # type: ignore after each REQUIRED #765
Conversation
WalkthroughThe pull request implements a series of modifications across various files in the XRPL codebase, primarily focusing on the removal of Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (17)
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
|
mvadari
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
xrpl/models/requests/manifest.py (1)
Line range hint
27-31: Consider enhancing field documentation with type information.While the docstring indicates this is a required field, it would be helpful to also document the expected type (str) for better developer experience.
""" - This field is required. + This field is required. + + :type: str :meta hide-value: """xrpl/models/requests/channel_verify.py (1)
24-28: Consider enhancing field documentation with format specifications.While the basic documentation is present, it would be helpful to add:
- Expected format for
channel_id(hex string)- Format and constraints for
amount(XRP amount)- Format for
public_key(hex string)- Format for
signature(hex string)Example enhancement for channel_id:
channel_id: str = REQUIRED """ - This field is required. + This field is required. + + :param channel_id: The Channel ID as a hex string + :type channel_id: str + :example: "5DB01B7FFED6B67E6B0414DED11E051D2EE2B7619CE0EAA6286D67A3A4D5BDB3" :meta hide-value: """Also applies to: 31-35, 38-42, 45-49
xrpl/models/requests/no_ripple_check.py (1)
Line range hint
51-52: Consider adding docstrings for optional parametersThe
transactionsandlimitfields would benefit from docstrings explaining their purpose and impact, similar to the required fields above them.Add documentation like this:
transactions: bool = False + """ + If true, return suggested transactions to fix the problems. + """ limit: Optional[int] = 300 + """ + Maximum number of trust line problems to return (default: 300). + """xrpl/models/requests/get_aggregate_price.py (1)
35-36: Consider enhancing the documentation for the oracles field.The type annotation is correct and the validation in
_get_errors()ensures a non-empty list. However, the documentation could be more descriptive about what constitutes a valid Oracle identifier and its format.Consider expanding the documentation like this:
- """The oracle identifier""" + """A non-empty list of Oracle identifiers to fetch prices from. Each Oracle must be + a valid identifier in the format specified by the Oracle class."""xrpl/models/transactions/nftoken_burn.py (1)
Line range hint
36-41: Consider enhancing the nftoken_id field documentationWhile the type annotation change is correct, the documentation could be more helpful by including:
- The expected format of an NFToken ID
- Any validation rules or constraints
- An example value
Example enhancement:
""" - Identifies the NFToken to be burned. This field is required. + Identifies the NFToken to be burned. This field is required. + + The NFToken ID is a 64-character hexadecimal string that uniquely identifies + the token. Example: "00081388DC5AA8D5F45498ED961B89B7FE85AA844B84148C052D0454DAC0D494" :meta hide-value: """xrpl/models/transactions/xchain_account_create_commit.py (1)
27-27: LGTM! Type annotations are now properly handled.The removal of
# type: ignorecomments improves code clarity while maintaining type safety. The field declarations are well-documented and follow XRPL conventions.Consider using a custom type (e.g.,
XRPAmount) forsignature_rewardandamountfields to better represent XRP amounts and enforce validation at the type level. This would make the API more type-safe and self-documenting.Also applies to: 34-34, 43-43, 50-50
xrpl/models/transactions/pseudo_transactions/unl_modify.py (1)
Line range hint
36-43: Consider using an enum for unl_modify_disabling.While the current implementation using an int with validation is functional, consider using an enum to make the binary state more type-safe and self-documenting.
Here's a suggested implementation:
from enum import IntEnum class UNLModifyState(IntEnum): REMOVE = 0 # Remove validator from Negative UNL ADD = 1 # Add validator to Negative UNL # Then in the class: unl_modify_disabling: UNLModifyState = REQUIRED # Update validation: def _get_errors(self: Self) -> Dict[str, str]: errors = super()._get_errors() if not isinstance(self.unl_modify_disabling, UNLModifyState): errors["unl_modify_disabling"] = "Invalid UNLModifyState value" return errorsAlso applies to: 65-70
xrpl/models/transactions/payment_channel_create.py (1)
Line range hint
32-39: Consider adding runtime validation for destination constraint.The docstring mentions that destination "Cannot be the same as the sender", but this business rule isn't enforced in the code.
Consider adding validation in the post-init method:
def __post_init__(self): super().__post_init__() if self.destination == self.account: raise ValueError("Destination account cannot be the same as the sender account")xrpl/models/transactions/xchain_claim.py (1)
Line range hint
53-58: Consider validating destination_tag in _get_errors.While
destination_tagis optional, when provided it should be validated. Consider adding range validation in_get_errors.def _get_errors(self: Self) -> Dict[str, str]: errors = super()._get_errors() + # Validate destination_tag if provided + if self.destination_tag is not None: + if self.destination_tag < 0: + errors["destination_tag"] = "destination_tag must be non-negative" bridge = self.xchain_bridgexrpl/models/transactions/amm_bid.py (1)
40-43: LGTM! Consider enhancing the documentation.The removal of
# type: ignoreis consistent with theassetfield change.Consider clarifying the relationship between
assetandasset2in the documentation:- The definition for the other asset in the AMM's pool. This field is required. + The definition for the other asset in the AMM's pool, forming a trading pair with + the first asset. This field is required.xrpl/models/requests/channel_authorize.py (1)
Based on the gathered information, I can now provide the final response:
Security documentation needs enhancement in sign-related classes
While
ChannelAuthorizehas comprehensive security warnings about handling secrets, similar warnings are missing in other security-critical classes that handle sensitive data:
Signclass only has a brief caution about local signingSignForandSignAndSubmitclasses lack security warnings about handling private keysWalletclass, which manages cryptographic keys, needs security documentation about key storage and handlingThe security documentation should be consistent across all classes handling sensitive cryptographic material, following the thorough example set in
ChannelAuthorize.🔗 Analysis chain
Line range hint
1-89: Verify security documentation is complete.This class handles cryptographic operations for payment channel authorization. While the existing security warnings are good, we should verify they cover all security aspects.
Let's check if similar security-critical files have consistent warning messages:
Let me gather more information about security-related code and documentation.
Let me check the security documentation in the sign-related files and wallet implementation since they also handle sensitive data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check security warnings in related files # Expected: Consistent security warning messages across similar crypto-related files # Search for files with similar security implications rg -l "Warning.*secret|Warning.*security" --type py # Check the specific warning message pattern rg "Warning:.*secret key.*unsecured network" --type pyLength of output: 406
Script:
#!/bin/bash # Search for other files handling secrets, keys, or crypto operations rg -l "secret|seed|passphrase|key_type|CryptoAlgorithm" --type py # Search for classes similar to ChannelAuthorize that handle sensitive data ast-grep --pattern 'class $_ { $$$ secret: $_ $$$ }' # Look for any security-related docstrings rg "security|warning|caution|important" --type py -A 2Length of output: 2511
Script:
#!/bin/bash # Check security documentation in sign-related files and wallet rg -B 2 -A 10 "class (Sign|SignFor|SignAndSubmit|Wallet)" --type py # Look for any existing security warnings in these files cat xrpl/models/requests/sign.py xrpl/models/requests/sign_for.py xrpl/models/requests/sign_and_submit.py xrpl/wallet/main.py | rg -B 2 -A 4 "Warning|security|caution|important" # Check if there are any security-related tests rg -l "test.*security|security.*test" --type pyLength of output: 6725
xrpl/models/transactions/xchain_create_bridge.py (1)
Line range hint
36-42: Consider using a more specific type for signature_rewardWhile removing
# type: ignoreis correct, the field is typed asstrbut is validated to be numeric in_get_errors(). Consider using a more specific type annotation to enforce this at the type level.You could improve type safety by:
- Using
typing.NewTypeto create a specific numeric string type- Or adding a validator/converter to ensure numeric strings
from typing import NewType NumericString = NewType('NumericString', str) # Then use: signature_reward: NumericString = REQUIREDtools/generate_tx_models.py (1)
Line range hint
1-262: Consider adding validation for generated models.While the script effectively generates transaction models, consider adding these improvements for better maintainability:
- Add a validation step that verifies generated models compile without type errors
- Consider generating type tests to ensure type safety of required fields
- Add comments in generated files indicating they are auto-generated
This will help catch any typing issues early and make maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (80)
tools/generate_tx_models.py(1 hunks)xrpl/models/amounts/issued_currency_amount.py(1 hunks)xrpl/models/auth_account.py(2 hunks)xrpl/models/currencies/issued_currency.py(1 hunks)xrpl/models/requests/account_channels.py(2 hunks)xrpl/models/requests/account_currencies.py(2 hunks)xrpl/models/requests/account_info.py(2 hunks)xrpl/models/requests/account_lines.py(2 hunks)xrpl/models/requests/account_nfts.py(2 hunks)xrpl/models/requests/account_objects.py(2 hunks)xrpl/models/requests/account_offers.py(2 hunks)xrpl/models/requests/account_tx.py(2 hunks)xrpl/models/requests/book_offers.py(2 hunks)xrpl/models/requests/channel_authorize.py(1 hunks)xrpl/models/requests/channel_verify.py(2 hunks)xrpl/models/requests/deposit_authorized.py(2 hunks)xrpl/models/requests/gateway_balances.py(2 hunks)xrpl/models/requests/get_aggregate_price.py(1 hunks)xrpl/models/requests/ledger_entry.py(9 hunks)xrpl/models/requests/manifest.py(2 hunks)xrpl/models/requests/nft_buy_offers.py(2 hunks)xrpl/models/requests/nft_history.py(2 hunks)xrpl/models/requests/nft_info.py(2 hunks)xrpl/models/requests/nft_sell_offers.py(2 hunks)xrpl/models/requests/nfts_by_issuer.py(2 hunks)xrpl/models/requests/no_ripple_check.py(2 hunks)xrpl/models/requests/path_find.py(2 hunks)xrpl/models/requests/request.py(1 hunks)xrpl/models/requests/ripple_path_find.py(2 hunks)xrpl/models/requests/sign.py(1 hunks)xrpl/models/requests/sign_and_submit.py(1 hunks)xrpl/models/requests/sign_for.py(1 hunks)xrpl/models/requests/submit_multisigned.py(1 hunks)xrpl/models/requests/submit_only.py(1 hunks)xrpl/models/requests/subscribe.py(2 hunks)xrpl/models/requests/transaction_entry.py(1 hunks)xrpl/models/requests/unsubscribe.py(2 hunks)xrpl/models/required.py(1 hunks)xrpl/models/response.py(3 hunks)xrpl/models/transactions/account_delete.py(1 hunks)xrpl/models/transactions/amm_bid.py(1 hunks)xrpl/models/transactions/amm_create.py(1 hunks)xrpl/models/transactions/amm_delete.py(2 hunks)xrpl/models/transactions/amm_deposit.py(1 hunks)xrpl/models/transactions/amm_vote.py(1 hunks)xrpl/models/transactions/amm_withdraw.py(1 hunks)xrpl/models/transactions/check_cancel.py(2 hunks)xrpl/models/transactions/check_cash.py(1 hunks)xrpl/models/transactions/check_create.py(3 hunks)xrpl/models/transactions/clawback.py(1 hunks)xrpl/models/transactions/escrow_cancel.py(1 hunks)xrpl/models/transactions/escrow_create.py(1 hunks)xrpl/models/transactions/escrow_finish.py(1 hunks)xrpl/models/transactions/nftoken_burn.py(2 hunks)xrpl/models/transactions/nftoken_cancel_offer.py(1 hunks)xrpl/models/transactions/nftoken_create_offer.py(1 hunks)xrpl/models/transactions/nftoken_mint.py(1 hunks)xrpl/models/transactions/offer_cancel.py(2 hunks)xrpl/models/transactions/offer_create.py(2 hunks)xrpl/models/transactions/oracle_delete.py(1 hunks)xrpl/models/transactions/oracle_set.py(3 hunks)xrpl/models/transactions/payment.py(1 hunks)xrpl/models/transactions/payment_channel_claim.py(2 hunks)xrpl/models/transactions/payment_channel_create.py(4 hunks)xrpl/models/transactions/payment_channel_fund.py(2 hunks)xrpl/models/transactions/pseudo_transactions/enable_amendment.py(2 hunks)xrpl/models/transactions/pseudo_transactions/pseudo_transaction.py(1 hunks)xrpl/models/transactions/pseudo_transactions/unl_modify.py(3 hunks)xrpl/models/transactions/signer_list_set.py(2 hunks)xrpl/models/transactions/ticket_create.py(1 hunks)xrpl/models/transactions/transaction.py(3 hunks)xrpl/models/transactions/trust_set.py(2 hunks)xrpl/models/transactions/xchain_account_create_commit.py(2 hunks)xrpl/models/transactions/xchain_add_account_create_attestation.py(2 hunks)xrpl/models/transactions/xchain_add_claim_attestation.py(2 hunks)xrpl/models/transactions/xchain_claim.py(2 hunks)xrpl/models/transactions/xchain_commit.py(2 hunks)xrpl/models/transactions/xchain_create_bridge.py(1 hunks)xrpl/models/transactions/xchain_create_claim_id.py(2 hunks)xrpl/models/transactions/xchain_modify_bridge.py(1 hunks)
✅ Files skipped from review due to trivial changes (27)
- xrpl/models/auth_account.py
- xrpl/models/currencies/issued_currency.py
- xrpl/models/requests/account_lines.py
- xrpl/models/requests/account_offers.py
- xrpl/models/requests/account_tx.py
- xrpl/models/requests/deposit_authorized.py
- xrpl/models/requests/nft_history.py
- xrpl/models/requests/request.py
- xrpl/models/requests/sign.py
- xrpl/models/requests/submit_multisigned.py
- xrpl/models/requests/unsubscribe.py
- xrpl/models/response.py
- xrpl/models/transactions/amm_vote.py
- xrpl/models/transactions/check_cancel.py
- xrpl/models/transactions/check_cash.py
- xrpl/models/transactions/check_create.py
- xrpl/models/transactions/clawback.py
- xrpl/models/transactions/escrow_finish.py
- xrpl/models/transactions/nftoken_cancel_offer.py
- xrpl/models/transactions/nftoken_mint.py
- xrpl/models/transactions/oracle_delete.py
- xrpl/models/transactions/signer_list_set.py
- xrpl/models/transactions/transaction.py
- xrpl/models/transactions/xchain_add_account_create_attestation.py
- xrpl/models/transactions/xchain_commit.py
- xrpl/models/transactions/xchain_create_claim_id.py
- xrpl/models/transactions/xchain_modify_bridge.py
🧰 Additional context used
📓 Learnings (1)
xrpl/models/requests/account_currencies.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-10-30T20:32:03.246Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
🔇 Additional comments (111)
xrpl/models/required.py (1)
7-7: LGTM! The type change improves type safety.
The change from Final[object] to Final[Any] is a good improvement because:
- It better represents the sentinel's purpose of being assignable to any required field type
- It eliminates the need for
# type: ignorecomments throughout the codebase - The
# noqa: ANN401suppression is appropriate here sinceAnyis intentionally used for the sentinel value
Let's verify that this change has successfully removed the need for # type: ignore comments:
Also applies to: 11-11
✅ Verification successful
The type change successfully improves type safety
The verification confirms that:
- The
REQUIREDsentinel is now correctly typed asFinal[Any]inxrpl/models/required.py - No
type: ignorecomments related toREQUIREDusage exist in the codebase - The remaining
type: ignorecomments are unrelated toREQUIREDand are for other legitimate use cases (e.g., third-party library imports)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any remaining "# type: ignore" comments related to REQUIRED
# Expected: No matches, confirming all such comments were removed
# Search for any remaining "type: ignore" comments near REQUIRED usage
rg -B 2 -A 2 'REQUIRED.*type:\s*ignore'
Length of output: 41
Script:
#!/bin/bash
# Let's try a different approach to verify the changes:
# 1. First, let's find all files that use REQUIRED
rg "REQUIRED" --type python
# 2. Then check for any remaining type: ignore comments in those files
rg "type:\s*ignore" --type python
# 3. Also verify the current type annotation of REQUIRED
rg -A 2 "REQUIRED\s*=" --type python
Length of output: 195
Script:
#!/bin/bash
# Let's try without file type filtering since python type is not recognized
# 1. Find all files that use REQUIRED
rg "REQUIRED"
# 2. Check for any remaining type: ignore comments
rg "type:\s*ignore"
# 3. Verify the current type annotation of REQUIRED
rg -A 2 "REQUIRED\s*:"
Length of output: 23930
xrpl/models/requests/nft_info.py (1)
22-22: LGTM! Type system improvement.
The removal of # type: ignore comment while maintaining the REQUIRED field declaration indicates proper typing support. This change improves code quality by enabling stricter type checking without compromising functionality.
Let's verify that this typing pattern is consistent across the codebase:
✅ Verification successful
Type system improvement verified across codebase
The removal of # type: ignore comment is consistent with the codebase's type system improvements. The search results confirm:
- No remaining
type: ignorecomments withREQUIREDfields exist in the codebase REQUIREDis consistently imported and used across all model files- The pattern of declaring required fields using
field_name: type = REQUIREDis uniform throughout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No occurrences, confirming consistent typing cleanup
# Search for any remaining "type: ignore" comments near REQUIRED
rg -l "REQUIRED.*#.*type:\s*ignore" || echo "No remaining type: ignore comments found with REQUIRED"
# Verify the pattern of REQUIRED usage is consistent
ast-grep --pattern 'REQUIRED' -l
Length of output: 295
Script:
#!/bin/bash
# Let's try a more specific search with ripgrep to find REQUIRED usage patterns
# and their context in Python files
# Search for REQUIRED field declarations in Python files
rg -t py "REQUIRED\s*$" -B1 -A1
# Also check for any remaining type comments with REQUIRED
rg -t py "REQUIRED.*#.*type" -B1 -A1
# Check imports of REQUIRED to verify consistency
rg -t py "^from.*import.*REQUIRED"
Length of output: 63004
xrpl/models/requests/nft_buy_offers.py (1)
22-22: LGTM! Type checking improvement confirmed.
The removal of # type: ignore while maintaining REQUIRED indicates proper typing support, which improves code quality without changing behavior.
Let's verify the type checking works correctly:
✅ Verification successful
Type checking improvement verified successfully
The verification shows:
- No remaining
# type: ignorecomments innft_buy_offers.py REQUIREDis properly imported fromxrpl.models.required- The field is used consistently with other required fields across the codebase
- The type annotation pattern
field: type = REQUIREDis used uniformly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that REQUIRED is properly typed across the codebase
# and no type: ignore comments remain for REQUIRED fields
# Test 1: Check if any REQUIRED fields still have type: ignore
rg "REQUIRED.*# type: ignore" .
# Test 2: Verify the typing implementation of REQUIRED
ast-grep --pattern 'REQUIRED = $_'
Length of output: 70
Script:
#!/bin/bash
# Let's try a different approach to verify the typing implementation
# Check the definition of REQUIRED constant
rg "REQUIRED\s*=" -A 2
# Check all usages of REQUIRED in type annotations
rg ":\s*.*REQUIRED"
# Check if there are any remaining type: ignore comments in the file
rg "# type: ignore" "xrpl/models/requests/nft_buy_offers.py"
# Look for the import of REQUIRED to understand its type
rg "from.*import.*REQUIRED"
Length of output: 20882
xrpl/models/requests/nft_sell_offers.py (1)
22-22: LGTM! Type annotation improvement
The removal of # type: ignore while maintaining REQUIRED is correct and aligns with the PR's objective to improve type safety. The field remains properly typed as a required string.
Let's verify this is part of a consistent pattern across the codebase:
✅ Verification successful
Let me verify further to ensure the change is consistent across the codebase:
This will help us verify:
- The pattern of REQUIRED field declarations across the codebase
- The import statements to confirm REQUIRED is properly imported everywhere it's used
Based on the comprehensive results, I can now provide the final response:
Type annotation improvement verified across codebase
The removal of # type: ignore from nft_id: str = REQUIRED is consistent with the codebase-wide pattern. The verification shows:
- No remaining
type: ignoreannotations with REQUIRED fields - Consistent usage of REQUIRED across all models with proper type annotations
- Proper imports of REQUIRED from
xrpl.models.requiredmodule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that type: ignore is being consistently removed from REQUIRED fields
# and that no new type: ignore comments are being added.
# Test: Search for any remaining "type: ignore" with REQUIRED
# Expect: No results or only in files not part of this PR
rg "REQUIRED.*# type: ignore" || echo "No remaining type: ignore with REQUIRED found"
Length of output: 132
Script:
#!/bin/bash
# Check for all REQUIRED field usages to ensure consistent typing
rg ":\s*\w+\s*=\s*REQUIRED" -A 1
# Also check imports of REQUIRED to ensure it's properly used
rg "from.*import.*REQUIRED"
Length of output: 28392
xrpl/models/requests/manifest.py (1)
26-26: LGTM! Improved type safety by removing type check suppression.
The removal of # type: ignore indicates that the typing system now correctly understands REQUIRED, which improves type safety without changing functionality.
xrpl/models/requests/nfts_by_issuer.py (2)
Line range hint 1-35: Well-structured class with proper documentation
The class maintains excellent documentation standards and follows best practices:
- Clear class and field-level documentation
- Proper use of dataclass with frozen=True for immutability
- Appropriate inheritance structure
- Clear type annotations
23-23: LGTM! Type annotation improvement
The removal of # type: ignore is appropriate as it aligns with the PR's goal to fix typing issues with REQUIRED fields.
Let's verify that similar changes have been made consistently across the codebase:
✅ Verification successful
Based on the comprehensive search results, I can now generate the final response:
Type annotation cleanup is consistent across codebase
The removal of # type: ignore from issuer: str = REQUIRED is part of a consistent cleanup across the codebase. The search results confirm:
REQUIREDis properly defined inxrpl/models/required.pyas a typed constant- No remaining
type: ignorecomments exist alongsideREQUIREDfield declarations - The pattern
field: type = REQUIREDis used consistently across many models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No occurrences, confirming consistent cleanup
# Search for any remaining "type: ignore" comments on lines with REQUIRED
rg "REQUIRED.*# type: ignore"
# Also check for any potential variations
rg "#.*type: ignore.*REQUIRED"
Length of output: 64
Script:
#!/bin/bash
# Let's verify this more thoroughly with different approaches
# 1. First, find all occurrences of REQUIRED to understand its usage
rg "REQUIRED" -B 1 -A 1
# 2. Search for any type ignore comments in general to ensure we haven't missed any format
rg "type:\s*ignore" --type py
# 3. Look for the REQUIRED constant definition to understand its typing
rg "REQUIRED\s*=" --type py
# 4. Find all files that might use REQUIRED to verify consistent typing
fd -e py -x grep -l "REQUIRED" {}
Length of output: 64982
xrpl/models/transactions/ticket_create.py (1)
20-20: LGTM! Type safety improvement.
The removal of # type: ignore while maintaining the REQUIRED constraint improves type safety without changing the behavior.
Let's verify that similar changes are consistent across the codebase:
✅ Verification successful
Type safety improvement verified across codebase
The verification confirms:
- No remaining
# type: ignorecomments withREQUIREDfields - Consistent typing pattern for required fields across all models
ticket_count: int = REQUIREDin ticket_create.py follows the same pattern as other required fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no remaining "type: ignore" comments with REQUIRED fields
# and ensure consistent typing approach across the codebase
# Test 1: Check for any remaining "type: ignore" comments with REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg "REQUIRED.*# type: ignore" || echo "✓ No remaining type: ignore comments with REQUIRED"
# Test 2: Verify consistent pattern of required field declarations
echo -e "\nVerifying consistent pattern of required fields..."
rg ":\s*\w+\s*=\s*REQUIRED\s*$"
Length of output: 12766
xrpl/models/requests/book_offers.py (3)
23-28: LGTM! Type annotation improvement for taker_gets
The removal of # type: ignore with proper typing of Currency improves type safety while maintaining the required field constraint.
Line range hint 30-35: LGTM! Type annotation improvement for taker_pays
The removal of # type: ignore with proper typing of Currency improves type safety while maintaining the required field constraint.
Line range hint 23-35: Verify the impact of type changes
Let's verify that these type changes don't affect other parts of the codebase that might be using these fields.
✅ Verification successful
Let me check the test files and usages to ensure there are no compatibility issues with the type changes.
Type changes are safe and properly used across the codebase
The verification shows that:
- The test files correctly use the
Currencytype with bothXRP()andIssuedCurrencyimplementations - No type-ignore comments related to these fields exist
- The type usage in tests matches the class definition
- The class is properly imported and used in test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining type: ignore comments related to Currency fields
# and verify usage of BookOffers class
# Check for any remaining type: ignore comments with Currency
echo "Checking for remaining type: ignore comments with Currency..."
rg "Currency.*=.*REQUIRED.*#.*type:.*ignore"
# Check for imports and usage of BookOffers class
echo "Checking BookOffers usage..."
rg -l "BookOffers"
# Check for any test files related to BookOffers
echo "Checking for related test files..."
fd -g "*test*book_offers*.py"
Length of output: 568
Script:
#!/bin/bash
# Check the content of test files and usages
echo "Checking test_book_offers.py content..."
cat tests/integration/reqs/test_book_offers.py
echo -e "\nChecking BookOffers class usage in test_base_model.py..."
rg "BookOffers" tests/unit/models/test_base_model.py -A 5
echo -e "\nChecking the actual BookOffers class definition..."
cat xrpl/models/requests/book_offers.py
Length of output: 2648
xrpl/models/transactions/offer_cancel.py (1)
20-20: LGTM! Type annotation looks correct.
The removal of # type: ignore is appropriate now that the typing for REQUIRED has been fixed upstream. The type annotation int is correct for a sequence number field.
Let's verify that similar changes were made consistently across the codebase:
✅ Verification successful
Type annotation changes are consistent across the codebase
The verification shows that all REQUIRED usages across the codebase follow a consistent pattern with proper type annotations and no remaining # type: ignore comments. The offer_sequence: int = REQUIRED in offer_cancel.py aligns with this pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No occurrences, confirming consistent cleanup
# Search for any remaining "type: ignore" comments on lines with REQUIRED
rg "REQUIRED.*# type: ignore"
# Also verify the proper usage of REQUIRED without type ignores
rg ":\s*\w+\s*=\s*REQUIRED\s*$"
Length of output: 12405
xrpl/models/transactions/escrow_cancel.py (3)
20-24: LGTM: Required field declaration looks correct.
The owner field is properly typed as a required string field with clear documentation explaining its purpose.
Line range hint 27-33: LGTM: Required field declaration looks correct.
The offer_sequence field is properly typed as a required integer field with comprehensive documentation explaining its relationship to the original EscrowCreate transaction.
Line range hint 20-33: Verify consistent type handling across transaction models.
The removal of # type: ignore comments looks good. Let's verify that similar transaction models follow the same pattern and that the type checker is satisfied.
✅ Verification successful
Let me try a different approach to verify the type handling consistency.
Type handling is consistent across transaction models
The verification shows that:
- No remaining
# type: ignorecomments exist withREQUIREDfields - The type annotation pattern
field_name: type = REQUIREDis consistently used across all transaction models - The
escrow_cancel.pyfollows the same type handling pattern as other transaction models in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining type: ignore comments on REQUIRED fields
# and verify consistent pattern usage across transaction models
# Check for any remaining "type: ignore" comments near REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg -l "REQUIRED.*# type: ignore" "xrpl/models/transactions/"
# Verify the pattern of required field declarations
echo "Checking pattern of required field declarations..."
ast-grep --pattern 'class $CLASS($BASE) {
$$$
$FIELD: $TYPE = REQUIRED
$$$
}'
Length of output: 390
Script:
#!/bin/bash
# Check type annotations and REQUIRED usage patterns in transaction models
# Look for all transaction model files
echo "Transaction model files:"
fd -e py . "xrpl/models/transactions"
# Check the pattern of field declarations with type annotations
echo -e "\nField declarations with REQUIRED:"
rg "^[[:space:]]+\w+:[[:space:]]+\w+[[:space:]]*=[[:space:]]*REQUIRED" "xrpl/models/transactions/" -A 1
# Look for any remaining type-related comments
echo -e "\nAny remaining type-related comments:"
rg -i "type" "xrpl/models/transactions/escrow_cancel.py" -B 1 -A 1
Length of output: 17131
xrpl/models/requests/gateway_balances.py (1)
26-26: LGTM! Type safety improvement
The removal of # type: ignore aligns with the PR objective to fix typing issues with REQUIRED. This change enables proper type checking while maintaining the same runtime behavior.
Let's verify that type checking passes for all similar changes:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to ensure the type safety of REQUIRED usage.
This will help us verify:
- The implementation of
REQUIREDand its type safety - How it's integrated in the base model
- Consistent usage pattern across different model types
Based on the results, I can now generate the final response as we have sufficient information about the REQUIRED implementation and its usage across the codebase.
Type safety for REQUIRED is properly implemented
The implementation in required.py shows REQUIRED is correctly typed as Final[Any], and the base model implementation properly handles required field validation through:
- Type hints validation in
from_dict - Post-initialization validation in
__post_init__ - Error checking in
_get_errors
The removal of # type: ignore is safe as the type system now properly understands REQUIRED fields across all model types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no type-ignore comments remain for REQUIRED fields
# and that type checking passes for these changes
# Test 1: Check for any remaining type-ignore comments near REQUIRED
rg -l "REQUIRED.*# type: ignore" || echo "No type-ignore comments found with REQUIRED"
# Test 2: Find all files using REQUIRED to ensure consistent pattern
rg -l "REQUIRED" --type py
Length of output: 3516
Script:
#!/bin/bash
# Check the definition and usage pattern of REQUIRED
# to verify type safety implementation
# Check the REQUIRED definition
cat xrpl/models/required.py
# Check base model implementation
cat xrpl/models/base_model.py
# Sample a few model files to verify consistent usage pattern
head -n 30 xrpl/models/requests/gateway_balances.py
head -n 30 xrpl/models/transactions/payment.py
head -n 30 xrpl/models/amounts/issued_currency_amount.py
Length of output: 15166
xrpl/models/requests/account_info.py (4)
9-9: LGTM: Clean import addition
The field import from dataclasses is properly grouped with related imports and follows the module's import style.
29-29: LGTM: Clean type annotation
The removal of # type: ignore while maintaining the str type annotation for the account field is correct. This aligns with the PR's objective of improving type checking without suppressing warnings.
Line range hint 34-34: LGTM: Well-structured method field
The method field is correctly defined using field(default=RequestMethod.ACCOUNT_INFO, init=False), which:
- Ensures the method is always set to ACCOUNT_INFO
- Prevents it from appearing in the constructor parameters
Line range hint 29-34: Verify consistent application across request models
Let's verify that similar changes are applied consistently across other request models and that the REQUIRED constant's type supports this change.
✅ Verification successful
Let me gather more information to verify the consistency of REQUIRED usage across request models.
Changes are consistently applied across request models
The verification shows that:
- No remaining
type: ignoreannotations exist with REQUIRED fields - REQUIRED is properly typed as
Final[Any]in required.py - All request models consistently use the same pattern for required fields without type ignores
- The changes in account_info.py align with the pattern used across all other request models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining type ignores with REQUIRED
echo "Checking for remaining 'type: ignore' with REQUIRED..."
rg "REQUIRED.*# type: ignore" "xrpl/models"
# Verify REQUIRED's type definition
echo "Checking REQUIRED's type definition..."
rg "REQUIRED.*=.*" "xrpl/models/required.py"
# Check method field pattern in other request models
echo "Checking method field pattern in other request models..."
rg "method: RequestMethod = field\(.*init=False\)" "xrpl/models/requests/"
Length of output: 5884
Script:
#!/bin/bash
# Check for REQUIRED field usage patterns in request models
echo "Checking REQUIRED field patterns in request models..."
rg ":\s*\w+\s*=\s*REQUIRED" "xrpl/models/requests/"
# Check for any remaining type annotations with REQUIRED
echo "Checking for type annotations with REQUIRED..."
ast-grep --pattern 'REQUIRED # type: $_'
# Check for other REQUIRED field patterns that might need updating
echo "Checking other REQUIRED patterns..."
rg "REQUIRED.*=.*REQUIRED" "xrpl/models/requests/"
Length of output: 4681
xrpl/models/requests/channel_verify.py (1)
24-24: LGTM! Removal of type ignores is correct.
The removal of # type: ignore comments from the required fields aligns with the PR objectives and improves type safety. The CI tests passing confirm that the typing system now properly handles these fields.
Let's verify that similar changes are consistent across the codebase:
Also applies to: 31-31, 38-38, 45-45
✅ Verification successful
Removal of type ignores from REQUIRED fields is correct and consistent
The verification confirms that:
- The
channel_verify.pyfile correctly defines the REQUIRED fields without type ignore comments - All REQUIRED field usages across the codebase follow the same pattern without type ignore comments
- Remaining type ignore comments in the codebase are unrelated to REQUIRED fields and are used for different purposes (e.g., third-party imports, runtime type checks)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No results, indicating all have been removed
rg "REQUIRED.*# type: ignore" --type py
Length of output: 41
Script:
#!/bin/bash
# Let's verify this more thoroughly with multiple checks
# 1. Check for any REQUIRED fields in the codebase
echo "=== Checking for REQUIRED field usage ==="
rg ":\s*\w+\s*=\s*REQUIRED" --type py
# 2. Check for any remaining type: ignore comments
echo -e "\n=== Checking for type: ignore comments ==="
rg "# type: ignore" --type py
# 3. Specifically check the channel_verify.py file
echo -e "\n=== Checking channel_verify.py content ==="
cat xrpl/models/requests/channel_verify.py
# 4. Check for similar request models to ensure consistency
echo -e "\n=== Checking other request models ==="
fd "^.*\.py$" xrpl/models/requests --exec grep -l "REQUIRED"
Length of output: 16590
xrpl/models/requests/account_currencies.py (1)
31-31: LGTM! Type annotation improvement.
The removal of # type: ignore for the REQUIRED field aligns with the PR's objective to improve type checking. Since CI tests are passing, this confirms that the typing works correctly without the suppression.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
LGTM! Type annotation improvement verified across codebase
The removal of # type: ignore for the REQUIRED field is consistent with the rest of the codebase. The search results show that:
- No remaining instances of
type: ignorewithREQUIREDassignments exist - All
REQUIREDfield usages across the codebase follow the same clean pattern without type ignores - The change aligns with the established typing patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No occurrences of "type: ignore" next to REQUIRED assignments
# Search for any remaining type ignore comments with REQUIRED
rg "REQUIRED.*#.*type:\s*ignore"
# Also check the inverse pattern
rg "#.*type:\s*ignore.*REQUIRED"
Length of output: 69
Script:
#!/bin/bash
# Let's try a different approach to verify the REQUIRED field usage
# First, let's find all files that use REQUIRED
rg "REQUIRED\s*$" -l
# Then let's see the actual usage patterns of REQUIRED in these files
rg "REQUIRED\s*$" -B1 -A1
# Also check for any remaining type comments in general around field definitions
rg ":\s*\w+\s*=.*#.*type:"
Length of output: 59656
xrpl/models/requests/transaction_entry.py (1)
30-30: LGTM! The type annotation removal aligns with the PR objectives.
The removal of # type: ignore for the tx_hash field is consistent with the PR's goal of improving type safety.
Let's verify that all similar type ignore comments have been addressed:
✅ Verification successful
The removal of # type: ignore from tx_hash field is safe and verified
The verification shows that:
- The
REQUIREDfield is consistently used across many models without# type: ignore - The remaining
type: ignorecomments in the codebase are used for different purposes (e.g., type casting infrom_dictmethods) - The change aligns with similar field declarations in other request models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining REQUIRED fields with type ignore comments
# Expect: No results, indicating all type ignores have been removed
# Search for REQUIRED fields that still have type ignore comments
rg "REQUIRED.*# type: ignore"
# Search for any remaining type ignore comments related to REQUIRED
rg "# type: ignore.*REQUIRED"
Length of output: 63
Script:
#!/bin/bash
# Let's verify this more thoroughly with different approaches
# 1. First, find all files that use REQUIRED to understand the pattern
rg "REQUIRED" -l
# 2. Check the specific pattern of field declarations with REQUIRED
rg ".*:\s*\w+\s*=\s*REQUIRED" -A 1
# 3. Look for any remaining type: ignore comments in the codebase
rg "# type: ignore" ./xrpl/models/
# 4. Check the specific file for context
cat ./xrpl/models/requests/transaction_entry.py
Length of output: 26971
xrpl/models/requests/account_nfts.py (2)
Line range hint 1-35: Class structure and documentation look good.
The class is well-structured with:
- Clear inheritance hierarchy
- Comprehensive docstrings
- Proper use of dataclass decorators
- Good type annotations
20-20: LGTM! Type safety improvement achieved.
The removal of # type: ignore while maintaining REQUIRED indicates proper type checking integration.
Let's verify this change is consistent across the codebase:
✅ Verification successful
The verification results show that:
- No remaining instances of "type: ignore" comments with REQUIRED were found
- REQUIRED is properly imported across the codebase, including in the file under review (account_nfts.py)
Type safety improvement verified successfully
The removal of # type: ignore from the account field is consistent with the codebase-wide pattern, where REQUIRED is now properly type-checked without requiring type ignores. The import and usage of REQUIRED is correctly maintained across all relevant files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expect: No occurrences of "type: ignore" next to REQUIRED assignments
# Search for any remaining type ignore comments with REQUIRED
rg "REQUIRED.*#.*type:\s*ignore"
# Verify REQUIRED is properly imported and used
ast-grep --pattern 'from xrpl.models.required import REQUIRED'
Length of output: 6906
xrpl/models/transactions/account_delete.py (1)
26-26: LGTM! Clean type annotation for required field.
The removal of # type: ignore while maintaining the REQUIRED constraint is a good improvement. This change enhances type safety without affecting runtime behavior.
Let's verify the consistency of this pattern across other transaction models:
✅ Verification successful
All REQUIRED fields are consistently typed without ignore comments
The verification confirms:
- No remaining "type: ignore" comments with REQUIRED fields across the models directory
- The
destination: str = REQUIREDpattern is consistently used in multiple transaction models including AccountDelete - All REQUIRED string fields follow the same clean type annotation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# Expected: No occurrences, confirming consistent cleanup
# Search for any remaining "type: ignore" comments near REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg -l "REQUIRED.*#.*type:\s*ignore" "xrpl/models/"
# Verify REQUIRED usage pattern across transaction models
echo "Verifying REQUIRED field pattern in transaction models..."
rg ":\s*str\s*=\s*REQUIRED" "xrpl/models/transactions/"
Length of output: 4081
xrpl/models/amounts/issued_currency_amount.py (1)
28-28: LGTM! The type annotation looks correct.
The removal of # type: ignore while maintaining the Union[str, int, float] type for the value field is appropriate for currency amounts. This change aligns with the PR's objective to improve typing without affecting functionality.
Let's verify that this typing change works consistently across the codebase:
✅ Verification successful
The type annotation change is safe and consistent across the codebase
The verification shows:
- No remaining
type: ignorecomments related toIssuedCurrencyAmount - All usages of
IssuedCurrencyAmountacross the codebase are consistent with the type annotationUnion[str, int, float]for thevaluefield - The class is used extensively in transactions, models, and tests without any typing conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that IssuedCurrencyAmount is used correctly without type errors
# Test: Search for IssuedCurrencyAmount usage and verify no type: ignore comments remain
# Check for any remaining type: ignore comments related to IssuedCurrencyAmount
rg -l "IssuedCurrencyAmount.*type:\s*ignore"
# Check for all usages of IssuedCurrencyAmount to ensure consistent typing
ast-grep --pattern 'IssuedCurrencyAmount'
Length of output: 11230
xrpl/models/transactions/pseudo_transactions/pseudo_transaction.py (1)
31-31: LGTM! Type safety improvement
The removal of # type: ignore while maintaining the REQUIRED type annotation is a positive change that improves type safety without suppressing checks.
Let's verify that similar changes have been applied consistently across the codebase and that type checking passes:
✅ Verification successful
Type safety for REQUIRED fields is now consistent across the codebase
The verification shows that:
- There are no remaining
# type: ignorecomments withREQUIREDin the codebase - All
REQUIREDfield declarations follow a consistent pattern with proper type annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type annotations for REQUIRED fields
# Expected: No remaining "type: ignore" comments with REQUIRED
# Check for any remaining "type: ignore" comments with REQUIRED
rg "REQUIRED.*# type: ignore" || echo "✓ No remaining type: ignore comments with REQUIRED"
# Verify the consistent usage of REQUIRED type annotations
rg ":\s*\w+\s*=\s*REQUIRED" -A 1
Length of output: 21825
xrpl/models/requests/no_ripple_check.py (1)
36-44: LGTM: Type annotations for required fields look correct
The type annotations for the account and role fields are properly defined using REQUIRED without needing type ignores. This aligns with the PR's objective of improving type safety.
Let's verify this pattern is consistent across other request models:
✅ Verification successful
Type annotations for REQUIRED fields are consistent across the codebase
The verification shows that the pattern of using REQUIRED without type ignores is consistently applied across all request models in the codebase. The no_ripple_check.py file follows the same pattern as other request models, where required fields are properly typed with their respective types (e.g., str, NoRippleCheckRole) followed by = REQUIRED. This confirms that the type annotations in the PR are correct and align with the codebase's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other request models follow the same pattern for REQUIRED fields
# without type: ignore comments
# Search for REQUIRED field declarations in request models
rg "= REQUIRED\s+$" "xrpl/models/requests/" -A 1
Length of output: 48
Script:
#!/bin/bash
# Let's try a different approach to find REQUIRED field patterns
# Search for REQUIRED field declarations with more flexible pattern
# Look for REQUIRED usage in request models
rg "REQUIRED" "xrpl/models/requests/" -B 1 -A 1
# Also check the specific file to understand the full context
cat "xrpl/models/requests/no_ripple_check.py"
Length of output: 23423
xrpl/models/requests/account_channels.py (1)
33-33: LGTM! Type annotation improvement
The removal of # type: ignore comment while maintaining account: str = REQUIRED indicates proper typing support, which improves code quality and static type checking capabilities.
Let's verify that this typing pattern works consistently across the codebase:
✅ Verification successful
Type annotations for REQUIRED fields are consistent across the codebase
The verification shows that:
- No remaining
# type: ignorecomments with REQUIRED fields exist in the codebase - The pattern
field: type = REQUIREDis consistently used across all models and requests - The change in
account_channels.pyaligns with the typing pattern used in 100+ other similar declarations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify REQUIRED usage pattern across the codebase
# Expected: No remaining "type: ignore" comments with REQUIRED
# Test 1: Search for any remaining "type: ignore" comments with REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg "REQUIRED.*# type: ignore" || echo "✓ No remaining type: ignore comments with REQUIRED"
# Test 2: Verify consistent REQUIRED usage pattern
echo -e "\nVerifying REQUIRED usage pattern..."
rg ":\s*\w+\s*=\s*REQUIRED" -A 1
Length of output: 22045
xrpl/models/transactions/payment_channel_fund.py (3)
23-30: LGTM: Clean type annotation for required channel field.
The removal of type ignore comment while maintaining the REQUIRED constraint improves type safety without affecting functionality.
Line range hint 31-37: LGTM: Clean type annotation for required amount field.
The removal of type ignore comment while maintaining the REQUIRED constraint improves type safety without affecting functionality.
Line range hint 1-50: LGTM: Well-structured transaction model.
The class maintains good practices:
- Proper inheritance from Transaction
- Immutable dataclass with kwargs enforcement
- Comprehensive documentation
- Clear type hints
Let's verify that similar changes have been applied consistently across other transaction models:
✅ Verification successful
Transaction models maintain consistent structure and best practices
The verification confirms that all transaction models in the codebase follow the same pattern as PaymentChannelFund:
- All inherit from the Transaction base class
- All use the @require_kwargs_on_init decorator
- Most use @DataClass(frozen=True, **KW_ONLY_DATACLASS) decorator (with only OracleSet and OracleDelete using a simpler frozen dataclass)
- All import and use REQUIRED for mandatory fields
- All properly import Transaction and TransactionType
The codebase demonstrates consistent implementation of transaction models with proper inheritance, immutability, and type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments after REQUIRED in transaction models
# Expected: No results, confirming consistent cleanup
rg -g "xrpl/models/transactions/*.py" "REQUIRED.*# type: ignore"
Length of output: 66
Script:
#!/bin/bash
# Let's verify the consistent structure across transaction models by checking:
# 1. Use of REQUIRED
# 2. Proper dataclass decorators
# 3. Transaction inheritance
rg -g "xrpl/models/transactions/*.py" "^class.*\(Transaction\)" -B 2 -A 1
# Also check the import patterns
rg -g "xrpl/models/transactions/*.py" "^from.*REQUIRED"
rg -g "xrpl/models/transactions/*.py" "^from.*Transaction"
rg -g "xrpl/models/transactions/*.py" "^from.*require_kwargs_on_init"
Length of output: 30444
xrpl/models/transactions/amm_delete.py (2)
Line range hint 1-30: Well-structured class with comprehensive documentation!
The class follows Python best practices with proper type hints, dataclass decorators, and detailed documentation explaining the purpose and edge cases of AMM deletion.
Line range hint 31-40: LGTM! Field declarations are properly typed.
The removal of # type: ignore comments while maintaining the Currency type and REQUIRED status aligns with the PR objectives. The fields are well-documented and maintain type safety.
Let's verify that type checking passes without the removed comments:
✅ Verification successful
Type declarations and imports are consistent across AMM transactions
The verification confirms:
- All AMM transaction files consistently import
Currencyfromxrpl.models.currencies - All AMM transaction files consistently import
REQUIREDfromxrpl.models.required - The
Currencytype withREQUIREDpattern is used uniformly across AMM-related transactions (amm_deposit.py,amm_withdraw.py,amm_vote.py,amm_bid.py,amm_delete.py) - No type-ignore comments were found in the AMM transaction files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type checking for the AMMDelete class
# Expected: No type errors related to REQUIRED fields
# Search for any remaining type: ignore comments in AMM transaction files
echo "Checking for remaining type: ignore comments in AMM files..."
rg "type:\s*ignore" "xrpl/models/transactions/amm_*.py"
# Search for usage of Currency type with REQUIRED to ensure consistency
echo "Checking Currency type usage with REQUIRED..."
rg "Currency.*=\s*REQUIRED" "xrpl/models/transactions/"
Length of output: 1289
Script:
#!/bin/bash
# Let's verify the consistency of Currency type imports and REQUIRED usage
# across AMM transaction files
# Check imports of Currency and REQUIRED in AMM files
echo "Checking imports in AMM transaction files..."
fd "amm_.*\.py$" "xrpl/models/transactions" --exec rg -B2 "from.*Currency|from.*REQUIRED" {}
# Check the base class pattern for AMM transactions
echo "Checking base class pattern for AMM transactions..."
ast-grep --pattern 'class $_ (Transaction):'
Length of output: 1393
xrpl/models/requests/get_aggregate_price.py (3)
29-30: LGTM! Type annotation and documentation are clear.
The base_asset field is correctly typed as a required string field with clear documentation explaining its purpose as a currency code.
32-33: LGTM! Type annotation and documentation are clear.
The quote_asset field is correctly typed as a required string field with clear documentation explaining its purpose for price quotation.
29-35: Verify type checking across dependent code.
The removal of # type: ignore comments could potentially affect code that depends on this class. Let's verify that type checking still passes in all dependent files.
✅ Verification successful
Type safety is maintained across the codebase
The removal of type ignore comments is safe because:
- The class is properly typed with frozen dataclass and all required fields
- Type validation is handled through the
REQUIREDsentinel andrequire_kwargs_on_initdecorator - Test coverage validates both valid and invalid type scenarios
- No other files in the codebase have type-ignore comments related to these fields
- The class is correctly exported in
__init__.pyand only imported in test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any typing issues in files that import GetAggregatePrice
# Find files that import GetAggregatePrice
echo "Files importing GetAggregatePrice:"
rg -l "from.*get_aggregate_price.*import.*GetAggregatePrice|from xrpl.models.requests import.*GetAggregatePrice"
# Look for any remaining type: ignore comments related to these fields
echo -e "\nChecking for any remaining type: ignore comments related to these fields:"
rg "type:\s*ignore.*(?:base_asset|quote_asset|oracles)"
Length of output: 502
Script:
#!/bin/bash
# Let's verify the type safety by checking:
# 1. The actual implementation of GetAggregatePrice
# 2. The test file to see how it's being used
# 3. Any validation or type-related code
# Check the implementation
echo "=== GetAggregatePrice Implementation ==="
cat xrpl/models/requests/get_aggregate_price.py
# Check the test file
echo -e "\n=== GetAggregatePrice Test File ==="
cat tests/unit/models/requests/test_get_aggregate_price.py
# Check the __init__.py to see how it's imported
echo -e "\n=== models/requests/__init__.py ==="
cat xrpl/models/requests/__init__.py
Length of output: 8648
xrpl/models/transactions/nftoken_burn.py (2)
Line range hint 26-34: LGTM: Type annotation for required account field is now properly handled
The removal of # type: ignore while maintaining the required field constraint is correct. The field is well-documented and the type hint accurately represents an XRP account address.
Line range hint 26-36: Verify REQUIRED constant type definition
The removal of # type: ignore comments suggests that REQUIRED has been properly typed. Let's verify its implementation:
✅ Verification successful
REQUIRED constant is properly typed and consistently used across the codebase
The verification confirms that:
REQUIREDis properly typed asFinal[Any]inxrpl/models/required.py- It's consistently imported and used across multiple model files
- No type-ignore comments or typing-related warnings exist for REQUIRED
- The removal of type-ignore comments in the NFTokenBurn model aligns with the proper typing implementation
The changes are safe and maintain type consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of REQUIRED constant
# Expected: Find the definition and verify it properly handles type hints
# Look for the REQUIRED constant definition
rg -p "class.*REQUIRED|REQUIRED.*=" xrpl/models/required.py
Length of output: 112
Script:
#!/bin/bash
# Let's verify the imports and usage of REQUIRED across the codebase
# to ensure it's properly typed everywhere
# Check imports of REQUIRED
rg "from.*required.*import.*REQUIRED" -A 2
# Check if there are any remaining type: ignore comments related to REQUIRED
rg "type:\s*ignore.*REQUIRED"
# Look for any typing-related errors or warnings in type hint comments
rg "#\s*type:.*REQUIRED"
Length of output: 21565
xrpl/models/requests/account_objects.py (2)
Line range hint 18-35: Well-structured enum implementation!
The AccountObjectType enum is well-implemented with:
- Proper inheritance from
str, Enum - Clear documentation
- Comprehensive list of XRPL account object types
Line range hint 38-51: Clean implementation with proper typing!
The class implementation correctly:
- Uses appropriate decorators for immutability and kwargs
- Implements the new
typefield with proper optional typing - Maintains clear documentation
Let's verify that the removal of # type: ignore doesn't cause typing issues elsewhere:
✅ Verification successful
Based on the search results, I can see that REQUIRED is consistently used across the codebase without any type: ignore comments. The rg command specifically searching for type: ignore near REQUIRED returned no results, confirming that there are no remaining type ignore comments related to REQUIRED. The ast-grep results show that REQUIRED is used consistently throughout the codebase for required fields in various models.
No typing issues with REQUIRED usage!
The removal of type: ignore comment is safe as:
- No other files use
type: ignorewithREQUIRED - The usage pattern is consistent across all models
- The
REQUIREDconstant is properly defined inxrpl/models/required.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining type: ignore comments related to REQUIRED
# and verify no typing issues were introduced
# Search for any remaining "type: ignore" comments near REQUIRED
rg -B2 -A2 "type:\s*ignore.*REQUIRED|REQUIRED.*type:\s*ignore"
# Search for REQUIRED usage pattern to ensure consistency
ast-grep --pattern 'REQUIRED'
Length of output: 22335
xrpl/models/requests/subscribe.py (3)
39-43: LGTM! Type annotation is now properly handled
The removal of # type: ignore is correct as the typing for REQUIRED has been fixed upstream. The type annotation with Currency is maintained correctly.
46-50: LGTM! Type annotation is now properly handled
The removal of # type: ignore is correct as the typing for REQUIRED has been fixed upstream. The type annotation with Currency is maintained correctly.
Line range hint 53-57: LGTM! Type annotation is now properly handled
The removal of # type: ignore is correct as the typing for REQUIRED has been fixed upstream. The type annotation with str is maintained correctly.
Let's verify that all # type: ignore comments related to REQUIRED have been removed:
✅ Verification successful
LGTM! All REQUIRED fields are properly typed without type ignore comments
The verification confirms that:
- The
taker: str = REQUIREDfield insubscribe.pyis correctly typed without any type ignore comments - All other REQUIRED assignments across the codebase are properly typed with their respective types
- The remaining
type: ignorecomments in the codebase are unrelated to REQUIRED assignments and are used for legitimate cases (external dependencies, type system limitations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments near REQUIRED assignments
# Expect: No matches, as all should have been removed
rg "REQUIRED.*#.*type:\s*ignore" --type py
Length of output: 44
Script:
#!/bin/bash
# Let's try a broader search to find any remaining type ignore comments and REQUIRED assignments
# First, find all REQUIRED assignments
echo "=== Checking REQUIRED assignments ==="
rg ":\s*\w+\s*=\s*REQUIRED" --type py -A 1
echo -e "\n=== Checking for type: ignore comments ==="
# Then check for any type: ignore comments
rg "type:\s*ignore" --type py
# Also check the specific file to ensure our changes are correct
echo -e "\n=== Checking the specific file ==="
cat xrpl/models/requests/subscribe.py
Length of output: 25480
xrpl/models/transactions/xchain_account_create_commit.py (2)
Line range hint 1-73: Overall implementation is clean and well-structured.
The changes successfully improve type safety by removing # type: ignore comments while maintaining the existing functionality. The code is well-documented and follows XRPL conventions.
Line range hint 65-73: Verify validation against Bridge ledger object constraints.
The error handling looks good for basic numeric validation, but consider adding validation for:
signature_rewardmatching the Bridge ledger object amountamountbeing >= MinAccountCreateAmount from Bridge object
Let's check if these validations exist elsewhere in the codebase:
xrpl/models/requests/ripple_path_find.py (3)
Line range hint 14-41: Class structure and documentation look good!
The class is well-structured with:
- Proper inheritance from Request and LookupByLedgerRequest
- Comprehensive docstrings explaining the purpose and limitations
- Appropriate use of dataclass decorators and configuration
Line range hint 42-63: Required fields are well-documented and properly typed
The required fields (source_account, destination_account, destination_amount) are:
- Clearly marked as required with appropriate type annotations
- Well-documented with explicit "This field is required" notes
- Properly hidden from documentation with
:meta hide-value:
Line range hint 42-63: Verify REQUIRED typing implementation
The removal of # type: ignore comments suggests that typing issues have been resolved. Let's verify the implementation of REQUIRED to ensure type safety.
✅ Verification successful
Based on the shell script results, I can now generate the final response. The key findings are:
- REQUIRED is properly defined in
xrpl/models/required.pyasREQUIRED: Final[Any] = object()with proper typing - The codebase extensively uses proper typing imports and type hints
- The only remaining
# type: ignorecomments are for legitimate use cases unrelated to REQUIRED fields - The dataclass fields are properly handled in the base model implementation
Type safety for REQUIRED fields is properly implemented
The removal of # type: ignore comments was justified as the codebase now has proper typing support:
- REQUIRED is properly defined as a Final[Any] type
- The dataclass field system correctly handles the REQUIRED sentinel value
- Type hints are consistently used throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of REQUIRED and its typing
echo "Checking REQUIRED implementation and usage..."
rg -l "REQUIRED" | xargs rg "# type: ignore" -C 2
# If no results, it confirms that type: ignore is not needed anymore
# Check the definition of REQUIRED
echo "Checking REQUIRED definition..."
rg -p "class.*REQUIRED|def.*REQUIRED"
Length of output: 1405
Script:
#!/bin/bash
# Let's search for REQUIRED definition and its usage patterns
echo "Searching for REQUIRED definition..."
rg -p "REQUIRED\s*=|REQUIRED\s*:" -C 3
# Let's also check for any typing-related imports or definitions
echo "Checking typing-related imports and definitions..."
rg "from typing import|import typing" -C 2
# Let's specifically look in base_model.py for field-related code
echo "Checking field-related code in base_model..."
rg "field|Field" xrpl/models/base_model.py -C 2
Length of output: 50623
xrpl/models/transactions/pseudo_transactions/unl_modify.py (1)
27-27: LGTM! Type annotations are now properly handled without suppressions.
The removal of # type: ignore comments while maintaining the REQUIRED fields is correct. The type hints are accurate for their respective purposes:
ledger_sequence: intfor the ledger indexunl_modify_disabling: intfor the binary stateunl_modify_validator: strfor the master public key
Also applies to: 36-36, 45-45
xrpl/models/transactions/payment_channel_create.py (2)
24-24: LGTM! Type ignore comments successfully removed.
The removal of # type: ignore comments from the REQUIRED fields improves type safety without changing functionality. This aligns with the PR objective of fixing typing issues originally discussed in PR #155.
Let's verify that similar changes were applied consistently across the codebase:
Also applies to: 32-32, 41-41, 50-50
✅ Verification successful
This will help us:
- See the pattern of REQUIRED field declarations
- Check if there are any remaining type: ignore comments
- Find similar transaction files that might need consistent treatment
The type ignore comments have been successfully removed from REQUIRED fields
Based on the search results:
- The REQUIRED field pattern is consistently used across the codebase without type ignore comments
- The remaining type ignore comments are for different purposes (mostly third-party imports and specific type casting scenarios)
- The PaymentChannelCreate transaction model's changes align with the codebase-wide pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments after REQUIRED fields
# Expect: No results, indicating all such comments were removed
rg "REQUIRED.*# type: ignore" --type py
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search to verify the REQUIRED field patterns
# First, find all REQUIRED field declarations
rg "^\s*\w+:.*=\s*REQUIRED.*" --type py -A 1
# Also check for any remaining type: ignore comments in the codebase
rg "# type: ignore" --type py
# And specifically look for files that might have similar transaction models
fd ".*transaction.*\.py$" --type f --exec grep -l "REQUIRED"
Length of output: 25809
Line range hint 50-57: Consider validating public key format.
The public key field accepts both secp256k1 and Ed25519 formats. Consider adding validation to ensure the provided hex string is a valid public key.
Let's check if there's existing validation elsewhere:
xrpl/models/transactions/trust_set.py (4)
Line range hint 16-45: Well-documented flag enumeration!
The TrustSetFlag enum is implemented correctly with clear, descriptive docstrings for each flag value. The bit flags are properly defined with non-overlapping values.
Line range hint 47-58: Clean interface implementation!
The TrustSetFlagInterface provides proper type hints for flag usage, matching the flags defined in the TrustSetFlag enum.
Line range hint 74-89: Clean and type-safe field declarations!
The field declarations are properly typed and documented, with limit_amount correctly marked as REQUIRED without type suppression.
74-74: Verify type consistency for IssuedCurrencyAmount.
Let's verify that the IssuedCurrencyAmount type is consistently used across the codebase for similar fields.
✅ Verification successful
Based on the search results, I can now provide a final response:
Type consistency for IssuedCurrencyAmount is verified and correct
The IssuedCurrencyAmount type is consistently used across the codebase for representing issued currency amounts, including:
- Trust lines (
limit_amountin TrustSet) - Payments (
amountandsend_maxin Payment) - AMM operations (
amount2,lp_token_in,lp_token_outin AMM transactions) - Clawback operations (
amountin Clawback)
The type usage in limit_amount: IssuedCurrencyAmount is correct and follows the established pattern throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check consistency of IssuedCurrencyAmount usage
# Expected: All similar fields should use the same type
# Search for other occurrences of IssuedCurrencyAmount
rg "IssuedCurrencyAmount" -A 2 -B 2
# Search for limit_amount fields to verify type consistency
ast-grep --pattern 'limit_amount: $_'
Length of output: 49333
xrpl/models/transactions/escrow_create.py (3)
25-25: LGTM: Type annotation for amount field is correct
The removal of # type: ignore is safe here as the Amount type is properly imported and matches the field's purpose for representing XRP amounts.
33-33: LGTM: Type annotation for destination field is correct
The removal of # type: ignore is safe here as the str type is appropriate for XRP addresses.
25-33: Verify consistent REQUIRED typing across codebase
Let's verify that the REQUIRED constant is properly typed in other transaction models to ensure consistency.
✅ Verification successful
REQUIRED usage is consistent across the codebase
The verification shows that REQUIRED is used consistently across the codebase for required fields in models, with proper type annotations and no remaining type ignores. The pattern matches the usage in escrow_create.py.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check REQUIRED usage in other transaction models
# Expected: All REQUIRED usages should follow the same pattern without type ignore
# Search for any remaining type ignores with REQUIRED
echo "Checking for remaining 'type: ignore' with REQUIRED..."
rg "REQUIRED.*# type: ignore" "xrpl/models"
# Show current REQUIRED usage pattern
echo "Showing current REQUIRED usage pattern..."
rg "= REQUIRED$" "xrpl/models"
Length of output: 14180
xrpl/models/transactions/xchain_claim.py (2)
29-29: LGTM! Type annotations for REQUIRED fields are correct.
The type annotations for all required fields (xchain_bridge, xchain_claim_id, destination, and amount) are properly defined and align with the transaction model's requirements.
Also applies to: 36-36, 44-44, 62-62
36-42: 🛠️ Refactor suggestion
Consider adding type validation for xchain_claim_id.
The xchain_claim_id field accepts both int and str types, but there's no validation to ensure consistent type usage or format. Consider adding validation in _get_errors to:
- Validate numeric range if int
- Validate format if str
def _get_errors(self: Self) -> Dict[str, str]:
errors = super()._get_errors()
+
+ # Validate xchain_claim_id
+ if isinstance(self.xchain_claim_id, int):
+ if self.xchain_claim_id < 0:
+ errors["xchain_claim_id"] = "xchain_claim_id must be non-negative"
+ elif isinstance(self.xchain_claim_id, str):
+ if not self.xchain_claim_id.strip():
+ errors["xchain_claim_id"] = "xchain_claim_id string must not be empty"
bridge = self.xchain_bridgexrpl/models/transactions/amm_create.py (4)
41-45: LGTM: Type annotation for amount is now properly handled
The removal of # type: ignore while maintaining the Amount type hint is correct and aligns with the PR's objective of fixing REQUIRED typing.
47-51: LGTM: Type annotation for amount2 follows the same pattern
The change is consistent with the amount field and correctly implements the typing fix.
Line range hint 53-59: LGTM: Type annotation for trading_fee is properly constrained
The int type hint is appropriate for this field, and the removal of # type: ignore is consistent with the typing improvements. The field remains well-protected by the validation logic in _get_trading_fee_error().
41-53: Verify consistent removal of type ignores across the codebase
Let's ensure this pattern is consistently applied across all transaction models.
✅ Verification successful
Let's fix the regex and try a more targeted search to understand the type annotation patterns across the codebase.
Type annotations for REQUIRED are consistently used across the codebase
The codebase shows a consistent pattern of using REQUIRED without type ignores. The changes in amm_create.py align with the broader codebase where REQUIRED is used as a type annotation for required fields across all models and requests.
Key observations:
REQUIREDis defined inxrpl/models/required.pyas a final object- All transaction models, request models, and base models use
REQUIREDconsistently without type ignores - The pattern matches the implementation in the file under review
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining instances of REQUIRED with type: ignore
rg "REQUIRED.*# type: ignore" "xrpl/models/"
# Check the pattern of REQUIRED usage without type: ignore
rg "REQUIRED(?!.*# type: ignore)" "xrpl/models/"
Length of output: 333
Script:
#!/bin/bash
# Search for REQUIRED usage patterns in model files
rg -l "REQUIRED" "xrpl/models/"
# Then get detailed context for each occurrence
rg -B1 -A1 "REQUIRED" "xrpl/models/"
Length of output: 61564
xrpl/models/requests/submit_only.py (1)
56-56: LGTM! Typing improvement aligns with PR objectives.
The removal of # type: ignore while maintaining tx_blob: str = REQUIRED is correct and consistent with the PR's goal of improving type safety.
Let's verify that the typing works correctly across similar patterns in the codebase:
✅ Verification successful
✓ Type safety improvements verified across the codebase
The verification confirms:
- No remaining
# type: ignorecomments with REQUIRED fields - Consistent typing pattern
field_name: type = REQUIREDacross 120+ occurrences in the codebase - The change in
submit_only.pyfollows the established pattern used throughout the project
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that there are no remaining "type: ignore" comments with REQUIRED
# and that the typing is consistent across the codebase
# Test 1: Check for any remaining "type: ignore" comments with REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg "REQUIRED.*# type: ignore" || echo "✓ No remaining type: ignore comments with REQUIRED"
# Test 2: Verify the pattern of field declarations with REQUIRED
echo -e "\nVerifying REQUIRED field declarations pattern..."
rg ":\s*\w+\s*=\s*REQUIRED\s*$"
Length of output: 12762
xrpl/models/transactions/amm_bid.py (1)
35-38: LGTM! Verify type safety across similar currency fields.
The removal of # type: ignore while maintaining the Currency type with REQUIRED looks good. This change aligns with the PR objectives.
Let's verify the consistency of this change across similar currency fields:
✅ Verification successful
Based on the search results, I can now generate the final response:
The removal of # type: ignore from Currency fields is consistent across the codebase
The search results show that:
- All Currency fields with REQUIRED are consistently declared without
# type: ignorecomments - The pattern
asset: Currency = REQUIREDis used consistently across AMM-related transactions - No remaining
# type: ignorecomments exist on Currency field declarations - The existing
# type: ignorecomments in the codebase are used for different purposes (class initialization, type casting) and not related to Currency fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments on Currency fields
# Expected: No remaining type ignore comments for Currency fields
# Search for Currency type fields with type ignore comments
rg "Currency.*=.*REQUIRED.*#.*type:.*ignore"
# Search for similar Currency field declarations to ensure consistency
ast-grep --pattern 'Currency = REQUIRED'
Length of output: 89
Script:
#!/bin/bash
# Let's try a broader search to understand the Currency field patterns
# First, let's find all Currency type annotations
rg ":\s*Currency\s*=" -A 1
# Then find all REQUIRED fields to see the pattern
rg "=\s*REQUIRED" -A 1
# Also search for any remaining type: ignore comments in the models directory
rg "# type: ignore" xrpl/models/
# Look for Currency imports to understand the context better
rg "from.*Currency.*import"
Length of output: 28383
xrpl/models/transactions/xchain_add_claim_attestation.py (5)
27-32: LGTM: Type annotation for xchain_bridge is correct
The type annotation properly uses the custom XChainBridge type and aligns with the docstring description.
Line range hint 49-63: LGTM: Appropriate type annotations for chain and amount fields
The type annotations are correct:
other_chain_sourceusesstrfor account addressesamountproperly uses the customAmounttype
66-80: LGTM: Account address fields properly typed
Both attestation_reward_account and attestation_signer_account correctly use str type for XRPL account addresses.
Line range hint 1-24: LGTM: Well-structured transaction class
The class is well-designed with:
- Proper inheritance from
Transaction - Appropriate use of dataclass features
- Comprehensive documentation
Line range hint 90-96: Consider narrowing the type of xchain_claim_id
The current Union[str, int] type is quite broad and might lead to confusion or errors. Consider:
- Documenting when each type is expected
- If possible, standardizing on a single type
Let's check how this field is used across the codebase:
xrpl/models/requests/channel_authorize.py (2)
49-49: LGTM: Type annotations for required fields look correct.
The removal of # type: ignore comments from channel_id and amount fields while maintaining their REQUIRED status is appropriate. This improves type safety without changing the runtime behavior.
Also applies to: 56-56
Line range hint 71-89: LGTM: Robust validation of signing methods.
The _get_errors implementation correctly ensures exactly one signing method is provided, which is crucial for security.
xrpl/models/transactions/amm_withdraw.py (3)
58-61: LGTM: Type ignore removal for asset field is correct.
The removal of # type: ignore while maintaining Currency = REQUIRED is the right approach. The field remains properly typed and documented as required.
63-66: LGTM: Type ignore removal for asset2 field is correct.
The removal of # type: ignore while maintaining Currency = REQUIRED is the right approach. The field remains properly typed and documented as required.
58-66: Verify the typing changes across the codebase.
Let's ensure this pattern is consistently applied across other transaction models.
✅ Verification successful
Let me gather more information to verify if there are any inconsistencies in the typing patterns.
Type annotations for Currency and IssuedCurrencyAmount are consistently applied across the codebase
The verification shows that:
- All AMM-related transactions (withdraw, deposit, delete, bid, vote) consistently use
Currency = REQUIREDfor asset fields - Book offers and subscription-related requests properly use
Currency = REQUIREDfor taker fields - Trust-related and clawback transactions correctly use
IssuedCurrencyAmount = REQUIRED - Optional Currency/IssuedCurrencyAmount fields consistently use the proper Optional type annotation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "type: ignore" comments with REQUIRED
# and verify the consistency of Currency typing across transaction models
# Look for any remaining "type: ignore" with REQUIRED
echo "Checking for remaining type: ignore with REQUIRED..."
rg "REQUIRED.*# type: ignore" "xrpl/models/"
# Look for Currency = REQUIRED pattern to verify consistency
echo "Checking Currency = REQUIRED pattern..."
rg "Currency.*=.*REQUIRED" "xrpl/models/"
Length of output: 1588
Script:
#!/bin/bash
# Check for any other Currency-related field patterns that might need consistency
echo "Checking other Currency field patterns..."
rg "Currency.*=.*(?!REQUIRED)" "xrpl/models/"
# Check for any IssuedCurrencyAmount patterns to ensure consistency
echo "Checking IssuedCurrencyAmount patterns..."
rg "IssuedCurrencyAmount.*=" "xrpl/models/"
# Look for any potential type annotations with Currency
echo "Checking Currency type annotations..."
ast-grep --pattern 'Currency'
Length of output: 6451
xrpl/models/transactions/xchain_create_bridge.py (2)
29-34: LGTM! Type annotation is correct
The removal of # type: ignore is safe here as the type annotation for xchain_bridge is properly defined as XChainBridge. The field remains required and well-documented.
Line range hint 29-42: Verify consistent REQUIRED typing across transaction types
Let's verify that similar transaction types follow the same pattern of removing # type: ignore comments.
✅ Verification successful
REQUIRED typing is consistently applied across transaction types
The verification shows that:
- No remaining
# type: ignorecomments with REQUIRED exist in transaction files - All transaction types consistently use the same pattern for REQUIRED fields
- The changes in
xchain_create_bridge.pyalign with the typing pattern used across the entire transactions directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining "type: ignore" comments with REQUIRED
echo "Checking for remaining type: ignore comments with REQUIRED..."
rg "REQUIRED.*# type: ignore" "xrpl/models/transactions/"
# Search for similar field declarations in other transaction types
echo "Checking similar field declarations in other transaction types..."
rg ":\s*\w+\s*=\s*REQUIRED" "xrpl/models/transactions/"
Length of output: 8235
xrpl/models/requests/sign_for.py (3)
41-45: LGTM! Type annotation improvement for required field.
The removal of # type: ignore while maintaining the str type annotation is correct for the account field. This change aligns with the PR objectives to improve type checking without suppressing warnings.
Line range hint 48-52: LGTM! Type annotation improvement for required Transaction field.
The removal of # type: ignore while maintaining the Transaction type annotation is correct. The field is properly used in the class methods, particularly in to_dict() where it's converted to XRPL format.
Line range hint 41-52: Verify consistent removal of type ignores across the codebase.
Let's verify that similar # type: ignore comments with REQUIRED have been addressed consistently across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
No remaining type ignores with REQUIRED found - changes are consistent
The verification shows that there are no remaining instances of type: ignore comments with REQUIRED in the codebase. All usages of REQUIRED follow a consistent pattern without type ignores, confirming that the cleanup has been thorough and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of "type: ignore" with REQUIRED
# to ensure consistent cleanup across the codebase
echo "Checking for remaining 'type: ignore' comments with REQUIRED..."
rg -l "REQUIRED.*# type: ignore" || echo "No remaining instances found!"
echo -e "\nChecking for all REQUIRED field declarations..."
rg "=\s*REQUIRED" -A 1
Length of output: 26209
xrpl/models/transactions/amm_deposit.py (1)
60-65: LGTM! Type annotations are now correctly handled.
The removal of # type: ignore comments is appropriate here as the type system can now properly handle Currency = REQUIRED annotations. The fields are well-documented and their required nature is correctly enforced through both typing and documentation.
Let's verify that similar typing patterns work correctly across the codebase:
✅ Verification successful
Type annotations for Currency fields are consistently implemented
The verification shows that:
- All Currency fields across the codebase consistently use the
Currency = REQUIREDpattern without type ignore comments - The pattern is used in multiple AMM-related files (deposit, withdraw, vote, bid, delete) and request models (subscribe, book_offers, unsubscribe)
- No remaining
type: ignorecomments exist for Currency field declarations
The removal of type ignore comments from the asset and asset2 fields in amm_deposit.py aligns with the codebase-wide typing patterns and is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent typing patterns for Currency fields
# Test: Look for other Currency fields using REQUIRED to ensure consistent typing
ast-grep --pattern 'Currency = REQUIRED'
# Test: Ensure no remaining type: ignore comments for Currency fields
rg "Currency.*=.*REQUIRED.*#.*type:.*ignore"
Length of output: 89
Script:
#!/bin/bash
# Let's try a different approach to verify the typing patterns
# Check for Currency field declarations with their types
rg ":\s*Currency\s*=" -A 1
# Look for REQUIRED field patterns in transaction models
rg "=\s*REQUIRED" --type py
# Check for any remaining type: ignore comments in the codebase
rg "type:\s*ignore" --type py
# Find all files that might use Currency type
fd -e py | xargs rg "Currency" -l
Length of output: 20810
xrpl/models/transactions/pseudo_transactions/enable_amendment.py (3)
Line range hint 69-76: LGTM: Type annotation is correct for required field.
The removal of # type: ignore with proper type annotation for the required amendment field aligns with the PR objectives of improving type safety.
Line range hint 78-85: LGTM: Type annotation is correct for required field.
The removal of # type: ignore with proper type annotation for the required ledger_sequence field aligns with the PR objectives of improving type safety.
Line range hint 69-85: Verify type checking across related files.
Since this is part of a broader change to remove # type: ignore comments, let's verify that type checking passes across related files.
✅ Verification successful
Type safety is maintained after removing type-ignore comments
The verification shows that:
REQUIREDis properly defined as a type-safe sentinel object usingtyping.Final[Any]- The file correctly imports and uses typing annotations (
List,Optional,Union) - The type annotations on the
amendmentandledger_sequencefields are properly defined without any type-ignore comments - The import and usage patterns are consistent across the codebase
The removal of type-ignore comments in this context is safe as the type system correctly handles the REQUIRED sentinel pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type checking in related files
# Expected: No type errors after removing '# type: ignore'
# Find all Python files with REQUIRED usage
echo "Files using REQUIRED:"
rg "REQUIRED" --type py -l
# Look for any remaining '# type: ignore' comments near REQUIRED
echo -e "\nChecking for remaining '# type: ignore' comments near REQUIRED:"
rg -C2 "REQUIRED.*# type: ignore" --type py
Length of output: 3632
Script:
#!/bin/bash
# Let's verify the type annotations and imports around REQUIRED usage
# Check the definition and imports of REQUIRED
echo "=== REQUIRED definition and imports ==="
rg -C3 "^REQUIRED\s*=" --type py
# Check how REQUIRED is imported and used
echo -e "\n=== REQUIRED imports ==="
rg "from.*import.*REQUIRED|import.*REQUIRED" --type py
# Look for any type hints with REQUIRED
echo -e "\n=== Type hints using REQUIRED ==="
ast-grep --pattern 'annotation: $_ = REQUIRED'
Length of output: 7057
Script:
#!/bin/bash
# Let's check the REQUIRED definition and its usage pattern
# Check the REQUIRED definition file
echo "=== REQUIRED definition ==="
cat xrpl/models/required.py
# Check a sample of type annotations in the file under review
echo -e "\n=== Type annotations in enable_amendment.py ==="
cat xrpl/models/transactions/pseudo_transactions/enable_amendment.py
# Look for any type-related imports in the file
echo -e "\n=== Type-related imports in enable_amendment.py ==="
rg "^from.*typing|^import.*typing" xrpl/models/transactions/pseudo_transactions/enable_amendment.py
Length of output: 4485
xrpl/models/transactions/offer_create.py (3)
83-89: LGTM! Type annotation is now properly handled
The removal of # type: ignore while maintaining Amount = REQUIRED is correct and aligns with the PR's objective to improve typing without changing functionality.
Line range hint 91-97: LGTM! Consistent type annotation improvement
The removal of # type: ignore is consistent with the previous field and maintains proper typing with Amount = REQUIRED.
83-91: Verify consistent removal of type ignores across the codebase
Let's ensure this pattern has been consistently applied across all similar usages.
✅ Verification successful
Type annotations with REQUIRED are consistently applied across the codebase
The verification shows that all instances of REQUIRED field declarations follow a consistent pattern with proper type annotations (e.g., field: Type = REQUIRED). No instances of type: ignore comments were found with REQUIRED, indicating the typing improvements have been thoroughly applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining instances of REQUIRED with type: ignore
echo "Checking for remaining 'type: ignore' with REQUIRED..."
rg "REQUIRED.*# type: ignore"
# Check for all REQUIRED field declarations to ensure consistency
echo "Checking all REQUIRED field declarations..."
rg ":\s*\w+\s*=\s*REQUIRED"
Length of output: 12623
xrpl/models/transactions/payment_channel_claim.py (3)
Line range hint 13-41: Well-structured enum implementation with comprehensive documentation!
The PaymentChannelClaimFlag enum is well-implemented with clear documentation and proper flag values that don't overlap.
Line range hint 44-56: Clean TypedDict implementation with consistent documentation!
The PaymentChannelClaimFlagInterface properly defines the flag interface with appropriate typing.
69-69: Clean type annotation for required field!
The channel field is properly typed as a required string field with clear documentation.
Let's verify that this change doesn't break any existing code:
✅ Verification successful
Based on the search results, I can now generate the final response:
Required channel field is correctly used across the codebase
The verification shows that all existing PaymentChannelClaim instantiations properly provide the required channel field:
- In tests:
test_better_transaction_flags.pyandtest_payment_channel_claim.py - No type-ignore comments related to the channel field were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PaymentChannelClaim instantiations to ensure they provide the required channel field
rg -l "PaymentChannelClaim\(" | xargs rg -A 5 "PaymentChannelClaim\("
# Search for any existing type ignore comments related to this field that might need cleanup
rg -l "channel.*type:\s*ignore" | xargs rg "channel.*type:\s*ignore"
Length of output: 3059
xrpl/models/transactions/nftoken_create_offer.py (2)
44-44: LGTM! Type annotation for nftoken_id is correct.
The string type for NFToken ID is appropriate, and the REQUIRED designation properly enforces this as a mandatory field.
52-52: Verify the Amount type implementation for zero-value handling.
The type annotation looks correct, but let's verify the Amount type implementation handles zero values appropriately, given the special case for XRP sell offers.
✅ Verification successful
Let me gather more information about the Amount type implementation since it wasn't found in the first search.
Let me try one more search strategy to find the Amount type and validation logic.
Let me check the Amount type definition from the models/amounts module and its zero value handling.
Let me try one final search to locate the Amount type definition.
Based on the gathered information, I can now provide the final response:
Amount type correctly handles zero values for NFToken sell offers
The Amount type implementation and validation in NFTokenCreateOffer properly handles zero values:
- Zero amounts are explicitly allowed for sell offers (TF_SELL_NFTOKEN flag)
- Zero amounts are properly validated and rejected for buy offers
- Test cases verify both scenarios:
test_nftoken_buy_offer_with_zero_amountconfirms rejectiontest_nftoken_sell_offer_with_zero_amountconfirms acceptance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Amount type implementation and its handling of zero values
# Look for Amount type definition and zero value handling
ast-grep --pattern 'class Amount:
$$$
'
# Check for XRP amount-specific handling
rg -A 5 'XRP.*amount'
Length of output: 10416
Script:
#!/bin/bash
# Search for Amount type definition in models directory
fd "amount.py" --type f --exec ast-grep --pattern 'class Amount:
$$$
'
# Search for NFTokenCreateOffer amount validation
rg -A 10 "_get_amount_error.*NFTokenCreateOffer"
Length of output: 123
Script:
#!/bin/bash
# Search for Amount type definition and imports
rg -l "class Amount"
# Look for NFTokenCreateOffer amount validation
rg -A 5 "def _get_amount_error" xrpl/models/transactions/nftoken_create_offer.py
# Check Amount imports in the NFTokenCreateOffer file
rg "^from.*Amount" xrpl/models/transactions/nftoken_create_offer.py
Length of output: 531
Script:
#!/bin/bash
# Check Amount definition and zero handling in amounts module
cat xrpl/models/amounts.py
# Look for any tests related to zero amount handling in NFTokenCreateOffer
rg -A 5 "amount.*0" tests/unit/models/transactions/test_nftoken_create_offer.py
Length of output: 524
Script:
#!/bin/bash
# Search for Amount in the models directory
fd "amount" --type f --full-path xrpl/models | xargs cat
# Check NFTokenCreateOffer test cases for amount validation
cat tests/unit/models/transactions/test_nftoken_create_offer.py
Length of output: 7005
xrpl/models/requests/path_find.py (4)
84-89: LGTM! Type annotation for required field is now properly handled
The removal of # type: ignore while maintaining the REQUIRED designation and proper type hint improves code clarity without changing functionality.
91-96: LGTM! Type annotation for source account is now properly handled
The string type hint is appropriate for an account address, and the removal of # type: ignore improves type safety.
98-103: LGTM! Type annotation for destination account is now properly handled
The string type hint is consistent with source_account, maintaining a uniform approach to account address typing.
Line range hint 105-110: LGTM! Type annotation for destination amount is now properly handled
The Amount type hint is appropriate, and the removal of # type: ignore improves type safety.
Consider verifying that there's proper validation when both destination_amount and optional send_max are provided. Let's check for existing validation:
xrpl/models/requests/sign_and_submit.py (2)
62-62: LGTM! Type annotation improvement successfully implemented.
The removal of "# type: ignore" while maintaining proper typing for the required transaction field aligns perfectly with the PR objectives. The type annotation is now correctly enforced without suppressing type checker warnings.
62-62: Verify consistent REQUIRED usage across the codebase.
Let's ensure this typing pattern is consistently applied across all REQUIRED fields in the codebase.
✅ Verification successful
Consistent REQUIRED usage verified across codebase
The verification shows that the REQUIRED field marker is consistently used across the entire codebase for required fields in models, following the same pattern as the reviewed line:
- Type annotation followed by
= REQUIRED - Used for mandatory fields in request/response models, transactions, and currencies
- No remaining type-ignore comments with REQUIRED found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining type: ignore comments with REQUIRED
# and verify consistent typing pattern
echo "Checking for remaining '# type: ignore' comments with REQUIRED..."
rg -l "# type: ignore.*REQUIRED" || echo "No remaining type: ignore comments found with REQUIRED"
echo -e "\nVerifying consistent REQUIRED field typing pattern..."
rg ".*=\s*REQUIRED" -A 1 -B 1
Length of output: 35826
xrpl/models/transactions/payment.py (2)
77-83: LGTM: Clean type annotation for required amount field.
The removal of # type: ignore while maintaining the Amount = REQUIRED type annotation is correct and improves type safety without changing behavior.
Line range hint 85-90: LGTM: Clean type annotation for required destination field.
The removal of # type: ignore while maintaining the str = REQUIRED type annotation is correct and improves type safety without changing behavior.
tools/generate_tx_models.py (1)
120-120: LGTM! Improved type safety for required parameters.
The removal of # type: ignore while maintaining REQUIRED as a valid type assignment indicates proper typing support. This change aligns with the PR objectives and improves type safety across generated models.
Let's verify the impact on generated transaction models:
xrpl/models/transactions/oracle_set.py (3)
43-46: LGTM! Type annotations are correct for account field.
The removal of # type: ignore is safe here as str is the correct type for XRPL accounts.
79-84: LGTM! Types and validation logic are well-aligned.
The removal of # type: ignore is safe here. The types are correct and backed by proper validation in _get_errors:
last_update_timeis validated against EPOCH_OFFSETprice_data_seriesis validated for length and content
160-165: Consider adding validation for base_asset and quote_asset.
While the removal of # type: ignore is safe and the types are correct, consider adding validation in _get_errors to ensure these fields aren't empty strings and follow any XRPL-specific asset identifier format requirements.
Let's check if other transaction types validate their asset identifiers:
xrpl/models/requests/ledger_entry.py (4)
54-54: LGTM: Type annotations are now properly handled
The removal of # type: ignore comments is safe as the string type annotations are correctly specified and the REQUIRED marker is properly imported.
Also applies to: 61-61
77-77: LGTM: Type annotations are properly handled
The removal of # type: ignore comments is safe as the string type annotations are correctly specified.
Also applies to: 84-84
101-101: LGTM: Type annotations consistently improved across all classes
The removal of # type: ignore comments across all remaining classes is safe and improves code quality because:
- All fields maintain their proper type annotations
- The
REQUIREDmarker is consistently applied - Classes are properly decorated with
@require_kwargs_on_init - No functional changes are introduced
This change successfully achieves the PR's objective of removing unnecessary type ignore comments while maintaining type safety.
Also applies to: 108-108, 124-124, 131-131, 147-147, 154-154, 167-167, 174-174, 187-187, 194-194, 207-207, 224-224
Line range hint 54-224: Verify type checking in dependent code
While the changes look good, let's verify that the removal of # type: ignore comments doesn't cause type checking issues in dependent code.
✅ Verification successful
Type safety is properly maintained after removal of type ignores
The verification shows that:
REQUIREDis properly typed asFinal[Any]inrequired.py- The base infrastructure has proper typing support with comprehensive imports from
typingandtyping_extensions - No remaining
type: ignorecomments related toREQUIREDexist in the codebase - The importing files properly use the typed
REQUIREDconstant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the changes don't introduce type checking issues
# Test 1: Check if REQUIRED is properly typed in its definition
echo "Checking REQUIRED definition..."
rg -l "class.*REQUIRED|type.*REQUIRED" src/
# Test 2: Look for any remaining type: ignore comments related to REQUIRED
echo "Checking for remaining type: ignore comments..."
rg "REQUIRED.*# type: ignore" src/
# Test 3: Look for imports of these models to verify impact
echo "Checking usage of these models..."
rg -l "from.*models\.requests\.ledger_entry import" src/
Length of output: 531
Script:
#!/bin/bash
# Description: Verify that the changes don't introduce type checking issues
# Test 1: Check if REQUIRED is properly typed in its definition
echo "Checking REQUIRED definition..."
rg -l "class.*REQUIRED|type.*REQUIRED"
# Test 2: Look for any remaining type: ignore comments related to REQUIRED
echo "Checking for remaining type: ignore comments..."
rg "REQUIRED.*# type: ignore"
# Test 3: Look for imports of these models to verify impact
echo "Checking usage of these models..."
rg -l "from.*models\.requests\.ledger_entry import"
# Test 4: Find the definition of REQUIRED
echo "Finding REQUIRED definition..."
rg "REQUIRED\s*=" -A 2
# Test 5: Look for type hints with REQUIRED
echo "Checking type hints with REQUIRED..."
ast-grep --pattern 'annotation: $_ = REQUIRED'
Length of output: 960
Script:
#!/bin/bash
# Let's try a different approach to find REQUIRED definition and its usage
# Find the REQUIRED constant definition
echo "Looking for REQUIRED constant definition..."
rg -A 3 "^REQUIRED\s*=|^class.*REQUIRED"
# Check imports and usage in test files
echo "Checking test files for usage patterns..."
rg -A 3 "REQUIRED" tests/unit/models/requests/test_ledger_entry.py
# Check the importing files for any type-related issues
echo "Checking importing files..."
rg -A 3 "REQUIRED" xrpl/models/requests/ledger_data.py
rg -A 3 "REQUIRED" xrpl/models/requests/get_aggregate_price.py
# Look for any type annotations or typing imports
echo "Checking typing-related code..."
rg "from typing import|import typing" xrpl/models/requests/ledger_entry.py
Length of output: 1077
Script:
#!/bin/bash
# Let's check the REQUIRED definition in the required module and its typing
echo "Checking REQUIRED definition in required module..."
rg -p "class.*REQUIRED|REQUIRED\s*=" xrpl/models/required.py
echo "Checking type annotations in required module..."
rg "from typing|import typing" xrpl/models/required.py
# Check if there are any remaining type-related issues in the codebase
echo "Checking for any type-related issues with REQUIRED..."
rg "type:\s*ignore.*REQUIRED" xrpl/
# Check the base model implementation
echo "Checking BaseModel implementation..."
rg -p "class BaseModel" xrpl/models/
Length of output: 722
Script:
#!/bin/bash
# Final verification - let's check the actual content of required.py and base_model.py
echo "Checking the full content of required.py..."
cat xrpl/models/required.py
echo "Checking BaseModel's relevant methods..."
rg -A 10 "class BaseModel" xrpl/models/base_model.py
# Check if there are any validation or type checking related imports
echo "Checking validation imports..."
rg "from.*typing|import.*typing" xrpl/models/base_model.py
Length of output: 1276
| was_locking_chain_send: Union[Literal[0], Literal[1]] = REQUIRED | ||
| """ | ||
| A boolean representing the chain where the event occurred. This field is | ||
| required. | ||
|
|
||
| :meta hide-value: | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using bool type for boolean field
The current type Union[Literal[0], Literal[1]] is overly complex for what is essentially a boolean value.
Consider simplifying to:
- was_locking_chain_send: Union[Literal[0], Literal[1]] = REQUIRED
+ was_locking_chain_send: bool = REQUIREDThis would make the code more intuitive while maintaining the same functionality.
📝 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.
| was_locking_chain_send: Union[Literal[0], Literal[1]] = REQUIRED | |
| """ | |
| A boolean representing the chain where the event occurred. This field is | |
| required. | |
| :meta hide-value: | |
| """ | |
| was_locking_chain_send: bool = REQUIRED | |
| """ | |
| A boolean representing the chain where the event occurred. This field is | |
| required. | |
| :meta hide-value: | |
| """ |
There was a problem hiding this comment.
Out of scope of this PR
There was a problem hiding this comment.
@ mvadari, understood. I've noted that changing the was_locking_chain_send field in xrpl/models/transactions/xchain_add_claim_attestation.py to bool is out of scope for this PR.
(˘︹˘)
✏️ Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.047Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
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.
| from typing_extensions import Final | ||
|
|
||
| REQUIRED: Final[object] = object() | ||
| REQUIRED: Final[Any] = object() # noqa: ANN401 |
There was a problem hiding this comment.
As per my understanding, we have shifted the # type: ignore mypy directive into this line with a # noqa: ANN401 directive. mypy will be unhappy with our code either way.
It would be more elegant to adopt a template with required+optional fields for every transaction and ledger-entry. The format used by rippled is instructive in this regard: https://github.com/XRPLF/rippled/blob/0ec17b6026298dc150099b66a2cecad0fe561d1b/include/xrpl/protocol/STValidation.h#L222
If you know of any other ideas to enforce a schema in Python, please share it.
However, that requires development effort. I'm happy with this change.
There was a problem hiding this comment.
As per my understanding, we have shifted the
# type: ignoremypy directive into this line with a# noqa: ANN401directive.mypywill be unhappy with our code either way.
noqa is ignoring a linter error, not mypy. mypy is perfectly happy with this situation.
It would be more elegant to adopt a template with required+optional fields for every transaction and ledger-entry. The format used by rippled is instructive in this regard: https://github.com/XRPLF/rippled/blob/0ec17b6026298dc150099b66a2cecad0fe561d1b/include/xrpl/protocol/STValidation.h#L222
If you know of any other ideas to enforce a schema in Python, please share it.
I don't believe that's possible without radically changing our model setup.
The reason we have REQUIRED in the first place is because Python inheritance won't let us have required fields after optional fields. Since there are optional fields in the base Transaction class, it isn't happy if we then try to add required fields in child transaction type classes.
Further, if it ain't broke, don't fix it - this system works just fine, so I wouldn't think it's much of a priority (if at all) to mess with it, unless there are other benefits to be had from that.
There was a problem hiding this comment.
I agree with the spirit of your argument
High Level Overview of Change
This PR adjusts the typing of
REQUIREDso that you no longer need to except it from typing checks.Context of Change
After reviewing some of the history of
REQUIREDin the codebase, I'm unsure if this change is desired. Here's the reference material, for others to make the decision:Type of Change
Did you update CHANGELOG.md?
Test Plan
CI passes. Typing checks are satisfied.