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.

Sign up for GitHub

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

Jump to bottom

feat(api/nonfungibles): pallet + devnet runtime #339

Closed

chungquantin wants to merge 127 commits into chungquantin/feat-nfts from chungquantin/feat-nonfungibles

Collaborator

chungquantin commented Oct 14, 2024 •

edited

Loading

Description

Implement the runtime pallet pallet-api/nonfungibles for the #259. This implementation follows the discussed NFT spec with the forked pallet-nfts.

Linked PRs

Weight & benchmarking PR: feat(api/nonfungibles): destroy collection witness data & weights #383

Pallet: `pallet-api/nonfungibles`

Implement basic functionalities of the pallet-api/nonfungibles.
Configure pallet-nfts and pallet-api/nonfungibles to Test runtime in mock.rs
- refactor: pallet tests of pallet-api/fungibles to use an updated Account instead of u8. (.e.g, account(ALICE))
pallet unit testing with pallet-nonfungibles.
Add non-fungibles feature to devnet. Merge this PR first: refactor: generic extension #218

chungquantin added the pallet label

chungquantin mentioned this pull request

feat(devnet): non-fungibles api implementation #260

Closed

12 tasks

chungquantin changed the title ~~feat: add nonfungibles pallet implementation~~ feat(devnet): add nonfungibles pallet implementation

chungquantin mentioned this pull request

feat(pop-api): nonfungibles use case #335

Open

20 tasks

codecov-commenter commented Oct 14, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 77.51310% with 472 lines in your changes missing coverage. Please review.

Project coverage is 71.63%. Comparing base (aff6408) to head (73f4994).

Files with missing lines	Patch %	Lines
pallets/nfts/src/weights.rs	7.18%	310 Missing ⚠️
pallets/api/src/nonfungibles/weights.rs	50.00%	64 Missing ⚠️
pallets/api/src/nonfungibles/mod.rs	72.72%	32 Missing and 13 partials ⚠️
pallets/api/src/mock.rs	8.69%	21 Missing ⚠️
pallets/nfts/src/features/approvals.rs	87.35%	3 Missing and 8 partials ⚠️
pallets/api/src/nonfungibles/benchmarking.rs	0.00%	5 Missing ⚠️
pallets/nfts/src/benchmarking.rs	80.95%	2 Missing and 2 partials ⚠️
runtime/devnet/src/config/api/mod.rs	96.15%	4 Missing ⚠️
pallets/api/src/nonfungibles/tests.rs	99.76%	1 Missing and 1 partial ⚠️
...lets/nfts/src/features/create_delete_collection.rs	80.00%	1 Missing and 1 partial ⚠️
... and 2 more

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   68.38%   71.63%   +3.24%     
==========================================
  Files          70       74       +4     
  Lines       11846    13495    +1649     
  Branches    11846    13495    +1649     
==========================================
+ Hits         8101     9667    +1566     
- Misses       3486     3545      +59     
- Partials      259      283      +24

Files with missing lines	Coverage Δ
pallets/nfts/src/common_functions.rs	`84.61% <100.00%> (+4.61%)`	⬆️
pallets/nfts/src/features/create_delete_item.rs	`90.05% <100.00%> (+0.84%)`	⬆️
pallets/nfts/src/tests.rs	`99.89% <100.00%> (+<0.01%)`	⬆️
pallets/nfts/src/types.rs	`67.54% <100.00%> (+0.43%)`	⬆️
runtime/devnet/src/config/assets.rs	`100.00% <100.00%> (ø)`
runtime/devnet/src/config/proxy.rs	`0.00% <ø> (ø)`
runtime/devnet/src/lib.rs	`5.53% <ø> (ø)`
pallets/api/src/nonfungibles/tests.rs	`99.76% <99.76%> (ø)`
...lets/nfts/src/features/create_delete_collection.rs	`84.94% <80.00%> (+0.60%)`	⬆️
pallets/nfts/src/features/transfer.rs	`85.61% <89.47%> (+1.77%)`	⬆️
... and 9 more

chungquantin added the blocked label

chungquantin changed the base branch from main to chungquantin/chore-pallet_nfts

October 14, 2024 15:25

chungquantin force-pushed the chungquantin/feat-nonfungibles branch from 74ccaa7 to 8832470 Compare

October 15, 2024 09:54

chungquantin changed the title ~~feat(devnet): add nonfungibles pallet implementation~~ feat(devnet): add nonfungibles runtime implementation

chungquantin changed the title ~~feat(devnet): add nonfungibles runtime implementation~~ feat(nfts): add nonfungibles runtime implementation

chungquantin added the runtime label

chungquantin self-assigned this

chungquantin changed the title ~~feat(nfts): add nonfungibles runtime implementation~~ feat(api/nonfungibles): pallet + devnet runtime

chungquantin mentioned this pull request

chore(api/nonfungibles): fork pallet nfts #342

Closed

chungquantin force-pushed the chungquantin/feat-nonfungibles branch from d4c29fe to 507e7f8 Compare

November 12, 2024 14:23

chungquantin mentioned this pull request

chore(api/nonfungibles): fork pallet nfts #381

Closed

chungquantin changed the base branch from chungquantin/chore-pallet_nfts to chungquantin/fork-pallet_nfts

November 13, 2024 02:44

This was linked to issues Nov 13, 2024

feat(pop-api): nonfungibles use case #335

Open

feat(devnet): pop-api methods for non-fungible use case #336

Open

chungquantin mentioned this pull request

chore(api/nonfungibles): fork pallet nfts #382

Merged

chungquantin changed the base branch from chungquantin/fork-pallet_nfts to chungquantin/chore-fork_pallet_nfts

November 13, 2024 10:40

chungquantin force-pushed the chungquantin/feat-nonfungibles branch from 032bb0c to 242e53b Compare

November 13, 2024 10:45

chungquantin added ready-for-final-review and removed blocked labels

chungquantin force-pushed the chungquantin/chore-fork_pallet_nfts branch from c69dd85 to b9fe4d8 Compare

November 18, 2024 04:01

chungquantin commented

View reviewed changes

pallets/api/src/nonfungibles/mod.rs

+              		},
+              		/// Details of a specified collection.
+              		#[codec(index = 9)]
+              		Collection(CollectionIdOf<T>),

Collaborator Author

chungquantin Nov 18, 2024

[NOT MENTIONED IN THE SPEC]

I added this Collection read query because I think it is necessary for the smart contract developer to get the details of the NFT collection.

chungquantin commented

View reviewed changes

pallets/api/src/nonfungibles/mod.rs

+              		Collection(CollectionIdOf<T>),
+              		/// Next collection ID.
+              		#[codec(index = 10)]
+              		NextCollectionId,

Collaborator Author

chungquantin Nov 18, 2024

[NOT MENTIONED IN THE SPEC]

As discussed for the implementation of create pop-api method, we need this read to query and return from the pop-api contract method.

chungquantin commented

View reviewed changes

pallets/api/src/nonfungibles/mod.rs

+              		/// Details of a specified collection.
+              		Collection(Option<CollectionDetailsFor<T>>),
+              		/// Next collection ID.
+              		NextCollectionId(Option<CollectionIdOf<T>>),

Collaborator Author

chungquantin Nov 18, 2024

Suggested change

      
            		NextCollectionId(Option<CollectionIdOf<T>>),
          
            		NextCollectionId(CollectionIdOf<T>),

If we suppose that T::CollectionId::initial_value() == 0, we could probably return a value instead of Option from this read.

chungquantin commented

View reviewed changes

pallets/api/src/nonfungibles/mod.rs

+              		/// - `approved` - The approval status of the collection item.
+              		#[pallet::call_index(4)]
+              		#[pallet::weight(
+                          NftsWeightInfoOf::<T>::approve_transfer(item.is_some() as u32) +

Collaborator Author

chungquantin Nov 18, 2024

[NOT MENTIONED IN THE SPEC]

The weight for the fn approve() method depends on branching logic determined by the states of approved and item. Refer to the updates in the benchmarking method for fn approve_transfer() and fn cancel_approval() to see these adjustments.

chungquantin commented

View reviewed changes

pallets/api/src/nonfungibles/mod.rs

+              		) -> DispatchResult {
+              			let creator = ensure_signed(origin.clone())?;
+              			// TODO: re-evaluate next collection id in nfts pallet. The `Incrementable` trait causes
+              			//  issues for setting it to xcm's `Location`. This can easily be done differently.

Collaborator Author

chungquantin Nov 18, 2024

TODO: re-evaluate next collection id in nfts pallet. The Incrementable trait cause issues for setting it to xcm's Location. This can easily be done differently.

Should this TODO be tackled in this PR?

chungquantin added 27 commits

November 19, 2024 13:08


          chore: fork pallet nfts

d7bc57e


          chore: fork pallet nfts

862f23b


          chore: forked pallet nfts

8c8d003


          fix: formatting

24ddc7d


          feat: add nonfungibles implementation

8c25fcf


          chore: add nfts pallet

b131c60


          feat: add new storage items to pallet-nfts

1a54094


          feat: check allowance

22651ba


          test(nfts): check account balance & test total supply

97917ae


          test(nfts): allowance works

2282b76


          refactor(test): check_allowance_works

d803fbb


          fix(nfts): cancel / approve collection transfer

1221f18


          test(nfts): cancel approval collection

8fcf123


          test(nfts): update approval test cases

1034a65


          feat(nfts): get attribute read method

8cdf05d


          feat(nonfungibles + nfts): add new methods to manage attributes

b467b40


          refactor(nfts): toml lock file

54b8228


          test(api/nonfungibles): encoding_read_result

52aea7f


          test(nonfungibles): add tests and remove collection owner read

22456c7


          chore: update pallet & runtime

e2cf885


          feat: multi instances & function comments

c3be7e1


          chore: resolve feedback

f8acb26


          feat(api/nonfungibles): destroy collection witness data & weights (#383)

874fb92

chore: rename nfts instance

feat(api/nonfungibles): destroy collection witness data & weights (#383)

chore: rename nfts instance

fix(api/nonfungibles): pallet weight testing


          chore: clippy

fba13cf


          chore: taplo fmt

946ad65


          chore: fork pallet nfts

7bfbd43


          Merge remote-tracking branch 'refs/remotes/origin/chungquantin/feat-n…

dcb4e33

…onfungibles' into chungquantin/feat-nonfungibles

This was unlinked from issues Nov 19, 2024

feat(devnet): pop-api methods for non-fungible use case #336

Open

feat(pop-api): nonfungibles use case #335

Open

chungquantin closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pallet ready-for-final-review runtime