Conviction pallet uses fungible trait #1821
Conversation
There was a problem hiding this comment.
Looking good, thanks!
Some notes
- Need to use freezes instead of holds here. Freezes replace locks, while holds replace reserves
- I think you could use the new
Considerationtickets for reserving funds here, but a benefit of using freezes directly is the minimal code changes that need to take place, so easier review, and the priority is primarily migrating away from Currency so I'm fine with this approach of not usingConsiderationif you prefer that - Needs a migration to migrate existing locks to freezes
| /// The overarching hold reason. | ||
| type RuntimeHoldReason: From<HoldReason>; |
There was a problem hiding this comment.
nit: maybe put this up the top with RuntimeEvent?
| #[pallet::weight(T::WeightInfo::unlock())] | ||
| pub fn unlock( | ||
| #[pallet::weight(T::WeightInfo::release())] | ||
| pub fn release( |
There was a problem hiding this comment.
Let's avoid breaking the API for the pallet and just leave this method called 'unlock'.
You can explain the naming in a doc comment.
There was a problem hiding this comment.
This shouldn't actually break it https://substrate.stackexchange.com/questions/246/does-renaming-an-extrinsic-require-a-transaction-version-bump
| type Currency: ReservableCurrency<Self::AccountId> | ||
| + LockableCurrency<Self::AccountId, Moment = BlockNumberFor<Self>> | ||
| + fungible::Inspect<Self::AccountId>; | ||
| type Currency: MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason> |
There was a problem hiding this comment.
and the comment above need to be updated
| type Currency: MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason> | |
| type Fungible: MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason> |
There was a problem hiding this comment.
I would keep Currency here, this has already been kept in other migrations like
https://github.com/paritytech/substrate/pull/14020/files#diff-70e9723e9db62816e35f6f885b6770a8449c75a6c2733e9fa7a245fe52c4656cR210-R213
There was a support from @shawntabrizi and @liamaharon towards renaming in here paritytech/substrate#14020 (comment), but it didn't end happening.
My reasoning behind not renaming is that, at the end of the day, the type is still representing a currency, even though it does not implement a currency trait directly but rather a summary of other traits that put together do represent a currency in this context
There was a problem hiding this comment.
I would keep Currency here, this has already been kept in other migrations
We shouldn't keep doing bad patterns just because they've been done in the past. I think if people forgot to rename 'Currency' in prior PRs, we should open a new PR to fix them instead of continuing the confusing naming.
the type is still representing a currency
The type also still represents a fungible. Fungible in this context I think is much more clear and less prone to being misinterpreted.
|
I've fixed the westend and rococo runtimes so they compile with the pallet now using freezes. Took me quite some time to realize the From impl of the RuntimeFreezeReason for the pallet specific FreezeReason comes from construct_runtime! 😆 I've used no renaming of imports so I hope this conforms with the latest standardization effort as mentioned in the tracking issue #226 |
PieWol
force-pushed
the
conviction-use-fungible
branch
from
ead606f
to
620f7e7
Compare
|
Hey @ggwpez , |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Trying to help out with Issue #226