-
Notifications
You must be signed in to change notification settings - Fork 524
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
base: main
Are you sure you want to change the base?
Support NFTokenMintOffer #2875
Changes from all commits
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 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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' | ||||||||||||||||||
|
@@ -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. | ||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||
* | ||||||||||||||||||
* 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 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
|
@@ -140,4 +165,16 @@ export function validateNFTokenMint(tx: Record<string, unknown>): void { | |||||||||||||||||
if (tx.NFTokenTaxon == null) { | ||||||||||||||||||
throw new ValidationError('NFTokenMint: missing field NFTokenTaxon') | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
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.
Suggested change
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. 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. 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
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. nit: move these to the top so we validate all fields before any other logic that might use them |
||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,9 @@ import { | |
NFTokenMint, | ||
TransactionMetadata, | ||
TxRequest, | ||
unixTimeToRippleTime, | ||
Wallet, | ||
xrpToDrops, | ||
} from '../../../src' | ||
import { hashSignedTx } from '../../../src/utils/hashes' | ||
import serverUrl from '../serverUrl' | ||
|
@@ -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)) | ||
|
||
|
@@ -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
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 Add negative test cases. The test suite would benefit from additional test cases that verify error handling:
Would you like me to help generate these negative test cases? 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 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:
Would you like me to add any other test cases or modify these? |
||
}) |
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.