Credentials #759
Credentials #759
Conversation
… at commit ea8e77ffec065cf1a8d1cd4517f9cebdab27cc17 Explicity specify featureCredentials inside the conf file. This enables the features inside the genesis ledger
Refactor common elements within Credential-related transactions
…ommit Deposit_preauth: array length checks on the authcreds and unauthcreds fields
| @@ -1,2775 +1,2933 @@ | |||
| { | |||
There was a problem hiding this comment.
Please keep the formatting the same in this JSON.
| sample_credentials = [ | ||
| Credential(issuer="SampleIssuer", credential_type="SampleCredType") | ||
| ] | ||
| for val in range(0, 16): |
There was a problem hiding this comment.
I get what you're trying to accomplish here, but this test is really hard to read and IMO a bit unnecessary.
There was a problem hiding this comment.
Hmm, I agree. Shall I test two-three cases where the inputs will throw an error? Will that suffice?
There was a problem hiding this comment.
Yes. The tests don't need to be fully exhaustive, as long as you're getting the code coverage.
There was a problem hiding this comment.
okay, simplified the tests in 379620a
| ) | ||
|
|
||
| if ( | ||
| sum( |
There was a problem hiding this comment.
| sum( | |
| len( |
There was a problem hiding this comment.
Note: I cannot accept this code-suggestion because it doesn't work in the given context.
len([True, False, False, False]) == 4, it does not capture the fact that only one of the elements is True.
However, sum([True, False, False, False]) == 1
There was a problem hiding this comment.
It's pretty un-Pythonic (and unintuitive for Python devs) to sum booleans. You can add an if param is not False if you want.
There was a problem hiding this comment.
if param is not False does not work. It will need to be if refined_param is True for refined_param in [if param is not None for param in inputs_params] -- I can't get around this nested loop comprehension.
I came with a filter construct 0719107 -- this is easier to understand
xrpl/models/utils.py
Outdated
| # Regular-Expression pertaining to Credentials Ledger Object transactions | ||
| # Note: This regex is not identical to the one used with DIDSet transaction. This regex | ||
| # mandates a minimum of length-1 strings. | ||
| HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]+") |
There was a problem hiding this comment.
Why not use the same hex regex for both DID strings and credential strings and just add an additional check to ensure the string isn't empty for credentials? Then it's more future-proof too (and much less confusing).
There was a problem hiding this comment.
The restrictions on hexa-decimal format and non-zero are applicable to both uri and credential_type fields. If we split them into two separate checks (as you propose), an engineer might forget to include the second length check explicity.
I feel using separate regex-es is still a better solution. I can rename this regex to something more appropriate, CREDENTIAL_REGEX ?
There was a problem hiding this comment.
I think it's less confusing to require the check than to only allow this regex to be usable in some hex situations.
There was a problem hiding this comment.
what is the rationale for the difference in the behavior between DID and Credentials strings?
There was a problem hiding this comment.
In DID, an empty string in a transaction deletes the field. That's not relevant for credentials.
There was a problem hiding this comment.
I've incorporated your suggestion in e63b49d
.ci-config/rippled.cfg
Outdated
| @@ -182,3 +182,5 @@ PriceOracle | |||
| fixEmptyDID | |||
| fixXChainRewardRounding | |||
| fixPreviousTxnID | |||
| # Explicitly enable featureCredentials | |||
There was a problem hiding this comment.
This comment is unnecessary
… successfully deleted Updates to Payment transaction model Update AccountDelete transaction model with Credential ID array Update EscrowFinish txn model with CredentialIDs Updates to the PaymentChannelClaim txn model -- Include new unit test file
Co-authored-by: Mayukha Vadari <[email protected]>
|
@mvadari thanks for the early feedback |
…ion model; Revert this file to an older version
| @@ -57,7 +57,8 @@ | |||
| "Child": -2, | |||
| "Nickname": 110, | |||
| "Contract": 99, | |||
| "GeneratorMap": 103 | |||
| "GeneratorMap": 103, | |||
| "Credential": 129 | |||
There was a problem hiding this comment.
nit: should go after Oracle
There was a problem hiding this comment.
ok, I've updated in f476a51
It doesn't look like there is any rationale for the order of LedgerEntryTypes -- alphabetical or numeric.
|
Warning Rate limit exceeded@ckeshava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces comprehensive support for Verifiable Credentials (XLS-70d) in the XRPL Python library. The changes span multiple files across the project, adding new transaction types ( Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (27)
tests/unit/models/transactions/test_account_delete.py (2)
1-8: LGTM! Clean imports and well-defined constants.The imports are appropriate, and the constants follow proper naming conventions. The account addresses appear to be valid XRPL addresses.
Remove the extra empty line at line 9 to maintain consistent spacing.
10-10: Add class docstring to improve test documentation.Add a docstring describing the purpose of this test class and what aspects of AccountDelete it verifies.
class TestAccountDelete(TestCase): + """Unit tests for the AccountDelete transaction model. + + Tests credential_ids constraints and valid transaction creation scenarios. + """xrpl/models/requests/account_objects.py (1)
Line range hint
1-7: Consider enhancing documentation for credential objects.Since credentials are a new feature, it would be helpful to update the docstring to mention credential objects as one of the possible object types that can be returned. This would help users understand the full capabilities of the API.
Consider adding a note like:
""" This request returns the raw ledger format for all objects owned by an account. For a higher-level view of an account's trust lines and balances, see AccountLinesRequest instead. + +The objects can include various types such as AMM, checks, credentials, escrows, +and more. See the AccountObjectType enum for a complete list of supported types. `See account_objects <https://xrpl.org/account_objects.html>`_ """tests/unit/models/transactions/test_credential_accept.py (2)
1-10: LGTM! Consider adding docstring comments.The imports and test constants are well-structured. Consider adding docstring comments to explain the purpose of the test constants and their format requirements.
_ACCOUNT_ISSUER = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ" _ACCOUNT_SUBJECT = "rNdY9XDnQ4Dr1EgefwU3CBRuAjt3sAutGg" _VALID_CREDENTIAL_TYPE = str_to_hex("Passport") + +# Constants used for testing CredentialAccept transaction +# _ACCOUNT_ISSUER: The account that issues the credential +# _ACCOUNT_SUBJECT: The account that receives the credential +# _VALID_CREDENTIAL_TYPE: A valid credential type in hex format
12-12: Add class-level documentation.Add a docstring to describe the purpose and scope of the test class.
class TestCredentialAccept(TestCase): + """ + Test suite for the CredentialAccept transaction model. + + This class verifies: + - Valid credential acceptance with correct parameters + - Validation of credential_type field constraints + - Error handling for invalid inputs + """tests/unit/models/transactions/test_escrow_finish.py (2)
40-53: Consider using constants for magic numbers and improving string concatenation.The test case effectively validates the maximum length constraint, but could be improved for maintainability.
+MAX_CREDENTIAL_IDS = 8 # Define at module level def test_creds_list_too_long(self): with self.assertRaises(XRPLModelException) as err: EscrowFinish( account=_ACCOUNT, owner=_ACCOUNT, offer_sequence=1, - credential_ids=["credential_index_" + str(i) for i in range(9)], + credential_ids=["credential_index_" + str(i) for i in range(MAX_CREDENTIAL_IDS + 1)], ) self.assertEqual( err.exception.args[0], - "{'credential_ids': 'CredentialIDs list cannot have more than 8 " - + "elements.'}", + "{'credential_ids': 'CredentialIDs list cannot have more than 8 elements.'}" )
39-77: Document the relationship between EscrowFinish and credentials.While the tests are well-structured, it would be helpful to add a docstring explaining why EscrowFinish transactions need credential IDs and how they relate to the XLS-0070d credentials standard mentioned in the PR objectives.
tests/integration/transactions/test_deposit_preauth.py (1)
17-37: Add docstring to clarify test purposeWhile the test implementation is solid, consider adding a docstring to explain:
- The purpose of testing both authorization and un-authorization in sequence
- Expected behavior for each operation
- Any prerequisites or assumptions
Example docstring:
""" Test the authorization and un-authorization fields of DepositPreauth transaction. Validates that: 1. An address can be successfully authorized 2. The same address can be subsequently unauthorized Both operations should return tesSUCCESS. """tests/unit/models/transactions/test_payment_channel_claim.py (1)
10-24: Add docstring and consider more realistic test dataWhile the test structure is good, consider:
- Adding a docstring to describe the test case
- Using a more realistic credential ID format (currently using a generic hex string)
Example docstring:
def test_valid(self): """Test successful creation of PaymentChannelClaim with valid credential ID."""🧰 Tools
🪛 Gitleaks
18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
xrpl/models/transactions/escrow_finish.py (1)
58-61: Consider adding validation hints in docstringWhile the docstring explains the basic functionality, it would be helpful to include the validation constraints (max 8 credentials, non-empty list requirement) to make the API more self-documenting.
"""Credentials associated with sender of this transaction. The credentials included must not be expired. If there are duplicates provided in the list, they will be - silently de-duped.""" + silently de-duped. The list must not be empty and cannot contain more than 8 + credential IDs."""tests/unit/models/transactions/test_credential_delete.py (1)
12-28: Add docstrings and improve test method naming.While the test implementation is correct and covers both valid parameter combinations, consider these improvements for better maintainability:
- Add a class docstring explaining the purpose of these tests
- Use more descriptive test method names (e.g.,
test_valid_with_issuer_accountandtest_valid_with_account_subject)- Add assertion messages to provide context when tests fail
Example improvement:
class TestCredentialDelete(TestCase): + """Unit tests for the CredentialDelete transaction model. + + Tests various combinations of parameters and validation rules for + credential deletion transactions. + """ - def test_valid(self): + def test_valid_with_issuer_account(self): tx = CredentialDelete( issuer=_ACCOUNT_ISSUER, account=_ACCOUNT_SUBJECT, credential_type=_VALID_CREDENTIAL_TYPE, ) - self.assertTrue(tx.is_valid()) + self.assertTrue(tx.is_valid(), "Transaction should be valid with issuer and account")tests/unit/models/transactions/test_credential_create.py (1)
1-11: Add module docstring and type hints for constants.Consider adding a module docstring to describe the purpose of these tests and type hints for the constants to improve code maintainability.
+"""Unit tests for the CredentialCreate transaction model.""" from unittest import TestCase from xrpl.models.exceptions import XRPLModelException from xrpl.models.transactions.credential_create import CredentialCreate from xrpl.utils import str_to_hex -_ACCOUNT_ISSUER = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ" -_ACCOUNT_SUBJECT = "rNdY9XDnQ4Dr1EgefwU3CBRuAjt3sAutGg" -_VALID_CREDENTIAL_TYPE = str_to_hex("Passport") -_VALID_URI = str_to_hex("www.my-id.com/username") +_ACCOUNT_ISSUER: str = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ" +_ACCOUNT_SUBJECT: str = "rNdY9XDnQ4Dr1EgefwU3CBRuAjt3sAutGg" +_VALID_CREDENTIAL_TYPE: str = str_to_hex("Passport") +_VALID_URI: str = str_to_hex("www.my-id.com/username")tests/integration/transactions/test_credential.py (2)
1-13: Add module-level documentation.Please add a module docstring describing the purpose of these integration tests and their relationship to the XLS-0070d credentials standard.
+""" +Integration tests for XLS-0070d compliant credential transactions. + +These tests verify the complete lifecycle of a credential: +1. Creation by an issuer +2. Acceptance by a subject +3. Deletion by the subject + +For more details on the credential standard, see: +https://github.com/XRPLF/XRPL-Standards/discussions/70 +""" from tests.integration.integration_test_case import IntegrationTestCase
14-14: Use a standards-compliant URI format.The URI should follow a more realistic format for credential identifiers as per XLS-0070d standard.
-_URI = "www.my-id.com/username" +_URI = "did:example:123456/credentials/1234"xrpl/models/transactions/payment_channel_claim.py (2)
111-114: Enhance docstring with validation constraintsWhile the docstring is informative, consider adding the validation constraints (max 8 elements) to make the requirements more visible to developers.
"""Credentials associated with sender of this transaction. The credentials included must not be expired. If there are duplicates provided in the list, they will be - silently de-duped.""" + silently de-duped. The list must not be empty when provided and cannot contain + more than 8 elements."""
111-128: Consider runtime validation of credential expirationWhile the docstring mentions that credentials must not be expired, there's no runtime validation for this constraint. Consider adding a validation mechanism or clarifying if this is handled at a different layer.
This could be implemented by:
- Adding an expiration check in
_get_errors- Documenting where the expiration validation occurs if it's handled elsewhere
- Adding a reference to the relevant section of the XLS-0070d specification
tests/unit/models/transactions/test_payment.py (3)
137-148: Consider improving test robustness and documentation.The test correctly validates the empty credential_ids case, but could be enhanced:
- Add a docstring explaining the test's purpose
- Consider using a constant for the error message to avoid string duplication across tests
- Consider using
assertIninstead ofassertEqualfor error message validation to make the test less brittledef test_credentials_array_empty(self): + """Test that Payment constructor raises XRPLModelException when credential_ids is empty.""" with self.assertRaises(XRPLModelException) as err: Payment( account=_ACCOUNT, amount=_XRP_AMOUNT, destination=_DESTINATION, credential_ids=[], ) - self.assertEqual( + self.assertIn( + "CredentialIDs list cannot be empty", err.exception.args[0], - "{'credential_ids': 'CredentialIDs list cannot be empty.'}" )
150-163: Enhance test clarity and maintainability.The test effectively validates the maximum length constraint, but could be improved:
- Add a docstring explaining the test's purpose
- Define MAX_CREDENTIALS constant (8) at class/module level
- Use f-strings instead of string concatenation
- Consider using
assertInfor more robust error message validation+ MAX_CREDENTIALS = 8 + def test_credentials_array_too_long(self): + """Test that Payment constructor raises XRPLModelException when credential_ids exceeds maximum length.""" with self.assertRaises(XRPLModelException) as err: Payment( account=_ACCOUNT, amount=_XRP_AMOUNT, destination=_DESTINATION, - credential_ids=["credential_index_" + str(i) for i in range(9)], + credential_ids=[f"credential_index_{i}" for i in range(self.MAX_CREDENTIALS + 1)], ) - self.assertEqual( + self.assertIn( + f"CredentialIDs list cannot have more than {self.MAX_CREDENTIALS} elements", err.exception.args[0], - "{'credential_ids': 'CredentialIDs list cannot have more than 8 " - + "elements.'}" )
137-163: Add test coverage for additional credential_ids scenarios.Consider adding the following test cases to ensure comprehensive validation:
- Test with valid credential_ids (1-8 elements)
- Test with invalid credential_id format/type (if there are format requirements)
Example test structure:
def test_valid_credentials(self): """Test that Payment constructor accepts valid credential_ids.""" tx = Payment( account=_ACCOUNT, amount=_XRP_AMOUNT, destination=_DESTINATION, credential_ids=["valid_credential_1", "valid_credential_2"], ) self.assertTrue(tx.is_valid())tests/unit/models/transactions/test_deposit_preauth.py (3)
19-25: Consider using named constants for better readability.The bitmap logic uses magic numbers that could be more self-documenting with named constants.
Consider this improvement:
+ # Bitmap flags for input parameters + AUTHORIZE_BIT = 1 # 0001 + UNAUTHORIZE_BIT = 2 # 0010 + AUTH_CREDS_BIT = 4 # 0100 + UNAUTH_CREDS_BIT = 8 # 1000 + + VALID_SINGLE_INPUT_CASES = [AUTHORIZE_BIT, UNAUTHORIZE_BIT, + AUTH_CREDS_BIT, UNAUTH_CREDS_BIT] + def test_all_input_combinations(self): - for val in range(0, 16): + for val in range(0, 16): # Test all possible combinations (2^4) # bitmap # 0'th bit represents authorize field # 1'th bit represents unauthorize field # 2'nd bit represents authorized_credentials field # 3'rd bit represents unauthorized_credentials field
59-78: Simplify error message construction.The error message concatenation is hard to maintain and prone to formatting issues.
Consider using f-strings or dedicated error message constants:
+ ERROR_NO_PARAMS = ( + "Exactly one input parameter amongst authorize, unauthorize, " + "authorize_credentials or unauthorize_credentials must be set. " + "It is invalid if none of the params are specified." + ) + + ERROR_MULTIPLE_PARAMS = ( + "More than one input param cannot be specified for DepositPreauth " + "transaction. Please specify exactly one input parameter." + ) + self.assertEqual( - error.exception.args[0], - "{'DepositPreauth': '" - + "Exactly one input parameter amongst authorize, unauthorize, " - + "authorize_credentials or unauthorize_credentials must be set" - + "." - + " It is " - + "invalid if none of the params are specified." - + "'}", + error.exception.args[0], + f"{{'DepositPreauth': '{ERROR_NO_PARAMS}'}}", )
82-83: Clarify the deduplication note.The note about credentials deduplication is vague. Consider adding more context about when and how deduplication might be implemented.
Add more detailed documentation:
- # Note: If credentials de-duplication is implemented in the client library, - # additional tests need to be written + # Note: If credential deduplication is implemented in the client library, + # additional tests will be needed to verify: + # 1. Duplicate credentials are properly filtered + # 2. The 8-element limit applies after deduplication + # 3. The order of credentials is preserved after deduplicationxrpl/models/transactions/credential_accept.py (2)
17-20: Improve docstring formatting for clarityThe class docstring can be enhanced by following PEP 257 conventions, which recommend starting with a one-line summary followed by a blank line and a more detailed description. Consider reformatting it as:
"""Accepts a credential issued to the Account. This transaction accepts a credential where the Account is the subject of the Credential object. The credential is not considered valid until it has been transferred or accepted. """
53-63: Optimize error message accumulationAccumulating error messages using string concatenation can be less efficient and harder to read. Consider using a list to collect error messages and then join them at the end. Here's an improved version:
def _get_credential_type_error(self: Self) -> Optional[str]: errors = [] if len(self.credential_type) == 0: errors.append("Length of credential_type field must be greater than 0.") if len(self.credential_type) > 64: errors.append("Length of credential_type field must not exceed 64 characters.") if not bool(HEX_REGEX.fullmatch(self.credential_type)): errors.append("credential_type field must be encoded in base-16 format.") return " ".join(errors) if errors else Nonexrpl/models/transactions/credential_delete.py (1)
29-35: Ensure consistent docstring formatting forsubjectandissuerfields.The docstrings for
subjectandissuerhave inconsistent indentation and line breaks. Aligning them enhances readability.Apply these diffs:
For the
subjectfield:-"""The person that the credential is for. If omitted, Account is assumed to be the - subject.""" +"""The person that the credential is for. If omitted, Account is assumed to be the subject."""For the
issuerfield:-"""The issuer of the credential. If omitted, Account is assumed to be the issuer.""" +"""The issuer of the credential. If omitted, Account is assumed to be the issuer."""xrpl/models/transactions/credential_create.py (1)
50-50: Avoid annotatingselfparameter in method definitionsPer PEP 484, the
selfparameter should not be annotated in method definitions. Removing the type annotation onselfimproves code readability and follows best practices.Apply this diff to remove the
self: Selfannotations:-def _get_errors(self: Self) -> Dict[str, str]: +def _get_errors(self) -> Dict[str, str]: -def _get_uri_error(self: Self) -> Optional[str]: +def _get_uri_error(self) -> Optional[str]: -def _get_credential_type_error(self: Self) -> Optional[str]: +def _get_credential_type_error(self) -> Optional[str]:Also applies to: 61-61, 73-73
xrpl/models/transactions/deposit_preauth.py (1)
67-78: Optimizesumusage by removing unnecessary list comprehensionYou can use a generator expression instead of a list comprehension inside
sumto avoid creating an intermediate list, which is more efficient.Apply this diff:
if ( - sum( - [ - param is not None - for param in ( - self.authorize, - self.unauthorize, - self.authorize_credentials, - self.unauthorize_credentials, - ) - ] - ) + sum( + param is not None + for param in ( + self.authorize, + self.unauthorize, + self.authorize_credentials, + self.unauthorize_credentials, + ) + ) > 1 ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
- .ci-config/rippled.cfg (1 hunks)
- tests/integration/transactions/test_credential.py (1 hunks)
- tests/integration/transactions/test_deposit_preauth.py (1 hunks)
- tests/unit/models/requests/test_deposit_authorized.py (1 hunks)
- tests/unit/models/transactions/test_account_delete.py (1 hunks)
- tests/unit/models/transactions/test_credential_accept.py (1 hunks)
- tests/unit/models/transactions/test_credential_create.py (1 hunks)
- tests/unit/models/transactions/test_credential_delete.py (1 hunks)
- tests/unit/models/transactions/test_deposit_preauth.py (1 hunks)
- tests/unit/models/transactions/test_escrow_finish.py (1 hunks)
- tests/unit/models/transactions/test_payment.py (1 hunks)
- tests/unit/models/transactions/test_payment_channel_claim.py (1 hunks)
- xrpl/core/binarycodec/definitions/definitions.json (9 hunks)
- xrpl/models/requests/account_objects.py (2 hunks)
- xrpl/models/requests/deposit_authorized.py (2 hunks)
- xrpl/models/transactions/init.py (2 hunks)
- xrpl/models/transactions/account_delete.py (2 hunks)
- xrpl/models/transactions/credential_accept.py (1 hunks)
- xrpl/models/transactions/credential_create.py (1 hunks)
- xrpl/models/transactions/credential_delete.py (1 hunks)
- xrpl/models/transactions/deposit_preauth.py (2 hunks)
- xrpl/models/transactions/escrow_finish.py (3 hunks)
- xrpl/models/transactions/payment.py (2 hunks)
- xrpl/models/transactions/payment_channel_claim.py (2 hunks)
- xrpl/models/transactions/types/transaction_type.py (1 hunks)
- xrpl/models/utils.py (1 hunks)
🧰 Additional context used
🪛 Gitleaks
tests/unit/models/transactions/test_payment_channel_claim.py
18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
57-57: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (47)
tests/unit/models/requests/test_deposit_authorized.py (1)
11-13: Verify credential ID format against XLS-0070d specification.The credential ID appears to be a 32-byte hex string. Let's verify this format matches the XLS-0070d specification.
xrpl/models/requests/deposit_authorized.py (2)
9-9: LGTM!The addition of
Setfrom typing is appropriate for the newcredentialsfield type annotation.
42-46: Consider adding validation and enhancing documentation.The implementation looks good but could benefit from some enhancements:
The docstring could specify:
- Expected format of credential IDs
- Any size limitations on the set
- Impact on authorization decision when credentials are provided
Consider adding validation for credential ID format if there are specific requirements.
Example enhancement:
credentials: Optional[Set[str]] = None """The object IDs of Credential objects. If this field is included, then the credential will be taken into account when analyzing whether the sender can send - funds to the destination. + funds to the destination. + + :param credentials: A set of credential IDs in canonical format. + Each ID must be a valid credential identifier as per XLS-0070d. + The authorization decision will consider both direct authorization + and any valid credentials provided in this set. """Let's verify the credential ID format requirements:
tests/unit/models/transactions/test_account_delete.py (1)
41-43: Verify credential ID format requirements.The test uses a 32-byte hex string as credential ID. Let's verify if this format is consistently required across the codebase.
✅ Verification successful
Credential ID format appears to be flexible with basic validation
Based on the code analysis:
- The
AccountDeletemodel only validates the number of credential IDs (must be between 1-8)- No specific format validation is enforced for the credential ID strings
- The test case uses a 64-character hex string, but this appears to be an example rather than a strict requirement
The 32-byte hex string format used in the test is valid but not explicitly required by the model validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for credential ID format validation or usage patterns # Look for credential ID validation logic rg -l "credential.*id" --type py # Search for credential ID format documentation rg -i "credential.*id.*format" --type mdLength of output: 559
Script:
#!/bin/bash # Look for credential ID validation or usage in model files rg -A 5 "credential.*id" xrpl/models/transactions/credential_accept.py rg -A 5 "credential.*id" xrpl/models/transactions/account_delete.py # Look for credential ID test cases to understand format patterns rg -A 5 "credential.*id" tests/integration/transactions/test_credential.py # Search for any hex string validation related to credentials ast-grep --pattern 'def validate_hex_string($$$)'Length of output: 1483
Script:
#!/bin/bash # Look for hex string patterns in credential-related test files rg -A 3 '[A-F0-9]{64}' tests/unit/models/transactions/test_account_delete.py tests/integration/transactions/test_credential.py # Look for credential ID validation in model files ast-grep --pattern 'class AccountDelete($$$)' # Check for any credential format constants or validation patterns rg -i "hex|format|pattern" xrpl/models/transactions/account_delete.pyLength of output: 611
xrpl/models/transactions/types/transaction_type.py (2)
21-23: LGTM! Verify related implementation files.The new credential transaction types are properly implemented and follow the existing conventions.
Let's verify the presence of corresponding transaction models and tests:
✅ Verification successful
Transaction types are properly implemented with corresponding models and tests ✅
The verification confirms that all necessary implementation files are present:
- Transaction models:
xrpl/models/transactions/credential_accept.pyxrpl/models/transactions/credential_create.pyxrpl/models/transactions/credential_delete.py- Test coverage:
- Integration:
tests/integration/transactions/test_credential.py- Unit tests for each model:
tests/unit/models/transactions/test_credential_accept.pytests/unit/models/transactions/test_credential_create.pytests/unit/models/transactions/test_credential_delete.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related credential implementation files # Test 1: Look for transaction model implementations echo "Checking for transaction models..." fd -t f "credential_(accept|create|delete).py" xrpl/models/transactions/ # Test 2: Look for test files echo "Checking for test files..." fd -t f "test_credential" tests/Length of output: 608
21-23: Consider updating documentation for new transaction types.Since these transaction types implement the XLS-0070d credentials standard, ensure they are properly documented.
Let's check for documentation updates:
xrpl/models/requests/account_objects.py (2)
25-25: LGTM! Enum value correctly added.The
CREDENTIALenum value follows the established pattern and maintains alphabetical ordering.
25-25: Verify credential object type usage.Let's ensure the new credential object type is properly integrated with the rest of the credential management features.
✅ Verification successful
The credential object type is properly integrated with the credential management features
The verification shows comprehensive integration:
- The
CREDENTIALaccount object type is used in the full credential lifecycle:
- Creation via
CredentialCreatetransaction- Acceptance via
CredentialAccepttransaction- Deletion via
CredentialDeletetransaction- Integration tests validate the complete credential workflow including:
- Object creation and storage in account directories
- Proper authorization flows between issuer and subject
- Cleanup and deletion verification
- The implementation includes proper validation for:
- Credential type format and length constraints
- Authorization rules and permissions
- Directory storage and cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the CREDENTIAL account object type # and its integration with credential-related features # Check for credential-related transaction types and their usage of account objects echo "Checking credential transaction implementations..." rg -l "AccountObjectType.CREDENTIAL" --type py # Check for credential-related tests echo "Checking credential-related tests..." rg -l "test.*credential" --type py tests/ # Check for any potential documentation about credentials echo "Checking for credential documentation..." fd -e md -e rst | xargs rg -l "credential"Length of output: 570
Script:
#!/bin/bash # Let's check the actual content of the credential test file and related code # Check the content of the credential test file echo "=== Content of credential test file ===" cat tests/integration/transactions/test_credential.py # Search for credential-related code in the codebase echo -e "\n=== Searching for credential-related code ===" rg "credential" -i --type py -C 2 # Look for any credential-related transaction types echo -e "\n=== Searching for credential transaction types ===" rg "class.*Credential" --type py -C 2Length of output: 86145
xrpl/models/transactions/account_delete.py (3)
4-6: LGTM: Import statements are appropriateThe new imports are correctly added to support type hints for the credential management feature.
53-65: 🛠️ Refactor suggestionImprove credential validation logic
The current validation has some potential improvements:
- The empty set check might be redundant since you're using
Setwhich already handles duplicates- Consider adding validation for individual credential ID format
- Error messages could be more specific about requirements
Consider this enhanced validation:
def _get_errors(self: Self) -> Dict[str, str]: errors = super()._get_errors() # Validation checks on the credential_ids field if self.credential_ids is not None: - if len(self.credential_ids) == 0: - errors["credential_ids"] = "CredentialIDs list cannot be empty." if len(self.credential_ids) > 8: errors[ "credential_ids" - ] = "CredentialIDs list cannot have more than 8 elements." + ] = "Maximum of 8 unique credentials are allowed, but got {}.".format( + len(self.credential_ids) + ) + # Add validation for individual credential ID format if specified in XLS-0070d + for cred_id in self.credential_ids: + if not self._is_valid_credential_id(cred_id): + errors["credential_ids"] = f"Invalid credential ID format: {cred_id}" + break return errorsLet's check the credential ID format requirements:
#!/bin/bash # Search for credential ID format specifications or examples in the codebase rg -A 3 "credential.*id.*format|credential.*example"
Line range hint
1-65: Consider transaction ordering and deletion constraintsSince AccountDelete is a critical transaction that removes an account from the ledger, we should carefully consider the interaction between credentials and account deletion:
- Should all associated credentials be automatically invalidated upon account deletion?
- Should account deletion be prevented if there are active credentials?
- How does this interact with the account deletion requirements mentioned in the class docstring?
Consider documenting these behavioral implications in the class docstring and ensuring they align with the XLS-0070d standard requirements.
Let's check for related documentation:
tests/unit/models/transactions/test_escrow_finish.py (1)
55-66: LGTM! Good edge case coverage.The test effectively validates the empty list constraint with clear error message verification.
tests/integration/transactions/test_deposit_preauth.py (2)
6-6: LGTM! Clean import organizationThe new imports are well-organized and the use of
DESTINATIONfrom reusable values promotes better maintainability over hardcoded addresses.Also applies to: 9-10
Line range hint
1-67: Verify test execution in CI environmentAccording to the PR objectives, integration tests may not pass in CI due to missing Docker container for the custom branch.
Let's check for any CI configuration:
Consider:
- Adding clear documentation about test environment requirements
- Providing instructions for running tests with custom rippled builds
- Adding CI skip markers or conditional test execution based on environment
tests/unit/models/transactions/test_payment_channel_claim.py (3)
1-7: LGTM: Clean imports and well-structured constantsThe imports are minimal and appropriate for the test suite, and the account constant helps maintain DRY principles.
18-19: Note: Static analysis warnings for public keys are false positivesThe public key strings flagged by static analysis are valid XRPL public keys in hex format, not API keys. This is the correct format for XRPL transaction test data.
Also applies to: 36-37, 57-58
🧰 Tools
🪛 Gitleaks
18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
20-22: Verify credential ID format against XLS-0070d standardLet's verify that the credential ID format in the test matches the specification.
✅ Verification successful
Based on the search results, I can now provide a final response:
Credential ID format is valid and consistent with implementation
The credential ID format used in the test (
EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A) is valid according to the codebase implementation which shows:
- The format is consistently used across multiple test files (PaymentChannelClaim, AccountDelete, EscrowFinish)
- The implementation enforces validation rules for credential IDs:
- Cannot be empty
- Maximum of 8 credentials allowed
- Must be unique (silently de-duped)
The 64-character hexadecimal format matches the standard cryptographic identifier pattern used in XRPL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for credential ID format documentation in the codebase rg -l "credential.*id.*format" --type md # Search for credential-related constants or validation patterns ast-grep --pattern 'CREDENTIAL_ID_PATTERN = $_'Length of output: 92
Script:
#!/bin/bash # Search for credential ID validation or format checks in Python files rg -A 5 "credential.*id" --type py # Search for any credential-related constants or patterns rg -A 5 "CREDENTIAL" --type py # Look for test files related to credentials fd "test.*credential.*\.py$"Length of output: 24150
xrpl/models/transactions/escrow_finish.py (3)
6-6: LGTM: Clean import additionThe Set import is appropriately added to support the new credential_ids field type.
74-82: Consider adding credential format validationThe current validation only checks for list constraints but doesn't validate the format of the credential IDs themselves. Consider adding validation for the expected format of credential IDs if specified in the XLS-0070d standard.
#!/bin/bash # Search for credential ID format specifications or validations rg -i "credential.*format|credential.*pattern|credential.*validate"
74-82: 🛠️ Refactor suggestionVerify credential limit alignment with XLS-0070d standard
The validation logic looks correct, but we should verify that the 8-credential limit aligns with the XLS-0070d specification requirements.
Additionally, consider extracting the magic number 8 into a class constant for better maintainability:
@dataclass(frozen=True, **KW_ONLY_DATACLASS) class EscrowFinish(Transaction): + MAX_CREDENTIALS = 8 ... def _get_errors(self: Self) -> Dict[str, str]: ... if self.credential_ids is not None: if len(self.credential_ids) == 0: errors["credential_ids"] = "CredentialIDs list cannot be empty." - if len(self.credential_ids) > 8: + if len(self.credential_ids) > self.MAX_CREDENTIALS: errors[ "credential_ids" - ] = "CredentialIDs list cannot have more than 8 elements." + ] = f"CredentialIDs list cannot have more than {self.MAX_CREDENTIALS} elements."tests/unit/models/transactions/test_credential_delete.py (2)
1-10: LGTM! Well-structured test setup.The imports are appropriate, and the test constants are well-defined at the module level. Good practice using
str_to_hexutility for credential type encoding.
1-79: Verify compliance with XLS-0070d standard and integration points.While the unit tests are well-structured, we should verify:
- The error messages and validation rules align with the XLS-0070d standard
- The test cases cover all required scenarios from the standard
- The integration points with the C++ implementation are consistent
Let's check for any related test files and validation rules:
✅ Verification successful
Integration and validation rules are properly implemented
The review concern can be resolved based on the comprehensive analysis of the implementation:
The validation rules are consistent across all credential transactions (Create, Accept, Delete):
- Hex-encoded credential_type with length limits (0-64 bytes)
- Required issuer/subject validation
- Consistent error messages
The integration test demonstrates proper lifecycle:
- Full create-accept-delete flow
- Directory page verification
- Proper cleanup validation
The implementation aligns with credential management best practices:
- Proper permission checks (subject/issuer relationships)
- Atomic operations
- Directory consistency checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other credential-related test files echo "=== Related test files ===" fd test_credential # Search for credential validation rules echo -e "\n=== Validation rules ===" rg -A 5 "class CredentialDelete" # Search for standard references echo -e "\n=== Standard references ===" rg -i "XLS-0070d"Length of output: 978
Script:
#!/bin/bash # Let's check the implementation and integration tests for more context # Check credential delete implementation echo "=== CredentialDelete Implementation ===" cat xrpl/models/transactions/credential_delete.py # Check integration tests echo -e "\n=== Integration Tests ===" cat tests/integration/transactions/test_credential.py # Check other credential transaction tests for consistency echo -e "\n=== Other Credential Tests ===" cat tests/unit/models/transactions/test_credential_create.py cat tests/unit/models/transactions/test_credential_accept.pyLength of output: 13462
.ci-config/rippled.cfg (1)
185-186: LGTM! Verify rippled implementation.The amendments are correctly added:
fixAMMv1_1: Addresses the missing amendment as discussed.Credentials: Aligns with the XLS-0070d credentials standard implementation.Let's verify the amendments are properly defined in rippled:
xrpl/models/utils.py (2)
3-7: LGTM! Import changes are well-structured.The addition of
PatternandFinaltypes enhances type safety and clearly indicates the immutability of the regex constant.
11-14: 🛠️ Refactor suggestionConsider enhancing the regex pattern and documentation.
While the implementation is functional, there are a few suggestions for improvement:
- The comment could be more specific about the exact length requirements and format expectations.
- Consider adding a maximum length constraint to prevent potential abuse.
- The regex could be more restrictive if there are additional format requirements for credentials.
Let's check if there are any length constraints or format requirements in the XLS-0070d spec or related code:
Consider updating the pattern to include length constraints if applicable:
-HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]+") +# Maximum length based on XLS-0070d spec (update the value as per spec) +HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]{1,64}")✅ Verification successful
Let me gather more context about the validation logic to understand the length constraints better.
Let me check one more thing to understand the validation implementation and length constraints:
Based on the gathered evidence, I can now provide the final response:
The regex pattern and validation are correctly implemented
The current implementation of
HEX_REGEXis appropriate for its use case:
- The length constraints are already enforced in the transaction validation logic, as evidenced by the error messages: "Length of credential_type field must not be greater" and "Length of credential_type field must be greater than 0"
- The format requirement is correctly enforced - credentials must be in base-16 format
- Test cases confirm the validation works with practical examples like "Passport" converted to hex
The comment could be more specific, but the implementation itself is sound and follows the codebase's validation requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for credential-related length or format constraints rg -i "credential.*length|credential.*format" # Search for usage of HEX_REGEX to understand validation context ast-grep --pattern 'HEX_REGEX.match($_)'Length of output: 1755
Script:
#!/bin/bash # Search for validation logic in credential-related files rg -A 5 "validate_credential" # Look for actual credential type validation implementation ast-grep --pattern 'def validate_credential_type(self, $_): $$$ ' # Check test files for example credential types rg "credential_type.*=" tests/Length of output: 2912
Script:
#!/bin/bash # Look for the actual validation implementation in credential transaction files rg -B 2 -A 10 "def validate_credential_type" xrpl/models/transactions/ # Check for _VALID_CREDENTIAL_TYPE constant definition rg "_VALID_CREDENTIAL_TYPE.*=" tests/Length of output: 427
tests/unit/models/transactions/test_credential_create.py (1)
64-106: LGTM! Comprehensive validation tests for credential_type field.The tests thoroughly cover the validation requirements for the credential_type field, including length limits, empty values, and format validation.
tests/integration/transactions/test_credential.py (1)
49-54: Validate credential data and transaction fees.The test should verify:
- The hex-encoded URI in the created credential object
- The transaction fees for each operation
- The complete credential object data after creation
xrpl/models/transactions/payment_channel_claim.py (1)
5-7: LGTM: Import changes are appropriateThe addition of
Setfrom typing andSelffrom typing_extensions provides proper type hints for the new functionality while maintaining backward compatibility.xrpl/models/transactions/__init__.py (2)
31-33: LGTM! New credential transaction imports are well-organized.The new imports for credential-related transactions (CredentialAccept, CredentialCreate, CredentialDelete) are properly organized and follow the existing import pattern.
119-121: LGTM! New credential transactions properly exposed in public API.The credential transaction classes are correctly added to the
__all__list, maintaining alphabetical order and following the module's public API conventions.Let's verify the complete integration of these new transaction types:
✅ Verification successful
Credential transaction types are properly integrated across the codebase
The verification confirms complete integration:
- Transaction type enums are defined in
transaction_type.py- Implementation files exist for all three transactions:
credential_accept.py,credential_create.py, andcredential_delete.py- Test coverage is comprehensive with both unit tests and integration tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the complete integration of credential transaction types # Test 1: Check if the corresponding transaction type definitions exist echo "Checking transaction type definitions..." rg "CREDENTIAL_(ACCEPT|CREATE|DELETE)" -A 1 # Test 2: Verify the existence of model implementation files echo "Checking model implementation files..." fd "credential_(accept|create|delete).py" "xrpl/models/transactions" # Test 3: Look for corresponding test files echo "Checking test coverage..." fd "test_credential" "tests"Length of output: 1606
xrpl/models/transactions/payment.py (3)
7-7: LGTM: Import addition is correctThe
Setimport is appropriately placed with other typing imports and is required for the newcredential_idsfield type.
132-148: Verify integration with credential systemSince this is part of a larger credential management feature, we should verify:
- That the Payment transaction properly integrates with the credential validation system
- That credential verification occurs before payment processing
Let's check the integration points:
✅ Verification successful
Based on the search results, I can now generate the final response:
Integration with credential system is properly implemented
The Payment transaction correctly integrates with the credential system through:
- Proper validation of credential_ids field with consistent checks across all transaction types (empty list and max 8 elements)
- Credential validation happens at the model level before any payment processing
- The credential system is consistently implemented across related transaction types (Payment, PaymentChannelClaim, AccountDelete, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for credential validation code rg -A 5 "verify.*credential|validate.*credential" # Search for payment processing order ast-grep --pattern $'def process_payment($$$) { $$$ }'Length of output: 258
Script:
#!/bin/bash # Search for credential validation code with broader patterns rg -A 5 "credential" # Search for payment processing with correct Python syntax ast-grep --pattern 'def process_payment($_): $$$' # Look for credential-related imports and class definitions rg -A 3 "class.*Payment|from.*credential|import.*credential"Length of output: 72643
140-148: 🛠️ Refactor suggestionConsider adding credential ID format validation
While the size validation is good, consider also validating the format of credential IDs. This would help catch issues earlier in the validation chain.
Here's a suggested enhancement:
if self.credential_ids is not None: if len(self.credential_ids) == 0: errors["credential_ids"] = "CredentialIDs list cannot be empty." if len(self.credential_ids) > 8: errors[ "credential_ids" ] = "CredentialIDs list cannot have more than 8 elements." + for cred_id in self.credential_ids: + if not is_valid_credential_id(cred_id): # Add this helper function + errors["credential_ids"] = f"Invalid credential ID format: {cred_id}"Let's verify if other transaction types have consistent validation:
xrpl/core/binarycodec/definitions/definitions.json (10)
56-56: LGTM: Credential ledger entry type additionThe new Credential ledger entry type is correctly added with ID 129, following the existing pattern and maintaining proper numeric ordering after Oracle (128).
1074-1093: LGTM: IssuerNode and SubjectNode field definitionsThe UInt64 fields for credential node references are properly defined with:
- Correct sequential nth values (24, 25)
- Consistent serialization flags
- Appropriate type assignment
2004-2013: LGTM: CredentialType field definitionThe CredentialType blob field is correctly defined with:
- Proper nth value (30)
- VL encoding enabled for variable-length data
- Consistent serialization flags
2174-2183: LGTM: Subject AccountID field definitionThe Subject field is properly defined as an AccountID type with:
- Correct nth value (24)
- VL encoding enabled for account addresses
- Consistent serialization flags
2224-2233: LGTM: CredentialIDs Vector256 field definitionThe CredentialIDs field is correctly defined with:
- Proper nth value (5)
- VL encoding enabled for array data
- Consistent serialization flags
2604-2613: LGTM: Credential STObject field definitionThe Credential object field is properly defined with:
- Correct nth value (33)
- Appropriate type assignment
- Consistent serialization flags
2803-2822: LGTM: Credential authorization array field definitionsThe AuthorizeCredentials and UnauthorizeCredentials array fields are properly defined with:
- Sequential nth values (26, 27)
- Consistent serialization flags
- Appropriate type assignments
3008-3008: LGTM: Credential transaction result codeThe tecBAD_CREDENTIALS result code is properly defined with value 192, following the existing pattern for technical failure codes.
3059-3061: LGTM: Credential transaction typesThe credential-related transaction types are properly defined with sequential values:
- CredentialCreate: 54
- CredentialAccept: 55
- CredentialDelete: 56
Line range hint
1-3061: Verify field references in the codebaseThe changes look good overall. Let's verify that all new fields are properly referenced in the codebase.
✅ Verification successful
The search results show that the new credential-related fields, transaction types, and result codes are properly referenced across the codebase:
- The credential fields are referenced in multiple transaction models and their corresponding test files
- The credential transaction types (Create/Accept/Delete) are defined in the transaction types and have corresponding model and test files
- The
tecBAD_CREDENTIALSresult code is referenced in the definitions fileLet's verify the actual implementation of these credential-related files to ensure completeness:
Based on the implementation files and integration tests, I can verify that the new credential-related fields in the definitions.json are properly implemented and used in the codebase:
All credential-related changes are properly integrated
- The transaction models (CredentialCreate/Accept/Delete) correctly use the new fields
- Field validations are implemented for credential_type and URI
- Integration tests verify the full credential lifecycle including creation, acceptance, and deletion
- The tests validate the presence/absence of credential objects in the ledger
- All new fields from definitions.json (IssuerNode, SubjectNode, CredentialType, etc.) are referenced and used appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new credential fields in the codebase # Search for credential field references echo "Searching for credential field references..." rg -l "IssuerNode|SubjectNode|CredentialType|CredentialIDs|AuthorizeCredentials|UnauthorizeCredentials" # Search for credential transaction type references echo "Searching for credential transaction type references..." rg -l "CredentialCreate|CredentialAccept|CredentialDelete" # Search for credential result code references echo "Searching for credential result code references..." rg -l "tecBAD_CREDENTIALS"Length of output: 1659
Script:
#!/bin/bash # Check implementation of credential transaction files echo "=== Credential Create Implementation ===" cat xrpl/models/transactions/credential_create.py echo -e "\n=== Credential Accept Implementation ===" cat xrpl/models/transactions/credential_accept.py echo -e "\n=== Credential Delete Implementation ===" cat xrpl/models/transactions/credential_delete.py echo -e "\n=== Integration Test ===" cat tests/integration/transactions/test_credential.pyLength of output: 12545
xrpl/models/transactions/credential_delete.py (2)
16-17: Well-definedCredentialDeletetransaction class.The class correctly defines the
CredentialDeletetransaction, inheriting fromTransaction, and uses dataclass for immutable field definitions.
39-56: Efficient error collection in_get_errorsmethod.The
_get_errorsmethod effectively aggregates errors from the superclass and validates thecredential_type. The check ensuring at least one ofsubjectorissueris provided is appropriate.xrpl/models/transactions/credential_create.py (1)
6-6: Verify compatibility ofSelffromtyping_extensionswith supported Python versionsEnsure that the use of
Selffromtyping_extensionsis compatible with the minimum Python version supported by the project. If the project supports Python versions earlier than 3.10 (whereSelfwas introduced intyping_extensions), importingSelfmay cause compatibility issues.Run the following script to identify usages of
Selfin the codebase:✅ Verification successful
Let me gather more information about the Python version requirements.
Using
Selffromtyping_extensionsis compatible with project requirementsThe project requires Python ^3.8 and typing-extensions ^4.2.0 as dependencies. The
Selftype was added to typing-extensions in version 3.11, which is well within the project's requirements. The widespread usage across 50+ files in the codebase also indicates this is an established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `Self` in the codebase to verify compatibility. # Expected: List of files using `Self`. rg --type=py --word-regexp 'from typing_extensions import Self' --files-with-matchesLength of output: 2509
Script:
#!/bin/bash # Check Python version requirements in setup.py, pyproject.toml, or similar files fd "setup.py|pyproject.toml|requirements.txt" --type f --exec cat {}Length of output: 2201
xrpl/models/transactions/deposit_preauth.py (1)
124-127: Review necessity of# type: ignorecomments on required fieldsThe
# type: ignorecomments on lines 124 and 127 suppress type checking for theissuerandcredential_typefields. If possible, adjust the type annotations or theREQUIREDusage to eliminate the need for these ignores, ensuring that type checking remains effective.Would you like assistance in resolving the type checking issues to remove the
# type: ignorecomments?
| credential_ids: Optional[Set[str]] = None | ||
| """Credentials associated with sender of this transaction. The credentials included | ||
| must not be expired. If there are duplicates provided in the list, they will be | ||
| silently de-duped.""" |
There was a problem hiding this comment.
💡 Codebase verification
Documentation update needed for credential_ids field
The review comment is partially correct. The code shows that there are validation checks in place for the credential_ids field that enforce:
- Non-empty list (error: "CredentialIDs list cannot be empty")
- Maximum of 8 credentials (error: "CredentialIDs list cannot have more than 8 elements")
- Automatic de-duplication through the use of Set[str]
The docstring should be updated to reflect these constraints:
credential_ids: Optional[Set[str]] = None
"""Credentials associated with sender of this transaction. The credentials included
- must not be expired. If there are duplicates provided in the list, they will be
- silently de-duped."""
+ must not be expired and must follow these constraints:
+ - Maximum of 8 unique credentials allowed
+ - Cannot be an empty list when specified
+ - Duplicates will be automatically removed through Set data structure
+ """🔗 Analysis chain
Enhance credential_ids documentation and validation
The docstring should be more specific about constraints:
- Maximum limit of 8 credentials should be documented
- Silent de-duplication behavior might be surprising to users
- Consider adding validation for credential ID format
Apply this enhancement to the docstring:
credential_ids: Optional[Set[str]] = None
"""Credentials associated with sender of this transaction. The credentials included
must not be expired. If there are duplicates provided in the list, they will be
- silently de-duped."""
+ silently de-duped. Maximum of 8 unique credentials are allowed.
+
+ Note: Duplicate credentials will be automatically removed without warning."""Let's check if there's a specific format for credential IDs:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for credential ID format validation in related files
rg -l "credential.*id.*format|credential.*validation"
Length of output: 148
Script:
#!/bin/bash
# Search for credential-related validation or format checks
rg -A 3 "credential.*id|credential.*format" --type python
# Search for any credential-related constants or limits
rg -A 3 "MAX.*CREDENTIAL|CREDENTIAL.*LIMIT" --type python
# Look for credential validation in test files
rg -A 3 "test.*credential" --type python
# Search for credential-related error messages or exceptions
rg "credential.*error|invalid.*credential" --type python
Length of output: 345
Script:
#!/bin/bash
# Search for credential-related validation or format checks
rg -A 3 "credential.*id|credential.*format"
# Search for any credential-related constants or limits
rg -A 3 "MAX.*CREDENTIAL|CREDENTIAL.*LIMIT"
# Look for credential validation in test files
rg -A 3 "test.*credential"
# Search for credential-related error messages or exceptions
rg "credential.*error|invalid.*credential"
# Look for credential-related code in Python files
fd -e py -x cat {} | rg -A 3 "credential"
Length of output: 39713
| # invalid inputs to the credential_type field | ||
| def test_cred_type_field_too_long(self): | ||
| with self.assertRaises(XRPLModelException) as error: | ||
| CredentialAccept( | ||
| issuer=_ACCOUNT_ISSUER, | ||
| account=_ACCOUNT_SUBJECT, | ||
| credential_type=str_to_hex("A" * 65), | ||
| ) | ||
| self.assertEqual( | ||
| error.exception.args[0], | ||
| "{'credential_type': 'Length of credential_type field must not be greater " | ||
| + "than 64 bytes. '}", | ||
| ) | ||
|
|
||
| def test_cred_type_field_empty(self): | ||
| with self.assertRaises(XRPLModelException) as error: | ||
| CredentialAccept( | ||
| issuer=_ACCOUNT_ISSUER, | ||
| account=_ACCOUNT_SUBJECT, | ||
| credential_type="", | ||
| ) | ||
| self.assertEqual( | ||
| error.exception.args[0], | ||
| "{'credential_type': 'Length of credential_type field must be greater than " | ||
| + "0. credential_type field must be encoded in base-16 format. '}", | ||
| ) | ||
|
|
||
| def test_cred_type_field_not_hex(self): | ||
| with self.assertRaises(XRPLModelException) as error: | ||
| CredentialAccept( | ||
| issuer=_ACCOUNT_ISSUER, | ||
| account=_ACCOUNT_SUBJECT, | ||
| credential_type="Passport", | ||
| ) | ||
| self.assertEqual( | ||
| error.exception.args[0], | ||
| "{'credential_type': 'credential_type field must be encoded in base-16 " | ||
| + "format. '}", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding boundary test cases.
The current tests cover important invalid scenarios, but consider adding these additional test cases:
- Test with exactly 64 bytes (boundary condition)
- Test with special characters in hex string
- Test with mixed case hex string
def test_cred_type_field_exact_length(self):
# Test with exactly 64 bytes
tx = CredentialAccept(
issuer=_ACCOUNT_ISSUER,
account=_ACCOUNT_SUBJECT,
credential_type=str_to_hex("A" * 32), # 32 chars = 64 bytes in hex
)
self.assertTrue(tx.is_valid())
def test_cred_type_field_special_hex(self):
# Test with valid hex containing special characters
with self.assertRaises(XRPLModelException) as error:
CredentialAccept(
issuer=_ACCOUNT_ISSUER,
account=_ACCOUNT_SUBJECT,
credential_type="!@#$%^",
)
def test_cred_type_field_mixed_case(self):
# Test with mixed case hex string
tx = CredentialAccept(
issuer=_ACCOUNT_ISSUER,
account=_ACCOUNT_SUBJECT,
credential_type="aAbBcCdD",
)
self.assertTrue(tx.is_valid())
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
xrpl/models/transactions/deposit_preauth.py (1)
62-87: Consider simplifying the parameter validation logic.While the current implementation is correct, it could be more concise by using list comprehension and sum:
- if ( - self.authorize is None - and self.unauthorize is None - and self.authorize_credentials is None - and self.unauthorize_credentials is None - ): + params = [ + self.authorize, + self.unauthorize, + self.authorize_credentials, + self.unauthorize_credentials, + ] + if not any(params):tests/unit/models/transactions/test_deposit_preauth.py (3)
32-54: Improve test maintainability and setup.
- Move sample credentials to test-level setup to avoid potential test interdependency.
- Consider using test helper methods for credential creation.
def setUp(self): self.sample_credential = Credential( issuer="SampleIssuer", credential_type="SampleCredType" ) def _create_sample_credentials(self, count=1): return [ Credential( issuer=f"SampleIssuer_{i}", credential_type="SampleCredType" ) for i in range(count) ]
56-138: Reduce test duplication and improve error message construction.
- Consider using
subTestto parameterize the array validation tests.- Use f-strings instead of string concatenation for error messages.
def test_credentials_array_validation(self): test_cases = [ ("authorize_credentials", "AuthorizeCredentials"), ("unauthorize_credentials", "UnauthorizeCredentials"), ] for field, prefix in test_cases: with self.subTest(f"{field}_validation"): # Test exceeding length credentials = self._create_sample_credentials(MAX_CREDENTIAL_ARRAY_LENGTH + 1) with self.assertRaises(XRPLModelException) as error: kwargs = { "account": _ACCOUNT, "fee": _FEE, "sequence": _SEQUENCE, field: credentials, } DepositPreauth(**kwargs) self.assertEqual( error.exception.args[0], f"{{'DepositPreauth': '{prefix} list cannot exceed {MAX_CREDENTIAL_ARRAY_LENGTH} elements.'}}" )
140-154: Consolidate empty array tests with array validation tests.The empty array tests could be included in the parameterized test structure suggested above:
# Add to test_credentials_array_validation with self.subTest(f"{field}_empty_array"): with self.assertRaises(XRPLModelException) as error: kwargs = { "account": _ACCOUNT, "fee": _FEE, "sequence": _SEQUENCE, field: [], } DepositPreauth(**kwargs) self.assertEqual( error.exception.args[0], f"{{'DepositPreauth': '{prefix} list cannot be empty.'}}" )tests/unit/models/transactions/test_payment.py (2)
150-162: Consider extracting the magic number for better maintainability.While the test is well-structured, consider extracting the magic number '9' as a constant to improve maintainability and make the relationship with the maximum limit of 8 more explicit.
+ MAX_CREDENTIAL_IDS = 8 + EXCEED_MAX_CREDENTIAL_IDS = MAX_CREDENTIAL_IDS + 1 def test_credentials_array_too_long(self): with self.assertRaises(XRPLModelException) as err: Payment( account=_ACCOUNT, amount=_XRP_AMOUNT, destination=_DESTINATION, - credential_ids=["credential_index_" + str(i) for i in range(9)], + credential_ids=["credential_index_" + str(i) for i in range(EXCEED_MAX_CREDENTIAL_IDS)], )
164-177: Enhance readability and clarity of the test.Two suggestions to improve this test:
- Use an f-string instead of string concatenation for the error message assertion
- Make the test data more explicit about demonstrating duplicates
def test_credentials_array_duplicates(self): with self.assertRaises(XRPLModelException) as err: Payment( account=_ACCOUNT, amount=_XRP_AMOUNT, destination=_DESTINATION, - credential_ids=["credential_index" for _ in range(5)], + credential_ids=["cred_1", "cred_2", "cred_1", "cred_3"], # explicit duplicate of "cred_1" ) self.assertEqual( err.exception.args[0], - "{'credential_ids_duplicates': 'CredentialIDs list cannot contain duplicate" - + " values.'}", + "{'credential_ids_duplicates': 'CredentialIDs list cannot contain duplicate values.'}" )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/unit/models/transactions/test_deposit_preauth.py(2 hunks)tests/unit/models/transactions/test_payment.py(1 hunks)xrpl/models/transactions/credential_accept.py(1 hunks)xrpl/models/transactions/credential_create.py(1 hunks)xrpl/models/transactions/credential_delete.py(1 hunks)xrpl/models/transactions/deposit_preauth.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- xrpl/models/transactions/credential_accept.py
- xrpl/models/transactions/credential_delete.py
- xrpl/models/transactions/credential_create.py
🧰 Additional context used
📓 Learnings (2)
xrpl/models/transactions/deposit_preauth.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
tests/unit/models/transactions/test_deposit_preauth.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
🔇 Additional comments (6)
xrpl/models/transactions/deposit_preauth.py (3)
6-7: LGTM! Clear and well-documented field additions.
The new imports and field declarations are appropriate for the credential management functionality. The docstrings clearly describe the purpose of each field.
Also applies to: 14-18, 48-53
89-104: LGTM! Well-structured validation logic.
The validation function effectively checks for:
- Empty arrays
- Arrays exceeding maximum length
- Duplicate credentials
The error messages are clear and descriptive.
119-131: 🛠️ Refactor suggestion
Add validation for Credential fields.
Consider adding validation to ensure:
issueris a valid account addresscredential_typeis a properly formatted hex string
@require_kwargs_on_init
@dataclass(frozen=True, **KW_ONLY_DATACLASS)
class Credential(NestedModel):
+ def _get_errors(self: Self) -> Dict[str, str]:
+ errors = {}
+ if not is_valid_classic_address(self.issuer):
+ errors["issuer"] = "Invalid XRPL account address"
+ if not is_hex_string(self.credential_type):
+ errors["credential_type"] = "Must be a hex-encoded string"
+ return errorsLikely invalid or redundant comment.
tests/unit/models/transactions/test_payment.py (3)
137-148: LGTM! Well-structured test for empty credential_ids validation.
The test effectively validates that an empty credential_ids list raises the appropriate exception with a clear error message.
137-177: Add test case for successful credential_ids validation.
While the error cases are well covered, consider adding a test case for successful validation with valid credential_ids.
Example test to add:
def test_valid_payment_with_credentials(self):
transaction_dict = {
"account": _ACCOUNT,
"amount": _XRP_AMOUNT,
"destination": _DESTINATION,
"credential_ids": ["cred1", "cred2", "cred3"], # adjust format as per spec
}
tx = Payment(**transaction_dict)
self.assertTrue(tx.is_valid())137-177: Verify consistent credential_ids validation across transaction types.
Since these tests validate credential_ids constraints for the Payment transaction, let's verify that the same validation rules are consistently implemented across other transaction types mentioned in the PR (AccountDelete, PaymentChannelClaim, EscrowFinish).
✅ Verification successful
Based on all the gathered information, I can now provide the final response:
Credential_ids validation is consistently implemented across transaction types
The verification confirms that credential_ids validation is properly and consistently implemented:
- All four transaction types (Payment, AccountDelete, PaymentChannelClaim, and EscrowFinish) import and use the same
validate_credential_idsfunction from utils.py - The validation function in utils.py enforces consistent rules:
- Non-empty list check
- Maximum 8 elements check
- No duplicates check
- Each transaction type properly integrates the validation by calling
errors.update(validate_credential_ids(self.credential_ids))in their validation logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent credential_ids validation across transaction types
# Search for credential_ids validation patterns
echo "Searching for credential_ids validation patterns..."
rg -A 5 "credential_ids.*=.*Field" xrpl/models/transactions/
# Search for validation error messages to ensure consistency
echo "Searching for credential_ids validation error messages..."
rg "CredentialIDs.*cannot" xrpl/models/
Length of output: 622
Script:
#!/bin/bash
# Let's check how credential_ids validation is implemented and used
# First, check the implementation in utils.py
echo "=== Checking credential_ids validation implementation ==="
rg -B 2 -A 10 "def validate_credential_ids" xrpl/models/utils.py
# Then, check which transaction types use this validation
echo -e "\n=== Checking transaction types using credential_ids ==="
ast-grep --pattern 'class $_ extends Transaction:
$$$
credential_ids: $$$
$$$'
# Also check for direct Field usage
echo -e "\n=== Checking direct credential_ids field definitions ==="
rg "credential_ids.*=.*Field" xrpl/models/transactions/
Length of output: 918
Script:
#!/bin/bash
# Let's try a different approach to find credential_ids usage
# Search for credential_ids in transaction models
echo "=== Searching for credential_ids in transaction models ==="
rg -l "credential_ids" xrpl/models/transactions/
# Check for imports of validate_credential_ids
echo -e "\n=== Checking imports of validate_credential_ids ==="
rg "from.*utils.*validate_credential_ids" xrpl/models/transactions/
# Get more context around credential_ids validation implementation
echo -e "\n=== Getting full credential_ids validation implementation ==="
rg -B 5 -A 15 "validate_credential_ids" xrpl/models/utils.py
# Look for any class definitions containing credential_ids
echo -e "\n=== Looking for classes with credential_ids ==="
ast-grep --pattern 'class $CLASS {
$$$
credential_ids$$$
$$$
}'
Length of output: 1627
Script:
#!/bin/bash
# Let's check how credential_ids is defined in each transaction type
echo "=== Checking Payment.py credential_ids implementation ==="
rg -B 2 -A 5 "credential_ids.*=.*Field" xrpl/models/transactions/payment.py
echo -e "\n=== Checking AccountDelete.py credential_ids implementation ==="
rg -B 2 -A 5 "credential_ids.*=.*Field" xrpl/models/transactions/account_delete.py
echo -e "\n=== Checking PaymentChannelClaim.py credential_ids implementation ==="
rg -B 2 -A 5 "credential_ids.*=.*Field" xrpl/models/transactions/payment_channel_claim.py
echo -e "\n=== Checking EscrowFinish.py credential_ids implementation ==="
rg -B 2 -A 5 "credential_ids.*=.*Field" xrpl/models/transactions/escrow_finish.py
# Check for validation calls in these files
echo -e "\n=== Checking validation calls in transaction files ==="
rg "validate_credential_ids" xrpl/models/transactions/{payment,account_delete,payment_channel_claim,escrow_finish}.py
Length of output: 1957
Script:
#!/bin/bash
# Let's check the class definitions to see how credential_ids is defined
echo "=== Checking class definitions in Payment.py ==="
rg -B 5 -A 15 "class Payment" xrpl/models/transactions/payment.py
echo -e "\n=== Checking class definitions in AccountDelete.py ==="
rg -B 5 -A 15 "class AccountDelete" xrpl/models/transactions/account_delete.py
echo -e "\n=== Checking class definitions in PaymentChannelClaim.py ==="
rg -B 5 -A 15 "class PaymentChannelClaim" xrpl/models/transactions/payment_channel_claim.py
echo -e "\n=== Checking class definitions in EscrowFinish.py ==="
rg -B 5 -A 15 "class EscrowFinish" xrpl/models/transactions/escrow_finish.py
Length of output: 5682
|
|
||
| from xrpl.models.exceptions import XRPLModelException | ||
|
|
||
| HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*") |
There was a problem hiding this comment.
How is the hex significantly different than what's in xrpl/constants.py? It looks like yours allows for empty string which doesn't look correct.
|
@ckeshava You'll need to make the same |
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
xrpl/models/utils.py (2)
13-13: Mark constant as Final.Use
Finaltype annotation to ensure immutability:-MAX_CREDENTIAL_ARRAY_LENGTH = 8 +MAX_CREDENTIAL_ARRAY_LENGTH: Final[int] = 8
34-39: Standardize error messages.Consider standardizing error messages for consistency:
- errors.append("cannot be an empty string.") + errors.append("Cannot be empty.") - errors.append(f"Length cannot exceed {_MAX_CREDENTIAL_LENGTH}.") + errors.append(f"Length must not exceed {_MAX_CREDENTIAL_LENGTH} characters.") - errors.append("credential_type field must be encoded in hex.") + errors.append("Must be a valid hexadecimal string.")xrpl/models/transactions/credential_create.py (2)
18-18: Add documentation for the _MAX_URI_LENGTH constantConsider adding a docstring to explain the purpose and constraints of this constant.
_MAX_URI_LENGTH = 256 +"""Maximum allowed length in characters for the URI field in a credential."""
70-82: Improve error message consistencyConsider defining constant error messages and making them more descriptive:
+ # Error message constants + _ERR_EMPTY = "URI cannot be empty" + _ERR_TOO_LONG = f"URI length cannot exceed {_MAX_URI_LENGTH} characters" + _ERR_NOT_HEX = "URI must be hex-encoded (required for Blob serialization)" + def _get_uri_error(self: Self) -> Optional[str]: if self.uri is None: return None errors = [] if len(self.uri) == 0: - errors.append("cannot be an empty string.") + errors.append(self._ERR_EMPTY) elif len(self.uri) > _MAX_URI_LENGTH: - errors.append(f"Length cannot exceed {_MAX_URI_LENGTH} characters.") + errors.append(self._ERR_TOO_LONG) if not HEX_REGEX.fullmatch(self.uri): - errors.append("Must be encoded in hex.") + errors.append(self._ERR_NOT_HEX) return " ".join(errors) if errors else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
xrpl/models/transactions/credential_accept.py(1 hunks)xrpl/models/transactions/credential_create.py(1 hunks)xrpl/models/transactions/credential_delete.py(1 hunks)xrpl/models/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- xrpl/models/transactions/credential_delete.py
- xrpl/models/transactions/credential_accept.py
🧰 Additional context used
📓 Learnings (1)
xrpl/models/transactions/credential_create.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_create.py:61-71
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In `xrpl/models/transactions/credential_create.py`, the `uri` field in `CredentialCreate` is stored as a `Blob` in `rippled` and must be base-16 encoded as per the serialization requirements.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-11-12T12:37:49.114Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
🔇 Additional comments (5)
xrpl/models/utils.py (2)
11-11: 🛠️ Refactor suggestion
Use stricter hex validation pattern.
The current pattern allows empty strings. Consider using [a-fA-F0-9]+ to require at least one character.
-HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*")
+HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]+")Likely invalid or redundant comment.
43-68:
Add individual credential validations.
The function should validate each credential in the list:
- Format using HEX_REGEX
- Length against _MAX_CREDENTIAL_LENGTH
if len(credential_list) != len(set(credential_list)):
errors["credential_ids_duplicates"] = (
"CredentialIDs list cannot contain duplicate values."
)
+ for i, cred in enumerate(credential_list):
+ if not HEX_REGEX.fullmatch(cred):
+ errors[f"credential_ids[{i}]"] = "Must be a valid hexadecimal string."
+ elif len(cred) > _MAX_CREDENTIAL_LENGTH:
+ errors[f"credential_ids[{i}]"] = f"Length must not exceed {_MAX_CREDENTIAL_LENGTH} characters."
+
return errorsLikely invalid or redundant comment.
xrpl/models/transactions/credential_create.py (3)
21-56: Well-structured class implementation!
The class definition follows best practices with:
- Proper use of dataclass decorators
- Clear field documentation
- Correct type annotations
- Appropriate use of REQUIRED sentinel
57-69: Clean error aggregation implementation!
The error handling logic is well-structured and properly extends the parent class functionality.
1-82: Verify integration with other credential transactions
The implementation looks solid, but let's verify the consistency of error handling across all credential-related transactions.
✅ Verification successful
Error handling and hex validation are consistent across credential transactions
The verification shows that:
- The error handling pattern in
CredentialCreatefollows the SDK's standard approach, using_get_errors()and specific error methods like_get_uri_error(). - The hex validation using
HEX_REGEXis consistently implemented across the codebase, including in credential-related transactions. - The credential type validation is centralized in
utils.pyand reused across credential transactions. - The error handling structure matches other transaction types in the SDK (e.g., NFToken, AMM transactions).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check consistency of error handling across credential transactions
# Expected: Similar error handling patterns in related files
# Find all credential-related transaction files
echo "Checking credential transaction files..."
fd "credential.*\.py$" --type f
# Check error handling patterns
echo -e "\nChecking error handling patterns..."
rg -A 3 "_get_.*_error" --type py
# Check hex validation consistency
echo -e "\nChecking hex validation patterns..."
rg "HEX_REGEX" --type py
Length of output: 14313
|
@khancode the said amendment is already present in the rippled.cfg file: https://github.com/ckeshava/xrpl-py/blob/cred/.ci-config/rippled.cfg#L186 In fact, it is present in the |
That's fixAMMv1_1 which should be part of v2.2.0. fixAMMv1_2 is the newest one for v2.3.0. |
Thanks. I've included the recent amendments in aeabe63 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
xrpl/models/transactions/credential_create.py (3)
18-18: Add documentation for the _MAX_URI_LENGTH constantConsider adding a docstring to explain the purpose and source of this limit.
_MAX_URI_LENGTH = 256 +"""Maximum allowed length for the URI field in characters, as defined by the XLS-0070d standard."""
74-82: Improve error message consistency and add constant for minimum length
- Error messages should follow a consistent style pattern
- Consider extracting the minimum length requirement to a constant
+_MIN_URI_LENGTH = 1 +"""Minimum allowed length for the URI field in characters.""" + def _get_uri_error(self: Self) -> Optional[str]: if self.uri is None: return None errors = [] - if len(self.uri) == 0: - errors.append("cannot be an empty string.") - elif len(self.uri) > _MAX_URI_LENGTH: - errors.append(f"Length cannot exceed {_MAX_URI_LENGTH} characters.") + if len(self.uri) < _MIN_URI_LENGTH: + errors.append(f"Length must be at least {_MIN_URI_LENGTH} character(s).") + elif len(self.uri) > _MAX_URI_LENGTH: + errors.append(f"Length must not exceed {_MAX_URI_LENGTH} characters.") if not HEX_REGEX.fullmatch(self.uri): - errors.append("Must be encoded in hex.") + errors.append("Value must be encoded in hexadecimal format.") return " ".join(errors) if errors else None
46-50: Enhance URI field documentation to explain hex encoding requirementThe URI field documentation should explain that the value must be hex-encoded because it's stored as a Blob type in rippled.
uri: Optional[str] = None """ Additional data about the credential (such as a link to the Verifiable - Credential document). + Credential document). The value must be hex-encoded as it is stored + as a Blob type in the XRPL ledger. """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci-config/rippled.cfg(1 hunks)xrpl/models/transactions/credential_create.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
xrpl/models/transactions/credential_create.py (2)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_create.py:61-71
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In `xrpl/models/transactions/credential_create.py`, the `uri` field in `CredentialCreate` is stored as a `Blob` in `rippled` and must be base-16 encoded as per the serialization requirements.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-11-12T12:37:49.114Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
🔇 Additional comments (4)
.ci-config/rippled.cfg (2)
Line range hint 1-191: LGTM! Configuration is properly set up for testing
The configuration file is well-structured with appropriate settings for testing the Credentials feature:
- Network ports and protocols are correctly configured
- Required amendments including Credentials are enabled
- Database and logging settings are suitable for testing environment
187-189: Verify the necessity of AMM-related amendments
The PR objectives focus on implementing the Credentials standard, but these changes add AMM-related amendments. While having additional amendments won't break anything, we should verify if they are required for the integration tests.
✅ Verification successful
AMM-related amendments are necessary for the test suite
The presence of extensive AMM-related test files (both unit and integration) in the codebase indicates that these amendments are required for proper testing functionality. The test suite includes AMM-specific features like AMM Create, Deposit, Withdraw, Vote, and Bid operations, which likely depend on these amendments being enabled in the rippled configuration.
- Found multiple AMM test files:
tests/integration/transactions/test_amm_*.pytests/unit/models/transactions/test_amm_*.py
- The amendments are correctly configured in
.ci-config/rippled.cfg
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new amendments are referenced in the test files
# Search for references to the new amendments in test files
echo "Searching for references to new amendments in test files..."
rg -i "fixAMMv1_2|AMMClawback|InvariantsV1_1" "*test*"
# Check if these amendments are enabled in the rippled version used for testing
echo "Checking rippled version compatibility..."
curl -s -X POST -H "Content-Type: application/json" -d '{"method": "feature", "params": [{"feature": "fixAMMv1_2"}]}' http://0.0.0.0:5005/
Length of output: 466
Script:
#!/bin/bash
# Search for test files and check for amendment references
echo "Searching for test files containing amendment references..."
fd -t f "test" | xargs rg -i "fixAMMv1_2|AMMClawback|InvariantsV1_1"
# Check if these amendments are used in AMM-related test files
echo "Searching for AMM-related test files..."
fd -t f "test|spec" | xargs rg -i "amm.*test"
# Check the rippled configuration for other amendment references
echo "Checking rippled configuration..."
rg -i "fixAMMv1_2|AMMClawback|InvariantsV1_1" .ci-config/
Length of output: 1802
xrpl/models/transactions/credential_create.py (2)
21-56: LGTM! Well-structured class definition with comprehensive documentation
The class follows SDK patterns with appropriate use of decorators, field types, and documentation.
1-82: Verify integration with credential system components
The implementation looks solid, but let's verify the integration with other credential-related components.
✅ Verification successful
Integration with credential system components verified successfully
The implementation is properly integrated with the credential system:
- All three credential transaction types (Create/Accept/Delete) consistently inherit from the Transaction base class
- Credential type validation is uniformly implemented across all credential transactions using the shared
get_credential_type_errorutility - Transaction types are properly defined in the TransactionType enum
- URI validation is unique to CredentialCreate and has appropriate length constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation across credential transaction types
# and check for potential integration issues.
# Check for consistent error handling patterns across credential transactions
echo "Checking error handling patterns..."
rg -A 3 "_get_.*_error" "xrpl/models/transactions/credential_*.py"
# Verify credential type validation consistency
echo "Checking credential type validation..."
rg -A 5 "get_credential_type_error" "xrpl/models/transactions/"
# Look for potential URI validation inconsistencies
echo "Checking URI validation patterns..."
rg -A 3 "_MAX_URI_LENGTH" "xrpl/models/transactions/"
Length of output: 4182
Script:
#!/bin/bash
# Let's verify the complete credential system integration
# Check for all credential-related transaction types
echo "Checking all credential transaction types..."
fd "credential_.*\.py$" xrpl/models/transactions/
# Verify consistent imports and base class usage
echo "Checking base class and imports..."
rg -l "class.*\(Transaction\)" xrpl/models/transactions/credential_*.py
# Check transaction type enum values
echo "Checking transaction type definitions..."
rg -A 5 "TransactionType.*CREDENTIAL" xrpl/models/
# Verify credential type validation utility
echo "Checking credential type validation implementation..."
fd "utils\.py$" xrpl/models/ --exec rg -A 5 "get_credential_type_error"
Length of output: 2536
| def _validate_credentials_length( | ||
| credentials: List[Credential], field_name: str | ||
| ) -> None: | ||
| if len(credentials) == 0: |
There was a problem hiding this comment.
If you add a check like this at the top, you can remove the ifs below when you call this helper function
if credentials is None:
return
There was a problem hiding this comment.
thanks, I've incorporated in cd7f860
Co-authored-by: Mayukha Vadari <[email protected]>
Co-authored-by: Mayukha Vadari <[email protected]>
|
thank you all for reviewing this PR |
High Level Overview of Change
Python Client SDK implementation for https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0070d-credentials
Corresponding c++ changes: https://github.com/XRPLF/rippled/pull/5103/files
Context of Change
Following changes are introduced in this PR:
(Quoting Verbatim from the XLS specification document)
Type of Change
Did you update CHANGELOG.md?
Test Plan
Note: The integration tests will not pass the CI because we don't have an appropriate docker container with the custom branch. At the moment, integration tests are to be run with custom builds of rippled.