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

feat: cw721 base max pagination limit #147

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

emidev98
Copy link
Contributor

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
Copy link
Collaborator

taitruong commented Dec 28, 2023

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
Copy link
Collaborator

shanev commented Dec 29, 2023

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.

Copy link
Collaborator

@Art3miX Art3miX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.
Just one question, did you do tests on actual live RPCs?
Even tho I don't think there will be a problem, as usually the official RPCs are generous with their query limits,
Its still a 10x increase, it would be nice to make sure it pass the "real world" test on chains that use nfts like stargaze, juno, terra, etc.

@emidev98
Copy link
Contributor Author

emidev98 commented Jan 4, 2024

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

@Art3miX
Copy link
Collaborator

Art3miX commented Jan 4, 2024

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
Copy link
Contributor Author

emidev98 commented Jan 4, 2024

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

@taitruong taitruong merged commit 2bd814b into CosmWasm:main Jan 4, 2024
6 checks passed
@taitruong taitruong mentioned this pull request Jan 10, 2024
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 this pull request may close these issues.

None yet

4 participants