PermissionedDomain XLS-80d #2874
Conversation
Integration tests Unit tests for PD; Update changelog update the rippled.cfg with latest amendments
WalkthroughThis pull request introduces the implementation of the XLS-80d PermissionedDomain feature for the XRPL JavaScript library. The changes include adding new ledger entry types, transaction types, and interfaces to support permissioned domains. Modifications span multiple files in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Learnings (2)📓 Common learningspackages/xrpl/src/models/transactions/permissionedDomainSet.ts (3)🪛 Biome (1.9.4)packages/xrpl/src/models/transactions/permissionedDomainSet.ts[error] 36-36: Use Array.isArray() instead of instanceof Array. instanceof Array returns false for array-like objects and arrays from other execution contexts. (lint/suspicious/useIsArray) ⏰ Context from checks skipped due to timeout of 90000ms (12)
🔇 Additional comments (4)
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 |
|
A bunch of linter issues failing |
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 (2)
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
28-37: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the delete operator with an undefined assignment.
- delete tx.DomainID + tx.DomainID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: 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 (1)
48-58: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the delete operator with an undefined assignment.
- 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 (8)
packages/xrpl/src/models/ledger/PermissionedDomain.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(2 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)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/xrpl/src/models/ledger/PermissionedDomain.ts
- packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
- packages/xrpl/src/models/transactions/permissionedDomainSet.ts
- packages/xrpl/src/models/transactions/common.ts
🧰 Additional context used
📓 Learnings (4)
📓 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.
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/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/permissionedDomainDelete.test.ts (3)
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.
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.
🪛 Biome (1.9.4)
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 29-29: 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] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (11)
packages/xrpl/test/models/permissionedDomainDelete.test.ts (2)
1-2: LGTM!The imports are appropriate for the test requirements.
14-21: LGTM!The test setup correctly uses generic JSON input (
as any) for validation testing, which aligns with the learned best practice for XRPL transaction validation tests.packages/xrpl/test/integration/transactions/permissionedDomain.test.ts (4)
1-18: LGTM!The imports and test setup are well-structured, with appropriate timeout configuration for integration tests.
33-45: LGTM!The credential setup and PermissionedDomainSet transaction construction are correct and well-documented.
49-72: LGTM!The validation steps are thorough, covering both account_objects and ledger_entry RPC methods. The assertions are comprehensive and well-structured.
75-86: LGTM!The PermissionedDomainDelete operation correctly uses the index from the previous response, completing the lifecycle test.
packages/xrpl/test/models/permissionedDomainSet.test.ts (2)
14-29: LGTM!The test setup is well-structured with a good sample credential and proper use of generic JSON input for validation testing.
96-103: LGTM!The invalid format test case provides good coverage for malformed credential objects.
packages/xrpl/src/models/transactions/transaction.ts (3)
77-84: LGTM!The imports for the new transaction types and their validation functions are properly organized.
162-163: LGTM!The new transaction types are correctly added to the SubmittableTransaction type union.
422-429: LGTM!The switch cases for the new transaction types are properly implemented and placed in alphabetical order.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
31-104: Add test case for string ID validation.The test suite provides good coverage. Consider adding a test case to verify that string IDs are rejected in the AcceptedCredentials array:
it('throws when AcceptedCredentials contains string ID', function () { tx.AcceptedCredentials = ['some_credential_id'] assert.throws( () => validatePermissionedDomainSet(tx), ValidationError, 'PermissionedDomainSet: Invalid Credentials format' ) })🧰 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)
packages/xrpl/src/models/transactions/common.ts (2)
Line range hint
460-480: Add JSDoc description for maxLengthCredentialsArray parameter.The JSDoc comment is missing the description for the
maxLengthCredentialsArrayparameter.Apply this diff to add the missing description:
/** * Check a CredentialAuthorize array for parameter errors * * @param credentials An array of credential IDs to check for errors * @param transactionType The transaction type to include in error messages * @param isStringID Toggle for if array contains IDs instead of AuthorizeCredential objects + * @param maxLengthCredentialsArray Maximum allowed length of the credentials array * @throws Validation Error if the formatting is incorrect */🧰 Tools
🪛 eslint
[error] 460-460: Missing JSDoc @param "maxLengthCredentialsArray" description.
(jsdoc/require-param-description)
507-513: Consider a more robust implementation for object comparison.The current implementation uses
JSON.stringifyfor object comparison which might have edge cases:
- Objects with the same properties in different orders will be considered different
- Objects with nested properties might not be compared correctly
Consider implementing a deep equality comparison function or using a library like Lodash's
isEqualfor more robust object comparison.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/common.ts(4 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.
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/src/models/transactions/permissionedDomainSet.ts (2)
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.
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/src/models/transactions/common.ts
[error] 460-460: Missing JSDoc @param "maxLengthCredentialsArray" description.
(jsdoc/require-param-description)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 1-1: 'lodash' should be listed in the project's dependencies, not devDependencies.
(import/no-extraneous-dependencies)
[error] 1-1: "lodash" is not published.
(node/no-unpublished-import)
🪛 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)
⏰ 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: integration (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
16-21: LGTM!The interface is well-structured and correctly defines the required fields for a PermissionedDomainSet transaction.
29-46: LGTM!The validation function is well-implemented with proper error handling and documentation.
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
1-29: LGTM!The test setup is well-structured with proper initialization and documentation. The use of
as anyfor type casting is appropriate here as it better represents real-world scenarios where thevalidatemethod needs to handle arbitrary JSON input from users.packages/xrpl/src/models/transactions/common.ts (1)
137-139: LGTM!The function is well-implemented with proper type checking and documentation.
| * @throws Validation Error if the formatting is incorrect | ||
| */ | ||
| // eslint-disable-next-line max-lines-per-function -- separating logic further will add unnecessary complexity | ||
| export function validateCredentialsList( | ||
| credentials: unknown, | ||
| transactionType: string, | ||
| isStringID: boolean, | ||
| maxLengthCredentialsArray: number = MAX_CREDENTIALS_LIST_LENGTH, |
There was a problem hiding this comment.
This shouldn't have a default value
There was a problem hiding this comment.
Do you expect this method to be used in other contexts? It has been predominantly used in the CredentialCreate validation
There was a problem hiding this comment.
Neither option really feels like a "default", and I wouldn't want a library developer to make a mistake in the future (and use the wrong value) because of that.
There was a problem hiding this comment.
ok, fixed in c619920
| * @param objectList - Array of objects to check for duplicates | ||
| * @returns True if duplicates exist, false otherwise | ||
| */ | ||
| export function containsDuplicates(objectList: object[]): boolean { | ||
| const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) |
There was a problem hiding this comment.
This will be incorrect if one has {Issuer: ..., CredentialType: ...} and the other has {CredentialType: ... Issuer: ...}
There was a problem hiding this comment.
thanks for pointing it out. How does this look ? da655b2
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
509-542:⚠️ Potential issueFix object comparison logic and improve code style.
- The current implementation doesn't handle cases where credential properties are in different orders.
- Several style improvements can be made.
Apply these changes:
export function containsDuplicates( objectList: AuthorizeCredential[] | string[], ): boolean { - let objSet: Set<string | object> // Case-1: Process a list of string-IDs if (typeof objectList[0] === 'string') { - objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) + const objSet = new Set(objectList) return objSet.size !== objectList.length - } else { - // Case-2: Process a list of nested objects - const seen = new Set<string>(); - for (const item of objectList) { - let key: string; - assert(typeof item === 'object') - assert('Credential' in item) - assert('Issuer' in item.Credential && 'CredentialType' in item.Credential) - key = `${item.Credential.Issuer}-${item.Credential.CredentialType}`; - - if (seen.has(key)) { - // Found a duplicate - return true; - } - - seen.add(key); - } - - return false } + + // Case-2: Process a list of nested objects + const seen = new Set<string>() + for (const item of objectList) { + assert(typeof item === 'object') + assert('Credential' in item) + assert('Issuer' in item.Credential && 'CredentialType' in item.Credential) + + // Sort properties to handle different property orders + const key = JSON.stringify({ + issuer: item.Credential.Issuer, + type: item.Credential.CredentialType + }) + + if (seen.has(key)) { + return true + } + seen.add(key) + } + return false }🧰 Tools
🪛 eslint
[error] 523-542: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 525-525: Delete
;(prettier/prettier)
[error] 527-527: Delete
;(prettier/prettier)
[error] 531-531: 'key' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 531-531: Delete
;(prettier/prettier)
[error] 535-535: Delete
;(prettier/prettier)
[error] 538-538: Delete
;(prettier/prettier)
🧹 Nitpick comments (1)
packages/xrpl/test/models/utils.test.ts (1)
31-63: Add more test cases for edge scenarios.The current tests cover basic functionality, but consider adding cases for:
- Empty arrays
- Arrays with different property orders in credentials
- Case sensitivity in string IDs
- Mixed case in issuer/credential type
Example additional test:
it('handles different property orders in credentials', function () { const list_with_same_credentials = [ { Credential: { Issuer: 'alice', CredentialType: 'Passport' } }, { Credential: { CredentialType: 'Passport', Issuer: 'alice' } } ] assert.isTrue(containsDuplicates(list_with_same_credentials)) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/test/models/utils.test.ts(2 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.
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/common.ts
[error] 18-18: 'chai' should be listed in the project's dependencies. Run 'npm i -S chai' to add it
(import/no-extraneous-dependencies)
[error] 18-18: chai import should occur before import of ripple-address-codec
(import/order)
[error] 18-18: "chai" is extraneous.
(node/no-extraneous-import)
[error] 523-542: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 525-525: Delete ;
(prettier/prettier)
[error] 527-527: Delete ;
(prettier/prettier)
[error] 531-531: 'key' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 531-531: Delete ;
(prettier/prettier)
[error] 535-535: Delete ;
(prettier/prettier)
[error] 538-538: Delete ;
(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 (2)
packages/xrpl/src/models/transactions/common.ts (2)
461-470: Remove default value from maxLengthCredentialsArray parameter.As discussed in previous reviews, neither value (8 or 10) feels like a true default. To prevent potential misuse, make the parameter required.
- maxLengthCredentialsArray: number = MAX_CREDENTIALS_LIST_LENGTH, + maxLengthCredentialsArray: number,
480-482: LGTM! Error message improvement.The dynamic error message clearly communicates the actual limit to users.
refactor: update the containsDuplicates method
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.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.
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/src/models/transactions/common.ts (2)
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.
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.
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
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.
🪛 eslint
packages/xrpl/src/models/transactions/common.ts
[error] 521-521: 'objSet' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 527-527: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
[error] 529-529: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 33-33: Replace tx,·'AcceptedCredentials',·()·=>·tx.AcceptedCredentials·instanceof·Array with ⏎····tx,⏎····'AcceptedCredentials',⏎····()·=>·tx.AcceptedCredentials·instanceof·Array,⏎··
(prettier/prettier)
🪛 Biome (1.9.4)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts
[error] 33-33: 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 (12)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (3)
1-12: LGTM!The imports are correctly ordered and the constant
ACCEPTED_CREDENTIALS_MAX_LENGTHis properly defined with the value 10 as per XLS-80d specification.
14-19: LGTM!The
PermissionedDomainSetinterface is well-structured with:
- Proper extension of
BaseTransaction- Correct type definitions for all fields
- Appropriate optional/required field markers
35-43: LGTM!The credential validation is well-implemented:
- Correctly uses the
validateCredentialsListhelper- Properly sets
isStringIDto false for nested objects only- Uses the correct maximum length of 10 as per XLS-80d specification
packages/xrpl/src/models/transactions/common.ts (2)
138-140: LGTM!The
isAuthorizeCredentialfunction is properly exported and includes comprehensive type checking.
Line range hint
461-482: LGTM!The
validateCredentialsListfunction is well-updated:
- Clear documentation for the new parameter
- Proper validation using the dynamic max length
- Accurate error messages
| validateOptionalField(tx, 'DomainID', isString) | ||
| validateRequiredField(tx, 'AcceptedCredentials', () => tx.AcceptedCredentials instanceof Array) |
There was a problem hiding this comment.
Use Array.isArray() instead of instanceof Array.
The instanceof Array check can return false for array-like objects and arrays from other execution contexts. Replace it with the more reliable Array.isArray().
Apply this diff:
- validateRequiredField(tx, 'AcceptedCredentials', () => tx.AcceptedCredentials instanceof Array)
+ validateRequiredField(tx, 'AcceptedCredentials', () => Array.isArray(tx.AcceptedCredentials))📝 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.
| validateOptionalField(tx, 'DomainID', isString) | |
| validateRequiredField(tx, 'AcceptedCredentials', () => tx.AcceptedCredentials instanceof Array) | |
| validateOptionalField(tx, 'DomainID', isString) | |
| validateRequiredField(tx, 'AcceptedCredentials', () => Array.isArray(tx.AcceptedCredentials)) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: 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)
🪛 eslint
[error] 33-33: Replace tx,·'AcceptedCredentials',·()·=>·tx.AcceptedCredentials·instanceof·Array with ⏎····tx,⏎····'AcceptedCredentials',⏎····()·=>·tx.AcceptedCredentials·instanceof·Array,⏎··
(prettier/prettier)
There was a problem hiding this comment.
could you give concrete examples where the proposed code fails?
There was a problem hiding this comment.
@ckeshava Here are concrete examples where instanceof Array fails but Array.isArray() works correctly:
// Example 1: Cross-realm arrays (e.g., arrays from iframes)
const iframe = document.createElement('iframe');
document.body.appendChild(iframe);
const arr = iframe.contentWindow.Array.prototype.slice.call([1,2,3]);
arr instanceof Array; // false (arr is from different realm)
Array.isArray(arr); // true
// Example 2: Array-like objects
const arrayLike = {
0: 'a',
1: 'b',
length: 2,
__proto__: Array.prototype
};
arrayLike instanceof Array; // false (despite inheriting from Array.prototype)
Array.isArray(arrayLike); // false (correctly identifies non-arrays)
// Example 3: Arrays from vm/node contexts
const vm = require('vm');
const otherArray = vm.runInNewContext('[]');
otherArray instanceof Array; // false (array from different context)
Array.isArray(otherArray); // trueThis is why Array.isArray() is the recommended approach - it correctly identifies arrays regardless of their origin context, while being immune to prototype chain manipulation.
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:32-33
Timestamp: 2025-01-17T09:27:33.183Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/xrpl/HISTORY.md (1)
13-14: Consider expanding the changelog entry with more details.While the entry correctly documents the addition of the XLS-80d feature, it would be helpful to briefly describe what the PermissionedDomain feature does and what functionality it adds. Consider adding a brief explanation or linking to the XLS-80d specification.
Example:
- * Implementation of XLS-80d PermissionedDomain feature. + * Implementation of XLS-80d PermissionedDomain feature, which enables domain-based permissioning for XRPL accounts. This adds new transaction types (PermissionedDomainSet, PermissionedDomainDelete) and ledger entry type (PermissionedDomain).🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: null
Multiple headings with the same content(MD024, no-duplicate-heading)
packages/xrpl/src/models/transactions/transaction.ts (2)
422-429: Remove extra newlines for consistency.While the validation logic is correct, the extra newlines around these cases break the consistent formatting with other cases in the switch statement.
case 'PaymentChannelFund': validatePaymentChannelFund(tx) break - case 'PermissionedDomainSet': validatePermissionedDomainSet(tx) break - case 'PermissionedDomainDelete': validatePermissionedDomainDelete(tx) break - case 'SetRegularKey':
Line range hint
77-429: Well-structured implementation of XLS-80d PermissionedDomain transactions.The changes effectively integrate the new transaction types while maintaining consistency with the existing codebase patterns. The implementation correctly delegates complex validation to the rippled C++ layer, as noted in the retrieved learnings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/test/models/utils.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/models/utils.test.ts
🧰 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.
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.
🪛 Markdownlint (0.37.0)
packages/xrpl/HISTORY.md
13-13: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ 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: integration (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: snippets (18.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 (3)
packages/xrpl/HISTORY.md (1)
13-14: LGTM on placement and formatting.The new entry is correctly placed under the "Unreleased" section and follows the established changelog format with bullet points under appropriate categories.
🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: null
Multiple headings with the same content(MD024, no-duplicate-heading)
packages/xrpl/src/models/transactions/transaction.ts (2)
77-84: LGTM! Import statements are well-organized.The new imports follow the established pattern and maintain alphabetical ordering.
162-163: LGTM! Transaction types properly added to union type.The new transaction types are correctly integrated into the
SubmittableTransactionunion type, maintaining alphabetical order.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/xrpl/src/models/transactions/accountDelete.ts (1)
10-10: Missing trailing comma in import listAdd a trailing comma after
MAX_CREDENTIALS_LIST_LENGTHto maintain consistency with the codebase style.- MAX_CREDENTIALS_LIST_LENGTH + MAX_CREDENTIALS_LIST_LENGTH,packages/xrpl/src/models/transactions/escrowFinish.ts (1)
10-10: Missing trailing comma in import listAdd a trailing comma after
MAX_CREDENTIALS_LIST_LENGTHto maintain consistency with the codebase style.- MAX_CREDENTIALS_LIST_LENGTH + MAX_CREDENTIALS_LIST_LENGTH,🧰 Tools
🪛 eslint
[error] 10-10: Insert
,(prettier/prettier)
packages/xrpl/src/models/transactions/depositPreauth.ts (1)
8-8: Missing trailing comma in import listAdd a trailing comma after
MAX_CREDENTIALS_LIST_LENGTHto maintain consistency with the codebase style.- MAX_CREDENTIALS_LIST_LENGTH + MAX_CREDENTIALS_LIST_LENGTH,🧰 Tools
🪛 eslint
[error] 8-8: Insert
,(prettier/prettier)
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (1)
Line range hint
1-1: Well-structured implementation of credential validationThe consistent application of
MAX_CREDENTIALS_LIST_LENGTHacross all transaction types demonstrates good architectural design. The approach aligns well with the established pattern of delegating complex error cases to the rippled C++ implementation while maintaining basic validation in the SDK.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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)
🧰 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.
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/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.
🪛 eslint
packages/xrpl/src/models/transactions/depositPreauth.ts
[error] 8-8: Insert ,
(prettier/prettier)
packages/xrpl/src/models/transactions/escrowFinish.ts
[error] 10-10: Insert ,
(prettier/prettier)
packages/xrpl/src/models/transactions/common.ts
[error] 470-470: Insert ,
(prettier/prettier)
[error] 521-521: 'objSet' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 527-527: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
[error] 529-529: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: browser (18.x)
- GitHub Check: snippets (18.x)
🔇 Additional comments (6)
packages/xrpl/src/models/transactions/accountDelete.ts (1)
58-58: LGTM! Good enhancement to credential validationThe addition of
MAX_CREDENTIALS_LIST_LENGTHproperly enforces the maximum length constraint for credential lists in AccountDelete transactions.packages/xrpl/src/models/transactions/escrowFinish.ts (1)
59-59: LGTM! Good enhancement to credential validationThe addition of
MAX_CREDENTIALS_LIST_LENGTHproperly enforces the maximum length constraint for credential lists in EscrowFinish transactions.packages/xrpl/src/models/transactions/depositPreauth.ts (1)
76-76: LGTM! Good enhancement to credential validationThe addition of
MAX_CREDENTIALS_LIST_LENGTHproperly enforces the maximum length constraint for both AuthorizeCredentials and UnauthorizeCredentials in DepositPreauth transactions.Also applies to: 84-84
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (1)
157-157: LGTM! Good enhancement to credential validationThe addition of
MAX_CREDENTIALS_LIST_LENGTHproperly enforces the maximum length constraint for credential lists in PaymentChannelClaim transactions.packages/xrpl/src/models/transactions/payment.ts (1)
16-16: LGTM!The changes correctly integrate the
MAX_CREDENTIALS_LIST_LENGTHconstant for validating the credentials list length.Also applies to: 192-192
packages/xrpl/src/models/transactions/common.ts (1)
17-17: LGTM!The changes to constants and function signatures are well-documented and properly implemented.
Also applies to: 21-21, 138-140, 461-462, 470-470, 480-482
| /** | ||
| * Check if an array of objects contains any duplicates. | ||
| * | ||
| * @param objectList - Array of objects to check for duplicates | ||
| * @returns True if duplicates exist, false otherwise | ||
| */ | ||
| export function containsDuplicates( | ||
| objectList: AuthorizeCredential[] | string[], | ||
| ): boolean { | ||
| // Case-1: Process a list of string-IDs | ||
| if (typeof objectList[0] === 'string') { | ||
| let objSet: Set<string> | ||
| 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>() | ||
| for (const item of objectList) { | ||
| // @ts-expect-error -- The previous if-block handles the string-inputs | ||
| item satisfies AuthorizeCredential['Credential'] | ||
| // @ts-expect-error -- The previous satisfies assertion checks the type requirements | ||
| const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}` | ||
|
|
||
| if (seen.has(key)) { | ||
| // Found a duplicate | ||
| return true | ||
| } | ||
|
|
||
| seen.add(key) | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the implementation of containsDuplicates.
The function has several areas for improvement:
- Use
constinstead ofletforobjSet - Replace unsafe type assertions with proper type narrowing
- Make the key generation more robust by using
JSON.stringifyto handle property order
Apply this diff:
export function containsDuplicates(
objectList: AuthorizeCredential[] | string[],
): boolean {
// Case-1: Process a list of string-IDs
if (typeof objectList[0] === 'string') {
- let objSet: Set<string>
- objSet = new Set(objectList.map((obj) => JSON.stringify(obj)))
+ 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>()
for (const item of objectList) {
- // @ts-expect-error -- The previous if-block handles the string-inputs
- item satisfies AuthorizeCredential['Credential']
- // @ts-expect-error -- The previous satisfies assertion checks the type requirements
- const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}`
+ if (!isAuthorizeCredential(item)) {
+ continue
+ }
+ const key = JSON.stringify({
+ issuer: item.Credential.Issuer,
+ type: item.Credential.CredentialType,
+ })
if (seen.has(key)) {
return true
}
seen.add(key)
}
return false
}📝 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.
| /** | |
| * Check if an array of objects contains any duplicates. | |
| * | |
| * @param objectList - Array of objects to check for duplicates | |
| * @returns True if duplicates exist, false otherwise | |
| */ | |
| export function containsDuplicates( | |
| objectList: AuthorizeCredential[] | string[], | |
| ): boolean { | |
| // Case-1: Process a list of string-IDs | |
| if (typeof objectList[0] === 'string') { | |
| let objSet: Set<string> | |
| 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>() | |
| for (const item of objectList) { | |
| // @ts-expect-error -- The previous if-block handles the string-inputs | |
| item satisfies AuthorizeCredential['Credential'] | |
| // @ts-expect-error -- The previous satisfies assertion checks the type requirements | |
| const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}` | |
| if (seen.has(key)) { | |
| // Found a duplicate | |
| return true | |
| } | |
| seen.add(key) | |
| } | |
| return false | |
| /** | |
| * Check if an array of objects contains any duplicates. | |
| * | |
| * @param objectList - Array of objects to check for duplicates | |
| * @returns True if duplicates exist, false otherwise | |
| */ | |
| 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>() | |
| for (const item of objectList) { | |
| if (!isAuthorizeCredential(item)) { | |
| continue | |
| } | |
| const key = JSON.stringify({ | |
| issuer: item.Credential.Issuer, | |
| type: item.Credential.CredentialType, | |
| }) | |
| if (seen.has(key)) { | |
| return true | |
| } | |
| seen.add(key) | |
| } | |
| return false | |
| } |
🧰 Tools
🪛 eslint
[error] 521-521: 'objSet' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 527-527: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
[error] 529-529: Do not use "@ts-expect-error" because it alters compilation errors.
(@typescript-eslint/ban-ts-comment)
There was a problem hiding this comment.
@ckeshava Thanks for implementing the improvements! The code is now more type-safe and uses proper constant declarations.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the specific commit changes
git show 2316945923a7b16ecfaaedc27cc33ded096dc64b
Length of output: 2403
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/xrpl/src/models/transactions/common.ts (3)
17-17: Remove empty line within import group.To maintain consistent import grouping, remove the empty line between imports.
import { onlyHasFields } from '../utils' - const MEMO_SIZE = 3
Line range hint
461-482: Consider grouping parameters into an options object.The function now has 4 parameters, exceeding the recommended limit of 3. Consider refactoring to use an options object for better maintainability.
-export function validateCredentialsList( - credentials: unknown, - transactionType: string, - isStringID: boolean, - maxLengthCredentialsArray: number -): void { +interface CredentialsValidationOptions { + credentials: unknown; + transactionType: string; + isStringID: boolean; + maxLength: number; +} + +export function validateCredentialsList(options: CredentialsValidationOptions): void { + const { credentials, transactionType, isStringID, maxLength } = options;🧰 Tools
🪛 eslint
[error] 470-470: Insert
,(prettier/prettier)
🪛 GitHub Actions: Node.js CI
[warning] 466-466: Function 'validateCredentialsList' has too many parameters (4). Maximum allowed is 3
[error] 470-470: Missing comma
265-265: Replace empty interface with type alias.The empty interface
GlobalFlagsserves no purpose. Replace it with a type alias.-export interface GlobalFlags { } +export type GlobalFlags = Record<string, never>;🧰 Tools
🪛 Biome (1.9.4)
[error] 265-265: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 eslint
[error] 265-265: Delete
·(prettier/prettier)
🪛 GitHub Actions: Node.js CI
[error] 265-265: Delete extra whitespace
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/common.ts(6 hunks)
🧰 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.
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/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.
🪛 Biome (1.9.4)
packages/xrpl/src/models/transactions/common.ts
[error] 265-265: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 eslint
packages/xrpl/src/models/transactions/common.ts
[error] 265-265: Delete ·
(prettier/prettier)
[error] 470-470: Insert ,
(prettier/prettier)
[error] 529-529: 'isAuthorizeCredential' is already declared in the upper scope on line 138 column 17.
(@typescript-eslint/no-shadow)
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/models/transactions/common.ts
[error] 7-7: There should be no empty line within import group
[error] 265-265: Delete extra whitespace
[warning] 466-466: Function 'validateCredentialsList' has too many parameters (4). Maximum allowed is 3
[error] 470-470: Missing comma
[warning] 529-529: Expected a function declaration
[error] 529-529: 'isAuthorizeCredential' is already declared in the upper scope on line 138 column 17
[warning] 538-538: Blocks are nested too deeply (3). Maximum allowed is 2
🔇 Additional comments (2)
packages/xrpl/src/models/transactions/common.ts (2)
138-140: LGTM! Clean export of validation function.The function is well-structured and correctly validates the AuthorizeCredential format.
535-543: 🛠️ Refactor suggestionSimplify nested conditions and improve key generation.
The implementation can be simplified to reduce nesting and make the key generation more robust:
- Deep nesting makes the code harder to follow
- Current key generation might have edge cases with different property orders
- if (isAuthorizeCredential(objectList)) { - for (const item of objectList) { - const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}` - if (seen.has(key)) { - return true - } - seen.add(key) - } - } - - return false + if (!isAuthorizeCredentialArray(objectList)) { + return false + } + + return objectList.some((item) => { + const key = JSON.stringify({ + issuer: item.Credential.Issuer, + type: item.Credential.CredentialType + }) + if (seen.has(key)) { + return true + } + seen.add(key) + return false + })Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[warning] 538-538: Blocks are nested too deeply (3). Maximum allowed is 2
| const isAuthorizeCredential = ( | ||
| list: AuthorizeCredential[] | string[], | ||
| ): list is AuthorizeCredential[] => { | ||
| return typeof list[0] !== 'string' | ||
| } |
There was a problem hiding this comment.
Rename inner type guard to avoid naming conflict.
The inner type guard shadows the exported isAuthorizeCredential function. Rename it to be more specific.
- const isAuthorizeCredential = (
+ const isAuthorizeCredentialArray = (
list: AuthorizeCredential[] | string[],
): list is AuthorizeCredential[] => {
return typeof list[0] !== '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.
| const isAuthorizeCredential = ( | |
| list: AuthorizeCredential[] | string[], | |
| ): list is AuthorizeCredential[] => { | |
| return typeof list[0] !== 'string' | |
| } | |
| const isAuthorizeCredentialArray = ( | |
| list: AuthorizeCredential[] | string[], | |
| ): list is AuthorizeCredential[] => { | |
| return typeof list[0] !== 'string' | |
| } |
🧰 Tools
🪛 eslint
[error] 529-529: 'isAuthorizeCredential' is already declared in the upper scope on line 138 column 17.
(@typescript-eslint/no-shadow)
🪛 GitHub Actions: Node.js CI
[warning] 529-529: Expected a function declaration
[error] 529-529: 'isAuthorizeCredential' is already declared in the upper scope on line 138 column 17
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
Type of Change
Did you update HISTORY.md?
Test Plan