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

Replace json_spirit with UniValue #236

Merged
merged 9 commits into from
Nov 23, 2015
Merged

Replace json_spirit with UniValue #236

merged 9 commits into from
Nov 23, 2015

Conversation

l0rdicon
Copy link
Collaborator

ref: bitcoin/bitcoin#6121

Includes all the recent commits to UniValue up the bitcoin master tree as of 10/09/2015

@dooglus
Copy link
Collaborator

dooglus commented Oct 13, 2015

I can't create a raw transaction:

$ clamd createrawtransaction '[{"txid":"deadbeef","vout":1}]' '{}'
error code: -8
error message: Invalid parameter, missing vout key

Note how rpcmisc.cpp has:

    const UniValue& vout_v = find_value(o, "vout");
    if (!vout_v.isNum())

but rpcrawtransaction.cpp has:

    const UniValue& vout_v = find_value(o, "vout");
    if (vout_v.isNum())

I think they should probably both have the ! at the start of the 2nd line there.

@dooglus
Copy link
Collaborator

dooglus commented Oct 13, 2015

In signrawtransaction:

        if (p.isObject())
            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "expected object with {\"txid'\",\"vout\",\"scriptPubKey\"}");

That should be !p.isObject(), right?

@l0rdicon
Copy link
Collaborator Author

I updated l0rdicon@7bc97ed with the mistakes you picked out.

@dooglus
Copy link
Collaborator

dooglus commented Oct 13, 2015

signrawtransaction seems OK if I add the ! I mentioned in my previous comment:

$ cc signrawtransaction $(cc createrawtransaction '[{"txid":"1234","vout":4}]' '{"xJDCLAMZkcp7fQ3ieHfZA4SLu3aTy2Y1mr":1}') '[{"txid":"1234","vout":4,"scriptPubKey":"76a91491e596f8e4620be793964af8979e76073907c5b188ac"}]' '["'$p'","'$p'"]'
{
  "hex": "020...",
  "complete": true
}

@dooglus
Copy link
Collaborator

dooglus commented Oct 14, 2015

I just got 'bit' by this change, in testing.

I have a script which uses awk '{print $3}' to pull the results from an RPC command.

Previously the input to the awk looked like this:

$ cc getblock $(cc getblockhash 684635) | grep difficulty
    "difficulty" : 65093.99454380,

but with this pull request it has changed to look like this:

$ cc getblock $(cc getblockhash 684635) | grep difficulty
  "difficulty": 65093.99454380115,

Note there's no longer a space before the colon, and so the awk fails.

I don't know if anyone else is processing "clamd" output with shell scripts, but it would be better if this spirit -> univalue change was as transparent as possible for the end user.

@l0rdicon
Copy link
Collaborator Author

Absolutely. There shouldn't be any changes in the output in my opinion
either. I didn't catch that.

I'll look into this

On Tue, Oct 13, 2015 at 10:20 PM, Chris Moore notifications@github.com
wrote:

I just got 'bit' by this change, in testing.

I have a script which uses awk '{print $3}' to pull the results from an
RPC command.

Previously the input to the awk looked like this:

$ cc getblock $(cc getblockhash 684635) | grep difficulty
"difficulty" : 65093.99454380,

but with this pull request it has changed to look like this:

$ cc getblock $(cc getblockhash 684635) | grep difficulty
"difficulty": 65093.99454380115,

Note there's no longer a space before the colon, and so the awk fails.

I don't know if anyone else is processing "clamd" output with shell
scripts, but it would be better if this spirit -> univalue change was as
transparent as possible for the end user.


Reply to this email directly or view it on GitHub
#236 (comment)
.

@l0rdicon
Copy link
Collaborator Author

I updated the response to include the separator

@dooglus
Copy link
Collaborator

dooglus commented Oct 16, 2015

listunspent is showing stuff like:

{
  "txid": "10e6e13f0eb340d07217cfec2b3bce1c02efbfb5906d22bd9aa1f8aa85cf8c8d",
  "vout": 13,
  "address": "xVH85MSAaUppSN5sYWDvikvu8e4zkVkcwW",
  "account": "1172957",
  "scriptPubKey": "76a914e61155624669f5615a9d6b730b61d1278397cc9188ac",
  "amount": 1.934e-05,
  "confirmations": 53004
}, 

Note the amount is in scientific notation. I'm pretty sure it always used to be in decimal notation, even if it was for a single satoshi.

@l0rdicon
Copy link
Collaborator Author

I'm unsure why your listupsent doesn't have the spacing between the separator.. I'll need to look deeper into that if it persists.

The update to ValueToAmount should fix any issues with the numbers being reported wrong.

@dooglus
Copy link
Collaborator

dooglus commented Oct 16, 2015

Oh, that's because I haven't rebuilt since I first saw your pull request. I
was referring to the 'amount' values, not the spacing.

On Thu, Oct 15, 2015 at 11:26 PM, l0rdicon notifications@github.com wrote:

I'm unsure why your listupsent doesn't have the spacing between the
separator.. I'll need to look deeper into that if it persists.

The update to ValueToAmount should fix any issues with the numbers being
reported wrong.


Reply to this email directly or view it on GitHub
#236 (comment)
.

@l0rdicon
Copy link
Collaborator Author

Makes sense, I was wondering if that was the case about you not having
rebuilt.

That amount should be fixed with that most recent update, and the spacing
for your AWK print failure should be there as well

On Fri, Oct 16, 2015 at 4:46 AM, Chris Moore notifications@github.com
wrote:

Oh, that's because I haven't rebuilt since I first saw your pull request. I
was referring to the 'amount' values, not the spacing.

On Thu, Oct 15, 2015 at 11:26 PM, l0rdicon notifications@github.com
wrote:

I'm unsure why your listupsent doesn't have the spacing between the
separator.. I'll need to look deeper into that if it persists.

The update to ValueToAmount should fix any issues with the numbers being
reported wrong.


Reply to this email directly or view it on GitHub
<
https://github.com/nochowderforyou/clams/pull/236#issuecomment-148628139>

.


Reply to this email directly or view it on GitHub
#236 (comment)
.

@dooglus
Copy link
Collaborator

dooglus commented Oct 16, 2015

Looks good now I've rebuilt.

Before:

$ clamd listunspent | grep amount
    "amount": 2.079e-05,
    "amount": 1.934e-05,
    "amount": 2.662e-05,
    "amount": 0.00020465,
    "amount": 1.97e-05,
    "amount": 5.408e-05,
    "amount": 2.752e-05,
    "amount": 3.408e-05,
    "amount": 1.645e-05,
    "amount": 2.475e-05,
    "amount": 0.00021877,
    "amount": 0.0002062,

After:

$ clamd listunspent | grep amount
    "amount" : 0.00002079,
    "amount" : 0.00001934,
    "amount" : 0.00002662,
    "amount" : 0.00020465,
    "amount" : 0.00001970,
    "amount" : 0.00005408,
    "amount" : 0.00002752,
    "amount" : 0.00003408,
    "amount" : 0.00001645,
    "amount" : 0.00002475,
    "amount" : 0.00021877,
    "amount" : 0.00020620,

l0rdicon added a commit that referenced this pull request Nov 23, 2015
Replace json_spirit with UniValue
@l0rdicon l0rdicon merged commit 408227e into nochowderforyou:master Nov 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants