Fetch native currency from chain API when required #8267
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b7eb23e The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
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. |
WalkthroughAdds a changeset entry; enhances getCurrencyMetadata to validate native currency with a zod schema, dynamically fetch chain metadata when Changes
Sequence DiagramsequenceDiagram
participant Caller
participant getCurrencyMetadata
participant ChainDef as "Chain Definition"
participant ChainAPI as "getChainMetadata"
participant Response
Caller->>getCurrencyMetadata: Request token metadata (address, chain, options)
getCurrencyMetadata->>ChainDef: Is address native? check chain.nativeCurrency
alt chain.nativeCurrency exists
ChainDef-->>getCurrencyMetadata: Return nativeCurrency
getCurrencyMetadata->>Response: Parse via NATIVE_CURRENCY_SCHEMA -> return
else chain.nativeCurrency missing
rect rgb(235,245,255)
note right of getCurrencyMetadata: Dynamic import + fetch chain metadata
getCurrencyMetadata->>ChainAPI: getChainMetadata(chainId)
alt fetch succeeds
ChainAPI-->>getCurrencyMetadata: Return chain metadata (includes nativeCurrency)
getCurrencyMetadata->>Response: Parse chain.nativeCurrency via schema -> return
else fetch fails
ChainAPI-->>getCurrencyMetadata: Error
getCurrencyMetadata->>Response: Use default native currency (Ether, ETH, 18)
end
end
end
Response-->>Caller: Final metadata (apply option overrides)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
WalkthroughA patch release adds fallback logic to fetch native currency metadata from a chain API when predefined chain definitions lack this information, with a changeset documenting the update. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getCurrencyMetadata
participant getChainMetadata
participant API
Caller->>getCurrencyMetadata: getCurrencyMetadata(address, options)
alt address is native
getCurrencyMetadata->>getCurrencyMetadata: check if chain.nativeCurrency exists
alt nativeCurrency defined
getCurrencyMetadata-->>Caller: return predefined metadata
else nativeCurrency not defined
getCurrencyMetadata->>getChainMetadata: fetch chain metadata
getChainMetadata->>API: query chain data
alt API call succeeds
API-->>getChainMetadata: return chain data
getChainMetadata-->>getCurrencyMetadata: return nativeCurrency
getCurrencyMetadata-->>Caller: return fetched metadata
else API call fails
API-->>getChainMetadata: error
getChainMetadata-->>getCurrencyMetadata: fallback
getCurrencyMetadata-->>Caller: return default (18 decimals, "Ether")
end
end
else address is not native
getCurrencyMetadata-->>Caller: return standard ERC20 metadata
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4).changeset/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/thirdweb/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (1)
17-28: Update TSDoc to document API fallback behavior.Per coding guidelines: "Every public symbol must have comprehensive TSDoc." The function description should mention the new fallback behavior where chain metadata is fetched from the API when
chain.nativeCurrencyis unavailable.Based on coding guidelines
Apply this diff to update the documentation:
/** - * Retrieves the metadata of a currency. + * Retrieves the metadata of a currency. For native tokens, attempts to fetch + * chain metadata from the API if the chain definition lacks native currency information. * @param options - The options for the transaction. * @returns A promise that resolves to an object containing the currency metadata. * @extension ERC20
🧹 Nitpick comments (2)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (2)
1-1: Consider lazy loading the import.Per coding guidelines: "Load heavy dependencies inside async paths to keep initial bundle lean." If
getChainMetadatainvolves significant dependencies or network calls, consider lazy-importing it within the conditional block.Example refactor if lazy loading is beneficial:
-import { getChainMetadata } from "../../../chains/utils.js"; import { isNativeTokenAddress } from "../../../constants/addresses.js";Then inside the function:
if (!options.contract.chain.nativeCurrency) { try { + const { getChainMetadata } = await import("../../../chains/utils.js"); const chain = await getChainMetadata(options.contract.chain);
36-42: Consider adding observability for API failures.The catch block silently swallows errors, which aids graceful degradation but may hide legitimate issues (network failures, API changes, etc.). Consider adding structured logging to track when the API fallback is used and when it fails.
Example:
} catch { // no-op, fall through to the default values below + // Optional: Add logging if observability framework is available + // console.debug("Failed to fetch chain metadata, using defaults"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/forty-donuts-happen.md(1 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/*.md: Each change inpackages/*must include a changeset for the appropriate package
Version bump rules: patch for non‑API changes; minor for new/modified public API
Files:
.changeset/forty-donuts-happen.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
.changeset/forty-donuts-happen.md (1)
1-5: LGTM! Correct patch bump for behavioral change.The changeset correctly identifies this as a patch bump since the change modifies internal behavior without altering the public API signature.
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (1)
37-39: Type compatibility verified. ChainMetadata.nativeCurrency has{ name: string; symbol: string; decimals: number }, matching GetCurrencyMetadataResult exactly.
size-limit report 📦
|
jnsdls
force-pushed
the
Fetch_native_currency_from_chain_API_when_required
branch
from
f5ec5f2 to
e939be5
Compare
jnsdls
force-pushed
the
Fetch_native_currency_from_chain_API_when_required
branch
from
e939be5 to
380408e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/forty-donuts-happen.md(1 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/forty-donuts-happen.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (2)
1-1: LGTM!The Zod import is correctly added to support schema validation for native currency metadata.
8-12: Schema structure is correct.The native currency schema with default values is well-defined and aligns with the PR objectives. The field-level defaults will be applied when an object is provided but individual fields are missing.
jnsdls
force-pushed
the
Fetch_native_currency_from_chain_API_when_required
branch
from
380408e to
88c9939
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (2)
46-58: Good implementation; consider adding observability.The logic correctly implements the graceful fallback behavior:
- Attempts to fetch from API when native currency is missing
- Falls back to default values on failure
- The schema's
.default()ensures safe parsing ofundefinedThis implementation addresses the previous review concerns about early returns and undefined parsing.
Recommended improvement: Add error logging in the catch block for better debugging and observability:
} catch { // no-op, fall through to the default values below } + } catch (error) { + // Log the error for debugging but fall through to defaults + if (process.env.NODE_ENV === 'development') { + console.warn('Failed to fetch native currency from API:', error); + } + }Optional improvement: Update the TSDoc to mention the new API fetch behavior:
/** * Retrieves the metadata of a currency. + * For native tokens, attempts to fetch metadata from the chain API if not available in local definitions. * @param options - The options for the transaction. * @returns A promise that resolves to an object containing the currency metadata.
1-1: Replace default zod import with named import
Useimport { z } from "zod";instead ofimport z from "zod"to align with the majority of the codebase.-import z from "zod"; +import { z } from "zod";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/forty-donuts-happen.md(1 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/forty-donuts-happen.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
🧠 Learnings (1)
📚 Learning: 2025-06-26T19:46:04.024Z
Learnt from: gregfromstl
PR: thirdweb-dev/js#7450
File: packages/thirdweb/src/bridge/Webhook.ts:57-81
Timestamp: 2025-06-26T19:46:04.024Z
Learning: In the onramp webhook schema (`packages/thirdweb/src/bridge/Webhook.ts`), the `currencyAmount` field is intentionally typed as `z.number()` while other amount fields use `z.string()` because `currencyAmount` represents fiat currency amounts in decimals (like $10.50), whereas other amount fields represent token amounts in wei (very large integers that benefit from bigint representation). The different naming convention (`currencyAmount` vs `amount`) reflects this intentional distinction.
Applied to files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (1)
8-18: LGTM! Schema design correctly addresses previous concerns.The dual-default pattern is well-designed:
- Field-level defaults (lines 10-12) apply when an object is passed with missing fields
- Schema-level default (lines 14-18) handles the case when
undefinedis parsedThis addresses the previous review concern about parsing
undefinedthrowing an error. The schema-level.default()ensures graceful fallback to default values.
jnsdls
force-pushed
the
Fetch_native_currency_from_chain_API_when_required
branch
from
88c9939 to
f4b4803
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (3)
8-18: Handle bothnullandundefinedfor graceful fallback.The schema's
.default()only applies when the value is strictlyundefined. Ifchain.nativeCurrencyor the API response is explicitlynull, parsing will throw instead of falling back to defaults. This breaks the graceful fallback behavior described in the PR objectives.Apply this diff to handle both
nullandundefined:const NATIVE_CURRENCY_SCHEMA = z .object({ name: z.string().default("Ether"), symbol: z.string().default("ETH"), decimals: z.number().default(18), }) + .nullish() .default({ name: "Ether", symbol: "ETH", decimals: 18, });With
.nullish(), the schema will treat bothnullandundefinedthe same way and apply the default values, ensuring truly graceful fallback.
8-18: Consider extracting default values to avoid duplication.The default values (
"Ether","ETH",18) are specified twice: once at the field level and once at the schema level. While this is functionally correct, extracting them into a constant would reduce duplication and improve maintainability.Here's an optional refactor to eliminate duplication:
+const DEFAULT_NATIVE_CURRENCY = { + name: "Ether", + symbol: "ETH", + decimals: 18, +}; + const NATIVE_CURRENCY_SCHEMA = z .object({ - name: z.string().default("Ether"), - symbol: z.string().default("ETH"), - decimals: z.number().default(18), + name: z.string().default(DEFAULT_NATIVE_CURRENCY.name), + symbol: z.string().default(DEFAULT_NATIVE_CURRENCY.symbol), + decimals: z.number().default(DEFAULT_NATIVE_CURRENCY.decimals), }) - .default({ - name: "Ether", - symbol: "ETH", - decimals: 18, - }); + .default(DEFAULT_NATIVE_CURRENCY);
53-55: Consider logging errors in the catch block for debugging.The empty catch block silently suppresses errors from the API fetch. While the graceful fallback is correct, logging these errors (e.g., to console.warn or a telemetry service) would help with debugging in production when the API is failing or returning unexpected data.
Optional enhancement:
} catch (error) { - // no-op, fall through to the default values below + // API fetch failed, fall through to the default values below + // In production, consider logging: console.warn("Failed to fetch native currency from API", error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/forty-donuts-happen.md(1 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/forty-donuts-happen.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (1)
46-58: Excellent use of dynamic imports and graceful fallback.The implementation correctly:
- Lazy-loads
getChainMetadataonly when needed, reducing initial bundle size (per coding guidelines)- Falls back to defaults when the API fetch fails
- Uses the zod schema to ensure type safety and provide defaults
The logic flow handles the key scenarios appropriately:
- When
nativeCurrencyis missing locally → fetch from API- When API fetch fails → fall through to parse local definition with defaults
- When
nativeCurrencyexists locally → parse and validate it
jnsdls
force-pushed
the
Fetch_native_currency_from_chain_API_when_required
branch
from
f4b4803 to
b7eb23e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.changeset/forty-donuts-happen.md(1 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.ts(2 hunks)packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts(2 hunks)packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.ts(2 hunks)packages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.ts(1 hunks)packages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.ts(1 hunks)packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/forty-donuts-happen.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.tspackages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.tspackages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
**/*.test.{ts,tsx}: Co‑locate tests asfoo.test.ts(x)next to the implementation
Use real function invocations with stub data; avoid brittle mocks
Use MSW to intercept HTTP calls for network interactions; mock only hard‑to‑reproduce scenarios
Keep tests deterministic and side‑effect free; use Vitest
Files:
packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.tspackages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.tspackages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.tspackages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.tspackages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.tspackages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.tspackages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.tspackages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.tspackages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to test/src/test-wallets.ts : Predefined test accounts are in `test/src/test-wallets.ts`
Applied to files:
packages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.ts
🧬 Code graph analysis (1)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.ts (1)
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (1)
getCurrencyMetadata(41-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
packages/thirdweb/src/wallets/smart/smart-wallet-integration-v07.test.ts (1)
304-305: LGTM - Appropriate handling of flaky test.Marking the consistently failing parallel transaction test as skipped is a reasonable approach while the root cause is investigated. The FIXME comment ensures this is tracked for resolution.
packages/thirdweb/src/wallets/in-app/web/lib/in-app-integration.test.ts (1)
44-45: LGTM - Consistent approach to flaky tests.Disabling the failing sponsor-gas test follows the same pattern applied across other test files in this PR. The FIXME comment appropriately flags this for future attention.
packages/thirdweb/src/extensions/erc7702/account/sessionkey.test.ts (1)
63-64: LGTM - Consistent handling of failing tests.Both session key tests are appropriately marked as skipped with FIXME comments, maintaining code for future resolution while preventing CI failures.
Also applies to: 82-83
packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts (3)
8-18: LGTM - Well-structured schema with proper defaults.The zod schema correctly handles both undefined input (via object-level default) and partial objects (via field-level defaults), ensuring graceful fallback behavior.
53-64: LGTM - Appropriate lazy loading and merge strategy.The dynamic import keeps the bundle lean, and the merge order correctly prioritizes local overrides over API-fetched values. The silent catch is acceptable here since it falls through to schema defaults.
66-66: LGTM - Correct fallback behavior.The schema parse at this line correctly applies defaults when
nativeCurrencyis undefined or partial, thanks to the object-level and field-level defaults defined inNATIVE_CURRENCY_SCHEMA.packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.test.ts (2)
25-38: Consider mocking the API call for determinism.This test calls
getChainMetadatainternally, which likely hits an external API. If the API is unavailable or returns different data in the future, the test becomes flaky—a pattern already evident in other tests being skipped in this PR.Per coding guidelines: "Use MSW to intercept HTTP calls for network interactions" and "Keep tests deterministic and side-effect free." Consider mocking the API response to ensure test stability.
As per coding guidelines
40-58: Good coverage of partial override behavior, but same API dependency.This test effectively validates the merge logic where local partial data takes precedence over API-fetched values. However, it shares the same external API dependency concern as the previous test.
Consider mocking for consistency with the deterministic testing principle.
As per coding guidelines
| if ( | ||
| !options.contract.chain.nativeCurrency || | ||
| !options.contract.chain.nativeCurrency.name || | ||
| !options.contract.chain.nativeCurrency.symbol || | ||
| !options.contract.chain.nativeCurrency.decimals | ||
| ) { |
There was a problem hiding this comment.
Potential edge case: Zero decimals incorrectly triggers API fetch.
The condition !options.contract.chain.nativeCurrency.decimals evaluates to true for both undefined and 0. While most tokens have non-zero decimals, ERC20 technically allows 0 decimals. If a chain's native currency legitimately has 0 decimals and it's specified in the chain definition, this condition would incorrectly attempt to fetch from the API.
Consider using explicit undefined checks:
if (
!options.contract.chain.nativeCurrency ||
- !options.contract.chain.nativeCurrency.name ||
- !options.contract.chain.nativeCurrency.symbol ||
- !options.contract.chain.nativeCurrency.decimals
+ options.contract.chain.nativeCurrency.name === undefined ||
+ options.contract.chain.nativeCurrency.symbol === undefined ||
+ options.contract.chain.nativeCurrency.decimals === undefined
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !options.contract.chain.nativeCurrency || | |
| !options.contract.chain.nativeCurrency.name || | |
| !options.contract.chain.nativeCurrency.symbol || | |
| !options.contract.chain.nativeCurrency.decimals | |
| ) { | |
| if ( | |
| !options.contract.chain.nativeCurrency || | |
| options.contract.chain.nativeCurrency.name === undefined || | |
| options.contract.chain.nativeCurrency.symbol === undefined || | |
| options.contract.chain.nativeCurrency.decimals === undefined | |
| ) { |
🤖 Prompt for AI Agents
In packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts around
lines 47 to 52, the current falsy checks treat 0 decimals as missing and will
trigger an unnecessary API fetch; change the condition to check decimals
explicitly (e.g., options.contract.chain.nativeCurrency.decimals === undefined)
while keeping the existing checks for name and symbol (or similarly use explicit
undefined/null checks if desired) so that a legitimate 0 value for decimals is
accepted and no API call is made.
There was a problem hiding this comment.
zero decimal can never happen, if we see that we SHOULD fetch
There was a problem hiding this comment.
@jnsdls, thank you for the clarification! I understand now that zero decimals is not a valid value for native currency in this system, and encountering it should indeed trigger a fetch from the API. The current implementation is correct for your use case.
ʕ •ᴥ•ʔ 📚
✏️ Learnings added
Learnt from: jnsdls
PR: thirdweb-dev/js#8267
File: packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts:47-52
Timestamp: 2025-10-16T19:00:34.682Z
Learning: In the thirdweb SDK's getCurrencyMetadata function (packages/thirdweb/src/extensions/erc20/read/getCurrencyMetadata.ts), zero decimals is not a valid value for native currency. If `options.contract.chain.nativeCurrency.decimals` is 0, it should be treated as missing/invalid data and trigger an API fetch to get the correct native currency metadata.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8267 +/- ##
==========================================
- Coverage 55.01% 54.89% -0.13%
==========================================
Files 919 919
Lines 60588 60613 +25
Branches 4126 4115 -11
==========================================
- Hits 33334 33273 -61
- Misses 27150 27239 +89
+ Partials 104 101 -3
🚀 New features to boost your workflow:
|
2 participants
TL;DR
Added functionality to fetch native currency metadata from the chain API when not available locally.
What changed?
Enhanced the
getCurrencyMetadatafunction to fetch native currency information from the chain API when the local chain definition doesn't include native currency details. This ensures that even for chains without predefined native currency information, the system can still retrieve the correct metadata instead of defaulting to "Ether".How to test?
Why make this change?
Some chains may not have their native currency information defined in the local chain definitions. This change improves the reliability of currency metadata retrieval by attempting to fetch this information from the chain API when needed, providing more accurate currency information across a wider range of chains.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
PR-Codex overview
This PR focuses on improving the handling of native currency metadata in the
thirdweblibrary and temporarily skipping failing tests to stabilize the codebase.Detailed summary
getCurrencyMetadatafunction to utilize the new fetching logic.