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 all commits
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
135 changes: 98 additions & 37 deletions test/library/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,56 +11,117 @@ const WRONG_MESSAGE = web3.sha3('Nope');

contract('ECDSA', function ([_, anyone]) {
beforeEach(async function () {
this.mock = await ECDSAMock.new();
this.ecdsa = await ECDSAMock.new();
});

it('recover v0', async function () {
// Signature generated outside ganache with method web3.eth.sign(signer, message)
const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
// eslint-disable-next-line max-len
const signature = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be89200';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
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 signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';

it('recover v1', async function () {
// Signature generated outside ganache with method web3.eth.sign(signer, message)
const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
// eslint-disable-next-line max-len
const signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e001';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
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 () {
const version = '00';
const signature = signatureWithoutVersion + version;
(await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});

it('recover using web3.eth.sign()', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
context('with 27 as version value', function () {
it('works', async function () {
const version = '1b'; // 27 = 1b.
const signature = signatureWithoutVersion + version;
(await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});

// Recover the signer address from the generated message and signature.
(await this.mock.recover(
toEthSignedMessageHash(TEST_MESSAGE),
signature
)).should.equal(anyone);
});
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.
const version = '02';
const signature = signatureWithoutVersion + version;
(await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(
'0x0000000000000000000000000000000000000000');
});
});
});

it('recover using web3.eth.sign() should return wrong signer', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
context('with v1 signature', function () {
const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
// eslint-disable-next-line max-len
const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';

// Recover the signer address from the generated message and wrong signature.
(await this.mock.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
context('with 01 as version value', function () {
it('works', async function () {
const version = '01';
const signature = signatureWithoutVersion + version;
(await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});

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

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.
const version = '02';
const signature = signatureWithoutVersion + version;
(await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(
'0x0000000000000000000000000000000000000000');
});
});
});

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);

// Recover the signer address from the generated message and signature.
(await this.ecdsa.recover(
toEthSignedMessageHash(TEST_MESSAGE),
signature
)).should.equal(anyone);
});
});

context('with wrong signature', function () {
it('does not return signer address', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);

// Recover the signer address from the generated message and wrong signature.
(await this.ecdsa.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
});
});
});
});

// @TODO - remove `skip` once we upgrade to solc^0.5
it.skip('recover should revert when a small hash is sent', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
await expectThrow(
this.mock.recover(TEST_MESSAGE.substring(2), signature)
);
context('with small hash', function () {
// @TODO - remove `skip` once we upgrade to solc^0.5
it.skip('reverts', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
await expectThrow(
this.ecdsa.recover(TEST_MESSAGE.substring(2), signature)
);
});
});

context('toEthSignedMessage', function () {
it('should prefix hashes correctly', async function () {
(await this.mock.toEthSignedMessageHash(TEST_MESSAGE)).should.equal(toEthSignedMessageHash(TEST_MESSAGE));
(await this.ecdsa.toEthSignedMessageHash(TEST_MESSAGE)).should.equal(toEthSignedMessageHash(TEST_MESSAGE));
});
});
});