Skip to content

Conversation

@ptrkstr
Copy link

@ptrkstr ptrkstr commented Sep 30, 2021

  • generateOTP(secret: Data, algorithm: OTPAlgorithm = .sha1, counter: UInt64, digits: Int = 6) -> String was returning String? but there wasn't a way in which it could return nil
  • func generate(secondsPast1970: UInt) -> String no longer returns an optional. Positive integer is enforced via UInt usage.
  • func validateTime(time: Int) -> Bool deleted as no longer a need to validate positive integer.

@ptrkstr
Copy link
Author

ptrkstr commented Sep 30, 2021

I saw your comment here @lachlanbell about UInt64 being in the spec but I couldn't see it.
#2 (comment)

@lachlanbell
Copy link
Owner

Hi @ptrkstr, thanks for the PR!

Really sorry, I haven't had a chance to properly review this yet due to other commitments eating up almost all of my free time. I'll give this a proper look soon.

Regarding the spec, it mentions that the counter value C should be 8 bytes (=64 bits), so from a preliminary look I think UInt64 should still be used. The UInt64 type isn't hardware dependent, so it should work on 32-bit systems as well.

@ptrkstr
Copy link
Author

ptrkstr commented Oct 4, 2021

Hey @lachlanbell,
Thanks for the reply, no apology necessary :)
Ahh great point, let me revert the UInt64 changes. I think there's value in the other changes I've made regarding UInt over Int to avoid a guard for negative number checking.
I'll comment back when it's done.

@ptrkstr ptrkstr force-pushed the master branch 3 times, most recently from 5ff0817 to 8eafaf6 Compare October 5, 2021 16:49
- `generateOTP(secret: Data, algorithm: OTPAlgorithm = .sha1, counter: UInt64, digits: Int = 6) -> String` was returning `String?` but there wasn't a way in which it could return `nil`
- `func generate(secondsPast1970: UInt) -> String` no longer returns an optional. Positive integer is enforced via `UInt` usage.
- `func validateTime(time: Int) -> Bool` deleted as no longer a need to validate positive integer.
@ptrkstr
Copy link
Author

ptrkstr commented Oct 5, 2021

Hey @lachlanbell, I've reverted the UInt64 changes, ready for review.

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