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

Add collection info - RPC call #792

Open

KarishmaBothara wants to merge 13 commits into main from feat/nft-header-info-rpc

Contributor

KarishmaBothara commented Feb 22, 2024 •

edited

Loading

Description

As the NFT collection grow in size with new token minted in it.. Storage to get collection info fails to get the huge data. This PR provides RPC method to get collection information from collection id. It has all the information except for collection owner

PR checklist

Describe the added/changed/fixed/removed behaviour
Tag related issue(s)
Add appropriate unit tests and/or integration tests to the integration test repository

KarishmaBothara marked this pull request as draft

February 22, 2024 10:17

KarishmaBothara marked this pull request as ready for review

February 23, 2024 10:14

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

Contributor

JasonTulp left a comment

Looking good, some fields I don't think are needed in this RPC call. I also think we should have a think about whether it would make sense to have one RPC for all information, or multiple RPC calls for each individual part. What do you think?

Contributor Author

KarishmaBothara commented Mar 5, 2024

Looking good, some fields I don't think are needed in this RPC call. I also think we should have a think about whether it would make sense to have one RPC for all information, or multiple RPC calls for each individual part. What do you think?

Thanks for your review.. I think the reason for this rpc call is, storage collection_info would fail, ownedTokens list if it is too huge, then the storage retrieval fails.. so an alternative to that without ownedTokens info

KarishmaBothara requested review from JasonTulp and ken-futureverse

March 7, 2024 10:01

KarishmaBothara changed the title ~~Add collection info - RPC call (WIP)~~ Add collection info - RPC call

JasonTulp reviewed

View reviewed changes

pallet/nft/src/types.rs Outdated Show resolved Hide resolved

JasonTulp reviewed

View reviewed changes

e2e/test/NftRPC.test.ts Outdated Show resolved Hide resolved

KarishmaBothara added 5 commits

October 14, 2024 11:06


          Add collection info - RPC call (WIP)

cf75a70

wip

e5d3e8b


          Working rpc nft call + tests

8a83e20


          Update comment and rename RPC method collectionInfo to collectionDetails

138eb5a


          T::AccountId::from(owner) is not required as owner is already T::Acco…

3cfa6cd

…untId

KarishmaBothara force-pushed the feat/nft-header-info-rpc branch from b1a0eef to 3cfa6cd Compare

October 14, 2024 08:15

KarishmaBothara added 5 commits

October 14, 2024 12:19


          Format

672c8a2


          Fix build

344fbd2


          Remove commented code

bcfa475


          Working nft collection detail rpc

aba6d14


          Merge branch 'main' into feat/nft-header-info-rpc

e99e960

KarishmaBothara requested a review from JasonTulp

October 16, 2024 08:56


          Merge branch 'main' into feat/nft-header-info-rpc

49d829d

JasonTulp reviewed

View reviewed changes

Contributor

JasonTulp left a comment

Looking good! The separate struct looks like the better option for sure. Few minor comments left to address

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

pallet/nft/src/types.rs Show resolved Hide resolved

runtime/src/lib.rs Outdated Show resolved Hide resolved

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

pallet/nft/rpc/runtime-api/Cargo.toml Outdated Show resolved Hide resolved


          Address review comments

623fa1b

KarishmaBothara requested review from JasonTulp and removed request for ken-futureverse

October 22, 2024 06:37

JasonTulp reviewed

View reviewed changes

pallet/nft/rpc/runtime-api/Cargo.toml

@@ @@ -19,6 +19,7 @@ default = ["std"] @@
               std = [
               	"codec/std",
               	"sp-api/std",
+              	"codec/std",

Contributor

JasonTulp Oct 29, 2024

Can sp-ruintime be removed from the cargo.toml?

Contributor Author

KarishmaBothara Oct 29, 2024

I end up with errors on removing sp-runtime

JasonTulp reviewed

View reviewed changes

pallet/nft/src/impls.rs Outdated Show resolved Hide resolved

JasonTulp approved these changes

View reviewed changes

Contributor

JasonTulp left a comment

lgtm, 2 more small comments. Needs a review from someone else before merging. Nice work Karishma


          Address review comment

e4f607c

KarishmaBothara requested a review from surangap

October 29, 2024 05:50

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

Labels

None yet