Skip to content

Conversation

@FredericHeem
Copy link

Add the function verifyBase64 to check signatures compatible with bitcoind and armory.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling d6fbd2a on FredericHeem:master into 77b68a5 on bitcoinjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling c150193 on FredericHeem:master into 77b68a5 on bitcoinjs:master.

@kyledrake
Copy link
Contributor

I've got some issues with this PR:

  • You need to use the existing test suite, and not a test suite built just for this test.
  • I don't understand the use of base64 here. Does armory and bitcoind not support hex?

@kyledrake
Copy link
Contributor

Also, you need to use two space soft tabs, per our code style convention.

@FredericHeem
Copy link
Author

The use of base64 comes from the fact that web browsers doesn't support Buffer out of the box, that's why some unit has been added to validate this function when run in a web browser.
Standard unit test has been added to keep the code coverage at the same level.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 58cec8f on FredericHeem:master into 77b68a5 on bitcoinjs:master.

@dcousens
Copy link
Contributor

Please only use the existing test suite. We use testling and browserify which allows you to test in the browser. You certainly don't need to include anything else (not jquery, and not the minified bitcoinjs). In short: git rm test/browser/* ; git commit --amend

As for the added functionality, I'm not opposed to it, it is only useful because Buffer is not readily exposed in the browser (though you can do this by using browserify).

@weilu thoughts?

@weilu
Copy link
Contributor

weilu commented Jun 18, 2014

Yea this PR reminded me of the same thing you mentioned: "Buffer is not readily exposed in the browser", unless one uses browserify. We could either do what this PR suggested, or we could accept string for verify and allow an optional encoding params which defaults to base64. Message.verify is rather an API exposed for app devs, instead of for internal consumption.

@dcousens
Copy link
Contributor

Or perhaps message.verify only accepts base64? What do most clients accept?
On Jun 18, 2014 2:39 PM, "Wei Lu" notifications@github.com wrote:

Yea this PR reminded me of the same thing you mentioned: "Buffer is not
readily exposed in the browser", unless one uses browserify. We could
either do what this PR suggested, or we could accept string for verify
and allow an optional encoding params which defaults to base64.
Message.verify is rather an API exposed for app devs, instead of for
internal consumption.


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

@weilu
Copy link
Contributor

weilu commented Jun 19, 2014

base64 for bitcoin qt (verifymessage <bitcoinaddress> <signature> <message>), bitcoinj does more or less the same: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/com/google/bitcoin/core/ECKey.java#L800-L804

Perhaps only accepting base64 is a good idea

@kyledrake
Copy link
Contributor

Everyone using base64 instead of hex is quite a WTF here, but if it's what everyone's doing, so be it.

@FredericHeem Thank you very much for bringing this to our attention! We will make sure you can use this shortly. Unfortunately we cannot pull your request due to the testing, but I will make a new PR for this and be sure to give you credit for the proposal and fix.

@FredericHeem
Copy link
Author

Adding browser based testing is useful and shouldn't be discarded.
In the context of the "proof of asset" implemented by aproof (https://github.com/olalonde/proof-of-assets), this library depends on bitcoinjs-lib for verifying signatures and provides the verifySignatures function to encapsulate this operation. Unfortunately, verifying signatures with aproof works only under Chrome and not under Firefox and Safari. aproof is just delegating the signature verification to bitcoinjs-lib. This issue is yet to be solved. aproof is browserifed and embeds bitcoinjs-lib which is also browserifed, could it be the source of the issue ?
That was the main reason to add a simple pure browser based testing without the need to bother with browserify.
We add use bower to automatically download jquery and avoid committing javascript dependencies to the git repo.
We could also use mocha for proper brower based testing.

@kyledrake
Copy link
Contributor

We have browser based testing, most of our mocha tests run in the browser. You can very easily add a test there, and it will be run in the browser.

@dcousens
Copy link
Contributor

@FredericHeem the version of Bitcoin js you are using in that GitHub repository is pre 0.2.0.

I'm actually thinking a base64 only verification is bad form, if anything it should be base58 check, assuming these are indeed user land signatures.

We have a bit of leeway in changing the status quo here, and base64 isn't the ideal.

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.

5 participants