Skip to content

Remove pallet::getter usage from pallet-nft-fractionalization #7124

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

Merged
bkchr merged 6 commits into from
Mar 3, 2025
Merged

Remove pallet::getter usage from pallet-nft-fractionalization #7124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9d7526
removed pallet::getter usage from pallet-nft-fractionalization
muraca Jan 13, 2025
19cd13d
prdoc
muraca Jan 13, 2025
ba67b39
Update prdoc/pr_7124.prdoc
ggwpez Jan 14, 2025
2dd3759
Merge branch 'master' into getters/nft-fractionalization
muraca Feb 14, 2025
cb3fafd
Merge branch 'master' into getters/nft-fractionalization
muraca Mar 2, 2025
2651fdb
explicit getter function
muraca Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions prdoc/pr_7124.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
title: Remove pallet::getter from pallet-nft-fractionalization
doc:
- audience: Runtime Dev
description: |
This PR removes all pallet::getter occurrences from pallet-nft-fractionalization and replaces them with explicit implementations.

crates:
- name: pallet-nft-fractionalization
bump: major
8 changes: 7 additions & 1 deletion substrate/frame/nft-fractionalization/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ pub mod pallet {

/// Keeps track of the corresponding NFT ID, asset ID and amount minted.
#[pallet::storage]
#[pallet::getter(fn nft_to_asset)]
Copy link
Member

@bkchr bkchr Jan 14, 2025

Choose a reason for hiding this comment

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

Where is the explicit implementation of this?

Copy link
Contributor Author

@muraca muraca Jan 15, 2025

Choose a reason for hiding this comment

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

It is not required since the storage item is public

Copy link
Contributor

@gui1117 gui1117 Jan 25, 2025

Choose a reason for hiding this comment

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

can we make a deprecate function with a deprecate message that link to the storage get call?

Copy link
Contributor Author

@muraca muraca Feb 14, 2025

Choose a reason for hiding this comment

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

@gui1117 deprecation messages are outside the scope of this individual PR which, like many others defined in #3326, is only responsible for removing getter usage from pallets.

I suggested adding a deprecation message in the original issue #223 (comment) however @ggwpez pointed out that CI would be unhappy and the deprecation message will be added later on, after removing its usage from all pallets in polkadot-sdk.

Copy link
Member

@bkchr bkchr Feb 15, 2025

Choose a reason for hiding this comment

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

@muraca but the other prs also added the getter method as normal function.

Copy link
Contributor

@gui1117 gui1117 Feb 15, 2025

Choose a reason for hiding this comment

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

Yes, I didn't meant to deprecate all getters I meant to keep the function by writing it manually and set it as deprecated.

Copy link
Contributor Author

@muraca muraca Mar 2, 2025

Choose a reason for hiding this comment

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

@bkchr @gui1117 I think you already know that an explicit getter is not needed, since the storage item is public and one can simply write NftToAsset::<T>::get(...). I have implemented an explicit getter anyway, to avoid breaking changes, since looks like this is the direction you want to bring these change towards, however I'm pretty sure that this was not the case when I first started working on these changes one year ago.

Copy link
Member

@bkchr bkchr Mar 2, 2025

Choose a reason for hiding this comment

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

Yeah, but we discussed to keep breaking changes to a minimal, if possible.

pub type NftToAsset<T: Config> = StorageMap<
_,
Blake2_128Concat,
Expand Down Expand Up @@ -338,6 +337,13 @@ pub mod pallet {
T::PalletId::get().into_account_truncating()
}

/// Keeps track of the corresponding NFT ID, asset ID and amount minted.
pub fn nft_to_asset(
key: (T::NftCollectionId, T::NftId),
) -> Option<Details<AssetIdOf<T>, AssetBalanceOf<T>, DepositOf<T>, T::AccountId>> {
NftToAsset::<T>::get(key)
}

/// Prevent further transferring of NFT.
fn do_lock_nft(nft_collection_id: T::NftCollectionId, nft_id: T::NftId) -> DispatchResult {
T::Nfts::disable_transfer(&nft_collection_id, &nft_id)
Expand Down
Loading