Unit test that sign_for returns a correct hash (RIPD-1583) #2333
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20180115 - 17:40:32 Test Results
|
| @@ -1134,6 +1134,80 @@ class MultiSign_test : public beast::unit_test::suite | |||
| env.close(); | |||
| } | |||
|
|
|||
| void test_signForHash() | |||
| { | |||
| // Make sure that the "hash" field returned by the "sign_for" RPC | |||
There was a problem hiding this comment.
Only the last sign_for before submitting returns the "right" hash huh? In general, each signer doesn't know the hash to look out for though right?
There was a problem hiding this comment.
Correct. Each added signature changes the hash. But if the transaction with those signatures is submitted, either as a blob through submit or as text through submit_multisigned, then the hash does not change.
As a use model, it is probably smartest to rely on the hash returned when the transaction is submitted. But since we are returning a hash value on signing, the value we return should be correct.
|
You could argue that it might just confuse people having it there, given
you can only rely on the submit* hash.
Might even make sense to remove it?
|
| // Add the next signature and sign again. | ||
| jvSig2[jss::result][jss::account] = ghost.human(); | ||
| jvSig2[jss::result][jss::secret] = ghost.name(); | ||
| Json::Value jvSubmit = env.rpc ( |
There was a problem hiding this comment.
Nit: Json::Value jvSubmit might be better named Json::Value jvSig3.
There was a problem hiding this comment.
I actually used jvSig3 initially. I changed the name because the JSON in jvSubmit is what we'll submit to the network (yeah, it's a unit test, so it's a network of one) on line 1190. So that's my justification for the variable name...
| jvSig2[jss::result][jss::status].asString() == "success"); | ||
|
|
||
| // Save the hash with one signature for use later. | ||
| std::string const hash1 = |
There was a problem hiding this comment.
Is it worth checking any of the hash characteristics here?
There was a problem hiding this comment.
Possibly? To me the important characteristic of the hash is it that it allows me to look up the transaction in the ledger. So the various hash checks I'm doing throughout the test show three things:
- Adding the first signature provides us a hash. When we add the second signature, that also provides a hash. Those two hashes should be different. It would be easy to say, well of course they are different. But the ripple-online-tool had a bug where they were not different. So it's worth checking.
- If we submit the transaction with two signatures, the hash that comes back from that submission should be the same as the hash that we got back when we signed the second time.
- The hash returned when we submitted the transaction (which is the same as the hash returned when we signed the second time) should be able to locate the transaction in the ledger.
Are there other characteristics I should be testing for? Thanks.
scottschurr
added
the
Passed
|
In 0.90.0-b5 |
There was some discussion with one of our partners regarding the hash returned by
sign_forandsubmit_multisigned. When I did manual testing I managed to get myself confused. So I figured that adding an automated unit test that exercises the hash returned by these two RPC commands would be time well spent.