-
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 21 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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_CREDENTIALS_ARRAY_LENGTH = 8 | ||||||
|
||||||
| export const MAX_CREDENTIALS_ARRAY_LENGTH = 8 | |
| export const MAX_AUTHORIZED_CREDENTIALS = 8 |
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 don't have strong preference about your variable-name-change suggestions, but they are used in multiple places across the codebase. Furthermore, these names are suited to the subjective preferences of the author.
If you could provide me a commit/patch will all the changes, I'm happy to incorporate that. Sorry, I'm a little pressed for time.
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.
Most IDEs will allow you to update all at once with F2 (or you can do a ctrl-f-replace). IMO this name as-is is incorrect/misleading since DepositPreauth and Permissioned Domains use different lengths of arrays.
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.
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
The name appropriately suggests the intention. Since PDSet transaction also uses AcceptedCredentials as the field name, users can understand the interpretation.
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.
Yes, it's AcceptedCredentials, not AuthorizeCredentials there. That's my point.
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.
@achowdhry-ripple If I rename this to AcceptedCredentials, will it confuse the Credential's usage? Should I rename it to isValidCredential instead?
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
Outdated
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.
| maxLengthCredentialsArray: number, | |
| maxCredentials: number, |
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.
khancode marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| 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
AuthorizeCredentialname 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