Skip to content
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

MPT #2661

Merged
merged 61 commits into from
Dec 11, 2024
Merged

MPT #2661

merged 61 commits into from
Dec 11, 2024

Conversation

shawnxie999
Copy link
Collaborator

@shawnxie999 shawnxie999 commented Mar 14, 2024

High Level Overview of Change

https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update HISTORY.md?

  • Yes
  • No, this change does not impact library users

Test Plan

Comment on lines 2780 to 2788
"temBAD_MPTOKEN_TRANSFER_FEE": -261,
"temBAD_AMM_TOKENS": -260,
"temXCHAIN_EQUAL_DOOR_ACCOUNTS": -259,
"temXCHAIN_BAD_PROOF": -258,
"temXCHAIN_BRIDGE_BAD_ISSUES": -257,
"temXCHAIN_BRIDGE_NONDOOR_OWNER": -256,
"temXCHAIN_BRIDGE_BAD_MIN_ACCOUNT_CREATE_AMOUNT": -255,
"temXCHAIN_BRIDGE_BAD_REWARD_AMOUNT": -254,
"temEMPTY_DID": -253,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes all the numbers, which isn't backwards-compatible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the README, I thought auto-generating this is sufficient, or do I need to manually add them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Auto-generating is sufficient, I'm saying this is a rippled problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is only a problem if you plan to merge this PR in as-is - if it's just for a beta version or something, it doesn't really matter.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
packages/xrpl/src/models/utils/index.ts (1)

2-2: Add documentation for the new constant.

Please add JSDoc comments explaining:

  • The purpose of this constant
  • Expected input format
  • Usage examples
  • Any limitations (e.g., no decimals, no negatives)

Apply this diff:

+/**
+ * Regular expression for validating that a string contains only digits.
+ * Used for validating numeric inputs in MPT transactions.
+ * Note: This pattern only allows positive integers (no decimals, no negatives).
+ * @example
+ * INTEGER_SANITY_CHECK.test("123") // true
+ * INTEGER_SANITY_CHECK.test("-123") // false
+ * INTEGER_SANITY_CHECK.test("12.3") // false
+ */
 export const INTEGER_SANITY_CHECK = /^[0-9]+$/u
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)

2-6: Consolidate imports and correct import order

To improve code readability and adhere to best practices, please consolidate multiple imports from '../utils' into a single import statement. Additionally, ensure that the import statements are grouped and ordered appropriately.

Apply this diff to address the issues:

-import { isHex } from '../utils'
...
-import { INTEGER_SANITY_CHECK } from '../utils'
...
+import { isHex, INTEGER_SANITY_CHECK } from '../utils'
🧰 Tools
🪛 eslint

[error] 2-2: '/home/jailuser/git/packages/xrpl/src/models/utils/index.ts' imported multiple times.

(import/no-duplicates)


[error] 5-5: There should be at least one empty line between import groups

(import/order)


[error] 6-6: ../utils import should occur before import of ./common

(import/order)


[error] 6-6: '/home/jailuser/git/packages/xrpl/src/models/utils/index.ts' imported multiple times.

(import/no-duplicates)

.github/workflows/nodejs.yml (3)

Line range hint 17-33: Consider adding Node.js 20.x to the 'build-and-lint' job for consistency

The 'unit', 'integration', and 'snippets' jobs are configured to run on both Node.js 18.x and 20.x. However, the 'build-and-lint' job is only running on Node.js 18.x. For consistency and to ensure compatibility across all supported Node.js versions, consider adding Node.js 20.x to the 'build-and-lint' job.

Apply this diff to update the 'build-and-lint' job:

 strategy:
   matrix:
-    node-version: [18.x]
+    node-version: [18.x, 20.x]

Line range hint 151-158: Consider adding Node.js 20.x to the 'browser' job for comprehensive testing

To ensure that browser tests are compatible with all supported Node.js versions, consider adding Node.js 20.x to the 'browser' job's matrix.

Apply this diff to update the 'browser' job:

 strategy:
   matrix:
-    node-version: [18.x]
+    node-version: [18.x, 20.x]

Line range hint 183-184: Update step name to 'Run browser test' for clarity

The step currently named 'Run integration test' is executing npm run test:browser. To improve clarity, update the step name to reflect the action being performed.

Apply this diff:

-  - name: Run integration test
+  - name: Run browser test
   run: npm run test:browser
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d861672 and e6b5c23.

📒 Files selected for processing (7)
  • .github/workflows/nodejs.yml (3 hunks)
  • packages/ripple-binary-codec/HISTORY.md (1 hunks)
  • packages/xrpl/HISTORY.md (1 hunks)
  • packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1 hunks)
  • packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1 hunks)
  • packages/xrpl/src/models/transactions/payment.ts (3 hunks)
  • packages/xrpl/src/models/utils/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts
  • packages/xrpl/HISTORY.md
🧰 Additional context used
📓 Learnings (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🪛 eslint
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts

[error] 2-2: '/home/jailuser/git/packages/xrpl/src/models/utils/index.ts' imported multiple times.

(import/no-duplicates)


[error] 5-5: There should be at least one empty line between import groups

(import/order)


[error] 6-6: ../utils import should occur before import of ./common

(import/order)


[error] 6-6: '/home/jailuser/git/packages/xrpl/src/models/utils/index.ts' imported multiple times.

(import/no-duplicates)

🔇 Additional comments (4)
packages/xrpl/src/models/utils/index.ts (1)

2-2: Consider enhancing the validation pattern.

The current pattern allows leading zeros and might be too restrictive for some use cases. Consider:

  1. Preventing leading zeros: /^[1-9][0-9]*$/u
  2. Supporting optional decimal places
  3. Supporting negative numbers if needed

Let's verify how this constant is being used across the codebase:

packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)

114-143: 🛠️ Refactor suggestion

Add validation for TransferFee and AssetScale using helper functions

The validateMPTokenIssuanceCreate function currently lacks validation for the optional fields TransferFee and AssetScale. To ensure data integrity and consistency, please add validation for these fields using existing helper functions like validateOptionalField, isNumber, and isInteger, as per the retrieved learnings.

Apply this diff to add the necessary validation:

if (tx.AssetScale !== undefined) {
  validateOptionalField(tx, 'AssetScale', [isInteger])
  if (tx.AssetScale < 0) {
    throw new ValidationError('MPTokenIssuanceCreate: AssetScale must be non-negative')
  }
}

if (tx.TransferFee !== undefined) {
  validateOptionalField(tx, 'TransferFee', [isNumber])
  if (tx.TransferFee < 0 || tx.TransferFee > 50000) {
    throw new ValidationError('MPTokenIssuanceCreate: TransferFee must be between 0 and 50000 inclusive')
  }

  const flags = tx.Flags ?? 0
  const canTransferFlag = typeof flags === 'number'
    ? (flags & MPTokenIssuanceCreateFlags.tfMPTCanTransfer) !== 0
    : flags.tfMPTCanTransfer === true

  if (!canTransferFlag) {
    throw new ValidationError('MPTokenIssuanceCreate: TransferFee must not be present if tfMPTCanTransfer flag is not set')
  }
}
⛔ Skipped due to learnings
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/ripple-binary-codec/HISTORY.md (1)

5-7: Well-documented addition to the release history

The addition clearly documents the new support for the Multi-Purpose Token amendment, maintaining consistency with the existing formatting.

packages/xrpl/src/models/transactions/payment.ts (1)

119-119: Include MPTAmount within Amount type to simplify code

By extending the Amount type to include MPTAmount, you can avoid modifying multiple transaction models to accept Amount | MPTAmount. This change enhances maintainability and reduces redundancy across the codebase, as suggested in past review comments.

Apply this change to the Amount type definition in common/index.ts:

export type Amount = string | IssuedCurrencyAmount | MPTAmount

Then, you can revert the changes in this file where Amount was replaced with Amount | MPTAmount:

-import { Amount, Path, MPTAmount } from '../common'
+import { Amount, Path } from '../common'

...

-Amount: Amount | MPTAmount
+Amount: Amount

...

-SendMax?: Amount | MPTAmount
+SendMax?: Amount

...

-DeliverMin?: Amount | MPTAmount
+DeliverMin?: Amount

...

-DeliveredAmount?: Amount | MPTAmount
+DeliveredAmount?: Amount

-delivered_amount?: Amount | MPTAmount | 'unavailable'
+delivered_amount?: Amount | 'unavailable' | 'unavailable'

Also applies to: 145-145, 151-151, 156-157

@achowdhry-ripple
Copy link
Collaborator

achowdhry-ripple commented Dec 6, 2024

Seeing the integration tests failing now, will need to be resolved. Looks like they are running now so the docker part is good. Might still need to use a different version of the docker image, there are a lot after rc1 and I suspect the temDisabled response has to do with it

@achowdhry-ripple
Copy link
Collaborator

achowdhry-ripple commented Dec 6, 2024

nit: I also agree with the typos raised by coderabbit, should be quick fixes

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
packages/xrpl/test/models/MPTokenAuthorize.test.ts (2)

7-11: Enhance test suite documentation

Consider expanding the JSDoc comments to include:

  • The purpose of MPTokenAuthorize transactions
  • The specific validation rules being tested
  • Examples of valid/invalid scenarios covered

13-49: Consider consolidating test cases

The test cases could be more maintainable using a parameterized approach:

interface TestCase {
  description: string
  input: {
    Account: string
    MPTokenIssuanceID: string
    Holder?: string
    Flags?: number
  }
}

const testCases: TestCase[] = [
  {
    description: 'basic valid case',
    input: {
      Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
      MPTokenIssuanceID: TOKEN_ID
    }
  },
  // Add other test cases here
]

testCases.forEach(({ description, input }) => {
  it(`verifies ${description}`, () => {
    const tx = {
      TransactionType: 'MPTokenAuthorize',
      ...input
    }
    assert.doesNotThrow(() => validate(tx))
  })
})
packages/ripple-binary-codec/test/uint.test.ts (1)

99-116: Consider documenting the MPTokenMetadata structure

The MPTokenMetadata hex string contains a complex JSON structure with important token details. Consider adding a comment with the decoded JSON structure for better readability and maintainability.

Here's the decoded metadata for reference:

// MPTokenMetadata decoded:
{
  "name": "US Treasury Bill Token",
  "symbol": "USTBT",
  "decimals": 2,
  "totalSupply": 100000,
  "issuer": "US Treasury",
  "issueDate": "2024-03-25",
  "maturityDate": "2025-03-25",
  "faceValue": "1000",
  "interestRate": "2.5",
  "interestFrequency": "Quarterly",
  "collateral": "US Government",
  "jurisdiction": "United States",
  "regulatoryCompliance": "SEC Regulations",
  "securityType": "Treasury Bill",
  "external_url": "https://example.com/t-bill-token-metadata.json"
}
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)

132-143: Improve MaximumAmount validation using helper functions.

Per the learnings, we should utilize helper functions for validation where appropriate.

Apply this diff:

  if (typeof tx.MaximumAmount === 'string') {
+    validateOptionalField(tx, 'MaximumAmount', ['string'])
+
    if (!INTEGER_SANITY_CHECK.exec(tx.MaximumAmount)) {
      throw new ValidationError('MPTokenIssuanceCreate: Invalid MaximumAmount')
-    } else if (
+    }
+    
+    if (
      BigInt(tx.MaximumAmount) > BigInt(MAX_AMT) ||
      BigInt(tx.MaximumAmount) < BigInt(`0`)
    ) {
      throw new ValidationError(
        'MPTokenIssuanceCreate: MaximumAmount out of range',
      )
    }
  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1668e76 and a8f5abf.

📒 Files selected for processing (5)
  • packages/ripple-binary-codec/test/uint.test.ts (3 hunks)
  • packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1 hunks)
  • packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts (1 hunks)
  • packages/xrpl/test/models/MPTokenAuthorize.test.ts (1 hunks)
  • packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts
  • packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
🧰 Additional context used
📓 Learnings (2)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🪛 Gitleaks (8.21.2)
packages/ripple-binary-codec/test/uint.test.ts

123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

packages/xrpl/test/models/MPTokenAuthorize.test.ts

5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🪛 eslint
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts

[error] 147-149: Expected { after 'if' condition.

(curly)

🔇 Additional comments (7)
packages/xrpl/test/models/MPTokenAuthorize.test.ts (1)

31-39: Remove duplicate test case

This test case is identical to the previous one and can be removed.

packages/ripple-binary-codec/test/uint.test.ts (3)

1-2: LGTM: Clean import addition

The addition of the decode import alongside encode is well-organized and necessary for the new test cases.


118-131: LGTM: Valid test MPTokenIssuanceID format

The MPTokenIssuanceID follows the correct format for MPT issuance identifiers. While it was flagged by static analysis as a potential API key, this is a false positive as this is a valid test identifier following the MPT standard format.

🧰 Tools
🪛 Gitleaks (8.21.2)

123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


182-197: LGTM: Comprehensive MPT amount parsing tests

The test case thoroughly validates:

  • Encoding/decoding of both issuance and token entries
  • String type preservation for amount fields
  • Correct handling of large numbers (MaximumAmount)
  • Precise amount values after encoding/decoding cycles
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (3)

1-9: LGTM! Well-structured imports and constants.

The imports are appropriate, and the MAX_AMT constant is well-documented with its mathematical significance (2^63 - 1).


10-60: LGTM! Well-structured and documented flag definitions.

The flag definitions are comprehensive with clear documentation. The bit patterns follow a logical progression, and the interface properly extends GlobalFlags.


61-107: LGTM! Well-defined interfaces with clear documentation.

The interfaces are well-structured with comprehensive documentation for each field, including constraints and examples where appropriate.

@shawnxie999
Copy link
Collaborator Author

@achowdhry-ripple fixed comments from coderabbits that are relevant. previously the integration test failed with temDISABLED because the rippled cfg didn't include the MPT amendment.

Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

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

LGTM

@tequdev
Copy link
Contributor

tequdev commented Dec 11, 2024

MPToken is an asset with a balance, don't we need to change to getBalanceChanges(packages/xrpl/src/utils/getBalanceChanges.ts)?

Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple left a comment

Choose a reason for hiding this comment

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

LGTM

@shawnxie999
Copy link
Collaborator Author

MPToken is an asset with a balance, don't we need to change to getBalanceChanges(packages/xrpl/src/utils/getBalanceChanges.ts)?

Good catch! Yes, we can support this later separately.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)

13-14: Consider a more descriptive constant name

The constant MAX_AMT could be renamed to MAX_MAXIMUM_AMOUNT or MAX_TOKEN_AMOUNT to better reflect its purpose in the context of MP Token issuance.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ccf54de and 46814fa.

📒 Files selected for processing (1)
  • packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🔇 Additional comments (6)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (6)

21-51: LGTM! Well-documented flags with clear bit values

The enum is well-structured with comprehensive documentation for each flag, following consistent naming patterns and proper bit value assignments.


59-66: LGTM! Proper flag mapping interface

The interface correctly extends GlobalFlags and provides optional boolean mappings for all MPTokenIssuanceCreateFlags.


88-89: Clarify MaximumAmount type in documentation

The documentation should explicitly mention that MaximumAmount must be an integer string, not a raw integer, to avoid confusion.


111-113: LGTM! Clean metadata interface

The metadata interface is properly defined with the optional issuance ID field.


128-138: Improve MPTokenMetadata validation using helper functions

The current validation can be simplified and made more robust using the helper functions as suggested in the learnings.


153-161: ⚠️ Potential issue

Add missing TransferFee flag validation

The TransferFee validation should check if the tfMPTCanTransfer flag is set when TransferFee is present, as specified in the interface documentation.

Apply this diff:

  const MAX_TRANSFER_FEE = 50000
  if (typeof tx.TransferFee === 'number') {
    if (tx.TransferFee < 0 || tx.TransferFee > MAX_TRANSFER_FEE) {
      throw new ValidationError(
        'MPTokenIssuanceCreate: TransferFee out of range',
      )
    }
+   
+   const flags = tx.Flags ?? 0
+   const canTransferFlag = typeof flags === 'number'
+     ? (flags & MPTokenIssuanceCreateFlags.tfMPTCanTransfer) !== 0
+     : (flags as MPTokenIssuanceCreateFlagsInterface).tfMPTCanTransfer === true
+   
+   if (!canTransferFlag) {
+     throw new ValidationError(
+       'MPTokenIssuanceCreate: TransferFee must not be present if tfMPTCanTransfer flag is not set',
+     )
+   }
  }

Likely invalid or redundant comment.

Comment on lines +121 to +161
export function validateMPTokenIssuanceCreate(
tx: Record<string, unknown>,
): void {
validateBaseTransaction(tx)
validateOptionalField(tx, 'MaximumAmount', isString)
validateOptionalField(tx, 'MPTokenMetadata', isString)

if (typeof tx.MPTokenMetadata === 'string' && tx.MPTokenMetadata === '') {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must not be empty string',
)
}

if (typeof tx.MPTokenMetadata === 'string' && !isHex(tx.MPTokenMetadata)) {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must be in hex format',
)
}

if (typeof tx.MaximumAmount === 'string') {
if (!INTEGER_SANITY_CHECK.exec(tx.MaximumAmount)) {
throw new ValidationError('MPTokenIssuanceCreate: Invalid MaximumAmount')
} else if (
BigInt(tx.MaximumAmount) > BigInt(MAX_AMT) ||
BigInt(tx.MaximumAmount) < BigInt(`0`)
) {
throw new ValidationError(
'MPTokenIssuanceCreate: MaximumAmount out of range',
)
}
}

const MAX_TRANSFER_FEE = 50000
if (typeof tx.TransferFee === 'number') {
if (tx.TransferFee < 0 || tx.TransferFee > MAX_TRANSFER_FEE) {
throw new ValidationError(
'MPTokenIssuanceCreate: TransferFee out of range',
)
}
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing AssetScale validation

The function is missing validation for the AssetScale field, which should be a non-negative integer according to the interface documentation.

Add this validation after line 127:

  validateBaseTransaction(tx)
  validateOptionalField(tx, 'MaximumAmount', isString)
  validateOptionalField(tx, 'MPTokenMetadata', isString)

+ if (tx.AssetScale !== undefined) {
+   if (typeof tx.AssetScale !== 'number' || !Number.isInteger(tx.AssetScale) || tx.AssetScale < 0) {
+     throw new ValidationError('MPTokenIssuanceCreate: AssetScale must be a non-negative integer')
+   }
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function validateMPTokenIssuanceCreate(
tx: Record<string, unknown>,
): void {
validateBaseTransaction(tx)
validateOptionalField(tx, 'MaximumAmount', isString)
validateOptionalField(tx, 'MPTokenMetadata', isString)
if (typeof tx.MPTokenMetadata === 'string' && tx.MPTokenMetadata === '') {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must not be empty string',
)
}
if (typeof tx.MPTokenMetadata === 'string' && !isHex(tx.MPTokenMetadata)) {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must be in hex format',
)
}
if (typeof tx.MaximumAmount === 'string') {
if (!INTEGER_SANITY_CHECK.exec(tx.MaximumAmount)) {
throw new ValidationError('MPTokenIssuanceCreate: Invalid MaximumAmount')
} else if (
BigInt(tx.MaximumAmount) > BigInt(MAX_AMT) ||
BigInt(tx.MaximumAmount) < BigInt(`0`)
) {
throw new ValidationError(
'MPTokenIssuanceCreate: MaximumAmount out of range',
)
}
}
const MAX_TRANSFER_FEE = 50000
if (typeof tx.TransferFee === 'number') {
if (tx.TransferFee < 0 || tx.TransferFee > MAX_TRANSFER_FEE) {
throw new ValidationError(
'MPTokenIssuanceCreate: TransferFee out of range',
)
}
}
}
export function validateMPTokenIssuanceCreate(
tx: Record<string, unknown>,
): void {
validateBaseTransaction(tx)
validateOptionalField(tx, 'MaximumAmount', isString)
validateOptionalField(tx, 'MPTokenMetadata', isString)
if (tx.AssetScale !== undefined) {
if (typeof tx.AssetScale !== 'number' || !Number.isInteger(tx.AssetScale) || tx.AssetScale < 0) {
throw new ValidationError('MPTokenIssuanceCreate: AssetScale must be a non-negative integer')
}
}
if (typeof tx.MPTokenMetadata === 'string' && tx.MPTokenMetadata === '') {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must not be empty string',
)
}
if (typeof tx.MPTokenMetadata === 'string' && !isHex(tx.MPTokenMetadata)) {
throw new ValidationError(
'MPTokenIssuanceCreate: MPTokenMetadata must be in hex format',
)
}
if (typeof tx.MaximumAmount === 'string') {
if (!INTEGER_SANITY_CHECK.exec(tx.MaximumAmount)) {
throw new ValidationError('MPTokenIssuanceCreate: Invalid MaximumAmount')
} else if (
BigInt(tx.MaximumAmount) > BigInt(MAX_AMT) ||
BigInt(tx.MaximumAmount) < BigInt(`0`)
) {
throw new ValidationError(
'MPTokenIssuanceCreate: MaximumAmount out of range',
)
}
}
const MAX_TRANSFER_FEE = 50000
if (typeof tx.TransferFee === 'number') {
if (tx.TransferFee < 0 || tx.TransferFee > MAX_TRANSFER_FEE) {
throw new ValidationError(
'MPTokenIssuanceCreate: TransferFee out of range',
)
}
}
}

@achowdhry-ripple
Copy link
Collaborator

MPToken is an asset with a balance, don't we need to change to getBalanceChanges(packages/xrpl/src/utils/getBalanceChanges.ts)?

Also agree, this is a good catch. If its not too much overhead would say it should go out with this, otherwise we should update the JSDoc to say this does not include MPTs for now

@shawnxie999
Copy link
Collaborator Author

@achowdhry-ripple i would prefer doing it separately, may not be necessary to hold off merging the main change for a sub-component

@shawnxie999 shawnxie999 merged commit b04efe8 into XRPLF:main Dec 11, 2024
13 checks passed
@achowdhry-ripple
Copy link
Collaborator

@achowdhry-ripple i would prefer doing it separately, may not be necessary to hold off merging the main change for a sub-component

In that case, could you raise a github issue here so we can track it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants