[SDK] Update token price APIs to query from insight directly #7220
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 7065da1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
## Walkthrough
This update standardizes token price API calls in the "thirdweb" component to use the "insight" service, replacing previous data sources. It refactors authentication for insight endpoints, removes deprecated webhook OTP verification, adds new token-related endpoints, updates NFT query parameters, and enhances error handling in crypto/fiat conversion utilities and their tests.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------|
| .changeset/fuzzy-walls-allow.md<br>.changeset/petite-falcons-ring.md | Added/updated changeset metadata files documenting patch updates for "thirdweb" and "insight" components. |
| packages/engine/CHANGELOG.md | Fixed changelog header to reference the correct package. |
| packages/insight/src/client/sdk.gen.ts | Standardized authentication to API key headers, removed webhook OTP endpoints, added new token endpoints, deprecated old balance APIs. |
| packages/thirdweb/src/insight/get-nfts.ts | Changed NFT query parameter from `chain` to `chain_id` (as an array). |
| packages/thirdweb/src/pay/convert/cryptoToFiat.ts<br>packages/thirdweb/src/pay/convert/fiatToCrypto.ts | Replaced manual fetch calls with `getV1TokensPrice` from insight, improved error handling, updated response parsing. |
| packages/thirdweb/src/pay/convert/cryptoToFiat.test.ts<br>packages/thirdweb/src/pay/convert/fiatToCrypto.test.ts | Removed tests for failed fetch responses and unused imports. |
| packages/thirdweb/src/pay/utils/definitions.ts | Removed deprecated functions returning fiat/crypto conversion endpoint URLs. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant Thirdweb
participant InsightAPI
Client->>Thirdweb: convertCryptoToFiat(token, amount, chain)
Thirdweb->>InsightAPI: getV1TokensPrice(token, chain_id)
InsightAPI-->>Thirdweb: token price data
Thirdweb-->>Client: fiat value (amount * price_usd)sequenceDiagram
participant Client
participant Thirdweb
participant InsightAPI
Client->>Thirdweb: convertFiatToCrypto(token, amount, chain)
Thirdweb->>InsightAPI: getV1TokensPrice(token, chain_id)
InsightAPI-->>Thirdweb: token price data
Thirdweb-->>Client: crypto amount (amount / price_usd)
Possibly related PRs
Suggested reviewers
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
joaquim-verges
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/insight/src/client/sdk.gen.ts (1)
1-1172: 🛠️ Refactor suggestionOverall assessment: Well-structured API evolution with breaking changes.
The changes represent a significant but well-planned API evolution:
Positives:
- Consistent authentication model (x-secret-key for webhooks, x-client-id for others)
- Proper deprecation strategy with backward compatibility
- New token functionality enhances API capabilities
- Code follows established patterns and conventions
Critical considerations:
- Breaking authentication changes require migration support
- Deprecated endpoints need clear migration paths
- Documentation should be updated to reflect all changes
Ensure comprehensive migration documentation is provided for the authentication changes and deprecated endpoints to minimize disruption to existing users.
🧹 Nitpick comments (2)
packages/thirdweb/src/pay/convert/cryptoToFiat.ts (1)
98-121: Consider adding zero price validation for consistency.The refactoring to use
getV1TokensPriceis well-implemented with proper error handling and response parsing. However, there's an inconsistency with thefiatToCrypto.tsimplementation.Apply this diff to add zero price validation for consistency with
fiatToCrypto.ts:- if (!firstResult) { + if (!firstResult || firstResult.price_usd === 0) { throw new Error( `Failed to fetch ${to} value for token (${fromTokenAddress}) on chainId: ${chain.id}`, ); }This ensures that tokens with zero USD price are handled consistently across both conversion functions.
packages/insight/src/client/sdk.gen.ts (1)
524-527: LGTM! Proper deprecation strategy with new tokens endpoint.The deprecation of
getV1TokensErc20ByOwnerAddressin favor of the newgetV1Tokensendpoint follows good practices by maintaining backward compatibility while guiding users to the new API.Consider providing migration examples in documentation showing how to transition from the deprecated ERC-20 specific endpoint to the new general tokens endpoint.
Also applies to: 549-570
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/fuzzy-walls-allow.md(1 hunks).changeset/petite-falcons-ring.md(1 hunks)packages/engine/CHANGELOG.md(1 hunks)packages/insight/src/client/sdk.gen.ts(15 hunks)packages/thirdweb/src/insight/get-nfts.ts(1 hunks)packages/thirdweb/src/pay/convert/cryptoToFiat.test.ts(1 hunks)packages/thirdweb/src/pay/convert/cryptoToFiat.ts(2 hunks)packages/thirdweb/src/pay/convert/fiatToCrypto.test.ts(1 hunks)packages/thirdweb/src/pay/convert/fiatToCrypto.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/insight/src/client/sdk.gen.ts (1)
packages/insight/src/client/types.gen.ts (6)
GetV1TokensOwnersData(1740-1763)GetV1TokensOwnersResponse(1800-1800)GetV1TokensOwnersError(1784-1784)GetV1TokensTransfersData(2014-2065)GetV1TokensData(2199-2238)GetV1TokensResponse(2270-2270)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit Tests
- GitHub Check: Size
🔇 Additional comments (13)
packages/engine/CHANGELOG.md (1)
1-1: LGTM! Correct package name in changelog header.This properly fixes the changelog header to reference the correct package name.
.changeset/fuzzy-walls-allow.md (1)
1-6: LGTM! Properly formatted changeset documentation.The changeset correctly documents the patch update for the thirdweb package with an appropriate description.
.changeset/petite-falcons-ring.md (1)
1-6: LGTM! Properly formatted changeset documentation.The changeset correctly documents the patch update for the insight package with an appropriate description.
packages/thirdweb/src/pay/convert/cryptoToFiat.test.ts (1)
1-1: LGTM! Import cleanup aligns with refactoring.The removal of the
viimport is consistent with the shift away from manual fetch mocking to using the insight SDK directly.packages/thirdweb/src/pay/convert/fiatToCrypto.test.ts (1)
1-1: LGTM! Import cleanup aligns with refactoring.The removal of the
viimport is consistent with the shift away from manual fetch mocking to using the insight SDK directly.packages/thirdweb/src/pay/convert/fiatToCrypto.ts (2)
1-1: LGTM! Updated imports for insight SDK integration.The new imports properly support the refactoring to use the insight service directly instead of manual fetch calls.
Also applies to: 9-11
96-119: LGTM! Well-implemented insight SDK integration with robust error handling.The refactoring to use
getV1TokensPricefrom the insight SDK is well-executed:
- Proper configuration with base URL and client fetch
- Enhanced error handling that includes HTTP status and detailed error information
- Defensive programming with checks for valid response data and non-zero price
- Correct conversion calculation (fiat amount divided by USD price)
The implementation maintains the same function behavior while improving the underlying data source and error reporting.
packages/thirdweb/src/pay/convert/cryptoToFiat.ts (1)
1-1: LGTM! Updated imports for insight SDK integration.The new imports properly support the refactoring to use the insight service directly instead of manual fetch calls.
Also applies to: 9-11
packages/insight/src/client/sdk.gen.ts (5)
72-72: LGTM! New token-related type imports.The new imports properly support the added token and token owners endpoints.
Also applies to: 81-83, 88-88
169-169: LGTM! Webhook descriptions updated to reflect removed OTP functionality.The descriptions now correctly reflect the current webhook functionality without the deprecated OTP verification features.
Also applies to: 221-221
424-445: LGTM! New token owners endpoint properly implemented.The
getV1TokensOwnersfunction follows established patterns and uses consistent authentication with other non-webhook endpoints.
501-507: LGTM! Token transfers endpoint made more flexible.The changes make the options parameter optional and update the description to reflect broader functionality. The implementation is consistent with other optional parameter functions.
Verify that the API endpoint now supports general token transfers queries without requiring specific wallet address parameters.
869-870: LGTM! NFT endpoints updated consistently.The changes to NFT endpoints follow the same patterns as token endpoints - simplified descriptions and optional parameters for increased flexibility.
Also applies to: 942-948
| @@ -213,7 +213,7 @@ export async function getNFT(args: { | |||
|
|
|||
| const defaultQueryOptions: GetV1NftsByContractAddressByTokenIdData["query"] = | |||
| { | |||
| chain: chain.id, | |||
| chain_id: [chain.id], | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency of query parameter formats across similar functions.
The parameter change from chain: chain.id to chain_id: [chain.id] looks correct and aligns with the API updates. However, other functions in this file still use the chain parameter format (lines 59, 136). Please verify that this inconsistency is intentional due to different endpoint requirements.
🏁 Script executed:
#!/bin/bash
# Verify the query parameter consistency across similar insight API calls
echo "Checking for chain/chain_id parameter usage patterns in insight-related files:"
rg -A 3 -B 3 "chain(_id)?:" packages/thirdweb/src/insight/
echo -e "\nChecking insight SDK types for expected parameter formats:"
rg -A 5 -B 2 "chain_id|chain.*:" packages/insight/ || echo "Insight package files not available for verification"Length of output: 71155
Ensure consistent use of chain_id across all Insight queries
According to the Insight SDK types, the chain parameter is now deprecated in favor of chain_id (an array of IDs). While getNFT has been updated, the other query builders in get-nfts.ts still use the old chain field. Please update them to match:
• packages/thirdweb/src/insight/get-nfts.ts
– In the defaultQueryOptions for fetching all NFTs (GetV1NftsData), change:
diff - chain: chains.map((chain) => chain.id), + chain_id: chains.map((chain) => chain.id),
• packages/thirdweb/src/insight/get-nfts.ts
– In the defaultQueryOptions for fetching NFTs by contract address (GetV1NftsByContractAddressData), apply the same replacement.
This will align every Insight endpoint with the new API spec and avoid relying on deprecated parameters.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/thirdweb/src/insight/get-nfts.ts around lines 59 and 136, update the
query parameter from the deprecated 'chain' to 'chain_id' as an array of IDs,
similar to the change made at line 216. Replace all instances of 'chain:
chain.id' with 'chain_id: [chain.id]' in the defaultQueryOptions for fetching
all NFTs and for fetching NFTs by contract address to ensure consistency with
the Insight SDK's current API specification.
| { | ||
| in: "query", | ||
| name: "clientId", | ||
| name: "x-secret-key", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: Webhook authentication mechanism updated.
All webhook endpoints now use x-secret-key API key authentication instead of the previous bearer token approach. This is a breaking change that will require users to update their webhook authentication.
Ensure that:
- Migration documentation is provided for existing webhook users
- The new authentication mechanism is properly documented
- Backward compatibility considerations have been addressed
Also applies to: 181-181, 210-210, 235-235, 262-262
🏁 Script executed:
#!/bin/bash
# Check if there are any references to the old bearer authentication in documentation
rg -i "bearer.*webhook|webhook.*bearer" --type md --type txtLength of output: 62
🏁 Script executed:
#!/bin/bash
# Broadly search for any mention of the old bearer auth in docs
rg -i "bearer" --type md --type txt -A 2
# Search for the new x-secret-key usage in docs to confirm documentation was updated
rg -i "x-secret-key" --type md --type txt -A 2Length of output: 444
Breaking change: Webhook authentication switched to x-secret-key
Documentation still references the old Bearer-token approach but lacks any mention of the new header. Before merging, please:
• Add migration guidance for existing users (e.g. “replace Authorization: Bearer <token> with x-secret-key: <your-secret>”).
• Update docs to remove or mark deprecated all Bearer examples (found in AGENTS.md and apps/portal/public/llms-full.txt).
• Include examples and parameter descriptions for the new x-secret-key header.
• Consider a deprecation window or backward-compatibility layer if needed.
Files to update:
- AGENTS.md
- apps/portal/public/llms-full.txt
- Any other Markdown/text docs with “Bearer”
🤖 Prompt for AI Agents
In packages/insight/src/client/sdk.gen.ts at lines 158, 181, 210, 235, and 262,
update the webhook authentication header from the old Bearer token to the new
"x-secret-key" header. Additionally, update all relevant documentation files
including AGENTS.md and apps/portal/public/llms-full.txt to remove or mark
deprecated the Bearer token usage, add migration instructions for users to
switch from "Authorization: Bearer <token>" to "x-secret-key: <your-secret>",
and include examples and parameter descriptions for the new header. Consider
adding a deprecation notice or backward compatibility if necessary before
finalizing the changes.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (78.94%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7220 +/- ##
==========================================
- Coverage 55.63% 55.62% -0.02%
==========================================
Files 908 908
Lines 58546 58570 +24
Branches 4128 4133 +5
==========================================
+ Hits 32572 32577 +5
- Misses 25868 25886 +18
- Partials 106 107 +1
🚀 New features to boost your workflow:
|
joaquim-verges
force-pushed
the
05-30-update_token_price_apis_to_query_from_insight_directly
branch
from
a68be7b to
7065da1
Compare
joaquim-verges
deleted the
05-30-update_token_price_apis_to_query_from_insight_directly
branch
PR-Codex overview
This PR primarily focuses on updating the
@thirdweb-dev/engineand related packages to improve API specifications, enhance error handling, and optimize fetching mechanisms for token prices and NFT data.Detailed summary
CHANGELOG.mdfor version3.0.3.@thirdweb-dev/insightandthirdweb.chaintochain_id.definitions.ts.convertFiatToCryptoandconvertCryptoToFiatfunctions.withCache.sdk.gen.tsandtypes.gen.tsto include new fields likenamein webhook responses.Summary by CodeRabbit