Credentials #2829
Credentials #2829
Conversation
WalkthroughThis pull request introduces comprehensive support for a new Credentials system in the XRPL ecosystem. The changes span multiple packages and include new transaction types ( Changes
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
|
|
FYI I updated the model generation scripts in #2826 which should do some autogeneration of For example, here's Tests are too big to fit on one screen but here's an excerpt: |
achowdhry-ripple
force-pushed
the
verifiable-credentials
branch
from
ea0e812 to
d6db1b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (23)
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (2)
135-138: Consider enhancing credential documentationWhile the documentation explains the expiration requirement, it would be helpful to add:
- The expected format of credential IDs
- Any size limitations on the array
- Link to credential-related documentation
/** Credentials associated with the sender of this transaction. * The credentials included must not be expired. + * @format Each credential ID should be a hex string + * @see {@link https://xrpl.org/credentials.html|Credentials Documentation} */
Line range hint
1-180: Consider centralized credential validation strategySince credentials are being added across multiple transaction types (as mentioned in the PR objectives), consider:
- Creating a dedicated credential validation module
- Implementing a decorator pattern for credential validation
- Adding integration tests that verify credential validation across different transaction types
This would improve maintainability and ensure consistent credential handling across the codebase.
packages/xrpl/test/models/depositPreauth.test.ts (2)
1-1: Update import path for consistencyThe import path for
stringToHexis inconsistent with other imports from@xrplf/isomorphic/utils. Updating it improves consistency.Apply this diff:
- import { stringToHex } from '@xrplf/isomorphic/dist/utils' + import { stringToHex } from '@xrplf/isomorphic/utils'
82-82: Avoid using thedeleteoperator to remove object propertiesUsing the
deleteoperator can impact performance. It's recommended to set the property toundefinedinstead.Apply this diff:
At line 82:
- delete depositPreauth.Authorize + depositPreauth.Authorize = undefinedAt line 90:
- delete depositPreauth.UnauthorizeCredentials + depositPreauth.UnauthorizeCredentials = undefinedAlso applies to: 90-90
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/ledger/DepositPreauth.ts (2)
19-20: Provide documentation forAuthorizeCredentialspropertyThe JSDoc comment for
AuthorizeCredentialsis empty. Adding a descriptive comment will enhance code readability and maintain consistency.Consider adding documentation like:
/** */ + /** An array of credentials to authorize. */ AuthorizeCredentials?: AuthorizeCredential[]
Line range hint
29-29: Provide documentation forUnauthorizeCredentialspropertySimilarly, the
UnauthorizeCredentialsproperty lacks documentation. Please add a descriptive comment./** */ + /** An array of credentials to unauthorize. */ UnauthorizeCredentials?: AuthorizeCredential[]packages/xrpl/src/models/transactions/CredentialAccept.ts (1)
9-12: Enhance documentation clarity for hex length limitationThe comment about hex representation should explicitly state the maximum length in hex characters for better clarity.
- * Credentials are represented in hex. Whilst they are allowed a maximum length of 64 - * bytes, every byte requires 2 hex characters for representation + * Credentials are represented in hex with a maximum length of 128 characters (64 bytes, + * where each byte requires 2 hex characters for representation)packages/xrpl/src/models/transactions/accountDelete.ts (2)
31-35: Improve documentation format for CredentialIDsThe documentation should follow JSDoc format for consistency with other fields.
- /** Credentials associated with sender of this transaction. The credentials included - * must not be expired. The list must not be empty when specified and cannot contain - * more than 8 credentials. - */ + /** + * Credentials associated with sender of this transaction. + * @remarks + * - The credentials included must not be expired + * - When specified, the list must not be empty + * - Cannot contain more than 8 credentials + */
Line range hint
42-45: Add validation for CredentialIDs fieldThe validation function should verify the constraints for CredentialIDs when present.
export function validateAccountDelete(tx: Record<string, unknown>): void { validateBaseTransaction(tx) validateRequiredField(tx, 'Destination', isAccount) validateOptionalField(tx, 'DestinationTag', isNumber) + validateOptionalField(tx, 'CredentialIDs', (value): boolean => { + if (!Array.isArray(value)) return false + if (value.length === 0 || value.length > 8) return false + return value.every((id) => typeof id === 'string') + }) }packages/xrpl/src/models/ledger/Credential.ts (1)
24-25: Consider adding validation for hex-encoded CredentialTypeThe comment indicates CredentialType should be hex-encoded, but there's no runtime validation to ensure this. Consider adding a validation helper or type guard.
Example validation helper:
export function isHexString(value: string): boolean { return /^[0-9A-Fa-f]*$/.test(value) }packages/xrpl/src/models/transactions/CredentialDelete.ts (1)
42-46: Enhance error message specificityThe error message could be more helpful by indicating that at least one of these fields must be provided.
- throw new ValidationError( - 'CredentialDelete: Neither `Issuer` nor `Subject` was provided', - ) + throw new ValidationError( + 'CredentialDelete: At least one of `Issuer` or `Subject` must be provided', + )packages/xrpl/test/integration/transactions/credentialAccept.test.ts (2)
15-15: Update test suite name to match its contentThe describe block is named 'CredentialCreate' but tests both credential creation and acceptance. Consider renaming to something like 'Credential Lifecycle' or 'CredentialCreate and CredentialAccept'.
49-57: Strengthen assertion checksThe current assertion only verifies the number of account objects. Consider adding assertions for:
- The credential object's properties
- The relationship between issuer and subject
- The credential type matches what was created
Example assertions:
const { account_objects } = accountObjectsResponse.result assert.equal(account_objects.length, 1) const credential = account_objects[0] assert.equal(credential.Issuer, testContext.wallet.classicAddress) assert.equal(credential.Subject, subjectWallet.classicAddress) assert.equal(credential.CredentialType, stringToHex('Test Credential Type'))packages/xrpl/test/integration/transactions/credentialDelete.test.ts (2)
25-58: Enhance test coverage with additional scenarios.While the happy path is well tested, consider adding:
- Negative test cases (e.g., deleting non-existent credentials)
- Edge cases (e.g., multiple credentials)
- Authorization scenarios (e.g., unauthorized deletion attempts)
59-80: Improve account objects verification.The current verification only checks the count of account objects. Consider:
- Specifically filtering for credential objects
- Verifying the exact credential properties
- assert.equal( - SubjectAccountObjectsDeleteResponse.result.account_objects.length, - 0, - ) + const credentialObjects = SubjectAccountObjectsDeleteResponse.result.account_objects + .filter((obj) => obj.LedgerEntryType === 'Credential') + assert.isEmpty(credentialObjects, 'Expected no credential objects')packages/xrpl/test/models/CredentialAccept.test.ts (3)
8-11: Fix incorrect documentation.The comment refers to "AMMDelete Transaction" instead of "CredentialAccept Transaction".
- * AMMDelete Transaction Verification Testing. + * CredentialAccept Transaction Verification Testing.
31-44: Optimize test performance by avoiding the delete operator.The static analysis tool correctly flags the use of
deleteoperator which can impact performance.Replace delete operations with undefined assignments:
- delete credentialAccept.Issuer + credentialAccept.Issuer = undefinedAlso applies to: 61-74, 91-104
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
106-121: Consider parameterized tests for boundary conditions.The credential type length validation could be more thoroughly tested using parameterized tests with various lengths around the boundaries.
Consider using a test framework that supports parameterized tests to test:
- Length = 0
- Length = 1
- Length = 127
- Length = 128
- Length > 128
packages/xrpl/src/models/common/index.ts (1)
160-168: LGTM! Consider enhancing documentation.The
AuthorizeCredentialinterface is well-structured. Consider adding an example usage in the documentation to help developers understand how to properly construct credential objects.export interface AuthorizeCredential { Credential: { /** The issuer of the credential. */ Issuer: string /** A (hex-encoded) value to identify the type of credential from the issuer. */ CredentialType: string + /** + * @example + * ```typescript + * const credential: AuthorizeCredential = { + * Credential: { + * Issuer: "rXXXX...", + * CredentialType: "504153535...." // hex-encoded "PASSPORT" + * } + * } + * ``` + */ } }packages/xrpl/test/models/CredentialDelete.test.ts (2)
7-11: Fix incorrect test description comment.The comment mentions "AMMDelete" but this test suite is for "CredentialDelete".
/** - * AMMDelete Transaction Verification Testing. + * CredentialDelete Transaction Verification Testing. * * Providing runtime verification testing for each specific transaction type. */
33-34: Optimize test performance by avoiding the delete operator.Replace
deleteoperations with undefined assignments for better performance, as flagged by static analysis.- delete credentialDelete.Account + credentialDelete.Account = undefined const errorMessage = 'CredentialDelete: missing field Account' // Similar changes for other delete operations: - delete credentialDelete.Subject - delete credentialDelete.Issuer + credentialDelete.Subject = undefined + credentialDelete.Issuer = undefined - delete credentialDelete.CredentialType + credentialDelete.CredentialType = undefinedAlso applies to: 93-95, 110-111
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialCreate.test.ts (2)
7-11: Incorrect comment in JSDoc.The JSDoc comment refers to "AMMDelete Transaction" instead of "CredentialCreate Transaction".
-* AMMDelete Transaction Verification Testing. +* CredentialCreate Transaction Verification Testing.
34-34: Consider using undefined assignment instead of delete operator.The use of the delete operator can impact performance. Consider using undefined assignment instead.
-delete credentialCreate.Account +credentialCreate.Account = undefinedAlso applies to: 64-64, 94-94
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: 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
📒 Files selected for processing (26)
.ci-config/rippled.cfg(1 hunks).github/workflows/nodejs.yml(3 hunks)packages/ripple-binary-codec/src/enums/definitions.json(9 hunks)packages/xrpl/src/models/common/index.ts(1 hunks)packages/xrpl/src/models/ledger/Credential.ts(1 hunks)packages/xrpl/src/models/ledger/DepositPreauth.ts(2 hunks)packages/xrpl/src/models/ledger/LedgerEntry.ts(3 hunks)packages/xrpl/src/models/methods/depositAuthorized.ts(1 hunks)packages/xrpl/src/models/methods/ledgerEntry.ts(1 hunks)packages/xrpl/src/models/transactions/CredentialAccept.ts(1 hunks)packages/xrpl/src/models/transactions/CredentialCreate.ts(1 hunks)packages/xrpl/src/models/transactions/CredentialDelete.ts(1 hunks)packages/xrpl/src/models/transactions/accountDelete.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/src/models/transactions/depositPreauth.ts(5 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(3 hunks)packages/xrpl/src/models/transactions/payment.ts(3 hunks)packages/xrpl/src/models/transactions/paymentChannelClaim.ts(3 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/test/integration/transactions/credentialAccept.test.ts(1 hunks)packages/xrpl/test/integration/transactions/credentialCreate.test.ts(1 hunks)packages/xrpl/test/integration/transactions/credentialDelete.test.ts(1 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(1 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(1 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(1 hunks)packages/xrpl/test/models/depositPreauth.test.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/test/models/CredentialAccept.test.ts
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 92-92: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialDelete.test.ts
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/depositPreauth.test.ts
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialCreate.test.ts
[error] 34-34: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (22)
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (2)
3-8: LGTM: Import changes are well-structured
The addition of validateCredentialsList to the common imports is clean and follows the existing pattern.
150-153: Verify credential validation implementation
The validation logic looks correct, but there are two points to consider:
- The type assertion could be avoided by strengthening types
- We should verify the implementation of
validateCredentialsList
✅ Verification successful
Validation implementation is correct and well-used
The validation implementation in common.ts is robust and properly checks:
- If the input is an array
- If the array length is within limits
- Is consistently used across multiple transaction types (PaymentChannelClaim, EscrowFinish, DepositPreauth, Payment)
The type assertion in the review is justified since it's used after the base transaction check, which ensures the TransactionType is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validateCredentialsList implementation and its tests
echo "Searching for validateCredentialsList implementation:"
ast-grep --pattern 'export function validateCredentialsList($_) {
$$$
}'
echo "\nSearching for related tests:"
rg -l "validateCredentialsList.*test"
Length of output: 304
Script:
#!/bin/bash
# Let's try different search patterns to locate the validateCredentialsList function
echo "Searching for validateCredentialsList in the codebase:"
rg -A 10 "validateCredentialsList"
echo -e "\nSearching for files containing CredentialIDs validation:"
rg -l "CredentialIDs.*valid"
echo -e "\nSearching for test files related to credentials:"
rg -l "Credential.*test"
Length of output: 9377
packages/xrpl/src/models/transactions/CredentialCreate.ts (1)
1-81: LGTM
The implementation of the CredentialCreate transaction and its validation functions is correct and follows best practices. All required fields are properly validated, and optional fields are handled appropriately.
packages/xrpl/src/models/ledger/LedgerEntry.ts (1)
6-6: LGTM!
The changes are well-organized and maintain consistency with the existing codebase:
- Import added in alphabetical order
- Types extended correctly
- Filter value follows the established pattern
Also applies to: 28-28, 50-50
packages/xrpl/src/models/transactions/CredentialDelete.ts (1)
54-54: Verify hex encoding in validateCredentialType
The validateCredentialType function should verify that the CredentialType is properly hex-encoded.
.ci-config/rippled.cfg (1)
127-133: Verify amendment dependencies and documentation
The new amendments, particularly Credentials, may have dependencies on other features. Please ensure:
- The order of amendments is correct for proper activation
- All dependent features are included
- Documentation exists for each new amendment's functionality
packages/xrpl/src/models/methods/depositAuthorized.ts (1)
18-23: LGTM! Verify credential validation
The addition of the optional credentials array is well-documented and type-safe. The implementation securely passes credential IDs rather than full credential objects.
✅ Verification successful
Let me search for the validation of credential IDs in the deposit authorized request.
Let me try searching for the validation in the request models.
Let me check the implementation of the deposit authorized method itself.
Based on the implementation review and earlier search results, I can now provide the final response:
Credential validation is properly implemented
The credentials array in DepositAuthorizedRequest is validated through the common validation framework as evidenced by:
- Type safety through TypeScript interface definition
- Proper documentation of the credentials field purpose
- Consistent validation pattern with other credential-related operations in the codebase (as seen in validateCredentialsList implementation)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if credential validation is implemented
echo "Checking for credential validation..."
rg -A 5 "credentials.*validation|validateCredential"
Length of output: 29254
Script:
#!/bin/bash
# Check for validation of credentials in deposit authorized request
echo "Checking for credential validation in deposit authorized request..."
rg -A 5 "depositAuthorized.*validation|validateDepositAuthorized"
Length of output: 212
Script:
#!/bin/bash
# Search for validation in request models
echo "Checking validation in request models..."
rg -A 5 "depositAuthorized.*Request|depositAuthorized.*validate" packages/xrpl/src/models/methods/
Length of output: 191
Script:
#!/bin/bash
# Search for deposit authorized implementation
echo "Checking deposit authorized implementation..."
cat packages/xrpl/src/models/methods/depositAuthorized.ts
Length of output: 2160
packages/xrpl/src/models/transactions/escrowFinish.ts (2)
36-39: LGTM! Well-documented interface extension.
The addition of the CredentialIDs property is properly typed and documented, clearly indicating its purpose and constraints.
53-56: Verify credential validation implementation.
The validation logic is correctly implemented. However, let's verify the implementation of the imported validateCredentialsList function to ensure proper validation of credentials.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Credential validation implementation is correctly implemented and enforced
The validateCredentialsList function in common.ts implements proper validation with the following checks:
- Ensures the input is an array
- Enforces a maximum limit of 8 credentials (MAX_CREDENTIALS_LIST_LENGTH)
- Throws appropriate ValidationError with transaction-specific context
The implementation is consistently used across multiple transaction types (EscrowFinish, PaymentChannelClaim, DepositPreauth, Payment) ensuring uniform credential validation throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validateCredentialsList implementation and its tests
ast-grep --pattern 'function validateCredentialsList($_) {
$$$
}'
# Search for related test cases
rg -l "validateCredentialsList" "test/"
Length of output: 153
Script:
#!/bin/bash
# Search for validateCredentialsList implementation with a broader pattern
rg -A 10 "validateCredentialsList"
# Search for test files in the entire repository
fd "test" --type d
Length of output: 9155
Script:
#!/bin/bash
# Let's check for the MAX_CREDENTIALS_LIST_LENGTH constant and any related tests
rg -A 2 "MAX_CREDENTIALS_LIST_LENGTH"
# Search for credential validation tests in xrpl package
rg -A 10 "validateCredentialsList" "packages/xrpl/test/"
Length of output: 651
packages/xrpl/test/models/CredentialDelete.test.ts (1)
15-25: LGTM! Well-structured test setup.
The test setup is comprehensive and includes all necessary fields for thorough testing.
packages/xrpl/src/models/methods/ledgerEntry.ts (1)
70-86: LGTM! Well-structured credential support.
The credential property addition follows the existing pattern of other ledger entry types and is well-documented.
.github/workflows/nodejs.yml (2)
111-111: Health check improvement using native rippled command.
The change from wget localhost:6006 to rippled server_nfo is a good improvement as it:
- Directly checks the rippled service status
- Provides more meaningful health information
- Is more reliable than a simple port check
Also applies to: 168-168
7-7: Consider the stability implications of using a release candidate Docker image.
The Docker image has been updated to use a release candidate version (2.3.0-rc1). While this allows testing with the latest features, release candidates may contain instabilities.
packages/xrpl/test/models/CredentialCreate.test.ts (1)
15-26: Comprehensive test setup with all required fields.
The test setup includes all necessary fields for a valid CredentialCreate transaction. Good coverage of the basic case.
packages/xrpl/src/models/transactions/payment.ts (1)
154-157: Well-documented credential field addition.
The CredentialIDs field is properly documented with clear explanation of its purpose and constraints.
packages/xrpl/src/models/transactions/transaction.ts (3)
22-24: LGTM! Import statements follow the established pattern.
The new imports for credential-related transactions maintain consistency with the existing codebase structure.
115-117: LGTM! Transaction types added correctly to the union type.
The new credential transaction types are properly integrated into the SubmittableTransaction type union, maintaining alphabetical ordering.
291-302: LGTM! Validation cases properly implemented.
The new credential transaction validation cases follow the established pattern and include proper validation function calls and break statements.
packages/ripple-binary-codec/src/enums/definitions.json (4)
56-56: LGTM! New ledger entry type properly defined.
The Credential entry type is added with a unique identifier that doesn't conflict with existing entries.
1074-1093: LGTM! Field definitions are well-structured and properly typed.
The new credential-related fields are correctly defined with:
- Appropriate field types for their purposes
- Unique nth values within their respective type groups
- Consistent serialization and signing field settings
Also applies to: 2004-2013, 2174-2183, 2224-2233, 2803-2822
3007-3008: LGTM! Transaction result code properly defined.
The tecBAD_CREDENTIALS error code is appropriately added with a unique identifier in the tec range.
3059-3061: LGTM! Transaction types properly defined with sequential IDs.
The new credential transaction types are correctly added:
- Sequential IDs (58-60)
- Placed appropriately before system-level transactions
- No conflicts with existing transaction types
| it('base', async function () { | ||
| const subjectWallet = await generateFundedWallet(testContext.client) | ||
|
|
||
| const credentialCreateTx: CredentialCreate = { | ||
| TransactionType: 'CredentialCreate', | ||
| Account: testContext.wallet.classicAddress, | ||
| Subject: subjectWallet.classicAddress, | ||
| CredentialType: stringToHex('Test Credential Type'), | ||
| } | ||
|
|
||
| await testTransaction( | ||
| testContext.client, | ||
| credentialCreateTx, | ||
| testContext.wallet, | ||
| ) | ||
|
|
||
| // Unaccepted credential still belongs to issuer's account | ||
| const accountObjectsResponse: AccountObjectsResponse = | ||
| await testContext.client.request({ | ||
| command: 'account_objects', | ||
| account: testContext.wallet.classicAddress, | ||
| }) | ||
| const { account_objects } = accountObjectsResponse.result | ||
|
|
||
| assert.equal(account_objects.length, 1) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional scenarios
The current test only covers the basic happy path. Consider adding tests for:
- Expiration and URI fields
- Invalid credential types
- Error cases (e.g., invalid subject)
- Verification of actual credential data in account_objects
Example additional test:
it('with optional fields', async function () {
const subjectWallet = await generateFundedWallet(testContext.client)
const expiration = Math.floor(Date.now() / 1000) + 3600 // 1 hour from now
const credentialCreateTx: CredentialCreate = {
TransactionType: 'CredentialCreate',
Account: testContext.wallet.classicAddress,
Subject: subjectWallet.classicAddress,
CredentialType: stringToHex('Test Credential Type'),
Expiration: expiration,
URI: 'https://example.com/credential'
}
await testTransaction(testContext.client, credentialCreateTx, testContext.wallet)
const response: AccountObjectsResponse = await testContext.client.request({
command: 'account_objects',
account: testContext.wallet.classicAddress
})
const credential = response.result.account_objects[0]
assert.equal(credential.URI, 'https://example.com/credential')
assert.equal(credential.Expiration, expiration)
})| // Unaccepted credential still belongs to issuer's account | ||
| const accountObjectsResponse: AccountObjectsResponse = | ||
| await testContext.client.request({ | ||
| command: 'account_objects', | ||
| account: testContext.wallet.classicAddress, | ||
| }) | ||
| const { account_objects } = accountObjectsResponse.result | ||
|
|
||
| assert.equal(account_objects.length, 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions for created credential
The test should verify the actual credential data in account_objects, not just the count.
const { account_objects } = accountObjectsResponse.result
+ const credential = account_objects[0]
assert.equal(account_objects.length, 1)
+ assert.equal(credential.LedgerEntryType, 'Credential')
+ assert.equal(credential.Subject, subjectWallet.classicAddress)
+ assert.equal(credential.Issuer, testContext.wallet.classicAddress)
+ assert.equal(credential.CredentialType, stringToHex('Test Credential Type'))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@achowdhry-ripple Agree with this suggestion to verify Credential fields are accurate.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| it('base', async function () { | ||
| const subjectWallet = await generateFundedWallet(testContext.client) | ||
|
|
||
| const credentialCreateTx: CredentialCreate = { | ||
| TransactionType: 'CredentialCreate', | ||
| Account: testContext.wallet.classicAddress, | ||
| Subject: subjectWallet.classicAddress, | ||
| CredentialType: stringToHex('Test Credential Type'), | ||
| } | ||
|
|
||
| await testTransaction( | ||
| testContext.client, | ||
| credentialCreateTx, | ||
| testContext.wallet, | ||
| ) | ||
|
|
||
| const credentialAcceptTx: CredentialAccept = { | ||
| TransactionType: 'CredentialAccept', | ||
| Account: subjectWallet.classicAddress, | ||
| Issuer: testContext.wallet.classicAddress, | ||
| CredentialType: stringToHex('Test Credential Type'), | ||
| } | ||
|
|
||
| await testTransaction(testContext.client, credentialAcceptTx, subjectWallet) | ||
|
|
||
| // Credential is now an object in recipient's wallet after accept | ||
| const accountObjectsResponse: AccountObjectsResponse = | ||
| await testContext.client.request({ | ||
| command: 'account_objects', | ||
| account: subjectWallet.classicAddress, | ||
| }) | ||
| const { account_objects } = accountObjectsResponse.result | ||
|
|
||
| assert.equal(account_objects.length, 1) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases
The current test only covers the happy path. Consider adding:
- Negative test cases (invalid credentials, unauthorized acceptance)
- Validation of credential object properties after acceptance
- Tests with different credential types
- Edge cases (max length credential type, special characters)
Example test structure:
describe('Credential Lifecycle', () => {
describe('successful flows', () => {
it('creates and accepts valid credential', async () => {
// Current test implementation
})
it('handles multiple credential types', async () => {
// Test with different credential types
})
})
describe('error cases', () => {
it('fails with invalid credential type', async () => {
// Test invalid credential scenarios
})
it('prevents unauthorized acceptance', async () => {
// Test unauthorized acceptance
})
})
})
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/xrpl/test/integration/transactions/depositPreauth.test.ts (1)
78-79: Remove unused variable 'wallet2'The variable
wallet2is created and funded but not used in the subsequent code. This could potentially lead to confusion. Consider removing it to keep the code clean.Apply this diff to remove the unused variable:
- const wallet2 = Wallet.generate() - await fundAccount(testContext.client, wallet2)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/test/integration/transactions/depositPreauth.test.ts(2 hunks)
🔇 Additional comments (3)
packages/xrpl/HISTORY.md (1)
8-12: Verify that the changelog entries match the PR objectives.
The changelog entries align with the PR objectives for adding Verifiable Credentials support (XLS-70d), including:
- New transactions: CredentialCreate, CredentialAccept, CredentialDelete
- New Credential ledger object
- Updated DepositPreauth ledger object with Credential support
✅ Verification successful
The changelog entries accurately reflect the implemented changes
The verification results show comprehensive implementation of the XLS-70d Verifiable Credentials features mentioned in the changelog:
-
New transaction types are properly implemented with validation logic:
- CredentialCreate
- CredentialAccept
- CredentialDelete
-
Integration with DepositPreauth is confirmed through test files showing credential support via AuthorizeCredentials and UnauthorizeCredentials fields
The code changes align perfectly with the changelog entries, demonstrating proper implementation of the documented features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new transaction types and ledger objects are properly implemented
# Check for new transaction type definitions
rg -A 5 "CredentialCreate|CredentialAccept|CredentialDelete"
# Check for new ledger object definitions
rg -A 5 "Credential.*ledger object"
# Check for DepositPreauth updates
rg -A 5 "DepositPreauth.*Credential"
Length of output: 55356
packages/xrpl/test/integration/transactions/depositPreauth.test.ts (2)
1-5: Necessary imports added for credential transactions
The added imports correctly include the required modules to handle credential creation and acceptance transactions.
40-87: Test case effectively validates credential functionality
The new test case 'credentials' successfully tests the full workflow of credential creation, acceptance, and authorization within a DepositPreauth transaction. This enhances the test coverage for the new feature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
CONTRIBUTING.md (2)
74-81: Standardize list formattingThe bullet points in the command explanation use asterisks (*) while other lists in the document use dashes (-). Let's maintain consistency.
Apply this diff to standardize the list formatting:
Breaking down the command: -* `docker run -p 6006:6006` starts a Docker container with an open port for admin WebSocket requests. -* `--rm` tells docker to close the container after processes are done running. -* `-it` allows you to interact with the container. -* `--name rippled_standalone` is an instance name for clarity -* `--volume $PWD/.ci-config:/etc/opt/ripple/` identifies the `rippled.cfg` and `validators.txt` to import. It must be an absolute path, so we use `$PWD` instead of `./`. -* `rippleci/rippled` is an image that is regularly updated with the latest `rippled` releases -* `--entrypoint bash rippleci/rippled:2.3.0-rc1` manually overrides the entrypoint (for versions of rippled >= 2.3.0) -* `-c 'rippled -a'` provides the bash command to start `rippled` in standalone mode from the manual entrypoint +- `docker run -p 6006:6006` starts a Docker container with an open port for admin WebSocket requests. +- `--rm` tells docker to close the container after processes are done running. +- `-it` allows you to interact with the container. +- `--name rippled_standalone` is an instance name for clarity +- `--volume $PWD/.ci-config:/etc/opt/ripple/` identifies the `rippled.cfg` and `validators.txt` to import. It must be an absolute path, so we use `$PWD` instead of `./`. +- `rippleci/rippled` is an image that is regularly updated with the latest `rippled` releases +- `--entrypoint bash rippleci/rippled:2.3.0-rc1` manually overrides the entrypoint (for versions of rippled >= 2.3.0) +- `-c 'rippled -a'` provides the bash command to start `rippled` in standalone mode from the manual entrypoint🧰 Tools
🪛 Markdownlint (0.35.0)
74-74: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
76-76: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
78-78: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
79-79: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
81-81: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
96-96: Remove extra whitespace in Docker commandThe Docker command contains an extra space after
run.Apply this diff to fix the command:
-docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:2.3.0-rc1 -c 'rippled -a' +docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:2.3.0-rc1 -c 'rippled -a'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
CONTRIBUTING.md(2 hunks)packages/xrpl/test/integration/README.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
packages/xrpl/test/integration/README.md
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
CONTRIBUTING.md
74-74: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
76-76: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
78-78: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
79-79: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
81-81: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
🔇 Additional comments (1)
packages/xrpl/test/integration/README.md (1)
4-4:
Docker command needs cleanup
The line appears to contain a merge conflict artifact or typo (0-b4) that needs to be removed.
Apply this diff to fix the command:
- * With docker, run `docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:2.3.0-rc1 -c 'rippled -a'`
+ * With docker, run `docker run -p 6006:6006 --rm -it --name rippled_standalone --volume $PWD/.ci-config:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:2.3.0-rc1 -c 'rippled -a'`Likely invalid or redundant comment.
🧰 Tools
🪛 Markdownlint (0.35.0)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/xrpl/src/models/transactions/depositPreauth.ts (1)
63-77: Consider refactoring type assertion in credential validationThe repeated ESLint disable comments for type assertion suggest a potential design issue. Consider:
- Creating a type guard for TransactionType
- Moving the type assertion to a single location
- // eslint-disable-next-line @typescript-eslint/consistent-type-assertions - tx.TransactionType as string + function isValidTransactionType(type: unknown): type is string { + return typeof type === 'string' && type.length > 0 + } + + const txType = isValidTransactionType(tx.TransactionType) + ? tx.TransactionType + : 'UnknownType'packages/xrpl/src/models/transactions/common.ts (3)
381-384: Remove redundant TransactionType checkThe TransactionType validation is already performed in
validateBaseTransaction. This check can be safely removed as it's redundant and the type is guaranteed to be a string at this point.export function validateCredentialType(tx: Record<string, unknown>): void { - if (typeof tx.TransactionType !== 'string') { - throw new ValidationError('Invalid TransactionType') - } if (tx.CredentialType === undefined) {
433-436: Use constant in error messageUse the MAX_CREDENTIALS_LIST_LENGTH constant in the error message for consistency and maintainability.
if (credentials.length > MAX_CREDENTIALS_LIST_LENGTH) { throw new ValidationError( - `${transactionType}: Credentials list cannot have more than 8 elements`, + `${transactionType}: Credentials list cannot have more than ${MAX_CREDENTIALS_LIST_LENGTH} elements`, )
422-426: Improve parameter naming for clarityThe
isStringIDparameter name could be more descriptive to better indicate its purpose.export function validateCredentialsList( credentials: unknown, transactionType: string, - isStringID: boolean, + isCredentialIDFormat: boolean, ): void {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/src/models/transactions/depositPreauth.ts(5 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(3 hunks)packages/xrpl/src/models/transactions/payment.ts(3 hunks)packages/xrpl/src/models/transactions/paymentChannelClaim.ts(3 hunks)packages/xrpl/test/models/escrowFinish.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/src/models/transactions/escrowFinish.ts
- packages/xrpl/src/models/transactions/payment.ts
🔇 Additional comments (5)
packages/xrpl/src/models/transactions/depositPreauth.ts (1)
85-88: Fix null value handling in field validation
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (2)
135-138: LGTM! Clear and informative documentation
The JSDoc comments clearly explain the purpose of CredentialIDs and the expiration requirement.
150-157: Verify the difference in validateCredentialsList usage
The validateCredentialsList is called with true here but false in depositPreauth.ts. Please verify if this difference is intentional and document the reason.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the validateCredentialsList usage pattern:
The difference in validateCredentialsList usage is intentional and correct
The isStringID parameter (third argument) follows a clear pattern across the codebase:
trueis used forCredentialIDsfield in Payment, PaymentChannelClaim, and EscrowFinish transactionsfalseis used forAuthorizeCredentialsandUnauthorizeCredentialsfields in DepositPreauth transaction
This is consistent with the different semantic meanings of these fields in their respective transaction types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other validateCredentialsList usages to understand the pattern
rg -A 3 "validateCredentialsList.*true|validateCredentialsList.*false"
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for validateCredentialsList usage
rg "validateCredentialsList" -A 5 -B 2
Length of output: 6710
Script:
#!/bin/bash
# Also search for the function definition to understand parameters
ast-grep --pattern 'function validateCredentialsList($_) { $$$ }'
Length of output: 67
Script:
#!/bin/bash
# Search for the file containing validateCredentialsList
fd validateCredentialsList
Length of output: 28
packages/xrpl/src/models/transactions/common.ts (2)
457-462:
Fix duplicate detection for credential objects
Using new Set(credentials) won't detect duplicates for objects as they are compared by reference. This is a critical issue that could allow duplicate credentials to pass validation.
- const credentialsSet = new Set(credentials)
+ const credentialsSet = new Set(
+ credentials.map((credential) =>
+ isStringID ? credential : JSON.stringify(credential)
+ )
+ )465-472:
Improve AuthorizeCredential validation
The function needs additional checks for robustness:
- Check if value and value.Credential are defined
- Add type checks for CredentialType and Issuer
function isAuthorizeCredential(
- value: AuthorizeCredential,
+ value: unknown,
): value is AuthorizeCredential {
- if (value.Credential.CredentialType && value.Credential.Issuer) {
+ if (
+ value != null &&
+ typeof value === 'object' &&
+ 'Credential' in value &&
+ value.Credential != null &&
+ typeof value.Credential === 'object' &&
+ 'CredentialType' in value.Credential &&
+ typeof value.Credential.CredentialType === 'string' &&
+ 'Issuer' in value.Credential &&
+ typeof value.Credential.Issuer === 'string'
+ ) {
return true
}
return false
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/xrpl/src/models/ledger/Credential.ts (3)
1-2: Add empty line between import groupsAdd an empty line between imports from different groups to follow the established code organization pattern.
import { GlobalFlags } from '../transactions/common' + import { BaseLedgerEntry, HasPreviousTxnID } from './BaseLedgerEntry'🧰 Tools
🪛 eslint
[error] 1-1: There should be at least one empty line between import groups
(import/order)
8-14: Consider enhancing documentation with references and examplesThe documentation is clear but could be more helpful with:
@seereferences to related transaction types (CredentialCreate, CredentialAccept)- A simple example of a credential object
Would you like me to provide an example of the enhanced documentation?
37-45: Enhance documentation for node fieldsThe SubjectNode and IssuerNode field documentation could be more specific about:
- The expected format of the hint (e.g., hex string)
- How these hints are calculated/used
- Typical scenarios where multiple pages occur
Would you like me to provide an example of the enhanced documentation?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/xrpl/src/models/ledger/Credential.ts(1 hunks)
🧰 Additional context used
🪛 eslint
packages/xrpl/src/models/ledger/Credential.ts
[error] 1-1: There should be at least one empty line between import groups
(import/order)
🔇 Additional comments (3)
packages/xrpl/src/models/ledger/Credential.ts (3)
4-6: LGTM!
The CredentialFlags interface is well-structured and follows the established naming conventions for flag interfaces.
35-35: URI field should be optional
The comment indicates URI is optional, but the type definition shows it as required.
28-29: Verify CredentialType format requirements
The field is documented as "hex-encoded" but there's no specification of:
- Expected length or format requirements
- Whether it should be prefixed with "0x"
- Whether it should be uppercase or lowercase
Let's check how CredentialType is used in the codebase:
✅ Verification successful
Based on the search results, I can now provide a complete response about the CredentialType format requirements:
CredentialType format requirements are validated and well-defined
The field has clear validation rules enforced in the codebase:
- Must be hex-encoded (validated against HEX_REGEX)
- Length must be greater than 0 and less than 128 characters
- No "0x" prefix requirement specified
- Case sensitivity not enforced
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CredentialType usage and validation
rg -A 5 "CredentialType" --type ts
Length of output: 24140
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CONTRIBUTING.md (2)
73-80: Maintain consistent list formattingFor better readability, use consistent list markers throughout the document. Consider replacing asterisks with dashes to match the rest of the document's style.
Apply this formatting change:
-* `docker run -p 6006:6006` starts a Docker container with an open port for admin WebSocket requests. -* `--rm` tells docker to close the container after processes are done running. -* `-it` allows you to interact with the container. -* `--name rippled_standalone` is an instance name for clarity -* `--volume $PWD/.ci-config:/etc/opt/ripple/` identifies the `rippled.cfg` and `validators.txt` to import. It must be an absolute path, so we use `$PWD` instead of `./`. -* `rippleci/rippled` is an image that is regularly updated with the latest `rippled` releases -* `--entrypoint bash rippleci/rippled:2.3.0-rc1` manually overrides the entrypoint (for versions of rippled >= 2.3.0) -* `-c 'rippled -a'` provides the bash command to start `rippled` in standalone mode from the manual entrypoint +- `docker run -p 6006:6006` starts a Docker container with an open port for admin WebSocket requests. +- `--rm` tells docker to close the container after processes are done running. +- `-it` allows you to interact with the container. +- `--name rippled_standalone` is an instance name for clarity +- `--volume $PWD/.ci-config:/etc/opt/ripple/` identifies the `rippled.cfg` and `validators.txt` to import. It must be an absolute path, so we use `$PWD` instead of `./`. +- `rippleci/rippled` is an image that is regularly updated with the latest `rippled` releases +- `--entrypoint bash rippleci/rippled:2.3.0-rc1` manually overrides the entrypoint (for versions of rippled >= 2.3.0) +- `-c 'rippled -a'` provides the bash command to start `rippled` in standalone mode from the manual entrypoint🧰 Tools
🪛 Markdownlint (0.35.0)
73-73: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
77-77: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
78-78: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
79-79: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
Line range hint
1-1: Consider documenting Verifiable Credentials featureGiven that this PR introduces Verifiable Credentials functionality, consider adding:
- A section about testing Verifiable Credentials
- Examples of using the new transaction types (CredentialCreate, CredentialAccept, CredentialDelete)
- Any special setup requirements for testing these features
Would you like help drafting the documentation for the new Verifiable Credentials feature?
🧰 Tools
🪛 Markdownlint (0.35.0)
73-73: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
77-77: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
78-78: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
79-79: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/nodejs.yml(3 hunks)CONTRIBUTING.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nodejs.yml
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CONTRIBUTING.md
73-73: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
77-77: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
78-78: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
79-79: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
🔇 Additional comments (1)
CONTRIBUTING.md (1)
67-67: LGTM! Docker command updates are well-documented
The Docker command has been properly updated with:
- Latest rippled version (2.3.0-rc1)
- Improved container management flags
- Clear explanations for each command component
Also applies to: 74-80, 95-95
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/xrpl/test/models/CredentialCreate.test.ts (1)
202-204: Improve readability of test data generation.The long string used for testing URI length limits could be simplified using string operations:
- credentialCreate.URI = stringToHex( - 'This is beyond the character limit This is beyond the character limit This is beyond the character limitThis is beyond the character limit This is beyond the character limit This is beyond the character limitThis is beyond the character limit This is beyond the character limit This is beyond the character limit', - ) + // Generate a string that's exactly 257 characters (just over the 256 limit) + credentialCreate.URI = stringToHex('A'.repeat(257))This makes it clearer exactly how many characters are being tested and is easier to maintain.
packages/xrpl/test/models/CredentialAccept.test.ts (2)
7-11: Enhance the docstring documentationThe current docstring could be more descriptive. Consider adding:
- Purpose and use case of the CredentialAccept transaction
- Description of required fields and their validation rules
- Relationship to the verifiable credentials feature
/** * CredentialAccept Transaction Verification Testing. * - * Providing runtime verification testing for each specific transaction type. + * Tests validation rules for the CredentialAccept transaction type, which is used + * to accept a credential issued by another account. Validates required fields: + * - Account: The account accepting the credential + * - Issuer: The account that issued the credential + * - CredentialType: Hex-encoded string identifying the credential type (max 128 chars) + * + * Part of the Verifiable Credentials feature that enables credential management on XRPL. */
16-24: Improve type safety by removing 'any' type assertionThe use of
as anybypasses TypeScript's type checking. Consider defining a proper interface for the test object.+ interface CredentialAccept { + TransactionType: 'CredentialAccept' + Issuer: string + Account: string + CredentialType: string + Sequence: number + Flags: number + } beforeEach(function () { credentialAccept = { TransactionType: 'CredentialAccept', Issuer: 'r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ', Account: 'rNdY9XDnQ4Dr1EgefwU3CBRuAjt3sAutGg', CredentialType: stringToHex('Passport'), Sequence: 1337, Flags: 0, - } as any + } as CredentialAccept })packages/xrpl/test/models/CredentialDelete.test.ts (1)
92-107: Document the Subject/Issuer requirement ruleThe test case reveals an important business rule: either Subject or Issuer must be provided. Consider adding a comment explaining this requirement.
+ // Business Rule: A CredentialDelete transaction must specify either the Subject + // (account whose credential is being deleted) or the Issuer (account that issued + // the credential), or both. This allows for credential deletion by either party. it(`throws w/ missing field Subject and Issuer`, function () { delete credentialDelete.Subject delete credentialDelete.Issuer🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: 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
📒 Files selected for processing (3)
packages/xrpl/test/models/CredentialAccept.test.ts(1 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(1 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/test/models/CredentialAccept.test.ts
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 92-92: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialCreate.test.ts
[error] 34-34: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialDelete.test.ts
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/xrpl/test/models/CredentialAccept.test.ts (1)
31-152: Test coverage looks comprehensive
The test suite thoroughly covers:
- Required field validation
- Type checking
- Length constraints
- Format validation for hex encoding
The use of delete operator in tests is acceptable as it effectively tests missing field scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 92-92: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/CredentialDelete.test.ts (2)
7-11: Enhance the docstring documentation
Similar to CredentialAccept, the docstring should be more descriptive.
/**
* CredentialDelete Transaction Verification Testing.
*
- * Providing runtime verification testing for each specific transaction type.
+ * Tests validation rules for the CredentialDelete transaction type, which is used
+ * to delete a credential from the ledger. Validates required fields:
+ * - Account: The account initiating the deletion
+ * - Subject: The account whose credential is being deleted
+ * - Issuer: The account that issued the credential
+ * - CredentialType: Hex-encoded string identifying the credential type (max 128 chars)
+ *
+ * Part of the Verifiable Credentials feature that enables credential management on XRPL.
*/27-171: Test implementation is thorough and consistent
The test suite provides comprehensive coverage of all validation rules and follows consistent patterns with other credential transaction tests.
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/test/models/CredentialCreate.test.ts (2)
15-26: Consider removing 'any' type assertionThe test fixture uses an 'any' type assertion which bypasses TypeScript's type checking. Consider defining an interface for the CredentialCreate transaction type.
+ interface CredentialCreate { + TransactionType: 'CredentialCreate' + Account: string + Subject: string + CredentialType: string + Expiration: number + URI: string + Sequence: number + Flags: number + } beforeEach(function () { credentialCreate = { TransactionType: 'CredentialCreate', Account: 'r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ', Subject: 'rNdY9XDnQ4Dr1EgefwU3CBRuAjt3sAutGg', CredentialType: stringToHex('Passport'), Expiration: 1212025, URI: stringToHex('TestURI'), Sequence: 1337, Flags: 0, - } as any + } as CredentialCreate })
199-213: Simplify URI length testThe current test uses a long string literal to test URI length validation. Consider using a simpler approach with string repetition.
it(`throws w/ URI field too long`, function () { - credentialCreate.URI = stringToHex( - 'This is beyond the character limit This is beyond the character limit This is beyond the character limitThis is beyond the character limit This is beyond the character limit This is beyond the character limitThis is beyond the character limit This is beyond the character limit This is beyond the character limit', - ) + credentialCreate.URI = stringToHex('A'.repeat(257)) const errorMessage = 'CredentialCreate: URI length must be <= 256' assert.throws( () => validateCredentialCreate(credentialCreate), ValidationError, errorMessage, ) // ... rest of the test })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/xrpl/test/models/CredentialCreate.test.ts(1 hunks)packages/xrpl/test/models/escrowFinish.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/models/escrowFinish.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/test/models/CredentialCreate.test.ts
[error] 34-34: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/xrpl/test/models/CredentialCreate.test.ts (3)
1-6: LGTM: Imports are clean and appropriate
The imports are well-organized and include all necessary dependencies for testing.
7-11: Documentation needs enhancement
The current documentation is minimal. Consider adding:
- Purpose and overview of the CredentialCreate transaction
- Description of required fields and their constraints
- Example of a valid CredentialCreate transaction
/**
* CredentialCreate Transaction Verification Testing.
*
+ * The CredentialCreate transaction allows issuers to create verifiable credentials
+ * on the XRPL. Required fields include:
+ * - Account: The issuer's account address
+ * - Subject: The subject's account address
+ * - CredentialType: Hex-encoded string (max 128 bytes)
+ * - Expiration: Unix timestamp for credential expiration
+ * - URI: Hex-encoded string (max 256 bytes)
+ *
+ * Example:
+ * {
+ * TransactionType: 'CredentialCreate',
+ * Account: 'r...',
+ * Subject: 'r...',
+ * CredentialType: '506173...', // 'Passport' in hex
+ * Expiration: 1212025,
+ * URI: '5465...' // 'TestURI' in hex
+ * }
*
* Providing runtime verification testing for each specific transaction type.
*/28-229: LGTM: Comprehensive test coverage
The test suite provides thorough validation coverage for all fields of the CredentialCreate transaction, including:
- Required field validation
- Type checking
- Length constraints
- Hex encoding validation
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| * @param tx A transaction to check the CredentialType for errors | ||
| * @throws Validation Error if the formatting is incorrect | ||
| */ | ||
| export function validateCredentialType(tx: Record<string, unknown>): void { |
There was a problem hiding this comment.
Would this be a more reusable helper function if it kept the format of is{type} but took a max length? e.g. isHex(length)
There was a problem hiding this comment.
So keeping it specific to credential lists, but adding the list length as a parameter? Definitely doable, wasn't sure if we had use cases for different lengths in the future (all of them have 8 max for now)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/models/CredentialDelete.test.ts (2)
7-11: Enhance the docstring documentationThe current docstring could be more descriptive about the specific validation rules being tested.
/** - * CredentialDelete Transaction Verification Testing. - * - * Providing runtime verification testing for each specific transaction type. + * Tests for CredentialDelete transaction validation rules including: + * - Required fields (Account, CredentialType) + * - Optional fields (Subject, Issuer - at least one must be present) + * - Field type validations + * - CredentialType constraints (hex format, length limits) */
16-25: Consider using proper TypeScript types instead of 'any'The test data initialization uses 'any' type which bypasses TypeScript's type checking benefits.
- } as any + } as CredentialDeleteAdd the import at the top:
import { CredentialDelete } from '../../src/models/transactions/CredentialDelete'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/models/CredentialDelete.test.ts(1 hunks)
🔇 Additional comments (5)
packages/xrpl/test/models/CredentialDelete.test.ts (5)
4-5: Consolidate validation imports
Combine the validation imports for better code organization.
-import { validate, ValidationError } from '../../src'
-import { validateCredentialDelete } from '../../src/models/transactions/CredentialDelete'
+import { validate, validateCredentialDelete, ValidationError } from '../../src'27-30: LGTM! Valid case test is well-structured
The test appropriately validates both the specific and generic validation functions.
33-33: Replace delete operator with undefined assignments
Using the delete operator can impact performance. Consider using undefined assignments instead.
- delete credentialDelete.Account
+ credentialDelete.Account = undefined
- delete credentialDelete.Subject
- delete credentialDelete.Issuer
+ credentialDelete.Subject = undefined
+ credentialDelete.Issuer = undefined
- delete credentialDelete.CredentialType
+ credentialDelete.CredentialType = undefinedAlso applies to: 93-94, 110-110
49-49: Improve error messages for type validation
The error messages for type validation could be more specific about the expected types.
- const errorMessage = 'CredentialDelete: invalid field Account'
+ const errorMessage = 'CredentialDelete: Account must be a string'
- const errorMessage = 'CredentialDelete: invalid field Subject'
+ const errorMessage = 'CredentialDelete: Subject must be a string'
- const errorMessage = 'CredentialDelete: invalid field Issuer'
+ const errorMessage = 'CredentialDelete: Issuer must be a string'Also applies to: 64-64, 79-79
32-170: Test coverage looks comprehensive
The test suite thoroughly covers:
- Required field validation
- Type validation
- Field constraints
- Error scenarios
- Both specific and generic validation functions
Good job on the comprehensive test coverage!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/models/escrowFinish.test.ts (2)
109-204: Comprehensive validation test coverage with one duplicate testThe validation test coverage is thorough and includes all necessary cases. However, there's a duplicate test case for non-array input (lines 109-121 and 123-135 are identical).
Remove one of the duplicate test cases for non-array CredentialIDs validation:
- it(`throws w/ non-array CredentialIDs`, function () { - escrow.CredentialIDs = - 'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A' - - const errorMessage = 'EscrowFinish: Credentials must be an array' - - assert.throws( - () => validateEscrowFinish(escrow), - ValidationError, - errorMessage, - ) - assert.throws(() => validate(escrow), ValidationError, errorMessage) - })
109-204: Consider adding validation tests for malformed credential IDsWhile the current test coverage is good, consider adding tests for:
- Malformed hex strings (non-hex characters)
- Incorrect length hex strings (not 256 bits)
Example test case:
it('throws w/ malformed credential ID', function () { escrow.CredentialIDs = ['invalid_hex_string'] const errorMessage = 'EscrowFinish: Invalid Credentials ID format' assert.throws( () => validateEscrowFinish(escrow), ValidationError, errorMessage, ) assert.throws(() => validate(escrow), ValidationError, errorMessage) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/models/escrowFinish.test.ts(3 hunks)
🔇 Additional comments (2)
packages/xrpl/test/models/escrowFinish.test.ts (2)
23-25: LGTM! Valid credential ID format used
The test now uses a proper 256-byte hexadecimal string for CredentialIDs, which serves as good documentation for developers.
34-36: LGTM! Proper handling of optional fields
The test correctly verifies that CredentialIDs is an optional field by setting it to undefined along with other optional fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/models/depositPreauth.test.ts (1)
283-309: Consider improving duplicate credential test casesThe duplicate test cases currently use the same
validCredentialobject reference twice. Consider creating two different credentials with the same values to make the test more realistic and robust.- const invalidCredentials = [validCredential, validCredential] + const duplicateCredentials = [ + { + Credential: { + Issuer: 'rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW', + CredentialType: stringToHex('Passport'), + }, + }, + { + Credential: { + Issuer: 'rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW', + CredentialType: stringToHex('Passport'), + }, + }, + ] - depositPreauth.AuthorizeCredentials = invalidCredentials + depositPreauth.AuthorizeCredentials = duplicateCredentialspackages/xrpl/src/models/transactions/depositPreauth.ts (1)
69-83: Consider refactoring repeated type assertions.The type assertions for
tx.TransactionTypeare duplicated in both credential validation blocks. Consider extracting this into a variable or helper function.+ const transactionType = tx.TransactionType as string // confirmed in base transaction check if (tx.AuthorizeCredentials !== undefined) { validateCredentialsList( tx.AuthorizeCredentials, - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- confirmed in base transaction check - tx.TransactionType as string, + transactionType, false, ) } else if (tx.UnauthorizeCredentials !== undefined) { validateCredentialsList( tx.UnauthorizeCredentials, - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- confirmed in base transaction check - tx.TransactionType as string, + transactionType, false, ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/depositPreauth.ts(5 hunks)packages/xrpl/test/models/depositPreauth.test.ts(4 hunks)
🔇 Additional comments (6)
packages/xrpl/test/models/depositPreauth.test.ts (3)
Line range hint 1-20: LGTM! Well-structured test setup
The imports and test fixtures are well organized. The validCredential constant provides a clean, reusable test fixture that will help maintain consistency across test cases.
41-107: LGTM! Comprehensive test coverage for credential authorization
The test cases thoroughly validate the mutual exclusivity of authorization types and provide clear error messages. The structure follows testing best practices with clear arrange-act-assert patterns.
161-281: LGTM! Thorough edge case coverage
The test suite comprehensively covers array validation, including empty arrays, length limits, and invalid shapes. Error messages are consistent and descriptive.
packages/xrpl/src/models/transactions/depositPreauth.ts (3)
2-8: LGTM! Well-documented interface extensions.
The new credential-related imports and interface extensions are properly typed and documented with clear JSDoc comments.
Also applies to: 26-35
86-101: Consider handling null values in field validation.
The current implementation uses !== undefined which doesn't account for null values. Consider using != null to cover both cases.
- const countProvided = fields.filter((key) => tx[key] !== undefined).length
+ const countProvided = fields.filter((key) => tx[key] != null).length72-73: Verify the base transaction check assumption.
The comment states that the type assertion is "confirmed in base transaction check". Let's verify this assumption.
✅ Verification successful
Type assertion is correctly validated in base transaction check
The code review comment questioned the type assertion's safety, which is marked with a comment stating it's "confirmed in base transaction check". Looking at the implementation of validateBaseTransaction() in common.ts, we can verify this is correct:
- The function explicitly checks that
TransactionTypeexists and is a string:
if (typeof common.TransactionType !== 'string') {
throw new ValidationError('BaseTransaction: TransactionType not string')
}- This validation happens before any transaction-specific validation, ensuring the type assertion is safe to use.
Therefore, the type assertion and its accompanying comment are valid - the base transaction validation does indeed confirm that TransactionType is a string.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TransactionType validation in base transaction
ast-grep --pattern 'function validateBaseTransaction($_) {
$$$
if ($_.TransactionType === undefined || typeof $_.TransactionType !== "string") {
$$$
}
$$$
}'
Length of output: 171
Script:
#!/bin/bash
# Let's try a broader search for base transaction validation
rg -l "validateBaseTransaction"
# Then let's look at the content of these files
rg "validateBaseTransaction" -A 10
Length of output: 69594
Script:
#!/bin/bash
# Let's look at the validateBaseTransaction implementation in common.ts
rg -A 10 "export function validateBaseTransaction" packages/xrpl/src/models/transactions/common.ts
Length of output: 581
packages/xrpl/test/integration/transactions/depositPreauth.test.ts
Outdated
Show resolved
Hide resolved
|
@achowdhry-ripple The integration tests are failing. I'd like to see the CI/CD tests pass before human-reviews. Furthermore, can you merge the |
Fixing the one failing test right now, should be good to review in the next few minutes. Also, I've already merged in the MPT changes so that should be there. There are a few smaller PRs I'll merge in right now, but the rest of everything should be good to review and the large MPT feature is pulled in here |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/xrpl/test/integration/transactions/payment.test.ts (1)
Line range hint
1-195: Consider adding credential-related payment test casesGiven that this PR introduces Credentials support (XLS-0070d), consider adding test cases that verify payment transactions with associated credentials. This would help ensure the integration between the Payment and Credential features works as expected.
Would you like me to help draft test cases for payments with credentials?
🧹 Nitpick comments (1)
packages/xrpl/test/integration/transactions/payment.test.ts (1)
138-138: Consider replacing non-null assertion with runtime validationInstead of using the non-null assertion operator (!), consider adding explicit runtime validation to handle potential undefined values more safely.
-const mptID = meta.mpt_issuance_id! +if (!meta.mpt_issuance_id) { + throw new Error('MPT issuance ID not found in transaction metadata') +} +const mptID = meta.mpt_issuance_id
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/integration/transactions/payment.test.ts(3 hunks)
🔇 Additional comments (2)
packages/xrpl/test/integration/transactions/payment.test.ts (2)
154-154: LGTM!
The MPTokenIssuanceID field is correctly used after validating the existence of the MPT issuance.
176-178: LGTM! Verify Amount object structure
The Amount object structure looks correct. However, let's verify this matches the protocol specification.
✅ Verification successful
Let me gather more information about the Amount type definitions and MPT-related structures.
LGTM! Amount object structure matches protocol specification
The Amount object structure in the code matches the protocol specification as defined in packages/xrpl/src/models/common/index.ts. The MPTAmount interface correctly defines the required fields:
mpt_issuance_id: stringvalue: string
This structure is consistently used across the codebase in payment transactions, clawback operations, and transaction metadata. The implementation is properly type-checked and validated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Amount object structure for MPT payments in the codebase
# Search for similar Amount object structures in tests and protocol definitions
rg -A 5 'mpt_issuance_id.*value' --type ts
# Search for Amount type definitions
ast-grep --pattern 'type Amount = {
$$$
mpt_issuance_id: $_
value: $_
$$$
}'
Length of output: 1139
Script:
#!/bin/bash
# Search for Amount type definitions and interfaces
ast-grep --pattern 'type Amount = {
$$$
}'
# Search for MPT-related Amount interfaces
ast-grep --pattern 'interface $_ {
$$$
mpt_issuance_id: $_
$$$
}'
# Search for documentation or comments about MPT Amount structure
rg -B 2 -A 2 'MPT.*Amount' --type ts
Length of output: 14560
packages/xrpl/test/integration/transactions/depositPreauth.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
460-461: Consider breaking down the validation functionThe eslint-disable comment suggests the function might be too long. Consider extracting the credential format validation logic into a separate function for better maintainability.
+function validateCredentialFormat( + credential: unknown, + transactionType: string, + isStringID: boolean, +): void { + if (isStringID) { + if (!isString(credential)) { + throw new ValidationError( + `${transactionType}: Invalid Credentials ID list format`, + ) + } + } else if (!isAuthorizeCredential(credential)) { + throw new ValidationError( + `${transactionType}: Invalid Credentials format`, + ) + } +} export function validateCredentialsList( credentials: unknown, transactionType: string, isStringID: boolean, ): void { if (credentials == null) { return } // ... length validation ... - credentials.forEach((credential) => { - if (isStringID) { - if (!isString(credential)) { - throw new ValidationError( - `${transactionType}: Invalid Credentials ID list format`, - ) - } - } else if (!isAuthorizeCredential(credential)) { - throw new ValidationError( - `${transactionType}: Invalid Credentials format`, - ) - } - }) + credentials.forEach((credential) => + validateCredentialFormat(credential, transactionType, isStringID) + ) // ... duplicate check ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/src/models/transactions/common.ts(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/xrpl/src/models/transactions/common.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
🔇 Additional comments (6)
packages/xrpl/src/models/transactions/common.ts (4)
9-9: LGTM: Constants and imports are well-defined
The new imports and constants follow the existing patterns and are clearly named.
Also applies to: 20-22
414-450: LGTM: Comprehensive validation of CredentialType
The validation function thoroughly checks all aspects of the CredentialType field:
- Presence check
- Type validation
- Length constraints
- Hex encoding validation
503-506: LGTM: Improved duplicate detection
The containsDuplicates function now correctly handles object comparison by stringifying the objects before comparison, addressing the previous review comment.
137-145:
Add null check for Credential object
The function assumes input.Credential is defined before accessing its properties. Add a null check to prevent potential runtime errors.
function isAuthorizeCredential(input: unknown): input is AuthorizeCredential {
return (
isRecord(input) &&
isRecord(input.Credential) &&
+ input.Credential != null &&
Object.keys(input).length === AUTHORIZE_CREDENTIAL_SIZE &&
typeof input.Credential.CredentialType === 'string' &&
typeof input.Credential.Issuer === 'string'
)
}Likely invalid or redundant comment.
.ci-config/rippled.cfg (2)
182-183: LGTM: Amendment version header is correctly updated
The header correctly identifies these as 2.3.0 amendments, which aligns with the PR requirements of rippled 2.3.0 or newer.
Line range hint 184-191: Verify amendment completeness for rippled 2.3.0
The amendments list includes all required features for 2.3.0, including:
fixAMMv1_2andCredentialsas mentioned in past review comments- Additional NFT-related fixes
- MPTokens and other improvements
Let's verify these amendments against the official rippled 2.3.0 release:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/xrpl/test/models/payment.test.ts (2)
277-378: Consider adding test case for invalid credential ID format.The current test suite thoroughly validates array constraints and element types. However, consider adding a test case to validate the format of credential IDs (e.g., length, hexadecimal format).
Example test case:
it(`throws w/ invalid format CredentialID`, function () { paymentTransaction.CredentialIDs = [ 'INVALID_CREDENTIAL_ID_FORMAT', // Not a valid hex string 'XYZ' // Too short ] const errorMessage = 'Payment: Invalid Credential ID format' assert.throws( () => validatePayment(paymentTransaction), ValidationError, errorMessage, ) assert.throws( () => validate(paymentTransaction), ValidationError, errorMessage, ) })
277-378: Add positive test case for valid CredentialIDs.While error cases are well covered, consider adding a positive test case demonstrating successful validation with valid CredentialIDs.
Example test case:
it(`verifies valid CredentialIDs`, function () { paymentTransaction.CredentialIDs = [ 'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A', 'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66B' ] assert.doesNotThrow(() => validatePayment(paymentTransaction)) assert.doesNotThrow(() => validate(paymentTransaction)) })packages/xrpl/test/models/accountDelete.test.ts (1)
66-80: Add missing DestinationTag validation test casesWhile the current test covers invalid type, consider adding tests for:
- Negative values
- Values exceeding uint32 max
- Decimal values
it('throws w/ negative DestinationTag', function () { validAccountDelete.DestinationTag = -1 assert.throws( () => validateAccountDelete(validAccountDelete), ValidationError, 'AccountDelete: invalid field DestinationTag' ) }) it('throws w/ DestinationTag exceeding uint32', function () { validAccountDelete.DestinationTag = 4294967296 // uint32 max + 1 assert.throws( () => validateAccountDelete(validAccountDelete), ValidationError, 'AccountDelete: invalid field DestinationTag' ) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/test/models/accountDelete.test.ts(1 hunks)packages/xrpl/test/models/escrowFinish.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/models/escrowFinish.test.ts
🔇 Additional comments (4)
packages/xrpl/test/models/payment.test.ts (1)
276-378: LGTM! Well-structured test cases.
The new test cases follow consistent patterns with existing tests, have clear descriptions, and properly test both validation functions.
packages/xrpl/test/models/accountDelete.test.ts (3)
28-30: LGTM!
The basic validation test is well-structured and verifies the happy path correctly.
33-61: LGTM!
The Destination validation tests are well-structured with:
- Clear error messages stored in variables
- Comprehensive validation using both direct and generic validators
- Good coverage of edge cases
82-183: 🛠️ Refactor suggestion
Add test for invalid credential ID format
The tests thoroughly cover array validation but should also verify the format of individual credential IDs.
it('throws w/ invalid credential ID format', function () {
validAccountDelete.CredentialIDs = [
'invalid-credential-id', // Not a valid hex string
'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F662'
]
assert.throws(
() => validateAccountDelete(validAccountDelete),
ValidationError,
'AccountDelete: Invalid Credentials ID format'
)
})Additionally, let's verify the credential format validation in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/models/accountDelete.test.ts (2)
33-61: Consider reducing test duplication using a test helper.The validation tests for
validateAccountDeleteandvalidatefunctions contain duplicate assertions. Consider creating a helper function to reduce duplication.function assertValidationError( transaction: AccountDeleteTest, errorMessage: string, ): void { [validateAccountDelete, validate].forEach((validationFn) => { assert.throws( () => validationFn(transaction), ValidationError, errorMessage, ) }) }
101-111: Consider extracting test data to constants.The credential IDs and test arrays are repeated across multiple tests. Consider extracting these to constants to improve maintainability.
const VALID_CREDENTIAL_ID = 'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A' const INVALID_CREDENTIAL_ID = 123123 const CREDENTIAL_IDS = { EXCEEDS_MAX: Array(9).fill(VALID_CREDENTIAL_ID).map((id, i) => id.slice(0, -1) + i), INVALID_TYPE: [INVALID_CREDENTIAL_ID, VALID_CREDENTIAL_ID], DUPLICATES: [VALID_CREDENTIAL_ID, VALID_CREDENTIAL_ID], }Also applies to: 146-149, 166-169
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/models/accountDelete.test.ts(1 hunks)
🔇 Additional comments (4)
packages/xrpl/test/models/accountDelete.test.ts (4)
15-26: Define proper types for test data instead of using 'as any'.
Using as any bypasses TypeScript's type checking. Consider defining an interface for the test data structure.
+interface AccountDeleteTest {
+ TransactionType: 'AccountDelete'
+ Account: string
+ Destination: string
+ DestinationTag?: number
+ Fee: string
+ Sequence: number
+ Flags: number
+ CredentialIDs?: string[]
+}
- validAccountDelete = {
+ validAccountDelete: AccountDeleteTest = {
TransactionType: 'AccountDelete',
Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
Destination: 'rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe',
DestinationTag: 13,
Fee: '5000000',
Sequence: 2470665,
Flags: 2147483648,
CredentialIDs: [
'EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A',
],
- } as any
+ }28-30: LGTM! Basic validation test is well-structured.
66-80: Test structure is consistent with previous tests.
82-183: Excellent test coverage for CredentialIDs validation!
The tests thoroughly cover all edge cases and validation rules for the CredentialIDs field.
High Level Overview of Change
JavaScript client implementation of https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0070d-credentials
Corresponding c++ changes: https://github.com/XRPLF/rippled/pull/5103/files
Context of Change
Following changes are introduced in this PR:
(Quoting Verbatim from the XLS specification document)
Creating a Credential ledger object
Creating a CredentialCreate transaction type
Creating a CredentialAccept transaction type
Creating a CredentialDelete transaction type
Modifying the DepositPreauth ledger object
Modifying the DepositPreauth transaction
Modifying other transactions that are affected by Deposit Authorization -- Payment, PaymentChannelClaim, AccountDelete and EscrowFinish
Type of Change
Did you update HISTORY.md?
Test Plan
unit and integration tests have been added, must use a version of rippled 2.3.0 or newer