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

fix: incessant requerying of nfts with missing metadata #5747

Closed
wants to merge 1 commit into from

Conversation

kyranjamie
Copy link
Collaborator

@kyranjamie kyranjamie commented Aug 9, 2024

Try out Leather build 4b54db2Extension build, Test report, Storybook, Chromatic

This PR adds some state to prevent requerying of metadata when we know that it isn't, and won't, be there in future. This'll save loads of requests to Hiro's APIs.

This PR only tackles NFTs. Perhaps you can follow up doing the same with FTs @alter-eggo?

Adding this highlights some of the failures of the query package, I think. For example, the "query hook" in question was actually a wrapper hook, preventing accessing the internals. A rule for query package should be that ONLY options creator functions are returned. I think we shouldn't even return wrapped useQueries as it get confusing when we need to create an app-specific clone of same name.

Before

2024-08-09-000266.mp4

After
Now only has FT missing queries

2024-08-09-000267.mp4

@kyranjamie kyranjamie requested a review from alter-eggo August 9, 2024 10:10
@kyranjamie kyranjamie force-pushed the fix/dead-nft-queries branch from 1fbc7c1 to 4b54db2 Compare August 9, 2024 10:28
@kyranjamie kyranjamie marked this pull request as ready for review August 9, 2024 11:21
@@ -18,7 +16,13 @@ interface StacksCryptoAssetsProps {
export function StacksCryptoAssets({ address }: StacksCryptoAssetsProps) {
const names = useGetBnsNamesOwnedByAddressQuery(address).data?.names;

const stacksNftsMetadataResp = useStacksNonFungibleTokensMetadata(address);
// NftMetadataResponse[]
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed I believe?

@kyranjamie kyranjamie linked an issue Aug 12, 2024 that may be closed by this pull request
@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Aug 12, 2024

@alter-eggo to implement similar solution for FTs to compare with this PR

@pete-watters
Copy link
Contributor

@alter-eggo do you think this is OK to merge and get released? Once we remove this code

@alter-eggo
Copy link
Contributor

@pete-watters I think we can close this pr

@pete-watters
Copy link
Contributor

@pete-watters I think we can close this pr

We can merge it? Or just close it as the work is no longer needed?

@alter-eggo
Copy link
Contributor

@pete-watters just closed, problem is resolved here leather-io/mono#377

@alter-eggo alter-eggo closed this Aug 28, 2024
@alter-eggo alter-eggo deleted the fix/dead-nft-queries branch August 28, 2024 12:43
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.

Prevent reload of data for Stacks NFTs without metadata
4 participants