Repository navigation
feat: add remote wallet package runtime - #1
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 22797d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (59)
📝 WalkthroughWalkthroughImplements a full remote-wallet: protocol primitives (crypto, encoding, Nostr), Node signer and relay connector, browser pairing/session provider and standard-wallet, CLI agent, React pairing UI, Playwright e2e fixtures/tests, and packaging/CI updates. ChangesRemote Wallet
Sequence Diagram(s)sequenceDiagram
participant DApp as Browser DApp
participant Relay as Nostr Relay
participant CLI as Wallet Agent (CLI)
participant Node as Node Signer
DApp->>Relay: REQ subscription + hello (ECDH pubkey + ECDSA sig)
CLI->>Relay: receives hello via relay
CLI->>CLI: verifyAssociationSignature
CLI->>CLI: deriveSharedSecret (ECDH+HKDF)
CLI->>Relay: encrypted session response
DApp->>DApp: parseHelloResponse -> derive shared secret
DApp->>Relay: encrypted authorize request (JSON-RPC)
CLI->>Node: dispatch authorize -> set auth/token
DApp->>Relay: encrypted signMessages request
CLI->>Node: signMessages (Ed25519)
CLI->>Relay: encrypted JSON-RPC result
DApp->>DApp: receive signature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
commit: |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/browser/create-remote-wallet-session.ts (1)
233-246: ⚡ Quick winNo timeout/cleanup for pending RPC requests.
request()registers a promise in#pendingRequeststhat is only ever settled when a matching response arrives viaresolveResponse. If the wallet never replies (or the session closes), these promises remain pending and the map grows unbounded. Consider rejecting outstanding requests on session close and/or adding a per-request timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/browser/create-remote-wallet-session.ts` around lines 233 - 246, The request() method currently leaves entries in `#pendingRequests` forever if no response arrives; add a per-request timeout and ensure pending requests are rejected on session close: when creating the pendingRequests entry in request(), attach a timeout (e.g., setTimeout) that rejects with a TimeoutError and deletes the entry from `#pendingRequests`; store the timer id on the pendingRequests value so it can be cleared when resolveResponse settles the promise; also update the session close/cleanup path (where `#sendNostrEvent` or session teardown happens) to iterate `#pendingRequests`, clear any timers, and call reject(...) for each outstanding request so the map is emptied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/browser/create-remote-wallet-session.ts`:
- Around line 377-379: The code assigns a possibly-rejecting promise to
session.authorizedSession when connect is true (session.authorizedSession =
connectRemoteWalletSession(...)), which can cause unhandled rejections if
callers ignore it; change the assignment to attach a no-op .catch handler (e.g.,
session.authorizedSession = connectRemoteWalletSession({ identity, session,
timeoutMs }).catch(() => {})) so any rejection is observed without changing
external behavior, ensuring connectRemoteWalletSession rejections are swallowed
safely while still preserving the promise reference used by callers (leave
pairingUrl logic unchanged).
- Around line 490-505: resolveSubscription currently marks settled=true and
removes websocket listeners which allows the relay to disconnect after
subscription/EOSE while leaving authorization.promise (from the authorize RPC
via RemoteWalletRpcClient.request) unresolved; fix by ensuring socket 'close'
and 'error' still reject the authorization promise even after
resolveSubscription runs and/or ensure RemoteWalletRpcClient.request rejects
outstanding requests on socket teardown: update handleClose/handleError (or the
socket teardown path) to, in addition to existing behavior, call
authorization.reject(new Error('socket closed')) when the socket closes/errors
after settled, and/or wire the socket close path into RemoteWalletRpcClient to
cancel/reject pending RPCs (and add per-RPC timeouts) so authorize cannot hang
indefinitely; reference resolveSubscription, handleClose, handleError,
authorization.promise, close, RemoteWalletRpcClient.request, and the authorize
RPC in your changes.
In `@src/cli/config.ts`:
- Around line 27-45: The parsing currently allows a flag to consume the next
token even if that token is another flag (e.g., "--label --chain") and treats
unknown flags (e.g., "--foo") as pairingUrl; update the argument parsing so that
any time you call requireValue(value, values.shift()) you first check the next
token and if it is missing or startsWith('--') throw a clear error (use the same
Error style used for unexpected arguments), and change the fallback that assigns
pairingUrl to instead reject any token that startsWith('--') as an unexpected
flag; locate usages around the variables and helpers chain, label, pairingUrl,
printSecret, rpcUrl, secretKey, timeoutMs and the requireValue helper in this
block and the similar block at lines 58-63 and apply the same deterministic
flag-missing / unknown-flag validation.
In `@src/cli/output.ts`:
- Line 2: formatJsonEvent currently serializes JSON.stringify({ event,
...payload }) which allows a caller-provided payload.event to override the event
argument; change it to ensure the explicit event argument wins (e.g., merge
payload first then set event, such as Object.assign({}, payload, { event }) or {
...payload, event }) so formatJsonEvent (and by extension writeJsonEvent) always
emits the provided event value regardless of payload contents.
In `@src/node/send-signed-transaction.ts`:
- Around line 41-52: The sendSignedTransaction call to fetchImpl lacks a
timeout, causing hung RPCs; update sendSignedTransaction to accept an optional
timeoutMs (with a sensible default), create an AbortSignal via
AbortSignal.timeout(timeoutMs), and pass that signal in the fetchImpl options
(i.e., add signal: AbortSignal.timeout(timeoutMs) alongside
method/headers/body). Locate the sendSignedTransaction function and the
fetchImpl invocation inside it and ensure callers that invoke
sendSignedTransaction can pass a timeoutMs or rely on the default.
In `@src/node/signer.ts`:
- Line 1: The code is using v2-style imports and APIs (importing from
'`@noble/curves/ed25519.js`' and calling ed25519.utils.randomSecretKey(), and
similarly secp256k1.js/schnorr.utils.randomSecretKey()), but package.json is
pinned to v1.x; either upgrade `@noble/curves` to a v2.x release or change the
modules to the v1 API: update src/node/signer.ts and src/protocol/nostr-event.ts
so they import the correct v1 packages (e.g., '`@noble/ed25519`' and the v1
secp256k1 package) and replace v2 method calls (ed25519.utils.randomSecretKey(),
schnorr.utils.randomSecretKey()) with the v1 equivalents (the v1 random key
generation utility names), ensuring imports and function names match the version
you choose.
In `@src/protocol/association-url.ts`:
- Around line 16-18: The pairing URL currently stores only the host in the
`relay` search param (via `new URL(relayUrl).host`), which drops scheme and
path; update `createNostrAssociationUrl` to store the full normalized relay URL
(including scheme and path) instead of just the host (use the same normalization
used by `normalizeRelayUrl` or call that function to produce a canonical full
URL) and ensure whatever parser that reads the `relay` param continues to call
`normalizeRelayUrl(relay)` or accept the full URL so ws/wss and path-based
relays round-trip correctly; update any tests or callers referencing the `relay`
param to expect the full URL.
In `@src/protocol/crypto.ts`:
- Around line 20-23: The error message in createSequenceNumberVector incorrectly
says "32-bytes" while the boundary check uses 2^32 (4_294_967_296), which is a
32-bit limit; update the thrown Error in createSequenceNumberVector to read
"Outbound sequence number overflow. The maximum sequence number is 32-bits." so
it accurately reflects the 32-bit limit referenced by the numeric check.
---
Nitpick comments:
In `@src/browser/create-remote-wallet-session.ts`:
- Around line 233-246: The request() method currently leaves entries in
`#pendingRequests` forever if no response arrives; add a per-request timeout and
ensure pending requests are rejected on session close: when creating the
pendingRequests entry in request(), attach a timeout (e.g., setTimeout) that
rejects with a TimeoutError and deletes the entry from `#pendingRequests`; store
the timer id on the pendingRequests value so it can be cleared when
resolveResponse settles the promise; also update the session close/cleanup path
(where `#sendNostrEvent` or session teardown happens) to iterate `#pendingRequests`,
clear any timers, and call reject(...) for each outstanding request so the map
is emptied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d86fc68-61e8-409a-b277-8523ba6efe33
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.changeset/remote-wallet-package.mdREADME.mdpackage.jsonsrc/bin/remote-wallet.tssrc/browser/create-remote-wallet-provider.tssrc/browser/create-remote-wallet-session.tssrc/browser/index.tssrc/browser/remote-wallet-standard-wallet.tssrc/cli.tssrc/cli/command.tssrc/cli/config.tssrc/cli/index.tssrc/cli/output.tssrc/index.tssrc/node/connect-remote-wallet.tssrc/node/index.tssrc/node/send-signed-transaction.tssrc/node/signer.tssrc/protocol/association-url.tssrc/protocol/constants.tssrc/protocol/crypto.tssrc/protocol/encoding.tssrc/protocol/errors.tssrc/protocol/index.tssrc/protocol/json-rpc.tssrc/protocol/nostr-event.tssrc/protocol/types.tssrc/react/index.tssrc/react/remote-wallet-pairing-panel.tsxsrc/react/use-remote-wallet-pairing.tstest/browser/create-remote-wallet-session.test.tstest/cli/config.test.tstest/cli/output.test.tstest/index.test.tstest/node/connect-remote-wallet.test.tstest/node/send-signed-transaction.test.tstest/node/signer.test.tstest/package-exports.test.tstest/protocol/association-url.test.tstest/protocol/encoding.test.tstest/protocol/json-rpc.test.tstest/protocol/nostr-event.test.tstest/react/use-remote-wallet-pairing.test.tstest/readme-smoke.test.tstsdown.config.ts
💤 Files with no reviewable changes (1)
- src/cli.ts
beeman
force-pushed
the
beeman/remote-wallet-package
branch
2 times, most recently
from
May 31, 2026 18:38
29a4689 to
cad641a
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
beeman
force-pushed
the
beeman/remote-wallet-package
branch
from
May 31, 2026 18:41
cad641a to
8b09876
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
beeman
commented
May 31, 2026
beeman
force-pushed
the
beeman/remote-wallet-package
branch
from
May 31, 2026 19:33
8b09876 to
1a3fbf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/browser/remote-wallet-standard-wallet.ts`:
- Around line 226-281: The connect() method has a TOCTOU where
this.#pendingConnect is only set after awaiting this.#provider.startPairing(),
allowing concurrent calls to race; reserve the pending slot synchronously by
creating and assigning a RemoteWalletPendingConnect object to
this.#pendingConnect before the await to ensure subsequent calls see and reuse
it. Widen RemoteWalletPendingConnect.session to allow undefined
(RemoteWalletPairingSession | undefined), set timeoutId and reject/resolve
handlers immediately, then call startPairing() and assign the returned session
into the existing pending object (and guard uses of pendingConnect.session where
needed); keep existing timeout/cancel and
`#clearPendingConnectTimeout/`#rejectPendingConnect flows intact so timeout,
cancel, resolve and reject still operate correctly.
In `@src/react/use-remote-wallet-pairing.ts`:
- Around line 6-26: The hook currently overwrites session in startPairing
without cancelling the previous session and lacks unmount cleanup; update
startPairing to use functional setSession so you can call cancel() on the prior
session (e.g., setSession(prev => { prev?.cancel(); return nextSession })) after
creating nextSession from createRemoteWalletSession, and add a React.useEffect
cleanup that calls session?.cancel() (or uses a ref and cancels the current
session) on unmount; ensure cancelPairing still sets session to null and that
all references use the same session state variable in useRemoteWalletPairing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ee436f8-5fd5-477c-abe6-a11f4a43b2d0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.changeset/remote-wallet-package.md.github/actions/setup/action.yml.github/workflows/ci.yml.gitignoreREADME.mdcompose.ymlpackage.jsonplaywright.config.tssrc/bin/remote-wallet.tssrc/browser/create-remote-wallet-provider.tssrc/browser/create-remote-wallet-session.tssrc/browser/index.tssrc/browser/remote-wallet-standard-wallet.tssrc/cli.tssrc/cli/command.tssrc/cli/config.tssrc/cli/index.tssrc/cli/output.tssrc/index.tssrc/node/connect-remote-wallet.tssrc/node/index.tssrc/node/send-signed-transaction.tssrc/node/signer.tssrc/protocol/association-url.tssrc/protocol/constants.tssrc/protocol/crypto.tssrc/protocol/encoding.tssrc/protocol/errors.tssrc/protocol/index.tssrc/protocol/json-rpc.tssrc/protocol/nostr-event.tssrc/protocol/types.tssrc/react/index.tssrc/react/remote-wallet-pairing-panel.tsxsrc/react/use-remote-wallet-pairing.tstest/browser/create-remote-wallet-session.test.tstest/browser/remote-wallet-standard-wallet.test.tstest/cli/config.test.tstest/cli/output.test.tstest/e2e/fixtures/remote-wallet-test-dapp/README.mdtest/e2e/fixtures/remote-wallet-test-dapp/index.htmltest/e2e/fixtures/remote-wallet-test-dapp/server.tstest/e2e/fixtures/remote-wallet-test-dapp/src/app.tstest/e2e/fixtures/remote-wallet-test-dapp/src/styles.csstest/e2e/remote-wallet-adapter-transaction.e2e.tstest/e2e/support/nostr-relay-container.tstest/index.test.tstest/node/connect-remote-wallet.test.tstest/node/send-signed-transaction.test.tstest/node/signer.test.tstest/package-exports.test.tstest/protocol/association-url.test.tstest/protocol/crypto.test.tstest/protocol/encoding.test.tstest/protocol/json-rpc.test.tstest/protocol/nostr-event.test.tstest/react/use-remote-wallet-pairing.test.tstest/readme-smoke.test.tstsdown.config.ts
💤 Files with no reviewable changes (1)
- src/cli.ts
✅ Files skipped from review due to trivial changes (13)
- .gitignore
- test/protocol/nostr-event.test.ts
- test/index.test.ts
- test/e2e/fixtures/remote-wallet-test-dapp/README.md
- src/protocol/constants.ts
- test/protocol/json-rpc.test.ts
- compose.yml
- test/protocol/crypto.test.ts
- .changeset/remote-wallet-package.md
- test/e2e/fixtures/remote-wallet-test-dapp/index.html
- src/bin/remote-wallet.ts
- src/protocol/types.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (26)
- test/protocol/encoding.test.ts
- src/protocol/index.ts
- src/react/index.ts
- src/cli/output.ts
- test/node/send-signed-transaction.test.ts
- tsdown.config.ts
- test/cli/config.test.ts
- src/cli/config.ts
- src/cli/index.ts
- test/node/connect-remote-wallet.test.ts
- test/cli/output.test.ts
- src/react/remote-wallet-pairing-panel.tsx
- src/index.ts
- src/protocol/errors.ts
- test/node/signer.test.ts
- test/package-exports.test.ts
- src/browser/index.ts
- src/node/index.ts
- test/readme-smoke.test.ts
- src/node/send-signed-transaction.ts
- src/protocol/json-rpc.ts
- test/protocol/association-url.test.ts
- src/browser/create-remote-wallet-provider.ts
- src/protocol/crypto.ts
- src/node/connect-remote-wallet.ts
- src/browser/create-remote-wallet-session.ts
beeman
force-pushed
the
beeman/remote-wallet-package
branch
from
May 31, 2026 19:45
1a3fbf4 to
ab6ce08
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add protocol, browser, Node, CLI, and React entrypoints for remote wallet pairing. Expose Wallet Standard registration and provider wiring so apps can register and handle the remote wallet outside React while still keeping the optional React helpers. Route connect, disconnect, account change events, sign-in, message signing, transaction signing, and sign-and-send through the remote session for both the browser wallet and CLI signer. Reserve pending Wallet Standard connects synchronously so concurrent connect calls share one pairing session instead of racing into duplicate sessions. Cancel replaced and unmounted React pairing sessions so optional React consumers do not leak in-flight relay sessions. Update the fixture app and e2e coverage to show the connected address and label, then prove the reference-app signing surface: sign in, sign message, sign transaction, and sign-and-send a localnet transaction.
beeman
force-pushed
the
beeman/remote-wallet-package
branch
from
May 31, 2026 19:51
ab6ce08 to
22797d8
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds reusable protocol, browser, Node, CLI, and React entrypoints for remote wallet pairing.
Reference app coverage:
Validation: