Add missing tests to ECSDA #1248

come-maiz · 2018-08-29T05:04:27Z

Requires #1243.
This is one step towards #1147

🚀 Description

Complete the test coverage for ECRecover:
cases when version is 27, 28 and invalid.

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

nventuro · 2018-08-29T09:37:45Z

Can you rebase from master, now that #1243 has been merged? The diff is hard to read otherwise.

come-maiz · 2018-08-29T15:01:59Z

@nventuro updated.

nventuro · 2018-08-29T16:23:44Z

test/library/ECRecovery.test.js

+    // The last two hex digits are the signature version.
+    // The only valid values are 0, 1, 27 and 28.
+    // eslint-disable-next-line max-len
+    const signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e002';


Why is this signature different from the others? Shouldn't just the version differ? If so, I'd abstract out the signature prefix, and add the suffix in each test.

But more importantly, why is the signature hardcoded and not generated in the test? :/

nventuro · 2018-08-29T16:27:07Z

test/library/ECRecovery.test.js

@@ -30,6 +30,33 @@ contract('ECRecovery', function ([_, anyone]) {
    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
  });

+  it('recover v27', async function () {


If possible, these may read more clearly with a structure like the following one:

context with valid signature context with v27 signature it works context with v28 signature it works context with invalid version signature it reverts context with invalid signature it reverts

(the test itself should probably be abstracted into a function to avoid repetition)

come-maiz · 2018-09-26T02:09:58Z

@nventuro please take another look.
I will leave removing the hardcoded signature as an exercise for a future reader, because I couldn't find how to generate the v1 signature.

frangio · 2018-09-26T16:00:40Z

test/library/ECDSA.test.js

@@ -14,48 +14,101 @@ contract('ECDSA', function ([_, anyone]) {
    this.mock = await ECDSAMock.new();


While we're at it, can you rename this to this.ecdsa or something more descriptive?

nventuro · 2018-09-26T15:14:49Z

test/library/ECDSA.test.js

+      const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
+      // eslint-disable-next-line max-len
+      const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
+      context('with 00 as version value', function () {


Please add an empty line before this context

nventuro · 2018-09-26T15:17:00Z

test/library/ECDSA.test.js

-    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
-  });
+  context('recover with valid signature', function () {
+    context('with v0 signature', function () {


Shouldn't we have a case for v0 (and v1) signatures, where the version is incorrect?

I don't think so. We check the version first, and if it's not 0/1/27/28, we return 0. It doesn't matter if the rest of the signature makes sense or not.

That's an internal detail though 😛 I wanted to include that it inside this context, to make it clear that all signatures fail if the version is wrong

alright. I kind-of thing that's a myth that you can (and should) keep your tests totally independent from the implementation. But I'll follow your advice here. One less magic number anyway.

nventuro · 2018-09-26T15:20:07Z

test/library/ECDSA.test.js

+          // Signature generated outside ganache with method web3.eth.sign(signer, message)
+          const version = '00';
+          const signature = signatureWithoutVersion + version;
+          (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);


I'd create a function as follows:

async function shouldRecoverSigner() { (await this.mock.recover(TEST_MESSAGE, this.signatureWithoutVersion + this.version)).should.equal(this.signer); }

then have each context assign version and friends in a beforeEach, and then simply do

it('works', async function () { await shouldRecoverSigner(); });

WDYT?

I'm slightly against this. I think the duplication here makes every test easier to read independently. shouldRecoverSigner hides how recover works, and for tests I tend to value more the expressiveness of each one than the amount of code. I find this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer); nice and short.

But there are good arguments for simplifying with the function, I can make the change. Your call @nventuro.

shouldRecoverSigner will be extremely close by for in-depth analysis, so I don't agree with it hiding the internals. IMO, the tests not sharing code seems to suggest they do different things, and I have to manually compare each line to realize they are actually all the same. @frangio wtb exec decision

frangio

I've been trying to understand more about the v value and I've learnt that it's not a version number. It's a "recovery identifier" according to the yellow paper; something related to the parameters of the ECDSA curve. So 1) we might want to change everywhere that says "version" in these tests, but 2) I have no idea how to generate the different v values so the hardcoded signatures will have to do for now.

frangio · 2018-09-26T17:39:54Z

test/library/ECDSA.test.js

-  });
+      context('with 00 as version value', function () {
+        it('works', async function () {
+          // Signature generated outside ganache with method web3.eth.sign(signer, message)


This comment seems like it should be moved above next to the declaration of signatureWithoutVersion.

Removed, thanks for catching it.

come-maiz · 2018-09-26T19:02:38Z

@frangio I think that research about v and refactoring everything to that new name once we understand what it is and how to get web3 to generate it should be a separate task.

frangio · 2018-09-26T19:08:25Z

test/library/ECDSA.test.js

@@ -9,7 +9,7 @@ require('chai')
 const TEST_MESSAGE = web3.sha3('OpenZeppelin');
 const WRONG_MESSAGE = web3.sha3('Nope');

-contract('ECDSA', function ([_, anyone]) {
+contract.only('ECDSA', function ([_, anyone]) {


I hate this tool :)

@nventuro

* fix: refactor sign.js and related tests * fix: remove unused dep * fix: update package.json correctly * Add missing tests to ECRecovery * fix lint * Reorganize the tests * Reuse signature * fix static errors * Apply suggestions by @frangion and @nventuro * Remove only * More suggestions * Remove unnecessary max-len * remove only (cherry picked from commit 75c0a59)

shrugs and others added 5 commits August 25, 2018 11:00

fix: refactor sign.js and related tests

6db5d6e

fix: remove unused dep

654b90b

fix: update package.json correctly

9f6170e

Add missing tests to ECRecovery

1be1641

fix lint

4f5abbb

come-maiz changed the title ~~Test/missing signature tests~~ Add missing tests to ECRecovery Aug 29, 2018

come-maiz added kind:improvement tests Test suite and helpers. labels Aug 29, 2018

Merge branch 'master' into test/missing-signature-tests

2b3ab7c

nventuro reviewed Aug 30, 2018

View reviewed changes

frangio added this to the v2.0 milestone Sep 24, 2018

come-maiz added 3 commits September 25, 2018 17:08

Merge branch 'master' into test/missing-signature-tests

c7373ef

Reorganize the tests

22a38de

Reuse signature

1ccf496

fix static errors

f2d2528

frangio changed the title ~~Add missing tests to ECRecovery~~ Add missing tests to ECSDA Sep 26, 2018

frangio reviewed Sep 26, 2018

View reviewed changes

nventuro reviewed Sep 26, 2018

View reviewed changes

come-maiz added 3 commits September 26, 2018 16:51

Merge branch 'master' into test/missing-signature-tests

0d0df73

Apply suggestions by @frangion and @nventuro

d83f04b

Remove only

25a5823

frangio reviewed Sep 26, 2018

View reviewed changes

More suggestions

fde2719

Remove unnecessary max-len

f5c8298

frangio reviewed Sep 26, 2018

View reviewed changes

remove only

35907fa

nventuro approved these changes Sep 26, 2018

View reviewed changes

nventuro merged commit 75c0a59 into OpenZeppelin:master Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing tests to ECSDA #1248

Add missing tests to ECSDA #1248

come-maiz commented Aug 29, 2018

nventuro commented Aug 29, 2018

come-maiz commented Aug 29, 2018

nventuro Aug 29, 2018

nventuro Aug 29, 2018

come-maiz commented Sep 26, 2018 •

edited

Loading

frangio Sep 26, 2018

come-maiz Sep 26, 2018

nventuro Sep 26, 2018

come-maiz Sep 26, 2018

nventuro Sep 26, 2018

come-maiz Sep 26, 2018

nventuro Sep 26, 2018

come-maiz Sep 26, 2018

nventuro Sep 26, 2018

come-maiz Sep 26, 2018 •

edited

Loading

nventuro Sep 26, 2018 •

edited

Loading

frangio left a comment

frangio Sep 26, 2018

come-maiz Sep 26, 2018

come-maiz commented Sep 26, 2018

frangio Sep 26, 2018

come-maiz Sep 26, 2018

		@@ -14,48 +14,101 @@ contract('ECDSA', function ([_, anyone]) {
		this.mock = await ECDSAMock.new();

Add missing tests to ECSDA #1248

Add missing tests to ECSDA #1248

Conversation

come-maiz commented Aug 29, 2018

🚀 Description

nventuro commented Aug 29, 2018

come-maiz commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz commented Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

nventuro Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz commented Sep 26, 2018 •

edited

Loading

come-maiz Sep 26, 2018 •

edited

Loading

nventuro Sep 26, 2018 •

edited

Loading