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

Update nft module internal functions #9014

Merged
merged 10 commits into from
Oct 9, 2023

Conversation

mosmartin
Copy link
Contributor

What was the problem?

This PR resolves #9004

How was it solved?

  • refactored the createNFTEntry, createEscrowEntry and createUserEntry methods of the InternalMethod class
  • added the hasDuplicateModuleNames method to the InternalMethod class
  • refactored the execute method of the CrossChainTransferCommand class
  • refactored the create method of the NFTMethod class
  • refactored the initGenesisState of the NFTModule class

How was it tested?

Added and updated existing tests

@mosmartin mosmartin requested review from shuse2 and Incede September 20, 2023 07:53
@mosmartin mosmartin self-assigned this Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

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%) ⬇️
framework/src/modules/nft/internal_method.ts 97.22% <100.00%> (+0.07%) ⬆️
framework/src/modules/nft/method.ts 98.37% <100.00%> (-0.04%) ⬇️
framework/src/modules/nft/module.ts 85.60% <100.00%> (-0.34%) ⬇️

@mosmartin mosmartin requested a review from has5aan September 20, 2023 11:47
@shuse2 shuse2 removed their request for review September 20, 2023 11:48
@mosmartin mosmartin requested a review from has5aan September 22, 2023 11:32
@mosmartin mosmartin requested a review from has5aan September 25, 2023 15:17
@mosmartin mosmartin force-pushed the 9004-upd-nft-mod-internal-functions branch from b65a788 to e703140 Compare September 25, 2023 16:39
@mosmartin mosmartin requested a review from has5aan September 26, 2023 07:35
Copy link
Contributor

@Incede Incede left a comment

Choose a reason for hiding this comment

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

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)

framework/src/modules/nft/cc_commands/cc_transfer.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/cc_commands/cc_transfer.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/method.ts Outdated Show resolved Hide resolved
@mosmartin
Copy link
Contributor Author

mosmartin commented Sep 29, 2023

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
Copy link
Contributor

Incede commented Sep 29, 2023

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.

@mosmartin mosmartin requested a review from Incede October 5, 2023 14:15
@shuse2 shuse2 merged commit 24bf961 into release/6.1.0 Oct 9, 2023
10 checks passed
@shuse2 shuse2 deleted the 9004-upd-nft-mod-internal-functions branch October 9, 2023 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants