Expose NFTs depositor and deposit amount

dudo50 commented 1 week ago

Is there an existing issue?

[X] I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

[X] This is not a support question.

Motivation

I am in the middle of building a pallet that needs to access the amount the depositor deposited and who the depositor is. I can see getter functions for the owner of Collection or NFT, but there is no getter function for Depositor or how much they deposited. This is for both NFT and Collection. Is there any reason why these structs are not exposed via the getter function? Thanks!

Request

Expose depositor and deposit amount for NFTs and deposit for Collections in both Pallet NFTs and Uniques.

Solution

Expose getters into function same way owner of nft or collection is exposed - eg here: https://github.com/paritytech/polkadot-sdk/blob/9128dca3c544a5327f0a89241aea1409d67c81b0/substrate/frame/nfts/src/common_functions.rs#L26

Are you willing to help with this request?

Yes!

bkchr commented 1 week ago

The Item storage is already public. This means you can already access all the data you need without requiring these getters.

dudo50 commented 1 week ago

@bkchr accessing that storage data from another pallet is not possible however because data in item or collection structs is private (I am able to get data from storage but I am unable to access it because it is private)

Or is there any workaround I may be missing? Could you share snippet towards it? Thanks!

dudo50 commented 1 week ago

@bkchr , to demonstrate what I meant, here is an example snippet:

let mut metadata = None;
if pallet_nfts::ItemMetadataOf::<T, I>::contains_key(&current_collection, &current_asset) {
    metadata = pallet_nfts::ItemMetadataOf::<T, I>::get(&current_collection, &current_asset);
    metadata = metadata.unwrap().data;
}

Results into following error:

error[E0616]: field `data` of struct `ItemMetadata` is private
   --> templates/parachain/pallets/xcnft/src/lib.rs:956:34
    |
956 |                 metadata = metadata.unwrap().data;
    |                                              ^^^^ private field

bkchr commented 1 week ago

Ahh! Yeah I only checked the storage items.

I think making these fields public is a better pr!

dudo50 commented 1 week ago

@bkchr , will do, I will ping you once PR is updated

dudo50 commented 1 week ago

@bkchr ,

Looking at the code:

/// Information about the item's metadata.
#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)]
#[scale_info(skip_type_params(StringLimit))]
pub struct ItemMetadata<Deposit, StringLimit: Get<u32>> {
    /// The balance deposited for this metadata.
    ///
    /// This pays for the data stored in this struct.
    pub(super) deposit: Deposit,
    /// General information concerning this item. Limited in length by `StringLimit`. This will
    /// generally be either a JSON dump or the hash of some JSON which can be found on a
    /// hash-addressable global publication system such as IPFS.
    pub(super) data: BoundedVec<u8, StringLimit>,
}

The item details struct is public as well as storage

    /// Metadata of an item.
    #[pallet::storage]
    pub type ItemMetadataOf<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
        _,
        Blake2_128Concat,
        T::CollectionId,
        Blake2_128Concat,
        T::ItemId,
        ItemMetadata<ItemMetadataDepositOf<T, I>, T::StringLimit>,
        OptionQuery,
    >;

This leads me to a question - am I doing something wrong when trying to access these fields?

EDIT: GPT says, that: The issue arises because the data field in the ItemMetadata struct is marked as pub(super), meaning it is private to the crate, or module, where it is defined. You cannot access the field directly from outside that module.

And basically gives me 2 options:

Use a getter method (if one is provided by the crate) to retrieve the data field.
Make a custom implementation that allows access to the data field by adding a public getter function in the ItemMetadata struct: by removing super from the structs.

Any thoughts on this? I think, that getter methods are safer, removing super from the structs seems incorrect to me because it is there for some reason.

bkchr commented 2 days ago

What I meant is that you just change pub(super) to pub

dudo50 commented 2 days ago

@bkchr , I understood, as pointed in the comment, however, isn't it private to pallet_nfts for some reason (perhaps safety or something else)? If it isn't then I will happily open PR that will expose these structs tommorow first thing in the morning.

Thanks in advance!

dudo50 commented 2 days ago

@bkchr ,

I have opened a new PR that exposes structs as you requested: https://github.com/paritytech/polkadot-sdk/pull/6087

I have tested it with build and run tests on it.

I have also updated the code of my pallet to interact with storage directly now, and it all works well.

Let me know if any other changes are required from me.

Thanks!

With kind regards, Dudo

paritytech / polkadot-sdk