feat: show list of subaddresses #685
Conversation
|
very cool! I'll review this ASAP |
|
Compilations fails with this error on my machine: |
|
bugbot run |
|
The changes look good! Please also write a test for the functionality that was added to |
src-gui/src/renderer/components/pages/monero/SubaddressesModal.tsx
Outdated
Show resolved
Hide resolved
src-gui/src/renderer/components/pages/monero/SubaddressesModal.tsx
Outdated
Show resolved
Hide resolved
src-gui/src/renderer/components/pages/monero/SubaddressesModal.tsx
Outdated
Show resolved
Hide resolved
src-gui/src/renderer/components/pages/monero/SubaddressesModal.tsx
Outdated
Show resolved
Hide resolved
binarybaron
left a comment
binarybaron
left a comment
There was a problem hiding this comment.
Looks promising! I'm impressed for quickly you got a hold of the codebase. This is definitely going into the right direction.
I'm reviewing this fairly strictly / in-depth because my hope is that I can pass onto you some of the unofficial and undocumented guidelines / best practices we have for the project.
|
The |
Does this compile on your machine? @rafael-xmr |
Einliterflasche
left a comment
Einliterflasche
left a comment
There was a problem hiding this comment.
I am getting this error:
[monero-sys 0.1.0] cargo:warning=In file included from /Users/me/code/xmr-btc-swap/target/debug/build/monero-sys-b1c8e6bd6f43bf85/out/cxxbridge/sources/monero-sys/src/bridge.rs.cc:2:
[monero-sys 0.1.0] cargo:warning=src/bridge.h:686:48: error: functions that differ only in their return type cannot be overloaded
[monero-sys 0.1.0] cargo:warning= 686 | inline std::unique_ptr<std::vector<TxKey>> pendingTransactionTxKeys(const PendingTransaction &tx, const std::string &tx_hash)
[monero-sys 0.1.0] cargo:warning= | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
[monero-sys 0.1.0] cargo:warning=src/bridge.h:384:54: note: previous definition is here
[monero-sys 0.1.0] cargo:warning= 384 | inline std::unique_ptr<std::vector<std::string>> pendingTransactionTxKeys(const PendingTransaction &tx, const std::string &tx_hash)
[monero-sys 0.1.0] cargo:warning= | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
It seems you added another pendingTxKeys function. But the existing one should work too. Getting rid of the new function should also remove the other two errors:
[monero-sys 0.1.0] cargo:warning=src/bridge.h:390:18: error: no matching member function for call to 'push_back'
[monero-sys 0.1.0] cargo:warning= 390 | vec->push_back(std::move(key));
[monero-sys 0.1.0] cargo:warning= | ~~~~~^~~~~~~~~
[monero-sys 0.1.0] cargo:warning=/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__vector/vector.h:452:60: note: candidate function not viable: no known conversion from '__libcpp_remove_reference_t<tuple<string, string, string> &>' (aka 'std::tuple<std::string, std::string, std::string>') to 'const value_type' (aka 'const std::string') for 1st argument
[monero-sys 0.1.0] cargo:warning= 452 | _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_back(const_reference __x) { emplace_back(__x); }
[monero-sys 0.1.0] cargo:warning= | ^ ~~~~~~~~~~~~~~~~~~~
C++ compilation errors in monero-sys are a bit hard to debug, because there is so much output. The gist is: compile with -vv (or -- -vv for tauri), scroll up until you see the colored warning: monero-sys@0.1.0: Applying patch to file: src/wallet/wallet2.h lines. Above that, when the colors stop are the real c++ compilation errors/warnings.
|
Ok cool, compiles on my machine. The Rust code looks fairly good to me but @Einliterflasche should give this another review since this is changing some things about I really like how snappy it feels. It's good that the addresses are cached and I think it's good that you made a modal for the subaddresses. Regarding the layout, I think we can still make some improvements: I'm unsure if this is the best place to display the subaddress. We should also consider not displaying it if its just the primary address. In its current form it is fairly wide and causes the column to be quite wide as well which leaves less space for the timestamp and the other things (which are arguably more important to the user). 
We may want to go into more of the direction of Feather Wallet here and use an actual table. I'm not a huge fan when clicking things changes the layout (when you click the "Edit label" button). The label could just be a TextField (like in Feather wallet). It'd have the current label pre-filled and editing would change it. You could do this by adding some logic that if the user did not change the text in the field for more than 5s, we save the label in the wallet. I'm also unsure if we even need to display the "tx count" for each subaddress. 
|
|
How about something like this for the subaddresses? diff --git a/src-gui/src/renderer/components/pages/monero/components/TransactionItem.tsx b/src-gui/src/renderer/components/pages/monero/components/TransactionItem.tsx
index f6aef9d3..20ccb2b8 100644
--- a/src-gui/src/renderer/components/pages/monero/components/TransactionItem.tsx
+++ b/src-gui/src/renderer/components/pages/monero/components/TransactionItem.tsx
@@ -6,7 +6,7 @@ import {
MenuItem,
Typography,
} from "@mui/material";
-import { TransactionDirection, TransactionInfo } from "models/tauriModel";
+import { SubaddressSummary, TransactionDirection, TransactionInfo } from "models/tauriModel";
import {
CallReceived as IncomingIcon,
MoreVert as MoreVertIcon,
@@ -22,6 +22,8 @@ import { isTestnet } from "store/config";
import { open } from "@tauri-apps/plugin-shell";
import dayjs from "dayjs";
import { useState } from "react";
+import { useAppSelector } from "store/hooks";
+import _ from "lodash";
interface TransactionItemProps {
transaction: TransactionInfo;
@@ -40,6 +42,21 @@ export default function TransactionItem({ transaction }: TransactionItemProps) {
const [menuAnchorEl, setMenuAnchorEl] = useState<null | HTMLElement>(null);
const menuOpen = Boolean(menuAnchorEl);
+ // Show the subaddress label if we have one, else the truncated address
+ const subaddresses: SubaddressSummary[] = useAppSelector((s) => s.wallet.state.subaddresses);
+ const subaddress: SubaddressSummary | undefined = subaddresses.find((s) => s.address === transaction.received_address);
+
+ let addressLabel: string | null = null;
+ if (isIncoming && transaction.received_address) {
+
+ if (subaddress) {
+ addressLabel = subaddress.label;
+ } else {
+ addressLabel = _.truncate(transaction.received_address, { length: 6 });
+ }
+ }
+
+
return (
<Box
sx={{
@@ -107,11 +124,11 @@ export default function TransactionItem({ transaction }: TransactionItemProps) {
<Typography variant="caption" sx={{ gridArea: "2 / 2" }}>
<FiatPiconeroAmount amount={transaction.amount} />
</Typography>
- {isIncoming && transaction.received_address && (
+ {isIncoming && transaction.received_address && subaddress && (
<Chip
size="small"
variant="outlined"
- label={transaction.received_address}
+ label={<>{`Address #${subaddress.address_index}`}<i>{`"${addressLabel}"`}</i></>}
sx={{ gridArea: "3 / 2", maxWidth: 360 }}
/>
)}
diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs
index 7225cf35..ef7da39c 100644
--- a/src-tauri/src/commands.rs
+++ b/src-tauri/src/commands.rs
@@ -7,16 +7,16 @@ use swap::cli::{
request::{
BalanceArgs, BuyXmrArgs, CancelAndRefundArgs, ChangeMoneroNodeArgs,
CheckElectrumNodeArgs, CheckElectrumNodeResponse, CheckMoneroNodeArgs,
- CheckMoneroNodeResponse, CheckSeedArgs, CheckSeedResponse, DfxAuthenticateResponse,
- ExportBitcoinWalletArgs, GetBitcoinAddressArgs, GetCurrentSwapArgs, GetDataDirArgs,
- GetHistoryArgs, GetLogsArgs, GetMoneroAddressesArgs, GetMoneroBalanceArgs,
- GetMoneroHistoryArgs, GetMoneroMainAddressArgs, GetMoneroSeedArgs,
- GetMoneroSubaddressesArgs, GetMoneroSyncProgressArgs, GetPendingApprovalsResponse,
- GetRestoreHeightArgs, GetSwapInfoArgs, GetSwapInfosAllArgs, ListSellersArgs,
- MoneroRecoveryArgs, RedactArgs, RejectApprovalArgs, RejectApprovalResponse,
- ResolveApprovalArgs, ResumeSwapArgs, SendMoneroArgs, SetMoneroSubaddressLabelArgs,
- SetMoneroWalletPasswordArgs, SetRestoreHeightArgs, SuspendCurrentSwapArgs,
- WithdrawBtcArgs,
+ CheckMoneroNodeResponse, CheckSeedArgs, CheckSeedResponse, CreateMoneroSubaddressArgs,
+ DfxAuthenticateResponse, ExportBitcoinWalletArgs, GetBitcoinAddressArgs,
+ GetCurrentSwapArgs, GetDataDirArgs, GetHistoryArgs, GetLogsArgs,
+ GetMoneroAddressesArgs, GetMoneroBalanceArgs, GetMoneroHistoryArgs,
+ GetMoneroMainAddressArgs, GetMoneroSeedArgs, GetMoneroSubaddressesArgs,
+ GetMoneroSyncProgressArgs, GetPendingApprovalsResponse, GetRestoreHeightArgs,
+ GetSwapInfoArgs, GetSwapInfosAllArgs, ListSellersArgs, MoneroRecoveryArgs, RedactArgs,
+ RejectApprovalArgs, RejectApprovalResponse, ResolveApprovalArgs, ResumeSwapArgs,
+ SendMoneroArgs, SetMoneroSubaddressLabelArgs, SetMoneroWalletPasswordArgs,
+ SetRestoreHeightArgs, SuspendCurrentSwapArgs, WithdrawBtcArgs,
},
tauri_bindings::{ContextStatus, TauriSettings},
ContextBuilder,
|
|
I apologize that this is taking so long to be merged... we were a bit overwhelmed with the p2p improvements and protocol changes. Should get better fairly soon. I think the UI is still a bit too cluttered. The add address button doesn't need a label field in my opinion and can be integrated into the table itself in some way. I don't think most users think about subaddresses in the way of "creating them". You usually expect them to just be present and to be able to give them labels. Similiar to how Feather Wallet does it. |
Compilation fails on my machine. @Einliterflasche does it compile for you? Not sure if this is an issue with my machine. |
|
Got it compiling. I can merge this once the conflicts are fixed! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let indices = | ||
| ffi::walletBalancePerSubaddrIndices(self.inner.pinned(), account_index, false); | ||
| let amounts = | ||
| ffi::walletBalancePerSubaddrAmounts(self.inner.pinned(), account_index, false); |
There was a problem hiding this comment.
Race condition between separate FFI calls for balance data
The code makes two sequential FFI calls to fetch subaddress balance data: walletBalancePerSubaddrIndices followed by walletBalancePerSubaddrAmounts. Each C++ function independently calls impl->balancePerSubaddress(), meaning the underlying wallet state is queried twice. If the wallet's background sync thread updates the balance cache between these calls, the returned indices and amounts could become misaligned, causing incorrect balance values to be associated with wrong subaddress indices. The same issue exists with walletUnlockedBalancePerSubaddrIndices and walletUnlockedBalancePerSubaddrAmounts. A single FFI function returning both indices and amounts from one balancePerSubaddress() call would prevent this inconsistency.
Additional Locations (2)
| received[address_index] = received[address_index].saturating_add(amount); | ||
| tx_count[address_index] = tx_count[address_index].saturating_add(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Transaction amount double-counted across multiple subaddress indices
When a single transaction involves multiple subaddress indices (as returned by transactionInfoSubaddrIndices), the code iterates over each index and adds the full transaction amount to each subaddress's received total. Since tx_info.amount() returns the total transaction amount (not the per-subaddress portion), this causes the same amount to be counted multiple times. For example, a 10 XMR transaction split across subaddresses 1 and 2 would incorrectly add 10 XMR to both, showing 20 XMR total received instead of 10 XMR. The tx_count field is similarly inflated by counting the same transaction once per subaddress index.
|
thanks for your work! @rafael-xmr, I'm very happy that we finally have this feature! |
| export default function SubaddressesModal({ open, onClose }: Props) { | ||
| const subaddresses = useMoneroSubaddresses(); | ||
| const [isCreating, setIsCreating] = useState(false); | ||
| const [labelEdits, setLabelEdits] = useState<Record<number, string>>({}); |
There was a problem hiding this comment.
Stale label edits override fresh backend data
The labelEdits state is never cleared after the modal closes. When the user reopens the modal, stale values in labelEdits take precedence over fresh backend data because labelDraft uses labelEdits[s.address_index] ?? subaddressesByIndex[s.address_index]?.label. If the backend label changed (via error recovery, another session, or any other means), the UI will display the outdated locally-cached edit instead of the current value. The state should be reset in handleClose or when the modal reopens.
Additional Locations (1)
3 participants
Note
Introduces complete subaddress functionality across the stack.
WalletImplto exposebalancePerSubaddress/unlockedBalancePerSubaddress; add bridge APIs for subaddress indices/amounts, labels, creation; newSubaddressSummarytype andreceived_addresson transactions.get_monero_subaddresses,create_monero_subaddress,set_monero_subaddress_label; emitMoneroWalletUpdate::SubaddressesUpdateand wire through RPC; harness adds helpers (address_at,create_subaddress, summaries, per-subaddress balances).SubaddressesModalto list/copy addresses, create, and label; show subaddress chip on incoming transactions; Redux state/actions/hooks forsubaddresses; load/update on startup and via background events; minor UI utilities (truncate,onComplete).subaddresses.rscovering creation, sending, and per-subaddress balance verification.Written by Cursor Bugbot for commit 37e73f5. This will update automatically on new commits. Configure here.