Subaddresses with multiple accounts support (preliminary) #793

kenshi84 · 2017-07-11T10:00:36Z

snaggen · 2017-07-11T12:15:01Z

Why is subadresses treated as separate accounts? Isn't it just different addresses to the same wallet? I would expect to have the wallet show the total amount, then maybe on the "account" page you could see the separate subtotals. On the recieve page I would suggest a drop down defaulting to a "main" adress (or maybe the last used). With the current version, the subadresses gets way to much focus.

knaccc · 2017-07-11T15:14:39Z

First, incredible work @kenshi84, thanks for doing this!!!

In my opinion, there is a critical problem with the visual hierarchy. By having the account selection alongside address selection, there is a disconnect between the logical hierarchy of accounts vs addresses and the visual precedence they're given in the GUI.

It needs to be obvious to a new user, without having to read a manual, that a wallet contains accounts, and that the list of addresses in the 'Receive' screen applies only to the currently selected account. At the moment, the relationship between accounts and addresses can be too easily confused because they appear alongside each other with the same precedence as each other.

I've put together a mockup of two screens which I think make things clearer:

Home page / account selection screen: http://imgur.com/a/sGNcc

Once account is selected: http://imgur.com/a/1gcr0

Note that in the second image, there is an area across the top of the screen displaying the name and balance of the currently selected account. This area is intended to be persistently displayed, no matter which account function (send/receive/history/etc) is currently selected. Also note that the text "Wallet #0" has been changed to "Account #0", to prevent confusing the user. I think it also makes sense for the CLI to say things like "[account/1 Bd2qqa]" instead of "[wallet/1 Bd2qqa]".

Another advantage of this new persistently displayed top area is that it makes space for the possibility of more status information to be displayed in the future. For example, in the future the wallet may allow transactions to be queued up, so that a user does not have to wait for change to become unlocked prior to instructing the wallet to queue up subsequent transactions. This top area would be a great place to communicate how many transactions are queued up and what the approximate time to completion will be.

In the Receive screen, it's probably a good idea to have an advanced section that hides the payment id stuff and everything below it (similar to how advanced options are hidden in the Send screen). It would overwhelm a user that just wants to know their wallet address. They are bombarded with all those extra options which they'll probably never use.

It would be very useful if under the list of addresses there was a short message saying something like: "Tip: Create an address for each different person that needs to send to you. This lets you distinguish between senders.". To make it easier to distinguish between senders without having to cross reference between screens, it'd be great if the History could display not just the index but also the label of the subaddress that receives funds.

I think it will confuse people that the first address in the list of receiving addresses is labelled "#0 Family account". This is confusing because the user is looking at an address, but it's labelled as an account (even if the word "account" is not part of the label). I think it would greatly reduce confusion if it was labelled as something like "Primary address" instead of sharing the label assigned to the account that contains it. This will make absolutely clear to them that they're looking at a list of addresses, not a list of accounts.

I was wondering what the default account label will be for the first account that appears in a new wallet? "Primary account" perhaps?

For the 'transfer' button, I'm imagining a simple dialog appearing that allows a destination account to be chosen from a dropdown list, and the only other option that would appear is a dropdown for the transaction priority fee multiplier. It feels too heavyweight to push users through the 'Send' screen just to move funds between accounts. Another reason for having a different 'transfer' dialog is that in the future, I'm anticipating extra options that appear that are specific to transfers and which do not apply to normal payments to third parties. An example of this is churn, which only applies to transfers between your own accounts, and does not apply when sending funds to other people.

Actions performed by other buttons:
'Switch account': Display the Accounts tab as in screenshot 1
'Select': Go to the Receive tab for the selected account
'Rename': Display a dialog to change the label on the account
'Send payment': Go to the Send tab for the selected account

snaggen · 2017-07-11T15:21:28Z

If subadresses are treated as separate accounts, then what is the benefit from just using multiple wallets? Den tis 11 juli 2017 18:14knaccc <notifications@github.com> skrev:

…

First, incredible work @kenshi84 <https://github.com/kenshi84> !!! From a UX perspective, I think there will be huge confusion as to what is going on, because it is not obvious from the UI that there are accounts, each with multiple receiving addresses. In my opinion, there is a critical problem with the visual hierarchy. By having the account selection alongside operations which apply only to specific accounts, there is a disconnect between the logical hierarchy of operations and the visual hierarchy displayed in the GUI. It needs to be obvious to a new user, without having to read a manual, that a Wallet contains Accounts, and each Account has multiple receiving addresses. I've put together a mockup of two screens which I think make things clearer: http://imgur.com/a/sGNcc (home page / account selection screen) http://imgur.com/a/4fHXq (once account is selected) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#793 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGH4n3oaf9hDLTKrXwjJCE7L3C1FyS8wks5sM5FggaJpZM4OUALq> .

knaccc · 2017-07-11T15:58:18Z

@snaggen Multiple wallets result in multiplying the blockchain scanning time. If you had 5 independent wallets, it'd take you 5x as long to scan the blockchain. Blockchain scanning is very computationally heavy (it's not just a bandwidth issue).

In contrast, you can have as many accounts as you want, and the blockchain will be scanned just as quickly as if there were only one account.

Note that a wallet has multiple accounts, and each account can have multiple receiving addresses (a.k.a. subaddresses) if you want them to.

It sounds like your personal preference would be to just have one account. This is what most people will probably do. You can then create multiple subaddress for your account if you wish. The subaddresses you create won't appear in the list of accounts, because they're not accounts. They're just alternative receiving addresses for your one account.

snaggen · 2017-07-11T18:31:09Z

I'm getting confused...
You write that a wallet may have multiple accounts, and each account can have multiple subadresses... Please explain what you then mean with account, since I got the impression from the UI that subadresses are used as accounts.

And as you write, and also as what is described in the Brief section of the subadresses mrl issue monero-project/research-lab#7, one of the big reasons for subadresses is to be able to handout adresses to multiple places without them being linked, and also to use as a mechanism to know from where the money were sent. And I can see myself use subadresses for those two reasons, but I still don't see it as having different accounts. In my mind they are just different addresses to the same wallet. Then in the wallet it would be nice to have a section where I can see the wallets "sub-accounts" to keep track of each subadress and how much is recieved by that adress.
So in my mind the wallet is the primary entity and the subadresses are a secondary entity. In the first UI it seems that each subadress is a primary entity, and the wallet total balance is only shown in the account tab.

snaggen · 2017-07-11T18:36:13Z

Also, if I come off as short and snarky it's just because my computer died and I'm currently doomed to use a phone to type all my comments. So, please give me some slack and read my comments as friendly and happy 😊. I'm really impressed by this work.

knaccc · 2017-07-11T18:48:56Z

@snaggen You're looking at an old document, the concept of accounts each with multiple receiving addresses is discussed here: monero-project/monero#2056

Think of accounts as a way to organize your affairs. You can create an account for your main personal spending, an account that keeps track of the money you're making by buying and selling on online auctions, an account that you use to accept donations for an open source project, and an account that you're using for a side business you've started with your spouse.

Each account has a balance, and a primary subaddress. For each account, you can create further subaddresses.

An account is therefore just a UX concept, and is simply a grouping of subaddresses.

Most people will probably just have one account, and will create a different address for each person they need to ask for payment from.

snaggen · 2017-07-11T19:01:12Z

Thanks for pointing me to the right document, now it makes more sense. Sorry for the noice.

kenshi84 · 2017-07-22T13:52:43Z

@knaccc Thanks a lot for your ideas. I wish I had enough Qt programming skills, but all I could do was to read existing code, copy it and modify it so that I can provide a bare minimum proof of concept that demonstrates how to use the feature.

@Jaqueeee The current version is far from being polished with at least two issues that I'm aware of:

In the Receive tab, the original address text line is still there which is redundant because there is a list of subaddresses below it. I did so because the "copy to clipboard" button didn't seem working on my macOS.
Also in the Receive tab, the QR code image gets chopped off at the bottom which cannot be brought back even with scrolling.

I really appreciate if you could step in to fix these problems and incorporate @knaccc's suggestion, and also make sure that the layout doesn't get broken under mobile or other environments.

Jaqueeee · 2017-07-22T14:09:12Z

@kenshi Great work. No problem, I can do some polishing on this later. Haven't had time to test it yet.

ghost · 2017-08-08T17:20:16Z

this looks amazing, the functionality that was missing to make me use the GUI

Jaqueeee · 2017-10-15T17:58:30Z

@kenshi84 @luigi1111 @fluffypony
Since lots of wallet api changes was made in #2056 this PR is required for GUI to build/run. I think we should merge this ASAP and fix the graphical/ux glitches later on.

Jaqueeee · 2017-10-15T18:43:57Z

@kenshi84 I created a bunch of accounts and addresses. When i restarted the gui and reopened the wallet they were gone.

kenshi84 · 2017-10-16T06:06:52Z

@Jaqueeee I did some research, and it looks like the problem is because of the wallet being unable to store when closing. I observe the same issue with the address book (i.e. any changes won't get saved to the file).

Perhaps this has something to do with one of the recently merged commits that changed the wallet behavior when closing? Paging @moneromooo-monero

Jaqueeee · 2017-10-16T07:58:21Z

@kenshi84 ok. That makes sense. Could you rebase this PR against master? There was a crash on exit bug that was fixed after you created this PR. Might be it.

kenshi84 · 2017-10-16T09:00:39Z

@Jaqueeee Oh, sorry I forgot to rebase. Done

kenshi84 · 2017-10-16T09:11:38Z

@Jaqueeee
I'm confused about how I can build libwallet_merged.a from the master branch; if I simply do ./build.sh, it builds from the release-v0.11 branch.

Jaqueeee · 2017-10-16T16:27:32Z

@kenshi84 thanks for the reminder. see #903

kenshi84 · 2017-10-17T01:29:44Z

@Jaqueeee

@kenshi84 ok. That makes sense. Could you rebase this PR against master? There was a crash on exit bug that was fixed after you created this PR. Might be it.

I confirmed that the wallet can't save its state when exiting, which prevents the address book from being saved. I'm not sure how to fix it. CC @moneromooo-monero

moneromooo-monero · 2017-10-23T21:14:57Z

Sorry, I don't usually follow this repo so didn't see I was being asked something. In the wallet2 stuff, there's nohting that I know of which would prevent saving the address book, it's serialized in the cache with the rest of the data AFAICT.

kenshi84 · 2017-10-24T00:28:44Z

I think #911 solves this store-on-exit issue which was introduced in monero-project/monero#2289.

moneromooo-monero · 2018-03-07T08:55:58Z

components/InputDialog.qml

@@ -0,0 +1,181 @@
+// Copyright (c) 2014-2015, The Monero Project


moneromooo-monero · 2018-03-07T09:00:47Z

pages/Account.qml

+            ListElement { address: "BYqbdA"; label: ""; balance: "0"; unlockedBalance: "0" }
+            ListElement { address: "BYqbdA"; label: ""; balance: "0"; unlockedBalance: "0" }
+            ListElement { address: "BYqbdA"; label: ""; balance: "0"; unlockedBalance: "0" }
+        }


Is this meant to be still in ?

moneromooo-monero · 2018-03-07T09:01:43Z

pages/Receive.qml

+                ListElement { address: "BcxioZpKqYaZsSDHP6pmesaV7gMrPYxvHXsnkG5ceVGkasPR83cEJyvGNpckYvs2wP76HsM1KGo1mbEmmiburnCUT4aSEv1"; label: "ShapeShift purchase on 2017-Jul-13" }
+                ListElement { address: "BZzdxkqM8fJhZLATeyD3bd2cSrehGsANH4CgyJ4DBUamJAdyRTqkZ1W3kvN1UpzLz1iQU8XfEQUi15QyobFyMQVb3LDj4gZ"; label: "ShapeShift purchase on 2017-Jul-17" }
+                ListElement { address: "BYNUQQfEtg2gshraM8RPGvPUNbWcHHu33Tzhxy8ZrL7r7MYDkdPUCDFYxNawBNSAdjhKUCsiWjg5tQoReRMKGQ1fJS2QuEB"; label: "ShapeShift purchase on 2017-Jul-21" }
+            }


Is this supposed to be still in ?

moneromooo-monero · 2018-03-07T09:06:05Z

src/libwalletqt/PendingTransaction.cpp

+        for (uint32_t i : x)
+            result.push_back(i);
+    return result;
+}


This looks like you'll get duplicates when having several txes. Maybe removing any would be better.

moneromooo-monero · 2018-03-07T09:07:51Z

src/libwalletqt/Subaddress.h

+    Q_INVOKABLE QList<Monero::SubaddressRow*> getAll(bool update = false) const;
+    Q_INVOKABLE Monero::SubaddressRow * getRow(int index) const;
+    Q_INVOKABLE void addRow(quint32 accountIndex, const QString &label) const;
+    Q_INVOKABLE void setLabel(quint32 accountIndex, quint32 addressIndex, const QString &label) const;


setters should not be const (might explain the mutable below)

moneromooo-monero · 2018-03-07T09:10:28Z

src/libwalletqt/Wallet.cpp

@@ -458,6 +516,34 @@ AddressBookModel *Wallet::addressBookModel() const
    return m_addressBookModel;
 }

+Subaddress *Wallet::subaddress() const


const methods should return const pointers to their data.

kayront · 2019-01-15T11:16:24Z

See bottom/final part of #1864 for my comments on why this is a very worthy addition.

In my opinion it would make sense to prioritize this feature - it enables more advanced use cases and because of the base layer privacy, it functions without damaging privacy for the end-user.

sanderfoobar · 2019-02-01T09:02:02Z

Implemented via #1905

+resolved

kenshi84 mentioned this pull request Jul 11, 2017

Subaddresses monero-project/monero#2056

Merged

kenshi84 force-pushed the subaddress branch 3 times, most recently from fa0a42a to afd9af6 Compare July 17, 2017 06:53

kenshi84 force-pushed the subaddress branch from afd9af6 to 2e61e2b Compare July 19, 2017 06:23

kenshi84 changed the title ~~Subaddresses (requires #2056)~~ [WIP] Subaddresses (requires #2056) Jul 22, 2017

kenshi84 force-pushed the subaddress branch from 2e61e2b to 9d4499c Compare July 31, 2017 09:21

kenshi84 force-pushed the subaddress branch from 9d4499c to 806fb42 Compare August 24, 2017 22:46

kenshi84 changed the title ~~[WIP] Subaddresses (requires #2056)~~ [WIP] Subaddresses Oct 16, 2017

kenshi84 force-pushed the subaddress branch from 806fb42 to 6d8ade9 Compare October 16, 2017 08:58

kenshi84 force-pushed the subaddress branch from 6d8ade9 to 21b0a2d Compare October 16, 2017 09:07

Jaqueeee mentioned this pull request Oct 22, 2017

subaddresses backwards compatibility monero-project/monero#2703

Merged

kenshi84 mentioned this pull request Oct 31, 2017

Deal with API changes due to subaddresses, with no visible change #908

Closed

kenshi84 force-pushed the subaddress branch from 21b0a2d to dad4b64 Compare November 22, 2017 00:59

kenshi84 changed the title ~~[WIP] Subaddresses~~ Subaddresses (preliminary) Nov 22, 2017

kenshi84 mentioned this pull request Nov 29, 2017

Subaddresses minimal version: supports default account only #977

Merged

kenshi84 force-pushed the subaddress branch 2 times, most recently from 0ec51a3 to bfda009 Compare November 30, 2017 00:35

kenshi84 changed the title ~~Subaddresses (preliminary)~~ Subaddresses with multiple accounts support (preliminary) Nov 30, 2017

kenshi84 force-pushed the subaddress branch 3 times, most recently from b793d9a to 5a3db3c Compare December 18, 2017 01:40

Subaddresses with multiple accounts support (preliminary)

84847e7

kenshi84 force-pushed the subaddress branch from 5a3db3c to 84847e7 Compare December 19, 2017 23:01

moneromooo-monero reviewed Mar 7, 2018

View reviewed changes

selsta mentioned this pull request Jan 15, 2019

add account support #1905

Merged

issue-helper added the resolved label Feb 1, 2019

issue-helper closed this Feb 1, 2019

knaccc mentioned this pull request Mar 18, 2019

GUI Re-design improvements. #2024

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subaddresses with multiple accounts support (preliminary) #793

Subaddresses with multiple accounts support (preliminary) #793

kenshi84 commented Jul 11, 2017

snaggen commented Jul 11, 2017

knaccc commented Jul 11, 2017 •

edited

Loading

snaggen commented Jul 11, 2017 via email

knaccc commented Jul 11, 2017 •

edited

Loading

snaggen commented Jul 11, 2017

snaggen commented Jul 11, 2017

knaccc commented Jul 11, 2017 •

edited

Loading

snaggen commented Jul 11, 2017

kenshi84 commented Jul 22, 2017 •

edited

Loading

Jaqueeee commented Jul 22, 2017

ghost commented Aug 8, 2017

Jaqueeee commented Oct 15, 2017

Jaqueeee commented Oct 15, 2017

kenshi84 commented Oct 16, 2017

Jaqueeee commented Oct 16, 2017

kenshi84 commented Oct 16, 2017

kenshi84 commented Oct 16, 2017

Jaqueeee commented Oct 16, 2017

kenshi84 commented Oct 17, 2017

moneromooo-monero commented Oct 23, 2017

kenshi84 commented Oct 24, 2017

moneromooo-monero Mar 7, 2018

moneromooo-monero Mar 7, 2018

moneromooo-monero Mar 7, 2018

moneromooo-monero Mar 7, 2018

moneromooo-monero Mar 7, 2018

moneromooo-monero Mar 7, 2018

kayront commented Jan 15, 2019

sanderfoobar commented Feb 1, 2019

		@@ -0,0 +1,181 @@
		// Copyright (c) 2014-2015, The Monero Project

Subaddresses with multiple accounts support (preliminary) #793

Subaddresses with multiple accounts support (preliminary) #793

Conversation

kenshi84 commented Jul 11, 2017

snaggen commented Jul 11, 2017

knaccc commented Jul 11, 2017 • edited Loading

snaggen commented Jul 11, 2017 via email

knaccc commented Jul 11, 2017 • edited Loading

snaggen commented Jul 11, 2017

snaggen commented Jul 11, 2017

knaccc commented Jul 11, 2017 • edited Loading

snaggen commented Jul 11, 2017

kenshi84 commented Jul 22, 2017 • edited Loading

Jaqueeee commented Jul 22, 2017

ghost commented Aug 8, 2017

Jaqueeee commented Oct 15, 2017

Jaqueeee commented Oct 15, 2017

kenshi84 commented Oct 16, 2017

Jaqueeee commented Oct 16, 2017

kenshi84 commented Oct 16, 2017

kenshi84 commented Oct 16, 2017

Jaqueeee commented Oct 16, 2017

kenshi84 commented Oct 17, 2017

moneromooo-monero commented Oct 23, 2017

kenshi84 commented Oct 24, 2017

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

moneromooo-monero Mar 7, 2018

Choose a reason for hiding this comment

kayront commented Jan 15, 2019

sanderfoobar commented Feb 1, 2019

knaccc commented Jul 11, 2017 •

edited

Loading

knaccc commented Jul 11, 2017 •

edited

Loading

knaccc commented Jul 11, 2017 •

edited

Loading

kenshi84 commented Jul 22, 2017 •

edited

Loading