-
Notifications
You must be signed in to change notification settings - Fork 561
PermissionedDomain XLS-80d #2874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
0169b41
f99d415
76e5d09
0ad5503
6a543f8
fa3b710
84061c0
035bf0d
ce9193c
cf772b9
98d9513
da655b2
2697a21
81551b0
c619920
2316945
94c0e37
7f0baad
2080c21
1311219
6d7905f
2b3580a
9154151
877aa20
5ee6c1a
c4df44c
86702e0
e9fb8f6
cbe6b56
8d0cc28
9d0e662
7d1a8d4
3dfeaf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| } | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| export default interface PermissionedDomain | ||
| extends BaseLedgerEntry, | ||
| HasPreviousTxnID { | ||
| LedgerEntryType: 'PermissionedDomain' | ||
|
|
||
| Owner: string | ||
|
|
||
| Flags: 0 | ||
|
|
||
| OwnerNode: string | ||
|
|
||
| Sequence: number | ||
|
|
||
| AcceptedCredentials: Credential[] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,9 @@ export function isIssuedCurrency( | |
| * @param input - The input to check the form and type of | ||
| * @returns Whether the AuthorizeCredential is properly formed | ||
| */ | ||
| function isAuthorizeCredential(input: unknown): input is AuthorizeCredential { | ||
| export function isAuthorizeCredential( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function's name is too specific now that it is also being used for PDs
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name appropriately suggests the intention. Since PDSet transaction also uses
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @achowdhry-ripple If I rename this to AcceptedCredentials, will it confuse the Credential's usage? Should I rename it to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mvadari I'd like to discuss this issue offline. I'll merge this PR at the moment because explorer changes need the client library |
||
| input: unknown, | ||
| ): input is AuthorizeCredential { | ||
| return ( | ||
| isRecord(input) && | ||
| isRecord(input.Credential) && | ||
|
|
@@ -500,7 +502,7 @@ export function validateCredentialsList( | |
| } | ||
| } | ||
|
|
||
| function containsDuplicates(objectList: object[]): boolean { | ||
| export function containsDuplicates(objectList: object[]): boolean { | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) | ||
|
||
| return objSet.size !== objectList.length | ||
| } | ||
| 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 { | ||
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| TransactionType: 'PermissionedDomainDelete' | ||
|
|
||
mvadari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| DomainID: string | ||
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| export function validatePermissionedDomainDelete(tx: Record<string, unknown>): void { | ||
| validateBaseTransaction(tx) | ||
|
|
||
| validateRequiredField(tx, 'DomainID', isString) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,63 @@ | ||||||
| import { ValidationError } from '../../errors' | ||||||
| import { Credential } from '../ledger/PermissionedDomain' | ||||||
|
|
||||||
| import { | ||||||
| BaseTransaction, | ||||||
| isString, | ||||||
| validateBaseTransaction, | ||||||
| validateOptionalField, | ||||||
| validateRequiredField, | ||||||
| isAuthorizeCredential, | ||||||
| containsDuplicates, | ||||||
| } from './common' | ||||||
|
|
||||||
| const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10 | ||||||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| const ACCEPTED_CREDENTIALS_MAX_LENGTH = 10 | |
| const MAX_ACCEPTED_CREDENTIALS = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
mvadari marked this conversation as resolved.
Show resolved
Hide resolved
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
| 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 () { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There is no benefit to splitting this test into multiple files.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The benefits outweigh the conventional needs in this case
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with tests like this.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 = { | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 = { | ||
ckeshava marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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, | ||
| ) | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.