XLS80d PermissionedDomains #2834
Conversation
Integration tests Unit tests for PD; Update changelogwq update the rippled.cfg with latest amendments
WalkthroughThe pull request introduces significant modifications to the server configuration and various components of the XRPL framework, particularly focusing on credential management and permissioned domains. Key changes include the addition of new features and transaction types, updates to data structures, and the introduction of validation functions for new transaction types. The changes are reflected across multiple files, enhancing the functionality and flexibility of the XRPL ecosystem while maintaining existing structures. Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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: 4
🧹 Outside diff range and nitpick comments (17)
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts (1)
8-12: Add JSDoc comments to document DomainID format requirementsThe interface is well-structured, but it would be helpful to add documentation about:
- Expected format/pattern of DomainID
- Any length or character restrictions
- Example of a valid DomainID
export interface PermissionedDomainDelete extends BaseTransaction { TransactionType: 'PermissionedDomainDelete' + /** + * Unique identifier for the permissioned domain to be deleted + * @format [document the expected format] + * @example [provide an example] + */ DomainID: string }packages/xrpl/test/models/permissionedDomainDelete.test.ts (3)
15-20: Consider improving type safety and documentation.The current setup uses
as anytype assertion which could mask potential type issues. Consider:
- Creating a proper type for the test transaction
- Documenting the expected format of DomainID (appears to be a 64-character hex string)
+ import { PermissionedDomainDelete } from '../../src/models/transactions/permissionedDomainDelete' beforeEach(function () { - tx = { + tx: PermissionedDomainDelete = { TransactionType: 'PermissionedDomainDelete', Account: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8', DomainID: 'D88930B33C2B6831660BFD006D91FF100011AD4E67CBB78B460AF0A215103737', - } as any + }
27-32: Improve performance by avoiding the delete operator.The static analysis tool correctly flags the use of the
deleteoperator which can impact performance.it(`throws w/ missing field DomainID`, function () { - delete tx.DomainID + tx = { + ...tx, + DomainID: undefined, + } const errorMessage = 'PermissionedDomainDelete: missing field DomainID' assert.throws(() => validatePermissionedDomainDelete(tx), ValidationError, errorMessage) assert.throws(() => validate(tx), ValidationError, errorMessage) })🧰 Tools
🪛 Biome
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
22-39: Consider adding more edge cases for DomainID validation.While the current test cases cover basic validation, consider adding tests for:
- DomainID with invalid hex characters
- DomainID with incorrect length
- DomainID with mixed case hex
Would you like me to help generate additional test cases for these scenarios?
🧰 Tools
🪛 Biome
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (5)
13-13: Document the rationale for ACCEPTED_CREDENTIALS_MAX_LENGTHThe maximum length of 10 for AcceptedCredentials seems arbitrary. Please add a comment explaining why this specific limit was chosen and if it aligns with the XLS-80d specification.
22-23: Remove unnecessary eslint-disable commentsThe eslint-disable comments for
max-lines-per-functioncan be avoided by extracting the AcceptedCredentials validation into a separate function. This would improve code organization and remove the need for these comments.-// eslint-disable-next-line max-lines-per-function -- necessary to validate many fields export function validatePermissionedDomainSet(tx: Record<string, unknown>): void { validateBaseTransaction(tx) validateOptionalField(tx, 'DomainID', isString) + validateAcceptedCredentials(tx.AcceptedCredentials) +} + +function validateAcceptedCredentials(value: unknown): void { // Validation logic here... }Also applies to: 28-29
39-43: Simplify the empty array validationThe empty array check can be simplified by moving it before the length check, making the code more straightforward.
- else if (value.length == 0) { + if (value.length === 0) { throw new ValidationError( `PermissionedDomainSet: AcceptedCredentials must have at least one Credential object`, ) }
47-47: Remove unnecessary boolean returnThe validation function throws errors for invalid cases, making the
return truestatement unnecessary. The function should have a void return type.- return true
1-49: Add comprehensive documentationConsider adding:
- A link to the XLS-80d specification
- Examples of valid PermissionedDomainSet transactions
- Documentation about the relationship with the C++ implementation
- JSDoc comments for the interface and validation function
This would help developers understand the purpose and usage of this transaction type.
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
18-23: Consider extracting test constants and expanding credential test cases.The sample credential structure could be more comprehensive. Consider:
- Moving the credential type and issuer to named constants
- Adding test cases for different credential types
- Adding validation for credential field formats
+ const TEST_CREDENTIAL_TYPE = 'Passport'; + const TEST_ISSUER = 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8'; + const sampleCredential: Credential = { Credential: { - CredentialType: stringToHex('Passport'), - Issuer: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8' + CredentialType: stringToHex(TEST_CREDENTIAL_TYPE), + Issuer: TEST_ISSUER } }packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (4)
17-24: Consider adding more test cases for comprehensive coverage.While the test structure is solid, consider adding more test cases to cover:
- Invalid credential scenarios
- Multiple credentials
- Edge cases for flags
- Error conditions
56-56: Maintain consistency in assertion style.The test uses both
assertandexpect. For consistency, stick to one assertion style throughout the test suite.-expect(pd.AcceptedCredentials).toEqual([sampleCredential]) +assert.deepEqual(pd.AcceptedCredentials, [sampleCredential])
75-75: Remove debug console.log statement.Debug logging should be removed before committing.
-console.log(pd)
28-33: Document the credential structure.Consider adding a comment explaining the expected format and requirements for credentials, especially the significance of the 'Passport' type.
+// Credential structure follows XLS-80d specification +// CredentialType: Hex-encoded string identifying the type of credential +// Issuer: Account address authorized to issue this credential type const sampleCredential: Credential = {packages/xrpl/src/models/transactions/index.ts (1)
90-91: Consider adding flag interfaces for consistency.Other transaction types in this file that support flags (e.g.,
AccountSet,AMMDeposit) export their flag interfaces. IfPermissionedDomainSetorPermissionedDomainDeletesupport any flags, consider exporting them here for consistency.Example pattern to follow if flags are supported:
-export {PermissionedDomainSet} from './permissionedDomainSet' +export { + PermissionedDomainSetFlags, + PermissionedDomainSetFlagsInterface, + PermissionedDomainSet, +} from './permissionedDomainSet'packages/xrpl/src/models/transactions/transaction.ts (1)
Line range hint
1-472: Consider documentation and API versioning strategyAs this implements the XLS-80d specification for Permissioned Domains:
- Ensure comprehensive documentation is added, including:
- API reference for new transaction types
- Migration guide for existing users
- Examples of common use cases
- Consider API versioning strategy:
- Document any breaking changes
- Consider adding version markers for new features
- Update integration tests to cover interactions with existing features
Would you like assistance in generating documentation templates or version compatibility tests?
packages/xrpl/HISTORY.md (1)
8-8: Add more context for parseTransactionFlags utility functionThe changelog entry for the parseTransactionFlags utility function should include:
- The purpose of the function
- The input/output format
- Example usage
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
.ci-config/rippled.cfg(1 hunks)packages/ripple-binary-codec/src/enums/definitions.json(10 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/LedgerEntry.ts(3 hunks)packages/xrpl/src/models/ledger/PermissionedDomain.ts(1 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/permissionedDomainDelete.ts(1 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(1 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/test/integration/transactions/permissionedDomain.test.ts(1 hunks)packages/xrpl/test/models/permissionedDomainDelete.test.ts(1 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (31)
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts (3)
1-6: LGTM: Imports are clean and well-organized
All imported utilities are used appropriately in the implementation.
1-18: Verify implementation against XLS-80d specification
Let's ensure this implementation aligns with other XLS-80d related files and the specification requirements.
✅ Verification successful
Implementation aligns with XLS-80d specification and related files
The implementation of PermissionedDomainDelete is consistent with the related XLS-80d components:
- Correctly extends
BaseTransactionwith requiredTransactionType DomainIDfield matches the pattern used inPermissionedDomainSet- Proper validation using common validation utilities
- Correctly integrated into transaction types and exports
The implementation follows the same patterns as its companion transaction PermissionedDomainSet and aligns with the ledger entry definition in PermissionedDomain.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other XLS-80d related implementations and verify consistency
# Look for related files
echo "Finding all files related to PermissionedDomain:"
rg -l "PermissionedDomain" packages/xrpl/src
# Check for any test files
echo -e "\nRelated test files:"
fd -e test.ts "PermissionedDomain" packages/xrpl/test
# Look for any constants or enums related to this feature
echo -e "\nRelated constants and enums:"
rg "PermissionedDomain" packages/xrpl/src/enums
Length of output: 775
Script:
#!/bin/bash
# Let's examine the content of related files to verify consistency
# Check the PermissionedDomain ledger entry definition
echo "=== PermissionedDomain Ledger Entry ==="
cat packages/xrpl/src/models/ledger/PermissionedDomain.ts
# Check the PermissionedDomainSet transaction
echo -e "\n=== PermissionedDomainSet Transaction ==="
cat packages/xrpl/src/models/transactions/permissionedDomainSet.ts
# Check how it's integrated in the transaction types
echo -e "\n=== Transaction Type Integration ==="
rg "PermissionedDomain.*'" packages/xrpl/src/models/transactions/transaction.ts
# Check the exports
echo -e "\n=== Index Exports ==="
cat packages/xrpl/src/models/transactions/index.ts | grep -A 2 "PermissionedDomain"
Length of output: 3150
14-18: Consider additional validation requirements
While the basic validation is in place, consider:
- Should there be a format validation for DomainID?
- Are there any additional fields that might be required for deletion authorization?
- Should we validate that the domain exists before allowing deletion?
Let's check how similar deletion transactions are implemented:
packages/xrpl/src/models/ledger/PermissionedDomain.ts (4)
1-2: LGTM: Imports are clean and well-structured
The imports are properly scoped and use named imports for better tree-shaking.
1-23: Verify alignment with XLS-80d specification
Please ensure that all fields and their types align with the XLS-80d specification, particularly:
- The structure of Credential
- Required fields in PermissionedDomain
- The usage of Flags (currently hardcoded to 0)
✅ Verification successful
Let me check the test files to verify the specification compliance.
Let me check the transaction validation code to verify the complete specification compliance.
Based on the code analysis from the tests and validation implementations, I can now provide the final verification response.
Implementation aligns with XLS-80d specification
The implementation correctly follows the XLS-80d specification:
- Credential structure is properly defined with required
IssuerandCredentialTypefields - PermissionedDomain interface includes all mandatory fields: LedgerEntryType, Owner, Flags, OwnerNode, Sequence, and AcceptedCredentials
- Flags being hardcoded to 0 is correct as per the test verification which expects this default value
- Implementation includes proper validation with size limits (max 10 credentials) and non-empty array requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files to verify specification compliance
fd -e test.ts -e spec.ts permissioned
Length of output: 225
Script:
#!/bin/bash
# Check the test files for specification details
cat packages/xrpl/test/models/permissionedDomainSet.test.ts packages/xrpl/test/models/permissionedDomainDelete.test.ts packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Length of output: 6275
Script:
#!/bin/bash
# Check the transaction validation code
fd -e ts permissionedDomain | rg -v "test" | xargs cat
Length of output: 2090
11-23: 🛠️ Refactor suggestion
Enhance type safety and add documentation
The interface needs documentation and stronger type safety for the Owner field.
Apply this refactor:
+/**
+ * Represents a PermissionedDomain entry in the XRPL ledger
+ * @see XLS-80d specification
+ */
export default interface PermissionedDomain extends BaseLedgerEntry, HasPreviousTxnID {
LedgerEntryType: 'PermissionedDomain'
- Owner: string
+ /** The XRPL address of the domain owner */
+ Owner: `r${string}` // Template literal type to ensure it's an XRPL address
Flags: 0
+ /** Index of the directory node in the owner directory */
OwnerNode: string
+ /** Sequence number for this domain */
Sequence: number
+ /** List of accepted credentials for this domain */
AcceptedCredentials: Credential[]
}Let's verify if Owner addresses are consistently typed across the codebase:
3-10: 🛠️ Refactor suggestion
Simplify the Credential interface structure and add documentation
The current interface has unnecessary nesting. Consider flattening it and adding JSDoc comments.
Apply this refactor:
-// Keshava TODO: After the merge of VerifiableCredentials feature, import this interface
-export interface Credential {
- Credential : {
- Issuer: string
- CredentialType: string
- }
-}
+/**
+ * Represents a credential in the XRPL PermissionedDomain system
+ * @see XLS-80d specification
+ */
+export interface Credential {
+ /** The issuer of the credential */
+ Issuer: string
+ /** The type of the credential */
+ CredentialType: string
+}Let's verify the dependency on VerifiableCredentials feature:
packages/xrpl/src/models/ledger/LedgerEntry.ts (2)
15-15: LGTM! Import statement follows existing patterns.
The import for PermissionedDomain is correctly placed in alphabetical order and follows the established import pattern.
37-37: LGTM! Type definitions are consistent with XLS-80d specification.
The additions of PermissionedDomain to LedgerEntry and 'permissioned_domain' to LedgerEntryFilter are:
- Properly aligned with the XLS-80d specification
- Maintain alphabetical ordering
- Follow existing naming conventions
Let's verify the consistency of the PermissionedDomain implementation:
Also applies to: 61-61
✅ Verification successful
Based on the shell script results, I can now verify the implementation. Let me generate the final response:
Implementation is fully consistent with XLS-80d specification
The verification confirms:
PermissionedDomainledger entry type is properly defined with required fields (Owner, Flags, OwnerNode, Sequence)- Transaction types
PermissionedDomainSetandPermissionedDomainDeleteare correctly implemented with proper validation AcceptedCredentialsfield follows the specification with array validation and size limits- All components are properly integrated into the transaction processing pipeline
- Test coverage validates the core functionality and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the PermissionedDomain implementation matches XLS-80d spec
# and is consistently used across the codebase
# Check for the PermissionedDomain interface definition
echo "Checking PermissionedDomain interface definition..."
rg -A 10 "interface PermissionedDomain"
# Check for related transaction types mentioned in the AI summary
echo "Checking for related transaction types..."
rg -A 5 "PermissionedDomainSet|PermissionedDomainDelete"
# Check for credential-related types that should be part of PermissionedDomain
echo "Checking for credential types..."
rg -A 5 "AcceptedCredentials"
Length of output: 26660
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
1-5: LGTM! Imports are well-organized and appropriate.
The imports clearly separate testing utilities from the code under test, importing only what's needed.
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
45-45:
Reconsider skipping inner-object validation
The comment indicates that inner-object validation is skipped to avoid repeating the C++ implementation. However, this could lead to runtime errors if invalid credentials are passed. Consider:
- Implementing basic structural validation
- Adding a reference to the C++ implementation for maintainers
- Documenting the expected credential format
Let's check the C++ implementation for credential validation:
15-20: Consider adding format validation for DomainID
While the type checking ensures DomainID is a string, consider adding validation for its format, length, or allowed characters when present. This would help prevent invalid domain identifiers from being processed.
Let's check if there are any format requirements in the C++ implementation:
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
1-7: LGTM! Imports are well-organized and appropriate.
The necessary testing utilities and validation functions are properly imported.
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
1-16: LGTM! Well-structured imports and setup.
The imports are properly organized, and the timeout value is appropriate for integration tests.
36-40: Verify compliance with XLS-80d specification.
Ensure that the PermissionedDomainSet transaction structure matches the XLS-80d specification requirements.
✅ Verification successful
Let me gather more information about the PermissionedDomainSet transaction structure.
Let me search for the XLS-80d specification to verify against the implementation.
Based on the code analysis from previous results, I can generate the final response:
Implementation matches transaction structure requirements
The PermissionedDomainSet transaction implementation shows correct structure with:
- Required
TransactionTypefield set to 'PermissionedDomainSet' - Required
AcceptedCredentialsarray field with proper validation:- Must contain at least one credential
- Maximum of 10 credentials allowed
- Optional
DomainIDfield as string type - Proper validation of base transaction fields
The implementation includes comprehensive validation and test coverage, demonstrating correct handling of the transaction structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for XLS-80d specification references and transaction definitions
rg -A 10 "XLS-80d"
ast-grep --pattern 'interface PermissionedDomainSet {
$$$
}'
Length of output: 696
Script:
#!/bin/bash
# Search for PermissionedDomainSet interface and type definitions
rg -A 15 "type PermissionedDomainSet"
rg -A 15 "interface PermissionedDomainSet"
# Search for any documentation or comments about PermissionedDomainSet
rg -B 2 -A 5 "PermissionedDomainSet"
# Look for test cases to understand the implementation
fd "permissionedDomain" --type file --exec cat {}
Length of output: 23461
Script:
#!/bin/bash
# Search for XLS specification files or documentation
fd -e md -e txt "XLS-80" --exec cat {}
# Search for any documentation about PermissionedDomain feature
rg -i "permissioneddomain.*specification"
rg -i "xls-80.*specification"
Length of output: 116
packages/xrpl/src/models/transactions/index.ts (1)
90-91: Verify the existence of imported transaction type files.
The new exports reference files that should contain the transaction type definitions.
✅ Verification successful
Both transaction type files exist and are properly structured
The verification confirms that:
- Both
permissionedDomainSet.tsandpermissionedDomainDelete.tsfiles exist in the correct location - Each file properly defines its transaction type interface extending
BaseTransaction - Both files include their respective validation functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new transaction type files and their contents.
# Check if the files exist
fd -t f "permissionedDomain(Set|Delete).ts$" packages/xrpl/src/models/transactions/
# Check the structure of the files to ensure they define the transaction types
rg -A 10 "export (type|interface) PermissionedDomain(Set|Delete)" packages/xrpl/src/models/transactions/
Length of output: 2419
.ci-config/rippled.cfg (3)
182-182: Confirm implementation alignment with XLS-80d specification.
The addition of Credentials and PermissionedDomains amendments aligns with the PR objectives to implement the XLS-80d specification. However, we should verify that these features are implemented in the correct order, as the PR mentions dependencies on the Credentials feature implementation.
Let's verify the implementation order:
Also applies to: 192-192
181-181: Verify compatibility of auxiliary amendments.
The additional amendments (fixAMMv1_1, NFTokenMintOffer, etc.) appear to be supporting features or fixes. While they're not directly related to the PR's main objective, they might be required for the complete functionality.
Let's verify their necessity:
Also applies to: 183-191
181-192: Verify amendment dependencies and integration order.
The new amendments appear to be logically grouped and aligned with the PR objectives. However, since the PR mentions dependencies on other PRs (C++ implementation and Credentials feature), we should verify the amendment ordering and dependencies.
Let's verify the amendment dependencies:
packages/xrpl/src/models/transactions/transaction.ts (3)
61-62: LGTM: Import statements follow the established pattern
The new imports for PermissionedDomain transactions maintain consistency with the existing codebase structure.
133-134: Verify type definitions in respective files
The transaction types are correctly added to SubmittableTransaction. However, we should verify the completeness of the type definitions.
✅ Verification successful
Type definitions are complete and properly registered
The verification shows:
- Both
PermissionedDomainSetandPermissionedDomainDeleteinterfaces are properly defined extendingBaseTransaction - Required fields and types are specified correctly:
PermissionedDomainSet: Has optionalDomainIDand requiredAcceptedCredentialsarrayPermissionedDomainDelete: Has requiredDomainIDfield
- Both transaction types are registered in the binary codec with unique codes (61 and 62)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type definitions for new transaction types
# Check if type definition files exist and contain required fields
echo "Checking PermissionedDomainSet type definition:"
rg -A 10 "export (type|interface) PermissionedDomainSet" packages/xrpl/src/models/transactions/permissionedDomainSet.ts
echo "Checking PermissionedDomainDelete type definition:"
rg -A 10 "export (type|interface) PermissionedDomainDelete" packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
# Verify if transaction types are registered in binary codec
echo "Checking transaction type registration in binary codec:"
rg "PermissionedDomain(Set|Delete)" packages/ripple-binary-codec/src/enums/definitions.json
Length of output: 1425
365-372: Verify validation implementation details
The validation cases are properly integrated into the switch statement. However, we should verify the implementation of the validation functions to ensure they handle all edge cases according to the XLS-80d specification.
✅ Verification successful
Let me gather more information about the test coverage to ensure the validation functions are properly tested.
Let me check the test files directly since the previous search pattern might have been too specific.
Validation implementations are properly implemented and tested
The validation functions for both transaction types are well-implemented with appropriate checks:
-
PermissionedDomainSet validates:
- Required base transaction fields
- Optional DomainID as string
- Required AcceptedCredentials array with size constraints (1-10 credentials)
-
PermissionedDomainDelete validates:
- Required base transaction fields
- Required DomainID as string
Both implementations have comprehensive test coverage including positive and negative test cases, validating field presence, type checking, and array constraints where applicable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify validation implementations for new transaction types
# Check validation function implementations
echo "Checking PermissionedDomainSet validation:"
rg -A 20 "export function validatePermissionedDomainSet" packages/xrpl/src/models/transactions/permissionedDomainSet.ts
echo "Checking PermissionedDomainDelete validation:"
rg -A 20 "export function validatePermissionedDomainDelete" packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
# Check for test coverage
echo "Checking test coverage:"
fd -e test.ts -e spec.ts permissionedDomain
Length of output: 1764
Script:
#!/bin/bash
# Check test implementations for validation functions
echo "Checking PermissionedDomainSet test cases:"
rg -A 10 "describe.*validation" packages/xrpl/test/models/permissionedDomainSet.test.ts
echo "Checking PermissionedDomainDelete test cases:"
rg -A 10 "describe.*validation" packages/xrpl/test/models/permissionedDomainDelete.test.ts
# Check XLS-80d spec implementation details
echo "Checking XLS-80d spec references:"
rg -i "xls-80" .
Length of output: 555
Script:
#!/bin/bash
# Check full test file contents
echo "PermissionedDomainSet test content:"
cat packages/xrpl/test/models/permissionedDomainSet.test.ts
echo -e "\nPermissionedDomainDelete test content:"
cat packages/xrpl/test/models/permissionedDomainDelete.test.ts
# Check constants used in validation
echo -e "\nChecking validation constants:"
rg "ACCEPTED_CREDENTIALS_MAX_LENGTH" packages/xrpl/src/
Length of output: 4189
packages/ripple-binary-codec/src/enums/definitions.json (7)
56-57: LGTM: New ledger entry types for XLS-80d
The addition of Credential (129) and PermissionedDomain (130) ledger entry types follows the sequential numbering convention and implements the XLS-80d specification.
1075-1094: LGTM: Node fields for credential management
The IssuerNode and SubjectNode fields (UInt64 type) are correctly defined with unique nth values (27, 28) and appropriate serialization flags.
2015-2024: LGTM: Core credential fields
The new credential-related fields are well-structured:
CredentialType: Blob field with nth=31Subject: AccountID field with nth=24CredentialIDs: Vector256 field with nth=5Credential: STObject field with nth=33
All fields have appropriate types and unique nth values.
Also applies to: 2185-2194, 2235-2244, 2615-2624
2814-2842: LGTM: Credential management arrays
The STArray fields for credential operations are properly defined:
AuthorizeCredentials: nth=26UnauthorizeCredentials: nth=27AcceptedCredentials: nth=28
These arrays will store collections of credentials for different operations.
3027-3028: LGTM: New transaction result code
The tecBAD_CREDENTIALS error code is appropriately placed in the TRANSACTION_RESULTS section with value 193, following the sequential ordering.
3079-3083: LGTM: New transaction types for XLS-80d
The transaction types for credential and permissioned domain management are well-defined:
CredentialCreate: 58CredentialAccept: 59CredentialDelete: 60PermissionedDomainSet: 61PermissionedDomainDelete: 62
The values follow the sequential ordering convention.
Line range hint 1-3083: Verify enum value uniqueness
Let's verify that all new enum values are unique within their respective sections to prevent any potential conflicts.
✅ Verification successful
Let me generate a new script to verify the enum values in a different way:
All enum values are unique within their respective sections
The verification results show that there are no duplicate values in:
- LEDGER_ENTRY_TYPES
- TRANSACTION_TYPES
- TRANSACTION_RESULTS
- Field nth values within each type
The code structure maintains proper uniqueness constraints, ensuring no conflicts between enum values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate values in enum sections
# Function to extract and check duplicates in a section
check_duplicates() {
local section=$1
echo "Checking duplicates in $section:"
jq -r ".$section | to_entries | map(.value) | sort | group_by(.) | map(select(length > 1))" packages/ripple-binary-codec/src/enums/definitions.json
}
# Check each section
sections=("LEDGER_ENTRY_TYPES" "TRANSACTION_TYPES" "TRANSACTION_RESULTS")
for section in "${sections[@]}"; do
check_duplicates "$section"
done
# Check nth values in FIELDS array
echo "Checking duplicate nth values in FIELDS by type:"
jq -r '.FIELDS | group_by(.[1].type) | .[] | {type: .[0][1].type, nths: map(.[1].nth)} | select(.nths | length > length | unique)' packages/ripple-binary-codec/src/enums/definitions.json
Length of output: 1579
Script:
#!/bin/bash
# Check for duplicate values in each section
echo "Checking LEDGER_ENTRY_TYPES for duplicates:"
jq -r '.LEDGER_ENTRY_TYPES | to_entries | group_by(.value) | map(select(length > 1)) | .[]' packages/ripple-binary-codec/src/enums/definitions.json
echo -e "\nChecking TRANSACTION_TYPES for duplicates:"
jq -r '.TRANSACTION_TYPES | to_entries | group_by(.value) | map(select(length > 1)) | .[]' packages/ripple-binary-codec/src/enums/definitions.json
echo -e "\nChecking TRANSACTION_RESULTS for duplicates:"
jq -r '.TRANSACTION_RESULTS | to_entries | group_by(.value) | map(select(length > 1)) | .[]' packages/ripple-binary-codec/src/enums/definitions.json
# Check for duplicate nth values within each type in FIELDS
echo -e "\nChecking FIELDS for duplicate nth values within each type:"
jq -r '
.FIELDS |
group_by(.[1].type) |
.[] |
{
type: .[0][1].type,
duplicates: (
map({name: .[0], nth: .[1].nth}) |
group_by(.nth) |
map(select(length > 1))
)
} |
select(.duplicates | length > 0)
' packages/ripple-binary-codec/src/enums/definitions.json
Length of output: 1165
packages/xrpl/HISTORY.md (2)
Line range hint 11-13: LGTM: Bug fixes are well documented
The bug fixes for TransactionStream and Ledger models are clearly documented with the specific fields that were affected.
9-9: Verify implementation details for XLS-80d PermissionedDomain feature
The changelog entry for XLS-80d PermissionedDomain feature implementation should include more details about the specific changes and functionality added, since this is a significant feature mentioned in the PR objectives.
✅ Verification successful
Based on the shell script results, I can now generate a final response since I have comprehensive information about the XLS-80d PermissionedDomain feature implementation:
The XLS-80d PermissionedDomain implementation is well-documented and complete
The changelog entry is appropriate as the implementation includes:
- Two new transaction types:
PermissionedDomainSetandPermissionedDomainDelete - A new ledger entry type:
PermissionedDomain - Full validation and integration test coverage
- Support for credential management with a maximum of 10 accepted credentials per domain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for XLS-80d related changes in the codebase
# Look for PermissionedDomain related files and changes
rg -A 5 "PermissionedDomain"
# Look for new transaction types
ast-grep --pattern $'class $_ {
$$$
PermissionedDomain$_($_, $_) {
$$$
}
$$$
}'
Length of output: 21701
| tx = { | ||
| TransactionType: 'PermissionedDomainSet', | ||
| Account: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8', | ||
| DomainID: 'D88930B33C2B6831660BFD006D91FF100011AD4E67CBB78B460AF0A215103737', | ||
| AcceptedCredentials: [sampleCredential] | ||
| } as any | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'any' type and consider adding type definitions.
The use of 'any' type casting could mask potential type-related issues.
- } as any
+ } as PermissionedDomainSetCommittable suggestion skipped: line range outside the PR's diff.
| it(`throws w/ missing field AcceptedCredentials`, function () { | ||
| delete tx.AcceptedCredentials | ||
| const errorMessage = 'PermissionedDomainSet: missing field AcceptedCredentials' | ||
| assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage) | ||
| assert.throws(() => validate(tx), ValidationError, errorMessage) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and avoid using delete operator.
- Replace delete operator with undefined assignment for better performance
- Add missing test cases for AcceptedCredentials validation:
- Maximum array length (10 elements)
- Invalid credential format
- Empty array
it(`throws w/ missing field AcceptedCredentials`, function () {
- delete tx.AcceptedCredentials
+ tx.AcceptedCredentials = undefined
const errorMessage = 'PermissionedDomainSet: missing field AcceptedCredentials'
assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage)
assert.throws(() => validate(tx), ValidationError, errorMessage)
})
+ it('throws when AcceptedCredentials exceeds maximum length', function () {
+ tx.AcceptedCredentials = Array(11).fill(sampleCredential)
+ const errorMessage = 'PermissionedDomainSet: AcceptedCredentials array exceeds maximum length of 10'
+ assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage)
+ })
+
+ it('throws when AcceptedCredentials is empty', function () {
+ tx.AcceptedCredentials = []
+ const errorMessage = 'PermissionedDomainSet: AcceptedCredentials array must not be empty'
+ assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage)
+ })Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| describe('PermissionedDomainSet', function () { | ||
| let tx | ||
|
|
||
| beforeEach(function () { | ||
|
|
||
| const sampleCredential: Credential = { | ||
| Credential: { | ||
| CredentialType: stringToHex('Passport'), | ||
| Issuer: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8' | ||
| } | ||
| } | ||
|
|
||
| tx = { | ||
| TransactionType: 'PermissionedDomainSet', | ||
| Account: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8', | ||
| DomainID: 'D88930B33C2B6831660BFD006D91FF100011AD4E67CBB78B460AF0A215103737', | ||
| AcceptedCredentials: [sampleCredential] | ||
| } as any | ||
| }) | ||
|
|
||
| it('verifies valid PermissionedDomainSet', function () { | ||
| assert.doesNotThrow(() => validatePermissionedDomainSet(tx)) | ||
| assert.doesNotThrow(() => validate(tx)) | ||
| }) | ||
|
|
||
| it(`throws with invalid field DomainID`, function () { | ||
| tx.DomainID = 1234 // DomainID is expected to be a string | ||
| const errorMessage = 'PermissionedDomainSet: invalid field DomainID' | ||
| assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage) | ||
| assert.throws(() => validate(tx), ValidationError, errorMessage) | ||
| }) | ||
|
|
||
| it(`throws w/ missing field AcceptedCredentials`, function () { | ||
| delete tx.AcceptedCredentials | ||
| const errorMessage = 'PermissionedDomainSet: missing field AcceptedCredentials' | ||
| assert.throws(() => validatePermissionedDomainSet(tx), ValidationError, errorMessage) | ||
| assert.throws(() => validate(tx), ValidationError, errorMessage) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for PermissionedDomainSet appears incomplete per XLS-80d requirements
The current test coverage has gaps when compared to the implementation requirements:
- Missing test for maximum AcceptedCredentials length (implementation enforces max 10)
- Missing test for empty AcceptedCredentials array validation
- Missing validation tests for Credential object structure
- Integration test only covers basic happy path with a single credential
The implementation shows these validations but corresponding test cases are absent in both unit and integration tests.
Recommended test additions:
- Test case for AcceptedCredentials array exceeding max length (10)
- Test case for empty AcceptedCredentials array
- Test cases for malformed Credential objects
- Integration test scenarios with multiple credentials
🔗 Analysis chain
Verify test coverage against XLS-80d specification requirements.
Please ensure that all requirements from the XLS-80d specification for PermissionedDomainSet transactions are covered in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files to analyze coverage
echo "Searching for related test files..."
fd -e ts -e js "permissioned.*test"
# Search for implementation details to cross-reference with tests
echo "Searching for implementation details..."
rg -l "PermissionedDomain" --type ts --type js
# Look for any TODO comments related to missing test coverage
echo "Checking for TODO comments..."
rg -i "todo.*test.*permission" --type ts --type js
Length of output: 1078
Script:
#!/bin/bash
# Let's analyze the implementation and test files to verify coverage
# First, let's look at the implementation details for PermissionedDomainSet
echo "=== PermissionedDomainSet Implementation ==="
cat packages/xrpl/src/models/transactions/permissionedDomainSet.ts
# Then check the integration tests
echo -e "\n=== Integration Tests ==="
cat packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
# Look at the model definition
echo -e "\n=== PermissionedDomain Model ==="
cat packages/xrpl/src/models/ledger/PermissionedDomain.ts
# Check other test cases
echo -e "\n=== PermissionedDomainDelete Tests ==="
cat packages/xrpl/test/models/permissionedDomainDelete.test.ts
Length of output: 6939
🧰 Tools
🪛 Biome
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| // Step-3: Test the PDDelete transaction | ||
| const tx_pd_delete: PermissionedDomainDelete = { | ||
| TransactionType: 'PermissionedDomainDelete', | ||
| Account: testContext.wallet.classicAddress, | ||
| // fetch the PD `index` from the previous account_objects RPC response | ||
| DomainID: pd.index | ||
| } | ||
|
|
||
| console.log(pd) | ||
|
|
||
| await testTransaction(testContext.client, tx_pd_delete, testContext.wallet) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify deletion success.
After deleting the PermissionedDomain, verify that it's actually removed by checking account_objects again.
await testTransaction(testContext.client, tx_pd_delete, testContext.wallet)
+
+// Verify the domain was deleted
+const resultAfterDelete = await testContext.client.request({
+ command: 'account_objects',
+ account: testContext.wallet.classicAddress,
+ type: 'permissioned_domain',
+})
+assert.equal(resultAfterDelete.result.account_objects.length, 0)📝 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.
| // Step-3: Test the PDDelete transaction | |
| const tx_pd_delete: PermissionedDomainDelete = { | |
| TransactionType: 'PermissionedDomainDelete', | |
| Account: testContext.wallet.classicAddress, | |
| // fetch the PD `index` from the previous account_objects RPC response | |
| DomainID: pd.index | |
| } | |
| console.log(pd) | |
| await testTransaction(testContext.client, tx_pd_delete, testContext.wallet) | |
| }, | |
| // Step-3: Test the PDDelete transaction | |
| const tx_pd_delete: PermissionedDomainDelete = { | |
| TransactionType: 'PermissionedDomainDelete', | |
| Account: testContext.wallet.classicAddress, | |
| // fetch the PD `index` from the previous account_objects RPC response | |
| DomainID: pd.index | |
| } | |
| console.log(pd) | |
| await testTransaction(testContext.client, tx_pd_delete, testContext.wallet) | |
| // Verify the domain was deleted | |
| const resultAfterDelete = await testContext.client.request({ | |
| command: 'account_objects', | |
| account: testContext.wallet.classicAddress, | |
| type: 'permissioned_domain', | |
| }) | |
| assert.equal(resultAfterDelete.result.account_objects.length, 0) | |
| }, |
High Level Overview of Change
This PR implements the XLS-80d specification titled "PermissionedDomains": XRPLF/XRPL-Standards#228
Here is the accompanying xrpl-py implementation: XRPLF/xrpl-py#773
This PR has the following dependencies. The final merge needs to await the below two PRs:
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
I've included unit and integration tests