-
Notifications
You must be signed in to change notification settings - Fork 45
RFC: Rename locking instructions #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
vstam1
wants to merge
2
commits into
master
Choose a base branch
from
vstam1/rename-locking-instructions
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| --- | ||
| Title: Renaming Lock Instructions | ||
| Number: 0 | ||
| Status: Draft | ||
| Version: 0 | ||
| Authors: | ||
| - vstam1 | ||
| Created: 2023-07-18 | ||
| Impact: Breaking | ||
| Requires: | ||
| Replaces: | ||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| This RFC proposes renaming several XCM instructions to align with changes in FRAME naming. | ||
| The following instructions and error are affected: | ||
|
|
||
| Instructions: | ||
| `LockAsset` -> `FreezeAsset` | ||
| `UnlockAsset` -> `ThawAsset` | ||
| `NoteUnlockable` -> `NoteThawable` | ||
| `RequestUnlock` -> `RequestThaw` | ||
|
|
||
| Error: | ||
| `LockError` -> `FreezeError` | ||
|
|
||
|
|
||
| ## Motivation | ||
|
|
||
| The renaming of these instructions is intended to eliminate potential confusion caused by naming mismatches between XCM and FRAME. By ensuring consistency in naming conventions across both platforms, we can enhance clarity and maintain a unified language. | ||
|
|
||
| ## Specification | ||
|
|
||
| The renamed instructions are defined as follows: | ||
| ```rust | ||
| /// Freeze the locally held asset and prevent further transfer or withdrawal. | ||
| /// | ||
| /// This restriction may be removed by the `ThawAsset` instruction being called with an | ||
| /// Origin of `thawer` and a `target` equal to the current `Origin`. | ||
| /// | ||
| /// If the freezing is successful, then a `NoteThawable` instruction is sent to `thawer`. | ||
| /// | ||
| /// - `asset`: The asset(s) which should be frozen. | ||
| /// - `unlocker`: The value which the Origin must be for a corresponding `ThawAsset` | ||
| /// instruction to work. | ||
| FreezeAsset { asset: MultiAsset, thawer: MultiLocation }, | ||
|
|
||
| /// Remove the freeze over `asset` on this chain and (if nothing else is preventing it) allow the | ||
| /// asset to be transferred. | ||
| /// | ||
| /// - `asset`: The asset to be thawed. | ||
| /// - `target`: The owner of the asset on the local chain. | ||
| ThawAsset { asset: MultiAsset, target: MultiLocation }, | ||
|
|
||
| /// Asset (`asset`) has been frozen on the `origin` system and may not be transferred. It may | ||
| /// only be thawed with the receipt of the `ThawAsset` instruction from this chain. | ||
| /// | ||
| /// - asset: The asset(s) which are now thawable from this origin. | ||
| /// - owner: The owner of the asset on the chain where it was frozen. This may be a | ||
| /// location specific to the origin network. | ||
| /// | ||
| /// Safety: origin must be trusted to have frozen the corresponding asset | ||
| /// prior as a consequence of sending this message. | ||
| NoteFreezable { asset: MultiAsset, owner: MultiLocation }, | ||
|
|
||
| /// Send a ThawAsset instruction to the thawer for the given asset. | ||
| /// | ||
| /// This may fail if the local system is making use of the fact that the asset is frozen or, | ||
| /// of course, if there is no record that the asset actually is frozen. | ||
| /// | ||
| /// - asset: The asset(s) to be thawed. | ||
| /// - thawer: The location from which a previous NoteThawable was sent and to which | ||
| /// a ThawAsset should be sent. | ||
| RequestThaw { asset: MultiAsset, thawer: MultiLocation }, | ||
| ``` | ||
|
|
||
| And Error: | ||
| ```rust | ||
| /// Some other error with Freeze. | ||
| FreezeError | ||
| ``` | ||
|
|
||
| ## Security considerations | ||
|
|
||
| The renaming process must ensure that all instances of the old instructions are properly | ||
| converted to the new ones to maintain compatibility between the different XCM versions. | ||
|
|
||
| ## Impact | ||
|
|
||
| This change will impact all XCM code that uses the lock instructions, constituting a breaking change. All code should be updated to use these new names. Failure to update the code could result in errors. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| An alternative to this change would be to keep the naming the same. However, this would result in a naming mismatch between XCM and FRAME, which could lead to confusion among developers and users. | ||
|
|
||
| ## Questions and open Discussions (optional) | ||
| No open questions. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep in mind this will probably go to a new XCM version, which means the migration is pretty straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a breaking change in the API level, but not on the transaction layer, since the codec indices for these instructions did not change, nor did the instructions parameters change.