feat: turnkey wallet #552
feat: turnkey wallet #552
Conversation
WalkthroughThis update introduces a new Turnkey wallet strategy package, supporting both OTP and OAuth authentication for Injective Protocol. It adds new wallet types, refactors wallet metadata handling, and implements strategy and utility classes for Turnkey integration. Documentation, configuration, and error handling are expanded, and several existing wallet strategies are updated for compatibility with the new metadata structure. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant WalletStrategy
participant TurnkeyOtpWalletStrategy
participant TurnkeyOauthWalletStrategy
participant BaseTurnkeyWalletStrategy
participant TurnkeyWallet
participant TurnkeyIframeClient
participant HttpRestClient
App->>WalletStrategy: createStrategy({ wallet, metadata })
WalletStrategy->>TurnkeyOtpWalletStrategy: new (args)
WalletStrategy->>TurnkeyOauthWalletStrategy: new (args)
Note over TurnkeyOtpWalletStrategy,TurnkeyOauthWalletStrategy: Both extend BaseTurnkeyWalletStrategy
App->>TurnkeyOtpWalletStrategy: enable()
TurnkeyOtpWalletStrategy->>BaseTurnkeyWalletStrategy: enable()
BaseTurnkeyWalletStrategy->>TurnkeyWallet: getSessionOrConfirm()
TurnkeyWallet->>TurnkeyIframeClient: getSession()
TurnkeyWallet->>HttpRestClient: API call (OTP/OAuth login)
HttpRestClient-->>TurnkeyWallet: credentialBundle/session
TurnkeyWallet-->>BaseTurnkeyWalletStrategy: session info
App->>TurnkeyOtpWalletStrategy: getAddresses()
TurnkeyOtpWalletStrategy->>BaseTurnkeyWalletStrategy: getAddresses()
BaseTurnkeyWalletStrategy->>TurnkeyWallet: getAccounts()
TurnkeyWallet->>TurnkeyIframeClient: fetch accounts
TurnkeyIframeClient-->>TurnkeyWallet: accounts
TurnkeyWallet-->>BaseTurnkeyWalletStrategy: addresses
App->>TurnkeyOtpWalletStrategy: sendTransaction()
TurnkeyOtpWalletStrategy->>BaseTurnkeyWalletStrategy: sendTransaction()
BaseTurnkeyWalletStrategy->>TurnkeyWallet: sign/broadcast transaction
TurnkeyWallet->>HttpRestClient: broadcast transaction
HttpRestClient-->>TurnkeyWallet: tx result
TurnkeyWallet-->>BaseTurnkeyWalletStrategy: tx result
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.tsOops! Something went wrong! :( ESLint: 7.32.0 Error: Error while loading rule 'jest/unbound-method': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser. packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.tsOops! Something went wrong! :( ESLint: 7.32.0 Error: Error while loading rule 'jest/unbound-method': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser. packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.tsOops! Something went wrong! :( ESLint: 7.32.0 Error: Error while loading rule 'jest/unbound-method': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code Graph Analysis (2)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (7)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (5)
🪛 Biome (1.9.4)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts[error] 14-98: Avoid classes that contain only static members. Prefer using simple functions instead of classes with only static members. (lint/complexity/noStaticOnlyClass) packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts[error] 12-72: Avoid classes that contain only static members. Prefer using simple functions instead of classes with only static members. (lint/complexity/noStaticOnlyClass) 🔇 Additional comments (2)
🪧 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 (
|
|
Hmm for some reason this doesn't work when I branch off dev, but it does off master. This branch works for me after building |
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
3 times, most recently
from
8b77d67 to
ccf5155
Compare
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
from
ccf5155 to
aded68e
Compare
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
from
aded68e to
8d59edc
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (32)
packages/wallets/wallet-turnkey/tsconfig.json (1)
1-3: Base TypeScript config extension is set up.Extending from the monorepo root
tsconfig.jsonis appropriate. You may optionally scope the compilation by adding:{ "extends": "../../../tsconfig.json", + "include": ["src"], + "exclude": ["node_modules", "dist"] }to limit the compiler to this package’s source files.
examples/turnkey/server/README.md (2)
1-1: Add a top-level heading
README files should start with a clear H1 heading to describe the example (e.g.,# Turnkey Server Example).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
1-1: First line in a file should be a top-level heading
null(MD041, first-line-heading, first-line-h1)
1-4: Specify language for fenced code blocks
For better readability and syntax highlighting, annotate each code fence with a language (e.g., usebash orsh instead of plain ```).Also applies to: 6-8
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
1-1: First line in a file should be a top-level heading
null(MD041, first-line-heading, first-line-h1)
examples/turnkey/app/README.md (2)
1-1: Clarify example purpose in heading
Consider renaming the H1 to reflect the Turnkey wallet integration, for example:# Turnkey Wallet Vue 3 + TypeScript + Vite Example
1-6: Expand README with usage instructions
The README currently lacks steps to install and run the example. Add a “## Usage” section, for instance:## Usage ```bash npm install npm run dev open http://localhost:5173</blockquote></details> <details> <summary>examples/turnkey/app/index.html (2)</summary><blockquote> `7-7`: **Update the document title** Replace the generic `<title>Vite + Vue + TS</title>` with something that reflects the example’s purpose, e.g.: ```html <title>Turnkey Wallet Example</title>
4-6: Enhance metadata
Consider adding a<meta name="description" content="Demo of Turnkey wallet integration with Injective Labs SDK">and updating the favicon to a custom icon instead of the default Vite logo for clarity.examples/turnkey/server/src/index.ts (1)
24-32: Make server port configurable via environment variablesThe server port is hardcoded to 3000. For better flexibility across different environments, consider making it configurable.
serve( { fetch: app.fetch, - port: 3000, + port: process.env.PORT ? parseInt(process.env.PORT, 10) : 3000, }, (info) => { console.log(`Server is running on http://localhost:${info.port}`) }, )Also consider adding error handling for the server startup.
examples/turnkey/server/package.json (2)
2-4: Use a more descriptive package name
The current"name": "server"is overly generic and may collide with other packages. Consider renaming it to something like"turnkey-server-example"to clearly indicate its purpose.
17-21: Add a license field
This example package doesn't specify a license. Adding"license": "Apache-2.0"(or another appropriate license) will clarify usage terms.packages/wallets/wallet-turnkey/package.json (1)
60-69: Consider moving runtime dependencies to peerDependencies
Libraries like@injectivelabs/wallet-base,@injectivelabs/exceptions, andviemare core dependencies that host applications will also depend on. Listing them aspeerDependencieshelps avoid duplicate installations and bundle bloat.examples/turnkey/app/package.json (1)
12-16: Preferfile:overlink:for broader npm compatibility
Usinglink:../../packages/...works only with Yarn workspaces. If consumers use npm or pnpm, consider switching tofile:../../packages/...or documenting the Yarn requirement in the README.examples/turnkey/app/src/style.css (1)
1-14: Encapsulate theme colors with CSS variables
Defining custom properties (e.g.,--color-bg,--color-text) will make it easier to maintain and extend light/dark themes across components.packages/wallets/wallet-turnkey/README.md (5)
7-7: Fix the license badge URL
Currently the license badge points toexpress.svgand has an empty link. Update it to reference the correct Apache-2.0 license for this package:-[]() +[](https://github.com/injective-labs/injective-ts/blob/main/packages/wallets/wallet-turnkey/LICENSE)
26-27: Refine wording: remove redundant “of”
Change:…contain all of the types…To:
…contain all the types…🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~27-~27: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...(ALL_OF_THE)
30-30: Eliminate double punctuation
Replace:…to send 1 INJ.:With:
…to send 1 INJ:🧰 Tools
🪛 LanguageTool
[formatting] ~30-~30: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
110-112: Add alt text to the image and use an en dash for year range
- Provide meaningful
alttext for accessibility.- Replace
2021 - 2022with2021–2022.- <a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px;" /></a> + <a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px;" /></a> … - Copyright © 2021 - 2022 Injective Labs Inc. + Copyright © 2021–2022 Injective Labs Inc.🧰 Tools
🪛 LanguageTool
[typographical] ~110-~110: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
110-110: Bare URL used
null(MD034, no-bare-urls)
112-112: Images should have alternate text (alt text)
null(MD045, no-alt-text)
114-116: Remove the colon after “under”
Replace:Originally released by Injective Labs Inc. under: Apache License…With:
Originally released by Injective Labs Inc. under Apache License…🧰 Tools
🪛 LanguageTool
[typographical] ~114-~114: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
examples/turnkey/app/src/components/LoginForm.vue (2)
6-7: Inconsistent variable naming conventionThe variable
OTPuses uppercase naming, which is inconsistent with JavaScript/TypeScript camelCase conventions.Rename for consistency:
-const OTP = ref('') +const otp = ref('')Make sure to update all references to this variable throughout the component.
99-99: Address TODO commentThere's an unaddressed TODO comment in the template.
Either implement the feature or remove the comment if it's no longer applicable.
examples/turnkey/server/src/turnkey-route.ts (2)
38-46: Improve email validator in schemaThe schema validates that
Enhance validation:
zValidator( 'json', z.object({ - otpId: z.string(), + otpId: z.string().min(1, "OTP ID is required"), otpCode: z.string(), targetPublicKey: z.string(), suborgID: z.string(), sessionLengthSeconds: z.number().optional(), }), ),
60-65: Improve providerName validation in oauthLogin schemaThe schema accepts any string for
providerName, but it should be restricted to supported providers only.Enhance validation:
z.object({ oidcToken: z.string(), - providerName: z.string(), + providerName: z.enum(['google']), // Restrict to supported providers only targetPublicKey: z.string(), expirationSeconds: z.number().optional(), }),examples/turnkey/app/src/injective-clients.ts (3)
68-74: Inconsistent endpoint handling for archiver APIWhile most API endpoints are derived from the network configuration, the archiver API URLs are hardcoded.
Consider extracting these URLs to environment variables or network configuration:
indexerGrpcArchiverApi: new IndexerGrpcArchiverApi( - IS_MAINNET - ? 'https://k8s.mainnet.archiver.grpc-web.injective.network' - : IS_TESTNET - ? 'https://k8s.testnet.archiver.grpc-web.injective.network' - : endpoints.indexer, + endpoints.archiver ?? ( + IS_MAINNET + ? 'https://k8s.mainnet.archiver.grpc-web.injective.network' + : IS_TESTNET + ? 'https://k8s.testnet.archiver.grpc-web.injective.network' + : endpoints.indexer + ), ),
63-65: Inconsistent URL path formattingThe
indexerRestSpotChronosApiURL has a trailing slash, while other endpoints don't have this trailing slash.For consistency:
indexerRestSpotChronosApi: new IndexerRestSpotChronosApi( - `${endpoints.chronos}/api/chronos/v1/spot/`, + `${endpoints.chronos}/api/chronos/v1/spot`, ),
28-47: Environment variable overrides only applied for mainnetThe function only applies environment variable overrides for mainnet, but not for other networks.
Consider extending environment variable support to all networks:
export function getInjectiveClients(network: Network) { const endpoints = (() => { - if (network === 'mainnet') { - const defaultEndpoints = getNetworkEndpoints(Network.Mainnet) - const endpoints: NetworkEndpoints = { - ...defaultEndpoints, - cacheGrpc: - import.meta.env.VITE_ENDPOINT_CACHE_GRPC || - defaultEndpoints.cacheGrpc, - grpc: import.meta.env.VITE_ENDPOINT_GRPC || defaultEndpoints.grpc, - rest: import.meta.env.VITE_ENDPOINT_REST || defaultEndpoints.rest, - indexer: - import.meta.env.VITE_ENDPOINT_INDEXER || defaultEndpoints.indexer, - chronos: - import.meta.env.VITE_ENDPOINT_CHRONOS || defaultEndpoints.chronos, - } - return endpoints - } - return getNetworkEndpoints(network) + const defaultEndpoints = getNetworkEndpoints(network) + const endpoints: NetworkEndpoints = { + ...defaultEndpoints, + cacheGrpc: + import.meta.env.VITE_ENDPOINT_CACHE_GRPC || + defaultEndpoints.cacheGrpc, + grpc: import.meta.env.VITE_ENDPOINT_GRPC || defaultEndpoints.grpc, + rest: import.meta.env.VITE_ENDPOINT_REST || defaultEndpoints.rest, + indexer: + import.meta.env.VITE_ENDPOINT_INDEXER || defaultEndpoints.indexer, + chronos: + import.meta.env.VITE_ENDPOINT_CHRONOS || defaultEndpoints.chronos, + } + return endpoints })()examples/turnkey/app/src/App.vue (1)
45-52: Clean up URL hash after consumingid_token.The token remains visible in the address bar, allowing it to be copied into logs or screenshots.
After storing it, callhistory.replaceStateto clear the fragment.oidcToken.value = params.idToken +if (params.idToken) { + window.history.replaceState({}, '', window.location.pathname) +}examples/turnkey/app/src/components/Connected.vue (3)
7-13: Initial msg defaults won’t follow address changes.
msgValues.from/tocapture the currentaddress.valueonly once; if the reactiveaddresslater changes (e.g. organisation switch) the defaults become stale.Either:
- Derive them lazily in
handleSendClick, or- Watch
addressand update the object.watch(address, (addr) => { msgValues.value.to = addr msgValues.value.from = addr })
18-66: Minor UX / robustness improvements inhandleSendClick.
- Previous hash is never cleared – a failing tx leaves an old hash displayed.
- Inputs accept any string; zero-length or non-numeric amounts will pass the
!valuecheck.isLoading.value = true -txHash.value = txHash.value // unchanged +txHash.value = '' // clear previous hash … if (isNaN(Number(msgValues.value.amount))) { throw new Error('Amount must be numeric') }
116-120: Await the async side-effect or handle errors.
updateUserWithTokenis fired but not awaited; unhandled rejections will surface in the console and Vue cannot report them.onMounted(async () => { - if (oidcToken.value) { - updateUserWithToken(oidcToken.value) - } + if (oidcToken.value) { + try { + await updateUserWithToken(oidcToken.value) + } catch (e) { + console.error(e) + } + } })packages/wallets/wallet-turnkey/src/strategy/strategy.ts (3)
178-180: Redundant null-checks – replace with optional chaining.
if (session && session?.expiry)can be simplified to the safer optional-chain access flagged by Biome.- if (session && session?.expiry) { - this.expiry = session.expiry - } + if (session?.expiry) { + this.expiry = session.expiry + }🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
552-561: Copy-paste error in exception context.The error mentions “Magic wallet” instead of Turnkey.
- 'You have to pass endpoints.grpc within the options for using Magic wallet', + 'You have to pass endpoints.grpc within the options for using Turnkey wallet',
405-410: Preferundefinedovervoidin union return type.
Promise<string | void>is flagged by Biome –voidis confusing for consumers. Useundefinedfor absence of a value.- }): Promise<string | void> { + }): Promise<string | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/turnkey/app/public/vite.svgis excluded by!**/*.svgexamples/turnkey/app/src/assets/vue.svgis excluded by!**/*.svgexamples/turnkey/app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/turnkey/server/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
examples/turnkey/app/.gitignore(1 hunks)examples/turnkey/app/README.md(1 hunks)examples/turnkey/app/index.html(1 hunks)examples/turnkey/app/package.json(1 hunks)examples/turnkey/app/src/App.vue(1 hunks)examples/turnkey/app/src/components/Connected.vue(1 hunks)examples/turnkey/app/src/components/LoginForm.vue(1 hunks)examples/turnkey/app/src/constants.ts(1 hunks)examples/turnkey/app/src/injective-clients.ts(1 hunks)examples/turnkey/app/src/main.ts(1 hunks)examples/turnkey/app/src/reactives.ts(1 hunks)examples/turnkey/app/src/style.css(1 hunks)examples/turnkey/app/src/utils.ts(1 hunks)examples/turnkey/app/src/vite-env.d.ts(1 hunks)examples/turnkey/app/tsconfig.app.json(1 hunks)examples/turnkey/app/tsconfig.json(1 hunks)examples/turnkey/app/tsconfig.node.json(1 hunks)examples/turnkey/app/vite.config.ts(1 hunks)examples/turnkey/server/.gitignore(1 hunks)examples/turnkey/server/README.md(1 hunks)examples/turnkey/server/package.json(1 hunks)examples/turnkey/server/src/index.ts(1 hunks)examples/turnkey/server/src/lib/turnkey-server.ts(1 hunks)examples/turnkey/server/src/lib/utils.ts(1 hunks)examples/turnkey/server/src/turnkey-route.ts(1 hunks)examples/turnkey/server/tsconfig.json(1 hunks)packages/exceptions/src/exceptions/types/modules.ts(1 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(1 hunks)packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.esm.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
examples/turnkey/server/src/index.ts (1)
examples/turnkey/server/src/turnkey-route.ts (1)
turnkeyRoute(11-74)
examples/turnkey/server/src/turnkey-route.ts (2)
examples/turnkey/server/src/lib/turnkey-server.ts (3)
initEmailOtp(105-136)verifyEmailOtp(138-180)oauthLogin(182-237)packages/wallets/wallet-turnkey/src/strategy/strategy.ts (1)
oauthLogin(405-448)
examples/turnkey/server/src/lib/turnkey-server.ts (2)
examples/turnkey/server/src/lib/utils.ts (1)
decodeJwt(3-11)packages/wallets/wallet-turnkey/src/strategy/strategy.ts (1)
oauthLogin(405-448)
🪛 markdownlint-cli2 (0.17.2)
examples/turnkey/server/README.md
1-1: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
1-1: First line in a file should be a top-level heading
null
(MD041, first-line-heading, first-line-h1)
6-6: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
packages/wallets/wallet-turnkey/README.md
110-110: Bare URL used
null
(MD034, no-bare-urls)
112-112: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
117-117: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
examples/turnkey/app/tsconfig.node.json
[error] 9-9: JSON standard does not allow comments.
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-16: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 16-16: JSON standard does not allow comments.
(parse)
[error] 17-17: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 17-17: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 17-17: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 17-17: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 18-18: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 18-18: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 18-18: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 18-18: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 22-22: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 23-23: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 23-23: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 23-23: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 23-24: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
examples/turnkey/app/tsconfig.app.json
[error] 6-6: JSON standard does not allow comments.
(parse)
[error] 7-7: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 7-7: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 7-7: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 7-7: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 8-8: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~27-~27: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~30-~30: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~110-~110: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~114-~114: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🔇 Additional comments (21)
examples/turnkey/app/src/constants.ts (1)
1-5: Constants definition is clear and immutable.Using
as constensures the storage keys are read-only, and theturnkey-prefix is consistently applied. This promotes type safety and avoids typos when interacting with local storage.examples/turnkey/server/.gitignore (1)
1-29: Gitignore covers common patterns for the Turnkey server example.The
.gitignoreproperly excludes environment files, dependencies, IDE settings, and log artifacts. It aligns well with standard Node/Hono project practices.packages/wallets/wallet-base/src/types/enums.ts (1)
23-23: Enum updated to include the Turnkey wallet type.Adding
Turnkey = 'turnkey'integrates the new wallet strategy consistently with existing entries. Confirm downstream utilities and UI components handle the new enum member correctly.examples/turnkey/app/src/vite-env.d.ts (1)
1-1: Vite client types reference is correctly declared.The triple-slash directive
/// <reference types="vite/client" />is necessary for Vite-specific globals and ensures proper typing in the example app.examples/turnkey/app/src/main.ts (1)
1-5: Bootstrapping looks correct
The Vue app initialization is concise and follows Vue 3 best practices. No changes needed here.examples/turnkey/app/tsconfig.json (1)
2-6: Verify project reference configuration
Since this roottsconfig.jsonuses"references", ensure that bothtsconfig.app.jsonandtsconfig.node.jsonhave"composite": truein theircompilerOptionsto support TypeScript project references correctly.packages/exceptions/src/exceptions/types/modules.ts (1)
62-63: Well-structured Turnkey wallet error handling additionsThe new error action modules for Turnkey email OTP initialization and verification follow the existing naming conventions and integrate well with the rest of the enum. These additions will enable proper error handling for the new Turnkey wallet integration.
packages/wallets/wallet-base/src/utils/wallet.ts (2)
18-18: Appropriate integration of Turnkey wallet as an EVM walletThe addition of
Wallet.Turnkeyto the EVM wallet list correctly categorizes it as an EVM-compatible wallet, aligning with the Turnkey integration feature.
23-31: Properly categorized Turnkey as an EVM browser walletThe inclusion of
Wallet.Turnkeyin theisEvmBrowserWalletfunction correctly identifies it as a browser-based EVM wallet, enabling proper wallet type detection throughout the SDK.examples/turnkey/server/tsconfig.json (1)
1-14: Appropriate TypeScript configuration for Turnkey serverThe TypeScript configuration is well-structured for a modern Node.js application using the Hono framework:
target: "ESNext"andmodule: "NodeNext"support the latest ECMAScript featuresstrict: trueensures robust type checking- JSX configuration properly supports Hono's JSX implementation
packages/wallets/wallet-turnkey/tsconfig.build.json (1)
1-12: LGTM! Configuration follows TypeScript project best practicesThe TypeScript configuration for the new Turnkey wallet package is properly set up with appropriate module settings, build caching, and test file exclusions.
examples/turnkey/app/.gitignore (1)
1-24: LGTM! Standard .gitignore for a Vue/Vite projectThe .gitignore file includes all the standard patterns for a modern web application built with Vite, properly excluding build artifacts, dependencies, and environment-specific files.
packages/wallets/wallet-turnkey/src/index.ts (1)
1-3: Public API surface is concise and clear
The index module cleanly re-exports the Turnkey wallet strategy under the aliasTurnkeyWalletStrategyalong with any additional types from./strategy/strategy.js.examples/turnkey/app/vite.config.ts (1)
1-20: Vite configuration looks solid
The setup correctly imports Vue support and applies Node.js polyfills for browser globals and thenode:protocol. This will enable proper bundling of the Turnkey SDK within a browser environment.examples/turnkey/app/tsconfig.app.json (2)
1-14: Application TypeScript config is comprehensive
Extending@vue/tsconfig/tsconfig.dom.jsonand enforcing strict and no-unused checks for.ts,.tsx, and.vuefiles ensures a robust dev experience.🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: JSON standard does not allow comments.
(parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-14: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
7-11: Verify new compiler option support
The"noUncheckedSideEffectImports"flag was introduced in recent TypeScript versions. Confirm your project’s TypeScript version supports this option to avoid unexpected compiler errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 7-7: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 8-8: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 9-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
packages/wallets/wallet-turnkey/tsconfig.build.esm.json (1)
1-13: ESM build configuration is correct
The ESM-specific tsconfig extends your base build config, outputs todist/esm, and usesNodeNextresolution—matching Node’s ES module semantics.examples/turnkey/app/tsconfig.node.json (2)
1-24: Node-targeted tsconfig aligns with Vite needs
Limiting this config tovite.config.tsand using bundler module resolution, isolated modules, and noEmit is appropriate for your build tooling.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: JSON standard does not allow comments.
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 14-14: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 14-14: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 14-14: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 14-16: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 16-16: JSON standard does not allow comments.
(parse)
[error] 17-17: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 17-17: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 17-17: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 17-17: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 18-18: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 18-18: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 18-18: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 18-18: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 19-19: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 19-19: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 19-19: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 19-19: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 20-20: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 20-20: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 20-20: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 20-20: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 21-21: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 21-21: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 21-21: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 22-22: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 23-23: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 23-23: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 23-23: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 23-24: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
10-13: Verify advanced compiler options
Options like"allowImportingTsExtensions","moduleDetection": "force", and"moduleResolution": "bundler"require TypeScript 5.x+. Ensure your setup uses a compatible TS version to prevent unexpected errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 10-10: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 11-11: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 12-12: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 13-13: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
examples/turnkey/app/src/reactives.ts (1)
1-12: LGTM! Effective use of reactive state pattern for the Turnkey wallet implementation.The implementation correctly uses Vue's
reffunction to create typed reactive state variables for managing the Turnkey wallet lifecycle. The shared state approach will facilitate communication between components.examples/turnkey/server/src/lib/turnkey-server.ts (1)
70-76: Decoded JWT is not verified – ensure you trust the source.
decodeJwtonly base64-decodes the token without signature verification.
If the backend is exposed beyond internal use, add verification with the issuer’s JWKS to avoid spoofed emails.
examples/turnkey/app/src/utils.ts
Outdated
| export function generateGoogleUrl(nonce: string) { | ||
| const clientId = import.meta.env.VITE_GOOGLE_CLIENT_ID | ||
|
|
||
| if (!clientId) { | ||
| throw new Error('Google client ID not found') | ||
| } | ||
|
|
||
| const redirectUri = encodeURIComponent('http://localhost:5173') | ||
| const scope = encodeURIComponent('openid profile email') | ||
| const responseType = 'id_token' | ||
|
|
||
| return `https://accounts.google.com/o/oauth2/v2/auth?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make redirect URI configurable instead of hardcoded
The OAuth redirect URI is currently hardcoded to http://localhost:5173, which limits this implementation to local development only. For production deployment, this should be configurable.
- const redirectUri = encodeURIComponent('http://localhost:5173')
+ const redirectUri = encodeURIComponent(
+ import.meta.env.VITE_OAUTH_REDIRECT_URI || 'http://localhost:5173'
+ )Also, consider adding JSDoc documentation for this function to explain its purpose, parameters, and return 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.
| export function generateGoogleUrl(nonce: string) { | |
| const clientId = import.meta.env.VITE_GOOGLE_CLIENT_ID | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| const redirectUri = encodeURIComponent('http://localhost:5173') | |
| const scope = encodeURIComponent('openid profile email') | |
| const responseType = 'id_token' | |
| return `https://accounts.google.com/o/oauth2/v2/auth?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}` | |
| } | |
| export function generateGoogleUrl(nonce: string) { | |
| const clientId = import.meta.env.VITE_GOOGLE_CLIENT_ID | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| const redirectUri = encodeURIComponent( | |
| import.meta.env.VITE_OAUTH_REDIRECT_URI || 'http://localhost:5173' | |
| ) | |
| const scope = encodeURIComponent('openid profile email') | |
| const responseType = 'id_token' | |
| return `https://accounts.google.com/o/oauth2/v2/auth?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}` | |
| } |
examples/turnkey/server/src/index.ts
Outdated
| app.use( | ||
| '*', | ||
| cors({ | ||
| origin: '*', | ||
| allowHeaders: ['*'], | ||
| allowMethods: ['*'], | ||
| credentials: true, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Overly permissive CORS policy
The current CORS configuration allows requests from any origin (*), with any headers, any methods, and includes credentials. This is extremely permissive and could pose security risks in production.
Consider restricting the CORS policy to specific origins, especially when credentials are allowed:
app.use(
'*',
cors({
- origin: '*',
+ origin: process.env.ALLOWED_ORIGINS ? process.env.ALLOWED_ORIGINS.split(',') : ['http://localhost:5173'],
allowHeaders: ['*'],
allowMethods: ['*'],
credentials: true,
}),
)When credentials: true is set, browsers enforce that the origin cannot be '*' in production environments. This can lead to unexpected CORS errors.
📝 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.
| app.use( | |
| '*', | |
| cors({ | |
| origin: '*', | |
| allowHeaders: ['*'], | |
| allowMethods: ['*'], | |
| credentials: true, | |
| }), | |
| ) | |
| app.use( | |
| '*', | |
| cors({ | |
| origin: process.env.ALLOWED_ORIGINS | |
| ? process.env.ALLOWED_ORIGINS.split(',') | |
| : ['http://localhost:5173'], | |
| allowHeaders: ['*'], | |
| allowMethods: ['*'], | |
| credentials: true, | |
| }), | |
| ) |
examples/turnkey/app/src/App.vue
Outdated
| watch(turnkeyReadyAndLoggedIn, async (_ready) => { | ||
| if (!_ready) { | ||
| return | ||
| } | ||
|
|
||
| const _walletStrategy = new BaseWalletStrategy({ | ||
| strategies: { | ||
| [Wallet.Turnkey]: turnkeyStrategy.value as TurnkeyWalletStrategy, | ||
| }, | ||
| chainId: injectiveClients.chainId, | ||
| wallet: Wallet.Turnkey, | ||
| }) | ||
|
|
||
| broadcaster.value = new MsgBroadcaster({ | ||
| network: injectiveClients.network, | ||
| walletStrategy: _walletStrategy, | ||
| ethereumChainId: injectiveClients.ethereumChainId!, | ||
| endpoints: injectiveClients.endpoints, | ||
| }) | ||
|
|
||
| const addresses = await _walletStrategy?.getAddresses() | ||
| address.value = addresses[0] | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid rebuilding broadcaster ‑ guard against duplicate watchers.
Every time turnkeyStatus flips to 'logged-in' a brand-new BaseWalletStrategy and MsgBroadcaster are instantiated.
If the status toggles (e.g. on session refresh) you will leak listeners, keep stale gRPC connections alive and lose the tx-hash history.
Suggested pattern:
watch(turnkeyReadyAndLoggedIn, async (ready) => {
- if (!ready) { return }
- const _walletStrategy = new BaseWalletStrategy({ … })
- broadcaster.value = new MsgBroadcaster({ … })
+ if (!ready || broadcaster.value) { return } // already initialised
+
+ const walletStrategy = new BaseWalletStrategy({ … })
+ broadcaster.value = new MsgBroadcaster({ …walletStrategy… })
}Committable suggestion skipped: line range outside the PR's diff.
| sessionType: 'SESSION_TYPE_READ_WRITE', | ||
| userId: userId, | ||
| organizationId: suborgID, | ||
| expiry: Date.now() + (sessionLengthSeconds ?? 9000) * 1000, // 900 is the default expiry time if you don't pass in a sessionLengthSeconds to the request. Request should probably return the expiry time, instead of hardcoding it. | ||
| token: credentialBundle, | ||
| } |
There was a problem hiding this comment.
Session expiry units likely incorrect (9000 vs 900).
otpAuth defaults to 900 s (15 min). The hard-coded fallback multiplies by 9000, extending the session to 2.5 h if sessionLengthSeconds is omitted.
- expiry: Date.now() + (sessionLengthSeconds ?? 9000) * 1000,
+ expiry: Date.now() + (sessionLengthSeconds ?? 900) * 1000,📝 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.
| sessionType: 'SESSION_TYPE_READ_WRITE', | |
| userId: userId, | |
| organizationId: suborgID, | |
| expiry: Date.now() + (sessionLengthSeconds ?? 9000) * 1000, // 900 is the default expiry time if you don't pass in a sessionLengthSeconds to the request. Request should probably return the expiry time, instead of hardcoding it. | |
| token: credentialBundle, | |
| } | |
| sessionType: 'SESSION_TYPE_READ_WRITE', | |
| userId: userId, | |
| organizationId: suborgID, | |
| - expiry: Date.now() + (sessionLengthSeconds ?? 9000) * 1000, | |
| + expiry: Date.now() + (sessionLengthSeconds ?? 900) * 1000, | |
| token: credentialBundle, |
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
from
8d59edc to
8cf5343
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (2)
463-470: Use enum value instead of string literalFor consistency and type safety, use the enum value
TurnkeyStatus.LoggedIninstead of the string literal 'logged-in'.-if (this.status !== 'logged-in') { +if (this.status !== TurnkeyStatus.LoggedIn) { throw new WalletException(new Error('User is not logged in'), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: WalletAction.GetAccounts, }) }
714-737: Fix potential infinite loop in keepSessionAliveThe current implementation uses a while loop that depends on external mutation of
this.statusto exit. This could lead to a resource leak if the status is never changed externally.-// TODO: change to 60 seconds - -while (this.status === 'logged-in') { +do { if (!this.authIframeClient?.config.organizationId) { return } if (this.organizationId) { this.authIframeClient.config.organizationId = this.organizationId } this.authIframeClient?.refreshSession({ sessionType: SessionType.READ_WRITE, targetPublicKey: this.authIframeClient.iframePublicKey, expirationSeconds: '900', organizationId: this.organizationId, }) const session = await this.turnkey.getSession() this.expiry = session?.expiry await new Promise((resolve) => setTimeout(resolve, REFRESH_INTERVAL_MS)) -} +} while (this.status === TurnkeyStatus.LoggedIn)Also remove the TODO comment or implement the change as it's not clear what needs to be changed to 60 seconds (REFRESH_INTERVAL_MS is already 60 * 1000).
packages/wallets/wallet-turnkey/README.md (1)
3-4: Remove placeholder TODO commentThe placeholder comment
<!-- TODO -->should be removed or replaced with actual content before publishing to avoid confusion.-<!-- TODO --> -
🧹 Nitpick comments (4)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (2)
48-50: Define TurnkeyErrorCodes as proper enum constantsCurrently, you're using a magic number
7forUserLoggedOut. Consider using a more descriptive naming convention for the enum values, such as:enum TurnkeyErrorCodes { - UserLoggedOut = 7, + UserLoggedOut = 7, // User session has expired or been terminated }Adding a comment explaining the meaning of the error code helps future maintainers understand its purpose.
410-448: Replacevoidin union type withundefinedAccording to static analysis, using
voidin a union type can be confusing. Replace it withundefinedfor better clarity.-async oauthLogin(args: { - endpoint: string - oidcToken: string - providerName: 'google' //TODO: apple - expirationSeconds?: number -}): Promise<string | void> { +async oauthLogin(args: { + endpoint: string + oidcToken: string + providerName: 'google' //TODO: apple + expirationSeconds?: number +}): Promise<string | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/wallets/wallet-turnkey/README.md (2)
30-31: Fix punctuation in example introductionThe statement ends with both a period and a colon, which is redundant.
-Here's a brief example of how to use this package to send 1 INJ.: +Here's a brief example of how to use this package to send 1 INJ:🧰 Tools
🪛 LanguageTool
[formatting] ~30-~30: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
110-117: Fix markdown formatting issuesThere are several markdown formatting issues that should be addressed:
- Use proper markdown links instead of bare URLs
- Add alt text to the image for accessibility
- Use en dash for date ranges
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021–2022 Injective Labs Inc. ([https://injectivelabs.org/](https://injectivelabs.org/)) -<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px; max-width: 100%; height: auto" /></a> -Originally released by Injective Labs Inc. under: <br /> +Originally released by Injective Labs Inc. under <br /> Apache License <br /> Version 2.0, January 2004 <br /> -http://www.apache.org/licenses/ +[http://www.apache.org/licenses/](http://www.apache.org/licenses/)🧰 Tools
🪛 LanguageTool
[typographical] ~110-~110: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
[typographical] ~114-~114: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
110-110: Bare URL used
null(MD034, no-bare-urls)
112-112: Images should have alternate text (alt text)
null(MD045, no-alt-text)
117-117: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
packages/exceptions/src/exceptions/types/modules.ts(1 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(1 hunks)packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.esm.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/wallets/wallet-turnkey/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/wallets/wallet-turnkey/tsconfig.json
- packages/wallets/wallet-base/src/types/enums.ts
- packages/exceptions/src/exceptions/types/modules.ts
- packages/wallets/wallet-turnkey/tsconfig.build.json
- packages/wallets/wallet-base/src/utils/wallet.ts
- packages/wallets/wallet-turnkey/package.json
- packages/wallets/wallet-turnkey/tsconfig.build.esm.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (5)
packages/wallets/wallet-base/src/types/strategy.ts (2)
ConcreteWalletStrategy(135-221)StdSignDoc(272-272)packages/ts-types/src/aliases.ts (1)
AccountAddress(1-1)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/sdk-ts/src/core/tx/api/TxGrpcApi.ts (1)
TxGrpcApi(26-279)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~26-~26: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~27-~27: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~30-~30: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~110-~110: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~114-~114: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
packages/wallets/wallet-turnkey/README.md
110-110: Bare URL used
null
(MD034, no-bare-urls)
112-112: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
117-117: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (1)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (1)
35-41: Good use of enums for status and providersThe code properly uses enums for TurnkeyStatus and TurnkeyProviders, which provides type safety and makes the code more maintainable.
| // TODO: for some reason this is writing the incorrect organizationId | ||
| if (session && session?.expiry) { | ||
| this.expiry = session.expiry | ||
| } |
There was a problem hiding this comment.
Address TODO comment and use optional chaining
There's a TODO comment about incorrect organizationId that should be investigated and fixed. Additionally, the static analysis tool suggests using optional chaining for the session expiry check.
- // TODO: for some reason this is writing the incorrect organizationId
- if (session && session?.expiry) {
+ if (session?.expiry) {
this.expiry = session.expiry
}Consider investigating why the organizationId is being written incorrectly before removing the TODO.
📝 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.
| // TODO: for some reason this is writing the incorrect organizationId | |
| if (session && session?.expiry) { | |
| this.expiry = session.expiry | |
| } | |
| if (session?.expiry) { | |
| this.expiry = session.expiry | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (4)
178-180: Fix redundant optional chaining.The condition can be simplified by using optional chaining syntax.
- if (session && session?.expiry) { + if (session?.expiry) { this.expiry = session.expiry }
463-469: Use enum value instead of string literal.Replace the string literal with the corresponding enum value for consistency and type safety.
- if (this.status !== 'logged-in') { + if (this.status !== TurnkeyStatus.LoggedIn) { throw new WalletException(new Error('User is not logged in'), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: WalletAction.GetAccounts, })
572-574: Fix wallet name in error message.The error message incorrectly references "Magic wallet" instead of "Turnkey wallet".
- 'You have to pass endpoints.grpc within the options for using Magic wallet', + 'You have to pass endpoints.grpc within the options for using Turnkey wallet',
714-737: 🛠️ Refactor suggestionFix infinite loop risk in keepSessionAlive method.
The current implementation doesn't re-check the status after sleeping, which could lead to an infinite loop if the status changes externally.
private async keepSessionAlive() { - if (this.status !== 'logged-in') { + if (this.status !== TurnkeyStatus.LoggedIn) { return } - while ( - this.status === TurnkeyStatus.LoggedIn && - this.authIframeClient?.config.organizationId - ) { - if (this.organizationId) { - this.authIframeClient.config.organizationId = this.organizationId - } - - this.authIframeClient?.refreshSession({ - sessionType: SessionType.READ_WRITE, - targetPublicKey: this.authIframeClient.iframePublicKey, - expirationSeconds: '900', - organizationId: this.organizationId, - }) - - const session = await this.turnkey.getSession() - this.expiry = session?.expiry - - await new Promise((resolve) => setTimeout(resolve, REFRESH_INTERVAL_MS)) + do { + if (this.organizationId && this.authIframeClient?.config.organizationId) { + this.authIframeClient.config.organizationId = this.organizationId + } + + this.authIframeClient?.refreshSession({ + sessionType: SessionType.READ_WRITE, + targetPublicKey: this.authIframeClient.iframePublicKey, + expirationSeconds: '900', + organizationId: this.organizationId, + }) + + const session = await this.turnkey.getSession() + this.expiry = session?.expiry + + await new Promise((resolve) => setTimeout(resolve, REFRESH_INTERVAL_MS)) + } while ( + this.status === TurnkeyStatus.LoggedIn && + this.authIframeClient?.config.organizationId + ) } }
🧹 Nitpick comments (7)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (2)
405-410: Replacevoidwithundefinedin union type.Using
voidin a union type can be confusing as it suggests the function doesn't return anything, but it actually returns either a string or nothing.async oauthLogin(args: { endpoint: string oidcToken: string providerName: 'google' //TODO: apple expirationSeconds?: number - }): Promise<string | void> { + }): Promise<string | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
446-447: Include error message in exception.The error handler should include the actual error message for better debugging.
- throw new WalletException(new Error('turnkey-oauth-login')) + throw new WalletException(new Error(`turnkey-oauth-login: ${(e as any).message}`))packages/wallets/wallet-turnkey/README.md (5)
24-25: Fix grammatical issues in documentation.There are a few minor grammatical issues that should be fixed for better readability.
-Regardless of which wallet package(s) you choose to use you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base` -installed. These contain all of the types and core wallet functionality, with the separate wallet packages only providing the necessary +Regardless of which wallet package(s) you choose to use, you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base` +installed. These contain all the types and core wallet functionality, with the separate wallet packages only providing the necessary🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...(ALL_OF_THE)
28-28: Fix punctuation in documentation.The sentence has both a period and a colon at the end, which is redundant.
-Here's a brief example of how to use this package to send 1 INJ.: +Here's a brief example of how to use this package to send 1 INJ:🧰 Tools
🪛 LanguageTool
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
108-108: Use en dash for date range.For date ranges, it's typographically correct to use an en dash instead of a hyphen.
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021–2022 Injective Labs Inc. (https://injectivelabs.org/)🧰 Tools
🪛 LanguageTool
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
108-115: Fix URL formatting and colon usage.The URLs should be properly formatted as hyperlinks, and there's an unnecessary colon after the preposition "under".
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021–2022 Injective Labs Inc. (<a href="https://injectivelabs.org/">https://injectivelabs.org/</a>) <a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> -Originally released by Injective Labs Inc. under: <br /> +Originally released by Injective Labs Inc. under <br /> Apache License <br /> Version 2.0, January 2004 <br /> -http://www.apache.org/licenses/ +<a href="http://www.apache.org/licenses/">http://www.apache.org/licenses/</a>🧰 Tools
🪛 LanguageTool
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null(MD045, no-alt-text)
115-115: Bare URL used
null(MD034, no-bare-urls)
110-110: Add alt text to image.Images should include alt text for accessibility.
-<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px; max-width: 100%; height: auto" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Images should have alternate text (alt text)
null(MD045, no-alt-text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/wallets/wallet-turnkey/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (6)
packages/wallets/wallet-base/src/types/strategy.ts (2)
ConcreteWalletStrategy(135-221)StdSignDoc(272-272)packages/ts-types/src/aliases.ts (1)
AccountAddress(1-1)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/sdk-ts/src/types/cosmos.ts (3)
TxRaw(22-22)DirectSignResponse(25-25)AminoSignResponse(25-25)packages/sdk-ts/src/core/tx/api/TxGrpcApi.ts (1)
TxGrpcApi(26-279)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts
[error] 410-410: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
packages/wallets/wallet-turnkey/README.md
108-108: Bare URL used
null
(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
115-115: Bare URL used
null
(MD034, no-bare-urls)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (1)
406-416: Apply the same URL-duplication fix to OAuth loginSee earlier comment –
HttpClientalready holds the base URL.-const client = new HttpClient(endpoint, { … }) -const response = await client.post(endpoint, { … }) +const client = new HttpClient(endpoint, { … }) +const response = await client.post('', { … })
🧹 Nitpick comments (4)
packages/wallets/wallet-base/src/types/strategy.ts (2)
53-60: Document what eachTurnkeyStatusrepresentsThe enum is spot-on, but a one-liner jsdoc for each status (e.g.
/** iframe loaded, waiting for user interaction */) would make downstream maintenance easier and avoid mis-interpreting “Ready” vs “Initializing”.
71-75: Align metadata property casing with prior conventionsElsewhere in the SDK (see previous PR discussions) the team leans on lower-camel-case keys like
turnKey. Here we settled onturnkey.
Consistent naming avoids double checks such asmetadata.turnKey ?? metadata.turnkey. Consider standardising across all wallet metadata objects in the next cleanup pass.packages/wallets/wallet-turnkey/src/strategy/strategy.ts (2)
394-397: Preferundefinedovervoidin union typeBiome flagged this:
Promise<string | void>can be confusing becausevoidwidens to any. Useundefinedto make intention explicit.- }): Promise<string | void> { + }): Promise<string | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 396-396: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
699-723:keepSessionAlivemay spin forever after logoutThe loop depends on
this.statusbeing mutated elsewhere. Ifdisconnect()is invoked while theawait setTimeoutis sleeping, the first condition staystrueand the loop continues.
Consider clearing anabortControlleror re-checking the status after the sleep:while (this.status === TurnkeyStatus.LoggedIn && this.authIframeClient?.config.organizationId) { + if (this.status !== TurnkeyStatus.LoggedIn) break; … await new Promise(res => setTimeout(res, REFRESH_INTERVAL_MS)) + if (this.status !== TurnkeyStatus.LoggedIn) break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/wallets/wallet-base/src/types/strategy.ts(4 hunks)packages/wallets/wallet-strategy/package.json(1 hunks)packages/wallets/wallet-strategy/src/strategy/index.ts(4 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/wallets/wallet-strategy/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/wallets/wallet-strategy/src/strategy/index.ts
- packages/wallets/wallet-turnkey/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-base/src/types/strategy.ts (1)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts (1)
TurnkeyMetadata(46-46)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/strategy.ts
[error] 396-396: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| if (args.provider === TurnkeyProviders.Google) { | ||
| if (!args.oidcToken) { | ||
| throw new WalletException(new Error('Oidc token is required')) | ||
| } | ||
| const result = await this.oauthLogin({ | ||
| endpoint: args.oauthLoginEndpoint, | ||
| oidcToken: args.oidcToken, | ||
| providerName: 'google', | ||
| }) | ||
|
|
||
| if (result) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
enable() returns opposite value for Google OAuth path
oauthLogin resolves undefined on success and a string message on soft-failure.
Current logic treats a truthy result as failure – correct – but then returns false/true inverted:
const result = await this.oauthLogin(...)
if (result) { // failure path
return false // <-- should indicate failure, OK
}
return true // success, OKYet the early return false after the OAuth call (line 258) executes before the success branch, so the method always returns false.
- if (result) {
- return false
- }
-
- return true
+ return !result| const bodyData = { | ||
| otpCode, | ||
| otpId: emailOTPId, | ||
| targetPublicKey: this.authIframeClient.iframePublicKey, | ||
| suborgID: organizationId, | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent casing: suborgID vs suborgId
The Turnkey API is case-sensitive. Earlier you send suborgId, here suborgID. This will silently drop the field for either request.
- suborgID: organizationId,
+ suborgId: organizationId,📝 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.
| const bodyData = { | |
| otpCode, | |
| otpId: emailOTPId, | |
| targetPublicKey: this.authIframeClient.iframePublicKey, | |
| suborgID: organizationId, | |
| } | |
| const bodyData = { | |
| otpCode, | |
| otpId: emailOTPId, | |
| targetPublicKey: this.authIframeClient.iframePublicKey, | |
| suborgId: organizationId, | |
| } |
| try { | ||
| const bodyData = { | ||
| email: args.email, | ||
| suborgId: args.suborgId, | ||
| invalidateExistingSessions: args.invalidateExistingSessions, | ||
| targetPublicKey: this.authIframeClient?.iframePublicKey, | ||
| } | ||
| const client = new HttpClient(args.endpoint, { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }) | ||
| const response = (await client.post(args.endpoint, bodyData)) as { | ||
| data?: { | ||
| otpId: string | ||
| organizationId: string | ||
| } | ||
| } | ||
|
|
||
| const { otpId, organizationId } = response?.data || {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate URL when posting with HttpClient
new HttpClient(endpoint) sets endpoint as the base URL, but the subsequent client.post(endpoint, …) appends the same path again, yielding a malformed URL (https://api/foohttps://api/foo).
-const client = new HttpClient(args.endpoint, { … })
-const response = await client.post(args.endpoint, bodyData)
+const client = new HttpClient(args.endpoint, { … })
+const response = await client.post('', bodyData) // or use a relative path segmentRepeat the same fix in oauthLogin.
Committable suggestion skipped: line range outside the PR's diff.
| } | ||
|
|
||
| enum TurnkeyErrorCodes { | ||
| UserLoggedOut = 7, |
There was a problem hiding this comment.
We don't care about any other ErrorCode?
| case Wallet.Turnkey: | ||
| if ( | ||
| !args.options?.metadata?.turnkey?.defaultOrganizationId || | ||
| !args.options.metadata.turnkey.turnkeyAuthIframeContainerId |
There was a problem hiding this comment.
Missing optional chaining operator here? (?.)
| [Wallet.Turnkey]: new TurnkeyWallet({ | ||
| onStatusChange(status) { | ||
| turnkeyStatus.value = status | ||
| }, | ||
| chainId: injectiveClients.chainId, | ||
| ethereumOptions: { | ||
| ethereumChainId: injectiveClients.ethereumChainId!, | ||
| }, | ||
| metadata: { | ||
| turnkeyAuthIframeContainerId, | ||
| defaultOrganizationId: import.meta.env | ||
| .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, | ||
| apiBaseUrl: 'https://api.turnkey.com', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Formatting in this file's examples seems wrong
| "default": "./dist/cjs/index.js" | ||
| } | ||
| }, | ||
| "./src/*": "./src/*" |
There was a problem hiding this comment.
Why do we need this? We are not publishing the src folder to npm
| @@ -321,7 +317,7 @@ export class WalletConnect | |||
| try { | |||
| this.provider = await EthereumProvider.init({ | |||
| projectId: this.metadata.projectId as string, | |||
| metadata: this.metadata | |||
| metadata: this | |||
There was a problem hiding this comment.
Are we sure this works? Previously it was this.metadata.metadata now it's this.metadata
| TurnkeyStatus, | ||
| } from '@injectivelabs/wallet-base' | ||
| import { SessionType, Turnkey, TurnkeyIframeClient } from '@turnkey/sdk-browser' | ||
|
|
| import { SessionType, Turnkey, TurnkeyIframeClient } from '@turnkey/sdk-browser' | ||
|
|
||
| import { createAccount } from '@turnkey/viem' | ||
|
|
| export type TurnkeyMetadata = ITurnkeyMetadata & TurnkeySDKBrowserConfig | ||
|
|
||
| type OAuthArgs = { | ||
| provider: 'google' | ||
| oidcToken: string | ||
| oauthLoginEndpoint: string | ||
| } | ||
|
|
||
| type EmailArgs = { | ||
| provider: 'email' | ||
| email: string | ||
| initEmailOTPEndpoint: string | ||
| } |
There was a problem hiding this comment.
Can we cleanup this file to export the types from types.ts file?
bangjelkoski
force-pushed
the
feat/turnkey-wallet
branch
3 times, most recently
from
79de61b to
8e59a6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
packages/wallets/wallet-base/src/types/strategy.ts (1)
161-165:⚠️ Potential issueMethod naming mismatch breaks type-safety
ConcreteCosmosWalletStrategydeclaressignAminoTransaction, whereas
ConcreteWalletStrategy(and your new Turnkey implementation) use
signAminoCosmosTransaction. TypeScript will not catch implementations that
only satisfy one interface, leading to runtime “method not found” errors.Standardise the name across all interfaces:
- signAminoTransaction(transaction: { + signAminoCosmosTransaction(transaction: {and update downstream references.
Also applies to: 239-243
♻️ Duplicate comments (1)
packages/wallets/wallet-strategy/src/strategy/index.ts (1)
206-206: Add optional chaining to prevent potential errors.When calling
setMetadataon a strategy that might not have this method, you should use the optional chaining operator (?.) to avoid runtime errors.- this.strategies[walletEnum]?.setMetadata?.(metadata) + this.strategies[walletEnum]?.setMetadata?.(metadata)
🧹 Nitpick comments (13)
packages/exceptions/src/exceptions/types/codes.ts (1)
820-820: Add documentation about the purpose of extending ErrorContextCode with number type.This change broadens the
ErrorContextCodetype to accept arbitrary numeric values, not just predefined enum values. While this likely supports the new Turnkey wallet error codes, adding a comment explaining the rationale would improve code maintainability.export type ErrorContextCode = | ChainAuctionErrorCodes | ChainAuthZErrorCodes | ChainCosmosErrorCode | ChainExchangeModuleErrorCode | ChainInsuranceErrorCodes | ChainOcrErrorCodes | ChainOracleErrorCodes | ChainPeggyErrorCodes | ChainTokenFactoryErrorCodes | ChainWasmXErrorCodes | ChainDistributionErrorCodes | ChainBankErrorCodes | ChainGovErrorCodes | ChainStakingErrorCodes | ChainWasmErrorCodes | ErrorCode - | number + | number // Added to support custom error codes from external integrations (e.g., Turnkey wallet) | typeof UnspecifiedErrorCodepackages/utils/src/classes/HttpRestClient.ts (2)
73-108: Duplicate error-handling blocks – extract helper to DRY the code
getand$get(and the analogous post pair) contain nearly identicaltry/catchlogic.
This repetition invites divergence and bugs (e.g., forgetting to adjust one path when adding new status-code handling).Refactor to a single private wrapper, e.g.:
- public async $get<T>(endpoint: string, params: Record<string, any> = {}): Promise<T> { - try { - return await this.client.$get(endpoint, params) - } catch (e: unknown) { - return this.handleAxiosError(e, endpoint) - } - } + private handleAxiosError(e: unknown, endpoint: string, method: HttpRequestMethod = HttpRequestMethod.Get): never { … }This keeps new logic (timeout categorisation, message extraction, etc.) in one place.
178-216:contextModulevsmethod– mixed semantics in POST error pathIn
$postyou set bothmethodandcontextModuletoHttpRequestMethod.Post.
HttpRequestExceptionalready stores the HTTP verb in.method; overloadingcontextModulewith the same enum dilutes its original purpose (logical module identifier).- context: endpoint, - contextModule: HttpRequestMethod.Post, + context: endpoint,packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (3)
12-67: Refactor to use standalone functions instead of a class with static methods.The
TurnkeyOauthWalletclass only contains static methods, which doesn't align with object-oriented design principles. Consider refactoring to use standalone functions, as suggested by the static analysis tool.-export class TurnkeyOauthWallet { - static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) { +export async function generateOAuthNonce(iframeClient: TurnkeyIframeClient) { // implementation unchanged } - static async oauthLogin(args: { +export async function oauthLogin(args: { // implementation unchanged } -}🧰 Tools
🪛 Biome (1.9.4)
[error] 12-67: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 39-39: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
25-29: Improve error handling when accessing error message.The current error handling assumes that the caught error has a
messageproperty, which might not always be the case. Consider using a more robust approach to extract error information.- throw new WalletException(new Error((e as any).message), { + const errorMessage = e instanceof Error ? e.message : String(e); + throw new WalletException(new Error(errorMessage), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-generate-oauth-nonce', })
33-39: Remove void from the function return type.The return type includes
void, which can be confusing in a union type. Since the function seems to always return a string when successful (or throw an exception on failure), remove thevoidtype.- static async oauthLogin(args: { + static async oauthLogin(args: { client: HttpRestClient oidcToken: string providerName: 'google' | 'apple' expirationSeconds?: number iframeClient: TurnkeyIframeClient - }): Promise<string | void> { + }): Promise<string> { // implementation unchanged🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (2)
14-96: Refactor to use standalone functions instead of a class with static methods.Similar to the OAuth implementation, the
TurnkeyOtpWalletclass only contains static methods. Consider refactoring to use standalone functions following functional programming principles.-export class TurnkeyOtpWallet { - static async initEmailOTP(args: { +export async function initEmailOTP(args: { // parameters unchanged }) { // implementation unchanged } - static async confirmEmailOTP(args: { +export async function confirmEmailOTP(args: { // parameters unchanged }) { // implementation unchanged } -}🧰 Tools
🪛 Biome (1.9.4)
[error] 14-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
42-48: Improve error handling when accessing error message.The current error handling assumes that the caught error has a
messageproperty, which might not always be the case. Consider using a more robust approach to extract error information.- throw new WalletException(new Error((e as any).message), { + const errorMessage = e instanceof Error ? e.message : String(e); + throw new WalletException(new Error(errorMessage), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-init-email-otp', })packages/utils/src/classes/HttpClient.ts (2)
42-52: Add robust error handling for $get method.The
$getmethod doesn't include specific error handling for HTTP errors, timeouts, or network issues. Consider adding try-catch blocks to provide more context in error messages.async $get<T, P extends Record<string, any>>( endpoint: string, params: P = {} as P, ): Promise<T> { + try { const response = await this.client.get(endpoint, { params, ...this.config, }) return 'data' in response ? response.data : response + } catch (error) { + // Optionally enhance error with context + throw error; + } }
65-72: Add explicit null/undefined check for response data.The current implementation checks if 'data' is in the response, but doesn't check if it's defined or non-null. If
response.dataisundefinedornull, the method will still return that value.async $post<T, P extends Record<string, any>>( endpoint: string, data: P = {} as P, ): Promise<T> { const response = await this.client.post(endpoint, data, this.config) - return 'data' in response ? response.data : response + return 'data' in response && response.data != null ? response.data : response }Apply the same change to the
$getmethod for consistency.packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (1)
92-104:refreshSessionis executed even wheninjectCredentialBundlefailsIf
loginResultis falsy (e.g. corrupted bundle) we still attempt a refresh, which
will always error and mask the original cause. Consider short-circuiting:- const loginResult = await iframeClient.injectCredentialBundle( - credentialBundle, - ) - - // If there is no session, we want to force a refresh … - await iframeClient.refreshSession({ + const loginResult = await iframeClient.injectCredentialBundle( + credentialBundle, + ) + if (!loginResult) { + return { session: undefined, organizationId } + } + + await iframeClient.refreshSession({packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (1)
115-118: Remove stray “Why are we doing this?” commentThis debug note slipped into production code and provides no actionable context.
Please delete it to avoid confusion.packages/wallets/wallet-base/src/types/strategy.ts (1)
70-76:TurnkeySession.sessionTypetyped asanyundermines the benefits of TSReplace
anywith the actual enum from@turnkey/sdk-browser:-import { … } from '@turnkey/sdk-browser' +import { SessionType } from '@turnkey/sdk-browser' … - sessionType: any + sessionType: SessionTypeThis preserves IDE autocompletion and avoids accidental misuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (25)
packages/exceptions/src/exceptions/types/codes.ts(1 hunks)packages/sdk-ts/src/core/tx/api/TxRestApi.ts(4 hunks)packages/utils/src/classes/HttpClient.ts(1 hunks)packages/utils/src/classes/HttpRestClient.ts(2 hunks)packages/utils/src/classes/index.ts(1 hunks)packages/wallets/wallet-base/src/base.ts(2 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(2 hunks)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts(3 hunks)packages/wallets/wallet-cosmos/src/strategy/strategy.ts(3 hunks)packages/wallets/wallet-cosmostation/src/strategy/strategy.ts(6 hunks)packages/wallets/wallet-magic/src/strategy/strategy.ts(5 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-strategy/src/strategy/index.ts(7 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/consts.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/types.ts(1 hunks)packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts(3 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts
- packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts
- packages/utils/src/classes/index.ts
- packages/wallets/wallet-turnkey/src/strategy/consts.ts
- packages/wallets/wallet-cosmostation/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/src/strategy/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/wallets/wallet-base/src/types/enums.ts
- packages/wallets/wallet-turnkey/src/index.ts
- packages/wallets/wallet-base/src/utils/wallet.ts
- packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
packages/wallets/wallet-private-key/src/strategy/strategy.ts (2)
packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/wallets/wallet-base/src/types/enums.ts (1)
WalletAction(49-49)
packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts (1)
packages/wallets/wallet-base/src/types/strategy.ts (2)
WalletMetadata(95-100)WalletStrategyArguments(171-178)
packages/utils/src/classes/HttpRestClient.ts (3)
packages/exceptions/src/exceptions/exceptions/HttpRequestException.ts (1)
HttpRequestException(4-25)packages/sdk-ts/src/utils/helpers.ts (1)
getErrorMessage(124-132)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)
packages/wallets/wallet-strategy/src/strategy/index.ts (3)
packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts (1)
TurnkeyOtpWalletStrategy(4-11)packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts (1)
TurnkeyOauthWalletStrategy(7-14)packages/wallets/wallet-base/src/types/strategy.ts (2)
ConcreteStrategiesArg(167-169)WalletMetadata(95-100)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (4)
packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(23-217)packages/wallets/wallet-turnkey/src/strategy/types.ts (2)
TurnkeyOTPCredentialsResponse(19-22)TurnkeyConfirmEmailOTPResponse(24-26)packages/wallets/wallet-turnkey/src/strategy/consts.ts (2)
TURNKEY_OTP_INIT_PATH(3-3)TURNKEY_OTP_VERIFY_PATH(4-4)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)
packages/wallets/wallet-base/src/base.ts (1)
packages/wallets/wallet-base/src/types/strategy.ts (4)
WalletMetadata(95-100)ConcreteWalletStrategyArgs(102-105)ConcreteEthereumWalletStrategyArgs(112-115)ConcreteCosmosWalletStrategyArgs(107-110)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (5)
packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(23-217)packages/wallets/wallet-turnkey/src/strategy/types.ts (1)
TurnkeyOauthLoginResponse(28-31)packages/wallets/wallet-turnkey/src/strategy/consts.ts (1)
TURNKEY_OAUTH_PATH(1-1)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts
[error] 14-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts
[error] 12-67: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 39-39: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (10)
packages/wallets/wallet-cosmos/src/strategy/strategy.ts (1)
53-53: Improve type safety with explicit casting.The explicit casting of
args.chainIdtoChainIdensures better type safety and maintains consistency. This is a good improvement.Also applies to: 64-64
packages/wallets/wallet-private-key/src/strategy/strategy.ts (1)
244-257: Good refactoring to use metadata-based lazy initialization.This change shifts from constructor injection to metadata-based lazy initialization, which aligns with the broader strategy across wallet implementations. This approach improves flexibility and consistency in how wallet configurations are managed.
One improvement to consider - add a more specific error message that tells users where to provide the private key now that it's coming from metadata:
throw new WalletException( - new Error('Please provide private key in the constructor'), + new Error('Please provide private key in the metadata.privateKey.privateKey property'), { code: UnspecifiedErrorCode, type: ErrorType.WalletNotInstalledError, contextModule: WalletAction.GetAccounts, }, )packages/wallets/wallet-base/src/base.ts (2)
21-21: Metadata support enables flexible wallet configuration.Adding the
metadataproperty toBaseConcreteStrategyprovides a standardized way to configure wallet strategies, supporting the new Turnkey wallet integration and other future wallet types. This is a good architectural improvement.Consider adding JSDoc comments to document the purpose and expected structure of the metadata property:
+ /** + * Holds configuration data for specific wallet implementations. + * Different wallet strategies can access their specific configuration + * through this property (e.g., metadata.turnkey, metadata.privateKey). + */ public metadata?: WalletMetadataAlso applies to: 34-34
23-28: Public constructor improves extensibility.Changing the constructor visibility from
protectedtopublicenables direct instantiation of the base class, which is necessary for the metadata-driven approach. This is a good change for extensibility.packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts (1)
80-86: Returnvoidvs. fluent interface – verify expectations
setMetadatacurrently returnsundefined. If consumers relied on the previoussetOptions(which often returnedthisfor chaining), this silent change is a breaking one.
Confirm no downstream code depends on chaining, or explicitly document the new behaviour.packages/wallets/wallet-magic/src/strategy/strategy.ts (1)
93-103:disconnectdoes nothing when user never logged in
magicWallet.user.isLoggedIn()returnsfalsefor accounts that were never connected, in which casedisconnectexits silently.
Some consumers expectdisconnectto always clear local session / provider state regardless of prior login.
If that assumption holds elsewhere in the SDK, callmagicWallet.user.logout()unconditionally and swallow the known “not logged in” error instead.packages/wallets/wallet-strategy/src/strategy/index.ts (1)
126-141: LGTM: Turnkey wallet integration looks good.The integration of Turnkey wallet strategies with appropriate validation of required metadata fields follows the established pattern in the codebase. The check for required fields (defaultOrganizationId and iframeContainerId) before instantiating the strategies is a good practice.
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (1)
69-75: LGTM: Good validation of required parameters.The validation of required parameters (emailOTPId and organizationId) before making the API call is a good practice that helps prevent runtime errors and provides clear error messages.
packages/utils/src/classes/HttpClient.ts (1)
38-51: LGTM: Good addition of utility methods to extract response data.The new
$getand$postmethods that extract and return just the data portion of the response are a useful abstraction that simplifies client code. The addition of explanatory JSDoc comments is also helpful.packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (1)
337-353: Validate required iframe IDs before instantiatingTurnkeyWallet
iframeElementIdandiframeContainerIdare mandatory for the iframe client, but
they’re not checked here. A missing value will only surface much later inside
TurnkeyWallet.initFrame, making debugging harder.+ if (!metadata.turnkey.iframeElementId || !metadata.turnkey.iframeContainerId) { + throw new WalletException( + new Error('Turnkey iframeElementId and iframeContainerId are required'), + ) + }
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (1)
179-201: Address‐key casing may still create duplicate entries inaccountMapYou lookup using both the raw and
toLowerCase()address, but you only store with the original casing. If future calls pass an address with a different checksum/case, the cache is missed and a second API call is made.Apply this diff to fix the issue:
- const { accountMap } = this + const key = address.toLowerCase() + const { accountMap } = this @@ - if (accountMap[address] || accountMap[address.toLowerCase()]) { - return accountMap[address] || accountMap[address.toLowerCase()] + if (accountMap[key]) { + return accountMap[key] } @@ - this.accountMap[address] = turnkeyAccount + this.accountMap[key] = turnkeyAccountpackages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (1)
79-98:enable()leaves status as Initializing on failureIf any step inside
trythrows, wecatchand returnfalse, but the wallet status never reverts fromInitializing, which can mislead callers/UI.Apply this fix:
- try { + try { // existing code... return !!(await turnkeyWallet.getIframeClient()) - } catch (e) { - return false + } catch (e) { + await this.setStatus(WalletStatus.Error) + return false }
🧹 Nitpick comments (8)
packages/wallets/wallet-base/src/types/strategy.ts (2)
63-63: Avoid usinganytype for better type safetyThe
sessionTypeproperty is defined with typeany. Using explicit types instead ofanywould improve type safety and developer experience.- sessionType: any + sessionType: SessionTypeConsider importing the appropriate
SessionTypetype from the Turnkey SDK.
70-88: Consider grouping related properties in TurnkeyMetadataThe
TurnkeyMetadatainterface has many properties that serve different purposes (API configuration, session data, authentication data). Consider grouping related properties into nested objects for better organization and maintainability.export interface TurnkeyMetadata { defaultOrganizationId: string apiBaseUrl: string apiServerEndpoint: string + // API configuration iframeUrl?: string - email?: string - session?: TurnkeySession - otpId?: string - otpCode?: string - oidcToken?: string iframeElementId?: string iframeContainerId: string - credentialBundle?: string organizationId?: string - provider?: TurnkeyProvider otpInitPath?: string otpVerifyPath?: string oauthLoginPath?: string + // Authentication data + auth?: { + email?: string + otpId?: string + otpCode?: string + oidcToken?: string + provider?: TurnkeyProvider + credentialBundle?: string + session?: TurnkeySession + } }packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (2)
24-30: Consider using more specific error codesUsing
UnspecifiedErrorCodefor all errors makes it difficult to identify the specific cause of failures. Consider defining and using specific error codes for different error scenarios.- throw new WalletException(new Error((e as any).message), { - code: UnspecifiedErrorCode, + throw new WalletException(new Error((e as any).message), { + code: YOUR_SPECIFIC_ERROR_CODE, type: ErrorType.WalletError, contextModule: 'turnkey-generate-oauth-nonce', })Also applies to: 59-65
68-92: Address TODO comment for future maintainabilityThe
loginUsermethod is marked with a TODO comment indicating it might be removed. Determine if this method should be kept or refactored based on the implementation plan.If this method is temporary, consider adding specific criteria or timeline for its removal to guide future maintainers. Alternatively, I can help refactor this into a more permanent solution if you provide more context on its intended usage.
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (2)
138-142: Make session expiration time configurableThe session expiration time is hard-coded to '900' seconds (15 minutes). Consider making this configurable to allow for different session durations based on requirements.
await iframeClient.refreshSession({ sessionType: SessionType.READ_WRITE, targetPublicKey: iframeClient.iframePublicKey, - expirationSeconds: '900', + expirationSeconds: args.expirationSeconds || '900', })And update the function signature:
static async loginUser(args: { client: HttpRestClient iframeClient: TurnkeyIframeClient organizationId: string otpCode: string emailOTPId: string + expirationSeconds?: string setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void }) {
100-101: Address TODO comment for future maintainabilitySimilar to the OAuth implementation, this TODO comment should be addressed to clarify if the method should be kept or removed.
If this method is temporary, consider documenting when and why it should be removed. I can help refactor this into a permanent solution if you provide more context on its intended usage.
packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (2)
141-142: Consider implementing the TODO commentThe commented code suggests abstracting the credential bundle logic into the TurnkeyWallet class to improve code organization and reduce duplication.
This refactoring would provide better separation of concerns and make the code more maintainable. The same TODO appears at line 163.
117-118: Fix typo in commentThere's a minor typo in the comment.
- // CHeck if the user is already connected + // Check if the user is already connected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
packages/sdk-ts/src/utils/address.spec.ts(2 hunks)packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/wallets/wallet-turnkey/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (5)
packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(23-217)packages/wallets/wallet-turnkey/src/strategy/consts.ts (1)
TURNKEY_OAUTH_PATH(1-1)packages/wallets/wallet-turnkey/src/strategy/types.ts (1)
TurnkeyOauthLoginResponse(28-31)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts
[error] 12-93: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 40-40: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (5)
packages/wallets/wallet-base/src/types/strategy.ts (2)
90-95: Good metadata consolidation approachThe consolidation of different wallet metadata types into a single
WalletMetadatainterface provides a cleaner and more maintainable approach compared to separate option types.
178-178:❌ Incorrect review comment
Verify the removed interfaces
The
isChainIdSupportedandsignAminoTransactionmethods are no longer excluded from the interface extension. Ensure this is intentional and that these methods are properly implemented by all concrete wallet strategies.
🏁 Script executed:
#!/bin/bash # Check if all wallet strategies implement the previously excluded methods ast-grep --pattern $'class $StrategyName extends $_ implements ConcreteWalletStrategy { $$$ isChainIdSupported($params) { $$$ } $$$ }' ast-grep --pattern $'class $StrategyName extends $_ implements ConcreteWalletStrategy { $$$ signAminoTransaction($params) { $$$ } $$$ }'Length of output: 297
🏁 Script executed:
#!/bin/bash # Locate the ConcreteWalletStrategy interface interface_file=$(rg -l 'interface ConcreteWalletStrategy') echo "Found interface in: $interface_file" echo "---------- Interface Definition ----------" sed -n '/interface ConcreteWalletStrategy/,/}/p' "$interface_file" echo # For each class implementing ConcreteWalletStrategy, verify the two methods exist echo "Verifying implementations of isChainIdSupported and signAminoTransaction..." for impl in $(rg -l 'implements .*ConcreteWalletStrategy'); do echo echo "Checking $impl" rg -n 'isChainIdSupported' -n "$impl" || echo " ✗ Missing isChainIdSupported in $impl" rg -n 'signAminoTransaction' -n "$impl" || echo " ✗ Missing signAminoTransaction in $impl" doneLength of output: 9204
#!/bin/bash # Check for method definitions in the ConcreteWalletStrategy interface echo "Searching for isChainIdSupported definitions in strategy.ts..." rg -n "isChainIdSupported\(" packages/wallets/wallet-base/src/types/strategy.ts || echo "No isChainIdSupported definition found." echo echo "Searching for signAminoTransaction definitions in strategy.ts..." rg -n "signAminoTransaction\(" packages/wallets/wallet-base/src/types/strategy.ts || echo "No signAminoTransaction definition found."
Ignore the suggestion to implement
isChainIdSupportedandsignAminoTransactionThe interface still omits both methods via
extends Omit<ConcreteCosmosWalletStrategy, 'sendTransaction' | 'isChainIdSupported' | 'signAminoTransaction'>Only
sendTransactionis re-declared in ConcreteWalletStrategy, so implementations are not expected to define the other two.Likely an incorrect or invalid review comment.
packages/sdk-ts/src/utils/address.spec.ts (1)
36-43: Good test case for checksum address verificationThe added test correctly verifies the
getChecksumAddressfunction by comparing a lowercase Ethereum address input to its expected checksummed output. This test improves coverage for address-related utilities.packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (2)
43-53: Implementation looks good, previous issue has been fixedI see you've addressed the previous comment about
this.turnkeynever being initialized. The method now correctly checks and initializesthis.turnkeyon line 49 if it doesn't exist.
213-214: Unresolved TODO commentThis TODO comment indicates a known issue that should be addressed before merging.
The current implementation doesn't reuse existing iframe clients, which may lead to multiple iframe instances and potential memory leaks. Please verify that this issue is tracked for resolution after the PR is merged.
| export class TurnkeyOauthWallet { | ||
| static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) { | ||
| try { | ||
| const targetPublicKey = iframeClient.iframePublicKey | ||
|
|
||
| if (!targetPublicKey) { | ||
| throw new WalletException(new Error('Target public key not found')) | ||
| } | ||
|
|
||
| return Array.from(sha256(new TextEncoder().encode(targetPublicKey))) | ||
| .map((b) => b.toString(16).padStart(2, '0')) | ||
| .join('') | ||
| } catch (e) { | ||
| throw new WalletException(new Error((e as any).message), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: 'turnkey-generate-oauth-nonce', | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| static async oauthLogin(args: { | ||
| client: HttpRestClient | ||
| oidcToken: string | ||
| providerName: 'google' | 'apple' | ||
| expirationSeconds?: number | ||
| iframeClient: TurnkeyIframeClient | ||
| oauthLoginPath?: string | ||
| }): Promise<{ credentialBundle: string } | void> { | ||
| const { client, iframeClient } = args | ||
|
|
||
| const path = args.oauthLoginPath || TURNKEY_OAUTH_PATH | ||
|
|
||
| try { | ||
| const targetPublicKey = iframeClient.iframePublicKey | ||
|
|
||
| if (!targetPublicKey) { | ||
| throw new WalletException(new Error('Target public key not found')) | ||
| } | ||
|
|
||
| const response = await client.$post<TurnkeyOauthLoginResponse>(path, { | ||
| targetPublicKey, | ||
| oidcToken: args.oidcToken, | ||
| providerName: args.providerName, | ||
| }) | ||
|
|
||
| return response | ||
| } catch (e) { | ||
| throw new WalletException(new Error((e as any).message), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: 'turnkey-oauth-login', | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TODO: should be able to remove this | ||
| static async loginUser(args: { | ||
| iframeClient: TurnkeyIframeClient | ||
| client: HttpRestClient | ||
| oidcToken: string | ||
| providerName: 'google' | 'apple' | ||
| setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void | ||
| }) { | ||
| const { client, iframeClient, oidcToken, providerName } = args | ||
| const result = await TurnkeyOauthWallet.oauthLogin({ | ||
| client, | ||
| iframeClient, | ||
| oidcToken, | ||
| providerName, | ||
| }) | ||
|
|
||
| if (result?.credentialBundle) { | ||
| args.setMetadata({ | ||
| turnkey: { | ||
| credentialBundle: result.credentialBundle, | ||
| }, | ||
| }) | ||
| iframeClient.injectCredentialBundle(result.credentialBundle) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor static-only class to module with functions
This class only contains static methods, which is considered an anti-pattern. Consider refactoring to a module with named exports for better maintainability and to follow functional programming principles.
-export class TurnkeyOauthWallet {
- static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) {
+export async function generateOAuthNonce(iframeClient: TurnkeyIframeClient) {
try {
const targetPublicKey = iframeClient.iframePublicKey
if (!targetPublicKey) {
throw new WalletException(new Error('Target public key not found'))
}
return Array.from(sha256(new TextEncoder().encode(targetPublicKey)))
.map((b) => b.toString(16).padStart(2, '0'))
.join('')
} catch (e) {
throw new WalletException(new Error((e as any).message), {
code: UnspecifiedErrorCode,
type: ErrorType.WalletError,
contextModule: 'turnkey-generate-oauth-nonce',
})
}
}
- static async oauthLogin(args: {
+export async function oauthLogin(args: {
client: HttpRestClient
oidcToken: string
providerName: 'google' | 'apple'
expirationSeconds?: number
iframeClient: TurnkeyIframeClient
oauthLoginPath?: string
- }): Promise<{ credentialBundle: string } | void> {
+ }): Promise<{ credentialBundle: string } | undefined> {Apply similar changes to the loginUser method.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-93: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 40-40: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| export class TurnkeyOtpWallet { | ||
| static async initEmailOTP(args: { | ||
| email: string | ||
| subOrgId?: string | ||
| client: HttpRestClient | ||
| iframeClient: TurnkeyIframeClient | ||
| invalidateExistingSessions?: boolean | ||
| otpInitPath?: string | ||
| }) { | ||
| const { client, iframeClient } = args | ||
|
|
||
| try { | ||
| const targetPublicKey = iframeClient.iframePublicKey | ||
|
|
||
| if (!targetPublicKey) { | ||
| throw new WalletException(new Error('Target public key not found')) | ||
| } | ||
|
|
||
| const response = await client.$post<TurnkeyOTPCredentialsResponse>( | ||
| args.otpInitPath || TURNKEY_OTP_INIT_PATH, | ||
| { | ||
| targetPublicKey, | ||
| email: args.email, | ||
| suborgId: args.subOrgId, | ||
| invalidateExistingSessions: args.invalidateExistingSessions, | ||
| }, | ||
| ) | ||
|
|
||
| return response | ||
| } catch (e) { | ||
| throw new WalletException(new Error((e as any).message), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: 'turnkey-init-email-otp', | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| static async confirmEmailOTP(args: { | ||
| otpCode: string | ||
| emailOTPId?: string | ||
| client: HttpRestClient | ||
| organizationId?: string | ||
| iframeClient: TurnkeyIframeClient | ||
| otpVerifyPath?: string | ||
| }) { | ||
| const { client, iframeClient } = args | ||
|
|
||
| try { | ||
| const organizationId = args.organizationId | ||
| const emailOTPId = args.emailOTPId | ||
| const targetPublicKey = iframeClient.iframePublicKey | ||
| const otpVerifyPath = args.otpVerifyPath || TURNKEY_OTP_VERIFY_PATH | ||
|
|
||
| if (!targetPublicKey) { | ||
| throw new WalletException(new Error('Target public key not found')) | ||
| } | ||
|
|
||
| if (!emailOTPId) { | ||
| throw new WalletException(new Error('Email OTP ID is required')) | ||
| } | ||
|
|
||
| if (!organizationId) { | ||
| throw new WalletException(new Error('Organization ID is required')) | ||
| } | ||
|
|
||
| const response = await client.$post<TurnkeyConfirmEmailOTPResponse>( | ||
| otpVerifyPath, | ||
| { | ||
| otpId: emailOTPId, | ||
| otpCode: args.otpCode, | ||
| suborgID: organizationId, | ||
| targetPublicKey, | ||
| }, | ||
| ) | ||
|
|
||
| return response | ||
| } catch (e) { | ||
| throw new WalletException(new Error((e as any).message), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: 'turnkey-confirm-email-otp', | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor static-only class to module with functions
Similar to the OAuth wallet, this class only contains static methods. Consider refactoring to a module with named exports for better maintainability and to follow functional programming principles.
-export class TurnkeyOtpWallet {
- static async initEmailOTP(args: {
+export async function initEmailOTP(args: {
email: string
subOrgId?: string
client: HttpRestClient
iframeClient: TurnkeyIframeClient
invalidateExistingSessions?: boolean
otpInitPath?: string
}) {Apply similar changes to the other methods in this class.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-146: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
3 times, most recently
from
de89897 to
af05b02
Compare
|
Flow diagram found here: https://www.tldraw.com/f/FSN_z4lqYjQoHsBm35kF1?d=v-1589.-1724.5917.5246.page |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
packages/wallets/wallet-turnkey/README.md (1)
36-56: Inconsistent indentation in code example.The indentation in this example is inconsistent, which affects readability.
const strategyArgs: WalletStrategyArguments = { chainId: ChainId.Mainnet, wallet: Wallet.Turnkey, strategies: { - [Wallet.Turnkey]: new TurnkeyWallet({ - onStatusChange(status) { - turnkeyStatus.value = status - }, - chainId: injectiveClients.chainId, - ethereumOptions: { - ethereumChainId: injectiveClients.ethereumChainId!, - }, - metadata: { - turnkeyAuthIframeContainerId, - defaultOrganizationId: import.meta.env - .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, - apiBaseUrl: 'https://api.turnkey.com', - }, - }) + [Wallet.Turnkey]: new TurnkeyWallet({ + onStatusChange(status) { + turnkeyStatus.value = status + }, + chainId: injectiveClients.chainId, + ethereumOptions: { + ethereumChainId: injectiveClients.ethereumChainId!, + }, + metadata: { + turnkeyAuthIframeContainerId, + defaultOrganizationId: import.meta.env + .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, + apiBaseUrl: 'https://api.turnkey.com', + }, + }) }, }packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (1)
12-96: Refactor static-only class to module with functionsThis class only contains static methods, which is an anti-pattern. Consider refactoring to a module with named exports for better maintainability and to follow functional programming principles.
-export class TurnkeyOauthWallet { - static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) { +export async function generateOAuthNonce(iframeClient: TurnkeyIframeClient) { try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } return Array.from(sha256(new TextEncoder().encode(targetPublicKey))) .map((b) => b.toString(16).padStart(2, '0')) .join('') } catch (e) { throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-generate-oauth-nonce', }) } - } +} - static async oauthLogin(args: { +export async function oauthLogin(args: { client: HttpRestClient oidcToken: string providerName: 'google' | 'apple' expirationSeconds?: number iframeClient: TurnkeyIframeClient oauthLoginPath?: string }): Promise<{ credentialBundle: string } | undefined> { const { client, iframeClient } = args const path = args.oauthLoginPath || TURNKEY_OAUTH_PATH try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } // client.$post is undefined, resorting to this for now const response = await client.post<{ data: TurnkeyOauthLoginResponse }>(path, { targetPublicKey, oidcToken: args.oidcToken, providerName: args.providerName, }) return response.data } catch (e) { throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-oauth-login', }) } - } +} // TODO: should be able to remove this - static async loginUser(args: { +export async function loginUser(args: { iframeClient: TurnkeyIframeClient client: HttpRestClient oidcToken: string providerName: 'google' | 'apple' setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void }) { const { client, iframeClient, oidcToken, providerName } = args - const result = await TurnkeyOauthWallet.oauthLogin({ + const result = await oauthLogin({ client, iframeClient, oidcToken, providerName, }) if (result?.credentialBundle) { args.setMetadata({ turnkey: { credentialBundle: result.credentialBundle, }, }) iframeClient.injectCredentialBundle(result.credentialBundle) } - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 12-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (1)
14-142: Refactor static-only class to module with functionsThis class only contains static methods, which is an anti-pattern. Consider refactoring to a module with named exports for better maintainability and to follow functional programming principles.
-export class TurnkeyOtpWallet { - static async initEmailOTP(args: { +export async function initEmailOTP(args: { email: string subOrgId?: string client: HttpRestClient iframeClient: TurnkeyIframeClient invalidateExistingSessions?: boolean otpInitPath?: string }) { const { client, iframeClient } = args try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } // client.$post is undefined, resorting to this for now const response = await client.post<{ data?: TurnkeyOTPCredentialsResponse }>(args.otpInitPath || TURNKEY_OTP_INIT_PATH, { targetPublicKey, email: args.email, suborgId: args.subOrgId, invalidateExistingSessions: args.invalidateExistingSessions, }) return response?.data } catch (e) { throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-init-email-otp', }) } - } +} - static async confirmEmailOTP(args: { +export async function confirmEmailOTP(args: { otpCode: string emailOTPId?: string client: HttpRestClient organizationId?: string iframeClient: TurnkeyIframeClient otpVerifyPath?: string }) { const { client, iframeClient } = args try { const organizationId = args.organizationId const emailOTPId = args.emailOTPId const targetPublicKey = iframeClient.iframePublicKey const otpVerifyPath = args.otpVerifyPath || TURNKEY_OTP_VERIFY_PATH if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } if (!emailOTPId) { throw new WalletException(new Error('Email OTP ID is required')) } if (!organizationId) { throw new WalletException(new Error('Organization ID is required')) } const response = await client.post<{ data?: TurnkeyConfirmEmailOTPResponse }>(otpVerifyPath, { otpId: emailOTPId, otpCode: args.otpCode, suborgID: organizationId, targetPublicKey, }) return response?.data } catch (e) { throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-confirm-email-otp', }) } - } +} // TODO: should be able to remove this - static async loginUser(args: { +export async function loginUser(args: { client: HttpRestClient iframeClient: TurnkeyIframeClient organizationId: string otpCode: string emailOTPId: string setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void }) { const { client, iframeClient, organizationId, otpCode, emailOTPId, setMetadata, } = args - const result = await TurnkeyOtpWallet.confirmEmailOTP({ + const result = await confirmEmailOTP({ client, iframeClient, otpCode, emailOTPId, organizationId, }) if (result?.credentialBundle) { setMetadata({ turnkey: { credentialBundle: result.credentialBundle, }, }) await iframeClient.injectCredentialBundle(result.credentialBundle) await iframeClient.refreshSession({ sessionType: SessionType.READ_WRITE, targetPublicKey: iframeClient.iframePublicKey, expirationSeconds: '900', }) } - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (1)
179-201: 🛠️ Refactor suggestionAddress key casing may create duplicate entries in accountMap
The current implementation uses both the raw and lowercase address as keys for cache lookups, but only stores using the original casing. This can lead to cache misses and duplicate API calls if future calls use a different checksum/case.
- if (accountMap[address] || accountMap[address.toLowerCase()]) { - return accountMap[address] || accountMap[address.toLowerCase()] + const key = address.toLowerCase() + if (accountMap[key]) { + return accountMap[key] } if (!organizationId) { throw new WalletException(new Error('Organization ID is required')) } iframeClient.config.organizationId = organizationId if (!address) { throw new WalletException(new Error('Account address not found')) } const turnkeyAccount = await createAccount({ organizationId, signWith: address, client: iframeClient, }) - this.accountMap[address] = turnkeyAccount + this.accountMap[key] = turnkeyAccount return turnkeyAccountpackages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (1)
79-98:enable()leaves status as Initializing on failureIf any step inside
trythrows, wecatchand returnfalse, but the wallet status never reverts fromInitializing, which can mislead callers/UI.- try { ... return !!(await turnkeyWallet.getIframeClient()) - } catch (e) { - return false + } catch (e) { + await this.setStatus(TurnkeyStatus.Error) + return false }
🧹 Nitpick comments (17)
packages/wallets/wallet-turnkey/README.md (4)
28-29: Fix punctuation for documentation clarity.There's unusual punctuation here where a period is immediately followed by a colon.
-Here's a brief example of how to use this package to send 1 INJ.: +Here's a brief example of how to use this package to send 1 INJ:🧰 Tools
🪛 LanguageTool
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
108-108: Update copyright year range.The copyright year range is outdated. It should include the current year.
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021 - 2025 Injective Labs Inc. (https://injectivelabs.org/)🧰 Tools
🪛 LanguageTool
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
24-24: Add missing comma for better readability.A comma is missing after "installed" and before "you must".
-Regardless of which wallet package(s) you choose to use you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base` +Regardless of which wallet package(s) you choose to use, you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base`🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
110-110: Add alt text to image for accessibility.The image tag is missing alternative text, which is important for accessibility.
-<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px; max-width: 100%; height: auto" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Images should have alternate text (alt text)
null(MD045, no-alt-text)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (1)
52-53: Remove or update the comment about client.$postThe comment about "client.$post is undefined, resorting to this for now" suggests a temporary workaround. Either implement the intended solution or remove the comment if this is the permanent approach.
- // client.$post is undefined, resorting to this for now const response = await client.post<{packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (2)
32-33: Remove or update the comment about client.$postThe comment about "client.$post is undefined, resorting to this for now" suggests a temporary workaround. Either implement the intended solution or remove the comment if this is the permanent approach.
- // client.$post is undefined, resorting to this for now const response = await client.post<{
138-138: Make session expiration time configurableThe session expiration time is hardcoded to '900' seconds. Consider making this configurable through a parameter.
await iframeClient.refreshSession({ sessionType: SessionType.READ_WRITE, targetPublicKey: iframeClient.iframePublicKey, - expirationSeconds: '900', + expirationSeconds: args.expirationSeconds || '900', })packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (3)
91-95: Extract session expiration time as a configuration parameterThe session expiration time is hardcoded to '900' seconds (15 minutes) in multiple places. Consider extracting this as a configurable parameter in the class constructor to make it more flexible.
export class TurnkeyWallet { protected iframeClient: TurnkeyIframeClient | undefined protected turnkey: Turnkey | undefined protected client: HttpRestClient private metadata: TurnkeyMetadata + private sessionExpirationSeconds: string private accountMap: Record< string, Awaited<ReturnType<typeof createAccount>> > = {} constructor(metadata: TurnkeyMetadata) { this.metadata = metadata this.client = new HttpRestClient(metadata.apiServerEndpoint) + this.sessionExpirationSeconds = metadata.sessionExpirationSeconds || '900' }Then update the references:
await iframeClient.refreshSession({ sessionType: SessionType.READ_WRITE, targetPublicKey: iframeClient.iframePublicKey, - expirationSeconds: '900', + expirationSeconds: this.sessionExpirationSeconds, })Also applies to: 208-212
63-65: Remove unnecessary type assertionThe non-null assertion in the return statement is unnecessary as you've already checked for null/undefined in the preceding if statement.
if (!this.iframeClient) { throw new WalletException(new Error('Iframe client not initialized')) } - return this.iframeClient as TurnkeyIframeClient + return this.iframeClient }
52-53: Remove unnecessary type assertionThe non-null assertion in the return statement is unnecessary as you've already checked for null/undefined in the preceding if statement.
if (!this.turnkey) { this.turnkey = new Turnkey(this.metadata) } - return this.turnkey as Turnkey + return this.turnkey }packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (7)
117-118: Typo in comment: CHeck → CheckMinor typo in comment.
- // CHeck if the user is already connected + // Check if the user is already connected
166-176: Improve error handling with proper type castingUsing
as anyfor error type casting reduces type safety. Consider creating a proper interface for Turnkey errors.- if ((e as any).contextCode === TurnkeyErrorCodes.UserLoggedOut) { + if (e instanceof Error && 'contextCode' in e && e.contextCode === TurnkeyErrorCodes.UserLoggedOut) { await this.disconnect() throw e } - throw new WalletException(new Error((e as any).message), { + throw new WalletException(new Error(e instanceof Error ? e.message : String(e)), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: WalletAction.GetAccounts, })
188-193: Simplify complex conditional logicThe negated and complex condition makes the code flow harder to follow. Consider refactoring for clarity.
- if ( - // If either of these values exist on the metadata, then we want to proceed with the login flow - !this.metadata?.turnkey?.email && - !this.metadata?.turnkey?.oidcToken && - session.session?.token - ) { + // If we have a valid session token and no login credentials, use the existing session + if (session.session?.token && + !this.metadata?.turnkey?.email && + !this.metadata?.turnkey?.oidcToken) {
241-252: Remove redundant condition checkThe condition
oauthResult?.credentialBundleis checked twice, which is unnecessary.if (oauthResult?.credentialBundle) { await turnkeyWallet.injectAndRefresh(oauthResult.credentialBundle) - } - - if (oauthResult?.credentialBundle) { const session = await turnkeyWallet.getSession() if (this.metadata?.turnkey && session.organizationId) { this.metadata.turnkey.organizationId = session.organizationId } return oauthResult.credentialBundle }
313-314: Consider documenting thegetAddresschoice and potential alternativesThe comment explains why
getAddressfrom viem is used instead ofgetChecksumAddressfrom sdk-ts, but it's worth documenting if there are plans to update the sdk-ts implementation.Add a more detailed comment explaining the discrepancy and any plans to address it in the future.
37-37: Consider makingDEFAULT_TURNKEY_API_ENDPOINTconfigurable through environment variablesHardcoded API endpoints can cause deployment issues in different environments. Consider making this configurable.
-const DEFAULT_TURNKEY_API_ENDPOINT = 'https://api.ui.injective.network/api/v1' +const DEFAULT_TURNKEY_API_ENDPOINT = process.env.TURNKEY_API_ENDPOINT || 'https://api.ui.injective.network/api/v1'
280-281: Return typeanycould be more specificThe return type of
sendTransactioncould be more specific thanany, which would improve type safety.- async sendTransaction( - transaction: TxRaw, - options: SendTransactionOptions, - ): Promise<any> { + async sendTransaction( + transaction: TxRaw, + options: SendTransactionOptions, + ): Promise<TxResponse> {You'll need to import
TxResponsefrom the appropriate package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
packages/exceptions/src/exceptions/types/codes.ts(1 hunks)packages/sdk-ts/src/utils/address.spec.ts(2 hunks)packages/wallets/wallet-base/src/base.ts(2 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(2 hunks)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts(3 hunks)packages/wallets/wallet-cosmos/src/strategy/strategy.ts(3 hunks)packages/wallets/wallet-cosmostation/src/strategy/strategy.ts(6 hunks)packages/wallets/wallet-magic/src/strategy/strategy.ts(5 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-strategy/package.json(1 hunks)packages/wallets/wallet-strategy/src/strategy/index.ts(7 hunks)packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/consts.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/types.ts(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.esm.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.json(1 hunks)packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/sdk-ts/src/utils/address.spec.ts
- packages/wallets/wallet-turnkey/src/index.ts
- packages/wallets/wallet-turnkey/src/strategy/consts.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/wallets/wallet-turnkey/tsconfig.json
- packages/exceptions/src/exceptions/types/codes.ts
- packages/wallets/wallet-base/src/types/enums.ts
- packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts
- packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts
- packages/wallets/wallet-turnkey/tsconfig.build.json
- packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts
- packages/wallets/wallet-base/src/base.ts
- packages/wallets/wallet-strategy/package.json
- packages/wallets/wallet-turnkey/tsconfig.build.esm.json
- packages/wallets/wallet-cosmos/src/strategy/strategy.ts
- packages/wallets/wallet-base/src/utils/wallet.ts
- packages/wallets/wallet-cosmostation/src/strategy/strategy.ts
- packages/wallets/wallet-private-key/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/package.json
- packages/wallets/wallet-magic/src/strategy/strategy.ts
- packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts
- packages/wallets/wallet-strategy/src/strategy/index.ts
- packages/wallets/wallet-turnkey/src/strategy/types.ts
- packages/wallets/wallet-base/src/types/strategy.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (5)
packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(23-139)packages/wallets/wallet-turnkey/src/strategy/consts.ts (1)
TURNKEY_OAUTH_PATH(1-1)packages/wallets/wallet-turnkey/src/strategy/types.ts (1)
TurnkeyOauthLoginResponse(28-31)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
packages/wallets/wallet-turnkey/README.md
108-108: Bare URL used
null
(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
115-115: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts
[error] 12-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| if ((e as any).code === TurnkeyErrorCodes.UserLoggedOut) { | ||
| throw new WalletException(new Error('User is not logged in'), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: WalletAction.GetAccounts, | ||
| contextCode: TurnkeyErrorCodes.UserLoggedOut, | ||
| }) | ||
| } | ||
|
|
||
| throw new WalletException(new Error((e as any).message), { | ||
| code: UnspecifiedErrorCode, | ||
| type: ErrorType.WalletError, | ||
| contextModule: 'turnkey-wallet-get-accounts', | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhance error handling with specific error codes
The error handling uses UnspecifiedErrorCode for most errors except for UserLoggedOut. Consider adding more specific error codes for better error identification and handling.
🏁 Script executed:
#!/bin/bash
# Check if there are more specific error codes defined in TurnkeyErrorCodes
rg --type ts -A 5 "export (const|enum) TurnkeyErrorCodes"Length of output: 493
Define and map specific Turnkey error codes
Currently, TurnkeyErrorCodes (packages/wallets/wallet-turnkey/src/strategy/types.ts) only includes UserLoggedOut. To improve error granularity, extend this enum with codes for each distinct error returned by Turnkey, and update the catch block in packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (lines 155–169) to switch on (e as any).code, throwing a WalletException with the matching contextCode instead of always using UnspecifiedErrorCode.
• packages/wallets/wallet-turnkey/src/strategy/types.ts
– Add new members to TurnkeyErrorCodes for each error scenario.
• packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (L155–169)
– Replace the final throw with a switch/case over TurnkeyErrorCodes, mapping each to a specific WalletException.
| async sendEthereumTransaction( | ||
| _transaction: unknown, | ||
| _options: { address: AccountAddress; ethereumChainId: EthereumChainId }, | ||
| ): Promise<string> { | ||
| throw new WalletException( | ||
| new Error( | ||
| 'sendEthereumTransaction is not supported. Turnkey only supports sending cosmos transactions', | ||
| ), | ||
| { | ||
| code: UnspecifiedErrorCode, | ||
| context: WalletAction.SendEthereumTransaction, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify error message for sendEthereumTransaction
The error message states that Turnkey only supports sending cosmos transactions, but other methods like signCosmosTransaction and signAminoCosmosTransaction throw errors stating they're not supported. This is contradictory and could confuse users.
- new Error(
- 'sendEthereumTransaction is not supported. Turnkey only supports sending cosmos transactions',
- ),
+ new Error(
+ 'sendEthereumTransaction is not supported. Turnkey only supports broadcasting pre-signed cosmos transactions',
+ ),📝 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.
| async sendEthereumTransaction( | |
| _transaction: unknown, | |
| _options: { address: AccountAddress; ethereumChainId: EthereumChainId }, | |
| ): Promise<string> { | |
| throw new WalletException( | |
| new Error( | |
| 'sendEthereumTransaction is not supported. Turnkey only supports sending cosmos transactions', | |
| ), | |
| { | |
| code: UnspecifiedErrorCode, | |
| context: WalletAction.SendEthereumTransaction, | |
| }, | |
| ) | |
| } | |
| async sendEthereumTransaction( | |
| _transaction: unknown, | |
| _options: { address: AccountAddress; ethereumChainId: EthereumChainId }, | |
| ): Promise<string> { | |
| throw new WalletException( | |
| new Error( | |
| 'sendEthereumTransaction is not supported. Turnkey only supports broadcasting pre-signed cosmos transactions', | |
| ), | |
| { | |
| code: UnspecifiedErrorCode, | |
| context: WalletAction.SendEthereumTransaction, | |
| }, | |
| ) | |
| } |
billyjacoby
force-pushed
the
feat/turnkey-wallet
branch
from
af05b02 to
7210bc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (13)
packages/wallets/wallet-turnkey/README.md (2)
1-121: New Turnkey wallet strategy README looks good overall, with a few clarity improvements neededThe README provides a clear overview of the Turnkey wallet strategy, including installation instructions and useful examples. A few improvements would enhance the documentation:
Consider these improvements:
- Add a missing comma in line 24: "Regardless of which wallet package(s) you choose to use, you must also have..."
- Fix the inconsistent indentation in the code example (lines 36-54)
- Remove the colon after "INJ" in line 28 to avoid double punctuation
- Add alt text to the image in line 110 for accessibility
- Consider adding a brief explanation of what Turnkey is and its benefits
-Regardless of which wallet package(s) you choose to use you must also have +Regardless of which wallet package(s) you choose to use, you must also have-Here's a brief example of how to use this package to send 1 INJ.: +Here's a brief example of how to use this package to send 1 INJ:-<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px; max-width: 100%; height: auto" />🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...(ALL_OF_THE)
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null(MD045, no-alt-text)
115-115: Bare URL used
null(MD034, no-bare-urls)
40-54: Fix inconsistent code formatting in exampleThe indentation in the TurnkeyWallet configuration example is inconsistent, making it harder to read.
Apply consistent indentation throughout the example:
[Wallet.Turnkey]: new TurnkeyWallet({ - onStatusChange(status) { - turnkeyStatus.value = status - }, - chainId: injectiveClients.chainId, - ethereumOptions: { - ethereumChainId: injectiveClients.ethereumChainId!, - }, - metadata: { - turnkeyAuthIframeContainerId, - defaultOrganizationId: import.meta.env - .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, - apiBaseUrl: 'https://api.turnkey.com', - }, + onStatusChange(status) { + turnkeyStatus.value = status + }, + chainId: injectiveClients.chainId, + ethereumOptions: { + ethereumChainId: injectiveClients.ethereumChainId!, + }, + metadata: { + turnkeyAuthIframeContainerId, + defaultOrganizationId: import.meta.env + .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, + apiBaseUrl: 'https://api.turnkey.com', + }, })packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (3)
12-96: Refactor static-only class to module with functionsThis class only contains static methods, which is considered an anti-pattern. A functional approach would be more maintainable.
Consider converting the static class to exported functions:
-export class TurnkeyOauthWallet { - static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) { +export async function generateOAuthNonce(iframeClient: TurnkeyIframeClient) { try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } return Array.from(sha256(new TextEncoder().encode(targetPublicKey))) .map((b) => b.toString(16).padStart(2, '0')) .join('') } catch (e) { throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-generate-oauth-nonce', }) } } - static async oauthLogin(args: { +export async function oauthLogin(args: { client: HttpRestClient oidcToken: string providerName: 'google' | 'apple' expirationSeconds?: number iframeClient: TurnkeyIframeClient oauthLoginPath?: string }): Promise<{ credentialBundle: string } | undefined> { // ... rest of the function } - static async loginUser(args: { +export async function loginUser(args: { // ... rest of the function } -}This approach aligns better with functional programming principles and improves maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
71-96:⚠️ Potential issueRemove unused
loginUsermethodThe method has a TODO comment indicating it should be removed, and it appears to be unused in the codebase.
Remove the entire method and its TODO comment:
- // TODO: should be able to remove this - static async loginUser(args: { - iframeClient: TurnkeyIframeClient - client: HttpRestClient - oidcToken: string - providerName: 'google' | 'apple' - setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void - }) { - const { client, iframeClient, oidcToken, providerName } = args - const result = await TurnkeyOauthWallet.oauthLogin({ - client, - iframeClient, - oidcToken, - providerName, - }) - - if (result?.credentialBundle) { - args.setMetadata({ - turnkey: { - credentialBundle: result.credentialBundle, - }, - }) - iframeClient.injectCredentialBundle(result.credentialBundle) - } - }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
40-40: 🛠️ Refactor suggestionReplace
voidin union type withundefinedUsing
voidin a union type can lead to confusing behavior.- }): Promise<{ credentialBundle: string } | void> { + }): Promise<{ credentialBundle: string } | undefined> {
voidrepresents the absence of a return value, whileundefinedrepresents a value that hasn't been assigned. In union types,undefinedis more appropriate.packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (2)
14-142: Refactor static-only class to module with functionsThis class only contains static methods, which is considered an anti-pattern. A functional approach would be more maintainable.
Consider converting the static class to exported functions:
-export class TurnkeyOtpWallet { - static async initEmailOTP(args: { +export async function initEmailOTP(args: { email: string subOrgId?: string client: HttpRestClient iframeClient: TurnkeyIframeClient invalidateExistingSessions?: boolean otpInitPath?: string }) { // ... implementation } - static async confirmEmailOTP(args: { +export async function confirmEmailOTP(args: { // ... implementation } - static async loginUser(args: { +export async function loginUser(args: { // ... implementation } -}This approach aligns better with functional programming principles and improves maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
99-141:⚠️ Potential issueRemove unused
loginUsermethodThe method has a TODO comment indicating it should be removed, and analysis confirms it's unused in the codebase.
Remove the entire method and its TODO comment:
- // TODO: should be able to remove this - - static async loginUser(args: { - client: HttpRestClient - iframeClient: TurnkeyIframeClient - organizationId: string - otpCode: string - emailOTPId: string - setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void - }) { - // ... entire method implementation - }packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (3)
179-201: 🛠️ Refactor suggestionFix address lookup in account map to prevent duplicate entries
The current implementation looks up accounts using both the original address and lowercase, but only stores them with the original casing, which could lead to duplicate API calls for the same address with different casing.
Always use lowercase for keys in the account map to ensure consistent lookups:
- if (accountMap[address] || accountMap[address.toLowerCase()]) { - return accountMap[address] || accountMap[address.toLowerCase()] + const lowercaseAddress = address.toLowerCase() + if (accountMap[lowercaseAddress]) { + return accountMap[lowercaseAddress] } // ... - this.accountMap[address] = turnkeyAccount + this.accountMap[lowercaseAddress] = turnkeyAccountThis ensures consistent key usage in the account map and prevents redundant API calls.
155-169: 🛠️ Refactor suggestionDefine specific error codes for better error handling
Currently, most errors use
UnspecifiedErrorCodeexcept forUserLoggedOut. This limits error identification and handling capabilities.Expand the
TurnkeyErrorCodesenum and map specific errors:
- First, extend the error codes in
packages/wallets/wallet-turnkey/src/strategy/types.ts:export enum TurnkeyErrorCodes { UserLoggedOut = 'USER_LOGGED_OUT', AccountNotFound = 'ACCOUNT_NOT_FOUND', InvalidCredentials = 'INVALID_CREDENTIALS', NetworkError = 'NETWORK_ERROR', // Add other specific error codes }
- Then update the error handling to use specific codes:
} catch (e) { if ((e as any).code === TurnkeyErrorCodes.UserLoggedOut) { throw new WalletException(new Error('User is not logged in'), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: WalletAction.GetAccounts, contextCode: TurnkeyErrorCodes.UserLoggedOut, }) + } else if ((e as any).code === 'NOT_FOUND') { + throw new WalletException(new Error('Account not found'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: WalletAction.GetAccounts, + contextCode: TurnkeyErrorCodes.AccountNotFound, + }) } throw new WalletException(new Error((e as any).message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-wallet-get-accounts', + contextCode: TurnkeyErrorCodes.NetworkError, })This provides more specific error handling and improves the developer experience when debugging issues.
48-52: 🛠️ Refactor suggestionEnsure consistent Turnkey instance initialization
The current implementation initializes
this.turnkeyin getTurnkey() but not in the initFrame() method, which could lead to inconsistent state.Update getTurnkey() to use the initFrame() result consistently:
public async getTurnkey(): Promise<Turnkey> { if (!this.iframeClient) { await this.initFrame() } - if (!this.turnkey) { - this.turnkey = new Turnkey(this.metadata) - } + if (!this.turnkey) { + throw new WalletException(new Error('Turnkey SDK not initialized')) + } - return this.turnkey as Turnkey + return this.turnkey }This ensures that the Turnkey instance is always initialized through the same path (initFrame) and adds proper error handling if initialization fails.
packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (3)
79-98:enable()leaves status as Initializing on failureIf any step inside
trythrows, wecatchand returnfalse, but the wallet status never reverts fromInitializing, which can mislead callers/UI.async enable(): Promise<boolean> { const turnkeyWallet = await this.getTurnkeyWallet() try { const session = await turnkeyWallet.getSession() if (session.session) { // User is already logged in, we don't need to do anything in the next steps if (this.metadata?.turnkey) { this.metadata.turnkey.session = session.session } return true } return !!(await turnkeyWallet.getIframeClient()) } catch (e) { + await this.setStatus(TurnkeyStatus.Error) return false } }
263-276: Clarify error message forsendEthereumTransactionThe error message states that Turnkey only supports sending cosmos transactions, but other methods like
signCosmosTransactionandsignAminoCosmosTransactionthrow errors stating they're not supported. This is contradictory and could confuse users.new Error( - 'sendEthereumTransaction is not supported. Turnkey only supports sending cosmos transactions', + 'sendEthereumTransaction is not supported. Turnkey only supports broadcasting pre-signed cosmos transactions', ),
325-325: Add error handling for JSON parsingJSON.parse can throw an exception if the input is malformed. Add a try/catch block for more robust error handling.
- const signature = await account.signTypedData(JSON.parse(eip712json)) + let parsedData; + try { + parsedData = JSON.parse(eip712json); + } catch (e) { + throw new WalletException(new Error('Failed to parse EIP-712 data: Invalid JSON format'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: WalletAction.SignTransaction, + }); + } + + const signature = await account.signTypedData(parsedData)
🧹 Nitpick comments (8)
packages/wallets/wallet-turnkey/README.md (1)
108-108: Use en dash for date rangesFor improved typographic style, use an en dash (–) instead of a hyphen for date ranges.
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021–2022 Injective Labs Inc. (https://injectivelabs.org/)🧰 Tools
🪛 LanguageTool
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (4)
60-64: Use consistent null checking and error message formattingThe error message for missing iframe client differs in format from other error messages, and the type assertion can be avoided.
Improve consistency in error handling:
if (!this.iframeClient) { - throw new WalletException(new Error('Iframe client not initialized')) + throw new WalletException(new Error('Iframe client not initialized'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: 'turnkey-wallet-get-iframe-client', + }) } - return this.iframeClient as TurnkeyIframeClient + return this.iframeClientThis makes error handling consistent with other methods and removes the unnecessary type assertion.
211-211: Consistency issue: expirationSeconds as stringThis is a duplicate of the issue in line 94 where expirationSeconds is passed as a string '900'.
For consistency, ensure all instances of expirationSeconds are handled the same way:
- expirationSeconds: '900', + expirationSeconds: 900, // Or keep as string if that's what the API expects
133-153: Improve error handling for Promise.allSettled resultsThe code filters for fulfilled promises but doesn't log or handle rejected promises, which could hide important errors.
Add logging for rejected promises to improve debuggability:
const accounts = await Promise.allSettled( response.wallets.map((wallet) => iframeClient.getWalletAccounts({ organizationId, walletId: wallet.walletId, }), ), ) + + // Log rejected promises for debugging + accounts + .filter((account) => account.status === 'rejected') + .forEach((result) => { + console.warn('Failed to fetch wallet account:', (result as PromiseRejectedResult).reason) + }) + const filteredAccounts = accounts .filter((account) => account.status === 'fulfilled') .flatMap((result) => result.value?.accounts)This helps identify issues with specific wallet accounts while allowing the overall operation to continue.
183-185: Add more context to the organizationId errorThe error for missing organizationId could be more helpful with additional context.
Enhance the error message:
if (!organizationId) { - throw new WalletException(new Error('Organization ID is required')) + throw new WalletException(new Error('Organization ID is required for account creation'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: 'turnkey-wallet-create-account', + }) }This provides more context about where and why the organizationId is required.
packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (3)
117-118: Fix typo in commentThere's a small typo in the comment.
- // CHeck if the user is already connected + // Check if the user is already connected
163-177: Use specific error type informationThe error handling could be improved by using more specific error properties instead of casting to
any.try { return await turnkeyWallet.getAccounts(organizationId) } catch (e: unknown) { - if ((e as any).contextCode === TurnkeyErrorCodes.UserLoggedOut) { + const error = e as { contextCode?: number; message?: string } + if (error.contextCode === TurnkeyErrorCodes.UserLoggedOut) { await this.disconnect() throw e } - throw new WalletException(new Error((e as any).message), { + throw new WalletException(new Error(error.message || 'Unknown error getting accounts'), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: WalletAction.GetAccounts, }) }
245-252: Remove redundant condition checkThe condition
oauthResult?.credentialBundleis checked twice, making the second check redundant.- if (oauthResult?.credentialBundle) { - const session = await turnkeyWallet.getSession() - - if (this.metadata?.turnkey && session.organizationId) { - this.metadata.turnkey.organizationId = session.organizationId - } - return oauthResult.credentialBundle - } + const session = await turnkeyWallet.getSession() + + if (this.metadata?.turnkey && session.organizationId) { + this.metadata.turnkey.organizationId = session.organizationId + } + return oauthResult.credentialBundle
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
packages/exceptions/src/exceptions/types/codes.ts(1 hunks)packages/sdk-ts/src/utils/address.spec.ts(2 hunks)packages/wallets/wallet-base/src/base.ts(2 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(2 hunks)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts(3 hunks)packages/wallets/wallet-cosmos/src/strategy/strategy.ts(3 hunks)packages/wallets/wallet-cosmostation/src/strategy/strategy.ts(6 hunks)packages/wallets/wallet-magic/src/strategy/strategy.ts(5 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-strategy/package.json(1 hunks)packages/wallets/wallet-strategy/src/strategy/index.ts(7 hunks)packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/consts.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/types.ts(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.esm.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.json(1 hunks)packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts
- packages/sdk-ts/src/utils/address.spec.ts
- packages/wallets/wallet-turnkey/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/wallets/wallet-turnkey/tsconfig.json
- packages/exceptions/src/exceptions/types/codes.ts
- packages/wallets/wallet-base/src/types/enums.ts
- packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts
- packages/wallets/wallet-strategy/package.json
- packages/wallets/wallet-private-key/src/strategy/strategy.ts
- packages/wallets/wallet-base/src/base.ts
- packages/wallets/wallet-cosmos/src/strategy/strategy.ts
- packages/wallets/wallet-magic/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/tsconfig.build.json
- packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts
- packages/wallets/wallet-turnkey/tsconfig.build.esm.json
- packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts
- packages/wallets/wallet-cosmostation/src/strategy/strategy.ts
- packages/wallets/wallet-base/src/utils/wallet.ts
- packages/wallets/wallet-turnkey/package.json
- packages/wallets/wallet-turnkey/src/strategy/consts.ts
- packages/wallets/wallet-turnkey/src/strategy/types.ts
- packages/wallets/wallet-base/src/types/strategy.ts
- packages/wallets/wallet-strategy/src/strategy/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (4)
packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(23-139)packages/wallets/wallet-turnkey/src/strategy/types.ts (2)
TurnkeyOTPCredentialsResponse(19-22)TurnkeyConfirmEmailOTPResponse(24-26)packages/wallets/wallet-turnkey/src/strategy/consts.ts (2)
TURNKEY_OTP_INIT_PATH(3-3)TURNKEY_OTP_VERIFY_PATH(4-4)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
packages/wallets/wallet-turnkey/README.md
108-108: Bare URL used
null
(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
115-115: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts
[error] 12-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (1)
94-94:❓ Verification inconclusive
Ensure expirationSeconds is passed as a number, not a string
The expirationSeconds parameter is passed as a string '900', but it should likely be a number.
Check the TurnkeyIframeClient type definition to confirm whether expirationSeconds should be a string or number:
If it expects a number, update this line to pass a number instead of a string.
🏁 Script executed:
#!/bin/bash # Find TurnkeyIframeClient refreshSession parameter types rg --type ts "refreshSession.*expirationSeconds" -A 3 -B 3Length of output: 60
Confirm
expirationSecondsTypeI couldn’t locate the
expirationSecondsdefinition in the local codebase—this appears to live in the externalTurnkeyIframeClientAPI. Please verify in the TurnkeyIframeClient type (e.g. in@turnkey/iframe-client) whetherrefreshSessionexpects a number. If so, update accordingly:- expirationSeconds: '900', + expirationSeconds: 900,
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (4)
84-103:enable()leaves status as Initializing on failureIf any step inside
trythrows, wecatchand returnfalse, but the wallet status never reverts fromInitializing, which can mislead callers/UI.Apply this fix:
async enable(): Promise<boolean> { const turnkeyWallet = await this.getTurnkeyWallet() try { const session = await turnkeyWallet.getSession() if (session.session) { // User is already logged in, we don't need to do anything in the next steps if (this.metadata?.turnkey) { this.metadata.turnkey.session = session.session } return true } return !!(await turnkeyWallet.getIframeClient()) } catch (e) { + await this.setStatus(TurnkeyStatus.Error) return false } }
349-349: Add error handling for JSON parsingJSON.parse can throw an exception if the input is malformed. Add a try/catch block for more robust error handling.
- const signature = await account.signTypedData(JSON.parse(eip712json)) + let parsedData; + try { + parsedData = JSON.parse(eip712json); + } catch (e) { + throw new WalletException(new Error('Failed to parse EIP-712 data: Invalid JSON format'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: WalletAction.SignTransaction, + }); + } + + const signature = await account.signTypedData(parsedData)
287-300: Clarify error message forsendEthereumTransactionThe error message states that Turnkey only supports sending cosmos transactions, but other methods like
signCosmosTransactionandsignAminoCosmosTransactionthrow errors stating they're not supported. This is contradictory and could confuse users.- new Error( - 'sendEthereumTransaction is not supported. Turnkey only supports sending cosmos transactions', - ), + new Error( + 'sendEthereumTransaction is not supported. Turnkey only supports broadcasting pre-signed cosmos transactions', + ),
199-199: Missing await for asynchronous callThe
injectCredentialBundlemethod is asynchronous but is called without await, which may lead to race conditions or unexpected behavior.- await iframeClient.injectCredentialBundle(session.session?.token) + await iframeClient.injectCredentialBundle(session.session?.token)
🧹 Nitpick comments (6)
packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (6)
122-122: Fix typo in commentThere's a small typo in the comment.
- // CHeck if the user is already connected + // Check if the user is already connected
171-176: Use type guard for error handlingUsing type assertion with
as anyis not type-safe. Consider using a type guard instead.- if ((e as any).contextCode === TurnkeyErrorCodes.UserLoggedOut) { + if (e && typeof e === 'object' && 'contextCode' in e && + e.contextCode === TurnkeyErrorCodes.UserLoggedOut) { await this.disconnect() throw e }
246-251: Remove redundant credentialBundle checkThe second check for
oauthResult?.credentialBundleis redundant since it's already checked above.if (oauthResult?.credentialBundle) { await turnkeyWallet.injectAndRefresh(oauthResult.credentialBundle) } - if (oauthResult?.credentialBundle) { + if (oauthResult?.credentialBundle) { const session = await turnkeyWallet.getSession() if (this.metadata?.turnkey && session.organizationId) { this.metadata.turnkey.organizationId = session.organizationId } return oauthResult.credentialBundle }
337-338: Add comment explaining checksum address importanceThe comment about checksummed addresses is incomplete and could be more informative.
- //? Turnkey expects the case sensitive address and the current impl of getChecksumAddress from sdk-ts doesn't play nice with browser envs + // Turnkey requires checksummed Ethereum addresses (mixed case) for proper security. + // We use viem's getAddress() instead of sdk-ts's getChecksumAddress() which has browser compatibility issues const checksumAddress = getAddress(address)
311-312: Enhance error message clarityThe error message when endpoints aren't provided could be more specific.
new Error( - 'You have to pass endpoints.grpc within the options for using Turnkey wallet', + 'Turnkey wallet requires endpoints.grpc to be specified in options for transaction broadcasting', ),
303-328: Add transaction validation before broadcastingThe code doesn't validate the transaction object before broadcasting, which could lead to failed transactions and poor user experience.
Consider adding validation before broadcasting:
async sendTransaction( transaction: TxRaw, options: SendTransactionOptions, ): Promise<any> { const { endpoints, txTimeout } = options if (!endpoints) { throw new WalletException( new Error( 'You have to pass endpoints.grpc within the options for using Turnkey wallet', ), ) } + // Validate transaction has required components + if (!transaction.authInfoBytes || !transaction.bodyBytes || !transaction.signatures) { + throw new WalletException( + new Error('Invalid transaction: missing required transaction components'), + { + code: UnspecifiedErrorCode, + contextModule: WalletAction.SendTransaction, + } + ) + } + const txApi = new TxGrpcApi(endpoints.grpc) const response = await txApi.broadcast(transaction, { txTimeout }) if (response.code !== 0) { throw new TransactionException(new Error(response.rawLog), { code: UnspecifiedErrorCode, contextCode: response.code, contextModule: response.codespace, }) } return response }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallets/wallet-base/src/types/strategy.ts
| export function generateGoogleUrl({ | ||
| nonce, | ||
| clientId, | ||
| redirectUri, | ||
| scope = 'openid profile email', | ||
| prompt = 'consent', | ||
| }: { | ||
| nonce: string | ||
| clientId: string | ||
| redirectUri: string | ||
| scope?: string | ||
| prompt?: string | ||
| }) { | ||
| if (!clientId) { | ||
| throw new Error('Google client ID not found') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required parameters
The function validates clientId but not nonce or redirectUri, which are also required parameters.
Update the validation to include all required parameters:
export function generateGoogleUrl({
nonce,
clientId,
redirectUri,
scope = 'openid profile email',
prompt = 'consent',
}: {
nonce: string
clientId: string
redirectUri: string
scope?: string
prompt?: string
}) {
if (!clientId) {
throw new Error('Google client ID not found')
}
+ if (!nonce) {
+ throw new Error('Nonce is required for OAuth security')
+ }
+ if (!redirectUri) {
+ throw new Error('Redirect URI is required')
+ }
const responseType = 'id_token'📝 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 function generateGoogleUrl({ | |
| nonce, | |
| clientId, | |
| redirectUri, | |
| scope = 'openid profile email', | |
| prompt = 'consent', | |
| }: { | |
| nonce: string | |
| clientId: string | |
| redirectUri: string | |
| scope?: string | |
| prompt?: string | |
| }) { | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| export function generateGoogleUrl({ | |
| nonce, | |
| clientId, | |
| redirectUri, | |
| scope = 'openid profile email', | |
| prompt = 'consent', | |
| }: { | |
| nonce: string | |
| clientId: string | |
| redirectUri: string | |
| scope?: string | |
| prompt?: string | |
| }) { | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| if (!nonce) { | |
| throw new Error('Nonce is required for OAuth security') | |
| } | |
| if (!redirectUri) { | |
| throw new Error('Redirect URI is required') | |
| } | |
| const responseType = 'id_token' | |
| // … | |
| } |
| export function generateGoogleUrl({ | ||
| nonce, | ||
| clientId, | ||
| redirectUri, | ||
| scope = 'openid profile email', | ||
| prompt = 'consent', | ||
| }: { | ||
| nonce: string | ||
| clientId: string | ||
| redirectUri: string | ||
| scope?: string | ||
| prompt?: string | ||
| }) { | ||
| if (!clientId) { | ||
| throw new Error('Google client ID not found') | ||
| } | ||
|
|
||
| const responseType = 'id_token' | ||
|
|
||
| return `https://accounts.google.com/o/oauth2/v2/auth?prompt=${prompt}&client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}` | ||
| } |
There was a problem hiding this comment.
Add URL encoding to prevent injection attacks
The current implementation constructs a URL without encoding parameters, which can lead to security vulnerabilities if parameters contain special characters.
Apply this fix:
- return `https://accounts.google.com/o/oauth2/v2/auth?prompt=${prompt}&client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}`
+ return `https://accounts.google.com/o/oauth2/v2/auth?prompt=${encodeURIComponent(prompt)}&client_id=${encodeURIComponent(clientId)}&redirect_uri=${encodeURIComponent(redirectUri)}&response_type=${encodeURIComponent(responseType)}&scope=${encodeURIComponent(scope)}&nonce=${encodeURIComponent(nonce)}`📝 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 function generateGoogleUrl({ | |
| nonce, | |
| clientId, | |
| redirectUri, | |
| scope = 'openid profile email', | |
| prompt = 'consent', | |
| }: { | |
| nonce: string | |
| clientId: string | |
| redirectUri: string | |
| scope?: string | |
| prompt?: string | |
| }) { | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| const responseType = 'id_token' | |
| return `https://accounts.google.com/o/oauth2/v2/auth?prompt=${prompt}&client_id=${clientId}&redirect_uri=${redirectUri}&response_type=${responseType}&scope=${scope}&nonce=${nonce}` | |
| } | |
| export function generateGoogleUrl({ | |
| nonce, | |
| clientId, | |
| redirectUri, | |
| scope = 'openid profile email', | |
| prompt = 'consent', | |
| }: { | |
| nonce: string | |
| clientId: string | |
| redirectUri: string | |
| scope?: string | |
| prompt?: string | |
| }) { | |
| if (!clientId) { | |
| throw new Error('Google client ID not found') | |
| } | |
| const responseType = 'id_token' | |
| return `https://accounts.google.com/o/oauth2/v2/auth?prompt=${encodeURIComponent(prompt)}&client_id=${encodeURIComponent(clientId)}&redirect_uri=${encodeURIComponent(redirectUri)}&response_type=${encodeURIComponent(responseType)}&scope=${encodeURIComponent(scope)}&nonce=${encodeURIComponent(nonce)}` | |
| } |
| } | ||
|
|
||
| return !!(await turnkeyWallet.getIframeClient()) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential infinite redirect issue
The function returns true if there's a session, but falls back to checking for an iframe client, which doesn't guarantee success of the complete flow.
Consider adding more detailed status verification:
return true
}
- return !!(await turnkeyWallet.getIframeClient())
+ const iframeClient = await turnkeyWallet.getIframeClient()
+ return !!iframeClient🌐 Web query:
What are common OAuth session verification patterns to prevent infinite redirects?
💡 Result:
To prevent infinite redirects during OAuth session verification, developers follow certain common patterns and best practices. Here are some of the key approaches:
-
Check for Existing Valid Session before Redirect
Before initiating an OAuth redirect for authentication, always check if the user session or token is still valid. If valid, skip the redirect. -
Use State Parameter
The OAuthstateparameter is used to maintain state between the request and callback. Checking and validating thestatevalue helps prevent replay attacks and can help detect repeated redirects. -
Detect Redirect Loops Using Flags or Cookies
Set a temporary flag (in session storage, cookies, or query parameters) before redirecting. Upon return, check if the flag is present or exceeded a threshold (e.g., redirect count). If too many redirects detected, stop the flow. -
Limit Redirect Count
Maintain a count of how many times a redirect has been performed. If the count exceeds a predefined threshold, abort the process to avoid infinite loops. -
Check Referrer or Origin
Verify if the OAuth callback is coming from the expected identity provider or authorized domain, avoiding retrying redirects on unexpected sources. -
Handle OAuth Errors Gracefully
Detect and handle OAuth error responses (e.g., access denied, invalid_token) in the callback to prevent continuous retries. -
Time-Based Validation
Use timestamps or nonce values to ensure a redirect only occurs once per session or within a short validity window. -
Use Redirect URL Whitelisting
Restrict OAuth return URLs to a predefined whitelist to avoid unexpected redirects that may cause loops.
By combining these patterns, you can effectively prevent infinite redirect loops while maintaining secure OAuth flows. If you'd like, I can provide sample code or configurations illustrating one or more of these patterns.
Strengthen session verification to prevent infinite redirects
The current fallback only checks for a non-null iframe client via !!(await turnkeyWallet.getIframeClient()), which doesn’t guarantee the client is initialized or that we won’t loop endlessly if authentication fails. Please enhance this block as follows:
• File: packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts
Lines: ~97–99
return true
}
- return !!(await turnkeyWallet.getIframeClient())
+ // 1. Retrieve the iframe client and ensure it’s valid
+ const iframeClient = await turnkeyWallet.getIframeClient()
+ if (!iframeClient) {
+ return false
+ }
+ // 2. Optionally verify the client is fully initialized/ready
+ if (!iframeClient.isReady) {
+ console.error('Turnkey iframe client not ready – aborting redirect')
+ return false
+ }
+ // 3. Prevent redirect loops by capping retries
+ const key = 'turnkey_redirect_count'
+ const count = Number(sessionStorage.getItem(key) || 0) + 1
+ if (count > 3) {
+ console.error('Exceeded maximum redirect attempts')
+ return false
+ }
+ sessionStorage.setItem(key, String(count))
+ return true• Consider adopting additional OAuth best practices (from common patterns):
– Validate and store a state parameter to detect replayed or repeated flows.
– Handle and inspect OAuth error callbacks (e.g., invalid_token, access_denied).
– Enforce a time-based nonce or timestamp to ensure each redirect is fresh.
These changes will ensure the iframe client is fully operational before proceeding and guard against infinite redirect loops.
📝 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.
| } | |
| return !!(await turnkeyWallet.getIframeClient()) | |
| return true | |
| } | |
| // 1. Retrieve the iframe client and ensure it’s valid | |
| const iframeClient = await turnkeyWallet.getIframeClient() | |
| if (!iframeClient) { | |
| return false | |
| } | |
| // 2. Optionally verify the client is fully initialized/ready | |
| if (!iframeClient.isReady) { | |
| console.error('Turnkey iframe client not ready – aborting redirect') | |
| return false | |
| } | |
| // 3. Prevent redirect loops by capping retries | |
| const key = 'turnkey_redirect_count' | |
| const count = Number(sessionStorage.getItem(key) || 0) + 1 | |
| if (count > 3) { | |
| console.error('Exceeded maximum redirect attempts') | |
| return false | |
| } | |
| sessionStorage.setItem(key, String(count)) | |
| return true |
ThomasRalee
force-pushed
the
feat/turnkey-wallet
branch
from
90883d6 to
78ee7f3
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (1)
73-97: 🛠️ Refactor suggestionRemove unused loginUser method and its TODO comment
The
loginUsermethod has a TODO comment "should be able to remove this" and appears to be unused based on previous reviews. Remove this method to eliminate dead code.- // TODO: should be able to remove this - static async loginUser(args: { - iframeClient: TurnkeyIframeClient - client: HttpRestClient - oidcToken: string - providerName: 'google' | 'apple' - setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void - }) { - const { client, iframeClient, oidcToken, providerName } = args - const result = await TurnkeyOauthWallet.oauthLogin({ - client, - iframeClient, - oidcToken, - providerName, - }) - - if (result?.credentialBundle) { - args.setMetadata({ - turnkey: { - credentialBundle: result.credentialBundle, - }, - }) - iframeClient.injectCredentialBundle(result.credentialBundle) - } - }packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (1)
99-141: 🛠️ Refactor suggestionRemove unused loginUser method and its TODO comment
The
loginUsermethod has a TODO comment "should be able to remove this" and appears to be unused based on previous reviews. Remove this method to eliminate dead code.- // TODO: should be able to remove this - - static async loginUser(args: { - otpCode: string - emailOTPId: string - client: HttpRestClient - organizationId: string - iframeClient: TurnkeyIframeClient - setMetadata: (metadata: { turnkey: { credentialBundle: string } }) => void - }) { - const { - client, - iframeClient, - organizationId, - otpCode, - emailOTPId, - setMetadata, - } = args - - const result = await TurnkeyOtpWallet.confirmEmailOTP({ - client, - otpCode, - emailOTPId, - iframeClient, - organizationId, - }) - - if (result?.credentialBundle) { - setMetadata({ - turnkey: { - credentialBundle: result.credentialBundle, - }, - }) - - await iframeClient.injectCredentialBundle(result.credentialBundle) - - await iframeClient.refreshSession({ - sessionType: SessionType.READ_WRITE, - targetPublicKey: iframeClient.iframePublicKey, - expirationSeconds: '900', - }) - } - }packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (2)
184-186: 🛠️ Refactor suggestionAddress‐key casing may create duplicate entries in
accountMapYou lookup using both the raw and
toLowerCase()address, but you only store the original casing. If future calls pass an address with a different checksum/case, the cache is missed and a second API call is made.Apply this diff to standardize on lowercase keys:
- if (accountMap[address] || accountMap[address.toLowerCase()]) { - return accountMap[address] || accountMap[address.toLowerCase()] + const key = address.toLowerCase() + if (accountMap[key]) { + return accountMap[key] } # Further down in the method: - this.accountMap[address] = turnkeyAccount + this.accountMap[key] = turnkeyAccount
159-174: 🛠️ Refactor suggestionDefine and map specific Turnkey error codes
The error handling can be improved by defining and using specific error codes for different error scenarios, rather than always using
UnspecifiedErrorCode.Currently, you're only handling the
UserLoggedOutcase specifically. Consider extendingTurnkeyErrorCodeswith additional error codes and use a switch statement to map Turnkey errors to appropriateWalletExceptioncodes.
🧹 Nitpick comments (17)
packages/wallets/wallet-turnkey/README.md (6)
24-26: Add missing comma in this sentenceThere should be a comma after "you choose to use" for better readability.
-Regardless of which wallet package(s) you choose to use you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base` +Regardless of which wallet package(s) you choose to use, you must also have `@injectivelabs/wallet-core` and `@injectivelabs/wallet-base`🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...(ALL_OF_THE)
28-28: Remove colon after sentence ending with periodThe colon after "INJ." is redundant since the sentence already ends with a period.
-Here's a brief example of how to use this package to send 1 INJ.: +Here's a brief example of how to use this package to send 1 INJ:🧰 Tools
🪛 LanguageTool
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...(DOUBLE_PUNCTUATION_PREMIUM)
40-54: Improve code formatting for readabilityThe indentation in this section of the example is inconsistent, making it harder to read.
- [Wallet.Turnkey]: new TurnkeyWallet({ - onStatusChange(status) { - turnkeyStatus.value = status - }, - chainId: injectiveClients.chainId, - ethereumOptions: { - ethereumChainId: injectiveClients.ethereumChainId!, - }, - metadata: { - turnkeyAuthIframeContainerId, - defaultOrganizationId: import.meta.env - .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, - apiBaseUrl: 'https://api.turnkey.com', - }, - }) + [Wallet.Turnkey]: new TurnkeyWallet({ + onStatusChange(status) { + turnkeyStatus.value = status + }, + chainId: injectiveClients.chainId, + ethereumOptions: { + ethereumChainId: injectiveClients.ethereumChainId!, + }, + metadata: { + turnkeyAuthIframeContainerId, + defaultOrganizationId: import.meta.env + .VITE_TURNKEY_DEFAULT_ORGANIZATION_ID, + apiBaseUrl: 'https://api.turnkey.com', + }, + })
108-108: Format date range with en dashUse an en dash instead of a hyphen for date ranges.
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021–2022 Injective Labs Inc. (https://injectivelabs.org/)🧰 Tools
🪛 LanguageTool
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
108-108: Bare URL used
null(MD034, no-bare-urls)
110-110: Add alt text to imageThe image is missing alternative text for accessibility.
-<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Logo" style="width: 300px; max-width: 100%; height: auto" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Images should have alternate text (alt text)
null(MD045, no-alt-text)
112-115: Format URLs properly in MarkdownReplace bare URLs with proper Markdown links for better accessibility and appearance.
-Apache License <br /> -Version 2.0, January 2004 <br /> -http://www.apache.org/licenses/ +Apache License <br /> +Version 2.0, January 2004 <br /> +[http://www.apache.org/licenses/](http://www.apache.org/licenses/)🧰 Tools
🪛 LanguageTool
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
115-115: Bare URL used
null(MD034, no-bare-urls)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (2)
12-31: Refactor static-only class to module with named exportsThis class only contains static methods, which is an anti-pattern in modern TypeScript. Consider refactoring to a module with named exports for better maintainability.
-export class TurnkeyOauthWallet { - static async generateOAuthNonce(iframeClient: TurnkeyIframeClient) { +export async function generateOAuthNonce(iframeClient: TurnkeyIframeClient) { try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } return Array.from(sha256(new TextEncoder().encode(targetPublicKey))) .map((b) => b.toString(16).padStart(2, '0')) .join('') } catch (e: any) { throw new WalletException(new Error(e.message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-generate-oauth-nonce', }) } }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
54-54: Remove or update comment about client.$postThe comment "client.$post is undefined, resorting to this for now" suggests a temporary solution. Either remove the comment if this is now the permanent approach or update the code to use the intended approach.
- // client.$post is undefined, resorting to this for now + // Use standard post method to send the requestpackages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (2)
14-50: Refactor static-only class to module with named exportsThis class only contains static methods, which is an anti-pattern in modern TypeScript. Consider refactoring to a module with named exports for better maintainability.
-export class TurnkeyOtpWallet { - static async initEmailOTP(args: { +export async function initEmailOTP(args: { email: string subOrgId?: string otpInitPath?: string client: HttpRestClient iframeClient: TurnkeyIframeClient invalidateExistingSessions?: boolean }) { const { client, iframeClient } = args try { const targetPublicKey = iframeClient.iframePublicKey if (!targetPublicKey) { throw new WalletException(new Error('Target public key not found')) } // client.$post is undefined, resorting to this for now const response = await client.post<{ data?: TurnkeyOTPCredentialsResponse }>(args.otpInitPath || TURNKEY_OTP_INIT_PATH, { targetPublicKey, email: args.email, suborgId: args.subOrgId, invalidateExistingSessions: args.invalidateExistingSessions, }) return response?.data } catch (e: any) { throw new WalletException(new Error(e.message), { code: UnspecifiedErrorCode, type: ErrorType.WalletError, contextModule: 'turnkey-init-email-otp', }) } }🧰 Tools
🪛 Biome (1.9.4)
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
32-32: Remove or update comment about client.$postThe comment "client.$post is undefined, resorting to this for now" suggests a temporary solution. Either remove the comment if this is now the permanent approach or update the code to use the intended approach.
- // client.$post is undefined, resorting to this for now + // Use standard post method to send the requestpackages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (7)
91-95: Extract session expiration time to a constantThe expiration seconds is hardcoded as '900' (15 minutes) both here and in the
injectAndRefreshmethod. Consider extracting this to a named constant for better maintainability.+ // Add at the top of the file or in consts.ts + const DEFAULT_SESSION_EXPIRATION_SECONDS = '900' // 15 minutes // Then use it in both places - expirationSeconds: '900', + expirationSeconds: DEFAULT_SESSION_EXPIRATION_SECONDS,
44-46: Missing turnkey check in initialization flowYou initialize the iframe client but don't check if
this.turnkeyis undefined in this initialization path. This could lead to inconsistent state if the iframe client is initialized but Turnkey isn't.Consider modifying the initialization logic to ensure both components are initialized together:
public async getTurnkey(): Promise<Turnkey> { - if (!this.iframeClient) { + if (!this.iframeClient || !this.turnkey) { await this.initFrame() }
264-264: Consider making default iframe URL configurableThe default iframe URL is hardcoded. Consider moving this to a constant defined in
consts.tsto improve maintainability and configurability.+// In consts.ts +export const DEFAULT_TURNKEY_IFRAME_URL = 'https://auth.turnkey.com' // Then in this file - iframeUrl: metadata?.iframeUrl || 'https://auth.turnkey.com', + iframeUrl: metadata?.iframeUrl || DEFAULT_TURNKEY_IFRAME_URL,
257-259: Consider cleaning up event listeners when removing iframeWhen removing an existing iframe client, event listeners and other resources might not be automatically cleaned up, potentially leading to memory leaks.
Consider properly disposing of the iframe client before removing it:
if (existingIframeClient) { + // Clean up any event listeners if needed + // For example: existingIframeClient.removeEventListener(...) existingIframeClient.remove() }
52-52: Remove non-null assertion operatorNon-null assertion operators (
as Turnkey) should generally be avoided as they bypass TypeScript's type checking. Since you have a check on line 48, you can simply returnthis.turnkey.- return this.turnkey as Turnkey + return this.turnkey
137-154: Consider error handling for Promise.allSettledThe current implementation filters out rejected promises but doesn't log or handle the errors. This could make debugging issues difficult.
Consider adding logging for rejected promises:
const accounts = await Promise.allSettled( response.wallets.map((wallet) => iframeClient.getWalletAccounts({ walletId: wallet.walletId, organizationId: this.metadata.organizationId, }), ), ) + // Log rejected promises for debugging + accounts + .filter((account) => account.status === 'rejected') + .forEach((result) => { + console.warn('Failed to fetch wallet account:', (result as PromiseRejectedResult).reason) + }) const filteredAccounts = accounts .filter((account) => account.status === 'fulfilled') .flatMap((result) => result.value?.accounts) .filter( (wa): wa is NonNullable<typeof wa> => !!wa && wa.addressFormat === 'ADDRESS_FORMAT_ETHEREUM' && !!wa.address, )
60-64: Consistent error handling approach neededIn this method, you throw a direct
WalletExceptionwith a custom message, while in other methods you wrap errors inWalletException. Consider maintaining a consistent approach throughout the class.Also, consider using a specific error code rather than
UnspecifiedErrorCode:if (!this.iframeClient) { - throw new WalletException(new Error('Iframe client not initialized')) + throw new WalletException(new Error('Iframe client not initialized'), { + code: UnspecifiedErrorCode, + type: ErrorType.WalletError, + contextModule: 'turnkey-wallet-get-iframe-client', + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
package.json(1 hunks)packages/exceptions/src/exceptions/types/codes.ts(1 hunks)packages/sdk-ts/src/utils/address.spec.ts(2 hunks)packages/utils/src/classes/HttpRestClient.ts(1 hunks)packages/wallets/wallet-base/src/base.ts(2 hunks)packages/wallets/wallet-base/src/types/enums.ts(1 hunks)packages/wallets/wallet-base/src/types/strategy.ts(7 hunks)packages/wallets/wallet-base/src/utils/wallet.ts(2 hunks)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts(3 hunks)packages/wallets/wallet-cosmos/src/strategy/strategy.ts(3 hunks)packages/wallets/wallet-cosmostation/src/strategy/strategy.ts(6 hunks)packages/wallets/wallet-magic/src/strategy/strategy.ts(5 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)packages/wallets/wallet-strategy/src/strategy/index.ts(7 hunks)packages/wallets/wallet-turnkey/README.md(1 hunks)packages/wallets/wallet-turnkey/package.json(1 hunks)packages/wallets/wallet-turnkey/src/index.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/consts.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts(1 hunks)packages/wallets/wallet-turnkey/src/strategy/types.ts(1 hunks)packages/wallets/wallet-turnkey/src/utils.ts(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.esm.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.build.json(1 hunks)packages/wallets/wallet-turnkey/tsconfig.json(1 hunks)packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/sdk-ts/src/utils/address.spec.ts
- package.json
- packages/wallets/wallet-turnkey/src/index.ts
- packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/exceptions/src/exceptions/types/codes.ts
- packages/wallets/wallet-turnkey/tsconfig.json
- packages/wallets/wallet-turnkey/tsconfig.build.json
- packages/wallets/wallet-turnkey/src/strategy/strategy/oauth.ts
- packages/wallets/wallet-magic/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/src/strategy/strategy/otp.ts
- packages/wallets/wallet-cosmostation/src/strategy/strategy.ts
- packages/wallets/wallet-base/src/types/enums.ts
- packages/wallets/wallet-turnkey/tsconfig.build.esm.json
- packages/wallets/wallet-base/src/utils/wallet.ts
- packages/wallets/wallet-cosmos/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/src/strategy/consts.ts
- packages/wallets/wallet-wallet-connect/src/strategy/strategy.ts
- packages/wallets/wallet-turnkey/src/utils.ts
- packages/wallets/wallet-turnkey/src/strategy/types.ts
- packages/wallets/wallet-private-key/src/strategy/strategy.ts
- packages/wallets/wallet-strategy/src/strategy/index.ts
- packages/wallets/wallet-turnkey/package.json
- packages/wallets/wallet-base/src/base.ts
- packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts
- packages/wallets/wallet-base/src/types/strategy.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (5)
packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(25-141)packages/wallets/wallet-base/src/types/strategy.ts (1)
TurnkeyMetadata(70-90)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/wallets/wallet-base/src/types/enums.ts (1)
WalletAction(49-49)packages/sdk-ts/src/core/accounts/Address.ts (1)
address(24-26)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts (5)
packages/sdk-ts/src/utils/crypto.ts (1)
sha256(18-23)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(25-141)packages/wallets/wallet-turnkey/src/strategy/consts.ts (1)
TURNKEY_OAUTH_PATH(1-1)packages/wallets/wallet-turnkey/src/strategy/types.ts (1)
TurnkeyOauthLoginResponse(28-32)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts (6)
packages/utils/src/classes/HttpRestClient.ts (1)
HttpRestClient(25-141)packages/wallets/wallet-turnkey/src/strategy/types.ts (2)
TurnkeyOTPCredentialsResponse(19-22)TurnkeyConfirmEmailOTPResponse(24-26)packages/wallets/wallet-turnkey/src/strategy/consts.ts (2)
TURNKEY_OTP_INIT_PATH(3-3)TURNKEY_OTP_VERIFY_PATH(4-4)packages/exceptions/src/exceptions/types/codes.ts (1)
UnspecifiedErrorCode(794-794)packages/wallets/wallet-turnkey/src/strategy/strategy/base.ts (1)
setMetadata(72-82)packages/wallets/wallet-core/src/strategy/BaseWalletStrategy.ts (1)
setMetadata(84-87)
🪛 LanguageTool
packages/wallets/wallet-turnkey/README.md
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...f which wallet package(s) you choose to use you must also have `@injectivelabs/wall...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s/wallet-base` installed. These contain all of the types and core wallet functionality, wi...
(ALL_OF_THE)
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...of how to use this package to send 1 INJ.: ```typescript import { Wallet } from '...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~108-~108: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...
(DASH_RULE)
[typographical] ~112-~112: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
packages/wallets/wallet-turnkey/README.md
108-108: Bare URL used
null
(MD034, no-bare-urls)
110-110: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
115-115: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
packages/wallets/wallet-turnkey/src/strategy/turnkey/oauth.ts
[error] 12-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/wallets/wallet-turnkey/src/strategy/turnkey/otp.ts
[error] 14-142: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
packages/utils/src/classes/HttpRestClient.ts (1)
15-18: Improved error message extractionThe enhanced error message extraction logic now includes
statusMessageas a fallback before returning the entire response data. This provides more specific error messages when available, improving debugging and error reporting.packages/wallets/wallet-turnkey/README.md (1)
1-6: Good package header with proper badgesThe header section clearly identifies the package with appropriate NPM badges for downloads, version, and license.
packages/wallets/wallet-turnkey/src/strategy/turnkey/turnkey.ts (1)
43-53: Improved implementation of thegetTurnkeymethod.This method properly initializes
this.turnkeyif it isn't already set, addressing a critical issue from a previous review wherethis.turnkeywas never initialized. This ensures any code path callinggetTurnkey()will have a valid instance.
First cut at adding Turnkey support to our SDK. One big gotcha to know here is that you need to provide endpoints for purpose built API to communicate with the Turnkey API server side. I'll include what this looks like and a minimal example before merging.
There's a lot going on here and could certainly use some cleaning up, but I've successfully tested and used this in our
MsgBroadcasterto broadcast an transaction.I'll post some more concrete general docs for how to properly use this in the near future.
Summary by CodeRabbit
New Features
Improvements
Documentation
Bug Fixes
Tests