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

Uniques: Allowing multiple approvals #12164

Closed
wants to merge 12 commits into from

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Sep 1, 2022

Solves the new approvals structure task from #11783

In short, this PR allows the item owner to make approvals to multiple accounts.

Note: I took a bit of a different approach than proposed in #11791. I believe that this is a bit simpler than storing a BoundedBTreeMap for approvals.

@jsidorenko
Copy link
Contributor

Hi @Szegoo , thanks for your contribution!
I'm thinking it's better to keep the existing Uniques pallet as feature-complete and start pushing all the new changes into V2.
I've created a new branch for this js/uniques-v2-main-branch, this is a new pallet for all V2 works.
It would be good if you could apply your changes on top of that pallet and point your PR to my branch.

@jsidorenko
Copy link
Contributor

The reason I wanted to use BoundedBTreeMap is because of the new structure of approvals I'm proposing: I think each approval should have an optional deadline (the block number) set.
Pls check my prev implementation for reference:
707d69b
a705c22

@Szegoo
Copy link
Contributor Author

Szegoo commented Sep 2, 2022

@jsidorenko What would be the benefit of creating a BoundedBTreeMap, cause we could also probably add deadline like this:

/// The approved transferrer of this item, if one is set.
pub(super) approved:
	BoundedVec<(AccountId, Option<<T as frame_system::Config>::BlockNumber>), ApprovalsLimit>,

I am going to use BoundedBTreeMap if you still think we should use that, I am just interested if there is a reason behind this decision.

@jsidorenko
Copy link
Contributor

BoundedBTreeMap simplifies the management of approvals:

  1. when you need to find an approval, you will simply call item.approvals.get(&sender) instead of item.approvals.get_key_value(&sender)
  2. once you insert, you don't need to check if approval for the same account exists
  3. when you remove, you will simply call item.approvals.remove(&delegate) instead of if let Some(pos) = item.approvals.iter().position(|ap| delegate == ap.who) { item.approvals.remove(pos); }

/// New metadata has been set for a `collection`.
CollectionMetadataSet {
collection: T::CollectionId,
data: BoundedVec<u8, T::StringLimit>,
is_frozen: bool,
},
/// Metadata has been cleared for a `collection`.
CollectionMetadataCleared { collection: T::CollectionId },
CollectionMetadataCleared {
collection: T::CollectionId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to run rustup update and then: cargo +nightly fmt
The latest fmt shouldn't change the syntax that way

@Szegoo
Copy link
Contributor Author

Szegoo commented Sep 3, 2022

@jsidorenko I opened a new PR with these changes to the js/uniques-v2-main-branch, Should I close this?

@Szegoo Szegoo closed this Sep 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants