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

Use fungible traits for treasury pallet #13476

Open
wants to merge 116 commits into
base: master
Choose a base branch
from

Conversation

tonyalaribe
Copy link
Contributor

@tonyalaribe tonyalaribe commented Feb 27, 2023

Background

The current currency traits are deprecated and there's currently a move to modify the existing pallets to support the fungible traits. paritytech/polkadot-sdk#226
This is an important step towards some more topics which are coming up, such as the Multi-Asset Treasury topic which will build on these fungible traits.

Review notes:

Since some of the fungible traits are fairly new and some methods appear to have similar uses, it's not always clear if i'm using the right traits and methods. For eg it's not always clear whether to use mint_into or set_balance in tests, as a replacement for Balances::make_free_balance_be()

It's also not always clear what the direct fungible equivalents of some Currency methods are. Eg Currency has deposit_into_existing and deposit_creating and only deposit exists under fungible with the Precision::Exact and Precision::BestEffort flags. Please keep this in mind while reviewing, to spot scenarios where this PR might be using the wrong fungible method equivalents.

The PR depends on the changes to the fungible traits in the gav-hold-freeze branch (#12951), and would have to be reanchored on master when that branch has been merged

@tonyalaribe tonyalaribe changed the title use fungibles traits for treasury pallet wip: use fungibles traits for treasury pallet Feb 28, 2023
@tonyalaribe tonyalaribe changed the title wip: use fungibles traits for treasury pallet use fungibles traits for treasury pallet Mar 3, 2023
@tonyalaribe tonyalaribe changed the title use fungibles traits for treasury pallet Use fungibles traits for treasury pallet Mar 3, 2023
@tonyalaribe tonyalaribe force-pushed the aa/use-fungibles-traits-for-treasury branch from ddb3ff8 to 42789a2 Compare March 6, 2023 09:06
@tonyalaribe tonyalaribe added A0-please_review Pull request needs code review. C3-medium PR touches the given topic and has a medium impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited E5-needs_cumulus_pr This is an issue that needs to be implemented upstream in Cumulus. F3-breaks_API This PR changes public API; next release should be major. labels Mar 6, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2487220

@tonyalaribe tonyalaribe changed the title Use fungibles traits for treasury pallet Use fungible traits for treasury pallet Mar 6, 2023
@tonyalaribe tonyalaribe marked this pull request as ready for review March 6, 2023 09:15
@vivekvpandya
Copy link
Contributor

Thanks! @tonyalaribe this is very helpful PR to understand the whole change going on with #12951
Does this PR require any migration? like converting existing balances which has been set with lock()/reserve() to be compatible with freeze()/hold() ?

@tonyalaribe
Copy link
Contributor Author

Thanks! @tonyalaribe this is very helpful PR to understand the whole change going on with #12951 Does this PR require any migration? like converting existing balances which has been set with lock()/reserve() to be compatible with freeze()/hold() ?

I believe so. But the need for migrations will be from the work on the balance pallet in #12951. So the migrations are more for the balance pallet than the treasury pallet.

Base automatically changed from gav-hold-freeze to master March 18, 2023 14:47
@paritytech-processbot paritytech-processbot bot requested a review from a team March 18, 2023 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C3-medium PR touches the given topic and has a medium impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited E5-needs_cumulus_pr This is an issue that needs to be implemented upstream in Cumulus. F3-breaks_API This PR changes public API; next release should be major.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants