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 tests for conversion from and to ASN.1 Integers for EC signatures #160

Merged
4 commits merged into from
Jun 27, 2019

Conversation

mschwaig
Copy link
Contributor

This resolves #157 by

  • adding tests for ASN.1 tests,

but also

  • improves some error messages in the EC implementation and
  • moves the conversion functions under test into an internal struct for now.

Moving the conversion logic into theECCurveType like suggested in #156 (comment) might still be a good idea, but I did not do this for now. Feel free to do that.

@Jose-ElRobot
Copy link

Jose-ElRobot commented Jun 19, 2019

1 Message
📖 Any non-trivial changes to code should be reflected in the changelog. Please consider adding a note in the Unreleased section of the CHANGELOG.md.

Generated by 🚫 Danger

@ghost
Copy link

ghost commented Jun 24, 2019

Hi @mschwaig, thanks for following up on #156 with an extended test suite and sorry for the delay. I'll give it a thorough review on Wednesday. Expect your changes to be released with a new version of JOSESwift this week. I hope this is ok with you.

@mschwaig
Copy link
Contributor Author

Hi @mschwaig, thanks for following up on #156 with an extended test suite and sorry for the delay. I'll give it a thorough review on Wednesday. Expect your changes to be released with a new version of JOSESwift this week. I hope this is ok with you.

Sounds good, thanks. 😎

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot @mschwaig! That's exactly what we needed. 👌

I think I spotted one small mistake in the tests. Should be an easy fix, though.

Tests/ECAsn1IntegerConversionTests.swift Outdated Show resolved Hide resolved
@mschwaig
Copy link
Contributor Author

Hi Daniel!
You definitely found a bug in the tests and fixed it correctly. I applied your fix, but I cannot re-run the tests locally right now. I'm guessing that's ok, as long as Travis runs them.

Co-Authored-By: Daniel <daniel-mohemian@users.noreply.github.com>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks, @mschwaig for your contribution, it was nice to work with you. 🎉

We'll release your changes as 1.8.1. I'll ping you here once they're out.

@ghost ghost merged commit 291de1e into airsidemobile:master Jun 27, 2019
@ghost ghost mentioned this pull request Jun 27, 2019
@ghost
Copy link

ghost commented Jun 27, 2019

1.8.1 is released and available on CocoaPods. 🎉

This pull request was closed.
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.

Add tests for encoding and decoding of raw ECDSA signatures to and from DER ASN.1
2 participants