From ee11b368b6d958333d2d440081538f2d665943a4 Mon Sep 17 00:00:00 2001 From: Alain Brenzikofer Date: Wed, 9 Sep 2026 09:05:47 +0000 Subject: [PATCH] core: wallet review fixes, and show the derived addresses Concurrency: the account index is incremented in SQL and read back in the same transaction, so two profiles cannot be handed the same key. Importing a phrase is one transaction and single_seed is UNIQUE, so a phrase cannot be discarded in favour of a key created meanwhile, and a device cannot end up with two keys. Wallet commands are no longer forwarded to a remote host: the recovery phrase must not leave the device, and the raw command is logged there. /wallet delete removes the key, confirmed by the last word of the phrase, so creating a key before importing your own is no longer a dead end. /wallet now shows every profile on the key with the first two name addresses each, to check derivation against other wallets. Hidden profiles are left out, as they are by /users. A bad phrase no longer says which word was wrong. /wallet export uses the profile's own key. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Rvc3HbiWBTqbAvRT45G5oX --- CHANGELOG.md | 15 +-- bots/src/API/Docs/Commands.hs | 1 + src/Simplex/Chat/Controller.hs | 8 +- src/Simplex/Chat/Help.hs | 26 ++-- src/Simplex/Chat/Library/Commands.hs | 57 +++++---- .../Migrations/M20260908_wallet_seeds.hs | 7 +- .../Store/Postgres/Migrations/chat_schema.sql | 8 +- .../Migrations/M20260908_wallet_seeds.hs | 17 +-- .../Store/SQLite/Migrations/chat_schema.sql | 11 +- src/Simplex/Chat/Store/Wallets.hs | 113 +++++++++++------- src/Simplex/Chat/View.hs | 21 ++-- src/Simplex/Chat/Wallet.hs | 12 +- tests/WalletTests.hs | 88 +++++++++----- 13 files changed, 232 insertions(+), 152 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa98875d6f..8a35756854 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,12 @@ ## Unreleased -Wallet (in development, CLI only). A key on the device, so that a name bought -later has an owner the app can still derive: -- `/wallet create` creates one BIP-39 key per device and one BIP-44 account per - chat profile under it, `/wallet` shows the address that would own the next - name that profile buys, `/wallet import` and `/wallet export` move the key - with its recovery phrase. -- A name key sits at `m/44'/60'/'/0/`, which is ordinary BIP-44, - so the phrase reaches the same addresses in other wallets. -- No signing, so nothing can be bought or edited yet. +Wallet (in development, CLI only): creates the key that will own your SimpleX +names, so a name you buy later has an owner this app can still derive. One key +per device, one account per chat profile, each name at `m/44'/60'/'/0/` +(standard BIP-44, so your recovery phrase works in other wallets). `/wallet` +shows the addresses, `/wallet import` and `/wallet export` move the key, +`/wallet delete` removes it. You cannot buy a name yet. ## v6.5 diff --git a/bots/src/API/Docs/Commands.hs b/bots/src/API/Docs/Commands.hs index c875c3f5e6..c08e63fe0c 100644 --- a/bots/src/API/Docs/Commands.hs +++ b/bots/src/API/Docs/Commands.hs @@ -452,6 +452,7 @@ undocumentedCommands = "APIVerifyToken", "APIWallet", "APIWalletCreate", + "APIWalletDelete", "APIWalletExport", "APIWalletImport", "CheckChatRunning", diff --git a/src/Simplex/Chat/Controller.hs b/src/Simplex/Chat/Controller.hs index 0f1e77cbc0..37e900f8a9 100644 --- a/src/Simplex/Chat/Controller.hs +++ b/src/Simplex/Chat/Controller.hs @@ -421,6 +421,7 @@ data ChatCommand | APIWalletCreate | APIWalletImport {recoveryPhrase :: Text} | APIWalletExport + | APIWalletDelete {confirmWord :: Text} | APISendCallInvitation ContactId CallType | SendCallInvitation ContactName CallType | APIRejectCall ContactId @@ -746,6 +747,11 @@ allowRemoteCommand = \case DeleteRemoteCtrl _ -> False ExecChatStoreSQL _ -> False ExecAgentStoreSQL _ -> False + APIWallet -> False + APIWalletCreate -> False + APIWalletImport _ -> False + APIWalletExport -> False + APIWalletDelete _ -> False _ -> True data RelayConnectionResult = RelayConnectionResult @@ -847,7 +853,7 @@ data ChatResponse | CRContactRequestRejected {user :: User, contactRequest :: UserContactRequest, contact_ :: Maybe Contact} | CRServiceResponse {user :: User, responseData :: J.Object} | CRServiceReplyAccepted {user :: User, connectionId :: AgentConnId} - | CRWallet {user :: User, walletKeyExists :: Bool, walletAccount :: Maybe (AccountIndex, Text, Text)} + | CRWallet {user :: User, walletKeyExists :: Bool, walletAccounts :: [(Text, AccountIndex, Bool, [(Text, Text)])]} | CRWalletPhrase {user :: User, recoveryPhrase :: Text} | CRUserAcceptedGroupSent {user :: User, groupInfo :: GroupInfo, hostContact :: Maybe Contact} | CRUserDeletedMembers {user :: User, groupInfo :: GroupInfo, members :: [GroupMember], withMessages :: Bool, msgSigned :: Bool} diff --git a/src/Simplex/Chat/Help.hs b/src/Simplex/Chat/Help.hs index 7ca1d632cd..81be9ce8ac 100644 --- a/src/Simplex/Chat/Help.hs +++ b/src/Simplex/Chat/Help.hs @@ -223,20 +223,24 @@ walletHelpInfo :: [StyledString] walletHelpInfo = map styleMarkdown - [ green "Your wallet key:", - indent <> highlight "/wallet " <> " - the address that would own the next name you buy", - indent <> highlight "/wallet create " <> " - create the key, and this profile's account under it", - indent <> highlight "/wallet import " <> " - use a key you already have", - indent <> highlight "/wallet export " <> " - the recovery phrase, to write down", + [ green "Wallet commands:", + indent <> highlight "/wallet " <> " - your key, and the addresses it derives", + indent <> highlight "/wallet create " <> " - create your key, or add this profile to it", + indent <> highlight "/wallet import " <> " - use a key you already have", + indent <> highlight "/wallet export " <> " - show your recovery phrase", + indent <> highlight "/wallet delete " <> " - delete the key, confirmed by the last word of the phrase", "", - "One key per device, and one account per chat profile under it. A name gets", - "its own key at " <> highlight "m/44'/60'/'/0/" <> ". That is ordinary BIP-44,", - "so importing the phrase into another wallet reaches the same addresses.", + "Please note: this is in development. You cannot buy a name yet.", "", - "Anyone who knows a recovery phrase controls the names it owns. The risk is", - "theft, not loss.", + "One key per device, one account per chat profile. Each name gets its own", + "key at " <> highlight "m/44'/60'/'/0/" <> ". This is standard BIP-44, so your", + "phrase works in other wallets.", "", - "Please note: this is in development. Nothing can be bought or signed yet." + "The key is stored in the chat database. It is only encrypted if you set a", + "database passphrase with " <> highlight "/db encrypt" <> ", and it is included in " <> highlight "/db export" <> ".", + "", + "Anyone who has your recovery phrase controls your names. Keep it secret,", + "and keep a copy." ] incognitoHelpInfo :: [StyledString] diff --git a/src/Simplex/Chat/Library/Commands.hs b/src/Simplex/Chat/Library/Commands.hs index c8672ab10b..77018c177a 100644 --- a/src/Simplex/Chat/Library/Commands.hs +++ b/src/Simplex/Chat/Library/Commands.hs @@ -58,8 +58,8 @@ import qualified Data.UUID.V4 as V4 import Simplex.Chat.Library.Subscriber import Simplex.Chat.Badges (BadgeCredential (..), LocalBadge (..), badgeServerCredential, maxXFTPFileSize, mkBadgeStatus, verifyCredential) import Simplex.Chat.Names (SimplexDomainProof (..), SimplexDomainClaim (..), claimDomain, mkDomainClaim) -import Simplex.Chat.Store.Wallets (boundAccount, deviceSeed, getOrCreateAccountRef) -import Simplex.Chat.Wallet (AccountRef (..), accountAddress, deriveNameKey, importRecoveryKey, newSeed, recoveryKeyPhrase, renderNameKeyPath) +import Simplex.Chat.Store.Wallets (deleteSeed, getBoundAccount, getDeviceSeed, getOrCreateAccountRef, getSeedAccounts, importSeed) +import Simplex.Chat.Wallet (NameIndex, WalletSeed (..), accountAddress, deriveNameKey, importRecoveryKey, newSeed, recoveryKeyPhrase, renderNameKeyPath) import Simplex.Chat.Call import Simplex.Chat.Controller import Simplex.Chat.Delivery (DeliveryJobScope (..), DeliveryJobSpec (..), DeliveryWorkerScope (..)) @@ -1491,34 +1491,41 @@ processChatCommand cxt nm = \case let AgentInvId invId = requestId connId <- withAgent $ \a -> sendServiceReplyAsync a "" (aUserId user) invId (LB.toStrict $ J.encode responseData) pure $ CRServiceReplyAccepted user (AgentConnId connId) - -- Read-only: a profile is never given keys as a side effect of asking which - -- address it has. APIWallet -> withUser $ \user -> do - exists <- isJust <$> withFastStore' deviceSeed - acc_ <- withFastStore' $ \db -> boundAccount db user - -- name index 0: with no purchases yet the next name is always the first - a <- forM acc_ $ \(seed, AccountRef {arIndex}) -> do - acc <- either (throwCmdError . ("wallet: " <>)) pure $ deriveNameKey seed arIndex 0 - pure (arIndex, renderNameKeyPath arIndex 0, tshow (accountAddress acc)) - pure $ CRWallet user exists a - -- Creates the device key on first use, and this profile's account under it. - -- A second profile lands on its own account index rather than sharing one. + seed_ <- withFastStore' getDeviceSeed + accs <- case seed_ of + Nothing -> pure [] + Just seed -> do + -- a hidden profile is left out, as it is by /users + as <- filter (\(_, _, active, hidden) -> active || not hidden) <$> withFastStore' (\db -> getSeedAccounts db (wsId seed)) + forM as $ \(n, acct, active, _) -> do + keys <- forM [0 .. walletNamesShown - 1] $ \k -> do + acc <- either (throwCmdError . ("wallet: " <>)) pure $ deriveNameKey seed acct k + pure (renderNameKeyPath acct k, tshow (accountAddress acc)) + pure (n, acct, active, keys) + pure $ CRWallet user (isJust seed_) accs APIWalletCreate -> withUser $ \user -> do g <- asks random - void $ withFastStore' $ \db -> getOrCreateAccountRef db user (atomically $ newSeed MS256 g) + entropy <- atomically $ newSeed MS256 g + void $ withFastStore' $ \db -> getOrCreateAccountRef db user entropy processChatCommand cxt nm APIWallet - -- One key per device: importing onto a device that already has one would make - -- the addresses it shows depend on which key was picked. APIWalletImport phrase -> withUser $ \user -> do - exists <- isJust <$> withFastStore' deviceSeed - when exists $ throwCmdError "this device already has a wallet key" - entropy <- either (throwCmdError . ("wallet: " <>)) pure $ importRecoveryKey (encodeUtf8 phrase) - void $ withFastStore' $ \db -> getOrCreateAccountRef db user (pure entropy) + entropy <- either (const $ throwCmdError "bad recovery phrase") pure $ importRecoveryKey (encodeUtf8 phrase) + r <- withFastStore' $ \db -> importSeed db user entropy + when (isNothing r) $ throwCmdError "this device already has a wallet key" processChatCommand cxt nm APIWallet APIWalletExport -> withUser $ \user -> do - seed <- withFastStore' deviceSeed >>= maybe (throwCmdError "no wallet key on this device") pure + (seed, _) <- withFastStore' (\db -> getBoundAccount db user) >>= maybe (throwCmdError noKeyError) pure phrase <- either (throwCmdError . ("wallet: " <>)) pure $ recoveryKeyPhrase seed pure $ CRWalletPhrase user (safeDecodeUtf8 phrase) + APIWalletDelete confirmWord -> withUser $ \user -> do + seed <- withFastStore' getDeviceSeed >>= maybe (throwCmdError noKeyError) pure + phrase <- either (throwCmdError . ("wallet: " <>)) pure $ recoveryKeyPhrase seed + case reverse . T.words $ safeDecodeUtf8 phrase of + w : _ | w == confirmWord -> do + withFastStore' $ \db -> deleteSeed db (wsId seed) + processChatCommand cxt nm APIWallet + _ -> throwCmdError "to confirm, pass the last word of the recovery phrase" APISendCallInvitation contactId callType -> withUser $ \user -> do -- party initiating call ct <- withFastStore $ \db -> getContact db cxt user contactId @@ -5455,6 +5462,13 @@ withExpirationDate globalTTL chatItemTTL action = do let ttl = fromMaybe globalTTL chatItemTTL when (ttl > 0) $ action $ addUTCTime (-1 * fromIntegral ttl) currentTs +-- | Name keys shown per profile by /wallet, to check derivation against other wallets. +walletNamesShown :: NameIndex +walletNamesShown = 2 + +noKeyError :: String +noKeyError = "no wallet key for this profile - create one with /wallet create" + chatCommandP :: Parser ChatCommand chatCommandP = choice @@ -5576,6 +5590,7 @@ chatCommandP = "/wallet create" $> APIWalletCreate, "/wallet import " *> (APIWalletImport <$> textP), "/wallet export" $> APIWalletExport, + "/wallet delete " *> (APIWalletDelete <$> textP), "/wallet" $> APIWallet, "/_call invite @" *> (APISendCallInvitation <$> A.decimal <* A.space <*> jsonP), "/call " *> char_ '@' *> (SendCallInvitation <$> displayNameP <*> pure defaultCallType), diff --git a/src/Simplex/Chat/Store/Postgres/Migrations/M20260908_wallet_seeds.hs b/src/Simplex/Chat/Store/Postgres/Migrations/M20260908_wallet_seeds.hs index c91c96ac3a..04b8b3a46f 100644 --- a/src/Simplex/Chat/Store/Postgres/Migrations/M20260908_wallet_seeds.hs +++ b/src/Simplex/Chat/Store/Postgres/Migrations/M20260908_wallet_seeds.hs @@ -12,9 +12,10 @@ m20260908_wallet_seeds = CREATE TABLE wallet_seeds ( wallet_seed_id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, seed BYTEA NOT NULL, - -- High-water mark for account allocation; see the SQLite migration for why - -- this cannot be derived from MAX(users.wallet_account_index). - next_account_index BIGINT NOT NULL DEFAULT 0 + -- Known issue: see the SQLite migration. + next_account_index BIGINT NOT NULL DEFAULT 0, + -- one key per device for now; drop when several are supported + single_seed SMALLINT NOT NULL DEFAULT 1 UNIQUE ); ALTER TABLE users ADD COLUMN wallet_seed_id BIGINT REFERENCES wallet_seeds ON DELETE RESTRICT; diff --git a/src/Simplex/Chat/Store/Postgres/Migrations/chat_schema.sql b/src/Simplex/Chat/Store/Postgres/Migrations/chat_schema.sql index 8c6526e5b2..b629dd814f 100644 --- a/src/Simplex/Chat/Store/Postgres/Migrations/chat_schema.sql +++ b/src/Simplex/Chat/Store/Postgres/Migrations/chat_schema.sql @@ -1538,7 +1538,8 @@ ALTER TABLE test_chat_schema.users ALTER COLUMN user_id ADD GENERATED ALWAYS AS CREATE TABLE test_chat_schema.wallet_seeds ( wallet_seed_id bigint NOT NULL, seed bytea NOT NULL, - next_account_index bigint DEFAULT 0 NOT NULL + next_account_index bigint DEFAULT 0 NOT NULL, + single_seed smallint DEFAULT 1 NOT NULL ); @@ -1917,6 +1918,11 @@ ALTER TABLE ONLY test_chat_schema.wallet_seeds +ALTER TABLE ONLY test_chat_schema.wallet_seeds + ADD CONSTRAINT wallet_seeds_single_seed_key UNIQUE (single_seed); + + + ALTER TABLE ONLY test_chat_schema.xftp_file_descriptions ADD CONSTRAINT xftp_file_descriptions_pkey PRIMARY KEY (file_descr_id); diff --git a/src/Simplex/Chat/Store/SQLite/Migrations/M20260908_wallet_seeds.hs b/src/Simplex/Chat/Store/SQLite/Migrations/M20260908_wallet_seeds.hs index 5255241bfd..36242b9647 100644 --- a/src/Simplex/Chat/Store/SQLite/Migrations/M20260908_wallet_seeds.hs +++ b/src/Simplex/Chat/Store/SQLite/Migrations/M20260908_wallet_seeds.hs @@ -5,28 +5,23 @@ module Simplex.Chat.Store.SQLite.Migrations.M20260908_wallet_seeds where import Database.SQLite.Simple (Query) import Database.SQLite.Simple.QQ (sql) --- | Wallet seeds: BIP-39 entropy, one or more per database. --- --- The schema allows several seeds, with each chat profile bound to exactly one --- of them plus its own BIP-44 account index. Only the single-seed case is --- reachable today. m20260908_wallet_seeds :: Query m20260908_wallet_seeds = [sql| CREATE TABLE wallet_seeds ( wallet_seed_id INTEGER PRIMARY KEY AUTOINCREMENT, seed BLOB NOT NULL, -- BIP-39 entropy, 16-32 bytes - -- High-water mark for account allocation. Deliberately not derived from - -- MAX(users.wallet_account_index): after recovery from the phrase alone that - -- table is empty while accounts 0..N already hold names on chain, so a new - -- profile would silently reuse a recovered account's keys. - next_account_index INTEGER NOT NULL DEFAULT 0 + -- Known issue: after importing a phrase this starts at 0, so a recovered + -- device can re-issue an account that already owns names. A recovery scan + -- will raise it. + next_account_index INTEGER NOT NULL DEFAULT 0, + -- one key per device for now; drop when several are supported + single_seed INTEGER NOT NULL DEFAULT 1 UNIQUE ) STRICT; ALTER TABLE users ADD COLUMN wallet_seed_id INTEGER REFERENCES wallet_seeds ON DELETE RESTRICT; ALTER TABLE users ADD COLUMN wallet_account_index INTEGER; --- required: the .lint check enforces an index on every foreign key CREATE INDEX idx_users_wallet_seed_id ON users(wallet_seed_id); |] diff --git a/src/Simplex/Chat/Store/SQLite/Migrations/chat_schema.sql b/src/Simplex/Chat/Store/SQLite/Migrations/chat_schema.sql index 7b616ebd3b..fa44bfe6fe 100644 --- a/src/Simplex/Chat/Store/SQLite/Migrations/chat_schema.sql +++ b/src/Simplex/Chat/Store/SQLite/Migrations/chat_schema.sql @@ -857,11 +857,12 @@ CREATE TABLE rcv_roster_transfers( CREATE TABLE wallet_seeds( wallet_seed_id INTEGER PRIMARY KEY AUTOINCREMENT, seed BLOB NOT NULL, -- BIP-39 entropy, 16-32 bytes - -- High-water mark for account allocation. Deliberately not derived from - -- MAX(users.wallet_account_index): after recovery from the phrase alone that - -- table is empty while accounts 0..N already hold names on chain, so a new - -- profile would silently reuse a recovered account's keys. -next_account_index INTEGER NOT NULL DEFAULT 0 + -- Known issue: after importing a phrase this starts at 0, so a recovered + -- device can re-issue an account that already owns names. A recovery scan + -- will raise it. + next_account_index INTEGER NOT NULL DEFAULT 0, + -- one key per device for now; drop when several are supported + single_seed INTEGER NOT NULL DEFAULT 1 UNIQUE ) STRICT; CREATE INDEX contact_profiles_index ON contact_profiles( display_name, diff --git a/src/Simplex/Chat/Store/Wallets.hs b/src/Simplex/Chat/Store/Wallets.hs index 32eb213d10..ac1ef938fb 100644 --- a/src/Simplex/Chat/Store/Wallets.hs +++ b/src/Simplex/Chat/Store/Wallets.hs @@ -2,39 +2,42 @@ {-# LANGUAGE LambdaCase #-} {-# LANGUAGE NamedFieldPuns #-} {-# LANGUAGE OverloadedStrings #-} +{-# LANGUAGE QuasiQuotes #-} --- | Persistence for wallet seeds and per-profile accounts. --- --- The schema holds several seeds and binds each chat profile to one of them --- plus its own account index. One seed per device is reachable today, so --- 'deviceSeed' is the seed, and 'getOrCreateAccountRef' creates it on first use. module Simplex.Chat.Store.Wallets - ( deviceSeed, - boundAccount, + ( getDeviceSeed, + getBoundAccount, + getSeedAccounts, getOrCreateAccountRef, + importSeed, + deleteSeed, ) where import Data.ByteString (ByteString) import Data.Int (Int64) +import Data.Maybe (isJust) +import Data.Text (Text) import Simplex.Chat.Store.Shared (insertedRowId) import Simplex.Chat.Types (User (..)) import Simplex.Chat.Wallet (AccountIndex, AccountRef (..), SeedId (..), WalletSeed (..)) import Simplex.Messaging.Agent.Store.AgentStore (maybeFirstRow) +import Simplex.Messaging.Agent.Store.DB (BoolInt (..)) import qualified Simplex.Messaging.Agent.Store.DB as DB #if defined(dbPostgres) import Database.PostgreSQL.Simple (Only (..)) +import Database.PostgreSQL.Simple.SqlQQ (sql) #else import Database.SQLite.Simple (Only (..)) +import Database.SQLite.Simple.QQ (sql) #endif toSeed :: (Int64, ByteString) -> WalletSeed toSeed (sId, seed) = WalletSeed {wsId = SeedId sId, wsEntropy = seed} --- | The seed on this device, or Nothing if the wallet has never been used. -deviceSeed :: DB.Connection -> IO (Maybe WalletSeed) -deviceSeed db = +getDeviceSeed :: DB.Connection -> IO (Maybe WalletSeed) +getDeviceSeed db = maybeFirstRow toSeed $ DB.query_ db "SELECT wallet_seed_id, seed FROM wallet_seeds ORDER BY wallet_seed_id LIMIT 1" @@ -43,14 +46,6 @@ getWalletSeed db (SeedId sId) = maybeFirstRow toSeed $ DB.query db "SELECT wallet_seed_id, seed FROM wallet_seeds WHERE wallet_seed_id = ?" (Only sId) --- | Insert a seed. Callers generate the entropy; this module never does, so the --- DRG stays with the agent. -createWalletSeed :: DB.Connection -> ByteString -> IO WalletSeed -createWalletSeed db seed = do - DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only seed) - sId <- insertedRowId db - pure WalletSeed {wsId = SeedId sId, wsEntropy = seed} - getAccountRef :: DB.Connection -> User -> IO (Maybe AccountRef) getAccountRef db User {userId} = do r <- @@ -67,39 +62,63 @@ bindAccount db User {userId} AccountRef {arSeedId = SeedId sId, arIndex} = "UPDATE users SET wallet_seed_id = ?, wallet_account_index = ? WHERE user_id = ?" (sId, fromIntegral arIndex :: Int64, userId) --- | The seed and account this profile is bound to, or Nothing if it has never --- used the wallet. Creates nothing: a profile is never given keys as a side --- effect of reading. -boundAccount :: DB.Connection -> User -> IO (Maybe (WalletSeed, AccountRef)) -boundAccount db user = +getBoundAccount :: DB.Connection -> User -> IO (Maybe (WalletSeed, AccountRef)) +getBoundAccount db user = getAccountRef db user >>= \case Nothing -> pure Nothing Just r -> fmap (\s -> (s, r)) <$> getWalletSeed db (arSeedId r) --- | Bind this profile to the device's seed, creating that seed from @mkSeed@ if --- there is none yet. Every profile gets its own account index within it. -getOrCreateAccountRef :: DB.Connection -> User -> IO ByteString -> IO (WalletSeed, AccountRef) -getOrCreateAccountRef db user mkSeed = - boundAccount db user >>= \case - Just bound -> pure bound - Nothing -> do - s <- deviceSeed db >>= maybe (mkSeed >>= createWalletSeed db) pure - ix <- takeAccountIndex db (wsId s) - let r = AccountRef {arSeedId = wsId s, arIndex = ix} - bindAccount db user r - pure (s, r) +-- | Profiles bound to this seed: display name, account index, whether active, +-- whether hidden. +getSeedAccounts :: DB.Connection -> SeedId -> IO [(Text, AccountIndex, Bool, Bool)] +getSeedAccounts db (SeedId sId) = + map toRow + <$> DB.query + db + [sql| + SELECT local_display_name, wallet_account_index, active_user, view_pwd_hash + FROM users WHERE wallet_seed_id = ? ORDER BY wallet_account_index + |] + (Only sId) + where + toRow (n, ix, BI active, pwdHash) = (n, fromIntegral (ix :: Int64), active, isJust (pwdHash :: Maybe ByteString)) --- | Take the next account index and advance the seed's high-water mark. --- --- The mark is stored rather than computed as @MAX(users.wallet_account_index)@, --- because after recovery from the phrase alone the @users@ table is empty while --- accounts @0..N@ already hold names on chain. Computing it would hand the first --- newly created profile index 0 and, with it, a recovered account's keys. +-- | Bind this profile to the device seed, creating it from @entropy@ if there +-- is none. +getOrCreateAccountRef :: DB.Connection -> User -> ByteString -> IO (WalletSeed, AccountRef) +getOrCreateAccountRef db user entropy = + getBoundAccount db user >>= \case + Just bound -> pure bound + Nothing -> getDeviceSeed db >>= maybe (createWalletSeed db entropy) pure >>= bindNewAccount db user + +-- | Nothing if the device already has a key. One transaction, so a phrase +-- cannot be discarded in favour of a key created meanwhile; single_seed is +-- UNIQUE, so a concurrent insert cannot add a second key either. +importSeed :: DB.Connection -> User -> ByteString -> IO (Maybe (WalletSeed, AccountRef)) +importSeed db user entropy = + getDeviceSeed db >>= \case + Just _ -> pure Nothing + Nothing -> Just <$> (createWalletSeed db entropy >>= bindNewAccount db user) + +bindNewAccount :: DB.Connection -> User -> WalletSeed -> IO (WalletSeed, AccountRef) +bindNewAccount db user s = do + ix <- takeAccountIndex db (wsId s) + let r = AccountRef {arSeedId = wsId s, arIndex = ix} + bindAccount db user r + pure (s, r) + +createWalletSeed :: DB.Connection -> ByteString -> IO WalletSeed +createWalletSeed db entropy = do + DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only entropy) + sId <- insertedRowId db + pure WalletSeed {wsId = SeedId sId, wsEntropy = entropy} + +-- | Incremented in SQL so that concurrent purchases cannot be handed the same +-- account, and read back inside the same transaction. takeAccountIndex :: DB.Connection -> SeedId -> IO AccountIndex takeAccountIndex db sId@(SeedId sId') = do - ix <- getNextAccountIndex db sId - DB.execute db "UPDATE wallet_seeds SET next_account_index = ? WHERE wallet_seed_id = ?" (fromIntegral ix + 1 :: Int64, sId') - pure ix + DB.execute db "UPDATE wallet_seeds SET next_account_index = next_account_index + 1 WHERE wallet_seed_id = ?" (Only sId') + subtract 1 <$> getNextAccountIndex db sId getNextAccountIndex :: DB.Connection -> SeedId -> IO AccountIndex getNextAccountIndex db (SeedId sId) = @@ -107,3 +126,9 @@ getNextAccountIndex db (SeedId sId) = <$> ( maybeFirstRow fromOnly $ DB.query db "SELECT next_account_index FROM wallet_seeds WHERE wallet_seed_id = ?" (Only sId) ) + +-- | Profiles are unbound first: the foreign key is ON DELETE RESTRICT. +deleteSeed :: DB.Connection -> SeedId -> IO () +deleteSeed db (SeedId sId) = do + DB.execute db "UPDATE users SET wallet_seed_id = NULL, wallet_account_index = NULL WHERE wallet_seed_id = ?" (Only sId) + DB.execute db "DELETE FROM wallet_seeds WHERE wallet_seed_id = ?" (Only sId) diff --git a/src/Simplex/Chat/View.hs b/src/Simplex/Chat/View.hs index d50695e532..73c599eee9 100644 --- a/src/Simplex/Chat/View.hs +++ b/src/Simplex/Chat/View.hs @@ -189,16 +189,17 @@ chatResponseToView hu cfg@ChatConfig {logLevel, showReactions, showFullLinks, te CRContactRequestRejected u UserContactRequest {localDisplayName = c} _ct_ -> ttyUser u [ttyContact c <> ": contact request rejected"] CRServiceResponse u resp -> ttyUser u ["service response: " <> viewJSON resp] CRServiceReplyAccepted u (AgentConnId cId) -> ttyUser u [plain $ "service reply accepted, connection id: " <> safeDecodeUtf8 (strEncode cId)] - CRWallet u exists acc_ -> - ttyUser u $ case acc_ of - Just (acct, path, addr) -> - [ plain $ "wallet account " <> tshow acct, - plain $ "next name will be owned by " <> addr, - plain $ " at " <> path - ] - Nothing - | exists -> ["wallet key on this device, but this profile has no account - add one with " <> highlight' "/wallet create"] - | otherwise -> ["no wallet key on this device - create one with " <> highlight' "/wallet create"] + CRWallet u exists accs + | not exists -> ttyUser u ["no wallet key on this device - create one with " <> highlight' "/wallet create"] + | otherwise -> + ttyUser u $ + ("key 1" : concatMap accountRows accs) + <> ["this profile has no key yet - add one with " <> highlight' "/wallet create" | not (any (\(_, _, active, _) -> active) accs)] + where + accountRows (n, acct, active, keys) = + plain (" account " <> tshow acct <> " (" <> n <> (if active then ", active" else "") <> ")") + : zipWith nameRow [0 :: Int ..] keys + nameRow k (path, addr) = plain $ " name " <> tshow k <> " " <> path <> " " <> addr CRWalletPhrase u phrase -> ttyUser u [ "write this down - anyone who knows these words controls the names this key owns:", diff --git a/src/Simplex/Chat/Wallet.hs b/src/Simplex/Chat/Wallet.hs index c4a031a92b..e35efc7a98 100644 --- a/src/Simplex/Chat/Wallet.hs +++ b/src/Simplex/Chat/Wallet.hs @@ -2,12 +2,12 @@ -- | The wallet: BIP-39 seeds, and the keys derived from them. -- --- * __seed__ — BIP-39 entropy. Generic, /not/ name-specific. --- * __account__ — a profile's slot in a seed, BIP-44 account index @i@. --- * __name key__ — @m\/44'\/60'\/i'\/0\/k@: one key per name, at BIP-44 +-- * __seed__: BIP-39 entropy. Generic, /not/ name-specific. +-- * __account__: a profile's slot in a seed, BIP-44 account index @i@. +-- * __name key__: @m\/44'\/60'\/i'\/0\/k@: one key per name, at BIP-44 -- address index @k@ under the profile that buys it. This is what the -- registry records as the name's owner. --- * __wallet__ — this module: creation and derivation. +-- * __wallet__: this module, creation and derivation. -- -- One key per name, not one per profile. A per-profile key would mean exporting -- it hands over every name that profile owns, and would put every name's signed @@ -91,7 +91,7 @@ instance Show WalletAccount where show a = "WalletAccount " <> show (waRef a) <> " " -- | Fresh seed entropy. The caller stores it; this module never persists. --- A 25th-word passphrase is deliberately not used — it would be a second secret +-- A 25th-word passphrase is deliberately not used: it would be a second secret -- to back up. newSeed :: B39.MnemonicStrength -> TVar ChaChaDRG -> STM ByteString newSeed strength g = B39.mnemonicToEntropy <$> B39.randomMnemonic strength g @@ -106,7 +106,7 @@ importRecoveryKey phrase = B39.mnemonicToEntropy <$> B39.parseMnemonic phrase recoveryKeyPhrase :: WalletSeed -> Either String ByteString recoveryKeyPhrase s = B39.mnemonicPhrase <$> B39.entropyToMnemonic (wsEntropy s) --- | @m\/44'\/60'\/i'\/0\/k@ — the standard BIP-44 layout, with the profile at +-- | @m\/44'\/60'\/i'\/0\/k@ is the standard BIP-44 layout, with the profile at -- the account level and the name at the address level. Nothing here is a custom -- path, so profile @i@'s names are the account list an ordinary Ethereum wallet -- would show for that account. diff --git a/tests/WalletTests.hs b/tests/WalletTests.hs index 9c39797dc7..3566fe353d 100644 --- a/tests/WalletTests.hs +++ b/tests/WalletTests.hs @@ -6,11 +6,10 @@ module WalletTests where import ChatClient import ChatTests.DBUtils import ChatTests.Utils -import Control.Monad (replicateM_) import Data.ByteString.Char8 (ByteString) import qualified Data.ByteString.Char8 as B import Data.Either (isLeft) -import Simplex.Chat.Help (walletHelpInfo) +import Data.List (intersect, nub) import Simplex.Chat.Wallet (SeedId (..), WalletSeed (..), accountAddress, deriveNameKey, importRecoveryKey, recoveryKeyPhrase, renderNameKeyPath) import Test.Hspec hiding (it) import qualified Test.Hspec as Hspec @@ -49,19 +48,21 @@ walletDerivationTests = do walletTests :: SpecWith TestParams walletTests = do - it "creates no key until asked, then shows the next name's address" testWalletCreate - it "the key and the address come back after a restart" testWalletPersists - it "a second profile gets its own account" testWalletSecondProfile + it "creates no key until asked, then shows the derived addresses" testWalletCreate + it "the key and the addresses come back after a restart" testWalletPersists + it "a second profile gets its own account, on the same key" testWalletSecondProfile it "imports a phrase, exports it, and refuses a second import" testWalletImport + it "deletes the key only with the last word of the phrase" testWalletDelete --- | The address a name would be bought at, and the path to reach it from the --- phrase. Returned so tests can compare addresses without pinning a random one. -nextNameAddress :: HasCallStack => TestCC -> Int -> IO String -nextNameAddress cc acct = do - cc <## ("wallet account " <> show acct) - addr <- getTermLine cc - cc <## (" at m/44'/60'/" <> show acct <> "'/0/0") - pure addr +-- | The derivation path and address of each name shown for a profile's account. +accountRows :: HasCallStack => TestCC -> String -> Int -> IO [(String, String)] +accountRows cc profile acct = do + cc <## (" account " <> show acct <> " (" <> profile <> ")") + mapM (\_ -> nameRow <$> getTermLine cc) [0 .. 1 :: Int] + where + nameRow l = case words l of + ["name", _, path, addr] -> (path, addr) + _ -> error $ "unexpected wallet row: " <> l testWalletCreate :: HasCallStack => TestParams -> IO () testWalletCreate ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do @@ -71,48 +72,75 @@ testWalletCreate ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do alice ##> "/wallet" alice <## "no wallet key on this device - create one with /wallet create" alice ##> "/wallet export" - alice <## "bad chat command: no wallet key on this device" + alice <## "bad chat command: no wallet key for this profile - create one with /wallet create" alice ##> "/wallet create" - _ <- nextNameAddress alice 0 - alice ##> "/help wallet" - alice <## "Your wallet key:" - replicateM_ (length walletHelpInfo - 1) (getTermLine alice) + alice <## "key 1" + rows <- accountRows alice "alice, active" 0 + -- one key per name: the two addresses differ and sit at consecutive indices + map fst rows `shouldBe` ["m/44'/60'/0'/0/0", "m/44'/60'/0'/0/1"] + length (nub $ map snd rows) `shouldBe` 2 testWalletPersists :: HasCallStack => TestParams -> IO () testWalletPersists ps = do - addr <- withNewTestChat ps "alice" aliceProfile $ \alice -> do + rows <- withNewTestChat ps "alice" aliceProfile $ \alice -> do alice ##> "/wallet create" - nextNameAddress alice 0 + alice <## "key 1" + accountRows alice "alice, active" 0 -- same database, new session: the seed has to come back from the DB, or the -- name bought at that address is unreachable withTestChat ps "alice" $ \alice -> do alice ##> "/wallet" - addr' <- nextNameAddress alice 0 - addr' `shouldBe` addr + alice <## "key 1" + rows' <- accountRows alice "alice, active" 0 + rows' `shouldBe` rows testWalletSecondProfile :: HasCallStack => TestParams -> IO () testWalletSecondProfile ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do alice ##> "/wallet create" - addr <- nextNameAddress alice 0 + alice <## "key 1" + rows <- accountRows alice "alice, active" 0 alice ##> "/create user alisa" showActiveUser alice "alisa" alice ##> "/wallet" - alice <## "wallet key on this device, but this profile has no account - add one with /wallet create" + alice <## "key 1" + _ <- accountRows alice "alice" 0 + alice <## "this profile has no key yet - add one with /wallet create" -- the same key, a different account, so the two profiles do not share names alice ##> "/wallet create" - addr' <- nextNameAddress alice 1 - addr' `shouldNotBe` addr + alice <## "key 1" + _ <- accountRows alice "alice" 0 + rows' <- accountRows alice "alisa, active" 1 + null (map snd rows `intersect` map snd rows') `shouldBe` True testWalletImport :: HasCallStack => TestParams -> IO () testWalletImport ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do alice ##> ("/wallet import " <> B.unpack testPhrase) - alice <## "wallet account 0" - alice <## "next name will be owned by 0x9858EfFD232B4033E47d90003D41EC34EcaEda94" - alice <## " at m/44'/60'/0'/0/0" + alice <## "key 1" + alice <## " account 0 (alice, active)" + alice <## " name 0 m/44'/60'/0'/0/0 0x9858EfFD232B4033E47d90003D41EC34EcaEda94" + alice <## " name 1 m/44'/60'/0'/0/1 0x6Fac4D18c912343BF86fa7049364Dd4E424Ab9C0" alice ##> "/wallet export" alice <## "write this down - anyone who knows these words controls the names this key owns:" alice <## (" " <> B.unpack testPhrase) - -- one key per device: a second would make the address shown depend on which + -- one key per device: a second would make the addresses shown depend on which -- key was picked alice ##> ("/wallet import " <> B.unpack testPhrase) alice <## "bad chat command: this device already has a wallet key" + -- a mistyped phrase says nothing about which word was wrong + alice ##> ("/wallet import " <> B.unpack (B.unwords $ replicate 12 "abandon")) + alice <## "bad chat command: bad recovery phrase" + +testWalletDelete :: HasCallStack => TestParams -> IO () +testWalletDelete ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do + alice ##> ("/wallet import " <> B.unpack testPhrase) + alice <## "key 1" + _ <- accountRows alice "alice, active" 0 + alice ##> "/wallet delete abandon" + alice <## "bad chat command: to confirm, pass the last word of the recovery phrase" + alice ##> "/wallet delete about" + alice <## "no wallet key on this device - create one with /wallet create" + -- deleting unbinds the profile, so a real phrase can now be imported + alice ##> ("/wallet import " <> B.unpack testPhrase) + alice <## "key 1" + _ <- accountRows alice "alice, active" 0 + pure ()