Skip to content
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.

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

Add key type tests #35

Merged
merged 12 commits into from
Jul 8, 2024
Merged

Add key type tests #35

merged 12 commits into from
Jul 8, 2024

Conversation

aljones15
Copy link
Contributor

@aljones15 aljones15 commented Jul 5, 2024

Refactors the test suite so a common set of assertions runs on all the curves and not just one of them.

Addresses: #34

@aljones15 aljones15 self-assigned this Jul 5, 2024
@@ -9,22 +9,55 @@ export const mockKey = {
};

export const mockKeyEcdsaSecp256 = {
type: 'EcdsaSecp256r1VerificationKey2019',
type: 'Multikey',
Copy link
Member

Choose a reason for hiding this comment

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

This one needs to be reverted back to EcdsaSecp256r1VerificationKey2019. This is for backwards compatibility tests. The others should be Multikey as they are now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted here: 28c76f3 but that breaks the id should be set test so will need to further revise this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlongley ok so inside of the multkeys map in mock-data I now set the type to Multikey so the P-256 one will pass the should set id test: https://github.com/digitalbazaar/ecdsa-multikey/pull/35/files/28c76f394ed4919f39581d2fdc60611810a2dd92..09a57e00aeebfdc1d25a6e6eee4c64273e64a5ff

Should we test both the backwards compatible EcdsaSecp256r1VerificationKey2019 P-256 and the Multikey P-256? if so I'm assuming for EcdsaSecp256r1VerificationKey2019 the id is not set by default?

This is the test that is failing

it('should auto-set key.id based on controller', async () => {
const {publicKeyMultibase} = serializedKeyPair;
const keyPair = await EcdsaMultikey.from(serializedKeyPair);
_ensurePublicKeyEncoding({keyPair, keyType, publicKeyMultibase});
expect(keyPair.id).to.equal(id);
});
for EcdsaSecp256r1VerificationKey2019

Copy link
Member

Choose a reason for hiding this comment

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

@aljones15, we shouldn't hack the key type to be Multikey just to pass this test. Either the library is supposed to auto-generate an id no matter what the key type was, or it shouldn't do it for a key type of EcdsaSecp256r1VerificationKey2019. For now, I'd say to skip this test if the key type isn't Multikey and file an issue to possibly add id auto-generation for other key types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlongley not sure I hacked the key type to be Multikey as by default generate({curve: 'P-256'}) produces a key with type Multikey. All the test suite does is ensure that all keys in the multikeys mock-data map have type Multikey. The original EcdsaSecp256r1VerificationKey2019 is not mutated and still exists in the suite. The question is should we test both P-256 Multikey and P-256 EcdsaSecp256r1VerificationKey2019, the answer to that appears to be file an issue about it for now as EcdsaSecp256r1VerificationKey2019 does not pass all the tests.

Copy link
Contributor Author

@aljones15 aljones15 Jul 7, 2024

Choose a reason for hiding this comment

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

issue created here: #36 for EcdsaSecp256r1VerificationKey2019 in test suite loop and and key id generation.

Copy link
Member

Choose a reason for hiding this comment

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

All we want with the EcdsaSecp256r1VerificationKey2019 key pair is to demonstrate that we can import and use it like any other keypair. It exists so we can run tests against legacy key pairs that use that type. So as long as this PR includes some tests against something that imports using that key type, we're good. That doesn't have to include id generation at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlongley the original key type import tests were left as is:

describe('Backwards compat with EcdsaSecp256r1VerificationKey2019', () => {
it('Multikey should import properly', async () => {
const keyPair = await EcdsaMultikey.from(mockKeyEcdsaSecp256);
const data = (new TextEncoder()).encode('test data goes here');
const signature = await keyPair.signer().sign({data});
expect(
await keyPair.verifier()
.verify({data, signature})
).to.be.true;
});
});

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good.

test/mock-data.js Outdated Show resolved Hide resolved
@aljones15 aljones15 requested a review from dlongley July 7, 2024 13:27
Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

I'm not sure about this style, but I don't have a better suggestion for reuse that is as succinct, so I'm going to approve and merge this. It's hard to tell that everything was preserved so I'm just assuming it is -- and maybe some codecov printout will tell us that things still get the same or better coverage somewhere.

@dlongley
Copy link
Member

dlongley commented Jul 8, 2024

Thanks! Going to merge this now.

@dlongley dlongley merged commit e9b2c6e into main Jul 8, 2024
5 checks passed
@dlongley dlongley deleted the add-key-type-tests branch July 8, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants