Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) #14572

bkontur · 2023-07-13T14:28:09Z

PR contains:

main fix for get_pool_id (Ord does not count with is_native flag) and consequnce is that e.g. on system parachain, we cannot use MultiLocation::parent() as a native asset, because MultiLocation { parents: 1, ..} > MultiLocation { parents: 0, ..} (parents: 0 is some local asset on system parachain)
improved MultiAssetIdConverter handling (e.g. adds ability for runtime to filter supported assets and so on)
other small nits
removed NativeOrAssetId/NativeOrAssetIdConverter dedicated for tests/mocks from types.rs

Cumulus companion: paritytech/cumulus#2860

…ive` flag)

…lper`

bkontur · 2023-07-14T15:40:19Z

bot help

bkontur · 2023-07-14T15:41:10Z

bot bench $ pallet dev pallet_asset_conversion

…rsion

…sset-conversion

frame/asset-conversion/src/benchmarking.rs

jsidorenko · 2023-07-17T11:11:05Z

LGTM, but I would probably avoid having 3 equal versions of the NativeOrAssetId

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

bkontur · 2023-07-17T11:33:20Z

LGTM, but I would probably avoid having 3 equal versions of the NativeOrAssetId

well, yes, I dont like it either, but I also I am not sure if it fits to types.rs,
I thought that it is dedicated for mocks/tests or as sample,
for system parachains we have custom implementation and I am not sure about other parachains if it fits for them

bkontur · 2023-07-18T20:25:45Z

bot rebase

bkontur · 2023-07-18T20:26:05Z

bot bench $ pallet dev pallet_asset_conversion

…rsion

joepetrowski

Some documentation errors but otherwise OK.

frame/asset-conversion/src/lib.rs

frame/asset-conversion/src/types.rs

frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs

frame/asset-conversion/src/mock.rs

bin/node/runtime/src/impls.rs

frame/asset-conversion/src/lib.rs

bin/node/runtime/src/impls.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

bkontur · 2023-07-19T06:48:08Z

bot fmt

bkontur · 2023-07-19T06:49:06Z

bot fmt

bkontur · 2023-07-19T07:51:47Z

bot clean

gilescope

Looks good. We could push the common code out into a crate under bin/node and have the three locations depend on that?

bkontur · 2023-07-21T11:06:07Z

Looks good. We could push the common code out into a crate under bin/node and have the three locations depend on that?

couldnt this cause some circrular dependecy?

bin/node/runtime -> common code
bin/node/runtime has deps pallet-asset-conversion, pallet-asset-conversion-tx-payment
pallet-asset-conversion-tx-payment -> need that common code from bin/node/runtime
pallet-asset-conversion -> need that common code from bin/node/runtime

or what would be the best place for this common code NativeOrAssetId/NativeOrAssetIdConverter?

@gilescope @jsidorenko
for simplicity, I can revert that, and return NativeOrAssetId/NativeOrAssetIdConverter to types.rs with some comment, but I think that anybody who will use this pallet will provide their custom implementation, I dont know
so please both:
thumbs up - leave as it is
thumbs down - I revert that back

gilescope · 2023-07-21T12:22:02Z

LGTM

bkontur · 2023-07-21T13:06:24Z

bot merge

…ive` flag) (#14572) * Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) * Removed unnecessery clones + added `pool_account` to `PoolCreated` event * Fix bench compile * Fix bench * Improved `MultiAssetIdConverter::try_convert` * Removed `into_multiasset_id` from converter and moved to `BenchmarkHelper` * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_asset_conversion * Fixed doc * Typo * Removed `NativeOrAssetId` (test/mock) impl from types.rs to mock.rs... * Typo + 0u32 -> 0 * Update frame/asset-conversion/src/benchmarking.rs Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> * Typo * ".git/.scripts/commands/fmt/fmt.sh" * Fix from Jegor * Try to fix the other failing benchmark * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_asset_conversion * Update frame/asset-conversion/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/asset-conversion/src/types.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/asset-conversion/src/mock.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update bin/node/runtime/src/impls.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/asset-conversion/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update bin/node/runtime/src/impls.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Reverted NativeOrAssetId --------- Co-authored-by: command-bot <> Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> Co-authored-by: Jegor Sidorenko <jegor@parity.io> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Asset conversion get_pool_id fix (Ord does not count with `is_nat…

4c3eade

…ive` flag)

bkontur added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 13, 2023

bkontur added 7 commits July 14, 2023 10:14

Removed unnecessery clones + added pool_account to PoolCreated event

50e0d10

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

eca50d1

Fix bench compile

8d159dc

Fix bench

2d1ef6b

Improved MultiAssetIdConverter::try_convert

aa1f784

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

f9ab61a

Removed into_multiasset_id from converter and moved to `BenchmarkHe…

8b4154a

…lper`

command-bot and others added 6 commits July 14, 2023 16:13

".git/.scripts/commands/bench/bench.sh" pallet dev pallet_asset_conve…

bf479cc

…rsion

Fixed doc

edc60f8

Typo

67bb71e

Removed NativeOrAssetId (test/mock) impl from types.rs to mock.rs...

3ffc6a3

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

3bf7e18

Merge remote-tracking branch 'origin/bko-asset-conversion' into bko-a…

76762c7

…sset-conversion

bkontur mentioned this pull request Jul 15, 2023

Asset conversion nits paritytech/cumulus#2860

Merged

4 tasks

bkontur marked this pull request as ready for review July 15, 2023 00:10

bkontur requested review from a team July 15, 2023 00:10

bkontur mentioned this pull request Jul 15, 2023

[asset-conversion] Fix benchmarks swap_exact_tokens_for_tokens / swap_tokens_for_exact_tokens paritytech/cumulus#2874

Closed

jsidorenko reviewed Jul 17, 2023

View reviewed changes

frame/asset-conversion/src/benchmarking.rs Outdated Show resolved Hide resolved

Typo + 0u32 -> 0

bdc9e1c

jsidorenko reviewed Jul 17, 2023

View reviewed changes

frame/asset-conversion/src/benchmarking.rs Outdated Show resolved Hide resolved

Update frame/asset-conversion/src/benchmarking.rs

71a5378

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

command-bot and others added 4 commits July 17, 2023 14:43

".git/.scripts/commands/fmt/fmt.sh"

7f5c653

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

01d8c10

Fix from Jegor

0ae654e

Try to fix the other failing benchmark

d9893de

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

84ff367

".git/.scripts/commands/bench/bench.sh" pallet dev pallet_asset_conve…

de49e45

…rsion

joepetrowski approved these changes Jul 19, 2023

View reviewed changes

bkontur and others added 7 commits July 19, 2023 08:46

Update frame/asset-conversion/src/lib.rs

45246c3

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/asset-conversion/src/types.rs

d9937a7

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs

44d52b2

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/asset-conversion/src/mock.rs

a3a9c78

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update bin/node/runtime/src/impls.rs

05d7500

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/asset-conversion/src/lib.rs

1741302

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update bin/node/runtime/src/impls.rs

00255e7

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

command-bot bot deleted a comment from paritytech-processbot bot Jul 19, 2023

gilescope approved these changes Jul 20, 2023

View reviewed changes

bkontur added 2 commits July 21, 2023 13:43

Merge remote-tracking branch 'origin/master' into bko-asset-conversion

7356c08

Reverted NativeOrAssetId

f1f6302

jsidorenko approved these changes Jul 21, 2023

View reviewed changes

paritytech-processbot bot merged commit e076bda into master Jul 21, 2023
45 checks passed

paritytech-processbot bot deleted the bko-asset-conversion branch July 21, 2023 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) #14572

Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) #14572

bkontur commented Jul 13, 2023 •

edited

bkontur commented Jul 14, 2023

bkontur commented Jul 14, 2023

jsidorenko commented Jul 17, 2023

bkontur commented Jul 17, 2023

bkontur commented Jul 18, 2023

bkontur commented Jul 18, 2023

joepetrowski left a comment

bkontur commented Jul 19, 2023

bkontur commented Jul 19, 2023

bkontur commented Jul 19, 2023

gilescope left a comment

bkontur commented Jul 21, 2023 •

edited

gilescope commented Jul 21, 2023

bkontur commented Jul 21, 2023

Asset conversion get_pool_id fix (Ord does not count with is_native flag) #14572

Asset conversion get_pool_id fix (Ord does not count with is_native flag) #14572

Conversation

bkontur commented Jul 13, 2023 • edited

bkontur commented Jul 14, 2023

bkontur commented Jul 14, 2023

jsidorenko commented Jul 17, 2023

bkontur commented Jul 17, 2023

bkontur commented Jul 18, 2023

bkontur commented Jul 18, 2023

joepetrowski left a comment

Choose a reason for hiding this comment

bkontur commented Jul 19, 2023

bkontur commented Jul 19, 2023

bkontur commented Jul 19, 2023

gilescope left a comment

Choose a reason for hiding this comment

bkontur commented Jul 21, 2023 • edited

gilescope commented Jul 21, 2023

bkontur commented Jul 21, 2023

Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) #14572

Asset conversion `get_pool_id` fix (`Ord` does not count with `is_native` flag) #14572

bkontur commented Jul 13, 2023 •

edited

bkontur commented Jul 21, 2023 •

edited