Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ensure liquidity mining NFT id is within reserved range #444

Merged
merged 7 commits into from
May 10, 2022

Conversation

Roznovjak
Copy link
Collaborator

Fixes: #427

@Roznovjak Roznovjak self-assigned this May 5, 2022
@github-actions
Copy link

github-actions bot commented May 5, 2022

Runtime version has not been increased.

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #444 (c4fcdfa) into master (72fe3a1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   84.02%   84.04%   +0.01%     
==========================================
  Files          26       26              
  Lines        3093     3096       +3     
==========================================
+ Hits         2599     2602       +3     
  Misses        494      494              
Impacted Files Coverage Δ
pallets/liquidity-mining/src/lib.rs 85.32% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 026e3b5...0070608. Read the comment docs.

@mrq1911 mrq1911 changed the title feat: test ClassId value feat: ensure liquidity mining NFT id is within reserved range May 6, 2022
@mrq1911
Copy link
Member

mrq1911 commented May 6, 2022

@Roznovjak can you add test that check works like expected?

@Roznovjak
Copy link
Collaborator Author

@Roznovjak can you add test that check works like expected?

A test is automatically generated from it. But it's more like a verification, because testing it (both scenarios) would require two runtimes with different pallet configurations.

@mrq1911
Copy link
Member

mrq1911 commented May 6, 2022

@Roznovjak can you add test that check works like expected?

A test is automatically generated from it. But it's more like a verification, because testing it (both scenarios) would require two runtimes with different pallet configurations.

when it actually panics? after runtime upgrade?

@Roznovjak
Copy link
Collaborator Author

@Roznovjak can you add test that check works like expected?

A test is automatically generated from it. But it's more like a verification, because testing it (both scenarios) would require two runtimes with different pallet configurations.

when it actually panics? after runtime upgrade?

It looks like it behaves just like a normal test. Ref

@mrq1911 mrq1911 self-requested a review May 9, 2022 11:21
Copy link
Member

@mrq1911 mrq1911 left a comment

Choose a reason for hiding this comment

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

found the test: test mock::__construct_runtime_integrity_test::runtime_integrity_tests ... ok

LGTM

@mrq1911 mrq1911 merged commit 54ebb19 into master May 10, 2022
@Roznovjak Roznovjak deleted the liq_mining/test_class_id branch May 11, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check if nft class id is from reserved range - liquidity mining
2 participants