Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UX: Provide endpoint to get full denom of account balance #1480

Closed
AdityaSripal opened this issue Jun 1, 2022 · 4 comments
Closed

UX: Provide endpoint to get full denom of account balance #1480

AdityaSripal opened this issue Jun 1, 2022 · 4 comments
Labels
client-UX needs discussion Issues that need discussion before they can be worked on

Comments

@AdityaSripal
Copy link
Member

Currently getting the account balance of an address will return the hashed IBC denoms. This hashed denom must then be converted to a full trace in a second query.

We could provide a convenience function to both query account balance and retrieve full IBC traces for IBC denoms in one query.

@crodriguezvega crodriguezvega added the needs discussion Issues that need discussion before they can be worked on label Jun 7, 2022
@crodriguezvega
Copy link
Contributor

So is the idea that should add a a new CLI/gRPC that returns the same result as q bank balances but incorporates an extra field with the IBC trace in case the balance of the account includes IBC tokens?

@crodriguezvega crodriguezvega added this to the Q3-2022-backlog milestone Jul 5, 2022
@crodriguezvega
Copy link
Contributor

crodriguezvega commented Aug 19, 2022

A proposal for a potential solution direction. Thanks a lot to @seantking for the help!

  • Add a new proto message in proto/ibc/applications/transfer/v1/transfer.proto that basically is a copy of sdk.Coin but includes also the denom trace information:
message Coin {
  option (gogoproto.equal) = true;

  string denom  = 1;
  string amount = 2 [
    (cosmos_proto.scalar)  = "cosmos.Int",
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", 
    (gogoproto.nullable)   = false
  ];

  string denom_trace = 3 [(gogoproto.moretags) = "yaml:\"denom_trace\""];
}
  • In proto/ibc/applications/transfer/v1/query.proto add the RPC service to query the account balances and request/response messages:
// AccountBalances queries the balance of all coins for a single account.
rpc AccountBalances(QueryAccountBalancesRequest) returns (QueryAccountBalancesResponse) {
  option (google.api.http).get = "/ibc/apps/transfer/v1/balances/{address}";
}
// QueryAccountBalancesRequest is the request type for the Query/AccountBalances RPC method.
message QueryAccountBalancesRequest {
  option (gogoproto.equal)           = false;
  option (gogoproto.goproto_getters) = false;

  // address is the address to query balances for.
  string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];

  // pagination defines an optional pagination for the request.
  cosmos.base.query.v1beta1.PageRequest pagination = 2;
}

// QueryAccountBalancesResponse is the response type for the Query/AccountBalances RPC method.
message QueryAccountBalancesResponse {
  // balances is the balances of all the coins.
  repeated Coin balances = 1
      [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "Coins"];

  // pagination defines the pagination in the response.
  cosmos.base.query.v1beta1.PageResponse pagination = 2;
}
  • In modules/apps/transfer/types/coin.go add a constructor function for the Coin type:
func NewCoin(
  denom string, amount math.Int, denomTrace string,
) Coin {
  return Coin{
    Denom:      denom,
    Amount:     amount,
    DenomTrace: denomTrace,
  }
}
  • In modules/apps/transfer/keeper/grpc_query.go add the RPC handler:
func (q Keeper) AccountBalances(
  c context.Context, 
  req *types.QueryAccountBalancesRequest
) (*types.QueryAccountBalancesResponse, error) {
  if req == nil {
    return nil, status.Error(codes.InvalidArgument, "empty request")
  }

  if req.Address == "" {
    return nil, status.Error(codes.InvalidArgument, "address cannot be empty")
  }

  addr, err := sdk.AccAddressFromBech32(req.Address)
  if err != nil {
    return nil, status.Errorf(codes.InvalidArgument, "invalid address: %s", err.Error())
  }

  ctx := sdk.UnwrapSDKContext(c)
  var balances []types.Coin
  store := ctx.KVStore(sdk.NewKVStoreKey(banktypes.StoreKey))
  accountStore := prefix.NewStore(store, banktypes.CreateAccountBalancesPrefix(addr))

  pageRes, err := query.Paginate(accountStore, req.Pagination, func(key, value []byte) error {
    var amount math.Int
    if err := amount.Unmarshal(value); err != nil {
      return err
    }

    denom := string(key)
    var denomTrace string
    if hash, err := types.ParseHexHash(strings.TrimPrefix(denom, "ibc/")); err != nil {
      if dt, found := q.GetDenomTrace(ctx, hash); found == true {
	denomTrace = dt.GetFullDenomPath()
      }
    }

    balances = append(balances, types.NewCoin(denom, amount, denomTrace))
    return nil
  })
  if err != nil {
    return nil, status.Errorf(codes.InvalidArgument, "paginate: %v", err)
  }

  return &types.QueryAccountBalancesResponse{Balances: balances, Pagination: pageRes}, nil
}

The code to get the account balances is pretty much a copy from the SDK bank keeper's function AllBalances. Other alternatives that were considered to implement this:

  1. Making a GRPC query to AllBalances, then iterate over the results and add the denom trace if needed. I couldn't figure out how to do this, though.
  2. Using q.bankKeeper.GetBalances to query the account balances and then iterate over the results and add the denom trace if needed. The drawback of this approach is that we need to take care of pagination ourselves.
  • In modules/apps/transfer/client/cli/query.go add the CLI query function:
// GetCmdQueryAccountBalances defines the command to query an account balances including denom trace for IBC denoms
func GetCmdQueryAccountBalances() *cobra.Command {
  cmd := &cobra.Command{
    Use:     "balances [address]",
    Short:   "<short description of the command>",
    Long:    "<long description of the command>",
    Example: fmt.Sprintf("%s query ibc-transfer balances cosmos1rsp837a4kvtgp2m4uqzdge0zzu6efqgucm0qdh", version.AppName),
    Args:    cobra.ExactArgs(1),
    RunE: func(cmd *cobra.Command, args []string) error {
      clientCtx, err := client.GetClientQueryContext(cmd)
      if err != nil {
        return err
      }
      queryClient := types.NewQueryClient(clientCtx)

      req := &types.QueryAccountBalancesRequest{
        Address: args[0],
      }

      res, err := queryClient.AccountBalances(cmd.Context(), req)
      if err != nil {
        return err
      }

      return clientCtx.PrintProto(res)
    },
  }

  flags.AddQueryFlagsToCmd(cmd)
  return cmd
}

Please comment with suggestions or feedback on whether this is a good direction or not. Open to improve on the location of the code, naming, etc.

@ValarDragon
Copy link
Contributor

I feel like this is generally not the right direction.

I think IBC should commit to doing a migration to get much more human readable denoms by default, and only hash in the situation where its too long. (Say total denom > 256 chars)

I then think we change the bank layer, to have a "trusted denom display" registry, where it can maintain a map from "input name" to on-chain denom, that we get all coins parsing to use, and a map from "on-chain denom" to "preferred display denom". Then we make bank queries / all user output use that.

@crodriguezvega
Copy link
Contributor

Closing this issue for now. After discussion with the SDK team, we will try to implement a solution on the SDK (see #14181).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-UX needs discussion Issues that need discussion before they can be worked on
Projects
None yet
Development

No branches or pull requests

4 participants
@ValarDragon @crodriguezvega @AdityaSripal and others