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

Implement cross chain command for cross chain transfer for NFT module #8535

Merged
merged 12 commits into from
Jun 7, 2023

Conversation

Incede
Copy link
Contributor

@Incede Incede commented Jun 4, 2023

What was the problem?

This PR resolves #8395 #8494 #8536

How was it solved?

Implement as specified

How was it tested?

Added unit tests

@Incede Incede requested review from mjerkov and has5aan June 4, 2023 18:46
@Incede Incede self-assigned this Jun 4, 2023
@Incede Incede changed the base branch from feature/6917-implement-nft-module to 8493_nft_transferCrossChainInternal June 4, 2023 18:47
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.

Overall, it looks good, I left only two comments for tests un cc_transfer.spec.ts. I would also add the getNewAttributes as defined in LIP for the reasons described in the Rationale:

When an NFT is sent to another chain, the attributes properties of the NFT can be modified according to specifications set on the receiving chain. When the NFT is received back on its native chain, the returned modified attributes are disregarded and the original attributes are restored, as currently defined by getNewAttributes function. If needed, custom modules can implement a more fine-grained approach towards the attributes that are modified cross-chain.

framework/src/modules/nft/schemas.ts Outdated Show resolved Hide resolved
@Incede Incede requested a review from mjerkov June 6, 2023 03:44
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #8535 (9036286) into feature/6917-implement-nft-module (0ae50b1) will increase coverage by 0.09%.
The diff coverage is 96.55%.

Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                          @@
##           feature/6917-implement-nft-module    #8535      +/-   ##
=====================================================================
+ Coverage                              83.31%   83.41%   +0.09%     
=====================================================================
  Files                                    621      622       +1     
  Lines                                  22604    22719     +115     
  Branches                                3268     3287      +19     
=====================================================================
+ Hits                                   18833    18951     +118     
+ Misses                                  3771     3768       -3     
Impacted Files Coverage Δ
framework/src/modules/nft/schemas.ts 100.00% <ø> (ø)
...ework/src/modules/token/cc_commands/cc_transfer.ts 95.31% <ø> (ø)
framework/src/modules/nft/internal_method.ts 96.61% <50.00%> (-3.39%) ⬇️
...amework/src/modules/nft/cc_commands/cc_transfer.ts 97.36% <97.36%> (ø)
framework/src/modules/nft/constants.ts 100.00% <100.00%> (ø)
framework/src/modules/nft/events/ccm_transfer.ts 100.00% <100.00%> (+28.57%) ⬆️
framework/src/modules/nft/method.ts 100.00% <100.00%> (+8.00%) ⬆️

... and 1 file with indirect coverage changes

@Incede Incede requested a review from has5aan June 6, 2023 21:50
Base automatically changed from 8493_nft_transferCrossChainInternal to feature/6917-implement-nft-module June 7, 2023 10:13
@Incede Incede merged commit 6692093 into feature/6917-implement-nft-module Jun 7, 2023
@Incede Incede deleted the 8395-cc-command branch June 7, 2023 11:06
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.

3 participants