Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Qt][WIP] allow possibility to add a comment to a WalletTx #5905

Closed

Conversation

jonasschnelli
Copy link
Contributor

Try to sync rpc/qt wallets implementations.
Solves #2086.

bildschirmfoto 2015-03-15 um 16 40 49

bildschirmfoto 2015-03-15 um 16 40 59

    {
        "account" : "",
        "address" : "mm2STH7X3rbBVd73XaDrwdJHM69ukj31N8",
        "category" : "send",
        "amount" : -1.00000000,
        "vout" : 1,
        "fee" : -0.00000521,
        "confirmations" : 0,
        "txid" : "151e1e32090b765ebc30ebcc385bd79978081130b331972b94e142f3aeb85e10",
        "walletconflicts" : [
        ],
        "time" : 1426434053,
        "timereceived" : 1426434053,
        "comment" : "This is a test wtx comment."
    }

@sipa sipa added the GUI label Mar 16, 2015
@@ -65,3 +65,8 @@ CReserveKey *WalletModelTransaction::getPossibleKeyChange()
{
return keyChange;
}

void WalletModelTransaction::setComment(QString comment)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a const reference?

@laanwj
Copy link
Member

laanwj commented Mar 16, 2015

Agree on the idea. Maybe we can make the comment editable too in the details view? As the information is purely local, there is no reason why it shouldn't be possible to add comments in retrospect.

@jonasschnelli
Copy link
Contributor Author

@laanwj Yes. This is a good idea. Let me try to extend the editable comment to the transaction details window. What about adding a possibility to change the comment over RPC (without adding a new rpc call)?

@luke-jr
Copy link
Member

luke-jr commented Mar 16, 2015

How is this any different from the Label? Seems redundant.

@jonasschnelli
Copy link
Contributor Author

@luke-jr comments already exists in RPC world and are per walletTx.
Labels are per address as far as I know.

@jonasschnelli
Copy link
Contributor Author

@laanwj making the local comment editable within the TransactionDetailDialog looks after a big diff. It needs changes here and there and may be hard to review. It would also require changes within CWallet. IMO wtx are currently meant to be immutable. I would like to keep the PR as it is and address this "extension" in another PR.

@jonasschnelli jonasschnelli force-pushed the 2015/03/qt_tx_comment branch from 7db577d to 6b1360d Compare March 18, 2015 13:09
@luke-jr
Copy link
Member

luke-jr commented Mar 18, 2015

@jonasschnelli Addresses are per walletTx generally as well (at least in proper use, which should be encouraged). RPC "comments" are basically just the deprecated accounting stuff which didn't have addresses associated.

@jonasschnelli
Copy link
Contributor Author

@luke-jr "comments" are not directly related with "accounts". By now they where not deprecated. sendtoaddress uses them and i think having a possibility to give a transaction a comment makes sense.

@luke-jr
Copy link
Member

luke-jr commented Mar 18, 2015

Hm, interesting. Surprised that isn't using the label stuff. It would make sense to deprecate labels for comments, in that case, except that it's impossible to assign a comment to an incoming request right now. Labels work better in that regard. But having both is just confusing since they serve the same purpose...

@laanwj
Copy link
Member

laanwj commented Mar 20, 2015

Looks like a lot of people are making the label-comment confusion, even in #2086.

Personally I'd say the distinction is the following:

  • label is a, usually short, name for the recipient or address
  • comment is arbitrary free text associated the the transaction. This can be anything from a description to e.g. current exchange rates for accounting/tax purposes

The annoying/user-surprising thing about these comments, though, is that they apply to a network-level transaction, so they would seem to be associated with multiple UI transactions if there are multiple outputs.

@luke-jr
Copy link
Member

luke-jr commented Mar 20, 2015

It also means you can't add comments to incoming transactions until after they're received (and confirm?). Maybe comment should be a text-area in the UI rather than a single line, and in the new wallet associated with an address so it has the same flexibility?

@laanwj
Copy link
Member

laanwj commented Mar 20, 2015

Maybe comment should be a text-area in the UI rather than a single line

+1 I somehow assumed that'd already be the case.

@jonasschnelli
Copy link
Contributor Author

Maybe comment should be a text-area in the UI rather than a single line

This PR will add a multiline text-area for adding comments to outgoing transactions.

[...] and in the new wallet associated with an address so it has the same flexibility?

I think adding comments to a wtx is something different to a address label. Lets say if i do ONE wtx to serval recipients it is nice then to give that ONE wtx a comment and not every receiving address.

But i'm also not completely sure what would be best for handling labels and comments.

@@ -65,3 +65,8 @@ CReserveKey *WalletModelTransaction::getPossibleKeyChange()
{
return keyChange;
}

void WalletModelTransaction::setComment(const QString comment)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use a pass-by-reference here ;)?
const QString& comment

@jonasschnelli jonasschnelli force-pushed the 2015/03/qt_tx_comment branch from 6b1360d to 16c0fc4 Compare April 2, 2015 20:13
@jonasschnelli
Copy link
Contributor Author

rebased and fixed @Diapolo 's nit

@jgarzik
Copy link
Contributor

jgarzik commented Sep 15, 2015

ut ACK

@jonasschnelli
Copy link
Contributor Author

Rebased.

It will slightly reduce the space in the already very packed sendcoinsdialog.
But i think it's useful.
I'm not sure if – long term speaking – the Addressbook has a reason to exists. The word "Adressbook" itself is connected to address reuse. If people using the Addressbook to label and track payments, then we should slowly remove (or rename and rewrite) the Addressbook and switch over to wallet comments.

@maflcko
Copy link
Member

maflcko commented Sep 16, 2015

Looks good to merge.

Screenshot:
screenshot from 2015-09-16 12-10-39

@gmaxwell
Copy link
Contributor

utACK. But can we also add an RPC to set these?

@jonasschnelli
Copy link
Contributor Author

@gmaxwell: Thanks for the review. Some test would be required, I agree. I'm also not fully happy with the UI look and feel. Consider the PR as WIP (changed title).

@jonasschnelli jonasschnelli changed the title [Qt] allow possibility to add a comment to a WalletTx [Qt][WIP] allow possibility to add a comment to a WalletTx Nov 24, 2015
@pstratem
Copy link
Contributor

pstratem commented Dec 8, 2015

lightly tested ACK 1dcdd42

@sipa
Copy link
Member

sipa commented May 20, 2016

Concept ACK

@paveljanik
Copy link
Contributor

Concept ACK

RPC first please.

@jonasschnelli
Copy link
Contributor Author

@paveljanik: It's already possible in RPC sendtoaddress \"bitcoinaddress\" amount ( \"comment\" \"comment-to\" subtractfeefromamount )

I don't think the current GUI UX makes sense...
Closing for now and maybe reopening after #7729

@paveljanik
Copy link
Contributor

paveljanik commented Oct 18, 2016

@jonasschnelli Hmm, it is not possible to use sendtoaddress RPC command to add comment to already received transaction from someone else.

@jonasschnelli
Copy link
Contributor Author

@paveljanik: this PR only allows to set the comment during GUI tx creation (which is also a point that makes this PR currently not ready).

@paveljanik
Copy link
Contributor

Ah, then yes :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants