Verifiable 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.
WalkthroughThe pull request introduces significant enhancements to the XRPL framework, focusing on credential management. It includes updates to configuration files, new transaction types, and expanded test coverage for various credential-related transactions. Key changes involve the addition of features in the configuration, new models for credential transactions (create, accept, delete), and comprehensive unit and integration tests to validate these functionalities. The updates ensure that the system can handle credential transactions effectively while maintaining backward compatibility. Changes
Possibly related PRs
Suggested reviewers
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>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
tests/integration/transactions/test_credential.py (5)
15-15: Consider using a more descriptive constant name.The constant
_URIcould be more descriptive, e.g.,SAMPLE_CREDENTIAL_URIorTEST_CREDENTIAL_URI, to better indicate its purpose in tests.
18-41: Enhance docstring with preconditions.The docstring should mention that the function assumes the presence of required fields (
Issuer,Subject,CredentialType) in the account objects.def is_cred_object_present( result: dict, issuer: str, subject: str, cred_type: str ) -> bool: """ + Check if a specific credential object exists in the account objects. + Args: result: JSON response from account_objects RPC issuer: Address of the credential issuer subject: Address of the credential subject cred_type: Type of the credential Returns: bool: True if credential exists, False otherwise + + Preconditions: + - The account objects in result must contain 'Issuer', 'Subject', + and 'CredentialType' fields """
50-53: Make credential type disambiguation more explicit.The comment explains the purpose but the implementation could be more explicit. Consider using an enum or predefined test cases.
- # Disambiguate the sync/async, json/websocket tests with different - # credential type values -- this avoids tecDUPLICATE error - # self.value is defined inside the above decorator - cred_type = str_to_hex("Passport_" + str(self.value)) + # Each test run needs a unique credential type to avoid tecDUPLICATE error + TEST_CREDENTIAL_TYPES = { + "sync_json": "Passport_Sync_JSON", + "sync_websocket": "Passport_Sync_WS", + "async_json": "Passport_Async_JSON", + "async_websocket": "Passport_Async_WS", + } + cred_type = str_to_hex(TEST_CREDENTIAL_TYPES[self.value])
76-81: Standardize transaction execution comments.The comments for transaction execution could follow a more consistent format.
- # Execute the CredentialAccept transaction on the above Credential ledger object - tx = CredentialAccept( - issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type - ) - # CredentialAccept transaction is submitted by SUBJECT - response = await sign_and_reliable_submission_async(tx, DESTINATION, client) + # Step 2: Accept credential (submitted by SUBJECT) + tx = CredentialAccept( + issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type + ) + response = await sign_and_reliable_submission_async(tx, DESTINATION, client) - # Execute the CredentialDelete transaction - # Subject initiates the deletion of the Credential ledger object - tx = CredentialDelete( - issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type - ) + # Step 3: Delete credential (initiated by SUBJECT) + tx = CredentialDelete( + issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type + )Also applies to: 86-91
95-121: Simplify credential deletion verification.Based on previous feedback, checking just one directory is sufficient since we're testing the library, not rippled functionality.
- # The credential ledger object must be deleted from both the Issuer and Subject - # account's directory pages - account_objects_response = await client.request( - AccountObjects(account=_ISSUER, type=AccountObjectType.CREDENTIAL) - ) - self.assertFalse( - is_cred_object_present( - account_objects_response.result, - issuer=_ISSUER, - subject=_SUBJECT, - cred_type=cred_type, - ) - ) - - # Verify that the Credential object has been deleted from the Subject's - # directory page as well + # Verify credential deletion using ledger_entry + with self.assertRaises(Exception): # Specific exception type from the library + await client.request( + LedgerEntry( + credential=Credential( + subject=_SUBJECT, + issuer=_ISSUER, + credential_type=cred_type + ) + ) + )tests/unit/models/requests/test_ledger_entry.py (1)
21-27: Enhance test coverage and documentationWhile the basic test is functional, consider these improvements:
- Add docstring explaining the test purpose and credential ID format
- Add assertions to verify the credential value was correctly assigned
- Include negative test cases (e.g., invalid credential ID format)
Example enhancement:
def test_query_credential_object_id(self): + """ + Test LedgerEntry initialization with a valid credential ID string. + Credential ID format: 32-byte hexadecimal string + """ + credential_id = "EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A" + ledger_entry = LedgerEntry(credential=credential_id) + + self.assertTrue(ledger_entry.is_valid()) + self.assertEqual(ledger_entry.credential, credential_id) + + # Negative test case + with self.assertRaises(XRPLModelException): + LedgerEntry(credential="invalid_credential_id")xrpl/models/requests/ledger_entry.py (2)
47-64: Enhance field documentationWhile the class structure is well-implemented, consider enhancing the field docstrings with:
- Format requirements for account addresses
- Allowed/expected values for credential_type
- Examples of valid values
61-61: Consider adding credential_type validationThe
credential_typefield accepts any string value. Consider adding validation to ensure only valid credential types are accepted, as per the XLS-0070d standard.Example implementation:
from typing import ClassVar from typing_extensions import Literal @require_kwargs_on_init @dataclass(frozen=True, **KW_ONLY_DATACLASS) class Credential(BaseModel): VALID_TYPES: ClassVar[tuple[str, ...]] = ("type1", "type2") # add valid types from XLS-0070d credential_type: str = REQUIRED # type: ignore def _get_errors(self) -> Dict[str, str]: errors = super()._get_errors() if self.credential_type not in self.VALID_TYPES: errors["credential_type"] = f"Must be one of: {', '.join(self.VALID_TYPES)}" return errors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/integration_test.yml(3 hunks)tests/integration/transactions/test_credential.py(1 hunks)tests/unit/models/requests/test_ledger_entry.py(2 hunks)xrpl/models/requests/ledger_entry.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration_test.yml
🧰 Additional context used
📓 Learnings (1)
tests/integration/transactions/test_credential.py (4)
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-12T12:37:49.114Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:42-44
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In this codebase, extensive unit tests are located in `rippled`, and additional tests are not required in this library.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:37-108
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In integration tests, setup and teardown are performed using the `CredentialCreate` and `CredentialDelete` transactions, and we prefer not to abstract these transactions into separate utility functions.
🔇 Additional comments (3)
tests/unit/models/requests/test_ledger_entry.py (1)
5-5: LGTM: Import changes are well-organized
The addition of Credential to the imports follows the existing pattern and is appropriately grouped with related model imports.
xrpl/models/requests/ledger_entry.py (2)
31-31: LGTM: Enum addition follows conventions
The new CREDENTIAL enum value is correctly placed and follows the established naming pattern.
267-267: LGTM: LedgerEntry changes are consistent
The credential field addition and its integration into the validation logic follow the established patterns and maintain the class's invariants.
Also applies to: 298-298
| def test_query_credential_by_object_params(self): | ||
| self.assertTrue( | ||
| LedgerEntry( | ||
| credential=Credential( | ||
| subject="rSubject", issuer="rIssuer", credential_type="ABCDE" | ||
| ) | ||
| ).is_valid() | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen credential object parameter testing
The test should be more comprehensive to ensure robust credential validation:
- Add docstring documenting parameter requirements
- Verify all object properties after construction
- Add negative test cases for invalid parameters
- Use realistic test data that matches the XLS-0070d standard
Suggested implementation:
def test_query_credential_by_object_params(self):
+ """
+ Test LedgerEntry initialization with a Credential object.
+
+ Requirements:
+ - subject: valid XRPL account address
+ - issuer: valid XRPL account address
+ - credential_type: string matching XLS-0070d standard types
+ """
+ subject = "rN7n7otQDd6FczFgLdSqtcsAUxDkw6fzRH"
+ issuer = "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK"
+ credential_type = "KYC_CREDENTIAL"
+
+ ledger_entry = LedgerEntry(
+ credential=Credential(
+ subject=subject,
+ issuer=issuer,
+ credential_type=credential_type
+ )
+ )
+
+ # Verify valid case
+ self.assertTrue(ledger_entry.is_valid())
+ self.assertEqual(ledger_entry.credential.subject, subject)
+ self.assertEqual(ledger_entry.credential.issuer, issuer)
+ self.assertEqual(ledger_entry.credential.credential_type, credential_type)
+
+ # Negative test cases
+ with self.assertRaises(XRPLModelException):
+ LedgerEntry(
+ credential=Credential(
+ subject="invalid_address",
+ issuer=issuer,
+ credential_type=credential_type
+ )
+ )
+
+ with self.assertRaises(XRPLModelException):
+ LedgerEntry(
+ credential=Credential(
+ subject=subject,
+ issuer="invalid_address",
+ credential_type=credential_type
+ )
+ )Committable suggestion skipped: line range outside the PR's diff.
| def test_query_credential_object_id(self): | ||
| self.assertTrue( | ||
| LedgerEntry( | ||
| credential="EA85602C1B41F6F1F5E83C0E6B87142FB8957B" | ||
| "D209469E4CC347BA2D0C26F66A" | ||
| ).is_valid() | ||
| ) | ||
|
|
||
| def test_query_credential_by_object_params(self): | ||
| self.assertTrue( | ||
| LedgerEntry( | ||
| credential=Credential( | ||
| subject="rSubject", issuer="rIssuer", credential_type="ABCDE" | ||
| ) | ||
| ).is_valid() | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Additional credential test scenarios needed
The suggested credential scenarios are not fully covered in existing tests. Consider adding the following tests in tests/unit/models/requests/test_ledger_entry.py:
- Test combining credential with other LedgerEntry fields (should be invalid)
- Test credential field mutations after object creation
- Test serialization/deserialization of credential objects
🔗 Analysis chain
Consider additional test scenarios
While the basic credential functionality is tested, consider adding these scenarios:
- Test combining credential with other LedgerEntry fields (should be invalid)
- Test credential field mutations after object creation
- Test serialization/deserialization of credential objects
Let's check if these scenarios are covered elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other credential-related tests
rg -l "credential.*test" tests/
Length of output: 33
Script:
#!/bin/bash
# Comprehensive search for any credential-related tests
rg "credential" tests/
Length of output: 12591
| @require_kwargs_on_init | ||
| @dataclass(frozen=True, **KW_ONLY_DATACLASS) | ||
| class Credential(BaseModel): | ||
| """Specify the Credential to retrieve. If a string, must be the ledger entry ID of | ||
| the entry, as hexadecimal. If an object, requires subject, issuer, and | ||
| credential_type sub-fields. | ||
| """ | ||
|
|
||
| subject: str = REQUIRED # type: ignore | ||
| """The account that is the subject of the credential.""" | ||
|
|
||
| issuer: str = REQUIRED # type: ignore | ||
| """The account that issued the credential.""" | ||
|
|
||
| credential_type: str = REQUIRED # type: ignore | ||
| """The type of the credential, as issued.""" | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Credential Implementation Lacks XLS-0070d Compliance
The Credential class does not reference or implement the XLS-0070d standard requirements.
🔗 Analysis chain
Verify compliance with XLS-0070d standard
Let's verify that the Credential implementation aligns with the XLS-0070d standard requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if implementation matches XLS-0070d standard
# Search for XLS-0070d documentation or implementation references
rg -i "xls-0070d|xls 0070d" -A 5
# Look for credential-related test files
fd -e py "test.*credential"
# Search for credential type definitions
rg -i "credential.*type.*=|CREDENTIAL_TYPE" -A 3
Length of output: 21132
There was a problem hiding this comment.
I don't understand what this means
There was a problem hiding this comment.
@ckeshava, apologies for the confusion. My previous comment was regarding the Credential class defined in xrpl/models/requests/ledger_entry.py. I noticed that while the PR aims to implement functionalities according to the XLS-0070d credentials standard, the Credential class doesn't seem to reference or fully implement the requirements specified in that standard.
To ensure compliance with XLS-0070d, could you verify if the Credential class includes all the necessary fields, data structures, and validation rules outlined in the specification? If it already does, adding comments or documentation that reference the standard could help clarify the implementation and assist future maintainers.
Please let me know if you need further clarification or assistance.
| if ( | ||
| self.authorize is None | ||
| and self.unauthorize is None | ||
| and self.authorize_credentials is None | ||
| and self.unauthorize_credentials is None | ||
| ): | ||
| errors["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." | ||
| ) | ||
|
|
||
| # Filter for input parameters which are provided in the transaction | ||
| # Optional fields in the transaction model are set to None | ||
| params = [ | ||
| self.authorize, | ||
| self.unauthorize, | ||
| self.authorize_credentials, | ||
| self.unauthorize_credentials, | ||
| ] | ||
| if sum(param is not None for param in params) > 1: | ||
| errors["DepositPreauth"] = ( | ||
| "DepositPreauth txn accepts exactly one input amongst authorize, " | ||
| + "unauthorize, authorize_credentials and unauthorize_credentials." |
There was a problem hiding this comment.
Why do we need both of these checks? Could we just have the second one and ensure exactly 1 is specified, and make the error message more generic to support this?
There was a problem hiding this comment.
Yeah, that is also an acceptable solution. I personally prefer to have separate error messages for different error cases.
| f"{field_name} list cannot contain duplicate credentials." | ||
| ) | ||
|
|
||
| # Then replace the checks with: |
There was a problem hiding this comment.
nit: this comment could be deleted
|
@mDuo13 thank you. By Credential faucet, are you referring to tutorials/example-snippets ? I discovered an unpleasant fact just now. The This does not hamper the usage of both the names, thanks to import aliasing. But if you guys feel strongly, I can re-name the I don't have the liberty to change the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
xrpl/models/transactions/deposit_preauth.py (1)
58-82: Consider consolidating parameter validation checks.The validation logic correctly ensures exactly one parameter is set, but maintains separate checks for clarity. While this approach works, it could be simplified to a single check with a more generic error message, as suggested in the past review comments.
- if ( - self.authorize is None - and self.unauthorize is None - and self.authorize_credentials is None - and self.unauthorize_credentials is None - ): - errors["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." - ) - - params = [ - self.authorize, - self.unauthorize, - self.authorize_credentials, - self.unauthorize_credentials, - ] - if sum(param is not None for param in params) > 1: - errors["DepositPreauth"] = ( - "DepositPreauth txn accepts exactly one input amongst authorize, " - + "unauthorize, authorize_credentials and unauthorize_credentials." - ) + params = [ + self.authorize, + self.unauthorize, + self.authorize_credentials, + self.unauthorize_credentials, + ] + set_params = sum(param is not None for param in params) + if set_params != 1: + errors["DepositPreauth"] = ( + "DepositPreauth transaction requires exactly one parameter among " + + "authorize, unauthorize, authorize_credentials, or unauthorize_credentials." + )CONTRIBUTING.md (1)
101-105: Improve documentation formatting and clarityThe command breakdown section needs formatting consistency and clarity improvements:
- Use consistent list formatting (dashes instead of asterisks)
- Add missing comma after 'Hence'
- Clarify volume mount explanation
Apply these changes:
-* `-it` allows you to interact with the container. -* `-d` runs the docker container in detached mode. The container will run in the background and developer gets back control of the terminal -* `-t` starts a terminal in the container for you to send commands to. -* `--volume $PWD/.ci-config:/etc/opt/ripple/` mounts the directories as indicated. It must be an absolute path, so we use `$PWD` instead of `./`. `rippled` software searches the location `/etc/opt/ripple/` (default behavior) for the config files. Hence there is no need to explicitly specify the config-file path. -* `rippleci/rippled:develop` is an image that is regularly updated with the latest build of the `develop` branch of `rippled`. +- `-it` combines interactive mode (`-i`) and pseudo-TTY allocation (`-t`) for container interaction +- `-d` runs the container in detached mode (background) +- `-t` allocates a pseudo-TTY for command execution +- `--volume $PWD/.ci-config:/opt/ripple/etc/` mounts the local config directory to the container's expected config path. We use `$PWD` for absolute paths. Hence, there is no need to explicitly specify the config-file path +- `rippleci/rippled:2.2.0-b3` specifies the stable version of the rippled image🧰 Tools
🪛 LanguageTool
[uncategorized] ~104-~104: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...default behavior) for the config files. Hence there is no need to explicitly specify ...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint
101-101: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
102-102: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
103-103: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
104-104: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
105-105: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/integration_test.yml(3 hunks)CONTRIBUTING.md(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 (2)
- .github/workflows/integration_test.yml
- xrpl/models/transactions/credential_delete.py
🧰 Additional context used
📓 Learnings (1)
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.
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~104-~104: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...default behavior) for the config files. Hence there is no need to explicitly specify ...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint
CONTRIBUTING.md
100-100: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
101-101: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
102-102: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
103-103: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
104-104: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
105-105: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
🔇 Additional comments (2)
xrpl/models/transactions/deposit_preauth.py (2)
44-49: LGTM! Well-documented credential fields.
The new credential fields are properly typed and documented, clearly explaining their purpose in the preauthorization flow.
84-107: LGTM! Comprehensive credential list validation.
The validation logic for credential lists is thorough and handles all edge cases:
- Empty list check
- Maximum size limit (8 elements)
- Duplicate detection
| @require_kwargs_on_init | ||
| @dataclass(frozen=True, **KW_ONLY_DATACLASS) | ||
| class Credential(NestedModel): | ||
| """ | ||
| An inner object representing individual element inside AuthorizeCredentials and | ||
| UnauthorizeCredentials array. | ||
| """ | ||
|
|
||
| issuer: str = REQUIRED # type: ignore | ||
| """The issuer of the credential.""" | ||
|
|
||
| credential_type: str = REQUIRED # type: ignore | ||
| """A (hex-encoded) value to identify the type of credential from the issuer.""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation for Credential class.
The class structure looks good, but it's missing validation for the fields:
issuershould be validated as a valid XRPL accountcredential_typeshould be validated as a hex-encoded 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_xrpl_account(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 errorsAdd these imports at the top:
from xrpl.core.addresscodec import is_valid_xrpl_account
from xrpl.utils import is_hex_string
CONTRIBUTING.md
Outdated
| docker run -dit -p 5005:5005 -p 6006:6006 -v $PWD/.ci-config/:/etc/opt/ripple/ --name rippled_standalone --entrypoint bash rippleci/rippled:develop | ||
| docker exec -d rippled_standalone rippled -a |
There was a problem hiding this comment.
Docker commands need revision for consistency and robustness
The new Docker commands have several issues that need to be addressed:
- The volume mount path (
/etc/opt/ripple/) differs from what's described in the command breakdown section (/opt/ripple/etc/). - Using a hardcoded container name (
rippled_standalone) could cause conflicts in multi-user environments. - Missing cleanup instructions for the detached container.
- Inconsistent image tags between old and new commands (
developvs2.2.0-b3).
Consider this revised version:
-docker run -dit -p 5005:5005 -p 6006:6006 -v $PWD/.ci-config/:/etc/opt/ripple/ --name rippled_standalone --entrypoint bash rippleci/rippled:develop
-docker exec -d rippled_standalone rippled -a
+# Start the rippled container
+CONTAINER_ID=$(docker run -dit \
+ -p 5005:5005 -p 6006:6006 \
+ -v "$PWD/.ci-config/":/opt/ripple/etc/ \
+ --rm \
+ rippleci/rippled:2.2.0-b3)
+
+# Start rippled in standalone mode
+docker exec -d "$CONTAINER_ID" rippled -a
+
+# Note: To stop the container when done:
+# docker stop "$CONTAINER_ID"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| docker run -dit -p 5005:5005 -p 6006:6006 -v $PWD/.ci-config/:/etc/opt/ripple/ --name rippled_standalone --entrypoint bash rippleci/rippled:develop | |
| docker exec -d rippled_standalone rippled -a | |
| # Start the rippled container | |
| CONTAINER_ID=$(docker run -dit \ | |
| -p 5005:5005 -p 6006:6006 \ | |
| -v "$PWD/.ci-config/":/opt/ripple/etc/ \ | |
| --rm \ | |
| rippleci/rippled:2.2.0-b3) | |
| # Start rippled in standalone mode | |
| docker exec -d "$CONTAINER_ID" rippled -a | |
| # Note: To stop the container when done: | |
| # docker stop "$CONTAINER_ID" |
update contributing instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CONTRIBUTING.md (1)
98-102: Improve command explanation accuracy and formattingThe command explanation section needs some improvements for consistency and completeness:
- Use consistent list formatting (dashes instead of asterisks)
- Add missing explanations for
--entrypointand-cflags- Add comma after "Hence" for better readability
Consider this revision:
-* `-it` allows you to interact with the container. -* `-d` runs the docker container in detached mode. The container will run in the background and developer gets back control of the terminal -* `-t` starts a terminal in the container for you to send commands to. -* `--volume $PWD/.ci-config:/etc/opt/ripple/` mounts the directories as indicated. It must be an absolute path, so we use `$PWD` instead of `./`. `rippled` software searches the location `/etc/opt/ripple/` (default behavior) for the config files. Hence there is no need to explicitly specify the config-file path. -* `rippleci/rippled:develop` is an image that is regularly updated with the latest build of the `develop` branch of `rippled`. -* `-a` starts `rippled` in standalone mode +- `-it` enables interactive mode with a TTY for container interaction +- `-d` runs the container in detached mode, returning control to the terminal +- `--volume $PWD/.ci-config:/etc/opt/ripple/` mounts the config directory. Uses absolute path with `$PWD`. The `rippled` software searches `/etc/opt/ripple/` (default behavior) for config files. Hence, there is no need to explicitly specify the config-file path +- `--entrypoint` overrides the default entrypoint to use a shell +- `-c` passes the command to start rippled in the container +- `rippleci/rippled:develop` is the latest build from the `develop` branch +- `-a` starts `rippled` in standalone mode🧰 Tools
🪛 LanguageTool
[uncategorized] ~101-~101: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...default behavior) for the config files. Hence there is no need to explicitly specify ...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint
98-98: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
99-99: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
100-100: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
101-101: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
102-102: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/integration_test.yml(2 hunks)CONTRIBUTING.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration_test.yml
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~101-~101: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...default behavior) for the config files. Hence there is no need to explicitly specify ...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint
CONTRIBUTING.md
97-97: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
98-98: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
99-99: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
100-100: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
101-101: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
102-102: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
| @@ -90,17 +90,17 @@ poetry run poe test_unit | |||
| To run integration tests, you'll need a standalone rippled node running with WS port `6006` and JSON RPC port `5005`. You can run a docker container for this: | |||
|
|
|||
| ```bash | |||
| docker run -p 5005:5005 -p 6006:6006 --interactive -t --volume $PWD/.ci-config:/opt/ripple/etc/ --platform linux/amd64 rippleci/rippled:2.2.0-b3 /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg | |||
| docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Docker command robustness and security
The current Docker command could benefit from additional security and reliability improvements:
- Add user specification to avoid running as root
- Include health check to ensure rippled is ready
- Add resource limits to prevent container from consuming excessive resources
- Add automatic cleanup with
--rmflag
Consider this improved version:
-docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a'
+# Start the rippled container with improved security and reliability
+docker run -dit \
+ --rm \
+ --user "$(id -u):$(id -g)" \
+ --memory="4g" \
+ --memory-swap="4g" \
+ --cpu-shares=1024 \
+ -p 5005:5005 -p 6006:6006 \
+ --volume "$PWD/.ci-config/":/etc/opt/ripple/:ro \
+ --health-cmd="rippled server_info" \
+ --health-interval=5s \
+ --health-retries=3 \
+ rippleci/rippled:develop \
+ sh -c 'rippled -a && while true; do sleep 30; done'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a' | |
| # Start the rippled container with improved security and reliability | |
| docker run -dit \ | |
| --rm \ | |
| --user "$(id -u):$(id -g)" \ | |
| --memory="4g" \ | |
| --memory-swap="4g" \ | |
| --cpu-shares=1024 \ | |
| -p 5005:5005 -p 6006:6006 \ | |
| --volume "$PWD/.ci-config/":/etc/opt/ripple/:ro \ | |
| --health-cmd="rippled server_info" \ | |
| --health-interval=5s \ | |
| --health-retries=3 \ | |
| rippleci/rippled:develop \ | |
| sh -c 'rippled -a && while true; do sleep 30; done' |
| def _get_credential_type_error(self: Self) -> Optional[str]: | ||
| errors = [] | ||
| if len(self.credential_type) == 0: | ||
| errors.append("Length must be > 0.") | ||
| elif len(self.credential_type) > _MAX_CREDENTIAL_LENGTH: | ||
| errors.append(f"Length must be < {_MAX_CREDENTIAL_LENGTH}.") | ||
| if not bool(HEX_REGEX.fullmatch(self.credential_type)): | ||
| errors.append("credential_type field must be encoded in hex.") | ||
| return " ".join(errors) if errors else None |
There was a problem hiding this comment.
can this method be extracted to utils since its recreated 3 times across the credentials transactions?
There was a problem hiding this comment.
thanks. I've incorporated this suggestion in 09b7d18
Doesn't matter too much, since there are other similar issues all over the codebase (e.g. the |
okay nice, that's a relief |
| @@ -32,7 +32,7 @@ jobs: | |||
|
|
|||
| - name: Run docker in background | |||
| run: | | |||
| docker run --detach --rm --name rippled-service -p 5005:5005 -p 6006:6006 --volume "${{ github.workspace }}/.ci-config/":"/opt/ripple/etc/" --health-cmd="wget localhost:6006 || exit 1" --health-interval=5s --health-retries=10 --health-timeout=2s --env GITHUB_ACTIONS=true --env CI=true ${{ env.RIPPLED_DOCKER_IMAGE }} /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg | |||
There was a problem hiding this comment.
Why all the changes here?
There was a problem hiding this comment.
- I have updated the docker image to use the latest tip from
developbranch - I'm given to understand that there are some updates to the
entrypoint.shbash file in the docker image. In consultation with the @micheal legleux, I've updated thedocker runcommand accordingly.
There was a problem hiding this comment.
lastly, I've updated the mount point of the docker container. We don't need to explicitly specify the conf file anymore, its already placed into the default location that is searched by rippled
| @@ -90,17 +90,17 @@ poetry run poe test_unit | |||
| To run integration tests, you'll need a standalone rippled node running with WS port `6006` and JSON RPC port `5005`. You can run a docker container for this: | |||
|
|
|||
| ```bash | |||
| docker run -p 5005:5005 -p 6006:6006 --interactive -t --volume $PWD/.ci-config:/opt/ripple/etc/ --platform linux/amd64 rippleci/rippled:2.2.0-b3 /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg | |||
| docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a' | |||
There was a problem hiding this comment.
This no longer matches above.
There was a problem hiding this comment.
yes, this is an intentional change. please check my comment on the integration_test.yml CI/CD file
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
xrpl/models/transactions/credential_accept.py (1)
21-24: Consider enhancing documentation with examples and specification reference.The docstring could be more helpful by including:
- A code example showing how to create and submit this transaction
- A reference to the XLS-0070d credentials standard
xrpl/models/transactions/credential_create.py (4)
14-14: Consider defining _MAX_CREDENTIAL_LENGTH constant locallyThe
_get_credential_type_errorutility is imported but its corresponding length constant is not defined in this file. For better maintainability and to keep related constants together, consider defining_MAX_CREDENTIAL_LENGTHalongside_MAX_URI_LENGTH._MAX_URI_LENGTH = 256 +_MAX_CREDENTIAL_LENGTH = 128 # Add this constant if not defined elsewhereAlso applies to: 18-18
24-24: Enhance class docstring with more detailsThe current docstring could be more informative about the Credential object being created and its purpose within the XLS-0070d standard.
Consider expanding the docstring:
- """This transaction creates a Credential object. It must be sent by the issuer.""" + """ + Creates a Credential object on the ledger as specified by XLS-0070d standard. + + This transaction must be sent by the issuer and establishes a new credential + with specified type, optional expiration, and optional URI linking to additional + credential data. + """
31-52: Enhance field documentation with format specificationsThe field docstrings could be more detailed about expected formats and constraints.
Consider updating the docstrings:
account: str = REQUIRED # type: ignore """ - The issuer of the credential. + The account address of the credential issuer in base58 format. """ subject: str = REQUIRED # type: ignore """ - The subject of the credential. + The account address of the credential subject in base58 format. """ credential_type: str = REQUIRED # type: ignore """ - A (hex-encoded) value to identify the type of credential from the issuer. + A hex-encoded string (up to 128 characters) identifying the credential type. """ expiration: Optional[int] = None """ - Optional credential expiration. + Optional Unix timestamp for credential expiration. """ uri: Optional[str] = None """ - Optional additional data about the credential (such as a link to the Verifiable - Credential document). + Optional hex-encoded string (up to 256 characters) containing additional data + about the credential, such as a link to the Verifiable Credential document. """
54-76: Optimize error handling and improve message consistencyThe error handling could be more efficient and consistent.
Consider these improvements:
- Use early returns to reduce nesting
- Make error messages more consistent
- Use constants for error messages
def _get_uri_error(self: Self) -> Optional[str]: if self.uri is None: return None - - errors = [] - if len(self.uri) == 0: - errors.append("Length must be > 0.") - elif len(self.uri) > _MAX_URI_LENGTH: - errors.append(f"Length must be < {_MAX_URI_LENGTH}.") - if not HEX_REGEX.fullmatch(self.uri): - errors.append("Must be encoded in hex.") - return " ".join(errors) if errors else None + + # Early validation for empty URI + if len(self.uri) == 0: + return "URI length must be greater than 0" + + # Validate URI length + if len(self.uri) > _MAX_URI_LENGTH: + return f"URI length must not exceed {_MAX_URI_LENGTH} characters" + + # Validate hex encoding + if not HEX_REGEX.fullmatch(self.uri): + return "URI must be hex-encoded" + + return Nonexrpl/models/utils.py (2)
11-11: Consider making the hex regex more restrictive.The current regex
[a-fA-F0-9]*allows empty strings. Consider using[a-fA-F0-9]+to enforce 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]+")
18-28: Add docstring and improve error messages.The function needs a docstring explaining its purpose and validation rules. Also, consider making error messages more specific.
def _get_credential_type_error(credential_type: str) -> Optional[str]: + """ + Validates a credential type string against format and length constraints. + + Args: + credential_type: The credential type string to validate + + Returns: + Optional[str]: Error message if validation fails, None if validation passes + """ errors = [] # credential_type is a required field in this transaction if len(credential_type) == 0: - errors.append("Length must be > 0.") + errors.append("Credential type cannot be empty.") elif len(credential_type) > _MAX_CREDENTIAL_LENGTH: - errors.append(f"Length must be less than {_MAX_CREDENTIAL_LENGTH}.") + errors.append(f"Credential type length must not exceed {_MAX_CREDENTIAL_LENGTH} characters.") if not HEX_REGEX.fullmatch(credential_type): - errors.append("credential_type field must be encoded in hex.") + errors.append("Credential type must be a valid hexadecimal string.") return " ".join(errors) if len(errors) > 0 else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
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)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 (4)
- tests/unit/models/transactions/test_credential_accept.py
- tests/unit/models/transactions/test_credential_create.py
- tests/unit/models/transactions/test_credential_delete.py
- xrpl/models/transactions/credential_delete.py
🧰 Additional context used
📓 Learnings (2)
xrpl/models/transactions/credential_accept.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-11-12T12:37:49.114Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
xrpl/models/transactions/credential_create.py (1)
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 (7)
xrpl/models/transactions/credential_accept.py (5)
1-20: LGTM! Well-structured class definition with proper imports.
The implementation follows best practices with proper type hints, enforced keyword arguments, and immutable dataclass design.
26-29: LGTM! Correct transaction type field definition.
The field is properly defined as non-initializable with the correct transaction type.
31-43: LGTM! Well-documented required fields.
The fields are properly defined with clear documentation explaining their purpose.
1-51: Verify integration with other credential transactions.
This implementation is part of a larger credential management feature. Let's verify the consistency across related changes.
#!/bin/bash
# Description: Check consistency across credential-related implementations
# Check for similar patterns in other credential transaction types
rg -l "class Credential.*\(" xrpl/models/transactions/
# Check for integration tests covering the full credential lifecycle
rg -l "test.*credential" tests/integration/45-51: Verify the extracted credential type validation.
The error handling looks good, using the extracted _get_credential_type_error function. Let's verify its implementation.
✅ Verification successful
Let me try a different search strategy to locate the credential type validation function.
Credential type validation is properly implemented and used
The _get_credential_type_error function is correctly implemented in xrpl/models/utils.py and performs comprehensive validation:
- Checks for non-empty credential type
- Validates length against
_MAX_CREDENTIAL_LENGTH - Ensures hex encoding using
HEX_REGEX
The function is properly imported and used consistently across all credential-related transactions:
- CredentialAccept
- CredentialCreate
- CredentialDelete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of _get_credential_type_error in utils
# Check the implementation
rg -A 10 "def _get_credential_type_error" xrpl/models/utils.py
# Check for any tests
rg -l "test.*credential.*type.*error" tests/unit/models/
Length of output: 339
Script:
#!/bin/bash
# Search for credential type validation across the codebase
rg -A 5 "credential_type" xrpl/models/
# Check if it's defined in the transaction files themselves
rg -A 5 "_get_credential_type_error" xrpl/models/transactions/
Length of output: 8351
xrpl/models/transactions/credential_create.py (2)
26-29: LGTM!
The transaction type field is correctly implemented following the standard pattern.
1-76: Verify compatibility with C++ implementation
Since this PR is related to changes in the C++ codebase (rippled repository), let's verify the field definitions match between implementations.
| def validate_credential_ids(credential_list: Optional[List[str]]) -> Dict[str, str]: | ||
| """ | ||
| Args: | ||
| credential_list: An optional list of input credentials | ||
|
|
||
| Returns: | ||
| Errors pertaining to credential_ids field | ||
| """ | ||
| errors: Dict[str, str] = {} | ||
| if credential_list is None: | ||
| return errors | ||
|
|
||
| if len(credential_list) == 0: | ||
| errors["credential_ids"] = "CredentialIDs list cannot be empty." | ||
| elif len(credential_list) > 8: | ||
| errors["credential_ids"] = ( | ||
| "CredentialIDs list cannot have more than 8 elements." | ||
| ) | ||
|
|
||
| if len(credential_list) != len(set(credential_list)): | ||
| errors["credential_ids_duplicates"] = ( | ||
| "CredentialIDs list cannot contain duplicate values." | ||
| ) | ||
|
|
||
| return errors |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add individual credential validation and use early returns.
The function should validate each credential's format and length. Also, using early returns would improve readability.
def validate_credential_ids(credential_list: Optional[List[str]]) -> Dict[str, str]:
"""
+ Validates a list of credential IDs against format and length constraints.
+
Args:
credential_list: An optional list of input credentials
Returns:
- Errors pertaining to credential_ids field
+ Dict[str, str]: Dictionary of validation errors, empty if validation passes
"""
errors: Dict[str, str] = {}
if credential_list is None:
return errors
if len(credential_list) == 0:
errors["credential_ids"] = "CredentialIDs list cannot be empty."
- elif len(credential_list) > 8:
- errors["credential_ids"] = (
- "CredentialIDs list cannot have more than 8 elements."
- )
+ return errors
+
+ if len(credential_list) > 8:
+ errors["credential_ids"] = "CredentialIDs list cannot have more than 8 elements."
+ return errors
if len(credential_list) != len(set(credential_list)):
- errors["credential_ids_duplicates"] = (
- "CredentialIDs list cannot contain duplicate values."
- )
+ errors["credential_ids_duplicates"] = "CredentialIDs list cannot contain duplicate values."
+ return errors
+
+ for i, cred in enumerate(credential_list):
+ if not HEX_REGEX.fullmatch(cred):
+ errors[f"credential_ids[{i}]"] = "Credential must be a valid hexadecimal string."
+ continue
+ if len(cred) > _MAX_CREDENTIAL_LENGTH:
+ errors[f"credential_ids[{i}]"] = f"Credential length must not exceed {_MAX_CREDENTIAL_LENGTH} characters."
return errorsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
95-95: Remove unnecessary empty line.This empty line disrupts the changelog format and is inconsistent with the rest of the file.
### BREAKING CHANGE - The default signing algorithm in the `Wallet` was changed from secp256k1 to ed25519
| @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Added | |||
| - Add `include_deleted` to ledger_entry request | |||
| - Add support for XLS-70d Verifiable Credentials in xrpl-py library. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog entry with more details about XLS-70d implementation.
The current entry could be more detailed given the significance of this feature. Consider expanding it to include:
- New transaction types (CredentialCreate, CredentialAccept, CredentialDelete)
- New Credential ledger object
- Modifications to DepositPreauth ledger object
-Add support for XLS-70d Verifiable Credentials in xrpl-py library.
+Add support for XLS-70d Verifiable Credentials in xrpl-py library:
+ - Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete
+ - Add new Credential ledger object
+ - Update DepositPreauth ledger object with credential support📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Add support for XLS-70d Verifiable Credentials in xrpl-py library. | |
| Add support for XLS-70d Verifiable Credentials in xrpl-py library: | |
| - Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete | |
| - Add new Credential ledger object | |
| - Update DepositPreauth ledger object with credential support |
There was a problem hiding this comment.
line 12 needs to begin with a - character to represent a bullet point in the markdown file
There was a problem hiding this comment.
@ckeshava, thank you for catching that. You're absolutely right—the line should start with a - to represent a bullet point in the markdown file. Here's the updated changelog entry:
-Add support for XLS-70d Verifiable Credentials in xrpl-py library.
+- Add support for XLS-70d Verifiable Credentials in xrpl-py library:
+ - Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete
+ - Add new Credential ledger object
+ - Update DepositPreauth ledger object with credential supportLet me know if there's anything else I can assist with.
There was a problem hiding this comment.
@rabbit can you present a "Commitable Suggestion" with this updated change?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.