-
Notifications
You must be signed in to change notification settings - Fork 539
Support NFTokenMintOffer #2875
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
Changes from all commits
c1a73fe
3a3b0e4
b6eb061
62d224e
d8ce312
d83199d
e0cae54
f0cd311
22fb4bb
4fbfffc
c7418c3
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 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 | ||
} | ||
|
||
/** | ||
|
@@ -140,4 +165,16 @@ export function validateNFTokenMint(tx: Record<string, unknown>): void { | |
if (tx.NFTokenTaxon == null) { | ||
throw new ValidationError('NFTokenMint: missing field NFTokenTaxon') | ||
} | ||
|
||
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 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. It is probably better for developers to check for cases where values should not exist before checking for values. But I have no objection to making the change. What do you think? 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. More of a code cleanup to stay consistent with the repo, and makes the logic a bit easier to follow. I don't feel strongly about this though so ultimately up to you |
||
} |
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, | ||
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 fix to the
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 This test uses rippled APIv2 (default version, if 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 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. @tequdev if you'd like to address this comment, we can wait. |
||
'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 commentThe reason will be displayed to describe this comment to others. Learn more. these cases would have been covered in the rippled cpp implementation. Usually, client libraries forward these transactions to the rippled node. 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.
|
||
}) |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
line 169 seems unnecessary as well
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.
Expiration
andDestination
can only be set ifAmount
is present, but they are still optional fields.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.
What happens if a developer specifies
Destination
field but not theAmount
field?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.
it will be
temMALFORMED
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, but that error is returned from the
rippled
node. I feel we can catch that error inside client-libraries itself.However, this is just a suggestion. I like the PR even without this change.