Implement getNFT function of NFT module #9034
Conversation
Incede
requested review from
mjerkov and
has5aan
and removed request for
has5aan
Codecov Report
@@ Coverage Diff @@
## release/6.1.0 #9034 +/- ##
=================================================
- Coverage 84.36% 84.34% -0.03%
=================================================
Files 651 651
Lines 24016 23985 -31
Branches 3493 3490 -3
=================================================
- Hits 20261 20230 -31
Misses 3755 3755
|
framework/src/modules/nft/types.ts
Outdated
| owner: Buffer; | ||
| attributesArray: NFTAttributes[]; | ||
| lockingModule?: string; | ||
| } | ||
|
|
||
| export interface NFTOutputEndpoint { |
There was a problem hiding this comment.
Instead of creating a duplicated interface with same properties but different types, I think it would be better to follow the pattern which we've been using throughout the codebase:
export type JSONObject<T> = Replaced<T, bigint | Buffer, string>;
export type NFTJSON = JSONObject<NFT>;There was a problem hiding this comment.
valid point 👍 I updated it
| let nft; | ||
| try { | ||
| nft = await this._method.getNFT(methodContext, params.nftID); | ||
| } catch (error) { | ||
| throw new Error('NFT does not exist'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
will update in the internal functions in PR #9005 to catch and throw if error is other than expected 👍
| @@ -347,12 +310,10 @@ export class NFTMethod extends BaseMethod { | |||
| NftEventResult.RESULT_NFT_DOES_NOT_EXIST, | |||
| ); | |||
|
|
|||
| throw new Error('NFT substore entry does not exist'); | |||
| throw new Error('NFT does not exist'); | |||
There was a problem hiding this comment.
If the goal of this try/catch was simply to log the event, then how about simply re-throwing the same error object, instead of creating a new one?
Same for the other instances of this pattern in the code.
There was a problem hiding this comment.
The errors are following the LIP and make sense for me - the errors inside getNFT are more granular depedning upon which substore entry is missing. Inside higher level functions such as lock it makes sense to encapsualte both into a single generic error instead of 2 different errors.
| let nft; | ||
| try { | ||
| nft = await this._method.getNFT(methodContext, params.nftID); | ||
| } catch (error) { | ||
| throw new Error('NFT does not exist'); |
There was a problem hiding this comment.
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
What was the problem?
This PR resolves #8958 #8959 #8960 and partially resolves #8961
How was it solved?
Implement and update usage as specified
How was it tested?
Added and updated unit tests