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

[xcm] ConvertedConcreteId convert to MultiAsset (fungible) back-and-forth #6760

Closed
wants to merge 1 commit into from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Feb 22, 2023

related to the #6777

Context:
We started on work on runtime FungiblesApi to query all fungible assets from runtime for better UX/UI experience.
This API could be used by all runtimes.

So in this first iteration we have api (Basti suggests to have it in Cumulus):
In Cumulus:
https://github.com/paritytech/cumulus/pull/2180/files#diff-5739a90b109dd72bff24e5e1d8a1e7854fb56fcd62d7174fcc6d838f495f52f0R34-R39
Or Polkadot:
https://github.com/paritytech/polkadot/pull/6777/files#diff-df0fea42045207170f8b159d2640ccfcd00da630bd57f38a2cca8a9ee79f698bR37-R42


So, in Cumulus for Statemine/Statemint/Westmint we have implementation, which reads account balances from pallet_balances and from multiple instances of pallet_assets.
For pallet_assets we use ConvertedConreteId, which is going to be replaced by MatchedConvertedConcreteId, when this PR is merged.

So, the idea behind this PR, is that ConvertedConreteId/MatchedConvertedConcreteId does conversion from MultiAsset->(AssetId, Balance), and it would be very useful, if it does the backwards also (AssetId, Balance)->MultiAsset to minimalize adding new code/custom conversions like this in every (Cumulus or whatever (para)chain) runtime.

@bkontur bkontur added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T6-XCM This PR/Issue is related to XCM. labels Feb 22, 2023
Balance: Clone,
ConvertAssetId: Convert<MultiLocation, AssetId>,
ConvertBalance: Convert<u128, Balance>,
> Convert<(AssetId, Balance), MultiAsset>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang
it would be cool and useful to have backwards conversion (AssetId, Balance) -> MultiAsset for ConvertedConcreteId/MatchedConvertedConcreteId automatically,

if I add the similar impl for ConvertedConcreteId for non-fungibles

impl<
		ClassId: Clone,
		InstanceId: Clone,
		ConvertClassId: Convert<MultiLocation, ClassId>,
		ConvertInstanceId: Convert<AssetInstance, InstanceId>,
	> Convert<(ClassId, InstanceId), MultiAsset>
 	for ConvertedConcreteId<ClassId, InstanceId, ConvertClassId, ConvertInstanceId>

it errors with:

error[E0119]: conflicting implementations of trait

so maybe, some new trait to extend traits MatchesFungibles / MatchesNonFungibles or some other solution?
or we dont want to support this here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you first give some context as to what problem this PR is trying to solve? It's hard to reason about your solutions here when I don't have much idea what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, sure, no problem, I updated here desc: #6760 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, but doesn't reverse_ref already does the thing that you expect it to?

@KiChjang
Copy link
Contributor

@bkontur Is this PR still wanted?

@bkontur
Copy link
Contributor Author

bkontur commented Aug 17, 2023

@bkontur Is this PR still wanted?

not really, closing

@bkontur bkontur closed this Aug 17, 2023
@bkontur bkontur deleted the bko-xcm-assets-convert branch August 17, 2023 07:08
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants