Skip to content

xcm: avoid subtracting delivery fees twice#7201

Merged
EgorPopelyaev merged 6 commits into
paritytech:stable2409from
carlosala:fix/reserve-jit-withdraw
Jan 23, 2025
Merged

xcm: avoid subtracting delivery fees twice#7201
EgorPopelyaev merged 6 commits into
paritytech:stable2409from
carlosala:fix/reserve-jit-withdraw

Conversation

@carlosala

Copy link
Copy Markdown
Contributor

At the moment, the delivery fees are being subtracted twice if the jit_withdraw mode is enabled, resulting in assets being trapped. This is an example of it.
This does not happen in 2412 anymore after #4834.

@carlosala carlosala requested a review from a team as a code owner January 16, 2025 11:20
@carlosala

Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience runtime_dev --bump patch

@github-actions

Copy link
Copy Markdown
Contributor

Sorry, only members of the organization paritytech members can run commands.

@bkchr

bkchr commented Jan 16, 2025

Copy link
Copy Markdown
Member

CC @franciscoaguirre

@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Jan 16, 2025
@bkchr

bkchr commented Jan 16, 2025

Copy link
Copy Markdown
Member

@carlosala could we maybe get some test for this? :D (Not sure how the test infrastructure looks right now)

@carlosala

Copy link
Copy Markdown
Contributor Author

I'm not really sure either, what do you think @franciscoaguirre?

acatangiu
acatangiu previously approved these changes Jan 17, 2025

@acatangiu acatangiu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAIK there is no test verifying the right delivery fee being charged - aka there are tests verifying fees are charged but not asserting on amounts.

But there are already many tests verifying that DepositReserveAsset is working under various conditions, so any breakage would be caught. CI should be green for this PR.

@franciscoaguirre

Copy link
Copy Markdown
Contributor

/cmd fmt

@github-actions

Copy link
Copy Markdown
Contributor

Command "fmt" has started 🚀 See logs here

@github-actions

Copy link
Copy Markdown
Contributor

Command "fmt" has failed ❌! See logs here

@franciscoaguirre

Copy link
Copy Markdown
Contributor

/cmd fmt

@github-actions

Copy link
Copy Markdown
Contributor

Command "fmt" has started 🚀 See logs here

@github-actions

Copy link
Copy Markdown
Contributor

Command "fmt" has failed ❌! See logs here

@franciscoaguirre

Copy link
Copy Markdown
Contributor

bot fmt

@command-bot

command-bot Bot commented Jan 20, 2025

Copy link
Copy Markdown

@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8058462 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-c6e70445-eda5-4839-b604-4dc2569a4536 to cancel this command or bot cancel to cancel all commands in this pull request.

@github-actions

Copy link
Copy Markdown
Contributor

We have migrated the command bot to GHA

Please, see the new usage instructions here or here. Soon the old commands will be disabled.

@command-bot

command-bot Bot commented Jan 20, 2025

Copy link
Copy Markdown

@franciscoaguirre Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8058462 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8058462/artifacts/download.

@paritytech-review-bot paritytech-review-bot Bot requested a review from a team January 20, 2025 12:58

@acatangiu acatangiu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The following 7 benchmarks failed:

  • pallet_xcm::force_subscribe_version_notify
  • pallet_xcm::force_unsubscribe_version_notify
  • pallet_xcm::send
  • pallet_xcm::transfer_assets
  • pallet_xcm_benchmarks::fungible::deposit_asset
  • pallet_xcm_benchmarks::fungible::deposit_reserve_asset
  • pallet_xcm_benchmarks::fungible::initiate_teleport

Please verify that these tests are passing for you locally (cargo test -p pallet-xcm --features=runtime-benchmarks)

Also please verify XCM emulated tests are passing locally:

cargo test -r -p asset-hub-westend-integration-tests -p bridge-hub-westend-integration-tests

@athei

athei commented Jan 20, 2025

Copy link
Copy Markdown
Member

Your editor seems to be reformatting unrelated files. Please re-run fmt (using the bot if you want) and fix your editor.

@carlosala

Copy link
Copy Markdown
Contributor Author

The unrelated files were changes by the bot, after bot fmt run by @franciscoaguirre

@athei

athei commented Jan 20, 2025

Copy link
Copy Markdown
Member

Okay not sure what is wrong with the bot. Maybe it is on an old version. We should not use it anymore I guess in favor of /cmd. But this seems to be broken in a different way. Sorry for insulting your editor :D

@carlosala

Copy link
Copy Markdown
Contributor Author

XCM emulated tests are passing as expected to me locally 👍🏻

Please verify that these tests are passing for you locally (cargo test -p pallet-xcm --features=runtime-benchmarks)

They are not compiling due to a compilation error in the staking pallet. I have the same compilation error on stable2409 branch 👍🏻

@franciscoaguirre franciscoaguirre enabled auto-merge (squash) January 22, 2025 13:53
@EgorPopelyaev EgorPopelyaev merged commit 4969952 into paritytech:stable2409 Jan 23, 2025
@Polkadot-Forum

Copy link
Copy Markdown

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

https://forum.polkadot.network/t/polkadot-api-2025-q1-update/11906/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T6-XCM This PR/Issue is related to XCM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants