Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0169b41
models for PermissionedDomain amendment
ckeshava Nov 19, 2024
f99d415
remove old comment
ckeshava Jan 8, 2025
76e5d09
Apply suggestions from code review
ckeshava Jan 8, 2025
0ad5503
code suggestions from rabbit AI
ckeshava Jan 8, 2025
6a543f8
Include updates for DynamicNFT amendment
ckeshava Jan 14, 2025
fa3b710
include PD amendment in config file
ckeshava Jan 14, 2025
84061c0
Additional unit tests for PDSet txn
ckeshava Jan 14, 2025
035bf0d
Update packages/xrpl/src/models/transactions/permissionedDomainSet.ts
ckeshava Jan 14, 2025
ce9193c
fix linter errors. Use AuthorizeCreds nested object format
ckeshava Jan 14, 2025
cf772b9
refactor: use validateCredsList method for PDSet.AcceptedCreds field
ckeshava Jan 15, 2025
98d9513
include description of method parameters
ckeshava Jan 15, 2025
da655b2
fix: update the containsDuplicates method to handle nested objects
ckeshava Jan 16, 2025
2697a21
remove the use of lodash dependency
ckeshava Jan 16, 2025
81551b0
Merge branch 'main' into xls_80d
ckeshava Jan 16, 2025
c619920
refactor: remove the default value in validateCredentialsList
ckeshava Jan 17, 2025
2316945
refactor: containsDup method
ckeshava Jan 17, 2025
94c0e37
fix linter issues
ckeshava Jan 18, 2025
7f0baad
add descriptions for the PD ledger object
ckeshava Feb 3, 2025
2080c21
address PR comments
ckeshava Feb 3, 2025
1311219
Merge branch 'main' into xls_80d
ckeshava Feb 3, 2025
6d7905f
pacify the linter
ckeshava Feb 3, 2025
2b3580a
remvoe the usage of inner functions
ckeshava Feb 4, 2025
9154151
address PR comments
ckeshava Feb 6, 2025
877aa20
Accomodate new "git" field in the ServerState RPC response
ckeshava Feb 6, 2025
5ee6c1a
update the serverState RPC response with network_id field
ckeshava Feb 6, 2025
c4df44c
Merge branch 'main' into xls_80d
ckeshava Feb 6, 2025
86702e0
update the removeKeys in the comparisons of serverState, serverInfo u…
ckeshava Feb 6, 2025
e9fb8f6
Update packages/xrpl/test/integration/transactions/permissionedDomain…
ckeshava Feb 7, 2025
cbe6b56
Update packages/xrpl/test/integration/transactions/permissionedDomain…
ckeshava Feb 7, 2025
8d0cc28
Merge branch 'main' into xls_80d
ckeshava Feb 7, 2025
9d0e662
address PR commetns
ckeshava Feb 7, 2025
7d1a8d4
fix: update variable names in tests
ckeshava Feb 7, 2025
3dfeaf1
change variable names
ckeshava Feb 7, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions packages/ripple-binary-codec/src/enums/definitions.json
Original file line number Diff line number Diff line change
Expand Up @@ -1250,6 +1250,16 @@
"type": "Hash256"
}
],
[
"DomainID",
{
"nth": 34,
"isVLEncoded": false,
"isSerialized": true,
"isSigningField": true,
"type": "Hash256"
}
],
[
"hash",
{
Expand Down Expand Up @@ -2530,6 +2540,15 @@
"type": "STArray"
}
],
[
"AcceptedCredentials", {
"nth": 28,
"isVLEncoded": false,
"isSerialized": true,
"isSigningField": true,
"type": "STArray"
}
],
[
"CloseResolution",
{
Expand Down Expand Up @@ -2863,6 +2882,7 @@
"Oracle": 128,
"Credential": 129,
"PayChannel": 120,
"PermissionedDomain": 130,
"RippleState": 114,
"SignerList": 83,
"Ticket": 84,
Expand Down Expand Up @@ -3093,6 +3113,8 @@
"PaymentChannelClaim": 15,
"PaymentChannelCreate": 13,
"PaymentChannelFund": 14,
"PermissionedDomainSet": 61,
"PermissionedDomainDelete": 62,
"SetFee": 101,
"SetRegularKey": 5,
"SignerListSet": 12,
Expand Down
3 changes: 3 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr

## Unreleased Changes

### Added
* Implementation of XLS-80d PermissionedDomain feature.

## 4.1.0 (2024-12-23)

### Added
Expand Down
3 changes: 3 additions & 0 deletions packages/xrpl/src/models/ledger/LedgerEntry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import NegativeUNL from './NegativeUNL'
import Offer from './Offer'
import Oracle from './Oracle'
import PayChannel from './PayChannel'
import PermissionedDomain from './PermissionedDomain'
import RippleState from './RippleState'
import SignerList from './SignerList'
import Ticket from './Ticket'
Expand All @@ -35,6 +36,7 @@ type LedgerEntry =
| Offer
| Oracle
| PayChannel
| PermissionedDomain
| RippleState
| SignerList
| Ticket
Expand All @@ -61,6 +63,7 @@ type LedgerEntryFilter =
| 'offer'
| 'oracle'
| 'payment_channel'
| 'permissioned_domain'
| 'signer_list'
| 'state'
| 'ticket'
Expand Down
24 changes: 24 additions & 0 deletions packages/xrpl/src/models/ledger/PermissionedDomain.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { BaseLedgerEntry, HasPreviousTxnID } from './BaseLedgerEntry'

export interface Credential {
Credential: {
Issuer: string
CredentialType: string
}
}

export default interface PermissionedDomain
extends BaseLedgerEntry,
HasPreviousTxnID {
LedgerEntryType: 'PermissionedDomain'

Owner: string

Flags: 0

OwnerNode: string

Sequence: number

AcceptedCredentials: Credential[]
}
3 changes: 3 additions & 0 deletions packages/xrpl/src/models/transactions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,6 @@ export {
XChainModifyBridgeFlags,
XChainModifyBridgeFlagsInterface,
} from './XChainModifyBridge'

export { PermissionedDomainSet } from './permissionedDomainSet'
export { PermissionedDomainDelete } from './permissionedDomainDelete'
18 changes: 18 additions & 0 deletions packages/xrpl/src/models/transactions/permissionedDomainDelete.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {
BaseTransaction,
isString,
validateBaseTransaction,
validateRequiredField
} from './common'

export interface PermissionedDomainDelete extends BaseTransaction {
TransactionType: 'PermissionedDomainDelete'

DomainID: string
}

export function validatePermissionedDomainDelete(tx: Record<string, unknown>): void {
validateBaseTransaction(tx)

validateRequiredField(tx, 'DomainID', isString)
}
47 changes: 47 additions & 0 deletions packages/xrpl/src/models/transactions/permissionedDomainSet.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { ValidationError } from '../../errors'
import {
BaseTransaction,
isString,
validateBaseTransaction,
validateOptionalField,
validateRequiredField,
} from './common'

import { Credential } from '../ledger/PermissionedDomain'
const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10
const MAX_ACCEPTED_CREDENTIALS = 10

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


export interface PermissionedDomainSet extends BaseTransaction {
TransactionType: 'PermissionedDomainSet'

DomainID?: string
AcceptedCredentials: Credential[]
}

// eslint-disable-next-line max-lines-per-function -- necessary to validate many fields
export function validatePermissionedDomainSet(tx: Record<string, unknown>): void {
validateBaseTransaction(tx)

validateOptionalField(tx, 'DomainID', isString)

// eslint-disable-next-line max-lines-per-function -- necessary to validate many fields
validateRequiredField(tx, 'AcceptedCredentials', (value) => {
if (!Array.isArray(value)) {
throw new ValidationError('PermissionedDomainSet: AcceptedCredentials must be an array')
}

if (value.length > ACCEPTED_CREDENTIALS_MAX_LENGTH) {
throw new ValidationError(
`PermissionedDomainSet: AcceptedCredentials must have at most ${ACCEPTED_CREDENTIALS_MAX_LENGTH} Credential objects`,
)
}
else if (value.length === 0) {
throw new ValidationError(
`PermissionedDomainSet: AcceptedCredentials must have at least one Credential object`,
)
}

// 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.

return true
})
}
12 changes: 12 additions & 0 deletions packages/xrpl/src/models/transactions/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ import {
PaymentChannelFund,
validatePaymentChannelFund,
} from './paymentChannelFund'
import { PermissionedDomainDelete, validatePermissionedDomainDelete } from './permissionedDomainDelete'
import { PermissionedDomainSet, validatePermissionedDomainSet } from './permissionedDomainSet'
import { SetFee } from './setFee'
import { SetRegularKey, validateSetRegularKey } from './setRegularKey'
import { SignerListSet, validateSignerListSet } from './signerListSet'
Expand Down Expand Up @@ -151,6 +153,8 @@ export type SubmittableTransaction =
| PaymentChannelClaim
| PaymentChannelCreate
| PaymentChannelFund
| PermissionedDomainSet
| PermissionedDomainDelete
| SetRegularKey
| SignerListSet
| TicketCreate
Expand Down Expand Up @@ -409,6 +413,14 @@ export function validate(transaction: Record<string, unknown>): void {
validatePaymentChannelFund(tx)
break

case 'PermissionedDomainSet':
validatePermissionedDomainSet(tx)
break

case 'PermissionedDomainDelete':
validatePermissionedDomainDelete(tx)
break

case 'SetRegularKey':
validateSetRegularKey(tx)
break
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { stringToHex } from '@xrplf/isomorphic/utils'
import { assert } from 'chai'

import { LedgerEntryRequest, PermissionedDomainDelete, PermissionedDomainSet } from '../../../src'
import PermissionedDomain, { Credential } from '../../../src/models/ledger/PermissionedDomain'
import serverUrl from '../serverUrl'
import {
setupClient,
teardownClient,
type XrplIntegrationTestContext,
} from '../setup'
import { testTransaction } from '../utils'

// how long before each test case times out
const TIMEOUT = 20000

describe('PermissionedDomainSet', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment on your Python PR: Testing all feature transactions in one integ file doesn't match the the codebase's convention. For each new transaction, there should be a separate integ test file for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels more natural to validate the complete lifecycle of a ledger-object. If we split this into multiple files, there will be a lot of repetition of code. You cannot execute PDDelete without a preceding PDSet transaction.

There is no benefit to splitting this test into multiple files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do understand the benefits of doing it in one single module but this breaks the convention of how we add tests to the codebase. We should be consistent to avoid confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefits outweigh the conventional needs in this case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Breaking the convention for one feature while all others follow it isn't good practice.

Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally didn't like this convention when implementing credentials, because separate files forces you to duplicate earlier transactions in each file, making the later lifecycle test cover the functionality of the first few anyways.

I would prefer we switch to this new lifecycle convention going forward, and if the team agrees could you delete the create/accept tests for credentials and rename the delete file? Quick cleanup to create a more logical convention going forward

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with tests like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achowdhry-ripple I'm happy to do that in a separate PR. I'd like to restrict the scope of this PR at the moment.

Copy link
Contributor

@khancode khancode Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still create a ticket to eventually refactor the existing tests to the new convention; this can be implemented at a later time. It's worth the time/effort to make the tests run more efficient and stick to this improved convention. @ckeshava can you create it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I've created a ticket

let testContext: XrplIntegrationTestContext

beforeEach(async () => {
testContext = await setupClient(serverUrl)
})
afterEach(async () => teardownClient(testContext))

it(
'Lifecycle of PermissionedDomain ledger object',
async () => {
const sampleCredential: Credential = {
Credential: {
CredentialType: stringToHex('Passport'),
Issuer: testContext.wallet.classicAddress
}
}

// Step-1: Test the PermissionedDomainSet transaction
const tx_pd_set: PermissionedDomainSet = {
TransactionType: 'PermissionedDomainSet',
Account: testContext.wallet.classicAddress,
AcceptedCredentials: [sampleCredential]
}

await testTransaction(testContext.client, tx_pd_set, testContext.wallet)

// Step-2: Validate the ledger_entry, account_objects RPC methods
// validate the account_objects RPC
const result = await testContext.client.request({
command: 'account_objects',
account: testContext.wallet.classicAddress,
type: 'permissioned_domain',
})

assert.equal(result.result.account_objects.length, 1)
const pd = result.result.account_objects[0] as PermissionedDomain

assert.equal(pd.Flags, 0)
expect(pd.AcceptedCredentials).toEqual([sampleCredential])

// validate the ledger_entry RPC
const ledger_entry_request: LedgerEntryRequest = {
command: 'ledger_entry',
// fetch the PD `index` from the previous account_objects RPC response
index: pd.index
}
const ledger_entry_result = await testContext.client.request(ledger_entry_request)
assert.deepEqual(pd, ledger_entry_result.result.node)

// Step-3: Test the PDDelete transaction
const tx_pd_delete: PermissionedDomainDelete = {
TransactionType: 'PermissionedDomainDelete',
Account: testContext.wallet.classicAddress,
// fetch the PD `index` from the previous account_objects RPC response
DomainID: pd.index
}

await testTransaction(testContext.client, tx_pd_delete, testContext.wallet)
},
TIMEOUT,
)
})
40 changes: 40 additions & 0 deletions packages/xrpl/test/models/permissionedDomainDelete.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { assert } from 'chai'

import { validate, ValidationError } from '../../src'
import { validatePermissionedDomainDelete } from '../../src/models/transactions/permissionedDomainDelete'

/**
* PermissionedDomainDelete Transaction Verification Testing.
*
* Providing runtime verification testing for each specific transaction type.
*/
describe('PermissionedDomainDelete', function () {
let tx

beforeEach(function () {
tx = {
TransactionType: 'PermissionedDomainDelete',
Account: 'rfmDuhDyLGgx94qiwf3YF8BUV5j6KSvE8',
DomainID: 'D88930B33C2B6831660BFD006D91FF100011AD4E67CBB78B460AF0A215103737',
} as any
})

it('verifies valid PermissionedDomainDelete', function () {
assert.doesNotThrow(() => validatePermissionedDomainDelete(tx))
assert.doesNotThrow(() => validate(tx))
})

it(`throws w/ missing field DomainID`, function () {
delete tx.DomainID
const errorMessage = 'PermissionedDomainDelete: missing field DomainID'
assert.throws(() => validatePermissionedDomainDelete(tx), ValidationError, errorMessage)
assert.throws(() => validate(tx), ValidationError, errorMessage)
})

it(`throws w/ invalid DomainID`, function () {
tx.DomainID = 1234
const errorMessage = 'PermissionedDomainDelete: invalid field DomainID'
assert.throws(() => validatePermissionedDomainDelete(tx), ValidationError, errorMessage)
assert.throws(() => validate(tx), ValidationError, errorMessage)
})
})
Loading
Loading