feat: add browser extension wallet strategy#44
feat: add browser extension wallet strategy#44frankomosh wants to merge 2 commits intokeep-starknet-strange:mainfrom
Conversation
|
Hello, thanks for the PR but for now we want to put more focus on the react native front for mobile apps. I'll come back on this in a few weeks |
Ok. thanks for the context |
|
@frankomosh would you be interested in contributing with a similar module but extending it with mobile managing installed Ready/Braavos wallets? That means we would have a feature parity in terms of wallet management between browser/mobile. |
Yes, interested. Which mobile wallet SDKs are you targeting, Argent Mobile, Braavos Mobile, or both? |
should the mobile strategy work alongside the existing Cartridge and Privy strategies on React Native, or would it replace them for mobile flows? |
Indeed, both Ready wallet, and Braavos Mobile.
Yes, alongside, that is, onboarding new users with Cartridge, Privy or Signer (current); onboarding existing users via Ready Wallet/Braavos Mobile. |
Ok. Working on it |
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Dependency Management package.json |
Adds optional peer dependency @starknet-io/get-starknet@^4.0.8 for browser wallet integration. |
Public API Surface src/index.ts, src/types/onboard.ts |
Exports BrowserWallet and BrowserWalletOptions; introduces OnboardStrategy.Browser enum value and OnboardBrowserOptions type with walletProvider, rpcUrl, explorer, and osVersion fields. |
SDK Integration src/sdk.ts |
Adds "browser" onboarding strategy branch that dynamically imports BrowserWallet, instantiates it with provided configuration, and conditionally ensures wallet readiness. |
Core Implementation src/wallet/browser.ts |
Implements BrowserWallet class extending BaseWallet with static factory method, deployment state caching with TTL, transaction execution, message signing, and explicit blocking of programmatic deployment for browser wallets. |
Test Suite tests/browser-wallet.test.ts |
Comprehensive Vitest suite validating BrowserWallet creation, deployment checks, transaction execution, error handling for unsupported operations, and wallet disconnection behavior. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant SDK as StarkSDK
participant BrowserWallet as BrowserWallet<br/>(Dynamic Import)
participant WalletExt as Browser Wallet<br/>Extension
participant Provider as RpcProvider
User->>SDK: onboard({ strategy: "browser", walletProvider, ... })
SDK->>BrowserWallet: import BrowserWallet
SDK->>BrowserWallet: BrowserWallet.create(walletProvider, options)
BrowserWallet->>WalletExt: walletProvider.request (get account)
WalletExt-->>BrowserWallet: connected account address
BrowserWallet->>Provider: instantiate RpcProvider(rpcUrl)
BrowserWallet->>Provider: getClassHashAt(address)
Provider-->>BrowserWallet: classHash (or undefined)
BrowserWallet-->>SDK: return BrowserWallet instance
SDK->>BrowserWallet: ensureReady(deploy, feeMode, onProgress)
alt Account not deployed
BrowserWallet->>BrowserWallet: deployment check via isDeployed()
BrowserWallet->>Provider: check contract at address
end
BrowserWallet-->>SDK: ready
SDK-->>User: { wallet, deployed, strategy: "browser" }
Estimated Code Review Effort
🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
🐰 A browser wallet hops into the fold,
With ArgentX and Braavos, so bold!
No deploy button here—we connect what's there,
Sign messages swift through extension's care.
StarkSDK's garden grows rich and wide! 🌱✨
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The pull request title 'feat: add browser extension wallet strategy' accurately describes the main change: introducing a new 'browser' strategy for onboarding existing browser extension wallets (ArgentX, Braavos, Keplr) via the onboard() function. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
Following up on your suggestion @0xsisyfos, I've extended the browser strategy to also cover installed mobile wallets. The approach have taken is to remove the |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk.ts (1)
250-253:⚠️ Potential issue | 🔴 CriticalFix the deploy flow for browser wallets in
ensureReady().The concern is valid. When
sdk.onboard({ strategy: "browser" })is called with an undeployed wallet, the defaultdeploy: "if_needed"will causeensureWalletReady()to callwallet.deploy()at line 88 ofsrc/wallet/utils.ts. However,BrowserWallet.deploy()explicitly throws because browser extension wallets deploy automatically on the user's first transaction, not programmatically.This makes the default onboarding flow impossible for undeployed browser wallets. Either:
- Skip the deploy call for BrowserWallet in
ensureWalletReady()(preferred), or- Default to
deploy: "never"for the browser strategy to prevent the failed deploy attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.ts` around lines 250 - 253, The onboarding flow calls ensureWalletReady() which unconditionally triggers wallet.deploy() when deploy is "if_needed", causing BrowserWallet.deploy() to throw; update the logic to skip programmatic deploys for browser extensions: in ensureReady()/ensureWalletReady() detect BrowserWallet (e.g., by checking instance of BrowserWallet or wallet.strategy === "browser") and prevent calling wallet.deploy() (leave deploy behavior for other wallet types), ensuring sdk.onboard({ strategy: "browser" }) with default options no longer attempts a programmatic deploy; alternatively, if you prefer configuration-side fix, set options.deploy = "never" when sdk.onboard is called with strategy "browser" so shouldEnsureReady becomes false.
🧹 Nitpick comments (1)
src/sdk.ts (1)
349-349: Use the@/alias for the new internal import.Keeps the new branch aligned with the repo's TS import rule.
Small cleanup
- const { BrowserWallet } = await import("./wallet/browser"); + const { BrowserWallet } = await import("@/wallet/browser");As per coding guidelines, "
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.ts` at line 349, The import in sdk.ts should use the project TypeScript path alias instead of a relative path; update the dynamic import that currently loads "./wallet/browser" to use "@/wallet/browser" so the BrowserWallet symbol is resolved via the repo alias rule, e.g. change the import specifier for BrowserWallet to the '@/wallet/browser' module and keep the rest of the dynamic import logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 28-38: The package exposes types that import
`@starknet-io/get-starknet-core` (see src/wallet/browser.ts and
src/types/onboard.ts) but package.json only lists `@starknet-io/get-starknet` as a
peerDependency; add "@starknet-io/get-starknet-core" to peerDependencies using
the same version constraint as "@starknet-io/get-starknet" and mark it optional
in peerDependenciesMeta if needed so consumers and strict package managers
(pnpm/Yarn PnP) resolve the public type dependency correctly.
In `@src/sdk.ts`:
- Around line 347-359: The browser strategy currently allows callers to override
rpcUrl but always passes this.config.chainId into BrowserWallet.create, which
can mismatch network info; update the options handling in the if
(options.strategy === "browser") branch so that before calling
BrowserWallet.create you either (A) derive the effective chainId from the
resolved RPC URL (resolve chain via a quick RPC call or util that maps
rpcUrl→chainId) and pass that derived chainId into BrowserWallet.create instead
of this.config.chainId, or (B) explicitly reject per-call rpcUrl overrides for
the browser strategy by throwing an error when options.rpcUrl is set; implement
one approach and adjust the call site around BrowserWallet.create (refer to
options.rpcUrl, options.strategy, BrowserWallet.create, and this.config.chainId)
accordingly.
- Around line 352-356: Summary: Browser onboarding currently forwards only
options.explorer to Tx, losing SDK-level explorer links; fix by merging SDK
config into the forwarded explorer. Update the code that builds the RPC/options
object (the block assembling rpcUrl/chainId/... including ...(options.explorer
&& { explorer: options.explorer })) to use a merged value like options.explorer
?? this.config.explorer (or merge fields from this.config.explorer into
options.explorer) so BrowserWallet.execute() and onboard() will bake the
SDK-level explorerConfig into each Tx; ensure references to
BrowserWallet.execute, onboard, Tx, options.explorer and this.config.explorer
are updated accordingly.
In `@src/wallet/browser.ts`:
- Around line 176-200: The execute() method incorrectly rejects undeployed
extension accounts, preventing the extension from performing the automatic
first-transaction deployment; update execute() (in BrowserWallet) to stop
throwing when isDeployed() returns false for user-pays flows—allow the call to
proceed so the extension can deploy on the first transaction (keep the existing
sponsored-mode rejection). Remove or relax the error that references
ensureReady({ deploy: "if_needed" }) and only require a hard error when feeMode
=== "sponsored" (or other non-user-pays modes) so that deploy() remains
non-programmatic while execute() lets the extension trigger deployment on first
user-paid tx.
In `@tests/browser-wallet.test.ts`:
- Around line 99-116: The test currently only mocks WalletAccount.connect but
still has MockRpcProvider.getClassHashAt resolving, so the undeployed-account
branch is never hit; update the test to make the provider's getClassHashAt
reject (or throw) when called so BrowserWallet.create triggers the
undeployed-account path, then assert wallet.getClassHash() === "0x0" instead of
toBeDefined(); specifically modify the mock for MockRpcProvider.getClassHashAt
to return a rejected promise (or throw) and keep references to
WalletAccount.connect, BrowserWallet.create and wallet.getClassHash() to locate
the code to change.
---
Outside diff comments:
In `@src/sdk.ts`:
- Around line 250-253: The onboarding flow calls ensureWalletReady() which
unconditionally triggers wallet.deploy() when deploy is "if_needed", causing
BrowserWallet.deploy() to throw; update the logic to skip programmatic deploys
for browser extensions: in ensureReady()/ensureWalletReady() detect
BrowserWallet (e.g., by checking instance of BrowserWallet or wallet.strategy
=== "browser") and prevent calling wallet.deploy() (leave deploy behavior for
other wallet types), ensuring sdk.onboard({ strategy: "browser" }) with default
options no longer attempts a programmatic deploy; alternatively, if you prefer
configuration-side fix, set options.deploy = "never" when sdk.onboard is called
with strategy "browser" so shouldEnsureReady becomes false.
---
Nitpick comments:
In `@src/sdk.ts`:
- Line 349: The import in sdk.ts should use the project TypeScript path alias
instead of a relative path; update the dynamic import that currently loads
"./wallet/browser" to use "@/wallet/browser" so the BrowserWallet symbol is
resolved via the repo alias rule, e.g. change the import specifier for
BrowserWallet to the '@/wallet/browser' module and keep the rest of the dynamic
import logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 521c6e15-7a72-4d82-bc6e-1545dd0cbe56
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/index.tssrc/sdk.tssrc/types/onboard.tssrc/wallet/browser.tstests/browser-wallet.test.ts
| "peerDependencies": { | ||
| "@cartridge/controller": "^0.13.9" | ||
| "@cartridge/controller": "^0.13.9", | ||
| "@starknet-io/get-starknet": "^4.0.8" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@cartridge/controller": { | ||
| "optional": true | ||
| }, | ||
| "@starknet-io/get-starknet": { | ||
| "optional": true | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Direct imports:"
rg -n '"@starknet-io/get-starknet-core"' src
echo
echo "Declared packages:"
jq '{dependencies, peerDependencies, devDependencies}' package.jsonRepository: keep-starknet-strange/starkzap
Length of output: 1074
Declare @starknet-io/get-starknet-core directly in peerDependencies.
src/wallet/browser.ts (line 10) and src/types/onboard.ts (line 2) import @starknet-io/get-starknet-core, but only @starknet-io/get-starknet is declared as a peer dependency. This makes the public type surface depend on an undeclared transitive dependency, which breaks in strict package managers (pnpm, Yarn PnP).
Add @starknet-io/get-starknet-core to peerDependencies with the same version constraint as @starknet-io/get-starknet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 28 - 38, The package exposes types that import
`@starknet-io/get-starknet-core` (see src/wallet/browser.ts and
src/types/onboard.ts) but package.json only lists `@starknet-io/get-starknet` as a
peerDependency; add "@starknet-io/get-starknet-core" to peerDependencies using
the same version constraint as "@starknet-io/get-starknet" and mark it optional
in peerDependenciesMeta if needed so consumers and strict package managers
(pnpm/Yarn PnP) resolve the public type dependency correctly.
| if (options.strategy === "browser") { | ||
|
|
||
| const { BrowserWallet } = await import("./wallet/browser"); | ||
| const wallet = await BrowserWallet.create( | ||
| options.walletProvider, | ||
| { | ||
| rpcUrl: options.rpcUrl ?? this.config.rpcUrl, | ||
| chainId: this.config.chainId, | ||
| ...(options.explorer && { explorer: options.explorer }), | ||
| ...(feeMode && { feeMode }), | ||
| }, | ||
| this.config.staking | ||
| ); |
There was a problem hiding this comment.
Don't pair an overridden rpcUrl with the SDK's fixed chainId.
This branch lets callers override rpcUrl, but it always passes chainId: this.config.chainId. A browser wallet created this way can query one network and stamp returned Tx objects with another. Either resolve the chain from the effective RPC here or reject per-call RPC overrides for the browser strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sdk.ts` around lines 347 - 359, The browser strategy currently allows
callers to override rpcUrl but always passes this.config.chainId into
BrowserWallet.create, which can mismatch network info; update the options
handling in the if (options.strategy === "browser") branch so that before
calling BrowserWallet.create you either (A) derive the effective chainId from
the resolved RPC URL (resolve chain via a quick RPC call or util that maps
rpcUrl→chainId) and pass that derived chainId into BrowserWallet.create instead
of this.config.chainId, or (B) explicitly reject per-call rpcUrl overrides for
the browser strategy by throwing an error when options.rpcUrl is set; implement
one approach and adjust the call site around BrowserWallet.create (refer to
options.rpcUrl, options.strategy, BrowserWallet.create, and this.config.chainId)
accordingly.
| { | ||
| rpcUrl: options.rpcUrl ?? this.config.rpcUrl, | ||
| chainId: this.config.chainId, | ||
| ...(options.explorer && { explorer: options.explorer }), | ||
| ...(feeMode && { feeMode }), |
There was a problem hiding this comment.
Carry the SDK explorer config into browser onboarding.
BrowserWallet.execute() bakes its explorerConfig into each Tx, but this path only forwards options.explorer. Browser flows therefore lose SDK-level explorer links unless callers repeat the same config on every onboard() call.
Possible fix
+ const explorer = options.explorer ?? this.config.explorer;
const wallet = await BrowserWallet.create(
options.walletProvider,
{
rpcUrl: options.rpcUrl ?? this.config.rpcUrl,
chainId: this.config.chainId,
- ...(options.explorer && { explorer: options.explorer }),
+ ...(explorer && { explorer }),
...(feeMode && { feeMode }),
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sdk.ts` around lines 352 - 356, Summary: Browser onboarding currently
forwards only options.explorer to Tx, losing SDK-level explorer links; fix by
merging SDK config into the forwarded explorer. Update the code that builds the
RPC/options object (the block assembling rpcUrl/chainId/... including
...(options.explorer && { explorer: options.explorer })) to use a merged value
like options.explorer ?? this.config.explorer (or merge fields from
this.config.explorer into options.explorer) so BrowserWallet.execute() and
onboard() will bake the SDK-level explorerConfig into each Tx; ensure references
to BrowserWallet.execute, onboard, Tx, options.explorer and this.config.explorer
are updated accordingly.
| async deploy(_options: DeployOptions = {}): Promise<Tx> { | ||
| // Browser extension wallets deploy the account on the user's first | ||
| // transaction. We cannot trigger it programmatically from outside. | ||
| throw new Error( | ||
| "BrowserWallet does not support programmatic deployment. " + | ||
| "The extension wallet deploys the account automatically on first transaction." | ||
| ); | ||
| } | ||
|
|
||
| async execute(calls: Call[], options: ExecuteOptions = {}): Promise<Tx> { | ||
| const feeMode = options.feeMode ?? this.defaultFeeMode; | ||
|
|
||
| if (feeMode === "sponsored") { | ||
| throw new Error( | ||
| "BrowserWallet does not support sponsored transactions. " + | ||
| "Use the cartridge strategy for gasless flows." | ||
| ); | ||
| } | ||
|
|
||
| const deployed = await this.isDeployed(); | ||
| if (!deployed) { | ||
| throw new Error( | ||
| 'Account is not deployed. Call wallet.ensureReady({ deploy: "if_needed" }) before execute() in user_pays mode.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don't block the first user-paid transaction.
Lines 177-181 say browser wallets deploy on the user's first transaction, but Lines 195-200 reject execute() whenever the account is not already deployed. As written, undeployed extension accounts can neither call deploy() nor send the transaction that would deploy them.
Possible fix
async execute(calls: Call[], options: ExecuteOptions = {}): Promise<Tx> {
const feeMode = options.feeMode ?? this.defaultFeeMode;
if (feeMode === "sponsored") {
throw new Error(
"BrowserWallet does not support sponsored transactions. " +
"Use the cartridge strategy for gasless flows."
);
}
-
- const deployed = await this.isDeployed();
- if (!deployed) {
- throw new Error(
- 'Account is not deployed. Call wallet.ensureReady({ deploy: "if_needed" }) before execute() in user_pays mode.'
- );
- }
const { transaction_hash } = await this.walletAccount.execute(calls);
this.clearDeploymentCache();📝 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 deploy(_options: DeployOptions = {}): Promise<Tx> { | |
| // Browser extension wallets deploy the account on the user's first | |
| // transaction. We cannot trigger it programmatically from outside. | |
| throw new Error( | |
| "BrowserWallet does not support programmatic deployment. " + | |
| "The extension wallet deploys the account automatically on first transaction." | |
| ); | |
| } | |
| async execute(calls: Call[], options: ExecuteOptions = {}): Promise<Tx> { | |
| const feeMode = options.feeMode ?? this.defaultFeeMode; | |
| if (feeMode === "sponsored") { | |
| throw new Error( | |
| "BrowserWallet does not support sponsored transactions. " + | |
| "Use the cartridge strategy for gasless flows." | |
| ); | |
| } | |
| const deployed = await this.isDeployed(); | |
| if (!deployed) { | |
| throw new Error( | |
| 'Account is not deployed. Call wallet.ensureReady({ deploy: "if_needed" }) before execute() in user_pays mode.' | |
| ); | |
| } | |
| async deploy(_options: DeployOptions = {}): Promise<Tx> { | |
| // Browser extension wallets deploy the account on the user's first | |
| // transaction. We cannot trigger it programmatically from outside. | |
| throw new Error( | |
| "BrowserWallet does not support programmatic deployment. " + | |
| "The extension wallet deploys the account automatically on first transaction." | |
| ); | |
| } | |
| async execute(calls: Call[], options: ExecuteOptions = {}): Promise<Tx> { | |
| const feeMode = options.feeMode ?? this.defaultFeeMode; | |
| if (feeMode === "sponsored") { | |
| throw new Error( | |
| "BrowserWallet does not support sponsored transactions. " + | |
| "Use the cartridge strategy for gasless flows." | |
| ); | |
| } | |
| const { transaction_hash } = await this.walletAccount.execute(calls); | |
| this.clearDeploymentCache(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/browser.ts` around lines 176 - 200, The execute() method
incorrectly rejects undeployed extension accounts, preventing the extension from
performing the automatic first-transaction deployment; update execute() (in
BrowserWallet) to stop throwing when isDeployed() returns false for user-pays
flows—allow the call to proceed so the extension can deploy on the first
transaction (keep the existing sponsored-mode rejection). Remove or relax the
error that references ensureReady({ deploy: "if_needed" }) and only require a
hard error when feeMode === "sponsored" (or other non-user-pays modes) so that
deploy() remains non-programmatic while execute() lets the extension trigger
deployment on first user-paid tx.
| it("should handle undeployed accounts — classHash stays 0x0", async () => { | ||
| const { WalletAccount } = await import("starknet"); | ||
| ( | ||
| WalletAccount as unknown as { connect: ReturnType<typeof vi.fn> } | ||
| ).connect.mockResolvedValueOnce({ | ||
| address: "0x1234567890abcdef", | ||
| execute: vi.fn(), | ||
| signMessage: vi.fn(), | ||
| simulateTransaction: vi.fn(), | ||
| estimateInvokeFee: vi.fn(), | ||
| }); | ||
|
|
||
| const wallet = await BrowserWallet.create(mockWalletProvider as never, { | ||
| rpcUrl: "https://rpc.starknet.io", | ||
| }); | ||
|
|
||
| expect(wallet.getClassHash()).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
This test never reaches the undeployed-account branch.
The block title says getClassHashAt() should fail and leave the class hash at 0x0, but the only mock changed here is WalletAccount.connect(). MockRpcProvider.getClassHashAt() still resolves "0xclasshash", and toBeDefined() would pass either way. Please drive the provider mock down the rejection path and assert wallet.getClassHash() === "0x0" here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/browser-wallet.test.ts` around lines 99 - 116, The test currently only
mocks WalletAccount.connect but still has MockRpcProvider.getClassHashAt
resolving, so the undeployed-account branch is never hit; update the test to
make the provider's getClassHashAt reject (or throw) when called so
BrowserWallet.create triggers the undeployed-account path, then assert
wallet.getClassHash() === "0x0" instead of toBeDefined(); specifically modify
the mock for MockRpcProvider.getClassHashAt to return a rejected promise (or
throw) and keep references to WalletAccount.connect, BrowserWallet.create and
wallet.getClassHash() to locate the code to change.
|
Sorry for the delay, was focused on getting v2 out. Are you still interested in the PR ? |
Have not had enough time to continue working on it, unfortunately |
|
Should we close the PR and if you want to continue the work later re open it ? |
Yes please. Let me close for now |
Some people may want to use this to build apps that would allow connecting their existing browser extension wallets (ArgentX, Braavos, Keplr….) rather than onboarding through Privy or Cartridge. Currently I don’t see any way of doing it.
Have added a "browser" strategy to
onboard()using@starknet-io/get-starknetfor wallet discovery andstarknet.jsWalletAccountas the signing bridge. The private key never leaves the extension.Summary by CodeRabbit
Release Notes
New Features
Tests