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

Adapter from tokens::fungible to tokens::fungibles #2858

Closed
wants to merge 2 commits into from

Conversation

nanocryk
Copy link

@nanocryk nanocryk commented Jan 5, 2024

Currently there is frame_support::traits::fungible::ItemOf which converts a fungibles into a fungible, but not the other way around. This PR thus adds an adapter from fungible to fungibles.

@nanocryk nanocryk requested review from a team January 5, 2024 14:30
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 5, 2024 14:30
// See the License for the specific language governing permissions and
// limitations under the License.

//! Adapter to use `fungibles:*` implementations as `fungibles::*`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! Adapter to use `fungibles:*` implementations as `fungibles::*`.
//! Adapter to use `fungible::*` implementations as `fungibles::*`.

@ggwpez ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jan 5, 2024
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM but better wait for someone who knows more to review.

Comment on lines 27 to 28
/// Redirects `fungibles` function to the `fungible` equivalent without
/// the AssetId argument.
Copy link
Member

Choose a reason for hiding this comment

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

Please try to do line-breaks at exactly 100 chars.


/// Redirects `fungibles` function to the `fungible` equivalent without
/// the AssetId argument.
macro_rules! redirect {
Copy link
Member

Choose a reason for hiding this comment

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

I am generally not a fan of macros, but this looks like a good use.
Maybe we should do the same for the ItemOf adapter.

Copy link
Author

@nanocryk nanocryk Jan 5, 2024

Choose a reason for hiding this comment

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

Yeah, me too. This one should not be too hard to read, and remove a lot of boilerplate.
Should the change in ItemOf be done in this PR or another?

Copy link
Member

Choose a reason for hiding this comment

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

In another merge request would be better 🙏

Copy link
Author

Choose a reason for hiding this comment

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

};
}

pub struct ConvertHandleImbalanceDrop<H>(PhantomData<H>);
Copy link
Member

Choose a reason for hiding this comment

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

Some doc please on all public items.

Copy link
Author

Choose a reason for hiding this comment

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

item_of::ConvertImbalanceDropHandler doesn't have documentation either, and both are not re-exported. Should both have documentation or none?

}
}

/// A wrapper to use a `fungible` as a `fungibles` with a single asset.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// A wrapper to use a `fungible` as a `fungibles` with a single asset.
/// A wrapper to use a `fungible` as a `fungibles` with a single asset `()`.

(maybe this is obvious to others but i am not that deep into the asset stuff)

@ggwpez ggwpez requested a review from muharem January 5, 2024 15:01
Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

There are adapters, fungible::UnionOf (adapts some fungible to fungibles) and fungibles::UnionOf (adapts some fungibles to new fungibles). they have unit tests, audited and already in use.

@nanocryk
Copy link
Author

nanocryk commented Jan 8, 2024

There are adapters, fungible::UnionOf (adapts some fungible to fungibles) and fungibles::UnionOf (adapts some fungibles to new fungibles). they have unit tests, audited and already in use.

fungible::UnionOf doesn't allow to use a single fungible as a fungibles, and requires another fungibles (hence "union"). Thus it is not possible to use a single fungible (like pallet_balances) in a pallet expecting fungibles.

@muharem
Copy link
Contributor

muharem commented Jan 9, 2024

There are adapters, fungible::UnionOf (adapts some fungible to fungibles) and fungibles::UnionOf (adapts some fungibles to new fungibles). they have unit tests, audited and already in use.

fungible::UnionOf doesn't allow to use a single fungible as a fungibles, and requires another fungibles (hence "union"). Thus it is not possible to use a single fungible (like pallet_balances) in a pallet expecting fungibles.

yes, but we do not need new adapter impls to achieve this. we need union of some fungible/s and empty set. where empty set can be presented as no-op implementation over unit type. plus you can introduce some type alias with some predefined type arguments (convertor, empty set) for UnionOf to get a better api.
with this you achieve the same, plus we get no-op impls, which useful by themself and most part of it is tested and audited. also we do not increase entropy for this package that much with new marco impls.

@nanocryk nanocryk closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants