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

XCM remote lock consumers #6947

Merged
merged 10 commits into from
May 5, 2023
Merged

XCM remote lock consumers #6947

merged 10 commits into from
May 5, 2023

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Mar 24, 2023 •
edited
Loading

The current version of the remote lock entry only tracks the total count of its local users, which means that each local user can only hold the entire amount of the lock. However, to allow for partial locking and unlocking of the remote lock, as well as the ability to extend the lock for a specific user, we need more information than just the user count. Specifically, we store identifiers for each user and the exact amount held by them.
Also in this PR we rename users to consumers.

cumulus companion: paritytech/cumulus#2463

Migration

Since there is no universal solution to migrate the storage to the new storage version, all clients who have used the remote locks has to write the migration them-self. Contact me for support.
All polkadot and cumulus repos' runtimes have no trusted lockers, hence the changed storage item is empty, no migration needed.
I also assume no one yet using the feature, since it was introduced in XCMv3, and there is no yet API to allocate the remote lock.

@muharem muharem added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. F3-breaks_API This PR changes public API; next release should be major. labels Mar 24, 2023
@paritytech-ci paritytech-ci requested review from a team March 24, 2023 10:32
@muharem muharem requested review from KiChjang and gavofyork March 24, 2023 12:27
@muharem muharem requested a review from KiChjang March 29, 2023 09:23
@muharem
Copy link
Contributor Author

muharem commented Apr 14, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem muharem mentioned this pull request Apr 18, 2023
Merged
@muharem muharem changed the title XCM remote lock consumers XCM remote lock users id Apr 20, 2023
@muharem muharem requested review from bkchr, kianenigma and ggwpez April 20, 2023 10:13
bkchr
bkchr approved these changes Apr 28, 2023
View reviewed changes
xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team April 28, 2023 08:56
@bkchr
Copy link
Member

bkchr commented Apr 28, 2023

All polkadot and cumulus repos' runtimes have no trusted lockers, we don't need a transaction for them for sure.
Probably no one yet using the feature, since it was introduced in XCMv3, and there is no yet API to allocate the remote lock. Regardless there is no universal solution to migrate to the new storage version.

Are you 100000% sure that no one is using this right now? Otherwise we should at least put the runtime migration label and leave some small text in the description on how to check and then at least tell them to reach out as I assume they may can not write the migration on their own.

@muharem
Copy link
Contributor Author

muharem commented Apr 28, 2023

All polkadot and cumulus repos' runtimes have no trusted lockers, we don't need a transaction for them for sure.
Probably no one yet using the feature, since it was introduced in XCMv3, and there is no yet API to allocate the remote lock. Regardless there is no universal solution to migrate to the new storage version.

Are you 100000% sure that no one is using this right now? Otherwise we should at least put the runtime migration label and leave some small text in the description on how to check and then at least tell them to reach out as I assume they may can not write the migration on their own.

I cant be that sure (:, but worth the label for sure.

@muharem muharem added the E1-database_migration PR introduces code that does a one-way migration of the database. label Apr 28, 2023
@muharem muharem changed the title XCM remote lock users id XCM remote lock consumers Apr 28, 2023
@paritytech-ci paritytech-ci requested a review from a team May 1, 2023 21:46
@muharem
Copy link
Contributor Author

muharem commented May 2, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem muharem requested review from eskimor, ordian and sandreim May 2, 2023 08:17
@ordian ordian added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels May 5, 2023
@muharem
Copy link
Contributor Author

muharem commented May 5, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 5e7103d into master May 5, 2023
@paritytech-processbot paritytech-processbot bot deleted the muharem-xcm-remote-lock-consumers branch May 5, 2023 14:11
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
@Agusrodri Agusrodri mentioned this pull request Jun 21, 2023
Merged
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jul 11, 2023
@AurevoirXavier
Companion of paritytech/polkadot#6947
41989f6
@AurevoirXavier AurevoirXavier mentioned this pull request Jul 11, 2023
Merged
xlc
xlc reviewed Jul 21, 2023
View reviewed changes
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I am reading this code and super confused.
consumers is never pushed. RemoteLockConsumerIdentifier is never passed or created.

How do we exactly configure our chain for MaxRemoteLockConsumers and RemoteLockConsumerIdentifier??

@muharem
Copy link
Contributor Author

muharem commented Aug 1, 2023

@xlc right now its not utilized. the main purpose of this PR was to change the storage schema into desired one.
the xcm pallet will need to provide means to hold/release a lock on a remote lock with a given identifier.

@simonsso simonsso mentioned this pull request Dec 5, 2023
Merged
10 tasks
@gianfra-t gianfra-t mentioned this pull request Aug 20, 2024
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xlc xlc xlc left review comments

@bkchr bkchr bkchr approved these changes

@ordian ordian ordian approved these changes

@KiChjang KiChjang KiChjang approved these changes

@gavofyork gavofyork Awaiting requested review from gavofyork

@kianenigma kianenigma Awaiting requested review from kianenigma

@ggwpez ggwpez Awaiting requested review from ggwpez

@eskimor eskimor Awaiting requested review from eskimor

@sandreim sandreim Awaiting requested review from sandreim

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E1-database_migration PR introduces code that does a one-way migration of the database. F3-breaks_API This PR changes public API; next release should be major.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@muharem @bkchr @xlc @KiChjang @ordian