PermissionedDomain XLS-80d #2874
Conversation
Integration tests Unit tests for PD; Update changelog update the rippled.cfg with latest amendments
WalkthroughThe changes expand support for PermissionedDomain functionality across several modules. In the codec package, new fields and type entries are added to JSON definitions. XRPL models and transactions now include the PermissionedDomain type with accompanying interfaces and validation functions for both set and delete operations. Updates to transaction utilities, tests, and configuration files further integrate these features, while constants and validation methods are refined for credentials handling. Documentation and Docker references have also been updated. Changes
Suggested reviewers
Poem
✨ Finishing Touches
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: 8
🧹 Nitpick comments (9)
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts (2)
8-12: Add JSDoc documentation for the interface.Add comprehensive documentation to explain the purpose and usage of the
PermissionedDomainDeleteinterface.+/** + * Interface for a transaction that deletes a permissioned domain. + * + * @see https://github.com/XRPLF/XRPL-Standards/discussions/80 + */ export interface PermissionedDomainDelete extends BaseTransaction {
14-18: Add JSDoc documentation for the validation function.Add documentation to explain the validation function's purpose, parameters, and potential errors.
+/** + * Validates a PermissionedDomainDelete transaction. + * + * @param tx - The transaction to validate. + * @throws ValidationError if the transaction is invalid. + */ export function validatePermissionedDomainDelete(tx: Record<string, unknown>): void {🧰 Tools
🪛 eslint
[error] 14-14: Replace
tx:·Record<string,·unknown>with⏎··tx:·Record<string,·unknown>,⏎(prettier/prettier)
🪛 GitHub Actions: Node.js CI
[warning] 14-14: Missing JSDoc comment and line length exceeds maximum of 80 characters
packages/xrpl/src/models/ledger/PermissionedDomain.ts (1)
10-24: Add comprehensive documentation for the PermissionedDomain interface.Add JSDoc documentation explaining the purpose, fields, and usage of the interface.
+/** + * Represents a permissioned domain in the XRP Ledger. + * + * @see https://github.com/XRPLF/XRPL-Standards/discussions/80 + * + * @property Owner - The account that owns this domain + * @property Flags - Domain flags (currently unused) + * @property OwnerNode - Internal node in the owner directory + * @property Sequence - The sequence number of this entry + * @property AcceptedCredentials - List of credentials accepted by this domain + */ export default interface PermissionedDomainpackages/xrpl/test/models/permissionedDomainDelete.test.ts (2)
27-32: Improve performance by avoiding the delete operator.Replace the delete operator with undefined assignment as recommended by static analysis:
- delete tx.DomainID + tx.DomainID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
[error] 30-30: Replace
()·=>·validatePermissionedDomainDelete(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainDelete(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
11-40: Enhance test coverage with additional test cases.Consider adding tests for:
- Invalid Account format
- Empty DomainID string
- Maximum length validation for DomainID
- Special characters in DomainID
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
[error] 18-18: Insert
⏎·······(prettier/prettier)
[error] 30-30: Replace
()·=>·validatePermissionedDomainDelete(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainDelete(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
[error] 37-37: Replace
()·=>·validatePermissionedDomainDelete(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainDelete(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
45-45: Enhance documentation for credential validation.The comment about not validating inner objects should explain why this is safe and reference the C++ implementation's validation logic.
- // Note: This implementation does not rigorously validate the inner-object format of AcceptedCredentials array because that would be a blatant repetition of the rippled cpp implementation. + // Note: Inner-object format validation of AcceptedCredentials is handled by the rippled C++ implementation + // (see: src/ripple/protocol/impl/STObject.cpp). Duplicating this validation here would be redundant + // and could potentially diverge from the authoritative implementation.packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
30-30: Avoid type casting with 'as any'.Type casting with 'as any' bypasses TypeScript's type checking. Consider creating a proper typed object.
- } as any + } as PermissionedDomainSetpackages/xrpl/src/models/transactions/transaction.ts (1)
77-78: Fix formatting according to eslintThe import statements should be formatted with line breaks for consistency.
-import { PermissionedDomainDelete, validatePermissionedDomainDelete } from './permissionedDomainDelete' -import { PermissionedDomainSet, validatePermissionedDomainSet } from './permissionedDomainSet' +import { + PermissionedDomainDelete, + validatePermissionedDomainDelete, +} from './permissionedDomainDelete' +import { + PermissionedDomainSet, + validatePermissionedDomainSet, +} from './permissionedDomainSet'🧰 Tools
🪛 eslint
[error] 77-77: Replace
·PermissionedDomainDelete,·validatePermissionedDomainDelete·with⏎··PermissionedDomainDelete,⏎··validatePermissionedDomainDelete,⏎(prettier/prettier)
[error] 78-78: Replace
·PermissionedDomainSet,·validatePermissionedDomainSet·with⏎··PermissionedDomainSet,⏎··validatePermissionedDomainSet,⏎(prettier/prettier)
packages/xrpl/HISTORY.md (1)
7-9: Consider adding more details to the changelog entryWhile the entry follows the correct format, it would be helpful to add more details about what the PermissionedDomain feature enables and any breaking changes or important notes for users.
### Added -* Implementation of XLS-80d PermissionedDomain feature. +* Implementation of XLS-80d PermissionedDomain feature: + * Added new transaction types: PermissionedDomainSet and PermissionedDomainDelete + * Added new ledger entry type: PermissionedDomain + * Added support for domain management with credential verification
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/ripple-binary-codec/src/enums/definitions.json(4 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
📓 Learnings (1)
packages/ripple-binary-codec/src/enums/definitions.json (1)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2826
File: packages/ripple-binary-codec/src/enums/definitions.json:33-57
Timestamp: 2024-11-14T16:41:13.360Z
Learning: When reviewing diffs in `definitions.json`, ensure to distinguish between entries that are duplicates and entries that are being moved or reordered to avoid false positives.
🪛 eslint
packages/xrpl/src/models/transactions/index.ts
[error] 109-109: Replace PermissionedDomainSet with ·PermissionedDomainSet·
(prettier/prettier)
[error] 110-110: Replace PermissionedDomainDelete with ·PermissionedDomainDelete·
(prettier/prettier)
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
[error] 5-5: Insert ,
(prettier/prettier)
[error] 14-14: Replace tx:·Record<string,·unknown> with ⏎··tx:·Record<string,·unknown>,⏎
(prettier/prettier)
packages/xrpl/src/models/transactions/transaction.ts
[error] 77-77: Replace ·PermissionedDomainDelete,·validatePermissionedDomainDelete· with ⏎··PermissionedDomainDelete,⏎··validatePermissionedDomainDelete,⏎
(prettier/prettier)
[error] 78-78: Replace ·PermissionedDomainSet,·validatePermissionedDomainSet· with ⏎··PermissionedDomainSet,⏎··validatePermissionedDomainSet,⏎
(prettier/prettier)
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 18-18: Insert ⏎·······
(prettier/prettier)
[error] 30-30: Replace ()·=>·validatePermissionedDomainDelete(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainDelete(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
[error] 37-37: Replace ()·=>·validatePermissionedDomainDelete(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainDelete(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 6-6: ../../src import should occur before import of ../../src/models/ledger/PermissionedDomain
(import/order)
[error] 17-18: Delete ⏎
(prettier/prettier)
[error] 21-21: Insert ,
(prettier/prettier)
[error] 22-22: Insert ,
(prettier/prettier)
[error] 28-28: Insert ⏎·······
(prettier/prettier)
[error] 29-29: Insert ,
(prettier/prettier)
[error] 41-41: Replace ()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
[error] 47-47: Insert ⏎·····
(prettier/prettier)
[error] 48-48: Replace ()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 11-11: ../ledger/PermissionedDomain import should occur before import of ./common
(import/order)
[error] 11-11: Replace Credential with ·Credential·
(prettier/prettier)
[error] 23-23: Replace tx:·Record<string,·unknown> with ⏎··tx:·Record<string,·unknown>,⏎
(prettier/prettier)
[error] 31-31: Replace 'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array' with ⏎········'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array',⏎······
(prettier/prettier)
[error] 38-39: Delete ⏎···
(prettier/prettier)
[error] 39-39: Expected '===' and instead saw '=='.
(eqeqeq)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
[error] 4-4: Replace ·LedgerEntryRequest,·PermissionedDomainDelete,·PermissionedDomainSet· with ⏎··LedgerEntryRequest,⏎··PermissionedDomainDelete,⏎··PermissionedDomainSet,⏎
(prettier/prettier)
[error] 5-5: Replace ·Credential· with ⏎··Credential,⏎
(prettier/prettier)
[error] 31-31: Insert ,
(prettier/prettier)
[error] 32-32: Insert ,
(prettier/prettier)
[error] 39-39: Insert ,
(prettier/prettier)
[error] 62-62: Insert ,
(prettier/prettier)
[error] 64-64: Replace ledger_entry_request with ⏎········ledger_entry_request,⏎······
(prettier/prettier)
[error] 72-72: Insert ,
(prettier/prettier)
[error] 77-77: Replace testContext.client,·tx_pd_delete,·testContext.wallet with ⏎········testContext.client,⏎········tx_pd_delete,⏎········testContext.wallet,⏎······
(prettier/prettier)
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/models/transactions/index.ts
[error] 109-110: Formatting errors with PermissionedDomainSet and PermissionedDomainDelete
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
[warning] 14-14: Missing JSDoc comment and line length exceeds maximum of 80 characters
packages/xrpl/test/models/permissionedDomainSet.test.ts
[warning] 39-39: Unexpected comment inline with code
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 11-11: Incorrect import order: '../ledger/PermissionedDomain' should be imported before './common'
[error] 39-39: Expected '===' and instead saw '=='
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
[warning] 75-75: Unexpected console statement
🪛 Biome (1.9.4)
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 (5)
packages/xrpl/src/models/ledger/PermissionedDomain.ts (1)
17-17: Verify the hardcoded Flags value.The
Flagsfield is hardcoded to0. This seems unusual as flags typically allow for different states.✅ Verification successful
Hardcoded Flags=0 is correct for PermissionedDomain
The PermissionedDomain ledger entry type, implemented as part of XLS-80d, intentionally has no configurable states. Integration tests explicitly verify this value, and the implementation shows no flag-based behavior variations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any flag definitions or usage patterns rg -A 5 "PermissionedDomain.*Flags" ast-grep --pattern 'const $_ = { $$$ Flags: $_ $$$ }'Length of output: 99
Script:
#!/bin/bash # Search for PermissionedDomain in docs and test files rg -g "*.md" -g "*.test.*" -g "*.spec.*" "PermissionedDomain" # Look for any PermissionedDomain related code fd PermissionedDomain # Search for flag definitions in ledger models rg -g "**/ledger/*.ts" "(?i)flags.*=|enum.*flags|flags.*{" -A 5 # Search for any mention of PermissionedDomain rg "PermissionedDomain" -A 5Length of output: 26214
packages/xrpl/src/models/ledger/LedgerEntry.ts (1)
16-16: LGTM!The changes correctly integrate the PermissionedDomain type into the ledger entry system while maintaining alphabetical ordering and consistency with existing patterns.
Also applies to: 39-39, 66-66
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
15-20: LGTM! Well-structured interface definition.The interface correctly extends BaseTransaction and defines the required fields.
packages/xrpl/src/models/transactions/transaction.ts (1)
156-157: LGTM! New transaction types properly integratedThe PermissionedDomain transaction types are correctly added to the SubmittableTransaction type and the validate() function.
Also applies to: 416-423
packages/ripple-binary-codec/src/enums/definitions.json (1)
1253-1262: LGTM! New fields and types properly definedThe additions follow the correct format and use unique identifiers:
- DomainID field (nth: 34, type: Hash256)
- AcceptedCredentials field (nth: 28, type: STArray)
- PermissionedDomain ledger entry type (130)
- PermissionedDomainSet (61) and PermissionedDomainDelete (62) transaction types
Also applies to: 2543-2551, 2885-2885, 3116-3117
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (3)
13-18: Well-structured interface definition.The interface follows TypeScript best practices with proper type definitions and clear optional/required field marking.
Consider documenting the purpose of each field with JSDoc comments for better developer experience.
export interface PermissionedDomainSet extends BaseTransaction { + /** Identifies this as a PermissionedDomainSet transaction */ TransactionType: 'PermissionedDomainSet' + /** Optional domain identifier */ DomainID?: string + /** List of accepted credentials for this domain */ AcceptedCredentials: Credential[] }
20-21: Improve eslint-disable comments.The eslint-disable comments should be more specific about why the rule violation is necessary:
-// eslint-disable-next-line max-lines-per-function -- necessary to validate many fields +// eslint-disable-next-line max-lines-per-function -- Complex validation logic requires multiple checks and error messagesAlso applies to: 26-26
🧰 Tools
🪛 eslint
[error] 21-21: Replace
tx:·Record<string,·unknown>with⏎··tx:·Record<string,·unknown>,⏎(prettier/prettier)
21-47: Robust validation implementation with proper error handling.The validation logic is well-structured with:
- Comprehensive array bounds checking
- Clear error messages
- Proper delegation of inner-object validation to rippled implementation (as confirmed by retrieved learnings)
Consider adding debug logging to help troubleshoot validation failures in production.
export function validatePermissionedDomainSet(tx: Record<string, unknown>): void { + const debug = require('debug')('xrpl:validation:permissionedDomainSet') + debug('Validating PermissionedDomainSet transaction:', tx) validateBaseTransaction(tx)🧰 Tools
🪛 eslint
[error] 21-21: Replace
tx:·Record<string,·unknown>with⏎··tx:·Record<string,·unknown>,⏎(prettier/prettier)
[error] 29-29: Replace
'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array'with⏎········'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array',⏎······(prettier/prettier)
[error] 36-37: Delete
⏎···(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/transactions/index.ts
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
🪛 eslint
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 1-1: There should be at least one empty line between import groups
(import/order)
[error] 10-10: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 10-10: ../ledger/PermissionedDomain import should occur before import of ./common
(import/order)
[error] 21-21: Replace tx:·Record<string,·unknown> with ⏎··tx:·Record<string,·unknown>,⏎
(prettier/prettier)
[error] 29-29: Replace 'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array' with ⏎········'PermissionedDomainSet:·AcceptedCredentials·must·be·an·array',⏎······
(prettier/prettier)
[error] 36-37: Delete ⏎···
(prettier/prettier)
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 1-43: Multiple issues: incorrect import ordering, missing empty lines, unused eslint-disable directives, line length violations, and formatting problems
🔇 Additional comments (2)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
11-11: LGTM! Well-defined constant.The constant follows naming conventions and sets a reasonable limit.
1-10:⚠️ Potential issueFix import organization.
Reorder imports and add empty lines between groups according to the style guide:
import { ValidationError } from '../../errors' + import { BaseTransaction, isString, validateBaseTransaction, validateOptionalField, validateRequiredField, } from './common' - -import { Credential } from '../ledger/PermissionedDomain' + +import { Credential } from '../ledger/PermissionedDomain'Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 1-1: There should be at least one empty line between import groups
(import/order)
[error] 10-10: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 10-10:
../ledger/PermissionedDomainimport should occur before import of./common(import/order)
🪛 GitHub Actions: Node.js CI
[error] 1-43: Multiple issues: incorrect import ordering, missing empty lines, unused eslint-disable directives, line length violations, and formatting problems
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
52-56: Add descriptive comments for assertions.Consider adding comments to explain what each assertion is validating. This will make the test more maintainable and easier to understand.
+ // Verify exactly one PermissionedDomain object was created assert.equal(result.result.account_objects.length, 1) const pd = result.result.account_objects[0] as PermissionedDomain + // Verify the default flags and credentials assert.equal(pd.Flags, 0) expect(pd.AcceptedCredentials).toEqual([sampleCredential])
31-32: Fix formatting issues.Add missing commas to maintain consistent formatting.
CredentialType: stringToHex('Passport'), - Issuer: testContext.wallet.classicAddress + Issuer: testContext.wallet.classicAddress, } Account: testContext.wallet.classicAddress, - AcceptedCredentials: [sampleCredential] + AcceptedCredentials: [sampleCredential], command: 'ledger_entry', - index: pd.index + index: pd.index, Account: testContext.wallet.classicAddress, - DomainID: pd.index + DomainID: pd.index,Also applies to: 39-39, 62-62, 72-72
🧰 Tools
🪛 eslint
[error] 31-31: Insert
,(prettier/prettier)
[error] 32-32: Insert
,(prettier/prettier)
packages/xrpl/test/models/permissionedDomainSet.test.ts (4)
1-7: Fix import order according to linting rules.Reorder imports to follow the project's linting rules:
+import { validate, ValidationError } from '../../src' import { stringToHex } from '@xrplf/isomorphic/dist/utils' import { assert } from 'chai' import { Credential } from '../../src/models/ledger/PermissionedDomain' import { validatePermissionedDomainSet } from '../../src/models/transactions/permissionedDomainSet' -import { validate, ValidationError } from '../../src'🧰 Tools
🪛 eslint
[error] 6-6:
../../srcimport should occur before import of../../src/models/ledger/PermissionedDomain(import/order)
8-12: Enhance test file documentation with XLS-80d context.Consider adding more context about the XLS-80d specification and the purpose of PermissionedDomainSet transactions in the documentation.
/** * PermissionedDomainSet Transaction Verification Testing. * * Providing runtime verification testing for each specific transaction type. + * + * This implements validation testing for XLS-80d PermissionedDomain feature, + * which allows domains to specify accepted credential types for authentication. + * Complex validation cases are handled by the rippled implementation. */
15-29: Fix formatting issues in sample data.Add trailing commas according to the project's style guide:
const sampleCredential: Credential = { Credential: { CredentialType: stringToHex('Passport'), - Issuer: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8' + Issuer: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8', } } beforeEach(function () { tx = { TransactionType: 'PermissionedDomainSet', Account: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8', DomainID: 'D88930B33C2B6831660BFD006D91FF100011AD4E67CBB78B460AF0A215103737', - AcceptedCredentials: [sampleCredential] + AcceptedCredentials: [sampleCredential], } as any })🧰 Tools
🪛 eslint
[error] 18-18: Insert
,(prettier/prettier)
[error] 19-19: Insert
,(prettier/prettier)
[error] 26-26: Insert
⏎·······(prettier/prettier)
[error] 27-27: Insert
,(prettier/prettier)
43-48: Optimize property removal in test.Replace the
deleteoperator with undefined assignment for better performance: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) })🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
[error] 45-45: Insert
⏎·····(prettier/prettier)
[error] 46-46: Replace
()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts(1 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
🪛 Biome (1.9.4)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 6-6: ../../src import should occur before import of ../../src/models/ledger/PermissionedDomain
(import/order)
[error] 18-18: Insert ,
(prettier/prettier)
[error] 19-19: Insert ,
(prettier/prettier)
[error] 26-26: Insert ⏎·······
(prettier/prettier)
[error] 27-27: Insert ,
(prettier/prettier)
[error] 39-39: Replace ()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
[error] 45-45: Insert ⏎·····
(prettier/prettier)
[error] 46-46: Replace ()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessage with ⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····
(prettier/prettier)
[error] 55-55: Insert ,
(prettier/prettier)
[error] 64-64: Insert ,
(prettier/prettier)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
[error] 4-4: Replace ·LedgerEntryRequest,·PermissionedDomainDelete,·PermissionedDomainSet· with ⏎··LedgerEntryRequest,⏎··PermissionedDomainDelete,⏎··PermissionedDomainSet,⏎
(prettier/prettier)
[error] 5-5: Replace ·Credential· with ⏎··Credential,⏎
(prettier/prettier)
[error] 31-31: Insert ,
(prettier/prettier)
[error] 32-32: Insert ,
(prettier/prettier)
[error] 39-39: Insert ,
(prettier/prettier)
[error] 62-62: Insert ,
(prettier/prettier)
[error] 64-64: Replace ledger_entry_request with ⏎········ledger_entry_request,⏎······
(prettier/prettier)
[error] 72-72: Insert ,
(prettier/prettier)
[error] 75-75: Replace testContext.client,·tx_pd_delete,·testContext.wallet with ⏎········testContext.client,⏎········tx_pd_delete,⏎········testContext.wallet,⏎······
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (4)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (3)
1-16: LGTM! Well-structured imports and timeout setup.The imports are comprehensive and the timeout value is appropriate for integration tests.
🧰 Tools
🪛 eslint
[error] 4-4: Replace
·LedgerEntryRequest,·PermissionedDomainDelete,·PermissionedDomainSet·with⏎··LedgerEntryRequest,⏎··PermissionedDomainDelete,⏎··PermissionedDomainSet,⏎(prettier/prettier)
[error] 5-5: Replace
·Credential·with⏎··Credential,⏎(prettier/prettier)
17-24: LGTM! Proper test suite setup with lifecycle hooks.The test suite follows best practices with proper setup and teardown of the test context.
25-78: LGTM! Well-structured lifecycle test.The test case thoroughly validates the PermissionedDomain object lifecycle through set, validate, and delete operations. The focus on happy path testing aligns with the approach mentioned in the retrieved learnings, as error cases are covered in the rippled C++ implementation.
🧰 Tools
🪛 eslint
[error] 31-31: Insert
,(prettier/prettier)
[error] 32-32: Insert
,(prettier/prettier)
[error] 39-39: Insert
,(prettier/prettier)
[error] 62-62: Insert
,(prettier/prettier)
[error] 64-64: Replace
ledger_entry_requestwith⏎········ledger_entry_request,⏎······(prettier/prettier)
[error] 72-72: Insert
,(prettier/prettier)
[error] 75-75: Replace
testContext.client,·tx_pd_delete,·testContext.walletwith⏎········testContext.client,⏎········tx_pd_delete,⏎········testContext.wallet,⏎······(prettier/prettier)
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
31-66: Test coverage strategy looks good.The test suite appropriately focuses on SDK-level validation, while leaving complex error cases (invalid credential format, duplicate credentials, non-existent DomainID) to the rippled implementation as per the project's testing strategy.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
[error] 39-39: Replace
()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
[error] 45-45: Insert
⏎·····(prettier/prettier)
[error] 46-46: Replace
()·=>·validatePermissionedDomainSet(tx),·ValidationError,·errorMessagewith⏎······()·=>·validatePermissionedDomainSet(tx),⏎······ValidationError,⏎······errorMessage,⏎····(prettier/prettier)
[error] 55-55: Insert
,(prettier/prettier)
[error] 64-64: Insert
,(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
67-67: Update Docker commands to use develop branch.The Docker commands have been updated to use the latest development version of rippled, which aligns with the CI workflow changes. This is necessary for testing the new PermissionedDomain feature.
Note: There's a minor markdown formatting inconsistency in the list style (asterisk vs dash). Consider updating for consistency.
Also applies to: 79-79, 95-95
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.ci-config/rippled.cfg(1 hunks).github/workflows/nodejs.yml(1 hunks)CONTRIBUTING.md(3 hunks)packages/ripple-binary-codec/src/enums/definitions.json(5 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
🪛 Markdownlint (0.37.0)
CONTRIBUTING.md
79-79: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: unit (22.x)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
.ci-config/rippled.cfg (1)
191-191: Verify the relevance of the DynamicNFT amendment.The addition of the
DynamicNFTamendment appears unrelated to the PR's objective of implementing the PermissionedDomain (XLS-80d) feature. Please clarify the relationship between DynamicNFT and PermissionedDomain, or consider submitting this amendment change in a separate PR.packages/ripple-binary-codec/src/enums/definitions.json (4)
1253-1262: LGTM! Field definition for DomainID.The field definition is well-structured with appropriate type (Hash256) and serialization flags for a unique identifier.
2543-2551: LGTM! Field definition for AcceptedCredentials.The field definition is well-structured with appropriate type (STArray) and serialization flags for storing multiple credentials.
2885-2885: LGTM! Ledger entry type for PermissionedDomain.The ledger entry type is correctly defined with a sequential value (130) that doesn't conflict with existing entries.
3117-3118: LGTM! Transaction types for PermissionedDomain operations.The transaction types are correctly defined:
- Sequential values (62, 63) without conflicts
- Names follow convention
- Appropriate operations (Set/Delete) for domain management
.github/workflows/nodejs.yml (1)
7-7: Verify the stability of the rippled develop branch for CI.Using the
developtag instead of a fixed version could potentially introduce instability in the CI pipeline. While this change aligns with testing the new PermissionedDomain feature, we should ensure the develop branch is stable.✅ Verification successful
Docker image usage is appropriate with sufficient stability safeguards
The current setup using
rippleci/rippled:developis well-implemented with multiple stability safeguards:
- Container health is actively monitored with
rippled server_infochecks- Failed health checks trigger container restart (10 retries, 5s intervals)
- Configuration is properly mounted for consistent environment
- Container cleanup is automated with
--rmflag🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the health and stability of the rippled develop branch. # Check if the Docker image exists and can be pulled docker pull rippleci/rippled:develop # Verify the image's health check docker run --rm --health-cmd="rippled server_info || exit 1" --health-interval=5s --health-retries=3 --health-timeout=2s rippleci/rippled:developLength of output: 380
Script:
#!/bin/bash # Search for Docker image references and version patterns rg -g '*.{yml,yaml}' 'rippleci/rippled:' -A 2 -B 2 # Check for documentation about Docker image versioning fd -g 'README*' -X cat {} # Look for any Docker-related configuration files fd -g 'Dockerfile*' -X cat {}Length of output: 33590
Script:
#!/bin/bash # Search for other workflow files and their Docker image usage rg -g '*.yml' -g '*.yaml' 'DOCKER_IMAGE|docker:' .github/workflows/ # Look for CI documentation fd -g 'CONTRIBUTING*' -g 'DEVELOPMENT*' -g 'docs/CI*' -g 'ci/*' -X cat {}Length of output: 1187
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/xrpl/src/models/transactions/common.ts (1)
505-505: Add JSDoc comment and consider performance optimization.
- Add JSDoc comment to comply with the style guide.
- Consider using a more efficient comparison method for large arrays.
+/** + * Check if an array of objects contains duplicates. + * + * @param objectList - The array of objects to check for duplicates. + * @returns True if duplicates are found, false otherwise. + */ export function containsDuplicates(objectList: object[]): boolean { - const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) - return objSet.size !== objectList.length + const seen = new Set(); + for (const obj of objectList) { + const key = JSON.stringify(obj); + if (seen.has(key)) return true; + seen.add(key); + } + return false; }🧰 Tools
🪛 GitHub Actions: Node.js CI
[warning] 505-505: Missing JSDoc comment
packages/xrpl/test/models/permissionedDomainSet.test.ts (2)
38-38: Move the inline comment to a separate line.To comply with the style guide, move the inline comment to its own line.
- tx.DomainID = 1234 // DomainID is expected to be a string + // DomainID is expected to be a string + tx.DomainID = 1234🧰 Tools
🪛 GitHub Actions: Node.js CI
[warning] 38-38: Unexpected comment inline with code
49-49: Replace delete operator with undefined assignment.For better performance, use undefined assignment instead of the delete operator.
- delete tx.AcceptedCredentials + tx.AcceptedCredentials = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(1 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[warning] 23-23: Missing JSDoc comment
packages/xrpl/src/models/transactions/common.ts
[warning] 505-505: Missing JSDoc comment
packages/xrpl/test/models/permissionedDomainSet.test.ts
[warning] 38-38: Unexpected comment inline with code
🪛 Biome (1.9.4)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/xrpl/src/models/transactions/common.ts (1)
137-139: LGTM!The function is correctly exported and properly validates the structure of an
AuthorizeCredential.packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
16-21: LGTM!The interface correctly extends BaseTransaction and defines the required fields.
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
32-104: LGTM! Test coverage aligns with SDK testing strategy.The test suite provides good coverage of the happy path and basic validation cases, which aligns with the SDK testing strategy. Complex error cases are handled by the rippled C++ implementation as noted in the retrieved learnings.
🧰 Tools
🪛 Biome (1.9.4)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Actions: Node.js CI
[warning] 38-38: Unexpected comment inline with code
|
A couple more cleanups, but LGTM after that. I really like the "lifecycle" test approach, it goes against the repo norm but it makes more sense for these kind of sequential transactions. In hindsight, this is what credentials tests in this repo should have done |
| export function validateCredentialsList( | ||
| credentials: unknown, | ||
| transactionType: string, | ||
| isStringID: boolean, | ||
| maxLengthCredentialsArray: number, |
There was a problem hiding this comment.
| maxLengthCredentialsArray: number, | |
| maxCredentials: number, |
| @@ -134,7 +134,9 @@ export function isIssuedCurrency( | |||
| * @param input - The input to check the form and type of | |||
| * @returns Whether the AuthorizeCredential is properly formed | |||
| */ | |||
| function isAuthorizeCredential(input: unknown): input is AuthorizeCredential { | |||
| export function isAuthorizeCredential( | |||
There was a problem hiding this comment.
This function's name is too specific now that it is also being used for PDs
There was a problem hiding this comment.
The name appropriately suggests the intention. Since PDSet transaction also uses AcceptedCredentials as the field name, users can understand the interpretation.
There was a problem hiding this comment.
Yes, it's AcceptedCredentials, not AuthorizeCredentials there. That's my point.
There was a problem hiding this comment.
@achowdhry-ripple If I rename this to AcceptedCredentials, will it confuse the Credential's usage? Should I rename it to isValidCredential instead?
There was a problem hiding this comment.
@mvadari I'd like to discuss this issue offline. I'll merge this PR at the moment because explorer changes need the client library
| validateCredentialsList, | ||
| } from './common' | ||
|
|
||
| const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10 |
There was a problem hiding this comment.
| const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10 | |
| const MAX_ACCEPTED_CREDENTIALS = 10 |
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (1)
41-47: Use camelCase for variable names.JavaScript conventions prefer camelCase over snake_case. Rename the variables:
tx_pd_set→txPdSettx_pd_delete→txPdDelete- const tx_pd_set: PermissionedDomainSet = { + const txPdSet: PermissionedDomainSet = { TransactionType: 'PermissionedDomainSet', Account: testContext.wallet.classicAddress, AcceptedCredentials: [sampleCredential], } - await testTransaction(testContext.client, tx_pd_set, testContext.wallet) + await testTransaction(testContext.client, txPdSet, testContext.wallet) - const tx_pd_delete: PermissionedDomainDelete = { + const txPdDelete: PermissionedDomainDelete = { TransactionType: 'PermissionedDomainDelete', Account: testContext.wallet.classicAddress, DomainID: pd.index, } await testTransaction( testContext.client, - tx_pd_delete, + txPdDelete, testContext.wallet, )Also applies to: 75-86
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
41-46: Replace delete operator with undefined assignment for better performance.Using the
deleteoperator can impact performance. Consider using undefined assignment instead.- delete tx.AcceptedCredentials + tx.AcceptedCredentials = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts(1 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
🪛 ESLint
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
[error] 82-86: Replace ⏎········testContext.client,⏎········txPDDelete,⏎········testContext.wallet,⏎······ with testContext.client,·txPDDelete,·testContext.wallet
(prettier/prettier)
🪛 Biome (1.9.4)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
1-21: LGTM!The imports and setup are well-organized and include all necessary dependencies.
22-29: LGTM! Test organization approach approved by team.The lifecycle-based test organization has been discussed and approved by the team as a better approach for sequential transactions, despite differing from the codebase's convention.
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
1-29: LGTM! Test setup follows best practices.The use of
as anyfor the tx object aligns with the team's preference for validation tests, as it better represents real-world scenarios where the validate method needs to handle arbitrary JSON input from users.
| @@ -51,6 +51,7 @@ export interface ServerStateResponse extends BaseResponse { | |||
| load_factor_fee_queue?: number | |||
| load_factor_fee_reference?: number | |||
| load_factor_server?: number | |||
| network_id: number | |||
There was a problem hiding this comment.
Please add this to the changelog as a fix
| @@ -116,6 +117,8 @@ describe('server_state', function () { | |||
| 'node_size', | |||
| 'initial_sync_duration_us', | |||
| 'ports', | |||
| 'git', | |||
| 'network_id', | |||
There was a problem hiding this comment.
Why does this need to be removed?
There was a problem hiding this comment.
The integration tests use a standalone docker instance of rippled. This rippled does not use a custom network_id field in its configuration. Hence, this field is not returned by server_state RPC.
There was a problem hiding this comment.
It is difficult to predict the hash and the branch of the git sub-field.
There was a problem hiding this comment.
git I understand, network_id is unnecessary.
There was a problem hiding this comment.
the tests won't pass without this ignore directive
There was a problem hiding this comment.
Alternatively, I'll have to update the rippled.cfg file
There was a problem hiding this comment.
@mvadari I'd like to discuss this issue offline. I'll merge this PR at the moment because explorer changes need the client library
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts
Outdated
Show resolved
Hide resolved
| /* The credentials that are accepted by the domain. | ||
| Ownership of one of these credentials automatically | ||
| makes you a member of the domain. */ | ||
| AcceptedCredentials: AuthorizeCredential[] |
There was a problem hiding this comment.
This type name is really confusing here - can we alias it or something?
There was a problem hiding this comment.
I had to use the AuthorizeCredential name because I imported it from the Credential-related file. Please check the earlier PR review comments from @achowdhry-ripple in this page
There was a problem hiding this comment.
Do you have a preference on the alias name?
There was a problem hiding this comment.
@mvadari I'd like to discuss this issue offline. I'll merge this PR at the moment because explorer changes need the client library
|
Hello all, I'd like to merge this PR soon. Please give another review of this PR. If you have pending concerns, I'll address them in a separate PR. |
….test.ts Co-authored-by: Mayukha Vadari <[email protected]>
….test.ts Co-authored-by: Mayukha Vadari <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xrpl/test/integration/requests/serverState.test.ts (1)
63-63: Consider making the network_id value configurable.The hardcoded value
63456fornetwork_idmight be problematic if the actual network ID changes. Consider making this configurable through test configuration or environment variables.- network_id: 63456, + network_id: process.env.TEST_NETWORK_ID || 63456,packages/xrpl/src/models/transactions/common.ts (1)
516-546: Improve duplicate detection robustness.The current implementation has a few areas for improvement:
- The key generation for nested objects could be more robust by using
JSON.stringifyto handle property order- The type guard check is redundant with the string check
export function containsDuplicates( objectList: AuthorizeCredential[] | string[], ): boolean { // Case-1: Process a list of string-IDs if (typeof objectList[0] === 'string') { const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) return objSet.size !== objectList.length } // Case-2: Process a list of nested objects const seen = new Set<string>() - if (isAuthorizeCredentialArray(objectList)) { - for (const item of objectList) { - const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}` - // eslint-disable-next-line max-depth -- necessary to check for type-guards - if (seen.has(key)) { - return true - } - seen.add(key) - } + for (const item of objectList) { + const key = JSON.stringify({ + issuer: item.Credential.Issuer, + type: item.Credential.CredentialType, + }) + if (seen.has(key)) { + return true + } + seen.add(key) } return false }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/methods/serverState.ts(1 hunks)packages/xrpl/src/models/transactions/accountDelete.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/src/models/transactions/depositPreauth.ts(2 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(2 hunks)packages/xrpl/src/models/transactions/payment.ts(2 hunks)packages/xrpl/src/models/transactions/paymentChannelClaim.ts(2 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(1 hunks)packages/xrpl/test/integration/requests/serverInfo.test.ts(1 hunks)packages/xrpl/test/integration/requests/serverState.test.ts(2 hunks)packages/xrpl/test/integration/transactions/permissionedDomain.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/xrpl/src/models/transactions/paymentChannelClaim.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/src/models/transactions/escrowFinish.ts
- packages/xrpl/src/models/transactions/accountDelete.ts
- packages/xrpl/src/models/transactions/depositPreauth.ts
🧰 Additional context used
📓 Learnings (3)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
packages/xrpl/src/models/transactions/common.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:32-33
Timestamp: 2025-01-17T09:27:33.335Z
Learning: Use `Array.isArray()` instead of `instanceof Array` for reliable array type checking in JavaScript, as `instanceof Array` fails for cross-realm arrays (e.g., from iframes) and arrays from different JavaScript contexts.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:14-14
Timestamp: 2025-01-14T20:47:34.164Z
Learning: The PermissionedDomain's AcceptedCredentials array has a maximum length of 10 as per XLS-80d specification, which is different from the credentials feature limit. These limits should not be aligned as they serve different purposes.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
🪛 Biome (1.9.4)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 42-42: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (14)
packages/xrpl/src/models/methods/serverState.ts (1)
54-54: LGTM! The addition ofnetwork_idfield is properly typed.The field is correctly added to the
ServerStateResponseinterface with the appropriate type.packages/xrpl/test/integration/requests/serverInfo.test.ts (1)
128-128: LGTM! Consistent handling of network_id in tests.The addition of
network_idtoremoveKeysis consistent with the changes inserverState.test.ts.packages/xrpl/HISTORY.md (1)
8-10: LGTM! Changes are well documented in the changelog.The changelog entries properly document both the XLS-80d implementation and the network_id field addition.
Also applies to: 15-16
packages/xrpl/src/models/transactions/common.ts (3)
12-12: LGTM: Import and constant changes.The addition of
MPTAmountto imports and renaming ofMAX_CREDENTIALS_LIST_LENGTHtoMAX_AUTHORIZED_CREDENTIALSwith export modifier looks good.Also applies to: 20-20
137-139: Consider renamingisAuthorizeCredentialfor broader applicability.The function name is too specific now that it's also being used for Permissioned Domains. Consider renaming to
isValidCredentialto better reflect its broader usage.
464-470: LGTM: Enhanced flexibility with dynamic max credentials.The addition of
maxCredentialsparameter improves the function's flexibility, allowing different limits for different transaction types (e.g., 10 for PermissionedDomainSet, 8 for others).Also applies to: 479-482
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (4)
12-12: LGTM!The constant follows naming conventions and the value 10 is correct as per XLS-80d specification.
14-25: LGTM!The interface is well-documented and properly structured with appropriate field types.
27-35: LGTM!The function signature and documentation are well-defined.
39-43:⚠️ Potential issueUse
Array.isArray()instead ofinstanceof Array.The
instanceof Arraycheck can fail for cross-realm arrays (e.g., from iframes) and arrays from different JavaScript contexts.Apply this diff:
- validateRequiredField( - tx, - 'AcceptedCredentials', - () => tx.AcceptedCredentials instanceof Array, - ) + validateRequiredField( + tx, + 'AcceptedCredentials', + () => Array.isArray(tx.AcceptedCredentials), + )Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (4)
1-20: LGTM!The imports are well-organized and the timeout value is appropriate for integration tests.
22-28: LGTM!The test suite setup properly manages test environment using setup and teardown hooks.
30-45: LGTM!The test case is well-structured with appropriate sample data.
47-85: LGTM!The test thoroughly covers the happy path for the PermissionedDomain lifecycle. Error cases are appropriately covered in the rippled C++ implementation (PR #5161) to avoid duplication across SDK implementations.
| // Type guard to ensure we're working with AuthorizeCredential[] | ||
| // Note: This is not a rigorous type-guard. A more thorough solution would be to iterate over the array and check each item. | ||
| function isAuthorizeCredentialArray( | ||
| list: AuthorizeCredential[] | string[], | ||
| ): list is AuthorizeCredential[] { | ||
| return typeof list[0] !== 'string' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type guard implementation.
The current type guard implementation is not rigorous as it only checks the first element. A more thorough solution would be to iterate over the array and check each item.
-function isAuthorizeCredentialArray(
+function isAuthorizeCredentialArray(
list: AuthorizeCredential[] | string[],
): list is AuthorizeCredential[] {
- return typeof list[0] !== 'string'
+ return list.length > 0 && list.every((item) => typeof item !== 'string')
}📝 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.
| // Type guard to ensure we're working with AuthorizeCredential[] | |
| // Note: This is not a rigorous type-guard. A more thorough solution would be to iterate over the array and check each item. | |
| function isAuthorizeCredentialArray( | |
| list: AuthorizeCredential[] | string[], | |
| ): list is AuthorizeCredential[] { | |
| return typeof list[0] !== 'string' | |
| } | |
| // Type guard to ensure we're working with AuthorizeCredential[] | |
| // Note: This is not a rigorous type-guard. A more thorough solution would be to iterate over the array and check each item. | |
| function isAuthorizeCredentialArray( | |
| list: AuthorizeCredential[] | string[], | |
| ): list is AuthorizeCredential[] { | |
| - return typeof list[0] !== 'string' | |
| + return list.length > 0 && list.every((item) => typeof item !== 'string') | |
| } |
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:
Reference C++ Implementation: XRPLF/rippled#5161
xrpl.js implementation of the Credentials feature: #2829
Context of Change
This PR implements the client library changes for two new transactions:
PermissionedDomainSetandPermissionedDomainDeleteType of Change
Did you update HISTORY.md?
Test Plan