XLS-80d Permissioned Domains

[ckeshava]

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 PermissionedDomain changes only, (but not the VerifiableCredentials changes) please read from this commit onwards: 8abf7b7

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release

Did you update CHANGELOG.md?

• Yes
• No, this change does not impact library users

Test Plan

I've included Unit and integration tests for the newly added transactions. No new unit tests have been attributed to the PermissionedDomainDelete transaction. The domain_id field must be retrieved from one of the rippled RPCs which conforms to the HASH256 internal type.

[ckeshava]

Hello, I'm going on a vacation next week. If possible, I'd appreciate if I can get a review this week.

[mvadari]

Hello, I'm going on a vacation next week. If possible, I'd appreciate if I can get a review this week.

This PR is very unlikely to be merged before then, as rippled code is unlikely to be merged by then and this PR can't be merged until the rippled PR is merged.

[achowdhry-ripple]

Integration tests need to be fixed

[ckeshava]

Integration tests need to be fixed

Hello @achowdhry-ripple , the integration test can't be fixed for this PR yet. The rippled c++ implementation is yet to merged into the develop branch. After that, a docker image will be generated for the new build of rippled, which can be used inside our integration tests.

[mvadari]

This PR should be rebased onto main now that the credentials PR has been merged.

[coderabbitai]

Actionable comments posted: 3

🔭 Outside diff range comments (1)

tests/integration/transactions/test_xchain_modify_bridge.py (1)

Line range hint 63-63: Add missing client parameter to sign_and_reliable_submission_async

The client parameter is missing in the call to sign_and_reliable_submission_async, which could cause issues.

Apply this fix:

-            door_wallet,
+            door_wallet,
+            client,

♻️ Duplicate comments (1)

xrpl/models/requests/ledger_entry.py (1)

227-237: ⚠️ Potential issue

Update seq field to domain_id in PermissionedDomain class

The PermissionedDomain class currently uses seq as a required field. However, in the context of permissioned domains, the identifier used is domain_id rather than a sequence number. To ensure consistency and correctness with the rest of the codebase and the PermissionedDomains specification, consider renaming seq to domain_id.

🧹 Nitpick comments (2)

xrpl/models/transactions/credential_accept.py (2)

Line range hint 19-24: Enhance docstring with usage example.

The docstring clearly explains the purpose, but adding a code example would help developers understand how to use this transaction type correctly.

Add an example like this to the docstring:

# Example usage:
credential_accept = CredentialAccept(
    account="rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
    issuer="rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW",
    credential_type="0123456789ABCDEF"
)

---

30-30: Remove unnecessary empty line.

This empty line disrupts the field definitions block.

-    

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26ea9b6 and f65ba6b.

📒 Files selected for processing (21)

• .ci-config/rippled.cfg (1 hunks)
• .github/workflows/integration_test.yml (2 hunks)
• CHANGELOG.md (3 hunks)
• tests/integration/transactions/test_credential.py (3 hunks)
• tests/integration/transactions/test_delete_oracle.py (2 hunks)
• tests/integration/transactions/test_did_delete.py (3 hunks)
• tests/integration/transactions/test_did_set.py (2 hunks)
• tests/integration/transactions/test_permissioned_domain.py (1 hunks)
• tests/integration/transactions/test_set_oracle.py (2 hunks)
• tests/integration/transactions/test_xchain_add_account_create_attestation.py (2 hunks)
• tests/integration/transactions/test_xchain_create_bridge.py (2 hunks)
• tests/integration/transactions/test_xchain_create_claim_id.py (2 hunks)
• tests/integration/transactions/test_xchain_modify_bridge.py (3 hunks)
• tests/unit/models/requests/test_ledger_entry.py (2 hunks)
• xrpl/core/binarycodec/definitions/definitions.json (10 hunks)
• xrpl/models/requests/account_objects.py (1 hunks)
• xrpl/models/requests/ledger_entry.py (5 hunks)
• xrpl/models/transactions/__init__.py (2 hunks)
• xrpl/models/transactions/credential_accept.py (1 hunks)
• xrpl/models/transactions/deposit_preauth.py (0 hunks)
• xrpl/models/transactions/types/transaction_type.py (1 hunks)

💤 Files with no reviewable changes (1)

• xrpl/models/transactions/deposit_preauth.py

🚧 Files skipped from review as they are similar to previous changes (8)

• xrpl/models/requests/account_objects.py
• xrpl/models/transactions/init.py
• tests/unit/models/requests/test_ledger_entry.py
• .github/workflows/integration_test.yml
• .ci-config/rippled.cfg
• xrpl/models/transactions/types/transaction_type.py
• tests/integration/transactions/test_credential.py
• tests/integration/transactions/test_permissioned_domain.py

🧰 Additional context used

📓 Learnings (1)

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.

🪛 LanguageTool

CHANGELOG.md

[grammar] ~10-~10: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Add support for the PermissionedDomains f...

(REPEATED_VERBS)

🔇 Additional comments (20)

tests/integration/transactions/test_xchain_add_account_create_attestation.py (1)

12-12: LGTM! Verify similar changes across the codebase.

The replacement of AccountObjectType with LedgerEntryType is consistent with the broader changes in the codebase.

Let's verify that this change has been consistently applied across all test files:

#!/bin/bash
# Description: Check for any remaining usage of AccountObjectType in test files
# Expected: No results, indicating complete migration to LedgerEntryType

# Search for any remaining AccountObjectType usage
echo "Checking for remaining AccountObjectType usage:"
rg "AccountObjectType" "tests/"

# Verify LedgerEntryType is used consistently
echo "Verifying LedgerEntryType usage in AccountObjects requests:"
ast-grep --pattern 'AccountObjects(
  $$$
  type=$_
  $$$
)'

Also applies to: 29-29

xrpl/models/transactions/credential_accept.py (3)

27-29: LGTM! Field definitions follow the established pattern.

The required fields are properly defined with type annotations and clear docstrings.

Also applies to: 31-38

---

Line range hint 40-44: LGTM! Transaction type field is correctly defined.

The transaction type is properly set as a non-init field using the correct enum value.

---

Line range hint 46-53: Verify error messages from credential type validation.

The error handling implementation looks good, but we should verify the error messages from get_credential_type_error.

Let's check the implementation of the validation function:

#!/bin/bash
# Search for the credential type validation implementation
rg -A 5 "def get_credential_type_error" 

xrpl/models/requests/ledger_entry.py (6)

30-30: Adding new LedgerEntryType enumeration: BRIDGE

The addition of BRIDGE = "bridge" to the LedgerEntryType enum enhances support for bridge ledger entries. This update aligns with the extended functionality required for cross-chain transactions.

---

42-42: Adding new LedgerEntryType enumeration: PERMISSIONED_DOMAIN

The inclusion of PERMISSIONED_DOMAIN = "permissioned_domain" in the LedgerEntryType enum appropriately extends the enum to support permissioned domain ledger entries as part of the PermissionedDomains amendment.

---

49-49: Adding new LedgerEntryType enumeration: XCHAIN_OWNED_CLAIM_ID

The addition of XCHAIN_OWNED_CLAIM_ID = "xchain_owned_claim_id" to LedgerEntryType correctly adds support for cross-chain owned claim ID ledger entries, facilitating the handling of cross-chain transfers.

---

52-67: Definition of Credential class is appropriate

The Credential class is properly defined with the required fields: subject, issuer, and credential_type. The implementation aligns with the specifications for handling credentials within the ledger.

---

335-335: Adding permissioned_domain field to LedgerEntry class

The addition of the permissioned_domain optional field to the LedgerEntry class enables querying of permissioned domain ledger entries. This enhancement is consistent with the new functionality introduced by the PermissionedDomains amendment.

---

369-369: Including permissioned_domain in error validation

The inclusion of self.permissioned_domain in the _get_errors method ensures that the validation logic correctly handles the new permissioned_domain field. This addition maintains the integrity of error checking for ledger entry requests.

tests/integration/transactions/test_did_set.py (2)

7-7: Replace deprecated AccountObjectType with LedgerEntryType

The import of LedgerEntryType and removal of AccountObjectType aligns with the deprecation of AccountObjectType in favor of LedgerEntryType. This update ensures compatibility with the latest codebase standards.

---

28-28: Update type parameter to use LedgerEntryType.DID

In the AccountObjects request, changing the type parameter to LedgerEntryType.DID reflects the updated enumeration. This modification ensures that the correct type is specified when querying for DID ledger entries.

tests/integration/transactions/test_xchain_create_claim_id.py (2)

8-8: Replace deprecated AccountObjectType with LedgerEntryType

The import of LedgerEntryType in place of AccountObjectType adheres to the updated usage within the codebase. This change is necessary due to the deprecation of AccountObjectType.

---

33-33: Update type parameter to use LedgerEntryType.XCHAIN_OWNED_CLAIM_ID

Adjusting the type parameter to LedgerEntryType.XCHAIN_OWNED_CLAIM_ID in the AccountObjects request correctly specifies the ledger entry type for cross-chain owned claim IDs. This update ensures accurate retrieval of the intended ledger entries.

tests/integration/transactions/test_xchain_create_bridge.py (1)

11-11: Verify documentation for LedgerEntryType migration

The change from AccountObjectType to LedgerEntryType looks correct. However, since this appears to be part of a broader refactoring effort:

1. Ensure this change is documented in the CHANGELOG.md
2. Consider adding a deprecation notice if AccountObjectType is still available

#!/bin/bash
# Check if the change is documented in CHANGELOG
rg -i "accountobjecttype.*ledgerentrytype" CHANGELOG.md

# Check if AccountObjectType is still available and needs deprecation
rg -l "class AccountObjectType" 

Also applies to: 43-43

tests/integration/transactions/test_did_delete.py (1)

7-7: LGTM! Changes are consistent and well-tested

The migration to LedgerEntryType is applied correctly, and the test coverage remains comprehensive, verifying both the creation and deletion of DIDs.

Also applies to: 29-29, 43-43

tests/integration/transactions/test_delete_oracle.py (1)

9-9: LGTM! Well-structured test with proper setup and verification

The migration to LedgerEntryType is implemented correctly, and the test maintains good coverage of Oracle creation and deletion scenarios.

Also applies to: 57-57

tests/integration/transactions/test_xchain_modify_bridge.py (1)

11-11: LGTM! Type changes are consistent

The migration to LedgerEntryType is implemented correctly, maintaining proper test coverage for XChain bridge operations.

Also applies to: 44-44, 70-70

xrpl/core/binarycodec/definitions/definitions.json (2)

56-57: LGTM! New ledger entry types look good.

The addition of Credential and PermissionedDomain types with their respective type numbers is correct and aligns with the PermissionedDomains amendment.

---

3138-3149: LGTM! New transaction types look good.

The addition of transaction types for credentials and permissioned domains with their respective type numbers is correct:

• CredentialCreate: 58
• CredentialAccept: 59
• CredentialDelete: 60
• PermissionedDomainSet: 61
• PermissionedDomainDelete: 62

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

CHANGELOG.md (1)

11-11: Enhance the changelog entry with more details and fix grammar.

The current entry could be more descriptive to help users understand the significance of this feature. Also, there's a grammatical redundancy with "Add".

Consider this enhanced entry:

-Add support for the `PermissionedDomains` feature
+Support for the `PermissionedDomains` amendment, enabling domain-based permissions for XRPL accounts

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e00d485 and 19c3caa.

📒 Files selected for processing (1)

• CHANGELOG.md (2 hunks)

🧰 Additional context used

🪛 LanguageTool

CHANGELOG.md

[grammar] ~10-~10: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Add support for the PermissionedDomains f...

(REPEATED_VERBS)