Return account's asset balances #13352

jsidorenko · 2023-02-09T16:45:41Z

This method solves the problem of fetching the account's asset balances, now instead of sending 100+ RPC requests it would be possible to send just one.

Cumulus Companion: paritytech/cumulus#2180

frame/assets/src/functions.rs

frame/assets/runtime-api/src/lib.rs

bkchr · 2023-02-09T22:21:49Z

Do we really need this here in Substrate? I mean the runtime api could also just be declared for Statemint/Statemine. It could also there directly have concrete namings like asset_x_account_balances, asset_y_account_balances.

gilescope · 2023-02-10T08:15:10Z

Cross-chain consistency of getting asset balances is better than bespoke naming. I'd suggest that being able to get the balances of an account is something that should come as standard.

bkchr · 2023-02-10T09:00:26Z

Cross-chain consistency of getting asset balances is better than bespoke naming. I'd suggest that being able to get the balances of an account is something that should come as standard.

While I agree with this. The current implementation here with a instance: u8 is also not really some standard. What is the instance? The index of the pallet in the runtime? Or just the the order of the asset pallets on how they appear in the in construct runtime? This isn't really defined and also not a "standard".

In the future when we have https://forum.polkadot.network/t/wasm-view-functions/1045 we can have a standard.

I will approve the pr for now, but I request some better docs on how "instance" is defined.

frame/assets/src/functions.rs

frame/assets/runtime-api/src/lib.rs

xlc

The fact this assumes a chain can have multiple kinds of asset id is just super confusing and not making sense.

Yes a chain can have multiple pallet-assets instance with different AssetId type for each instance. However the chain SHOULD still have a single unified asset id. It could be

enum AssetId {
  LocalAssets(u32),
  ForeignAssets(MultiLocation),
}

and having one pallet-assets handle local assets and another one handle foreign assets. But the API / UI / UX should never require user to specify an instance ID AND an asset id.

bin/node/runtime/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

jsidorenko · 2023-02-10T15:19:56Z

I removed the instance param as it causes too much confusion

jsidorenko · 2023-02-10T15:26:11Z

The fact this assumes a chain can have multiple kinds of asset id is just super confusing and not making sense.

Yes a chain can have multiple pallet-assets instance with different AssetId type for each instance. However the chain SHOULD still have a single unified asset id. It could be
enum AssetId {
  LocalAssets(u32),
  ForeignAssets(MultiLocation),
}
and having one pallet-assets handle local assets and another one handle foreign assets. But the API / UI / UX should never require user to specify an instance ID AND an asset id.

What would be the proper solution then?

xlc · 2023-02-11T22:17:52Z

Acala for example, we have pallet-balances, orml-tokens, and ERC20 from EVM and we have a single CurrencyId for all those.

https://github.com/AcalaNetwork/Acala/blob/01f19ae1bfb0afff62a0b938c8e2d02cf5d831c5/primitives/src/currency.rs#L466

xlc · 2023-02-11T22:20:50Z

But this is kind of a bad idea anyway. It could be very very expensive to iterate all the assets. This could be a DoS vector to the node. Take a look at Ethereum, how many ERC20 are there and how are you going to iterate every ERC20 on Ethereum to return a balance list?

jsidorenko · 2023-02-13T09:18:59Z

@xlc In Ethereum, there are balance contracts those accept an array of contracts to check and return you up to 1k balances at once

kianenigma · 2023-02-21T12:33:42Z

I am happy to remove my review, but I'd like the PR authors to also study the links that I have provided and express if they agree that the correct solution is to not only solve this at the level of assets pallet, but at the XCM layer.

Currently, an account can have "value bearing assets" in balances/assets pallet, in a DEX pallet, in some liquid staking pool pallet, in a lending platform. A wallet in our ecosystem has no meaningful way to scrape all of this information as it stands now, unless if they go on and study each parachain/platform one by one.

The XCM-centric approach would solve all of this in one go, and moves the responsibility of implementing this API to each individual chain, which is where it should logically reside.

Polkadot-Forum · 2023-02-21T12:34:56Z

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

https://forum.polkadot.network/t/xcm-as-a-standard-for-reading-and-interacting-with-parachains/266/5

PR can go ahead.

kianenigma · 2023-02-21T12:59:09Z

Okay, sorry, but I am still not onboard. What about a max_iter: Option<u32> or something?

kianenigma · 2023-02-21T12:59:46Z

frame/assets/src/functions.rs

@@ -924,4 +924,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Ok(())
 		})
 	}
+
+	/// Returns all the non-zero balances for all assets of the given `account`.
+	pub fn account_balances(account: T::AccountId) -> Vec<(T::AssetId, T::Balance)> {


Suggested change

pub fn account_balances(account: T::AccountId) -> Vec<(T::AssetId, T::Balance)> {

pub fn account_balances(account: T::AccountId, max_iters: Option<u32>) -> Vec<(T::AssetId, T::Balance)> {

Then the node implementing this can decide up to how many iterations it wants to support.

The node does not augments calls to the runtime. So, what ever calls this would need to set this value. But I'm fine with the idea in general

I don’t think this approach will work in general. This will need to have proper paging support to be exhaustive and this kind of API is simply impossible for ERC20 tokens implementation.

kianenigma · 2023-02-21T13:00:07Z

frame/assets/src/functions.rs

+
+	/// Returns all the non-zero balances for all assets of the given `account`.
+	pub fn account_balances(account: T::AccountId) -> Vec<(T::AssetId, T::Balance)> {
+		Asset::<T, I>::iter_keys()


Suggested change

Asset::<T, I>::iter_keys()

Asset::<T, I>::iter_keys().take(max_iters.unwrap_or(u32::MAX))

kianenigma · 2023-02-21T13:01:16Z

bin/node/runtime/src/assets_api.rs

+use codec::Codec;
+use sp_std::vec::Vec;
+
+sp_api::decl_runtime_apis! {


Why is this re-defined in every single runtime in Cumulus?

there is just one common definition for all statemine/t runtimes (statemine/statemint/westmint),
but lets discuss it here: paritytech/cumulus#2180 (comment)

shawntabrizi · 2023-02-21T15:21:03Z

I am not against this PR getting in if it solves some specific problem we need tomorrow, but yeah, the XCM route seems like the right approach. We should instead have a new XCM Query RPC, and one of the XCM queries should be exactly like this, and implementable by runtime developers to work against any pallets.

mustermeiszer · 2023-02-22T16:59:15Z

I am not against this PR getting in if it solves some specific problem we need tomorrow, but yeah, the XCM route seems like the right approach. We should instead have a new XCM Query RPC, and one of the XCM queries should be exactly like this, and implementable by runtime developers to work against any pallets.

What is the advantage of having XCM-query-rpc over normal query-rpc?

Would be great if one of you could draft how this would look like.

Is every query a single XCM instruction and must live and be accepted by the Polkdot repo?
Is there a generic query -- like Transact -- and chains can define their own queries?
How is the query unified/interface-like so that it does not depend on chain knowledge -- the Call enum of the chains is quite a problem for unification already?

bkontur · 2023-02-22T17:21:02Z

I am not against this PR getting in if it solves some specific problem we need tomorrow, but yeah, the XCM route seems like the right approach. We should instead have a new XCM Query RPC, and one of the XCM queries should be exactly like this, and implementable by runtime developers to work against any pallets.

@shawntabrizi
yes, please, I also would like to know more about "XCM Query RPC", what exactly does that mean?
Does it mean something like "expose XCM-dedicated runtime api", e.g.: https://github.com/paritytech/cumulus/pull/2180/files#diff-4bcbbb9c6ce00315b062b0f075d0fb9f452e3091420b5d2234b092385c09b716R56-R64 ?

bkontur · 2023-02-23T08:22:24Z

bot rebase

paritytech-processbot · 2023-02-23T08:22:32Z

Rebased

bkontur · 2023-02-27T11:54:19Z

bot rebase

paritytech-processbot · 2023-02-27T11:54:26Z

Rebased

jsidorenko · 2023-03-02T11:31:31Z

bot rebase

paritytech-processbot · 2023-03-02T11:31:39Z

Rebased

jsidorenko · 2023-03-02T13:58:32Z

bot merge

* Runtime method to get user's assets balances * Fix test (typo) * Update frame/assets/src/functions.rs * Remove instance param * Update frame/assets/src/functions.rs Co-authored-by: Bastian Köcher <[email protected]> * Remove instance param * Refactor * Chore * Update doc --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: parity-processbot <>

Polkadot-Forum · 2023-03-30T10:12:53Z

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

* Runtime method to get user's assets balances * Fix test (typo) * Update frame/assets/src/functions.rs * Remove instance param * Update frame/assets/src/functions.rs Co-authored-by: Bastian Köcher <[email protected]> * Remove instance param * Refactor * Chore * Update doc --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: parity-processbot <>

Runtime method to get user's assets balances

20eb6d4

jsidorenko added A0-please_review Pull request needs code review. B7-runtimenoteworthy 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 9, 2023

jsidorenko requested review from gilescope, bkchr and joepetrowski February 9, 2023 16:45

Fix test (typo)

2465123

gilescope reviewed Feb 9, 2023

View reviewed changes

frame/assets/src/functions.rs Outdated Show resolved Hide resolved

jsidorenko and others added 2 commits February 9, 2023 19:00

Update frame/assets/src/functions.rs

bbe9efb

Remove instance param

45d7303

gilescope reviewed Feb 9, 2023

View reviewed changes

frame/assets/runtime-api/src/lib.rs Outdated Show resolved Hide resolved

gilescope approved these changes Feb 9, 2023

View reviewed changes

bkchr approved these changes Feb 10, 2023

View reviewed changes

frame/assets/src/functions.rs Outdated Show resolved Hide resolved

frame/assets/runtime-api/src/lib.rs Outdated Show resolved Hide resolved

xlc suggested changes Feb 10, 2023

View reviewed changes

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved

jsidorenko and others added 2 commits February 10, 2023 16:59

Update frame/assets/src/functions.rs

14a9cd2

Co-authored-by: Bastian Köcher <[email protected]>

Remove instance param

668838d

the-right-joyce added B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. and removed B7-runtimenoteworthy labels Feb 13, 2023

Refactor

e7a3271

kianenigma reviewed Feb 21, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into js/assets-balances

1a2ed63

Merge remote-tracking branch 'origin/master' into js/assets-balances

1689507

Merge remote-tracking branch 'origin/master' into js/assets-balances

ea653a3

paritytech-processbot bot merged commit d1d67cd into master Mar 2, 2023

paritytech-processbot bot deleted the js/assets-balances branch March 2, 2023 13:58

kiltbot mentioned this pull request Apr 3, 2023

3️⃣ [AUTOMATIC] Update Polkadot dependencies from 0.9.39 to 0.9.40 KILTprotocol/kilt-node#496

Closed

librelois mentioned this pull request Apr 6, 2023

Update substrate/polkadot/cumulus from v0.9.38 to v0.9.40 moonbeam-foundation/moonbeam#2200

Closed

hbulgarini mentioned this pull request May 29, 2023

[0.9.40] Add query_account_balances runtime api for pallet assets. paritytech/trappist#199

Closed

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return account's asset balances #13352

Return account's asset balances #13352

jsidorenko commented Feb 9, 2023 •

edited

Loading

bkchr commented Feb 9, 2023

gilescope commented Feb 10, 2023

bkchr commented Feb 10, 2023

xlc left a comment

jsidorenko commented Feb 10, 2023

jsidorenko commented Feb 10, 2023

xlc commented Feb 11, 2023

xlc commented Feb 11, 2023

jsidorenko commented Feb 13, 2023

kianenigma commented Feb 21, 2023

Polkadot-Forum commented Feb 21, 2023

kianenigma commented Feb 21, 2023

kianenigma Feb 21, 2023

kianenigma Feb 21, 2023

bkchr Feb 21, 2023

xlc Feb 21, 2023

kianenigma Feb 21, 2023

kianenigma Feb 21, 2023 •

edited

Loading

bkontur Feb 21, 2023

shawntabrizi commented Feb 21, 2023

mustermeiszer commented Feb 22, 2023 •

edited

Loading

bkontur commented Feb 22, 2023

bkontur commented Feb 23, 2023

paritytech-processbot bot commented Feb 23, 2023

bkontur commented Feb 27, 2023

paritytech-processbot bot commented Feb 27, 2023

jsidorenko commented Mar 2, 2023

paritytech-processbot bot commented Mar 2, 2023

jsidorenko commented Mar 2, 2023

Polkadot-Forum commented Mar 30, 2023

	pub fn account_balances(account: T::AccountId) -> Vec<(T::AssetId, T::Balance)> {
	pub fn account_balances(account: T::AccountId, max_iters: Option<u32>) -> Vec<(T::AssetId, T::Balance)> {

	Asset::<T, I>::iter_keys()
	Asset::<T, I>::iter_keys().take(max_iters.unwrap_or(u32::MAX))

Return account's asset balances #13352

Return account's asset balances #13352

Conversation

jsidorenko commented Feb 9, 2023 • edited Loading

bkchr commented Feb 9, 2023

gilescope commented Feb 10, 2023

bkchr commented Feb 10, 2023

xlc left a comment

Choose a reason for hiding this comment

jsidorenko commented Feb 10, 2023

jsidorenko commented Feb 10, 2023

xlc commented Feb 11, 2023

xlc commented Feb 11, 2023

jsidorenko commented Feb 13, 2023

kianenigma commented Feb 21, 2023

Polkadot-Forum commented Feb 21, 2023

kianenigma commented Feb 21, 2023

kianenigma Feb 21, 2023

Choose a reason for hiding this comment

kianenigma Feb 21, 2023

Choose a reason for hiding this comment

bkchr Feb 21, 2023

Choose a reason for hiding this comment

xlc Feb 21, 2023

Choose a reason for hiding this comment

kianenigma Feb 21, 2023

Choose a reason for hiding this comment

kianenigma Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

bkontur Feb 21, 2023

Choose a reason for hiding this comment

shawntabrizi commented Feb 21, 2023

mustermeiszer commented Feb 22, 2023 • edited Loading

bkontur commented Feb 22, 2023

bkontur commented Feb 23, 2023

paritytech-processbot bot commented Feb 23, 2023

bkontur commented Feb 27, 2023

paritytech-processbot bot commented Feb 27, 2023

jsidorenko commented Mar 2, 2023

paritytech-processbot bot commented Mar 2, 2023

jsidorenko commented Mar 2, 2023

Polkadot-Forum commented Mar 30, 2023

jsidorenko commented Feb 9, 2023 •

edited

Loading

kianenigma Feb 21, 2023 •

edited

Loading

mustermeiszer commented Feb 22, 2023 •

edited

Loading