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
… 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 <mvadari@gmail.com>
…ion model; Revert this file to an older version
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/transactions/test_permissioned_domain_set.py (1)
1-25: Add test cases for domain_id validation.While the basic test case is good, consider adding test cases for
domain_idvalidation:
- Invalid hex string format
- Incorrect length
- Missing domain_id when required for modification
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/transactions/test_permissioned_domain.py(1 hunks)tests/unit/models/transactions/test_permissioned_domain_set.py(1 hunks)xrpl/models/transactions/permissioned_domain_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/transactions/test_permissioned_domain.py
🧰 Additional context used
📓 Learnings (1)
xrpl/models/transactions/permissioned_domain_set.py (1)
Learnt from: mvadari
PR: XRPLF/xrpl-py#773
File: xrpl/models/transactions/permissioned_domain_set.py:22-22
Timestamp: 2024-11-20T21:16:43.491Z
Learning: In the `xrpl.models.transactions.PermissionedDomainSet` class, the `domain_id` field is optional in the `PermissionedDomainSet` transaction.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (7)
xrpl/models/transactions/permissioned_domain_set.py (4)
1-15: LGTM!The imports are well-organized and the constant is properly defined with a clear naming convention.
17-20: LGTM!The class is well-defined with appropriate decorators enforcing immutability and keyword-only arguments.
22-34: LGTM!The fields are well-documented and properly typed. The
domain_idis correctly marked as optional, and thetransaction_typeis appropriately set as non-initializable.
36-53: LGTM!The error handling is comprehensive, covering all necessary validations for the
accepted_credentialslist with clear error messages.tests/unit/models/transactions/test_permissioned_domain_set.py (3)
26-44: LGTM!The test effectively validates the maximum length constraint for
accepted_credentials.
46-56: LGTM!The test effectively validates that empty
accepted_credentialslists are rejected.
58-76: LGTM!The test effectively validates that duplicate credentials are rejected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
xrpl/models/transactions/permissioned_domain_delete.py (1)
19-20: Enhance the docstring.The docstring should include more details about the transaction and its fields based on the specification.
Apply this diff to enhance the docstring:
- """This transaction deletes a PermissionedDomain object.""" + """ + This transaction deletes a PermissionedDomain object from the ledger. + + Attributes: + domain_id: A 64-character hexadecimal string that uniquely identifies + the PermissionedDomain object to be deleted. + """tests/unit/models/transactions/test_permissioned_domain_delete.py (1)
9-9: Add test cases for edge cases.Consider adding test cases for:
- Empty domain_id
- None domain_id
Apply this diff to add the test cases:
+ def test_domain_id_empty(self): + with self.assertRaises(XRPLModelException) as err: + PermissionedDomainDelete( + account=_ACCOUNT_1, + domain_id="", + ) + + self.assertEqual( + err.exception.args[0], + "{'PermissionedDomainDelete': 'domain_id must be 64 characters long.'}", + ) + + def test_domain_id_none(self): + with self.assertRaises(XRPLModelException) as err: + PermissionedDomainDelete( + account=_ACCOUNT_1, + domain_id=None, + ) + + self.assertEqual( + err.exception.args[0], + "{'PermissionedDomainDelete': 'domain_id must be 64 characters long.'}", + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(2 hunks)tests/integration/transactions/test_permissioned_domain.py(1 hunks)tests/unit/models/transactions/test_permissioned_domain_delete.py(1 hunks)tests/unit/models/transactions/test_permissioned_domain_set.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(5 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/permissioned_domain_delete.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- xrpl/models/transactions/init.py
- xrpl/models/transactions/types/transaction_type.py
- CHANGELOG.md
- tests/unit/models/transactions/test_permissioned_domain_set.py
- tests/integration/transactions/test_permissioned_domain.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (3)
xrpl/models/transactions/permissioned_domain_delete.py (1)
1-14: LGTM!The imports are appropriate and the regex pattern is correctly defined as a Final constant to match a 64-character hexadecimal string.
tests/unit/models/transactions/test_permissioned_domain_delete.py (1)
10-16: LGTM!The test cases cover the basic validation scenarios with appropriate assertions.
xrpl/core/binarycodec/definitions/definitions.json (1)
1253-1262: LGTM!The additions to the definitions file are well-structured and follow the existing patterns:
- New fields
DomainIDandAcceptedCredentialsare correctly defined with appropriate types and properties- New ledger entry type
PermissionedDomainuses a valid unique number- New transaction types
PermissionedDomainSetandPermissionedDomainDeleteuse valid unique numbersAlso applies to: 2543-2551, 2885-2885, 3118-3119
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| default=TransactionType.PERMISSIONED_DOMAIN_DELETE, | ||
| init=False, | ||
| ) | ||
| """The transaction type (PermissionedDomainDelete).""" |
There was a problem hiding this comment.
I don't think we've done this in other transactions but makes sense to add it for transaction_type and not exclude it from all other class variables.
There was a problem hiding this comment.
I don't understand. What do you propose to add/remove/change in this snippet?
There was a problem hiding this comment.
I agree with this change. I was just noting that we haven't added docstrings to transaction_type variables before but I do think we may as well such that all class variables have docstrings.
There was a problem hiding this comment.
yes okay 👍
I felt we could add one for the sake of consistency
| def _get_errors(self: Self) -> Dict[str, str]: | ||
| errors = super()._get_errors() | ||
|
|
||
| if len(self.domain_id) != 64: |
There was a problem hiding this comment.
We should use a variable for this.
| if len(self.domain_id) != 64: | |
| if len(self.domain_id) != DOMAIN_ID_LENGTH: |
| errors = super()._get_errors() | ||
|
|
||
| if len(self.domain_id) != 64: | ||
| errors["PermissionedDomainDelete"] = "domain_id must be 64 characters long." |
There was a problem hiding this comment.
| errors["PermissionedDomainDelete"] = "domain_id must be 64 characters long." | |
| errors["PermissionedDomainDelete"] = "domain_id must be " + DOMAIN_ID_LENGTH + " characters long." |
There was a problem hiding this comment.
I would recommend this:
| errors["PermissionedDomainDelete"] = "domain_id must be 64 characters long." | |
| errors["PermissionedDomainDelete"] = f"domain_id must be {DOMAIN_ID_LENGTH} characters long." |
There was a problem hiding this comment.
that violates the maximum length rule, I'll update it in a slightly different way
| "{'PermissionedDomainDelete': 'domain_id must be " | ||
| + str(DOMAIN_ID_LENGTH) | ||
| + " characters long.'}", |
There was a problem hiding this comment.
You can still use what @mvadari recommended:
| "{'PermissionedDomainDelete': 'domain_id must be " | |
| + str(DOMAIN_ID_LENGTH) | |
| + " characters long.'}", | |
| "{'PermissionedDomainDelete': 'domain_id must be " | |
| f"{DOMAIN_ID_LENGTH} characters long.'}}", |
There was a problem hiding this comment.
how is this better than what I already have in the code?
There was a problem hiding this comment.
You don't think it's cleaner? f-strings are generally considered the best practice for string interpolation. It's why we also use them in the codebase too.
| "{'PermissionedDomainDelete': 'domain_id must be " | ||
| + str(DOMAIN_ID_LENGTH) | ||
| + " characters long.'}", |
There was a problem hiding this comment.
how is this better than what I already have in the code?
|
thank you for all the reviews. |
5 participants
High Level Overview of Change
This PR implements the PermissionedDomains amendment in the xrpl-py library.
Specification: XRPLF/XRPL-Standards#228
C++ Implementation: XRPLF/rippled#5161
#759 is a prerequisite for this PR.
For the readers who are only interested in the
PermissionedDomainchanges only, (but not theVerifiableCredentialschanges) please read from this commit onwards: 8abf7b7Context of Change
This PR implements the client library changes for two new transactions:
PermissionedDomainSetandPermissionedDomainDeleteType of Change
Did you update CHANGELOG.md?
Test Plan
I've included Unit and integration tests for the newly added transactions. No new unit tests have been attributed to the
PermissionedDomainDeletetransaction. Thedomain_idfield must be retrieved from one of the rippled RPCs which conforms to the HASH256 internal type.