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

Refactor NFT transfer verification #9005

Merged
merged 12 commits into from
Oct 7, 2023
Merged

Conversation

bobanm
Copy link
Contributor

@bobanm bobanm commented Sep 19, 2023

What was the problem?

This PR resolves #8954, resolves #8986, partially resolves #8961 and resolves #9066

How was it solved?

  • Extracted common verification functionality to internal method, both for transfer and transfer cross chain scenarios
  • Both commands and methods use the internal verification method, instead of quadruplicating the verification logic
  • Fixed invalid tests

How was it tested?

All the existing tests are passing.

@bobanm bobanm requested review from shuse2 and Incede September 19, 2023 01:03
@bobanm bobanm self-assigned this Sep 19, 2023
@bobanm bobanm force-pushed the 8954-refactor-nft-transfers branch from 2671af6 to 6dc1c4b Compare September 20, 2023 15:09
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #9005 (36c2039) into release/6.1.0 (d9e8a89) will decrease coverage by 0.02%.
The diff coverage is 91.45%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.1.0    #9005      +/-   ##
=================================================
- Coverage          84.36%   84.34%   -0.02%     
=================================================
  Files                651      652       +1     
  Lines              23977    23972       -5     
  Branches            3488     3486       -2     
=================================================
- Hits               20227    20220       -7     
- Misses              3750     3752       +2     
Files Coverage Δ
framework/src/modules/nft/commands/transfer.ts 100.00% <100.00%> (ø)
...k/src/modules/nft/commands/transfer_cross_chain.ts 100.00% <100.00%> (ø)
framework/src/modules/nft/error.ts 100.00% <100.00%> (ø)
framework/src/modules/nft/events/unlock.ts 100.00% <100.00%> (+28.57%) ⬆️
framework/src/modules/nft/internal_method.ts 97.14% <94.11%> (-2.86%) ⬇️
framework/src/modules/nft/module.ts 85.93% <0.00%> (ø)
framework/src/modules/nft/method.ts 98.71% <91.66%> (-0.40%) ⬇️

framework/src/modules/nft/method.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/types.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/internal_method.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/internal_method.ts Show resolved Hide resolved
@bobanm bobanm requested a review from mjerkov September 22, 2023 08:42
@bobanm bobanm requested a review from shuse2 September 22, 2023 10:30
Copy link
Contributor

@mjerkov mjerkov left a comment

Choose a reason for hiding this comment

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

Nice refactorization, this part will have to be touched upon again when getNFT is introduced, and getChainID, getNFTOwner, and getLockingModule are removed.

framework/src/modules/nft/internal_method.ts Show resolved Hide resolved
framework/src/modules/nft/internal_method.ts Show resolved Hide resolved
framework/src/modules/nft/method.ts Show resolved Hide resolved
@Incede Incede requested a review from shuse2 October 6, 2023 08:36
@Incede Incede self-assigned this Oct 6, 2023
@Incede Incede removed their request for review October 6, 2023 08:41
framework/src/modules/nft/internal_method.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/internal_method.ts Outdated Show resolved Hide resolved
@shuse2 shuse2 merged commit 5b2792f into release/6.1.0 Oct 7, 2023
@shuse2 shuse2 deleted the 8954-refactor-nft-transfers branch October 7, 2023 09:05
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