Skip to content
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

Merged
merged 14 commits into from
Aug 7, 2024

Conversation

pandres95
Copy link
Contributor

Closes #4517

Polkadot address: 12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR

@pandres95 pandres95 requested a review from a team as a code owner May 20, 2024 14:52
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6247608

@kianenigma
Copy link
Contributor

/tip medium

Copy link

@kianenigma A referendum for a medium (80 DOT) tip was successfully submitted for @pandres95 (12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR on polkadot).

Referendum number: 816.
tip

Copy link

The referendum has appeared on Polkassembly.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 30, 2024 02:52
…fer to minimum balance instead of existential depost
@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 30, 2024 21:31
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Sorry that I missed the ping @pandres95

Comment on lines 14 to 17
- 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

substrate/frame/assets/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/assets/src/tests.rs Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Copy link
Contributor

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.

prdoc/pr_4527.prdoc Outdated Show resolved Hide resolved
@pandres95 pandres95 requested a review from muharem June 28, 2024 16:21
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.
Copy link
Contributor

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.

substrate/frame/assets/src/benchmarking.rs Outdated Show resolved Hide resolved
@pandres95 pandres95 requested a review from muharem July 2, 2024 18:17
@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jul 3, 2024
@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/2024-07-16-technical-fellowship-opendev-call/9512/1

@joepetrowski
Copy link
Contributor

@pandres95 is this waiting for anything before merging?

@pandres95
Copy link
Contributor Author

@joepetrowski not that I know. I thought it was waiting for an additional review / auditing process. 😅

@joepetrowski joepetrowski added this pull request to the merge queue Aug 7, 2024
Merged via the queue into paritytech:master with commit 6db2038 Aug 7, 2024
158 of 161 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Audited
Development

Successfully merging this pull request may close these issues.

transfer_all Call in Assets Pallet
7 participants