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

Add generated field to listunspent output #2522

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
4 participants
@bitcartel
Copy link
Contributor

commented Jul 11, 2017

Closes #2446

@bitcartel bitcartel requested review from daira and str4d Jul 11, 2017

@bitcartel bitcartel added the wallet label Jul 11, 2017

@bitcartel bitcartel added this to the 1.0.11 Release milestone Jul 11, 2017

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

Note: I thought #2446 was in the 1.0.11 milestone but it seems I was mistaken, although it's related to #2449 which is in the milestone. This PR can help third parties such as wallets and exchanges identify coinbase utxos which must be shielded.

@bitcartel bitcartel force-pushed the bitcartel:2446_coinbase_field_listunspent branch 2 times, most recently from 3348330 to c994309 Jul 11, 2017

@bitcartel bitcartel requested a review from ebfull Jul 11, 2017

@daira

daira approved these changes Jul 11, 2017

Copy link
Contributor

left a comment

ut(ACK+cov). What was the rationale for using "generated" for the field name as opposed to, say, "isCoinbase"?

for utxo in node0utxos:
if utxo["generated"] is True:
generatedCounter = generatedCounter + 1
assert(generatedCounter == 1)

This comment has been minimized.

Copy link
@daira

daira Jul 11, 2017

Contributor

You can write this as assert_equal(sum(int(uxto["generated"] is True) for uxto in node0utxos), 1), and similarly below.

This comment has been minimized.

Copy link
@daira

This comment has been minimized.

Copy link
@bitcartel

bitcartel Jul 11, 2017

Author Contributor

I would prefer to use something like coinbase (over isCoinbase which is camel case) but the ticket calls for generated and it's already being used in one location here https://github.com/zcash/zcash/blob/master/src/wallet/rpcwallet.cpp#L80 which is returned in two existing RPCs, listtransactions and gettransaction.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Jul 11, 2017

Author Contributor

Updated test to be more python-esque!

@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2017

I think it was an oversight that #2446 was not proposed for the milestone. In any case I've added it since the work on the PR and my review is a sunk cost now, and an exchange was requesting it.

Closes #2446 by adding generated field to listunspent.
If generated is true, the unspent transaction output is from a
coinbase transaction and can only be sent to a shielded address.

@bitcartel bitcartel force-pushed the bitcartel:2446_coinbase_field_listunspent branch from c994309 to d77a0ac Jul 11, 2017

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2017

⌛️ Trying commit d77a0ac with merge f54ea75...

zkbot added a commit that referenced this pull request Jul 11, 2017

Auto merge of #2522 - bitcartel:2446_coinbase_field_listunspent, r=<try>
Add generated field to listunspent output

Closes #2446
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2017

☀️ Test successful - pr-try
State: approved= try=True

@str4d

str4d approved these changes Jul 12, 2017

Copy link
Contributor

left a comment

ut(ACK+cov)

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

+1 on using "generated" in line with other usage in the transparent APIs.

@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

ut(ACK+cov)

@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2017

📌 Commit d77a0ac has been approved by daira

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2017

⌛️ Testing commit d77a0ac with merge 2c0fd9e...

zkbot added a commit that referenced this pull request Jul 13, 2017

Auto merge of #2522 - bitcartel:2446_coinbase_field_listunspent, r=daira
Add generated field to listunspent output

Closes #2446
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2017

☀️ Test successful - pr-merge
Approved by: daira
Pushing 2c0fd9e to master...

@zkbot zkbot merged commit d77a0ac into zcash:master Jul 13, 2017

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.