[SDK] Init new API namespaces #7637
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new API client package, Changes
Sequence Diagram(s)Example: Wallet Login Flow (OTP, Guest, OAuth, SIWE, Passkey)sequenceDiagram
participant User
participant App
participant ThirdwebClient
participant API
participant Wallet
User->>App: Initiate login (OTP/OAuth/Passkey/SIWE/Guest)
App->>ThirdwebClient: Prepare login options
alt OTP
App->>API: sendCode (email/phone)
API-->>App: OTP sent
User->>App: Enter OTP code
App->>API: loginWithCode (verify OTP)
API-->>App: Auth result
else OAuth
App->>API: loginWithOauthRedirect
API-->>User: Redirect to OAuth provider
User->>API: Complete OAuth flow
API-->>App: Auth result
else Passkey
App->>API: loginWithPasskey/registerPasskey
API-->>App: Auth result
else SIWE
App->>Wallet: Sign SIWE message
Wallet-->>App: Signature
App->>API: loginWithWallet (SIWE)
API-->>App: Auth result
else Guest
App->>API: guestAuthenticate
API-->>App: Auth result
end
App->>App: createUserWallet(authResult)
App-->>User: UserWallet instance
Example: Contract Write OperationsequenceDiagram
participant User
participant App
participant UserWallet
participant API
User->>App: Request contract write
App->>UserWallet: write({ wallet, chainId, calls })
UserWallet->>API: writeContract (with auth token)
API-->>UserWallet: Transaction result or error
UserWallet-->>App: Transaction ID or error
App-->>User: Result
Example: Fetching User ProfilessequenceDiagram
participant App
participant UserWallet
participant API
App->>UserWallet: getProfiles()
UserWallet->>API: fetchUserProfiles (with auth token)
API-->>UserWallet: Profile list
UserWallet-->>App: Profile list
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
size-limit report 📦
|
joaquim-verges
force-pushed
the
_SDK_Add_authentication_methods_for_v2_wallets
branch
from
5bd242f to
5483874
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (22)
packages/thirdweb/package.json (1)
26-26: Add@thirdweb-dev/apito size-limit & bundle-budget tracking
@thirdweb-dev/apican pull in large OpenAPI payloads.
Please updatepackages/thirdweb/size-limit(or equivalent) so CI fails if the added code bloats the bundle.packages/thirdweb/knip.json (1)
15-17: AlphabetiseignoreDependenciesto avoid merge churnKeeping this list sorted makes future diffs smaller and prevents accidental duplicates (
engine/insightflipped order across PRs previously).packages/thirdweb/src/wallets/in-app/web/lib/auth/otp.ts (1)
55-59: LGTM - Enhanced error handling with actionable suggestion.The improved error handling provides much better debugging information with status code, status text, and response body. However, consider applying the same detailed error handling to the
verifyOtpfunction for consistency.Apply similar error handling to
verifyOtpfunction:if (!response.ok) { - throw new Error("Failed to verify verification code"); + const error = await response.text(); + throw new Error( + `Failed to verify verification code: ${response.status} ${response.statusText} ${error}`, + ); }packages/api/openapi-ts.config.ts (1)
1-1: Missing file-level comment disables clarityA short header describing what this config does and how to regenerate (e.g.
pnpm api:generate) would help future maintainers.packages/thirdweb/src/v2/contracts/types.ts (1)
1-6:params: unknown[]sacrifices type-safetyInstead of forcing all callers to cast, turn the type into a generic:
-export type ContractCall = { - contractAddress: string; - method: string; - params: unknown[]; - value?: string; -}; +export type ContractCall<Args extends readonly unknown[] = readonly unknown[]> = { + contractAddress: string; + method: string; + params: Args; + value?: string; +};This keeps flexibility while preserving inference for most call sites.
packages/api/src/exports/thirdweb.ts (1)
1-8: Public exports lack TSDoc andexportsgroupingBarrel files under
src/exportsform the library’s public interface; adding TSDoc (and grouping per feature) prevents accidental API surface creep and satisfies our guidelines.Consider annotating at least the top-level symbols:
/** * Re-export the generated client plus helper utilities. * * @example * ```ts * import { configure } from "@thirdweb-dev/api"; * ``` * @beta */packages/thirdweb/src/wallets/in-app/core/authentication/linkAccount.ts (1)
158-187: Consider refactoring to reduce code duplication.The
fetchUserProfilesfunction is nearly identical togetLinkedProfilesInternal(lines 119-156), with the main difference being that it accepts anauthTokenparameter instead of retrieving it from storage.While the implementation is correct and follows established patterns, consider refactoring to reduce duplication:
+async function fetchLinkedProfiles(options: { + client: ThirdwebClient; + ecosystem?: Ecosystem; + authToken: string; +}): Promise<Profile[]> { + const clientFetch = getClientFetch(options.client, options.ecosystem); + const IN_APP_URL = getThirdwebBaseUrl("inAppWallet"); + + const headers: Record<string, string> = { + Authorization: `Bearer iaw-auth-token:${options.authToken}`, + "Content-Type": "application/json", + }; + + const linkedAccountsResp = await clientFetch( + `${IN_APP_URL}/api/2024-05-05/accounts`, + { + headers, + method: "GET", + }, + ); + + if (!linkedAccountsResp.ok) { + const body = await linkedAccountsResp.json(); + throw new Error(body.message || "Failed to get linked accounts."); + } + + const { linkedAccounts } = await linkedAccountsResp.json(); + return (linkedAccounts ?? []) satisfies Profile[]; +} export async function getLinkedProfilesInternal({ client, ecosystem, storage, }: { client: ThirdwebClient; ecosystem?: Ecosystem; storage: ClientScopedStorage; }): Promise<Profile[]> { - const clientFetch = getClientFetch(client, ecosystem); - const IN_APP_URL = getThirdwebBaseUrl("inAppWallet"); const currentAccountToken = await storage.getAuthCookie(); if (!currentAccountToken) { throw new Error("Failed to get linked accounts, no user logged in"); } - - const headers: Record<string, string> = { - Authorization: `Bearer iaw-auth-token:${currentAccountToken}`, - "Content-Type": "application/json", - }; - - const linkedAccountsResp = await clientFetch( - `${IN_APP_URL}/api/2024-05-05/accounts`, - { - headers, - method: "GET", - }, - ); - - if (!linkedAccountsResp.ok) { - const body = await linkedAccountsResp.json(); - throw new Error(body.message || "Failed to get linked accounts."); - } - - const { linkedAccounts } = await linkedAccountsResp.json(); - - return (linkedAccounts ?? []) satisfies Profile[]; + return fetchLinkedProfiles({ + client, + ecosystem, + authToken: currentAccountToken, + }); } -export async function fetchUserProfiles(options: { - client: ThirdwebClient; - ecosystem?: Ecosystem; - authToken: string; -}): Promise<Profile[]> { - const clientFetch = getClientFetch(options.client, options.ecosystem); - const IN_APP_URL = getThirdwebBaseUrl("inAppWallet"); - - const headers: Record<string, string> = { - Authorization: `Bearer iaw-auth-token:${options.authToken}`, - "Content-Type": "application/json", - }; - - const linkedAccountsResp = await clientFetch( - `${IN_APP_URL}/api/2024-05-05/accounts`, - { - headers, - method: "GET", - }, - ); - - if (!linkedAccountsResp.ok) { - const body = await linkedAccountsResp.json(); - throw new Error(body.message || "Failed to get linked accounts."); - } - - const { linkedAccounts } = await linkedAccountsResp.json(); - - return (linkedAccounts ?? []) satisfies Profile[]; -} +export const fetchUserProfiles = fetchLinkedProfiles;packages/thirdweb/src/v2/contracts/write.ts (1)
28-45: Consider validating calls array is not emptyAdd validation to ensure the calls array contains at least one call before making the API request.
export async function write(options: WriteOptions) { + if (!options.calls || options.calls.length === 0) { + throw new Error("At least one contract call is required"); + } + const result = await writeContract({ baseUrl: getThirdwebBaseUrl("api"),packages/thirdweb/src/v2/wallets/passkey.ts (3)
46-46: Address the TODO comment for React Native supportThe TODO comment indicates that React Native support is needed. Consider creating a follow-up issue to implement the React Native version of this functionality.
Would you like me to create a GitHub issue to track React Native support for passkey authentication?
70-77: Duplicate relying party configurationThe same relying party configuration is duplicated for both "sign-in" and "sign-up" cases. This violates the DRY principle and increases maintenance burden.
+ const rp = { + id: options.passkeyDomain ?? window.location.hostname, + name: window.document.title, + }; + const storage = options.storeLastUsedPasskey ? storage : undefined; const authResult = await (async () => { switch (options.action) { case "sign-in": return loginWithPasskey({ client: options.client, passkeyClient, - rp: { - id: options.passkeyDomain ?? window.location.hostname, - name: options.passkeyDomain ?? window.document.title, - }, - storage: options.storeLastUsedPasskey ? storage : undefined, + rp, + storage, ecosystem: options.ecosystem, }); case "sign-up": return registerPasskey({ client: options.client, passkeyClient, - rp: { - id: options.passkeyDomain ?? window.location.hostname, - name: options.passkeyDomain ?? window.document.title, - }, - storage: options.storeLastUsedPasskey ? storage : undefined, + rp, + storage, ecosystem: options.ecosystem, }); } })();
38-86: Consider adding error handling for WebAuthn unsupported environmentsThe function doesn't handle cases where WebAuthn is not supported in the browser environment. This could lead to runtime errors in older browsers or environments without WebAuthn support.
Add a WebAuthn availability check:
export async function loginWithPasskey(options: LoginWithPasskeyOptions) { + if (!window.navigator.credentials || !window.PublicKeyCredential) { + throw new Error("WebAuthn is not supported in this environment"); + } const { PasskeyWebClient } = await import( "../../wallets/in-app/web/lib/auth/passkeys.js" ); // ... rest of implementation }packages/thirdweb/src/v2/wallets/otp.ts (1)
27-27: Inconsistent function naming in exampleThe example shows
Wallets.verifyCodebut the actual function is namedloginWithCode. This could confuse users.- const wallet = await Wallets.verifyCode({ + const wallet = await Wallets.loginWithCode({packages/thirdweb/src/v2/wallets/login.test.ts (4)
24-24: Use realistic test phone numberThe phone number
"+1111111111"may not be valid for testing. Consider using a proper test phone number format.- phoneNumber: "+1111111111", + phoneNumber: "+1234567890", // Use a valid test format
40-47: Clean up commented placeholder codeThe commented code contains placeholder descriptions that should either be implemented or removed to keep the test file clean.
- // rest of the sdk maps to HTTP API as close as possible: - - // Contract.prepareCall() - // Contract.write({ wallet, contractAddress, chainId, calls }) - // Contract.read({ contractAddress, chainId, calls }) - - // Transactions.send({ wallet, chainId, transaction }) - // Transactions.get(id)Either implement these test cases or remove the comments entirely.
61-68: Remove or implement server wallet test codeThe commented server wallet code suggests incomplete functionality. Either implement the test or remove the commented code.
- // const serverWalletAddress = await Wallets.createServerWallet({ - // client, - // identifier: "test", - // }) - // const serverWallet = Wallets.serverWallet({ - // client, - // serverWalletAddress, - // });If this functionality is planned, consider adding a TODO comment instead.
89-90: Remove incomplete transaction polling commentThe commented transaction polling code should be implemented or removed to keep the test clean.
- // poll for status - // Transactions.get(transactionId)packages/thirdweb/src/v2/wallets/user.ts (2)
20-28: Address the TODO comment about gateway APIThe TODO comment indicates this wallet generation logic will move to the gateway API. Consider tracking this as a technical debt item.
Would you like me to create a GitHub issue to track the migration of wallet generation to the gateway API?
21-21: Consider adding error handling for wallet generationThe wallet generation call doesn't have explicit error handling. While the underlying
generateWalletfunction likely handles errors, consider adding context-specific error handling.if (!address) { - const wallet = await generateWallet({ - client: args.client, - ecosystem: args.ecosystem, - authToken, - }); + try { + const wallet = await generateWallet({ + client: args.client, + ecosystem: args.ecosystem, + authToken, + }); + } catch (error) { + throw new Error(`Failed to generate wallet: ${error.message}`); + } address = wallet.address; }packages/api/src/configure.ts (1)
23-26: Consider adding type annotations for better clarityWhile the biome-ignore comments explain the
anyusage, consider adding JSDoc comments to document the parameter types more clearly./** * Custom JSON.stringify that handles bigint serialization * @param value - The value to stringify * @param replacer - Optional replacer function * @param space - Optional spacing parameter */ function stringify( // biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature value: any,packages/api/src/client/core/params.ts (1)
74-141: Complex but correct parameter mapping implementationThe function handles all parameter mapping scenarios correctly, including direct assignment, field mapping, special prefixes, and extra parameters. Consider adding inline comments to clarify the different mapping strategies for future maintainability.
packages/api/src/client/client/utils.ts (1)
296-301: Document JSON stringification behavior for object headersThe automatic JSON stringification of object header values assumes all objects should be serialized as JSON. This might not be appropriate for all use cases.
Consider making this behavior configurable or at least documenting it clearly in the function's JSDoc:
/** * Merges multiple header sources into a single Headers object. * Note: Object values are automatically JSON.stringify'd. * ... */packages/api/src/client/client/types.ts (1)
201-222: Consider simplifying OptionsLegacyParser typeThe
OptionsLegacyParsertype has deeply nested conditional types that are hard to understand and maintain. Consider refactoring into smaller, composable types for better readability.type WithBody<T> = T extends { body?: any } ? T : T & Pick<RequestOptions, "body">; type WithHeaders<T> = T extends { headers?: any } ? T : T & Pick<RequestOptions, "headers">; export type OptionsLegacyParser< TData = unknown, ThrowOnError extends boolean = boolean, TResponseStyle extends ResponseStyle = "fields", > = OmitKeys<RequestOptions<TResponseStyle, ThrowOnError>, "url"> & WithBody<WithHeaders<TData>>;
| // Incremental builds | ||
| // NOTE: Enabling incremental builds speeds up `tsc`. Keep in mind though that it does not reliably bust the cache when the `tsconfig.json` file changes. | ||
| "allowJs": false, | ||
| "allowSyntheticDefaultImports": true, | ||
| "checkJs": false, | ||
|
|
||
| // Interop constraints | ||
| "esModuleInterop": false, | ||
| "exactOptionalPropertyTypes": false, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "importHelpers": true, | ||
| // Incremental builds | ||
| // NOTE: Enabling incremental builds speeds up `tsc`. Keep in mind though that it does not reliably bust the cache when the `tsconfig.json` file changes. | ||
| "incremental": false, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicated incremental-build note & contradictory setting
The “Incremental builds” comment appears twice and claims the option is enabled, yet "incremental" is set to false.
That’s misleading for future maintainers.
- // Incremental builds
- // NOTE: Enabling incremental builds speeds up `tsc`. …
- "incremental": false,
+ // Incremental builds are disabled because the cache does not bust reliably.
+ "incremental": false,Delete the earlier duplicate comment block (lines 4-6) or enable the flag instead.
🤖 Prompt for AI Agents
In packages/api/tsconfig.base.json between lines 4 and 18, there is a duplicated
comment about incremental builds and the "incremental" option is set to false,
which contradicts the comment. To fix this, remove the first duplicate comment
block around lines 4-6 or alternatively set "incremental" to true to match the
comment. Ensure the comments and settings are consistent to avoid confusion for
future maintainers.
| ```ts | ||
| import { writeContract } from "@thirdweb-dev/api"; | ||
|
|
||
| const result = await sendTransaction({ |
There was a problem hiding this comment.
Fix the import/function call mismatch.
The example imports writeContract but calls sendTransaction, which will cause a runtime error.
Apply this fix:
-import { writeContract } from "@thirdweb-dev/api";
+import { sendTransaction } from "@thirdweb-dev/api";Or alternatively, if writeContract is the correct function:
-const result = await sendTransaction({
+const result = await writeContract({📝 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.
| ```ts | |
| import { writeContract } from "@thirdweb-dev/api"; | |
| const result = await sendTransaction({ | |
| import { sendTransaction } from "@thirdweb-dev/api"; | |
| const result = await sendTransaction({ |
🤖 Prompt for AI Agents
In packages/api/README.md around lines 18 to 21, the code imports writeContract
but calls sendTransaction, causing a runtime error. Fix this by either changing
the import to import sendTransaction from the correct module or update the
function call to writeContract if that is the intended function. Ensure the
import and function call match to avoid runtime issues.
| "useImportExtensions": { | ||
| "fix": "safe", | ||
| "level": "error", | ||
| "options": { | ||
| "forceJsExtensions": true | ||
| } | ||
| } |
There was a problem hiding this comment.
forceJsExtensions may break non-relative (bare module specifier) imports
The rule is applied globally, so it will enforce adding .js even on bare-specifier paths such as "react" or "@thirdweb-dev/sdk".
Biome’s own docs warn that the rule is only safe when "ignorePackageImports": true is set (or when limited to relative imports).
Consider narrowing the rule scope or adding
{
"forceJsExtensions": true,
"ignorePackageImports": true
}to avoid turning valid bare imports into invalid ones.
🤖 Prompt for AI Agents
In packages/api/biome.json around lines 6 to 12, the "forceJsExtensions" option
is set to true globally, which incorrectly adds ".js" extensions to bare module
specifiers like "react" or "@thirdweb-dev/sdk". To fix this, add
"ignorePackageImports": true to the options object to ensure the rule only
applies to relative imports and does not break bare imports.
| export default defineConfig({ | ||
| input: "https://api.thirdweb-dev.com/openapi.json", | ||
| output: { format: "biome", lint: "biome", path: "src/client" }, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin the OpenAPI schema version to guarantee reproducible builds
Fetching https://api.thirdweb-dev.com/openapi.json on every generation ties build determinism to a live, mutable endpoint.
If the spec changes, all generated client types will drift without code review.
Store a copy in-repo (or pin to a tagged commit URL) and update intentionally.
- input: "https://api.thirdweb-dev.com/openapi.json",
+ // Prefer a pinned revision for reproducibility
+ input: "./schemas/[email protected]",🏁 Script executed:
#!/bin/bash
# Search for a local 'schemas' directory
fd --type d --max-depth 3 schemas
# List all JSON files in the repo
fd --extension json
# Find JSON files mentioning 'openapi'
rg -l 'openapi' -g '*.json' || trueLength of output: 13458
Pin OpenAPI schema for reproducible builds
A live URL will produce drifting client types whenever the remote spec changes. To guarantee deterministic generations:
- Add a copy of the OpenAPI spec under
packages/api/schemas/, e.g.
packages/api/schemas/[email protected] - Update the config in
packages/api/openapi-ts.config.ts:
export default defineConfig({
- input: "https://api.thirdweb-dev.com/openapi.json",
+ // Point to a pinned, in-repo spec for stable builds
+ input: "./schemas/[email protected]",
output: { format: "biome", lint: "biome", path: "src/client" },
});• Create the schemas folder next to openapi-ts.config.ts if it doesn’t exist.
• Commit the downloaded JSON with a clear date-or-version suffix.
📝 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.
| export default defineConfig({ | |
| input: "https://api.thirdweb-dev.com/openapi.json", | |
| output: { format: "biome", lint: "biome", path: "src/client" }, | |
| }); | |
| export default defineConfig({ | |
| // Point to a pinned, in-repo spec for stable builds | |
| input: "./schemas/[email protected]", | |
| output: { format: "biome", lint: "biome", path: "src/client" }, | |
| }); |
🤖 Prompt for AI Agents
In packages/api/openapi-ts.config.ts around lines 3 to 6, the OpenAPI schema is
currently referenced via a live URL, which causes non-reproducible builds due to
changes in the remote spec. To fix this, download the OpenAPI JSON spec and save
it under packages/api/schemas/ with a clear date or version suffix, such as
[email protected]. Then update the input path in the config to
point to this local file instead of the URL. Also, create the schemas folder if
it does not exist and commit the downloaded JSON file to the repo.
| export * as Contracts from "../v2/contracts/index.js"; | ||
|
|
||
| // namespaced API | ||
| export * as Wallets from "../v2/wallets/index.js"; | ||
| export { deploySmartAccount } from "../wallets/smart/lib/signing.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wildcard namespace re-exports balloon bundle size
export * as Contracts / Wallets forwards every internal symbol, including low-level helpers that were never meant for public API.
Once published these names are permanent, making future de-scoping a breaking change.
Audit the two index files and create explicit export lists instead, e.g.
-export * as Contracts from "../v2/contracts/index.js";
+export {
+ readContract,
+ writeContract,
+ type ContractCall,
+} from "../v2/contracts/index.js";Same for Wallets.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/thirdweb/src/exports/thirdweb.ts around lines 300 to 304, the use of
wildcard namespace exports for Contracts and Wallets forwards all internal
symbols, including unintended low-level helpers, which increases bundle size and
risks breaking changes later. To fix this, audit the exports in
../v2/contracts/index.js and ../v2/wallets/index.js, then replace the wildcard
exports with explicit named exports listing only the intended public API symbols
for both Contracts and Wallets.
| it.skip("login with OTP", { retry: 0 }, async () => { | ||
| const client = createThirdwebClient({ | ||
| clientId: "...", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace placeholder client ID with test constant
The test uses a placeholder client ID ("..."), which would cause the test to fail if enabled. Use a proper test client ID or the existing TEST_CLIENT.
- const client = createThirdwebClient({
- clientId: "...",
- });
+ const client = TEST_CLIENT;📝 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.
| it.skip("login with OTP", { retry: 0 }, async () => { | |
| const client = createThirdwebClient({ | |
| clientId: "...", | |
| }); | |
| it.skip("login with OTP", { retry: 0 }, async () => { | |
| - const client = createThirdwebClient({ | |
| - clientId: "...", | |
| - }); | |
| + const client = TEST_CLIENT; |
🤖 Prompt for AI Agents
In packages/thirdweb/src/v2/wallets/login.test.ts around lines 16 to 19, replace
the placeholder client ID string "..." with the proper test client ID constant,
such as TEST_CLIENT, to ensure the test runs correctly when enabled.
| } | ||
| } | ||
|
|
||
| finalError = finalError || ({} as string); |
There was a problem hiding this comment.
Type inconsistency in error default assignment
The fallback assigns an empty object to finalError which is expected to be a string. Consider using an empty string or a more appropriate default error value.
-finalError = finalError || ({} as string);
+finalError = finalError || "";📝 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.
| finalError = finalError || ({} as string); | |
| finalError = finalError || ""; |
🤖 Prompt for AI Agents
In packages/api/src/client/client/client.ts at line 158, the fallback assignment
sets finalError to an empty object cast as a string, causing a type
inconsistency. Change the fallback to assign an empty string or another suitable
string value to finalError to maintain type consistency.
| if (value === undefined || value === null) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Handle undefined/null path parameters more explicitly
When path parameters are undefined or null, the code continues without replacing the placeholder, which could leave {paramName} in the final URL. Consider throwing an error or providing a default value to ensure valid URLs.
if (value === undefined || value === null) {
- continue;
+ throw new Error(`Required path parameter '${name}' is ${value}`);
}📝 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 (value === undefined || value === null) { | |
| continue; | |
| } | |
| if (value === undefined || value === null) { | |
| throw new Error(`Required path parameter '${name}' is ${value}`); | |
| } |
🤖 Prompt for AI Agents
In packages/api/src/client/client/utils.ts around lines 49 to 51, the code
currently skips replacing path parameters if their value is undefined or null,
which can leave placeholders like {paramName} in the final URL. To fix this,
modify the code to explicitly check for undefined or null path parameters and
either throw a descriptive error indicating the missing parameter or assign a
sensible default value before continuing. This ensures the final URL is always
valid and complete.
| export const getParseAs = ( | ||
| contentType: string | null, | ||
| ): Exclude<Config["parseAs"], "auto"> => { | ||
| if (!contentType) { | ||
| // If no Content-Type header is provided, the best we can do is return the raw response body, | ||
| // which is effectively the same as the 'stream' option. | ||
| return "stream"; | ||
| } | ||
|
|
||
| const cleanContent = contentType.split(";")[0]?.trim(); | ||
|
|
||
| if (!cleanContent) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| cleanContent.startsWith("application/json") || | ||
| cleanContent.endsWith("+json") | ||
| ) { | ||
| return "json"; | ||
| } | ||
|
|
||
| if (cleanContent === "multipart/form-data") { | ||
| return "formData"; | ||
| } | ||
|
|
||
| if ( | ||
| ["application/", "audio/", "image/", "video/"].some((type) => | ||
| cleanContent.startsWith(type), | ||
| ) | ||
| ) { | ||
| return "blob"; | ||
| } | ||
|
|
||
| if (cleanContent.startsWith("text/")) { | ||
| return "text"; | ||
| } | ||
|
|
||
| return; | ||
| }; |
There was a problem hiding this comment.
Fix return type inconsistency
The function signature indicates it returns Exclude<Config["parseAs"], "auto"> but it can return undefined on lines 160 and 186. This violates the type contract and could cause runtime errors.
export const getParseAs = (
contentType: string | null,
-): Exclude<Config["parseAs"], "auto"> => {
+): Exclude<Config["parseAs"], "auto"> | undefined => {Alternatively, provide a default return value like "stream" to match the type signature.
📝 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.
| export const getParseAs = ( | |
| contentType: string | null, | |
| ): Exclude<Config["parseAs"], "auto"> => { | |
| if (!contentType) { | |
| // If no Content-Type header is provided, the best we can do is return the raw response body, | |
| // which is effectively the same as the 'stream' option. | |
| return "stream"; | |
| } | |
| const cleanContent = contentType.split(";")[0]?.trim(); | |
| if (!cleanContent) { | |
| return; | |
| } | |
| if ( | |
| cleanContent.startsWith("application/json") || | |
| cleanContent.endsWith("+json") | |
| ) { | |
| return "json"; | |
| } | |
| if (cleanContent === "multipart/form-data") { | |
| return "formData"; | |
| } | |
| if ( | |
| ["application/", "audio/", "image/", "video/"].some((type) => | |
| cleanContent.startsWith(type), | |
| ) | |
| ) { | |
| return "blob"; | |
| } | |
| if (cleanContent.startsWith("text/")) { | |
| return "text"; | |
| } | |
| return; | |
| }; | |
| export const getParseAs = ( | |
| contentType: string | null, | |
| ): Exclude<Config["parseAs"], "auto"> | undefined => { | |
| if (!contentType) { | |
| // If no Content-Type header is provided, the best we can do is return the raw response body, | |
| // which is effectively the same as the 'stream' option. | |
| return "stream"; | |
| } | |
| const cleanContent = contentType.split(";")[0]?.trim(); | |
| if (!cleanContent) { | |
| return; | |
| } | |
| if ( | |
| cleanContent.startsWith("application/json") || | |
| cleanContent.endsWith("+json") | |
| ) { | |
| return "json"; | |
| } | |
| if (cleanContent === "multipart/form-data") { | |
| return "formData"; | |
| } | |
| if ( | |
| ["application/", "audio/", "image/", "video/"].some((type) => | |
| cleanContent.startsWith(type), | |
| ) | |
| ) { | |
| return "blob"; | |
| } | |
| if (cleanContent.startsWith("text/")) { | |
| return "text"; | |
| } | |
| return; | |
| }; |
🤖 Prompt for AI Agents
In packages/api/src/client/client/utils.ts between lines 148 and 187, the
function getParseAs has a return type excluding undefined, but it currently
returns undefined on lines 160 and 186. To fix this, ensure the function always
returns a valid value of the expected type by replacing those undefined returns
with a default value like "stream" to maintain type consistency and avoid
runtime errors.
| switch (auth.in) { | ||
| case "query": | ||
| if (!options.query) { | ||
| options.query = {}; | ||
| } | ||
| options.query[name] = token; | ||
| break; |
There was a problem hiding this comment.
Consider security implications of query parameter authentication
Adding authentication tokens as query parameters (lines 206-211) can expose sensitive data in server logs, browser history, and proxy logs. Consider logging a warning or documenting this security consideration.
case "query":
if (!options.query) {
options.query = {};
}
+ // Security warning: Tokens in query parameters may be logged
+ console.warn(`Adding auth token '${name}' to query parameters. This may expose sensitive data in logs.`);
options.query[name] = token;
break;📝 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.
| switch (auth.in) { | |
| case "query": | |
| if (!options.query) { | |
| options.query = {}; | |
| } | |
| options.query[name] = token; | |
| break; | |
| switch (auth.in) { | |
| case "query": | |
| if (!options.query) { | |
| options.query = {}; | |
| } | |
| // Security warning: Tokens in query parameters may be logged | |
| console.warn(`Adding auth token '${name}' to query parameters. This may expose sensitive data in logs.`); | |
| options.query[name] = token; | |
| break; |
🤖 Prompt for AI Agents
In packages/api/src/client/client/utils.ts around lines 205 to 211, the code
adds authentication tokens as query parameters, which can expose sensitive data
in logs and browser history. To address this, add a warning log message when
tokens are added to query parameters to highlight the security risk, or include
a clear comment documenting this security consideration for future maintainers.
| * ```typescript | ||
| * import { createThirdwebClient } from "thirdweb/v2"; | ||
| * | ||
| * const thirdwebClient = createThirdwebClient({ |
There was a problem hiding this comment.
reminder to fix this example
| * | ||
| * ## Get a transaction by ID | ||
| * ```typescript | ||
| * import { Client, Transactions } from "thirdweb/v2"; |
There was a problem hiding this comment.
thinking more and more that we can get away with just import from "thirdweb" with the new namespaces. Just so much simpler. over time we can deprecate/move things out of the main export
| confirmedAt: transaction.confirmedAt, | ||
| confirmedAtBlockNumber: transaction.confirmedAtBlockNumber, | ||
| cancelledAt: transaction.cancelledAt, | ||
| errorMessage: transaction.errorMessage, |
There was a problem hiding this comment.
thinking we probably need a 'status' top level here, in the API too
| })); | ||
| } | ||
|
|
||
| export declare namespace list { |
| * import { Client, Transactions, Wallets } from "thirdweb/v2"; | ||
| * | ||
| * const thirdwebClient = Client.init({ | ||
| * clientId: "YOUR_CLIENT_ID", |
There was a problem hiding this comment.
you're not initializing the client in the other 2 examples
| if (!transactionIds) { | ||
| throw new Error("Failed to send transactions: no transaction ids"); | ||
| } | ||
| return transactionIds; |
There was a problem hiding this comment.
tempted to change the API to be a single transaction id, feels simpler
| @@ -0,0 +1 @@ | |||
| export type EthereumAddress = `0x${string}`; // Joaquim do you think we should use the types from the existing SDK or create entirely new ones so we don't cross over at all? | |||
There was a problem hiding this comment.
i think if we keep things namespaced we can get away with new types.
but tbh for addresses i still fill like 0x{string} is overkill. super annoying when you get back nested structs from api that you're just trying to feed back into a typed function. I would just use string, and validate at runtime
| type SendCodeOptions, | ||
| sendCode, | ||
| type VerifyCodeOptions, | ||
| verifyLoginCode, |
There was a problem hiding this comment.
not loginWithCode?
| type VerifyCodeOptions, | ||
| verifyLoginCode, | ||
| type SendLoginCodeOptions, | ||
| sendLoginCode, |
There was a problem hiding this comment.
we can do sendVerificationCode if you think sendCode is confusing
|
This PR has been inactive for 7 days. It is now marked as stale and will be closed in 2 days if no further activity occurs. |
PR-Codex overview
This PR focuses on enhancing the
@thirdweb-dev/apipackage by adding new features, improving existing functionalities, and updating configurations to support better client-server interactions.Detailed summary
ContractCallinpackages/thirdweb/src/v2/contracts/types.ts.ContractCallandReadOptionsfromindex.ts.packages/thirdweb/src/v2/wallets.packages/thirdweb/src/v2/wallets/otp.ts.packages/api/src/client/client.gen.ts.packages/thirdweb/src/v2/wallets/oauth.ts.packages/api/src/client/core/bodySerializer.ts.Summary by CodeRabbit
New Features
@thirdweb-dev/apipackage with a fully typed OpenAPI-based TypeScript client for blockchain contracts, wallets, transactions, and token operations.Bug Fixes
Chores