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

Expose GUI labels in RPC as comments #5574

Merged
merged 1 commit into from Nov 10, 2015
Merged

Expose GUI labels in RPC as comments #5574

merged 1 commit into from Nov 10, 2015

Conversation

@luke-jr
Copy link
Member

luke-jr commented Dec 30, 2014

No description provided.

@laanwj
Copy link
Member

laanwj commented Dec 31, 2014

I'm not opposed to adding this information, but won't this confuse GUI labels and RPC accounts further? Right now these collide, so people should either use the GUI with labels or RPC with (or without) accounts.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 31, 2014

If we're going to deprecate accounts, IMO we should add support for labels first. It doesn't increase the conflict, since the GUI already supports accounts via RPC and the debug window. This just makes it possible to access the GUI-only information from bitcoind as well.

@laanwj
Copy link
Member

laanwj commented Jan 29, 2015

I think then we should simply call it 'label', or 'address label'. Any API replacing the account system would use that for consistency with the GUI.

@laanwj
Copy link
Member

laanwj commented Jun 10, 2015

Needs rebase. And I still think the string should have the same name as in the GUI, e.g. 'label', introducing a new term 'comment' will just cause confusion.

@Diapolo
Copy link

Diapolo commented Jun 15, 2015

+1 for a consistent term between GUI and core.

@laanwj
Copy link
Member

laanwj commented Jul 21, 2015

Need rebase(and comment addressed, probably)

@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2015

ut ACK, once feedback is addressed

@jgarzik
Copy link
Contributor

jgarzik commented Sep 15, 2015

Ping @luke-jr

@dcousens
Copy link
Contributor

dcousens commented Sep 16, 2015

concept ACK, will review on rebase

@luke-jr luke-jr force-pushed the luke-jr:rpc_labels branch Oct 2, 2015
@luke-jr luke-jr force-pushed the luke-jr:rpc_labels branch to fd55571 Oct 2, 2015
@luke-jr
Copy link
Member Author

luke-jr commented Oct 2, 2015

Rebased and renamed to "label" (although I think we'll regret that).

@@ -1182,6 +1182,8 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
obj.push_back(Pair("account", strAccount));
obj.push_back(Pair("amount", ValueFromAmount(nAmount)));
obj.push_back(Pair("confirmations", (nConf == std::numeric_limits<int>::max() ? 0 : nConf)));
if (!fByAccounts)

This comment has been minimized.

Copy link
@dcousens

dcousens Oct 2, 2015

Contributor

What is fByAccounts representing here?

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 2, 2015

Member

listreceivedbyaccount

@dcousens
Copy link
Contributor

dcousens commented Oct 2, 2015

utACK

@@ -1235,7 +1237,8 @@ UniValue listreceivedbyaddress(const UniValue& params, bool fHelp)
" \"address\" : \"receivingaddress\", (string) The receiving address\n"
" \"account\" : \"accountname\", (string) DEPRECATED. The account of the receiving address. The default account is \"\".\n"
" \"amount\" : x.xxx, (numeric) The total amount in " + CURRENCY_UNIT + " received by the address\n"
" \"confirmations\" : n (numeric) The number of confirmations of the most recent transaction included\n"
" \"confirmations\" : n, (numeric) The number of confirmations of the most recent transaction included\n"
" \"label\" : \"label\" (string) A comment for the address/transaction, if any\n"

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Oct 2, 2015

Member

Hmm... i think we should not use "transaction" as entity where a label is stored.
What about just using (string) A comment for the address, if any\n"?

This comment has been minimized.

Copy link
@luke-jr

luke-jr Oct 2, 2015

Author Member

An address and a transaction are essentially the same thing.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Oct 5, 2015

Member

An address and a transaction are essentially the same thing.
^^
Not that I like address re-using: But there are use cases (and users-behaviors) where multiple transactions having outputs to the same address. How could you distinct between these transactions over labels/comments if we would say address==transaction?

We have the term "Address" book where every address has one label. Maybe in future, the Addressbook has identities which could create a payment address over BIP70 or BIP32.

For now i think there are reasons to label/comment an address as well as a transaction.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Oct 5, 2015

Author Member

There is no need to be distinct across an unsupported use-case. That would just encourage people to do it more often.

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 2, 2015

Tested ACK (fd55571) (mind the help text nit).

bildschirmfoto 2015-10-02 um 10 48 47

bildschirmfoto 2015-10-02 um 10 49 04

bildschirmfoto 2015-10-02 um 10 48 51

@laanwj laanwj merged commit fd55571 into bitcoin:master Nov 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Nov 10, 2015
fd55571 wallet: Expose GUI labels in RPC (Luke Dashjr)
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.