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

Conversation

simonsso
Copy link
Contributor

@simonsso simonsso commented Feb 7, 2023

This PR renames classes and instances to collections and items in the Uniques pallet in order to follow the commonly accepted NFTs terminology.

PR #11389 renamed the extrinsics but storage helpers kept old names

This PR renames classes and instances to collections and items in
the Uniques pallet in order to follow the commonly accepted NFTs
terminology.

PR paritytech#11389 renamed the extrinsics but storage helpers kept old names
@simonsso
Copy link
Contributor Author

simonsso commented Feb 7, 2023

This PR will change the API here:

image

Should I update some other documentation files somewhere?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Not sure we should do this.

@@ -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

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.

@stale
Copy link

stale bot commented Mar 16, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Mar 16, 2023
@stale stale bot closed this Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants