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
Reorganize the tests
  • Loading branch information
Leo Arias committed Sep 26, 2018
commit 22a38de0f4b2e3bc1df01240c3eb03fe955c8205
144 changes: 85 additions & 59 deletions test/library/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,75 +14,101 @@ 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.

});

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() {
// 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() {
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;
(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

});
});

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 27 as version value', function() {
it('works', async function () {
const version = '1b'; // 27 = 1b.
const signature = signature_without_version + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});
});

it('recover v27', async function () {
// Signature generated outside ganache with method web3.eth.sign(signer, message)
const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
// The last two hex digits are the signature version: 27 = 1b.
// eslint-disable-next-line max-len
const signature = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be8921b';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
context('with v1 signature', function() {
const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
// eslint-disable-next-line max-len
const signature_without_version = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';

it('recover v28', async function () {
// Signature generated outside ganache with method web3.eth.sign(signer, message)
const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
// The last two hex digits are the signature version: 28 = 1c.
// eslint-disable-next-line max-len
const signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
context('with 01 as version value', function() {
it('works', async function () {
const version = '01';
const signature = signature_without_version + version;
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
});
});

it('wrong signature version', 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 signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e002';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(
'0x0000000000000000000000000000000000000000');
});
context('with 28 signature', function() {
it('works', async function () {
const version = '1c'; // 28 = 1c.
const signature = signature_without_version + version;
(await this.mock.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('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.mock.recover(
toEthSignedMessageHash(TEST_MESSAGE),
signature
)).should.equal(anyone);
});
// Recover the signer address from the generated message and signature.
(await this.mock.recover(
toEthSignedMessageHash(TEST_MESSAGE),
signature
)).should.equal(anyone);
});
});

it('recover using web3.eth.sign() should return wrong signer', async function () {
// Create the signature
const signature = signMessage(anyone, TEST_MESSAGE);
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.mock.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
// Recover the signer address from the generated message and wrong signature.
(await this.mock.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('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 = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
const signature = dummy_signature_without_version + '02';
(await this.mock.recover(TEST_MESSAGE, signature)).should.equal(
'0x0000000000000000000000000000000000000000');
});
});

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.mock.recover(TEST_MESSAGE.substring(2), signature)
);
});
});
});

context('toEthSignedMessage', function () {
Expand Down