refactor(wallet): delegate Base wallet resolution to @blockrun/core - #26
Merged
Conversation
scanWallets, listDiscoveredWallets, importWallet, loadWallet, getOrCreateWallet and getWalletAddress now call the shared kernel instead of re-implementing the canonical order (env -> ~/.blockrun/.session -> legacy wallet.key) here. The duplication was not theoretical. This SDK fixed provider-wallet takeover in 3.7.1 (#14); core kept resolving provider wallet.json files first until @blockrun/core@0.1.0, so for three weeks the blockrun CLI signed x402 payments with whatever key a planted ~/.<app>/wallet.json supplied. One implementation cannot drift from itself. Public signatures are unchanged. All ten canonical-selection regression tests pass against the delegated implementation, including the ones proving a discovered wallet is never adopted automatically and that a file cannot claim an address it holds no key for. - importWallet(address) maps onto core's adoptWallet(); core's importWallet() takes a raw private key and is a different operation, which is why the two could not be reconciled before core 0.1.0. - BLOCKRUN_HOME now overrides the wallet directory (path resolution comes from core). Unset, behaviour is identical. - Solana wallet resolution stays SDK-local — core has no Solana key store yet. - Rebased onto 3.12.0 (Router Core V3 bundling); lockfile resolves @blockrun/core@0.1.1 from npm.
VickyXAI
pushed a commit
that referenced
this pull request
Aug 11, 2026
…review)
Seven-pass review (testing/maintainability/security specialists +
Claude/Codex adversarial + Codex structured) found two criticals, both
reproduced empirically:
- @blockrun/core publishes ESM-only, so keeping it external made the CJS
entry require("@blockrun/core") and throw ERR_PACKAGE_PATH_NOT_EXPORTED
for every CommonJS consumer at load — invisible to the all-ESM CI.
Core moves to devDependencies and tsup noExternal bundles it into both
formats (the router-core pattern), which also freezes the reviewed
kernel bytes into dist instead of floating on ^0.1.0.
- WALLET_DIR/WALLET_FILE were snapshotted at module load while core
re-resolves BLOCKRUN_HOME per call: change the env after import (dotenv
ordering) and getOrCreateWallet() minted a fresh key that saveWallet()
wrote over the user's real, possibly funded ~/.blockrun/.session with
no backup. Paths now resolve per call; the exported WALLET_FILE_PATH /
WALLET_DIR_PATH stay import-time snapshots for API compatibility and
are documented as such.
Also from the review:
- Pin the security properties that moved into core with six new tests:
env-beats-file, .session-before-legacy, unprefixed-key normalization,
the post-import BLOCKRUN_HOME split-brain regression, re-adopt without
backup, and a fixture-home guard so a core path regression can never
silently point the suite at the developer's real home.
- afterEach now restores BLOCKRUN_WALLET_KEY/BASE_CHAIN_WALLET_KEY too.
- scanWallets() TSDoc matches core's semantics (derived address, address
field ignored); importWallet() documents the name collision with
core's importWallet(rawKey); wallet.ts header states what stays local
and marks BLOCKRUN_HOME security-sensitive; solana-wallet.ts documents
why it is deliberately SDK-local; AGENTS.md/README/CHANGELOG synced.
Verified: 265 tests green; packed tarball into a clean project — CJS
require and ESM import both load, node_modules contains only @blockrun/llm.
Contributor
Author
|
Merged via
Also landed: six tests pinning the security properties that moved into core (env precedence, |
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.
Summary
scanWallets,listDiscoveredWallets,importWallet,loadWallet,getOrCreateWallet, andgetWalletAddressnow delegate to the shared@blockrun/corekernel instead of re-implementing the canonical resolution order (env → ~/.blockrun/.session → legacy wallet.key) in this packagesrc/wallet.tsdrops ~128 lines of duplicated logic; public signatures unchangedBLOCKRUN_HOMEnow overrides the wallet directory (from core's path resolution); unset, behaviour is identicalWhy
The duplication was not theoretical: this SDK fixed provider-wallet takeover in 3.7.1 (#14) while core kept resolving provider
wallet.jsonfiles first until@blockrun/core@0.1.0— for three weeks theblockrunCLI signed x402 payments with whatever key a planted~/.<app>/wallet.jsonsupplied. One implementation cannot drift from itself.Requires
@blockrun/core@^0.1.0(0.1.1 now on npm; the lockfile resolves it). Earlier versions carry the provider-takeover defect and must not be used.Validation
🤖 Generated with Claude Code