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

Support NFTokenMintOffer #2875

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
### Changed
* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber instead

### Added
* Support for XLS-52 (NFTokenMintOffer)
Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple Feb 12, 2025

Choose a reason for hiding this comment

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

Suggested change
* Support for XLS-52 (NFTokenMintOffer)
* Support for `NFTokenMintOffer` (XLS-52)


## 4.1.0 (2024-12-23)

### Added
Expand Down
37 changes: 37 additions & 0 deletions packages/xrpl/src/models/transactions/NFTokenMint.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { ValidationError } from '../../errors'
import { Amount } from '../common'
import { isHex } from '../utils'

import {
Account,
BaseTransaction,
GlobalFlags,
isAccount,
isAmount,
isNumber,
validateBaseTransaction,
validateOptionalField,
} from './common'
Expand Down Expand Up @@ -104,12 +107,34 @@ export interface NFTokenMint extends BaseTransaction {
* set to `undefined` value if you do not use it.
*/
URI?: string | null
/**
* Indicates the amount for the Token.
Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple Feb 12, 2025

Choose a reason for hiding this comment

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

Suggested change
* Indicates the amount for the Token.
* Indicates the amount expected for the Token.

*
* The amount can be zero. This would indicate that the account is giving
* the token away free, either to anyone at all, or to the account identified
* by the Destination field.
*/
Amount?: Amount
/**
* Indicates the time after which the offer will no longer
* be valid. The value is the number of seconds since the
* Ripple Epoch.
*/
Expiration?: number
/**
* If present, indicates that this offer may only be
* accepted by the specified account. Attempts by other
* accounts to accept this offer MUST fail.
*/
Destination?: Account
Flags?: number | NFTokenMintFlagsInterface
}

export interface NFTokenMintMetadata extends TransactionMetadataBase {
// rippled 1.11.0 or later
nftoken_id?: string
// if Amount is present
offer_id?: string
}

/**
Expand Down Expand Up @@ -140,4 +165,16 @@ export function validateNFTokenMint(tx: Record<string, unknown>): void {
if (tx.NFTokenTaxon == null) {
throw new ValidationError('NFTokenMint: missing field NFTokenTaxon')
}

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
if (!((tx.Amount == null && tx.Expiration == null && tx.Destination == null) || (tx.Amount != null && tx.Expiration != null && tx.Destination != null))) {
if (tx.Expiration != null || tx.Destination != null) {
throw new ValidationError(
'NFTokenMint: All three fields Amount, Expiration, Destination must be present',
)
}
}

We will need all three fields to be present (or) absent to make logical sense, isn't it? Isn't this a more comprehensive check?

I'm guessing this is already checked in rippled, so perhaps there is no need for a rigorous check here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

line 169 seems unnecessary as well

if (tx.Amount == null) {
if (tx.Expiration != null || tx.Destination != null) {
throw new ValidationError(
'NFTokenMint: Amount is required when Expiration or Destination is present',
)
}
}

validateOptionalField(tx, 'Amount', isAmount)
validateOptionalField(tx, 'Expiration', isNumber)
validateOptionalField(tx, 'Destination', isAccount)
Comment on lines +177 to +179
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: move these to the top so we validate all fields before any other logic that might use them

}
66 changes: 65 additions & 1 deletion packages/xrpl/test/integration/transactions/nftokenMint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import {
NFTokenMint,
TransactionMetadata,
TxRequest,
unixTimeToRippleTime,
Wallet,
xrpToDrops,
} from '../../../src'
import { hashSignedTx } from '../../../src/utils/hashes'
import serverUrl from '../serverUrl'
Expand All @@ -14,16 +17,18 @@ import {
teardownClient,
type XrplIntegrationTestContext,
} from '../setup'
import { testTransaction } from '../utils'
import { generateFundedWallet, testTransaction } from '../utils'

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

describe('NFTokenMint', function () {
let testContext: XrplIntegrationTestContext
let destinationWallet: Wallet

beforeEach(async () => {
testContext = await setupClient(serverUrl)
destinationWallet = await generateFundedWallet(testContext.client)
})
afterEach(async () => teardownClient(testContext))

Expand Down Expand Up @@ -91,4 +96,63 @@ describe('NFTokenMint', function () {
},
TIMEOUT,
)

it(
'test with Amount',
async function () {
const tx: NFTokenMint = {
TransactionType: 'NFTokenMint',
Account: testContext.wallet.address,
URI: convertStringToHex('https://www.google.com'),
NFTokenTaxon: 0,
Amount: xrpToDrops(1),
Expiration: unixTimeToRippleTime(Date.now() + 1000 * 60 * 60 * 24),
Destination: destinationWallet.address,
}
const response = await testTransaction(
testContext.client,
tx,
testContext.wallet,
)
assert.equal(response.type, 'response')

const txRequest: TxRequest = {
command: 'tx',
transaction: hashSignedTx(response.result.tx_blob),
}
const txResponse = await testContext.client.request(txRequest)

assert.equal(
(txResponse.result.meta as TransactionMetadata).TransactionResult,
'tesSUCCESS',
)

const nftokenID =
getNFTokenID(
txResponse.result.meta as TransactionMetadata<NFTokenMint>,
) ?? 'undefined'

const nftokenOfferID = (
txResponse.result.meta as TransactionMetadata<NFTokenMint>
).offer_id

const sellOffers = await testContext.client.request({
command: 'nft_sell_offers',
nft_id: nftokenID,
})

const existsOffer = sellOffers.result.offers.some(
(value) => value.nft_offer_index === nftokenOfferID,
)

assert.isTrue(
existsOffer,
`Expected to exist an offer for NFT with NFTokenID ${nftokenID} but did not find it.
\n\nHere's what was returned from 'nft_sell_offers': ${JSON.stringify(
sellOffers,
)}`,
)
},
TIMEOUT,
)
Comment on lines +100 to +157
Copy link

@coderabbitai coderabbitai bot Jan 10, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add negative test cases.

The test suite would benefit from additional test cases that verify error handling:

  • Invalid Amount (zero or negative)
  • Expired timestamp
  • Invalid destination address

Would you like me to help generate these negative test cases?

Copy link

Choose a reason for hiding this comment

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

@achowdhry-ripple Here are the additional test cases to verify error handling:

it(
  'should fail with zero Amount',
  async function () {
    const tx: NFTokenMint = {
      TransactionType: 'NFTokenMint',
      Account: testContext.wallet.address,
      URI: convertStringToHex('https://www.google.com'),
      NFTokenTaxon: 0,
      Amount: '0',
      Destination: destinationWallet.address,
    }
    
    await assert.isRejected(
      testTransaction(testContext.client, tx, testContext.wallet),
      /Amount must be greater than zero/,
    )
  },
  TIMEOUT,
)

it(
  'should fail with expired timestamp',
  async function () {
    const tx: NFTokenMint = {
      TransactionType: 'NFTokenMint',
      Account: testContext.wallet.address,
      URI: convertStringToHex('https://www.google.com'),
      NFTokenTaxon: 0,
      Amount: xrpToDrops(1),
      Expiration: unixTimeToRippleTime(Date.now() - 1000 * 60), // 1 minute ago
      Destination: destinationWallet.address,
    }
    
    await assert.isRejected(
      testTransaction(testContext.client, tx, testContext.wallet),
      /Expiration time must be in the future/,
    )
  },
  TIMEOUT,
)

it(
  'should fail with invalid destination address',
  async function () {
    const tx: NFTokenMint = {
      TransactionType: 'NFTokenMint',
      Account: testContext.wallet.address,
      URI: convertStringToHex('https://www.google.com'),
      NFTokenTaxon: 0,
      Amount: xrpToDrops(1),
      Destination: 'invalid_address',
    }
    
    await assert.isRejected(
      testTransaction(testContext.client, tx, testContext.wallet),
      /invalid address/i,
    )
  },
  TIMEOUT,
)

These test cases verify that the transaction fails appropriately when:

  1. Amount is set to zero
  2. Expiration is set to a past timestamp
  3. Destination address is invalid

Would you like me to add any other test cases or modify these?

})
52 changes: 52 additions & 0 deletions packages/xrpl/test/models/NFTokenMint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,56 @@ describe('NFTokenMint', function () {
'NFTokenMint: URI must be in hex format',
)
})

it(`throws when Amount is null but Expiration is present`, function () {
const invalid = {
TransactionType: 'NFTokenMint',
Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
Fee: '5000000',
Sequence: 2470665,
NFTokenTaxon: 0,
Issuer: 'r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ',
Expiration: 123456,
} as any

assert.throws(
() => validate(invalid),
ValidationError,
'NFTokenMint: Amount is required when Expiration or Destination is present',
)
})

it(`throws when Amount is null but Destination is present`, function () {
const invalid = {
TransactionType: 'NFTokenMint',
Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
Fee: '5000000',
Sequence: 2470665,
NFTokenTaxon: 0,
Issuer: 'r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ',
Destination: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn',
} as any

assert.throws(
() => validate(invalid),
ValidationError,
'NFTokenMint: Amount is required when Expiration or Destination is present',
)
})

it(`verifies valid NFTokenMint with Amount, Destination and Expiration`, function () {
const valid = {
TransactionType: 'NFTokenMint',
Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
Fee: '5000000',
Sequence: 2470665,
NFTokenTaxon: 0,
Issuer: 'r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ',
Amount: '1000000',
Destination: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn',
Expiration: 123456,
} as any

assert.doesNotThrow(() => validate(valid))
})
})
Loading