-
Notifications
You must be signed in to change notification settings - Fork 25
Expose counter to support more efficient sync #421
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
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
=======================================
Coverage 76.75% 76.76%
=======================================
Files 270 270
Lines 25664 25664
=======================================
+ Hits 19698 19700 +2
+ Misses 5966 5964 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@addisonbeck aside from a unit test or two, this is ready to start looking at. I figured I'd wait to take this PR out of draft until I received a first pass from you. I went ahead and included fixes for the things that the |
addisonbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I request a review from Dani, since I don't have my name in the commit history of the SDK and he's the expert.
|
I think the issues you were finding with the static assertions and the elided lifetime were likely caused by using a newer |
I thought so too, so I tried running the same command with the At this point I'm curious what it is about my env that is different from CI. |
When using 0.1.53 locally, I was experiencing an error[1]. [1]: est31/cargo-udeps#293
ec1c3ea to
df88442
Compare
|
addisonbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
coroiu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice job 👍




🎟️ Tracking
PM-22201
📔 Objective
Mobile clients need to know if the FIDO 2 counter property is nonzero in order to determine if sync is required before performing authentication.
Updates
cargo-udepsinlint.ymlto version0.1.57. Previous version was 9 months old. Locally, I was experiencing a bug wherecargo-udepswas considering#[cfg(test)]to be invalid.Updates a couple static assertions that were showing up as unused when running
cargo +nightly udeps --workspace --all-featureslocally. Unsure why this wasn't previously showing up in CI.Added an explicit lifetime to fix a
mismatched-lifetime-syntaxerror, again when running theudepscheck locally.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes