Skip to content

Get events from insight #6683

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

Merged
merged 19 commits into from
Apr 15, 2025
Merged

Get events from insight #6683

merged 19 commits into from
Apr 15, 2025

Conversation

kumaryash90
Copy link
Member

@kumaryash90 kumaryash90 commented Apr 9, 2025


PR-Codex overview

This PR focuses on enhancing the thirdweb library by introducing new features, improving error handling, and updating existing functionalities related to event fetching and chain services.

Detailed summary

  • Added ChainService type to types.ts.
  • Updated size limit in .size-limit.json.
  • Introduced getContractEvents method in get-events.ts for fetching indexed events.
  • Improved error handling in getOwnedNFTs and getOwnedTokens.
  • Added useIndexer parameter for fetching events.
  • Enhanced tests for getContractEvents to include indexer functionality.
  • Documented new features in ready-tigers-pump.md.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@kumaryash90 kumaryash90 requested review from a team as code owners April 9, 2025 19:09
Copy link

changeset-bot bot commented Apr 9, 2025

🦋 Changeset detected

Latest commit: b6ba290

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
thirdweb Patch
@thirdweb-dev/wagmi-adapter Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Apr 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 10:16am
login ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 10:16am
thirdweb_playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 10:16am
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 10:16am
wallet-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 10:16am

Copy link
Contributor

graphite-app bot commented Apr 9, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge-queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@kumaryash90 kumaryash90 marked this pull request as draft April 9, 2025 19:09
@kumaryash90 kumaryash90 added the DO NOT MERGE This pull request is still in progress and is not ready to be merged. label Apr 9, 2025
@github-actions github-actions bot added packages SDK Involves changes to the thirdweb SDK and removed DO NOT MERGE This pull request is still in progress and is not ready to be merged. labels Apr 9, 2025
Copy link
Contributor

github-actions bot commented Apr 9, 2025

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 50.33 KB (+0.85% 🔺) 1.1 s (+0.85% 🔺) 262 ms (+178.6% 🔺) 1.3 s
thirdweb (cjs) 136.91 KB (+0.4% 🔺) 2.8 s (+0.4% 🔺) 382 ms (+37.45% 🔺) 3.2 s
thirdweb (minimal + tree-shaking) 5.63 KB (0%) 113 ms (0%) 100 ms (+1076.64% 🔺) 212 ms
thirdweb/chains (tree-shaking) 514 B (0%) 11 ms (0%) 73 ms (+2839.68% 🔺) 83 ms
thirdweb/react (minimal + tree-shaking) 19.35 KB (0%) 388 ms (0%) 128 ms (+626.17% 🔺) 516 ms

}

const clientFetch = getClientFetch(client);
const result = await clientFetch(url.toString());
Copy link
Member

Choose a reason for hiding this comment

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

should always check for error on response btw. and parse the error. will fix it when i switch to the autogen api though

throw new Error(result.error.error);
throw new Error(
`${result.response.status} ${result.response.statusText} - ${result.error ? stringify(result.error) : "Unknown error"}`,
);
}
return result.data.data || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a potential null reference issue in this code. The expression result.data.data || [] will throw a TypeError if result.data is undefined.

This pattern is inconsistent with the other insight functions in get-nfts.ts and get-tokens.ts, which use the nullish coalescing operator with optional chaining: result.data?.data ?? [].

To prevent runtime errors when the API returns a response without a data property, consider updating this line to:

return result.data?.data ?? [];

This change would make the error handling consistent across all insight functions and provide better resilience against unexpected API responses.

Suggested change
return result.data.data || [];
return result.data?.data ?? [];

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dashboard Involves changes to the Dashboard. packages SDK Involves changes to the thirdweb SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants