-
Notifications
You must be signed in to change notification settings - Fork 525
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
PermissionedDomain XLS-80d #2874
Changes from all 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 |
---|---|---|
|
@@ -189,3 +189,4 @@ fixInnerObjTemplate2 | |
fixEnforceNFTokenTrustline | ||
fixReducedOffersV2 | ||
DynamicNFT | ||
PermissionedDomains |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { AuthorizeCredential } from '../common' | ||
|
||
import { BaseLedgerEntry, HasPreviousTxnID } from './BaseLedgerEntry' | ||
|
||
export default interface PermissionedDomain | ||
extends BaseLedgerEntry, | ||
HasPreviousTxnID { | ||
/* The ledger object's type (PermissionedDomain). */ | ||
LedgerEntryType: 'PermissionedDomain' | ||
|
||
/* The account that controls the settings of the domain. */ | ||
Owner: string | ||
|
||
/* The credentials that are accepted by the domain. | ||
Ownership of one of these credentials automatically | ||
makes you a member of the domain. */ | ||
AcceptedCredentials: AuthorizeCredential[] | ||
|
||
/* Flag values associated with this object. */ | ||
Flags: 0 | ||
|
||
/* Owner account's directory page containing the PermissionedDomain object. */ | ||
OwnerNode: string | ||
|
||
/* The Sequence value of the PermissionedDomainSet | ||
transaction that created this domain. Used in combination | ||
with the Account to identify this domain. */ | ||
Sequence: number | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ export interface ServerStateResponse extends BaseResponse { | |
load_factor_fee_queue?: number | ||
load_factor_fee_reference?: number | ||
load_factor_server?: number | ||
network_id: number | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add this to the changelog as a fix |
||
peer_disconnects?: string | ||
peer_disconnects_resources?: string | ||
peers: number | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,15 +9,15 @@ import { | |||||||||||||||||||||||||||||||
AuthorizeCredential, | ||||||||||||||||||||||||||||||||
Currency, | ||||||||||||||||||||||||||||||||
IssuedCurrencyAmount, | ||||||||||||||||||||||||||||||||
MPTAmount, | ||||||||||||||||||||||||||||||||
Memo, | ||||||||||||||||||||||||||||||||
Signer, | ||||||||||||||||||||||||||||||||
XChainBridge, | ||||||||||||||||||||||||||||||||
MPTAmount, | ||||||||||||||||||||||||||||||||
} from '../common' | ||||||||||||||||||||||||||||||||
import { onlyHasFields } from '../utils' | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const MEMO_SIZE = 3 | ||||||||||||||||||||||||||||||||
const MAX_CREDENTIALS_LIST_LENGTH = 8 | ||||||||||||||||||||||||||||||||
export const MAX_AUTHORIZED_CREDENTIALS = 8 | ||||||||||||||||||||||||||||||||
const MAX_CREDENTIAL_BYTE_LENGTH = 64 | ||||||||||||||||||||||||||||||||
const MAX_CREDENTIAL_TYPE_LENGTH = MAX_CREDENTIAL_BYTE_LENGTH * 2 | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
@@ -134,7 +134,9 @@ export function isIssuedCurrency( | |||||||||||||||||||||||||||||||
* @param input - The input to check the form and type of | ||||||||||||||||||||||||||||||||
* @returns Whether the AuthorizeCredential is properly formed | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
function isAuthorizeCredential(input: unknown): input is AuthorizeCredential { | ||||||||||||||||||||||||||||||||
export function isAuthorizeCredential( | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. 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 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 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 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 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) && | ||||||||||||||||||||||||||||||||
|
@@ -455,13 +457,16 @@ export function validateCredentialType(tx: Record<string, unknown>): void { | |||||||||||||||||||||||||||||||
* @param credentials An array of credential IDs to check for errors | ||||||||||||||||||||||||||||||||
* @param transactionType The transaction type to include in error messages | ||||||||||||||||||||||||||||||||
* @param isStringID Toggle for if array contains IDs instead of AuthorizeCredential objects | ||||||||||||||||||||||||||||||||
* @param maxCredentials The maximum length of the credentials array. | ||||||||||||||||||||||||||||||||
* PermissionedDomainSet transaction uses 10, other transactions use 8. | ||||||||||||||||||||||||||||||||
* @throws Validation Error if the formatting is incorrect | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
// eslint-disable-next-line max-lines-per-function -- separating logic further will add unnecessary complexity | ||||||||||||||||||||||||||||||||
// eslint-disable-next-line max-lines-per-function, max-params -- separating logic further will add unnecessary complexity | ||||||||||||||||||||||||||||||||
export function validateCredentialsList( | ||||||||||||||||||||||||||||||||
credentials: unknown, | ||||||||||||||||||||||||||||||||
transactionType: string, | ||||||||||||||||||||||||||||||||
isStringID: boolean, | ||||||||||||||||||||||||||||||||
maxCredentials: number, | ||||||||||||||||||||||||||||||||
): void { | ||||||||||||||||||||||||||||||||
if (credentials == null) { | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
|
@@ -471,9 +476,9 @@ export function validateCredentialsList( | |||||||||||||||||||||||||||||||
`${transactionType}: Credentials must be an array`, | ||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
if (credentials.length > MAX_CREDENTIALS_LIST_LENGTH) { | ||||||||||||||||||||||||||||||||
if (credentials.length > maxCredentials) { | ||||||||||||||||||||||||||||||||
throw new ValidationError( | ||||||||||||||||||||||||||||||||
`${transactionType}: Credentials length cannot exceed ${MAX_CREDENTIALS_LIST_LENGTH} elements`, | ||||||||||||||||||||||||||||||||
`${transactionType}: Credentials length cannot exceed ${maxCredentials} elements`, | ||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||
} else if (credentials.length === 0) { | ||||||||||||||||||||||||||||||||
throw new ValidationError( | ||||||||||||||||||||||||||||||||
|
@@ -500,7 +505,42 @@ export function validateCredentialsList( | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function containsDuplicates(objectList: object[]): boolean { | ||||||||||||||||||||||||||||||||
const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) | ||||||||||||||||||||||||||||||||
return objSet.size !== objectList.length | ||||||||||||||||||||||||||||||||
// Type guard to ensure we're working with AuthorizeCredential[] | ||||||||||||||||||||||||||||||||
// Note: This is not a rigorous type-guard. A more thorough solution would be to iterate over the array and check each item. | ||||||||||||||||||||||||||||||||
function isAuthorizeCredentialArray( | ||||||||||||||||||||||||||||||||
list: AuthorizeCredential[] | string[], | ||||||||||||||||||||||||||||||||
): list is AuthorizeCredential[] { | ||||||||||||||||||||||||||||||||
return typeof list[0] !== 'string' | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+508
to
+514
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. 🛠️ Refactor suggestion Improve type guard implementation. The current type guard implementation is not rigorous as it only checks the first element. A more thorough solution would be to iterate over the array and check each item. -function isAuthorizeCredentialArray(
+function isAuthorizeCredentialArray(
list: AuthorizeCredential[] | string[],
): list is AuthorizeCredential[] {
- return typeof list[0] !== 'string'
+ return list.length > 0 && list.every((item) => typeof item !== 'string')
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* Check if an array of objects contains any duplicates. | ||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||
* @param objectList - Array of objects to check for duplicates | ||||||||||||||||||||||||||||||||
* @returns True if duplicates exist, false otherwise | ||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||
export function containsDuplicates( | ||||||||||||||||||||||||||||||||
objectList: AuthorizeCredential[] | string[], | ||||||||||||||||||||||||||||||||
): boolean { | ||||||||||||||||||||||||||||||||
// Case-1: Process a list of string-IDs | ||||||||||||||||||||||||||||||||
if (typeof objectList[0] === 'string') { | ||||||||||||||||||||||||||||||||
const objSet = new Set(objectList.map((obj) => JSON.stringify(obj))) | ||||||||||||||||||||||||||||||||
return objSet.size !== objectList.length | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// Case-2: Process a list of nested objects | ||||||||||||||||||||||||||||||||
const seen = new Set<string>() | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (isAuthorizeCredentialArray(objectList)) { | ||||||||||||||||||||||||||||||||
for (const item of objectList) { | ||||||||||||||||||||||||||||||||
const key = `${item.Credential.Issuer}-${item.Credential.CredentialType}` | ||||||||||||||||||||||||||||||||
// eslint-disable-next-line max-depth -- necessary to check for type-guards | ||||||||||||||||||||||||||||||||
if (seen.has(key)) { | ||||||||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
seen.add(key) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { | ||
BaseTransaction, | ||
isString, | ||
validateBaseTransaction, | ||
validateRequiredField, | ||
} from './common' | ||
|
||
export interface PermissionedDomainDelete extends BaseTransaction { | ||
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* The transaction type (PermissionedDomainDelete). */ | ||
TransactionType: 'PermissionedDomainDelete' | ||
|
||
mvadari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* The domain to delete. */ | ||
DomainID: string | ||
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Verify the form and type of a PermissionedDomainDelete transaction. | ||
* | ||
* @param tx - The transaction to verify. | ||
* @throws When the transaction is malformed. | ||
*/ | ||
export function validatePermissionedDomainDelete( | ||
tx: Record<string, unknown>, | ||
): void { | ||
validateBaseTransaction(tx) | ||
|
||
validateRequiredField(tx, 'DomainID', isString) | ||
} |
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.
This type name is really confusing here - can we alias it or something?
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.
I had to use the
AuthorizeCredential
name because I imported it from the Credential-related file. Please check the earlier PR review comments from @achowdhry-ripple in this pageThere 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.
Do you have a preference on the alias name?
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.
@mvadari I'd like to discuss this issue offline. I'll merge this PR at the moment because explorer changes need the client library