search

public-awesome / cw-nfts

Examples and helpers to build NFT contracts on CosmWasm
Apache License 2.0
186 stars 179 forks source link

feat: cw721 base max pagination limit #147

Closed emidev98 closed 6 months ago

emidev98 commented 7 months ago

This pull request increases the MAX_LIMIT for the pagination from 100 to 1000. Increasing this value may lead to performance issues when too many memory points have to be accessed. But it also reduce the bandwith consumption because instead of querying 10 times 100 NFT(s) you can do it with 1 request. This increment of the MAX_LIMIT also helps the frontend developers to display more values and to not have to wait for each 100 NFT(s) sequentially to display the next ones because they can do larger batches.

USECASE: all_tokens return the id's of the already minted NFT(s), combined with a JSON embedded in the frontend you will not need to create an indexer nor you will be penalized that heavily for multiple requests that have to be executed to the API to return the values and you will be able to display more minted NFT(s) at once.

taitruong commented 6 months ago

I second that! Could you revert release 0.18.1 back to 0.18.0? once merged into main, there'll be another official release merge.

This here should only cover change of MAX_LIMIT from 100 to 1000. Alternatively we could extend contract with a new MAX_LIMIT state (instead of const) and allow contract to be instantiated with any given max limit. In this case, migration should also consider this.

@JakeHartnell @shanev, thoughts on this?

shanev commented 6 months ago

I second that! Could you revert release 0.18.1 back to 0.18.0? once merged into main, there'll be another official release merge.

This here should only cover change of MAX_LIMIT from 100 to 1000. Alternatively we could extend contract with a new MAX_LIMIT state (instead of const) and allow contract to be instantiated with any given max limit. In this case, migration should also consider this.

@JakeHartnell @shanev, thoughts on this?

Wouldn't each contract having different max limits cause issues with indexing? I'm all for increasing the default though.

emidev98 commented 6 months ago

Hello @Art3miX well catch! Seems to be working fine with 100 I have done some tests with a localnet

Art3miX commented 6 months ago

Hello @Art3miX well catch! Seems to be working fine with 100 I have done some tests with a localnet

The gas limit for queries are set per RPC per chain, so testnet limit is different then mainnet, just to have a piece of mind, I would test it on at least the testnets of some of the chains, using the main RPC just to confirm it is really not an issue.

Again, I don't think it is an issue, but thats an 10x increase, so I think its worth checking just to confirm.

emidev98 commented 6 months ago

I see what you mean @Art3miX , then overall the changes should be fine right? In the end the query gas fee depends on the validators and the functionality already works. Besides that developers will always be the ones in charge to set the LIMIT, they are not forced to set the LIMIT as the same value from MAX_LIMIT