Skip to content

Add missing tests to ECSDA #1248

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.

Sign up for GitHub

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

Merged
nventuro merged 16 commits into from
Sep 26, 2018
Merged

Add missing tests to ECSDA #1248

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6db5d6e
fix: refactor sign.js and related tests
shrugs Aug 25, 2018
654b90b
fix: remove unused dep
shrugs Aug 25, 2018
9f6170e
fix: update package.json correctly
shrugs Aug 26, 2018
1be1641
Add missing tests to ECRecovery
Aug 29, 2018
4f5abbb
fix lint
Aug 29, 2018
2b3ab7c
Merge branch 'master' into test/missing-signature-tests
Aug 29, 2018
c7373ef
Merge branch 'master' into test/missing-signature-tests
Sep 25, 2018
22a38de
Reorganize the tests
Sep 26, 2018
1ccf496
Reuse signature
Sep 26, 2018
f2d2528
fix static errors
Sep 26, 2018
0d0df73
Merge branch 'master' into test/missing-signature-tests
Sep 26, 2018
d83f04b
Apply suggestions by @frangion and @nventuro
Sep 26, 2018
25a5823
Remove only
Sep 26, 2018
fde2719
More suggestions
Sep 26, 2018
f5c8298
Remove unnecessary max-len
Sep 26, 2018
35907fa
remove only
Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix static errors
  • Loading branch information
Leo Arias committed Sep 26, 2018
commit f2d2528fd91e21c38287a7aabd5863ee096ab09b
46 changes: 23 additions & 23 deletions test/library/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,54 +14,54 @@ contract('ECDSA', function ([_, anyone]) {
this.mock = await ECDSAMock.new();
Copy link
Contributor

@frangio frangio Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@come-maiz come-maiz Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

});

context('recover with valid signature', function() {
context('with v0 signature', function() {
context('recover with valid signature', function () {
context('with v0 signature', function () {
Copy link
Contributor

@nventuro nventuro Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@come-maiz come-maiz Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nventuro nventuro Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@come-maiz come-maiz Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Signature generated outside ganache with method web3.eth.sign(signer, message)
const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
// eslint-disable-next-line max-len
const signature_without_version = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
context('with 00 as version value', function() {
const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
context('with 00 as version value', function () {
Copy link
Contributor

@nventuro nventuro Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an empty line before this context

Copy link
Author

@come-maiz come-maiz Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

it('works', async function () {
// Signature generated outside ganache with method web3.eth.sign(signer, message)
Copy link
Contributor

@frangio frangio Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@come-maiz come-maiz Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks for catching it.

const version = '00';
const signature = signature_without_version + version;
const signature = signatureWithoutVersion + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
Copy link
Contributor

@nventuro nventuro Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@come-maiz come-maiz Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nventuro nventuro Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
});

context('with 27 as version value', function() {
context('with 27 as version value', function () {
it('works', async function () {
const version = '1b'; // 27 = 1b.
const signature = signature_without_version + version;
const version = '1b'; // 27 = 1b.
const signature = signatureWithoutVersion + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});
});

context('with v1 signature', function() {
context('with v1 signature', function () {
const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
// eslint-disable-next-line max-len
const signature_without_version = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';

context('with 01 as version value', function() {
context('with 01 as version value', function () {
it('works', async function () {
const version = '01';
const signature = signature_without_version + version;
const signature = signatureWithoutVersion + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});

context('with 28 signature', function() {
context('with 28 signature', function () {
it('works', async function () {
const version = '1c'; // 28 = 1c.
const signature = signature_without_version + version;
const version = '1c'; // 28 = 1c.
const signature = signatureWithoutVersion + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});
});

context('using web3.eth.sign', function() {
context('with correct signature', function() {
context('using web3.eth.sign', function () {
context('with correct signature', function () {
it('returns signer address', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
Expand All @@ -74,7 +74,7 @@ contract('ECDSA', function ([_, anyone]) {
});
});

context('with wrong signature', function() {
context('with wrong signature', function () {
it('does not return signer address', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
Expand All @@ -86,20 +86,20 @@ contract('ECDSA', function ([_, anyone]) {
});
});

context('recover with invalid signature', function() {
context('with wrong version', function() {
context('recover with invalid signature', function () {
context('with wrong version', function () {
it('returns 0', async function () {
// 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 dummy_signature_without_version = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
const signature = dummy_signature_without_version + '02';
const dummySignatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
const signature = dummySignatureWithoutVersion + '02';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(
'0x0000000000000000000000000000000000000000');
});
});

context('with small hash', function() {
context('with small hash', function () {
// @TODO - remove `skip` once we upgrade to solc^0.5
it.skip('reverts', async function () {
// Create the signature
Expand Down