UnionOf types for merged fungible and fungibles implementations
#2033
Conversation
muharem
added
the
T1-FRAME
muharem
changed the title
UnionOf fungible and fungibles, or fungibles and fungiblesUnionOf types for merged fungible and fungibles implementations
liamaharon
left a comment
liamaharon
left a comment
There was a problem hiding this comment.
Looking good.
I'm also curious to learn what use cases there are for this feature rn.
substrate/frame/support/src/traits/tokens/fungibles/union_of.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Liam Aharon <[email protected]>
I have updated the PR description with the links to where it is used. |
substrate/frame/support/src/traits/tokens/fungibles/union_of.rs
Outdated
Show resolved
Hide resolved
franciscoaguirre
left a comment
franciscoaguirre
left a comment
There was a problem hiding this comment.
I feel like the repetitive code in the union_ofs could be simplified with a macro somehow. At least I hope
substrate/frame/assets/src/tests.rs
Outdated
| }); | ||
| } | ||
|
|
||
| mod sets { |
There was a problem hiding this comment.
I'm not a fan of extremely long files, maybe we can move this to its own file under tests/sets.rs? What do you think?
ggwpez
left a comment
ggwpez
left a comment
There was a problem hiding this comment.
Approving, but the dedicated conversion function would still be nice 😄
| Self::Balance::zero() | ||
| } | ||
| fn should_touch(_: (), _: &T::AccountId) -> bool { | ||
| false |
There was a problem hiding this comment.
I dont know much about fungibles, but why is this not doing anything?
There was a problem hiding this comment.
this is because there is a guarantee that if you transfer some amount which is greater than ED, the account will be created. it wont fail because of some account creation requirements.
initially this trait was introduced for the asset conversion pallet, so that the pool account creation for some asset can be guaranteed. otherwise someone for example can derive future pool account ids and exhaust all consumer references.
I also tried to explain this in docs for AccountTouch trait.
| } | ||
| } | ||
| impl<AssetId: Ord> Ord for NativeOrWithId<AssetId> { | ||
| fn cmp(&self, other: &Self) -> Ordering { |
There was a problem hiding this comment.
Why can you not use the default impl?
There was a problem hiding this comment.
it cannot be derived for this enum. if you tell how, I drop this
| |inner_debt| { | ||
| let debt = Imbalance::new(inner_debt.peek()); | ||
| fungibles::Imbalance::forget(inner_debt); | ||
| debt | ||
| }, |
There was a problem hiding this comment.
Would be nice to put this into a conversion function, so that it can become <F as fungibles::Balanced<AccountId>>::deposit(A::get(), who, value, precision).map(conversion_function).
|
@ggwpez can you check last two commits, with conversion functions looks cleaner, thank you. also replied to other comments. |
substrate/frame/support/src/traits/tokens/fungibles/imbalance.rs
Outdated
Show resolved
Hide resolved
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/polkadot-release-analysis-v1-6-0/5855/1 |
Introduces
UnionOftypes, crafted to mergefungibleandfungiblesimplementations or twofungiblesimplementations into a single type implementingfungibles.This also addresses an issue where
ItemOfinitiates a double drop for an imbalance type, leading to inaccurate total issuance accounting.Find the application of these types in this PR - link, places in code - 1, 2.