fungibles methods do not consume AssetId
#4224
Conversation
liamaharon
left a comment
liamaharon
left a comment
There was a problem hiding this comment.
Thanks for helping with this!
| assert_eq!(Fungibles::name(&asset_id.clone().into()), Vec::from(expected_name),); | ||
| assert_eq!(Fungibles::symbol(&asset_id.clone().into()), Vec::from(expected_symbol),); |
There was a problem hiding this comment.
Possible to remove the .clones in scenarios like this or?
There was a problem hiding this comment.
All these tests helper functions provide asset_id as impl Into<Fungibles::AssetId> + Clone. I would need to change the API of these functions, then a lot of test callsites.
I was thinking it's not worth the effort and the noise since these are only tests.
| fn total_issuance(asset: Self::AssetId) -> Self::Balance { | ||
| match Criterion::convert(asset) { | ||
| fn total_issuance(asset: &Self::AssetId) -> Self::Balance { | ||
| match Criterion::convert(asset.clone()) { |
There was a problem hiding this comment.
Hmm I wonder if Criterion::convert should also take a ref?
There was a problem hiding this comment.
I initially tried, but got lost in the sea of errors. There are so many generic types and trait bounds used here, hard to follow.
I will give it another try.
There was a problem hiding this comment.
Can't make it work.. got as far as dianpopa@090c74e, maybe someone can help or do it in another PR.
|
Context: This is good clean up. It's because |
|
PR has a lot of changes, but the vast majority is test code because of the changed inner functions APIs. The "main" API changes are small I think. |
|
@liamaharon @gilescope is there anything else required here? |
liamaharon
left a comment
liamaharon
left a comment
There was a problem hiding this comment.
Looks good, maybe some redundant .clones still in there?
| type ForeignAssets = <PenpalA as PenpalAPallet>::ForeignAssets; | ||
| <ForeignAssets as Inspect<_>>::balance(system_para_native_asset_location.clone(), &receiver) | ||
| <ForeignAssets as Inspect<_>>::balance( | ||
| &system_para_native_asset_location.clone(), |
There was a problem hiding this comment.
Does this and all similar still need .clone()?
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
@liamaharon is this fix for #3968 still wanted? |
|
I agree this is a good fix and a tedious one (the amount of compiler errors, number of files), we should push for it to get merged before more conflicts pile up. @liamaharon thoughts? |
|
Looks like the pr is stale. |
7 participants
Fungiblesmethods now takeAssetIdby reference instead of value where the value is not needed.pallet-assetsmethods now also takeAssetIdby reference instead of value where the value is not needed.This avoids a lot of cloning across the codebase.
There are more objects and traits that could do the same, but this PR already blew up in size as I was changing the most relevant ones. This "small" change is actually pretty big..
fixes #3968
Polkadot address: 14rY421S6kMMSRo9GkbMrWiVjD2UnbhFp6Boy9pDeYnBy1hD