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

Companion for #13352 #2180

Merged
merged 20 commits into from
Mar 2, 2023
Merged

Companion for #13352 #2180

merged 20 commits into from
Mar 2, 2023

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko added A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the 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 Feb 13, 2023
@paritytech-ci paritytech-ci requested review from a team February 13, 2023 11:09
@jsidorenko jsidorenko added the T1-runtime This PR/Issue is related to the topic “runtime”. label Feb 13, 2023
@bkontur
Copy link
Contributor

bkontur commented Feb 15, 2023

assets-common + AssetsApi refactor for multi pallet_assets instances is ready here: #2187

…nstances (#2187)

* AssetsApi with MultiLocation for Westmint + assets-common

* AssetsApi with MultiLocation for Statemine/t

* typo
@gilescope
Copy link
Contributor

There's definitely overlap with #2148 . I think 2148 should simplify some of this when it lands, but I don't think I have any objections to this PR as it stands. All roads seem to lead to AssetId being a multilocation.

@gilescope
Copy link
Contributor

gilescope commented Feb 15, 2023

Is it possible that AssetIdForTrustBackedAssetsConvert::reverse_ref(1) could be made infallible? If so that would make things a tad simpler.

@bkontur
Copy link
Contributor

bkontur commented Feb 15, 2023

There's definitely overlap with #2148 . I think 2148 should simplify some of this when it lands, but I don't think I have any objections to this PR as it stands. All roads seem to lead to AssetId being a multilocation.

yes, agree, but maybe these two should be merged before #2148:
#2133
#2167

at least at #2167 - I have tests and working AssetTransactor with multiple instance:

pub type AssetTransactors = (CurrencyTransactor, FungiblesTransactor, ForeignFungiblesTransactor);

because there is a issue with ConvertedConcreteId, which I need to fix better: https://github.com/paritytech/cumulus/pull/2167/files#diff-aa9ec9164c4412c613444721bada9490dd697084f621658cba0a8db2ae360a13R19-R33

@bkontur
Copy link
Contributor

bkontur commented Feb 15, 2023

Is it possible that AssetIdForTrustBackedAssetsConvert::reverse_ref(1) could be made infallible? If so that would make things a tad simpler.

yes, it is not possible: https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/traits/conversion.rs#L32-L47,
I wanted to reuse the existing conversions AsPrefixedGeneralIndex - to use the same conversion everywhere,
thats why I needed to wrap that Vec<()> in Result

@bkontur
Copy link
Contributor

bkontur commented Feb 21, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-ci paritytech-ci requested a review from a team February 21, 2023 13:01
@bkontur
Copy link
Contributor

bkontur commented Feb 23, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor

bkontur commented Feb 23, 2023

Some context:
iiuc, guys (working on DEX + UI) wanted to achieve with this companion and paritytech/substrate#13352 : read account balances from pallet_assets from statemine/t, I just try to help refactor here some common stuff, to be better prepared for multi pallet_assets instance (ForeignAssets).

Actual status of this companion:


@kianenigma suggested here that it should be new api, xcm-based, and should be able to collect all MultiAssets from runtime (actually this new AssetsApi does exactly that)

@shawntabrizi mentioned here some "XCM Query RPC", so if we just move this AssetsApito the polkadot xcm module somewhere, this could be a starting point and all parachains could use it.

yes, it is missing:

  • pagination
  • maybe some filter
  • max_iters: Option<u32>

@KiChjang mentioned that there will be "XCM fungibles retreat ", so this is pretty good topic for that


@joepetrowski @kianenigma @shawntabrizi
so guys, please, somebody should tell what to do next:

@joepetrowski
Copy link
Contributor

@joepetrowski @kianenigma @shawntabrizi
so guys, please, somebody should tell what to do next:

I have discussed a bit with @kianenigma - the tentative plan now is that we merge these runtime RPCs as a short-term solution for better UI/UX development. But there will be someone joining soon who can work full time on a more holistic runtime interface for the long term. So I think we can proceed with this PR (but still need to address the above comment about making this part of Polkadot-XCM).

@bkontur
Copy link
Contributor

bkontur commented Feb 24, 2023

@joepetrowski @kianenigma @shawntabrizi
so guys, please, somebody should tell what to do next:

I have discussed a bit with @kianenigma - the tentative plan now is that we merge these runtime RPCs as a short-term solution for better UI/UX development. But there will be someone joining soon who can work full time on a more holistic runtime interface for the long term. So I think we can proceed with this PR (but still need to address the above comment about making this part of Polkadot-XCM).

ok, cool, thank you,
I moved AssetsApi (renamed to FungiblesApi) to xcm-builder here: paritytech/polkadot#6777,
once this PR is merged, I will change this companion to use it

@bkchr
Copy link
Member

bkchr commented Feb 26, 2023

Please keep the runtime api here in this repository. It makes no sense to have them in Polkadot, especially if this will be some intermediate solution. People are also completely free to just copy this code, this isn't really complicated and there is also not that much code attached to this.

@bkontur
Copy link
Contributor

bkontur commented Feb 26, 2023

Please keep the runtime api here in this repository. It makes no sense to have them in Polkadot, especially if this will be some intermediate solution. People are also completely free to just copy this code, this isn't really complicated and there is also not that much code attached to this.

thank you, makes sense,
I refactored that runtime_api.rs definition, now is clean and the same as file from polkadot/xcm PR

if we get one more approve here, I can close that polkadot/xcm PR (unless anybody really wants to put it to the polkadot/xcm)

parachains/runtimes/assets/common/Cargo.toml Outdated Show resolved Hide resolved
@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 5854708 into master Mar 2, 2023
@paritytech-processbot paritytech-processbot bot deleted the js/assets-balances branch March 2, 2023 14:37
@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-40/2468/1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the 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 T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants