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

Remove the Copy bound on AssetId #14158

Merged
merged 12 commits into from
May 23, 2023
Merged

Remove the Copy bound on AssetId #14158

merged 12 commits into from
May 23, 2023

Conversation

koute
Copy link
Contributor

@koute koute commented May 16, 2023 •
edited
Loading

This PR relaxes the requirement for AssetIds to be Copy; now it only needs to be Clone.

This is necessary for paritytech/polkadot#7236 (or more specifically, for its cumulus companion)

This is based on the following PR (thanks @gilescope!): #12731

cumulus companion: paritytech/cumulus#2586

@koute koute added A0-please_review Pull request needs code review. I7-refactor Code needs refactoring. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 16, 2023
@koute koute requested review from gilescope, KiChjang, ggwpez, a team and acatangiu May 16, 2023 11:10
@koute koute mentioned this pull request May 16, 2023
Open
melekes
melekes approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@koute koute mentioned this pull request May 16, 2023
Merged
@koute koute changed the title Remove the Copy bound on the AssetId trait Remove the Copy bound on AssetId May 16, 2023
@juangirini
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

ggwpez
ggwpez approved these changes May 16, 2023
View reviewed changes
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.

Did not comment all instances of where an impl Emcode is enough.

frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Show resolved Hide resolved
frame/assets/src/functions.rs Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented May 17, 2023

@ggwpez I did remove some more unnecessary clone()s, although please note in that some of the places that you've highlighted the clone is actually necessary (e.g. the id needs to be moved into the closure which try_mutate takes so try_mutate itself can't take id by a reference).

Would be nice if we had a custom clippy lint for this. When you're mechanically changing dozens/hundreds of these at a time it's hard to make sure that all of the clone()s are really needed.

@koute
Copy link
Contributor Author

koute commented May 20, 2023

(Just an FYI, even though the CI was green and I could have merged this PR I've pushed a merge commit with the current master anyway to make sure the CI's still green. There were some new commits that were merged to master since the CI ran on this PR, and I don't really want to accidentally break master.)

@koute
Copy link
Contributor Author

koute commented May 23, 2023

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/cumulus#2586 is not mergeable

@koute
Copy link
Contributor Author

koute commented May 23, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit aacba0f into paritytech:master May 23, 2023
gpestana pushed a commit that referenced this pull request May 27, 2023
@koute @gpestana
Remove the Copy bound on AssetId (#14158)
9848703 
* Remove the `Copy` bound on `AssetId`

* Also relax the `Copy` bound in the assets pallet

* Fix warnings on the newest nightly Rust

* Remove some unnecessary `clone()`s

* Try to satisfy clippy

* Remove some more unnecessary `clone()`s

* Add more `.clone()`s for newly merged code

* Also add `clone()`s in the benchmarks

---------

Co-authored-by: parity-processbot <>
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-43/3158/3

@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
@jasl jasl mentioned this pull request Jun 30, 2023
Merged
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
@koute @Ank4n
Remove the Copy bound on AssetId (#14158)
3233c9f 
* Remove the `Copy` bound on `AssetId`

* Also relax the `Copy` bound in the assets pallet

* Fix warnings on the newest nightly Rust

* Remove some unnecessary `clone()`s

* Try to satisfy clippy

* Remove some more unnecessary `clone()`s

* Add more `.clone()`s for newly merged code

* Also add `clone()`s in the benchmarks

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@koute @nathanwhit
Remove the Copy bound on AssetId (paritytech#14158)
38d491e 
* Remove the `Copy` bound on `AssetId`

* Also relax the `Copy` bound in the assets pallet

* Fix warnings on the newest nightly Rust

* Remove some unnecessary `clone()`s

* Try to satisfy clippy

* Remove some more unnecessary `clone()`s

* Add more `.clone()`s for newly merged code

* Also add `clone()`s in the benchmarks

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@KiChjang KiChjang KiChjang approved these changes

@sam0x17 sam0x17 sam0x17 approved these changes

@melekes melekes melekes approved these changes

@gilescope gilescope gilescope approved these changes

@acatangiu acatangiu Awaiting requested review from acatangiu

Assignees
No one assigned
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit I7-refactor Code needs refactoring.
Projects
SDK Node
Status: done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@koute @juangirini @Polkadot-Forum @gilescope @melekes @sam0x17 @KiChjang @franciscoaguirre @ggwpez