feat: show list of subaddresses #685
Conversation
|
very cool! I'll review this ASAP |
|
Compilations fails with this error on my machine: |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
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.
… to rpc.ts, use PromiseInvokeButton
Einliterflasche
left a comment
Einliterflasche
left a comment
There was a problem hiding this comment.
some comments I forgot to submit
|
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. |
3 participants
Note
Add Monero subaddress listing (with received/tx counts), creation, and labeling, wired through FFI, Tauri commands/events, Redux, and a new UI modal/button.
monero-sys(numSubaddresses,getSubaddressLabel,addSubaddress,setSubaddressLabel).SubaddressSummaryand wallet methods to compute summaries per account (address, label, received, tx_count) and create/update subaddresses.get_monero_subaddresses,create_monero_subaddress,set_monero_subaddress_label(wired insrc-tauri/src/commands.rs).MoneroWalletUpdate::SubaddressesUpdatewith current summaries.SubaddressesModalto list subaddresses, copy addresses, show received/tx count, create new subaddress, and edit labels.WalletActionButtons.subaddressesto wallet state andsetSubaddressesaction; handleSubaddressesUpdateinbackground.ts.renderer/rpc.ts.Written by Cursor Bugbot for commit d893570. This will update automatically on new commits. Configure here.