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

Show all JoinSplit components in getrawtransaction and decoderawtransaction #2054

Merged

Conversation

Projects
None yet
4 participants
@str4d
Copy link
Contributor

commented Jan 31, 2017

Closes #2030.

@str4d str4d added this to the 1.0.6 milestone Jan 31, 2017

@@ -88,8 +94,17 @@ Array TxJoinSplitToJSON(const CTransaction& tx) {
joinsplit.push_back(Pair("macs", macs));
}

joinsplit.push_back(Pair("vpub_old", ValueFromAmount(jsdescription.vpub_old)));
joinsplit.push_back(Pair("vpub_new", ValueFromAmount(jsdescription.vpub_new)));
CDataStream ssProof(SER_NETWORK, PROTOCOL_VERSION);

This comment has been minimized.

Copy link
@str4d

str4d Jan 31, 2017

Author Contributor

We need to check that this renders data in the expected order. The numbers and uint256 fields in this method are reversed from what appears in the hex of a block, but the non-uint256 fields are not reversed. I believe this is consistent with Bitcoin because the non-uint256 fields are vectors or arrays in the backend, but this was the root of my original uncertainty that led to me not including these at launch.

This comment has been minimized.

Copy link
@daira

daira Feb 1, 2017

Contributor

I'm happy with this order as long as it is documented in the RPC interface doc.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

Needs tests and documentation for payment-api.md.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

A side-effect of this PR is that gettransaction now also shows all this data, because of #1493. I don't think this is the right place for that, so I'm inclined to remove vjoinsplit from gettransaction entirely, since that RPC call is intended to show data from the wallet context, and users can get the exact same info from getrawtransaction. Instead, I think gettransaction should be extended to show private data we can access, probably at the same time as we do #1333 (since they already both show the same transparent details).

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

#1446 specifically wanted this in the output of gettransaction.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

Yes, but I think that ticket was solved incorrectly. I agree there should be data about JoinSplits in gettransaction - I just don't think it should be this data.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

I agree that at least the encrypted memo fields are probably not useful in the gettransaction output. However, I don't see any great harm in including them for now.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

ut(ACK+cov)

@bitcartel
Copy link
Contributor

left a comment

Update the test to assert for existence of new fields, oneTimePubKey, randomSeed, etc. even though we can't assert the value of those fields.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

Will ACK once test is updated per review comment. Regarding gettransaction we can update what joinsplit information is returned in a separate ticket+PR.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

Updated test.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

ACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

📌 Commit 91270dc has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

⌛️ Testing commit 91270dc with merge d32511c...

zkbot added a commit that referenced this pull request Feb 9, 2017

Auto merge of #2054 - str4d:2030-decoderawtransaction-joinsplit-field…
…s, r=bitcartel

Show all JoinSplit components in getrawtransaction and decoderawtransaction

Closes #2030.
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

post-ACK 91270dc

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 91270dc into zcash:master Feb 9, 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.