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

Fix XcmPaymentApi::query_weight_to_asset_fee version conversion #6459

Merged
merged 17 commits into from
Nov 26, 2024

Conversation

franciscoaguirre
Copy link
Contributor

The query_weight_to_asset_fee function was trying to convert versions by using try_as, this function doesn't convert from a versioned to a concrete type.
This would cause all calls with a lower version to fail.

The correct function to use is the good old try_into.
Now those calls work :)

@franciscoaguirre franciscoaguirre requested a review from a team as a code owner November 12, 2024 20:11
@franciscoaguirre
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Nov 12, 2024

@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7748401 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 7-7b71fbec-42d5-4285-ae51-a1bb4e834e9f to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Nov 12, 2024

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

@bkontur bkontur assigned bkontur and unassigned bkontur Nov 13, 2024
Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

lgmt modulo @franciscoaguirre could you please check #6467 - it could simplify

@bkontur bkontur added A4-needs-backport Pull request must be backported to all maintained releases. T9-cumulus This PR/Issue is related to cumulus. T6-XCM This PR/Issue is related to XCM. labels Nov 13, 2024
@franciscoaguirre
Copy link
Contributor Author

/cmd fmt

Copy link

Command "fmt" has started 🚀 See logs here

Copy link

Command "fmt" has finished ✅ See logs here

@franciscoaguirre
Copy link
Contributor Author

@bkontur I deduplicated the test and extended it to call quote_price_tokens_for_exact_tokens. Please take a look again.

@franciscoaguirre
Copy link
Contributor Author

/cmd help

Copy link

Command "help" has started 🚀 See logs here

Copy link

Command "help" has failed ❌! See logs here

@franciscoaguirre
Copy link
Contributor Author

/cmd --help

Copy link

Command help:
usage: /cmd  [--help] [--quiet] [--clean] [--image IMAGE]
             {bench,fmt,update-ui,prdoc} ...

A command runner for polkadot-sdk repo

positional arguments:
  {bench,fmt,update-ui,prdoc}
                        a command to run
    bench               Runs benchmarks
    fmt                 Formats code (cargo +nightly-VERSION fmt) and configs
                        (taplo format)
    update-ui           Updates UI tests
    prdoc               Generates PR documentation

options:
  --help                help for help if you need some help
  --quiet               Won't print start/end/failed messages in PR
  --clean               Clean up the previous bot's & author's comments in PR
  --image IMAGE         Override docker image '--image
                        docker.io/paritytech/ci-unified:latest'

### Command 'bench'
usage: /cmd bench [-h] [--quiet] [--clean] [--image IMAGE]
                  [--runtime [{dev,westend,rococo,asset-hub-westend,asset-hub-rococo,bridge-hub-rococo,bridge-hub-westend,collectives-westend,contracts-rococo,coretime-rococo,coretime-westend,glutton-westend,people-rococo,people-westend} ...]]
                  [--pallet [PALLET ...]] [--fail-fast]

options:
  -h, --help            show this help message and exit
  --quiet               Won't print start/end/failed messages in PR
  --clean               Clean up the previous bot's & author's comments in PR
  --image IMAGE         Override docker image '--image
                        docker.io/paritytech/ci-unified:latest'
  --runtime [{dev,westend,rococo,asset-hub-westend,asset-hub-rococo,bridge-hub-rococo,bridge-hub-westend,collectives-westend,contracts-rococo,coretime-rococo,coretime-westend,glutton-westend,people-rococo,people-westend} ...]
                        Runtime(s) space separated
  --pallet [PALLET ...]
                        Pallet(s) space separated
  --fail-fast           Fail fast on first failed benchmark

**Examples**:
 Runs all benchmarks
 /cmd bench

 Runs benchmarks for pallet_balances and pallet_multisig for all runtimes which have these pallets. **--quiet** makes it to output nothing to PR but reactions
 /cmd bench --pallet pallet_balances pallet_xcm_benchmarks::generic --quiet

 Runs bench for all pallets for westend runtime and fails fast on first failed benchmark
 /cmd bench --runtime westend --fail-fast

 Does not output anything and cleans up the previous bot's & author command triggering comments in PR
 /cmd bench --runtime westend rococo --pallet pallet_balances pallet_multisig --quiet --clean


### Command 'fmt'
usage: /cmd fmt [-h] [--quiet] [--clean] [--image IMAGE]

options:
  -h, --help     show this help message and exit
  --quiet        Won't print start/end/failed messages in PR
  --clean        Clean up the previous bot's & author's comments in PR
  --image IMAGE  Override docker image '--image docker.io/paritytech/ci-
                 unified:latest'


### Command 'update-ui'
usage: /cmd update-ui [-h] [--quiet] [--clean] [--image IMAGE]

options:
  -h, --help     show this help message and exit
  --quiet        Won't print start/end/failed messages in PR
  --clean        Clean up the previous bot's & author's comments in PR
  --image IMAGE  Override docker image '--image docker.io/paritytech/ci-
                 unified:latest'


### Command 'prdoc'
usage: /cmd prdoc [-h] [--pr PR]
                  [--audience [{runtime_dev,runtime_user,node_dev,node_operator} ...]]
                  [--bump {patch,minor,major,silent,ignore,no_change}]
                  [--force]

options:
  -h, --help            show this help message and exit
  --pr PR               The PR number to generate the PrDoc for.
  --audience [{runtime_dev,runtime_user,node_dev,node_operator} ...]
                        The audience of whom the changes may concern. Example:
                        --audience runtime_dev node_dev
  --bump {patch,minor,major,silent,ignore,no_change}
                        A default bump level for all crates. Example: --bump
                        patch
  --force               Whether to overwrite any existing PrDoc.

@bkontur
Copy link
Contributor

bkontur commented Nov 18, 2024

@bkontur I deduplicated the test and extended it to call quote_price_tokens_for_exact_tokens. Please take a look again.

very nice, I am just thinking now, that we could/should split xcm_payment_api_works to two test-cases:

  • xcm_payment_api_with_native_token_works
  • xcm_payment_api_with_pools_works

or something like that, because not every runtime has the pools, e.g. coretime/people/relaychains...

@franciscoaguirre
Copy link
Contributor Author

Right now it's in assets/test-utils/. If I divide it in two I can put them in test-utils and use the pools test only in asset hubs

@franciscoaguirre
Copy link
Contributor Author

/cmd fmt

Copy link

Command "fmt" has started 🚀 See logs here

Copy link

Command "fmt" has finished ✅ See logs here

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12031120784
Failed job name: fmt

@x3c41a x3c41a self-requested a review November 26, 2024 13:50
Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't Cargo.lock supposed to be changed automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was changed automatically. All the added lines are because I changed dependencies in various crates' Cargo.tomls

@franciscoaguirre franciscoaguirre self-assigned this Nov 26, 2024
@franciscoaguirre franciscoaguirre added this pull request to the merge queue Nov 26, 2024
Merged via the queue into master with commit 139691b Nov 26, 2024
193 of 198 checks passed
@franciscoaguirre franciscoaguirre deleted the fix-xcm-payment-api-conversion branch November 26, 2024 20:16
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6459-to-stable2407
git worktree add --checkout .worktree/backport-6459-to-stable2407 backport-6459-to-stable2407
cd .worktree/backport-6459-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 139691b17c66aa074d2b4ae935158d4296068f72
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6459-to-stable2409
git worktree add --checkout .worktree/backport-6459-to-stable2409 backport-6459-to-stable2409
cd .worktree/backport-6459-to-stable2409
git reset --hard HEAD^
git cherry-pick -x 139691b17c66aa074d2b4ae935158d4296068f72
git push --force-with-lease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T6-XCM This PR/Issue is related to XCM. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants