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

NFTModule.initGenesisState #8630

Merged
merged 8 commits into from
Jun 23, 2023

Conversation

has5aan
Copy link
Contributor

@has5aan has5aan commented Jun 19, 2023

What was the problem?

This PR resolves #8397

How was it solved?

Implemented NFTModule.initGenesisState.

How was it tested?

Implemented unit tests.

@has5aan has5aan requested review from Incede and mjerkov June 19, 2023 21:16
@has5aan has5aan self-assigned this Jun 19, 2023
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #8630 (d0a501d) into feature/6917-implement-nft-module (67043dd) will increase coverage by 0.03%.
The diff coverage is 98.00%.

❗ Current head d0a501d differs from pull request most recent head a15632e. Consider uploading reports for the commit a15632e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                          @@
##           feature/6917-implement-nft-module    #8630      +/-   ##
=====================================================================
+ Coverage                              83.84%   83.88%   +0.03%     
=====================================================================
  Files                                    623      623              
  Lines                                  23164    23208      +44     
  Branches                                3350     3360      +10     
=====================================================================
+ Hits                                   19423    19467      +44     
  Misses                                  3741     3741              
Impacted Files Coverage Δ
framework/src/modules/nft/module.ts 92.30% <97.95%> (+5.35%) ⬆️
framework/src/modules/nft/schemas.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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.

This is a partial review, I would suggest until my comment in lines 302-316 of module.ts gets some input from the research reviewers of the LIP (also feel free to comment on this).

framework/src/modules/nft/module.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/module.ts Outdated Show resolved Hide resolved
framework/test/unit/modules/nft/module.spec.ts Outdated Show resolved Hide resolved
framework/test/unit/modules/nft/module.spec.ts Outdated Show resolved Hide resolved
framework/test/unit/modules/nft/module.spec.ts Outdated Show resolved Hide resolved
framework/src/modules/nft/module.ts Outdated Show resolved Hide resolved
…Substore and EscrowSubstore and adds checks to throw if UserSubstore and EscrowSubstore has additional entries for an NFT not contained in NFTSubstore
@has5aan has5aan requested a review from mjerkov June 22, 2023 09:32
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.

Good job! Now the code is much cleaner

@Incede Incede merged commit 4968ad4 into feature/6917-implement-nft-module Jun 23, 2023
@Incede Incede deleted the 8397_nft_initGenesisState branch June 23, 2023 13:17
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