feat(monero-sys): Monero bindings #303
Conversation
…round basic Wallet functions, depends on monero#9464
binarybaron
changed the title
binarybaron
changed the title
Removed the monero-wallet crate which was not being used anywhere in the codebase, simplifying the project structure. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
We will first replicate the monero wallet rpc interface, to keep our changes in the codebase minimal. These are the functions we currently use:
|
|
Check out this code from the We can most likely easily copy most of the integration tests from there. I'd try to keep it as simplistic as possible though. We only need to test the I'd also recommend you check out lib.rs from the same PR. You might pick up on some useful patterns. sneurlax and MrCyjaneK have a ton of experience working with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
monero-sys/src/bridge.rs (1)
331-336: Per-messagepanic::take_hook/set_hookis still expensive and racy
(This was flagged previously.)Swapping the panic hook on every log costs a lock and can interleave across threads. Install a one-time probe (e.g.
OnceCell<bool>) during init and just early-return when tracing is unavailable.
🧹 Nitpick comments (3)
.vscode/settings.json (1)
71-72:"*.rs": "cpp"disables Rust analyzer featuresAssociating every Rust file with the C++ language mode turns off rust-analyzer, code completion, and inline errors for the whole workspace. Unless you have a very specific reason, drop this mapping or scope it narrowly (e.g. only for the autogenerated
bridge.rs.hheader).monero-sys/src/bridge.h (2)
98-103: Avoid extra CString conversion on push_back
txids.push_back(txid.c_str());constructs a temporarystd::stringfrom the C-string.
push_back(txid);is clearer and one copy cheaper.
169-171: Self-include is redundant
#include "bridge.h"inside the same header is a no-op thanks to#pragma once, but it is confusing and risks circular inclusion when this file is copied/renamed. Drop the line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.vscode/settings.json(1 hunks)monero-sys/build.rs(1 hunks)monero-sys/src/bridge.h(1 hunks)monero-sys/src/bridge.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- monero-sys/build.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
monero-sys/src/bridge.h (2)
monero-sys/src/bridge.rs (3)
getWalletManager(63-63)address(138-139)forward_cpp_log(287-294)monero-sys/src/lib.rs (3)
address(975-978)seed(390-392)seed(1363-1366)
| { | ||
| // static variable to make sure we don't install twice. | ||
| bool installed = false; | ||
| std::string span_name; | ||
|
|
||
| /** |
There was a problem hiding this comment.
installed and span_name are racy globals
The log callback may run from multiple threads, yet the installed flag and span_name string are mutated/read without synchronisation.
Use std::atomic_bool installed and an immutable const std::string span_name set before publishing the callback, or protect them with a mutex.
Also applies to: 238-244
🤖 Prompt for AI Agents
In monero-sys/src/bridge.h around lines 178 to 183 and also lines 238 to 244,
the variables 'installed' and 'span_name' are accessed concurrently by multiple
threads without synchronization, causing race conditions. Change 'installed' to
be a std::atomic_bool to ensure atomic reads and writes, and make 'span_name' a
const std::string initialized before the callback is published to avoid
mutation. Alternatively, protect both variables with a mutex to synchronize
access and prevent data races.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
swap/src/monero/wallet.rs (2)
8-8: Remove unusedHashMapimport
HashMapis no longer referenced anywhere in this file. Keeping the import yields a compilation warning and adds noise.-use std::{collections::HashMap, path::PathBuf, sync::Arc}; +use std::{path::PathBuf, sync::Arc};
24-27: Fix typo in documentation“safty nets” → “safety nets”.
-/// When we're in regtest mode, we need to unplug some safty nets to make the Wallet work. +/// When we're in regtest mode, we need to unplug some safety nets to make the wallet work.docs/pages/becoming_a_maker/overview.mdx (1)
177-179: Grammar & clarity tweaks fordaemon_urldescription-| `daemon_url` | The URL of the Monero daemon (monerod) that the asb will connect to directly. The asb manages wallets inter... +| `daemon_url` | The URL of the Monero daemon (monerod) that the ASB connects to directly. The ASB manages its wallets internally via FFI bindings. Optional: if omitted, the ASB connects to a random public Monero node. |This fixes the truncated word (“inter…”) and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/pages/becoming_a_maker/overview.mdx(3 hunks)swap/src/monero/wallet.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/pages/becoming_a_maker/overview.mdx
[typographical] ~163-~163: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...do not recommend changing these settings, however we document them for completeness sake....
(HOWEVER_SENTENCE)
[uncategorized] ~163-~163: It appears that in this idiom a possessive apostrophe is missing.
Context: ...hese settings, however we document them for completeness sake. ```toml filename="config_mainnet.toml...
(FOR_NOUN_SAKE)
[misspelling] ~177-~177: Did you mean “too”?
Context: ...mon (monerod) that the asb will connect to directly. The asb manages wallets inter...
(TO_TOO)
| /// Get the current blockchain height. | ||
| /// May fail if not connected to a daemon. | ||
| pub async fn blockchain_height(&self) -> Result<BlockHeight> { | ||
| let wallet = self.main_wallet().await; | ||
|
|
||
| Ok(()) | ||
| Ok(BlockHeight { | ||
| height: wallet.blockchain_height().await.context( | ||
| "Failed to get blockchain height: wallet manager not connected to daemon", | ||
| )?, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Wait until the specified transfer has been completed or failed. | ||
| pub async fn watch_for_transfer( | ||
| wallet: Arc<Mutex<Wallet>>, | ||
| request: WatchRequest, | ||
| ) -> Result<(), InsufficientFunds> { | ||
| watch_for_transfer_with(wallet, request, None).await | ||
| } | ||
|
|
||
| /// Wait until the specified transfer has been completed or failed and listen to each new confirmation. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn watch_for_transfer_with( | ||
| wallet: Arc<Mutex<Wallet>>, | ||
| request: WatchRequest, | ||
| listener: Option<ConfirmationListener>, | ||
| ) -> Result<(), InsufficientFunds> { | ||
| let WatchRequest { | ||
| conf_target, | ||
| public_view_key, | ||
| public_spend_key, | ||
| transfer_proof, | ||
| expected, | ||
| } = request; | ||
|
|
||
| let txid = transfer_proof.tx_hash(); | ||
|
|
||
| tracing::info!( | ||
| %txid, | ||
| target_confirmations = %conf_target, | ||
| "Waiting for Monero transaction finality" | ||
| ); | ||
|
|
||
| let address = Address::standard( | ||
| wallet.lock().await.network, | ||
| public_spend_key, | ||
| public_view_key.into(), | ||
| ); | ||
|
|
||
| let check_interval = tokio::time::interval(wallet.lock().await.sync_interval.div(10)); | ||
| let wallet_name = wallet.lock().await.main_wallet.clone(); | ||
|
|
||
| wait_for_confirmations_with( | ||
| wallet.clone(), | ||
| transfer_proof, | ||
| address, | ||
| expected, | ||
| conf_target, | ||
| check_interval, | ||
| wallet_name, | ||
| listener, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct TransferRequest { | ||
| pub public_spend_key: PublicKey, | ||
| pub public_view_key: PublicViewKey, | ||
| pub amount: Amount, | ||
| } | ||
| /// Wait until a transfer is detected and confirmed. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate height helpers – consolidate into one
blockchain_height and block_height do the same job. Keeping both increases surface area and maintenance overhead.
Suggested approach:
- Keep the more descriptive
blockchain_height. - Remove
block_heightor mark it#[deprecated]until callers migrate.
No diff shown because choice depends on external call-sites.
Also applies to: 197-205
🤖 Prompt for AI Agents
In swap/src/monero/wallet.rs around lines 153-165 and 197-205, there are two
functions, blockchain_height and block_height, that perform the same task. To
reduce redundancy and maintenance overhead, keep the more descriptive
blockchain_height function and either remove the block_height function or mark
it as #[deprecated] to allow callers to migrate gradually. Ensure all external
call-sites are updated accordingly if you remove block_height.
| pub async fn wait_until_confirmed( | ||
| &self, | ||
| watch_request: WatchRequest, | ||
| listener: Option<impl Fn(u64) + Send + 'static>, | ||
| ) -> Result<()> { | ||
| let wallet = self.main_wallet().await; |
There was a problem hiding this comment.
Option<impl Fn(..)> is illegal – this will not compile
impl Trait cannot be nested inside another type. Use a boxed trait object instead.
- listener: Option<impl Fn(u64) + Send + 'static>,
+ listener: Option<Box<dyn Fn(u64) + Send + 'static>>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn wait_until_confirmed( | |
| &self, | |
| watch_request: WatchRequest, | |
| listener: Option<impl Fn(u64) + Send + 'static>, | |
| ) -> Result<()> { | |
| let wallet = self.main_wallet().await; | |
| pub async fn wait_until_confirmed( | |
| &self, | |
| watch_request: WatchRequest, | |
| listener: Option<Box<dyn Fn(u64) + Send + 'static>>, | |
| ) -> Result<()> { | |
| let wallet = self.main_wallet().await; |
🤖 Prompt for AI Agents
In swap/src/monero/wallet.rs around lines 170 to 175, the function parameter
uses `Option<impl Fn(u64) + Send + 'static>`, which is illegal because `impl
Trait` cannot be nested inside another type. To fix this, change the parameter
type to `Option<Box<dyn Fn(u64) + Send + 'static>>` to use a boxed trait object
instead.
| pub fn no_listener<T>() -> Option<impl Fn(T) + Send + 'static> { | ||
| Some(|_| {}) | ||
| } |
There was a problem hiding this comment.
no_listener returns an invalid type
Same impl Trait-in-Option problem as above plus it captures T, forcing monomorphisation for every type.
-pub fn no_listener<T>() -> Option<impl Fn(T) + Send + 'static> {
- Some(|_| {})
-}
+pub fn no_listener<T>() -> Option<Box<dyn Fn(T) + Send + 'static>> {
+ Some(Box::new(|_| {}))
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn no_listener<T>() -> Option<impl Fn(T) + Send + 'static> { | |
| Some(|_| {}) | |
| } | |
| pub fn no_listener<T>() -> Option<Box<dyn Fn(T) + Send + 'static>> { | |
| Some(Box::new(|_| {})) | |
| } |
🤖 Prompt for AI Agents
In swap/src/monero/wallet.rs around lines 219 to 221, the no_listener function
returns an Option containing an impl Trait, which is invalid and causes
monomorphisation for every type T. To fix this, change the function to return
Option<Box<dyn Fn(T) + Send + 'static>> instead, boxing the closure to erase the
concrete type and avoid monomorphisation. This will allow returning a single
concrete type inside the Option without capturing T generically.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
swap/src/monero/wallet.rs (3)
199-207: Duplicate helper (block_height) – same behaviour asblockchain_heightThe file still contains both helpers even after previous feedback. Please keep
one canonical function to avoid accidental divergence.
173-176:Option<impl Fn(..)>in parameter won’t compile on stable Rust
impl Traitcannot appear inside another type (here:Option<…>).
Compilation fails unless you enable unstable features few projects can rely on.- pub async fn wait_until_confirmed( - &self, - watch_request: WatchRequest, - listener: Option<impl Fn(u64) + Send + 'static>, + pub async fn wait_until_confirmed( + &self, + watch_request: WatchRequest, + listener: Option<Box<dyn Fn(u64) + Send + 'static>>,Update all call-sites accordingly (e.g. pass
no_listener()after refactor below).
221-223:no_listenerstill returns illegalOption<impl Fn(..)>Same root cause as above plus generic
Tcan’t be inferred by callers (it already fails inwait_until_synced(no_listener())).-pub fn no_listener<T>() -> Option<impl Fn(T) + Send + 'static> { - Some(|_| {}) +pub fn no_listener<U>() -> Option<Box<dyn Fn(U) + Send + 'static>> { + Some(Box::new(|_| {})) }Callers can now simply write
no_listener::<u64>()orno_listener()where type
inference suffices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
monero-sys/build.rs(1 hunks)swap/src/cli/event_loop.rs(7 hunks)swap/src/monero/wallet.rs(1 hunks)swap/tests/harness/mod.rs(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- swap/src/cli/event_loop.rs
- monero-sys/build.rs
🔇 Additional comments (1)
swap/tests/harness/mod.rs (1)
301-320: Mismatch between network (Mainnet) andregtestflag
Wallets::new(..., monero::Network::Mainnet, /*regtest=*/ true)mixes
main-net address semantics with regtest expectations.
This is fragile – e.g. address prefix & consensus rules differ.Verify whether
Network::Regtestcan be added tomonerocrate, or pass
Network::Mainnetand setregtest = false. At minimum, document why the
combination is safe.
| tracing::info!("Waiting for monero wallet to sync"); | ||
| xmr_wallet.wait_until_synced(no_listener()).await.unwrap(); | ||
|
|
||
| tracing::info!("Monero wallet synced"); | ||
|
|
There was a problem hiding this comment.
Generic type of no_listener() cannot be inferred
After the fix suggested in wallet.rs, this call must specify the type:
- xmr_wallet.wait_until_synced(no_listener()).await.unwrap();
+ xmr_wallet.wait_until_synced(no_listener::<u64>()).await.unwrap();Without the turbofish the compiler can’t deduce U.
🤖 Prompt for AI Agents
In swap/tests/harness/mod.rs around lines 393 to 397, the call to no_listener()
lacks explicit generic type specification, causing the compiler to fail
inferring the generic type U. Fix this by adding the turbofish syntax to specify
the concrete type parameter for no_listener(), matching the expected type in
wait_until_synced, so the compiler can correctly infer the generic type.
| trait Wallet { | ||
| type Amount; | ||
|
|
||
| async fn refresh(&self) -> Result<()>; | ||
| async fn get_balance(&self) -> Result<Self::Amount>; | ||
| fn refresh(&self) -> impl Future<Output = Result<()>>; | ||
| fn get_balance(&self) -> impl Future<Output = Result<Self::Amount>>; | ||
| } |
There was a problem hiding this comment.
impl Future in trait methods is not yet stable
Traits cannot return impl Trait without nightly features. Replace with a
boxed future:
use futures::future::BoxFuture;
trait Wallet {
type Amount;
- fn refresh(&self) -> impl Future<Output = Result<()>>;
- fn get_balance(&self) -> impl Future<Output = Result<Self::Amount>>;
+ fn refresh(&self) -> BoxFuture<'_, Result<()>>;
+ fn get_balance(&self) -> BoxFuture<'_, Result<Self::Amount>>;
}Remember to use futures::FutureExt; in impls to convert with .boxed().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trait Wallet { | |
| type Amount; | |
| async fn refresh(&self) -> Result<()>; | |
| async fn get_balance(&self) -> Result<Self::Amount>; | |
| fn refresh(&self) -> impl Future<Output = Result<()>>; | |
| fn get_balance(&self) -> impl Future<Output = Result<Self::Amount>>; | |
| } | |
| use futures::future::BoxFuture; | |
| trait Wallet { | |
| type Amount; | |
| fn refresh(&self) -> BoxFuture<'_, Result<()>>; | |
| fn get_balance(&self) -> BoxFuture<'_, Result<Self::Amount>>; | |
| } |
🤖 Prompt for AI Agents
In swap/tests/harness/mod.rs around lines 967 to 972, the trait methods return
`impl Future`, which is unstable in traits. Change the return types to boxed
futures by using `Box<dyn Future<Output = Result<...>> + Send + '_>`. Update the
method signatures accordingly and ensure that in the implementations you convert
the futures to boxed ones using `.boxed()` after importing `futures::FutureExt`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
monero-harness/src/lib.rs (1)
287-296:fund_addressstill ignores theamountargumentSame as the earlier review, the parameter is accepted but never used; the function always mines a fixed
120 + 10blocks. Callers cannot request a specific funding amount and the mined reward is opaque.Suggested fix:
-pub async fn fund_address(&self, address: &str, amount: u64) -> Result<()> { - let monerod = &self.monerod; - - // Make sure miner has funds by generating blocks - monerod.generate_blocks(120, address.to_string()).await?; - - // Mine more blocks to confirm the transaction - monerod.generate_blocks(10, address.to_string()).await?; - - tracing::info!("Successfully funded address with {} piconero", amount); - Ok(()) -} +pub async fn fund_address(&self, address: &str, amount: u64) -> Result<()> { + // Mine until the *estimated* block reward meets or exceeds `amount`. + // (≈0.6 XMR/block on regtest; tweak as necessary.) + const BLOCK_REWARD_PICO: u64 = 600_000_000_000; // 0.6 XMR + let blocks_needed = (amount + BLOCK_REWARD_PICO - 1) / BLOCK_REWARD_PICO; + + let monerod = &self.monerod; + monerod.generate_blocks(blocks_needed, address.to_string()).await?; + // extra 10 blocks for unlock + monerod.generate_blocks(10, address.to_string()).await?; + + tracing::info!("Successfully funded address with {} piconero", amount); + Ok(()) +}monero-sys/src/bridge.rs (1)
342-347: Per-logpanic::take_hookjuggling still presentTaking & resetting the panic hook on every log message is highly contended and still susceptible to the races flagged earlier. Switch to a once-only probe guarded by
OnceCell/AtomicBool, then early-return if tracing is unavailable.-static INIT: OnceCell<bool> = OnceCell::new(); - -// inside forward_cpp_log -if INIT.get_or_init(|| { - std::panic::take_hook(); - true -}) == &false { return; } +static TRACING_OK: AtomicBool = AtomicBool::new(true); +if !TRACING_OK.load(Ordering::Relaxed) { return; }
🧹 Nitpick comments (2)
monero-harness/src/lib.rs (2)
450-464:balance()forces a full sync on every call
balance()callsrefresh().await?which blocks untilwait_until_syncedcompletes.
This can easily turn a cheap getter into an un-bounded await, affecting test-suite latency and hiding deadlocks.Consider:
- Making the refresh explicit (let callers decide when to sync), or
- Adding a timeout or a fast-path that returns cached values when already synced.
391-409: Inefficient block-generation loop
generate_blocksissues one RPC per block with a 250 ms sleep.
Generating 120 blocks ⇒ 30 s wall-time instead of a single call.If reliability of the bulk RPC is the concern, a compromise of
min(amount, 20)blocks per request or a configurable batch size would speed up harness execution dramatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
monero-harness/src/lib.rs(8 hunks)monero-harness/tests/wallet.rs(2 hunks)monero-sys/build.rs(1 hunks)monero-sys/src/bridge.rs(1 hunks)monero-sys/src/lib.rs(1 hunks)swap/src/asb/command.rs(4 hunks)swap/src/asb/config.rs(9 hunks)swap/src/asb/recovery/punish.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- swap/src/asb/recovery/punish.rs
- swap/src/asb/config.rs
- swap/src/asb/command.rs
- monero-harness/tests/wallet.rs
- monero-sys/build.rs
- monero-sys/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
monero-harness/src/lib.rs (3)
monero-sys/src/lib.rs (21)
no_listener(84-86)new(621-631)new(672-688)new(932-934)new(955-986)new(1503-1508)new(1560-1562)address(999-1005)main_address(344-346)main_address(1023-1025)sync_progress(415-417)sync_progress(1061-1074)open_or_create(127-166)connected(420-422)connected(1076-1095)unlocked_balance(400-402)unlocked_balance(1208-1215)transfer(373-381)transfer(1302-1350)blockchain_height(353-370)blockchain_height(1168-1173)monero-harness/src/image.rs (4)
new(65-68)new(174-182)name(22-24)name(46-48)monero-rpc/src/monerod.rs (1)
new(32-47)
🔇 Additional comments (1)
monero-harness/src/lib.rs (1)
420-426: Mainnet network type used for regtest – confirm downstream expectations
WalletHandle::open_or_createis hard-coded tomonero::Network::Mainnet.
While regtest accepts mainnet prefixes, any address parsing or faucet logic that assumes regtest/testnet prefixes may break.Please double-check cross-crate assumptions (e.g. address pattern matching, UI hints) and add a comment or TODO if this is intentional.
This reverts commit acdab29.
This PR aims to bringt support for native Monero bindings to our codebase. We'll be migrating away from
monero-wallet-rpcand towardscxxbindings on top ofwallet2_api.h.Closes #114
This PR imports @Einliterflasche work on the
monero-wallet-systo this mono-repo.task list:
monerocrate formonero-sysstatusWithErrorStringto return an error if an ffi call failscore/swap/src/monero/wallet.rsto usemonero-sysmonero-walletcrate may be unused. Remove it after confirming.monero-harnessmonero-sysSummary by CodeRabbit