multi: Add Monero wallet - Phase I#3295
Conversation
martonp
left a comment
martonp
left a comment
There was a problem hiding this comment.
Initial pass. I got it to connect to simnet by starting updating the command to monero-wallet-rpc but the sync on the UI is staying at 0%.
I will play around with this. Simnet is not very useful at the moment. I think what should happen is the simnet option should only use the running daemon from the harness-alpha. Then follow the same pattern of starting a new
|
|
Resolved merge conflict in client/cmd/bisonw-desktop/go.mod by keeping github.com/zalando/go-keyring v0.2.6 in bisonw-desktop (required for Monero wallet, commit 4cf6bdd). CI passed. I cannot build UPDATE: |
|
Still TODO:
|
Add an XMR wallet that can:
- Configure a monerod
- configure and run monero rpc server
- Send, Receive & Withdraw funds
- Live reconfigure fee priority
- Rescan wallet transactions
- Tx Fee estimation
- New sub-addresses
Note: remote daemon login removed. It is unused by any remote daemons on <https://monero.fail> which all seem to be configured as 'restricted'. That means the RPCs they can answer are restricted in the monero code itself.
- Use monero-wallet-cli for initial sync - Enable wallet password storeage in os keystore - Allow 2 simnet wallet-rpc servers for simnet
dev-warrior777
force-pushed
the
xmr-wallet
branch
from
4cf6bdd to
91f45f4
Compare
|
TODO: previous bad suggestions
|
- app starts locked and user cannot unlock during initial sync - refactor keystore and tests
|
Key-store Test failed due to Missing Secret Service Implementation:There is no software installed that provides the org.freedesktop.secrets D-Bus service. Common implementations include:
Will comment out the test for CI |
Ok all the above is erroneous thinking. It was based on thinking we had need of the actual outputs (as we do for bitcoin, etc.) I looked again at monero swap flow we did last year and there are no contracts,etc on monero side .. just txs We may need to parse out key images to freeze unspents .. I will ask @martonp what he needs for Private Swaps |
- scripts and support to enable simnet development - added minimal back end code
// Sender sends with more granular control parameters
type Sender interface {
// SendAndLock sends the exact value to the specified address with an unlock time.
// This can be n blocks or n milliseconds depending on interpretation.
SendAndLock(address string, value uint64, unlock uint64, feeRate uint64) (Coin, error)
}
|
This is the end of Phase I. Some things to do in Phase II:
|
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
Looks great. Some comments.
| func (ks *keystore) get(net dex.Network) (string, error) { | ||
| svc, user, err := makeCreds(net) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| pw, err := keyring.Get(svc, user) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return pw, nil | ||
| } |
There was a problem hiding this comment.
How is this better than just saving the password as plaintext? It may be just genuinely don't know.
There was a problem hiding this comment.
I am not sure where else to put the password (derived from appPass) I receive in the first (*Driver).Create call .. it is already not plaintext and not available after that in (*Driver).Open or in asset.Wallet.Connect that follows in sequence. (See also: xmr.go)
- Since this is used as the monero wallet
.keysfile encryption password I would not want to store in plain text in ram. - Some of the structures in xmr.go/xmrpc.go where things could be stored are not yet initialized when the password is first used.
- As a seeded wallet the password can be recovered.
Create is where a monero wallet database file (dex.keys) is created and synced. There is no dex database access. So I use the keystore supplied by the OS in windows/linux/mac to store a stringified version of asset.CreateWalletParams.Pass which is a 32 byte slice.
This is also not new as a similar technique is used in Ethereum.
Note: Fitting monero requirements into basic dex structure is quite awkward .. I re-wrote startup and sync several times.
There was a problem hiding this comment.
Can another program use the svc value to get the password?
There was a problem hiding this comment.
Nuanced: Here is what the Gnome keyring developer has to say:
There was a problem hiding this comment.
It looks like yes, any program can get the password if they have the key, which is easy to duplicate.
There was a problem hiding this comment.
I made the test program in their readme here https://github.com/zalando/go-keyring
Then I commented out the part about setting the password and I was able to get the password. So, any program can get the password if they know the "service" and "user". imo this is not secure at all. Am I missing something? Also, this is not working on arch out of the box, I must be missing something there.
Anyway, I think we should find another answer to the problem if any program can grab the password. It's not much better than saving in plain text to a file. Am I wrong here?
There was a problem hiding this comment.
No need to change this in this pr, lets look into it in another.
There was a problem hiding this comment.
@JoeGruffins Have left you a message about my understanding of the threat you pose.
Leaving open
|
Looks good but I'm just a little worried about the password. We need it sooner correct? Like, the wallet isn't useable at all unless we "unlock" it? We may need to figure something out there. Did you say we are doing something similar with eth? Need to look. |
|
- Increased WalletServerInitializeWait to 5s and used select to allow context cancel. - Removed the "Unlock button while syncing" logic. - Other minor changes
|
Proposed Additional for Phase II based on above comments:
|
|
You are right
|
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
This is ongoing review, but wanted to comment on the password thing again.
| var td = make([]string, 0) | ||
| td = getUserDamons(td, net, cli, dataDir) |
There was a problem hiding this comment.
Probably just create the td wth "getUserDaemons"
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| b, err := os.ReadFile(userDaemonFilepath) | ||
| if err != nil { | ||
| return td | ||
| } | ||
| var daemons []daemon | ||
| err = json.Unmarshal(b, &daemons) | ||
| if err != nil { | ||
| return td | ||
| } |
There was a problem hiding this comment.
Can you log error if either of these errors? Also in getTrustedDaemons you already made var td = make([]string, 0) so if this errors that's nil again. If something is appended its no longer nil so it's fine really, but its weird to overwrite the td in the first place.
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| // getUserDamons returns any user defined daemons in dataDir/daemons.json. | ||
| // [cli] indicates that we can also select an HTTPS URL to send to monero-wallet-cli | ||
| // for initial sync. | ||
| func getUserDamons(td []string, net dex.Network, cli bool, dataDir string) []string { |
There was a problem hiding this comment.
| // getUserDamons returns any user defined daemons in dataDir/daemons.json. | |
| // [cli] indicates that we can also select an HTTPS URL to send to monero-wallet-cli | |
| // for initial sync. | |
| func getUserDamons(td []string, net dex.Network, cli bool, dataDir string) []string { | |
| // getUserDaemons returns any user defined daemons in dataDir/daemons.json. | |
| // [cli] indicates that we can also select an HTTPS URL to send to monero-wallet-cli | |
| // for initial sync. | |
| func getUserDaemons(td []string, net dex.Network, cli bool, dataDir string) []string { |
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| # It is for XMR golang development usage alongside the simnet-walletpair tool. | ||
| # The design of the golang code is such that it needs only a monerod instance | ||
| # and creates and tears down a monero-wallet-rpc process, maybe several times. | ||
| # The main harness is overkill for this and could be could be confusing when |
There was a problem hiding this comment.
| # The main harness is overkill for this and could be could be confusing when | |
| # The main harness is overkill for this and could be confusing when |
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| func (ks *keystore) get(net dex.Network) (string, error) { | ||
| svc, user, err := makeCreds(net) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| pw, err := keyring.Get(svc, user) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return pw, nil | ||
| } |
There was a problem hiding this comment.
I made the test program in their readme here https://github.com/zalando/go-keyring
Then I commented out the part about setting the password and I was able to get the password. So, any program can get the password if they know the "service" and "user". imo this is not secure at all. Am I missing something? Also, this is not working on arch out of the box, I must be missing something there.
Anyway, I think we should find another answer to the problem if any program can grab the password. It's not much better than saving in plain text to a file. Am I wrong here?
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
I'm unable to run atm because of the keystore gives me an error:
``
2025-10-16 08:56:16.916 [ERR] WEB: error creating xmr wallet: Error creating wallet: failed to store pw The name is not activatable
But that's fine. Everything looks great. Three things that I think we should look at further in ohter prs, no need to do now:
1. I still don't think the version checking is correct as it doesnt allow higher patches. Also looking at the dir name isn't great, I think something there needs to change.
2. The keystore looks to be easily exploitable and doesn't work for me out of the box anyhow.
3. Having the user download the tools separately. We can probably check if they have them installed already. Download for them like you suggested maybe even? Related to number 1
| ) | ||
|
|
||
| func (r *xmrRpc) probeDaemon() error { | ||
| r.log.Trace("probeDaemon") |
There was a problem hiding this comment.
Can trace a sentence, like, "Running probeDaemon."
But there is a debug later, so I don't think this log is necessary.
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| r.log.Debugf("daemon %s -- height: %d, synchronized: %v, restricted: %v, untrusted: %v", r.daemonAddr, | ||
| r.daemonState.height, r.daemonState.synchronized, r.daemonState.restricted, r.daemonState.untrusted) |
There was a problem hiding this comment.
Its probably better all the logs be sentences, or at least have some more info. This could start with "Probing daemon %s" so we know where it's being called.
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| } | ||
| // started | ||
| r.walletRpcProcess = cmd.Process | ||
| r.log.Debug("wallet rpc server is started") |
There was a problem hiding this comment.
I'll not dwell on these, but for instance, probably better as a sentence. Anyway should put in that this is xmr otherwise we dont know what wallet rpc server started:
| r.log.Debug("wallet rpc server is started") | |
| r.log.Debug("Started xmr wallet rpc server.") |
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| _, err := r.wallet.GetVersion(verCtx) | ||
| // just assume error means RPC or net error and the process is not running | ||
| if err != nil { | ||
| r.log.Debugf("get_version from the wallet server: %v", err) |
There was a problem hiding this comment.
| r.log.Debugf("get_version from the wallet server: %v", err) | |
| r.log.Debugf("Error getting xmr version from the wallet server: %v", err) |
There was a problem hiding this comment.
Leaving open for transfer to a new PR
| emit *asset.WalletEmitter | ||
| peersChange func(uint32, error) | ||
| dataDir string | ||
| cliToolsDir string |
There was a problem hiding this comment.
Do we need this? I think we can just ask the user to have the tools monero-wallet-rpc and monero-wallet-cli in path. My package manager has already installed these for me on arch with its xmr package. Is ubuntu different?
I guess it doesn't hurt to do it like this, but I have to download everything separately. Mine are just in /usr/bin/
For dcrwallet we ask the wallet already be running. Can we not just ask the user to run the wallet and cli tool first before starting bisonw?
Although that is more steps...
Sorry no need to change now I guess.
There was a problem hiding this comment.
This is ongoing for sure:
My preference would be to have a button on the monero wallet Create/Settings that:
- downloads the latest valid monero version of the tools.
- does not have to search for existing that may be already hard forked; or adjust paths
Monero's GUI wallet presents an update when it becomes available. I have not had the GUI wallet long enough to see what happens when there is a hard fork or other reason to discard outdated version.
There is also infrastructure in the daemon related to this and the possibility of downloading an update hash.
But let's try and find the best way that is a good UX but safe
There was a problem hiding this comment.
Leaving open for transfer to a new PR
Additional notes (Grok)Overview of go-keyring Usage for Cross-Platform Private Data StorageThe
This works out-of-the-box on all three platforms for basic use cases, with no additional setup on Windows or macOS. Focus on Linux below, as requested. 1. Only Works for the Logged-On UserThe library is strictly per-user (i.e., scoped to the currently logged-in user). It does not support system-wide storage:
No configuration changes this behavior—it's inherent to the library's design. 2. Linux Distros Without GNOME such as Archnon-GNOME distros (or minimal setups without GNOME Keyring) require installing and enabling the GNOME Keyring service, as the library exclusively uses its DBus Secret Service interface. There's no built-in support for alternatives like KDE Wallet (KWallet). Here's a breakdown:
Setup for Linux (Prioritizing Common Distros)Use your package manager to install. The library assumes the "login" collection exists (standard in most distros); if not, create it manually.
Compatibility Across Distros
NOTE: Some edits for clarity only |
That was updated in commit e1c0ce4 |
go-monero is a wrapper library on monero RPC's - previous repo: github.com/dev-warrior777/go-monero - current repo: github.com/bisoncraft/go-monero Also: answered most of JoeGruffins review decred#2. any outstanding issues will be addressed in a new PR.
3 participants
Fix Task: #3231
Add an XMR wallet that can:
- Configure a monerod
- configure and run monero rpc server
- Send, Receive & Withdraw funds
- Live reconfigure fee priority
- Tx Fee estimation
- New sub-addresses
later phases will add trading capability