-
Notifications
You must be signed in to change notification settings - Fork 809
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
[Assets] Call implementation for transfer_all
#4527
Changes from 8 commits
4fdb4f5
b7086a8
4d7a328
d3247a1
baf45ad
5d3c189
de997aa
ae3dd9f
1ba0962
7e11870
f0159d8
1ac9bbb
cd0961d
a9359c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,27 @@ | ||||||||||
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 | ||||||||||
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json | ||||||||||
|
||||||||||
title: Call implementation for `transfer_all` | ||||||||||
|
||||||||||
doc: | ||||||||||
- audience: Runtime Dev | ||||||||||
description: | | ||||||||||
This PR introduces the `transfer_all` call for `pallet-assets`. | ||||||||||
The parameters are analog to the same call in `pallet-balances`. | ||||||||||
This change shouldn't break the existing APIs. | ||||||||||
This change requires running benchmarkings to set accurate weights for | ||||||||||
the call. | ||||||||||
- audience: Runtime User | ||||||||||
description: | | ||||||||||
This PR introduces the `transfer_all` call for `pallet-assets`. | ||||||||||
The parameters are analog to the same call in `pallet-balances`. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I consider it's still useful to let runtime users know about this update, as this potentially impacts users like CEXes that will come to the changelog to know about how this change works. |
||||||||||
When using this call, consider that transferring all funds of an asset | ||||||||||
account might kill it when setting `keep_alive` as false. | ||||||||||
|
||||||||||
crates: | ||||||||||
- name: pallet-assets | ||||||||||
bump: minor | ||||||||||
pandres95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
- name: asset-hub-rococo-runtime | ||||||||||
bump: minor | ||||||||||
- name: asset-hub-westend-runtime | ||||||||||
bump: minor |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
new call is new enum variant within Call enum. so this is a breaking change as for a rust crate.
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.
That's rough. I guess we have to follow crates rules but it's not breaking for transaction encoding.
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.
I can change it with "This change is expected to be backwards-compatible". While technically it is true that the encoding changes, @joepetrowski is right in the sense that it doesn't change transaction encoding, as it is appending a new call at the end of the
Call
enum. Therefore, it is safe to say it is backwards compatible (which is the expected behaviour in these cases).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.
Yes, but for crates versioning I think it should follow the rust semver rules. At least that's what we have not in guidelines.
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.
Yeah, @ggwpez also pointed out that it breaks the
WeightInfo
trait for anyone using this pallet. So I agree it is (unfortunately) major.