Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

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

Merged
paritytech-processbot[bot] merged 32 commits intomasterfrom
bko-asset-conversion
Jul 21, 2023
Merged

Asset conversion get_pool_id fix (Ord does not count with is_native flag)#14572
paritytech-processbot[bot] merged 32 commits intomasterfrom
bko-asset-conversion

Conversation

@bkontur
Copy link
Contributor

@bkontur bkontur commented Jul 13, 2023 •
edited
Loading

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

@bkontur 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
Copy link
Contributor Author

bkontur commented Jul 14, 2023

bot help

@bkontur
Copy link
Contributor Author

bkontur commented Jul 14, 2023

bot bench $ pallet dev pallet_asset_conversion

@bkontur bkontur mentioned this pull request Jul 15, 2023
Merged
4 tasks
@bkontur bkontur marked this pull request as ready for review July 15, 2023 00:10
@bkontur bkontur requested review from a team July 15, 2023 00:10
@bkontur bkontur mentioned this pull request Jul 15, 2023
Closed
jsidorenko
jsidorenko reviewed Jul 17, 2023
View reviewed changes
jsidorenko
jsidorenko reviewed Jul 17, 2023
View reviewed changes
@jsidorenko
Copy link
Contributor

jsidorenko commented Jul 17, 2023

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

@bkontur @jsidorenko
Update frame/asset-conversion/src/benchmarking.rs
71a5378 
Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>
@bkontur
Copy link
Contributor Author

bkontur commented Jul 17, 2023

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
Copy link
Contributor Author

bkontur commented Jul 18, 2023

bot rebase

@bkontur
Copy link
Contributor Author

bkontur commented Jul 18, 2023

bot bench $ pallet dev pallet_asset_conversion

joepetrowski
joepetrowski approved these changes Jul 19, 2023
View reviewed changes
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.

Some documentation errors but otherwise OK.

bkontur and others added 7 commits July 19, 2023 08:46
@bkontur @joepetrowski
Update frame/asset-conversion/src/lib.rs
45246c3 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update frame/asset-conversion/src/types.rs
d9937a7 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs
44d52b2 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update frame/asset-conversion/src/mock.rs
a3a9c78 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update bin/node/runtime/src/impls.rs
05d7500 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update frame/asset-conversion/src/lib.rs
1741302 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur @joepetrowski
Update bin/node/runtime/src/impls.rs
00255e7 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkontur
Copy link
Contributor Author

bkontur commented Jul 19, 2023

bot fmt

1 similar comment
@bkontur
Copy link
Contributor Author

bkontur commented Jul 19, 2023

bot fmt

@bkontur
Copy link
Contributor Author

bkontur commented Jul 19, 2023

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Jul 19, 2023
gilescope
gilescope approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

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

@bkontur
Copy link
Contributor Author

bkontur commented Jul 21, 2023 •
edited
Loading

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
Copy link
Contributor

gilescope commented Jul 21, 2023

LGTM

@bkontur
Copy link
Contributor Author

bkontur commented Jul 21, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e076bda into master Jul 21, 2023
@paritytech-processbot paritytech-processbot bot deleted the bko-asset-conversion branch July 21, 2023 13:06
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
@bkontur @jsidorenko
@joepetrowski @Ank4n
Asset conversion get_pool_id fix (Ord does not count with `is_nat…
c3d7615 
…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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@gilescope gilescope gilescope approved these changes

@jsidorenko jsidorenko jsidorenko approved these changes

@joepetrowski joepetrowski joepetrowski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bkontur @jsidorenko @gilescope @joepetrowski