LiskArchive / lisk-sdk

🔩 Lisk software development kit
https://lisk.com
Apache License 2.0
2.72k stars 454 forks source link

Update nft module internal functions #9014

Closed mosmartin closed 11 months ago

mosmartin commented 1 year ago

What was the problem?

This PR resolves #9004

How was it solved?

How was it tested?

Added and updated existing tests

codecov[bot] commented 1 year ago

Codecov Report

Merging #9014 (5fbae3f) into release/6.1.0 (2eabd20) will decrease coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.1.0    #9014      +/-   ##
=================================================
- Coverage          84.34%   84.33%   -0.01%     
=================================================
  Files                652      652              
  Lines              23976    23961      -15     
  Branches            3487     3485       -2     
=================================================
- Hits               20223    20208      -15     
  Misses              3753     3753              
Files Coverage Δ
...amework/src/modules/nft/cc_commands/cc_transfer.ts 97.33% <100.00%> (-0.26%) :arrow_down:
framework/src/modules/nft/internal_method.ts 97.22% <100.00%> (+0.07%) :arrow_up:
framework/src/modules/nft/method.ts 98.37% <100.00%> (-0.04%) :arrow_down:
framework/src/modules/nft/module.ts 85.60% <100.00%> (-0.34%) :arrow_down:
mosmartin commented 1 year ago

I think it'd be nice to add unit test to check if NFT has duplicate module attributes for these 2 scenarios :

  1. When a foreign NFT is received (lisk-sdk/framework/src/modules/nft/cc_commands/cc_transfer.ts#L142-L145)
  2. When a foreign NFT is bounced (lisk-sdk/framework/src/modules/nft/cc_commands/cc_transfer.ts#L149-L152)

@Incede for the first test, isn't it covered by it('should reject and emit unsuccessful ccm transfer event if nft chain id does not equal own chain id and nft is not supported', async () => {...}?

I've also noticed that the CCM_STATUS_CODE_OK constant here is used in the function and CCM_STATUS_OK used in the tests. Is this the way it should be?

Incede commented 1 year ago

I think it'd be nice to add unit test to check if NFT has duplicate module attributes for these 2 scenarios :

  1. When a foreign NFT is received (lisk-sdk/framework/src/modules/nft/cc_commands/cc_transfer.ts#L142-L145)
  2. When a foreign NFT is bounced (lisk-sdk/framework/src/modules/nft/cc_commands/cc_transfer.ts#L149-L152)

@Incede for the first test, isn't it covered by it('should reject and emit unsuccessful ccm transfer event if nft chain id does not equal own chain id and nft is not supported', async () => {...}?

I don't think the test case you mention is related to any of the 2 scenarios :

  1. foreign NFT is received
  2. foreign NFT is bounced

plus my suggestion is to check if NFT has duplicate module attributes for these 2 scenarios which isn't being currently checked, since the original issue was primarily about allowing "duplicate module attributes" and if the suggested test was already there the issue would have been caught before. Although I understand that the function createNFTEntry is already tested for duplicate module attributes I thought it'd be nice to test it again here for the sake of this issue.