Add Bank.getAllHolders(denom) grpc/api

[ghost]

Summary

It would be nice to retrieve a list of holders for a particular denom.

Problem Definition

We are currently doing this very specifically for our marker module, but it would be very helpful to have this available on a regular denom as well. Pass in a denom name, and spit out a paginated list of account holders with balances.

Proposal

Following this example:

func (k Keeper) GetAllMarkerHolders(ctx sdk.Context, denom string) []types.Balance {	
	var results []types.Balance	
        k.bankKeeper.IterateAllBalances(ctx, func(addr sdk.AccAddress, coin sdk.Coin) (stop bool) {
		if coin.Denom == denom && !coin.Amount.IsZero() {
			results = append(results,
				types.Balance{
					Address: addr.String(),
					Coins:   sdk.NewCoins(coin),
				})
		}
		return false // do not stop iterating
	})
	return results
}

add somethings similar within the bank module.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

Would you be willing to open a PR for this? For performance it would need to be paginated

[ghost]

Would you be willing to open a PR for this? For performance it would need to be paginated

@aaronc I appreciate the confidence. Unfortunately I am just acting as a user of these APIs at this time. I don't feel comfortable enough to make this change.

[alexanderbez]

@aaronc can you point me to docs or resources on how to actually add a gRPC query please?

[aaronc]

@aaronc can you point me to docs or resources on how to actually add a gRPC query please?

This is what we've got: https://docs.cosmos.network/v0.42/building-modules/query-services.html#implementation-of-a-module-query-service. I think it could be improved for sure (cc @ryanchristo). You can see examples for other query methods in bank.

[aaronc]

Is it worth creating a denom - address reverse index for this? (i.e. concat(denom, address) as the key). An on-chain use case could be distributing some coins proportionally to all owners of a denom (basically an on-chain air drop).

[alexanderbez]

Yes, I think that idea makes sense. Then we could just prefix scan on denom instead of all denominations, right?

[aaronc]

Yes, I think that idea makes sense. Then we could just prefix scan on denom instead of all denominations, right?

Exactly, it is extra storage and gas to make that work, but it would be kind of nice. We probably don't need to store the actually balance under that reverse key so it only needs to be set once per denom/address pair and then deleted when the balance goes to zero. If we do it, it will need a migration.

[alexanderbez]

If we do it, it will need a migration.

Why would this need a migration and how would that look like? Is this worth the change?

[amaury1093]

how would that look like?

See example here for 0.42->0.43 store migrations.

Is this worth the change?

For now #9533 is non-breaking, so let's merge that into 0.43. The denom++address index is a nice-to-have for me, we can create a separate issue for it, and include it in 0.44 (with a 0.43->0.44 store migration).

[aaronc]

For now #9533 is non-breaking, so let's merge that into 0.43. The denom++address index is a nice-to-have for me, we can create a separate issue for it, and include it in 0.44 (with a 0.43->0.44 store migration).

Could you create an issue for that @AmauryM ? I'd like to put it on the roadmap for the next release. It's also desirable if bank is used for storing NFTs.

[amaury1093]

#9590