Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rename storage methods to follow the commonly accepted NFTs terminology. #13325

Closed
wants to merge 1 commit into from
Closed

Rename storage methods to follow the commonly accepted NFTs terminology. #13325

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions frame/uniques/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ pub mod pallet {
}

#[pallet::storage]
#[pallet::storage_prefix = "Class"]
#[pallet::storage_prefix = "Collection"]
Copy link
Member

Choose a reason for hiding this comment

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

We only set the storage_prefix to have the better name Collection inside the rust implementation without requiring a migration. Your proposed changes here require a migration.

Copy link
Contributor Author

@simonsso simonsso Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I created this PR because the current naming is not consistent. In one interface (extrinsics) it was refereed to as Collections and items and in the rpc interface there were suddenly called Classes and instances. I only discovered this when I saw the mismatch in documentation of #13322

I think the changes made in #11389 should have renamed these interfaces as well.

Pros: It gives a better experience for the users (developers) where it is easy to understand the connection
Cons: It is a change and it will break things.

Anyhow @bkchr I think you have a better view about the impact and implications here so I will trust your judgement if you don't want to do this, just pointing out the inconsistency which made me confused. Please handle it as you see fit.

Your proposed changes here require a migration.

@bkchr, So this is not only the name of the getter function, does this macro also declare the storage name?

Copy link
Member

Choose a reason for hiding this comment

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

does this macro also declare the storage name?

Yes. storage_prefix in this case is the storage name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkchr Is it possible to have meaningful names in the external API which out chaining the internal storage

Copy link
Member

Choose a reason for hiding this comment

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

Currently not.

/// Details of a collection.
pub(super) type Collection<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Expand Down Expand Up @@ -191,7 +191,7 @@ pub mod pallet {
>;

#[pallet::storage]
#[pallet::storage_prefix = "ClassAccount"]
#[pallet::storage_prefix = "CollectionAccount"]
/// The collections owned by any given account; set out this way so that collections owned by
/// a single account can be enumerated.
pub(super) type CollectionAccount<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
Expand All @@ -218,7 +218,7 @@ pub mod pallet {
>;

#[pallet::storage]
#[pallet::storage_prefix = "ClassMetadataOf"]
#[pallet::storage_prefix = "CollectionMetadataOf"]
/// Metadata of a collection.
pub(super) type CollectionMetadataOf<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Expand All @@ -229,7 +229,7 @@ pub mod pallet {
>;

#[pallet::storage]
#[pallet::storage_prefix = "InstanceMetadataOf"]
#[pallet::storage_prefix = "MetadataOf"]
/// Metadata of an item.
pub(super) type ItemMetadataOf<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
_,
Expand Down