-
Notifications
You must be signed in to change notification settings - Fork 1
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
Handle reset signal from the admin app #188
Conversation
af099a8
to
b8ba8f5
Compare
b8ba8f5
to
ee932c0
Compare
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.
Please add a changelog entry and check if we need to clear the volatile state before resetting the state. Otherwise LGTM!
} | ||
} | ||
} | ||
|
||
self.state.volatile.clear(self.backend.client_mut()); |
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.
Do we need this in the factory reset case in handle
too? Maybe introduce a helper method reset_state
?
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 clear
method removes the private keys in volatile memory. Since the Factory Reset has already cleared all the filesystems, including the volatile, it will fail.
It was actually a mistake to do it after a factory reset.
src/card.rs
Outdated
self.state = State::default(); | ||
reset_signal.ack_factory_reset(); |
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.
nit: We could also return after this because we don’t need to reset the state again.
src/state.rs
Outdated
#[derive(Clone, Debug, Default, Eq, PartialEq)] | ||
#[derive(Clone, Debug, Default)] |
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.
nit: no more need to remove the derives, but doesn’t make a difference
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.
Thanks for the changes, looks good!
Built on top of #187. Only the last commit matters.