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

Add pagination support to bank.totalSupply query in BankExtension #1095

Closed
webmaster128 opened this issue Mar 15, 2022 · 5 comments · Fixed by #1238
Closed

Add pagination support to bank.totalSupply query in BankExtension #1095

webmaster128 opened this issue Mar 15, 2022 · 5 comments · Fixed by #1238
Milestone

Comments

@webmaster128
Copy link
Member

After #1094 all supported backends have the pagination support in QueryTotalSupplyRequest and we should support it directly in BankExtension using the pagination key as we do for other list queries.

@JGJP
Copy link

JGJP commented Mar 16, 2022

I would actually recommend that we expand pagination support to other queries also. Specifically for the limit param. Another example that I had to manually override would be Staking/ValidatorDelegations.

Currently extensions that support pagination only accept the pagination key as a param and will form their own Pagination object for the request that sets 0 as the limit, not sure what the intention is here but it's not particularly useful since the RPC node will time out on heavy requests, trying to return all results:

export function createPagination(paginationKey?: Uint8Array): PageRequest | undefined {
return paginationKey
? PageRequest.fromPartial({
key: paginationKey,
offset: Long.fromNumber(0, true),
limit: Long.fromNumber(0, true),
countTotal: false,
})
: undefined;
}

I suggest we allow the passing of limit as a regular integer, and then pass that to createPagination to use, if provided.

@JGJP
Copy link

JGJP commented Mar 16, 2022

I mean ideally we could pass any of these for it to use, turning any integers to Longs in this utility function. It would not be particularly confusing as long as the arguments are typed exactly as the underlying client Request types are:
https://github.com/confio/cosmjs-types/blob/e3e0c4ba52d38af45522f5705c24eb734494b9a4/src/cosmos/bank/v1beta1/query.ts#L43-L54

We can write a type that uses the original type as is, except instead of taking Longs we take numbers:

type QueryTypeAsExtensionParam<QueryType> = {
    [Key in keyof QueryType]: Key extends "pagination"
        ? {
            [PKey in keyof QueryType[Key]]: QueryType[Key][PKey] extends Long
                ? number
                : QueryType[Key][PKey]
        }
        : QueryType[Key]
}

@webmaster128
Copy link
Member Author

object for the request that sets 0 as the limit, not sure what the intention is here but it's not particularly useful since the RPC node will time out on heavy requests, trying to return all results:

In protobuf, the empty/default value 0 cannot be differentiated from unset. So 0 means unset. In the protobuf docs of interface PageRequest we have

  /**
   * limit is the total number of results to be returned in the result page.
   * If left empty it will default to a value to be set by each app.
   */
  limit: Long;

So we don't use unlimited by default.

@webmaster128
Copy link
Member Author

@JGJP Thanks for all your input. I did not forget your request regarding more detailed pagination settings. But if we do that change, we should do that for all queries that are supported right now. And looking at the evolution of those query wrappers, they will hopefully become obsolete soon. Then you can just use the full protobuf API.

@webmaster128
Copy link
Member Author

Shipped as part of CosmJS 0.29.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants