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

Update NFT module with additional checks and LIP updates #8635

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

Incede
Copy link
Contributor

@Incede Incede commented Jun 22, 2023

What was the problem?

This PR partially resolves #8580

How was it solved?

Update NFT module as specified

How was it tested?

Added unit tests

@Incede Incede requested review from shuse2 and mjerkov June 22, 2023 02:33
@Incede Incede self-assigned this Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #8635 (52d1ad2) into feature/6917-implement-nft-module (70042f2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@                        Coverage Diff                         @@
##           feature/6917-implement-nft-module    #8635   +/-   ##
==================================================================
  Coverage                              83.84%   83.84%           
==================================================================
  Files                                    623      623           
  Lines                                  23159    23164    +5     
  Branches                                3348     3350    +2     
==================================================================
+ Hits                                   19417    19423    +6     
+ Misses                                  3742     3741    -1     
Impacted Files Coverage Δ
...amework/src/modules/nft/cc_commands/cc_transfer.ts 97.43% <100.00%> (ø)
...k/src/modules/nft/commands/transfer_cross_chain.ts 97.67% <100.00%> (+0.11%) ⬆️
framework/src/modules/nft/method.ts 99.16% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Token module change should be a different PR targeting release/6.0.0 branch 🙏
Assuming above, token module part is not reviewed

framework/src/modules/nft/commands/transfer_cross_chain.ts Outdated Show resolved Hide resolved
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.

Looks good, as per comment I reviewed only the NFT part.

@Incede Incede changed the title Update NFT and token module with additional checks and LIP updates Update NFT module with additional checks and LIP updates Jun 22, 2023
@Incede Incede requested a review from shuse2 June 22, 2023 14:59
@mjerkov
Copy link
Contributor

mjerkov commented Jun 22, 2023

I see that some tests are failing, do you have idea why that may be?

@Incede
Copy link
Contributor Author

Incede commented Jun 22, 2023

I see that some tests are failing, do you have idea why that may be?

yes, the tests were failing due to the incorrect setup of context - the chain id in a command context is supposed to be the same as the own chain id but for some test cases it was not. I fixed it now. It was not detected previously as it was using the internal method getOwnChainID to get ownChainID from config rather from the context before.

@shuse2 shuse2 merged commit 67043dd into feature/6917-implement-nft-module Jun 23, 2023
@shuse2 shuse2 deleted the 8580-lip-updates branch June 23, 2023 01:43
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