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

Relax Copy to Clone for AssetId#12731

Closed
gilescope wants to merge 3 commits intomasterfrom
giles-asset-id-copy-to-clone
Closed

Relax Copy to Clone for AssetId#12731
gilescope wants to merge 3 commits intomasterfrom
giles-asset-id-copy-to-clone

Conversation

@gilescope
Copy link
Contributor

@gilescope gilescope commented Nov 17, 2022 •
edited
Loading

For the bridgehub we want to use a second Asset pallet for bridged assets. The suggestion is to have the AssetId of this instance be a MultiLocation. However Multilocation is not Copy, so this PR is exploring the impact of relaxing the Copy bound to only require Clone.

cumulus companion: paritytech/cumulus#1900

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 17, 2022
@gilescope gilescope added B3-apinoteworthy 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 Nov 17, 2022
acatangiu
acatangiu approved these changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks ok to me

@gilescope
Copy link
Contributor Author

gilescope commented Nov 18, 2022 •
edited
Loading

Maybe this is not needed - MulitiLocation v3 is Copy. We could potentially wait for that to land.

@joepetrowski
Copy link
Contributor

joepetrowski commented Nov 19, 2022

Maybe this is not needed - MulitiLocation v3 is Copy. We could potentially wait for that to land.

I agree focusing on v3 is probably the best strategy as it's already needed for related goals with the second Assets pallet.

@gilescope
Copy link
Contributor Author

gilescope commented Nov 21, 2022

if we're doing #12740 then maybe we should as then its not delayed by xcm3. We can have it all hit master which is going to be less complicated.

@gilescope gilescope mentioned this pull request Nov 21, 2022
Closed
@stale
Copy link

stale bot commented Dec 21, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 21, 2022
@gilescope
Copy link
Contributor Author

gilescope commented Dec 22, 2022

I think we can get away with keeping copy.

@gilescope gilescope closed this Dec 22, 2022
@koute koute mentioned this pull request May 16, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@acatangiu acatangiu acatangiu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gilescope @joepetrowski @acatangiu