Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Implement getNFT function of NFT module #9034

Merged
merged 5 commits into from
Sep 28, 2023
Merged

Implement getNFT function of NFT module #9034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf507ef
Implement and use method getft
Incede Sep 25, 2023
cd9fe08
Added and updated unit tests
Incede Sep 25, 2023
b1f62e7
Add check per feedback
Incede Sep 26, 2023
e123602
Use json type per feedback
Incede Sep 27, 2023
adbfbde
Update check with has per feedback
Incede Sep 28, 2023
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
14 changes: 11 additions & 3 deletions framework/src/modules/nft/cc_commands/cc_transfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
import { InternalMethod } from '../internal_method';
import { BaseCCCommand } from '../../interoperability/base_cc_command';
import { CrossChainMessageContext } from '../../interoperability/types';
import { MAX_RESERVED_ERROR_STATUS } from '../../interoperability/constants';
import { CCMStatusCode, MAX_RESERVED_ERROR_STATUS } from '../../interoperability/constants';
import { FeeMethod } from '../types';
import { EscrowStore } from '../stores/escrow';
import { CcmTransferEvent } from '../events/ccm_transfer';
Expand Down Expand Up @@ -75,12 +75,20 @@ export class CrossChainTransferCommand extends BaseCCCommand {
throw new Error('Non-existent entry in the NFT substore');
}

const owner = await this._method.getNFTOwner(getMethodContext(), nftID);
if (!owner.equals(sendingChainID)) {
const nft = await nftStore.get(getMethodContext(), nftID);
if (!nft.owner.equals(sendingChainID)) {
throw new Error('NFT has not been properly escrowed');
}
}

if (
!nftChainID.equals(ownChainID) &&
(ccm.status === CCMStatusCode.MODULE_NOT_SUPPORTED ||
ccm.status === CCMStatusCode.CROSS_CHAIN_COMMAND_NOT_SUPPORTED)
) {
throw new Error('Module or cross-chain command not supported');
}

if (!nftChainID.equals(ownChainID) && nftExists) {
throw new Error('NFT substore entry already exists');
}
Expand Down
27 changes: 9 additions & 18 deletions framework/src/modules/nft/commands/transfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ import {
} from '../../../state_machine';
import { BaseCommand } from '../../base_command';
import { transferParamsSchema } from '../schemas';
import { NFTStore } from '../stores/nft';
import { NFTMethod } from '../method';
import { LENGTH_CHAIN_ID, NFT_NOT_LOCKED } from '../constants';
import { InternalMethod } from '../internal_method';

export interface Params {
Expand All @@ -43,31 +41,24 @@ export class TransferCommand extends BaseCommand {

public async verify(context: CommandVerifyContext<Params>): Promise<VerificationResult> {
const { params } = context;
const methodContext = context.getMethodContext();

const nftStore = this.stores.get(NFTStore);

const nftExists = await nftStore.has(context, params.nftID);

if (!nftExists) {
throw new Error('NFT substore entry does not exist');
let nft;
try {
nft = await this._method.getNFT(methodContext, params.nftID);
} catch (error) {
throw new Error('NFT does not exist');
Comment on lines +46 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason the code is catching and re-throwing the error from getNFT() method?

getNFT() already throws 2 types of errors, depending on if the entry is missing from NFT store or from User store.

It seems that this try/catch not only complicates the command verification, but also provide less accurate error message than if there was no try/catch here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular case (and verify hook of other commands), I think it makes sense to emit more granular error(s). And yes it'll get rid of the try-catch. Since this code will already be replaced by verifyTransfer, I'll apply the suggestion directly in the internal functions in PR #9005 instead of redundant update here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

applies to other places, but the error can be false because it's possible to have error with different reason. (ex: DB is dead)
It would be better not to ignore the error received. At least, we should wrap it if we want to have additional message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update in the internal functions in PR #9005 to catch and throw if error is other than expected 👍

}

const owner = await this._method.getNFTOwner(context.getMethodContext(), params.nftID);

if (owner.length === LENGTH_CHAIN_ID) {
if (this._method.isNFTEscrowed(nft)) {
throw new Error('NFT is escrowed to another chain');
}

if (!owner.equals(context.transaction.senderAddress)) {
if (!nft.owner.equals(context.transaction.senderAddress)) {
throw new Error('Transfer not initiated by the NFT owner');
}

const lockingModule = await this._method.getLockingModule(
context.getMethodContext(),
params.nftID,
);

if (lockingModule !== NFT_NOT_LOCKED) {
if (this._method.isNFTLocked(nft)) {
throw new Error('Locked NFTs cannot be transferred');
}

Expand Down
30 changes: 11 additions & 19 deletions framework/src/modules/nft/commands/transfer_cross_chain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@
*/

import { crossChainTransferParamsSchema } from '../schemas';
import { NFTStore } from '../stores/nft';
import { NFTMethod } from '../method';
import { LENGTH_CHAIN_ID, NFT_NOT_LOCKED } from '../constants';
import { InteroperabilityMethod, TokenMethod } from '../types';
import { BaseCommand } from '../../base_command';
import {
Expand Down Expand Up @@ -57,21 +55,20 @@ export class TransferCrossChainCommand extends BaseCommand {

public async verify(context: CommandVerifyContext<Params>): Promise<VerificationResult> {
const { params } = context;

const nftStore = this.stores.get(NFTStore);
const nftExists = await nftStore.has(context.getMethodContext(), params.nftID);
const methodContext = context.getMethodContext();

if (params.receivingChainID.equals(context.chainID)) {
throw new Error('Receiving chain cannot be the sending chain');
}

if (!nftExists) {
throw new Error('NFT substore entry does not exist');
let nft;
try {
nft = await this._nftMethod.getNFT(methodContext, params.nftID);
} catch (error) {
throw new Error('NFT does not exist');
}

const owner = await this._nftMethod.getNFTOwner(context.getMethodContext(), params.nftID);

if (owner.length === LENGTH_CHAIN_ID) {
if (this._nftMethod.isNFTEscrowed(nft)) {
throw new Error('NFT is escrowed to another chain');
}

Expand All @@ -82,25 +79,20 @@ export class TransferCrossChainCommand extends BaseCommand {
}

const messageFeeTokenID = await this._interoperabilityMethod.getMessageFeeTokenID(
context.getMethodContext(),
methodContext,
params.receivingChainID,
);

if (!owner.equals(context.transaction.senderAddress)) {
if (!nft.owner.equals(context.transaction.senderAddress)) {
throw new Error('Transfer not initiated by the NFT owner');
}

const lockingModule = await this._nftMethod.getLockingModule(
context.getMethodContext(),
params.nftID,
);

if (lockingModule !== NFT_NOT_LOCKED) {
if (this._nftMethod.isNFTLocked(nft)) {
throw new Error('Locked NFTs cannot be transferred');
}

const availableBalance = await this._tokenMethod.getAvailableBalance(
context.getMethodContext(),
methodContext,
context.transaction.senderAddress,
messageFeeTokenID,
);
Expand Down
15 changes: 11 additions & 4 deletions framework/src/modules/nft/endpoint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
import { NFTStore } from './stores/nft';
import { ALL_SUPPORTED_NFTS_KEY, LENGTH_ADDRESS, LENGTH_NFT_ID } from './constants';
import { UserStore } from './stores/user';
import { NFT } from './types';
import { NFTJSON } from './types';
import { SupportedNFTsStore } from './stores/supported_nfts';
import { NFTMethod } from './method';

Expand All @@ -41,7 +41,7 @@ export class NFTEndpoint extends BaseEndpoint {

public async getNFTs(
context: ModuleEndpointContext,
): Promise<{ nfts: JSONObject<Omit<NFT, 'owner'> & { id: string }>[] }> {
): Promise<{ nfts: JSONObject<Omit<NFTJSON, 'owner'> & { id: string }>[] }> {
validator.validate<{ address: string }>(getNFTsRequestSchema, context.params);

const nftStore = this.stores.get(NFTStore);
Expand Down Expand Up @@ -97,7 +97,7 @@ export class NFTEndpoint extends BaseEndpoint {
return { hasNFT: nftData.owner.equals(owner) };
}

public async getNFT(context: ModuleEndpointContext): Promise<JSONObject<NFT>> {
public async getNFT(context: ModuleEndpointContext): Promise<JSONObject<NFTJSON>> {
const { params } = context;
validator.validate<{ id: string }>(getNFTRequestSchema, params);

Expand All @@ -106,7 +106,7 @@ export class NFTEndpoint extends BaseEndpoint {
const nftExists = await nftStore.has(context.getImmutableMethodContext(), nftID);

if (!nftExists) {
throw new Error('NFT does not exist');
throw new Error('NFT substore entry does not exist');
}

const userStore = this.stores.get(UserStore);
Expand All @@ -118,6 +118,13 @@ export class NFTEndpoint extends BaseEndpoint {
}));

if (nftData.owner.length === LENGTH_ADDRESS) {
const userExists = await userStore.has(
context.getImmutableMethodContext(),
userStore.getKey(nftData.owner, nftID),
);
if (!userExists) {
throw new Error('User substore entry does not exist');
}
const userData = await userStore.get(
context.getImmutableMethodContext(),
userStore.getKey(nftData.owner, nftID),
Expand Down
Loading