-
Notifications
You must be signed in to change notification settings - Fork 109
fix(monero): harden address, purge, and unlocked balance #1765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@xchainjs/xchain-monero': patch | ||
| --- | ||
|
|
||
| Harden Monero client: sync getAddress/setPhrase, clear wallet state on purge, return unlocked balance from wallet-rpc getBalance, add getWalletBalanceDetail, and refuse transfers above unlocked |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,8 @@ import { | |
| TxHistoryParams, | ||
| TxType, | ||
| } from '@xchainjs/xchain-client' | ||
| import { getSeed } from '@xchainjs/xchain-crypto' | ||
| import { Address, baseAmount } from '@xchainjs/xchain-util' | ||
| import { getSeed, validatePhrase } from '@xchainjs/xchain-crypto' | ||
| import { Address, BaseAmount, baseAmount } from '@xchainjs/xchain-util' | ||
| import { keccak_256 } from '@noble/hashes/sha3' | ||
| import slip10 from 'micro-key-producer/slip10.js' | ||
|
|
||
|
|
@@ -81,48 +81,55 @@ export class Client extends BaseXChainClient { | |
| } | ||
|
|
||
| /** | ||
| * Get the current address asynchronously. | ||
| * Derives keys from mnemonic and encodes as Monero address (pure JS, no WASM). | ||
| * Derive the Monero address for a wallet index (pure JS, sync). | ||
| */ | ||
| public async getAddressAsync(index?: number): Promise<string> { | ||
| public getAddress(index?: number): string { | ||
| const spendKey = this.getPrivateSpendKey(index ?? 0) | ||
| const keys = deriveKeyPairs(spendKey) | ||
| const networkType = getMoneroNetworkType(this.getNetwork()) | ||
| return encodeAddress(keys.publicSpendKey, keys.publicViewKey, networkType) | ||
| } | ||
|
|
||
| public async getAddressAsync(index?: number): Promise<string> { | ||
| return this.getAddress(index) | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated Use getAddressAsync instead | ||
| * Set or update the mnemonic. Clears cached scan / LWS / wallet-rpc session state | ||
| * when the phrase changes so a new wallet cannot reuse another wallet's cache. | ||
| */ | ||
| public getAddress(): string { | ||
| throw Error('Sync method not supported') | ||
| public setPhrase(phrase: string, walletIndex = 0): Address { | ||
| if (this.phrase !== phrase) { | ||
| if (!validatePhrase(phrase)) { | ||
| throw new Error('Invalid phrase') | ||
| } | ||
| this.phrase = phrase | ||
| this.resetWalletState() | ||
| } | ||
| return this.getAddress(walletIndex) | ||
| } | ||
|
|
||
| /** | ||
| * Clear phrase and all wallet session state (scan cache, LWS login, rpc lock). | ||
| */ | ||
| public purgeClient(): void { | ||
| super.purgeClient() | ||
| this.resetWalletState() | ||
| } | ||
|
|
||
| public validateAddress(address: Address): boolean { | ||
| return validateMoneroAddress(address) | ||
| } | ||
|
|
||
| /** | ||
| * Get balance via wallet-rpc (local monerod), then LWS, then a bounded daemon scan. | ||
| * Get spendable balance via wallet-rpc (unlocked), then LWS, then a bounded daemon scan. | ||
| * For wallet-rpc, prefer {@link getWalletBalanceDetail} when both total and unlocked are needed. | ||
| */ | ||
| public async getBalance(address: Address): Promise<Balance[]> { | ||
| const walletRpcUrls = this.walletRpcUrls[this.getNetwork()] | ||
| if (walletRpcUrls && walletRpcUrls.length > 0) { | ||
| const ownAddress = await this.getAddressAsync(0) | ||
| if (address !== ownAddress) { | ||
| throw new Error('Monero wallet RPC can only return the balance for the unlocked wallet address') | ||
| } | ||
| let lastError: unknown | ||
| for (const url of walletRpcUrls) { | ||
| try { | ||
| const amount = await this.getBalanceFromWalletRpc(url) | ||
| return [{ asset: AssetXMR, amount: baseAmount(amount.toString(), XMR_DECIMALS) }] | ||
| } catch (error) { | ||
| lastError = error | ||
| console.warn(`Wallet RPC ${url} failed for getBalance:`, (error as Error).message) | ||
| } | ||
| } | ||
| throw lastError instanceof Error ? lastError : new Error('All Monero wallet RPC endpoints failed for getBalance') | ||
| const detail = await this.getWalletBalanceDetail(address) | ||
| return [{ asset: AssetXMR, amount: detail.unlocked }] | ||
| } | ||
|
|
||
| // Try LWS next | ||
|
|
@@ -305,6 +312,37 @@ export class Client extends BaseXChainClient { | |
| return { total: allOutputs.length, txs } | ||
| } | ||
|
|
||
| /** | ||
| * Total and unlocked balances from monero-wallet-rpc (own address only). | ||
| * Unlocked is what can be spent immediately; total includes locked outputs. | ||
| */ | ||
| public async getWalletBalanceDetail(address: Address): Promise<{ total: BaseAmount; unlocked: BaseAmount }> { | ||
| const walletRpcUrls = this.walletRpcUrls[this.getNetwork()] | ||
| if (!walletRpcUrls || walletRpcUrls.length === 0) { | ||
| throw new Error('getWalletBalanceDetail requires walletRpcUrls') | ||
| } | ||
| const ownAddress = this.getAddress(0) | ||
| if (address !== ownAddress) { | ||
| throw new Error('Monero wallet RPC can only return the balance for the unlocked wallet address') | ||
| } | ||
| let lastError: unknown | ||
| for (const url of walletRpcUrls) { | ||
| try { | ||
| const result = await this.getBalanceFromWalletRpc(url) | ||
| return { | ||
| total: baseAmount(result.total.toString(), XMR_DECIMALS), | ||
| unlocked: baseAmount(result.unlocked.toString(), XMR_DECIMALS), | ||
| } | ||
| } catch (error) { | ||
| lastError = error | ||
| console.warn(`Wallet RPC ${url} failed for getWalletBalanceDetail:`, (error as Error).message) | ||
| } | ||
| } | ||
| throw lastError instanceof Error | ||
| ? lastError | ||
| : new Error('All Monero wallet RPC endpoints failed for getWalletBalanceDetail') | ||
| } | ||
|
|
||
| /** | ||
| * Transfer XMR to a recipient address via monero-wallet-rpc. | ||
| * The in-process RingCT builder is not used: it is not consensus-compatible. | ||
|
|
@@ -484,21 +522,38 @@ export class Client extends BaseXChainClient { | |
| return address | ||
| } | ||
|
|
||
| private async getBalanceFromWalletRpc(url: string): Promise<bigint> { | ||
| private resetWalletState(): void { | ||
| this.scanCache = null | ||
| this.lwsLoggedIn = false | ||
| this.walletRpcLock = Promise.resolve() | ||
| } | ||
|
Comment on lines
+525
to
+529
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
# Verify whether ensureWallet opens or generates a process-wide wallet on the RPC endpoint.
ast-grep outline packages/xchain-monero/src/walletRpc.ts --items all --match ensureWallet
rg -n -C 12 'ensureWallet|open_wallet|generate_from_keys|close_wallet' packages/xchain-monero/src/walletRpc.tsRepository: xchainjs/xchainjs-lib Length of output: 3372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- ensureWallet implementation ---'
sed -n '268,325p' packages/xchain-monero/src/walletRpc.ts
printf '%s\n' '--- wallet RPC lock and reset call sites ---'
rg -n -C 8 'walletRpcLock|withWalletRpcLock|resetWalletState|ensureWallet|prepareWalletRpc|getBalance|transfer' packages/xchain-monero/src/client.tsRepository: xchainjs/xchainjs-lib Length of output: 12569 Keep the active wallet-RPC queue during a state reset.
🤖 Prompt for AI Agents |
||
|
|
||
| private async getBalanceFromWalletRpc(url: string): Promise<{ total: bigint; unlocked: bigint }> { | ||
| return this.withWalletRpcLock(async () => { | ||
| await this.prepareWalletRpc(url) | ||
| const result = await walletRpc.callWithBusyRetry(() => walletRpc.getBalance(url)) | ||
| return BigInt(result.balance) | ||
| return { | ||
| total: BigInt(result.balance), | ||
| unlocked: BigInt(result.unlockedBalance), | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| private async transferViaWalletRpc(url: string, params: TxParams): Promise<string> { | ||
| return this.withWalletRpcLock(async () => { | ||
| await this.prepareWalletRpc(url, params.walletIndex ?? 0) | ||
| const balances = await walletRpc.callWithBusyRetry(() => walletRpc.getBalance(url)) | ||
| const unlocked = BigInt(balances.unlockedBalance) | ||
| const amountPiconero = BigInt(params.amount.amount().toFixed(0)) | ||
| if (amountPiconero > unlocked) { | ||
| throw new Error( | ||
| `Insufficient unlocked balance: need ${amountPiconero.toString()} piconero, unlocked ${unlocked.toString()}`, | ||
| ) | ||
| } | ||
| return walletRpc.callWithBusyRetry(() => | ||
| walletRpc.transfer(url, { | ||
| address: params.recipient, | ||
| amountPiconero: params.amount.amount().toFixed(0).toString(), | ||
| amountPiconero: amountPiconero.toString(), | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test the wallet-state reset contract.
This test passes if
resetWalletState()is removed.super.purgeClient()already clearsphrase, so the assertion only verifies inherited phrase clearing.Seed observable scan-cache and LWS-session state. Verify that both
setPhrasewith a new phrase andpurgeClientclear that state.As per coding guidelines, “Define verifiable success criteria for each task, write regression tests for fixes or validation changes where applicable, and verify each step of multi-step work.”
🤖 Prompt for AI Agents
Source: Coding guidelines