Skip to content
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.

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

[Assets] Implement fungibles::freeze::Mutate #3342

Closed
2 tasks done
pandres95 opened this issue Feb 15, 2024 · 7 comments · Fixed by #3951
Closed
2 tasks done

[Assets] Implement fungibles::freeze::Mutate #3342

pandres95 opened this issue Feb 15, 2024 · 7 comments · Fixed by #3951
Labels
I5-enhancement An additional feature request.

Comments

@pandres95
Copy link
Contributor

pandres95 commented Feb 15, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

Locks are currently implemented on Balances due to the needs for existing use cases (e.g. pallet-staking, pallet-conviction-voting, etc.).

However, over time, similar or novel use cases have been arising where locks are needed to be implemented on Assets

One example of that is Multi-governance on pallet-communities, where communities have an OpenGov-like system enabled, for which AssetBalance is a voting option. In this case, the balance of the asset used to vote from a user needs to be locked to prevent an account from voting using such balance, and then sending it over to another member, thus altering the poll results.

Request

Implement fungibles::MutateFreeze for pallet-assets.

Solution

Due to the current design of pallet-assets, there are two options (so, I open this issue to discuss the best course of action):

  1. Create a pallet extension (conveniently called pallet-assets-freezer) that implements both FrozenBalance (the trait described in T::Freezer within pallet-assets) and fungibles::freeze::Mutate.
  2. Replace T::Freezer with a proper implementation of fungibles::freeze::Mutate within pallet-assets.

Are you willing to help with this request?

Yes! (Of, course, assign it to me).

Tasks

Preview Give feedback
@pandres95 pandres95 added the I5-enhancement An additional feature request. label Feb 15, 2024
@pandres95 pandres95 changed the title [Assets] Implement MutateFreeze [Assets] Implement fungibles::freeze::Mutate Feb 15, 2024
@pandres95
Copy link
Contributor Author

pandres95 commented Feb 20, 2024

According to @gavofyork's response on OpenDev Call, the best course of action is not to implement MutateFreeze directly in pallet-assets in order to maintain the minimal necessary logic of it.

In consequence, it's best proceeding with the first option:

Since there's a potential for a pallet extension, are there any public calls that might work on it?

@JelliedOwl
Copy link

JelliedOwl commented Feb 22, 2024

Thank you for raising this - I suspect it's something I would want to use. Sadly, I suspect my other issue with assets needs a separate solution. #3451

Edit to clarify - I think I need to be able to lock part for an asset balance as well as freezing all of an asset on an account (locked or otherwise).

@JelliedOwl
Copy link

JelliedOwl commented Feb 24, 2024

Is what you're proposing more akin to locking (part of a balance) rather than freezing (all of an asset)? In which case, should it be called pallet-assets-locker instead?

Or I might be misinterpreting the intent of your issue. My use case would probably require multiple (overlapping and/or mutually exclusive) lock categories - like we have now for balances. Perhaps that's not what you're aiming to address?

@pandres95
Copy link
Contributor Author

It is, makes sense to call it an assets-locker as this is the concept that's historically been used to refer to freezes (consider calling them Freezes instead of Locks is fairly recent).

@JelliedOwl
Copy link

Thanks Pablo

I suspect I'll end up with an extension pallet to do the Multi-asset Locking, which will interact with yours - but yours is probably more likely to be widely used so the interfacing and testing / maintaining should probably by my pallet's responsibility. I suspect Parity isn't going to jump at taking on maintaining it for me.

There might be too much cross-interaction for my pallet to be just an extension anyway, so I might end up having to fork pallet-assets, which I'd really like to avoid.

For example, I need account-frozen assets which are frozen by my multi-asset freezer not to be able to call the unlock function on those assets. (That possibly applies to assets frozen from the regular pallet_assets too, so you might need to handle that bit, at least). Hopefully that makes sense...

@liamaharon
Copy link
Contributor

Any update on this @pandres95? It's needed for #3149, happy to provide any help you may need getting it moving.

@liamaharon
Copy link
Contributor

Also want to mention this PR paritytech/substrate#8476 here. The solution for this issue should probably follow it closely.

github-merge-queue bot pushed a commit that referenced this issue Jun 21, 2024
Closes #3342

cc/ @liamaharon

TODO:

- [x] Improve docs.
- [x] Define public interface (See #3342).
  In case we define public calls to the pallet implementation:
  - Implement public calls.
  - Benchmarks.
  
polkadot address: 12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR

---------

Co-authored-by: command-bot <>
Co-authored-by: Liam Aharon <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Closes paritytech#3342

cc/ @liamaharon

TODO:

- [x] Improve docs.
- [x] Define public interface (See paritytech#3342).
  In case we define public calls to the pallet implementation:
  - Implement public calls.
  - Benchmarks.
  
polkadot address: 12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR

---------

Co-authored-by: command-bot <>
Co-authored-by: Liam Aharon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants