-
Notifications
You must be signed in to change notification settings - Fork 1
Full Keystone HW Wallet Support #355
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
this helps the hardware wallet. there may be another way to do this but for now dummy signature works well.
upgrade dependencies to pointy castle 4.0.0 to be compatible with the latest polkadart version. also upgrade human checksum dart to v1.1.0 tag so we are compatible with pointycastle.
The HW wallet will need to blake2 the output before signing.
These are useful as a reference implementation
removed uneeded file, fix doc comment, formatting
Alles was nicht erlaubt ist ist verboten!
no need for hex encoding since UR is CBOR encoded
add known value test
mobile-app/lib/features/main/screens/account_settings_screen.dart
Outdated
Show resolved
Hide resolved
| Item(value: _WalletMoreAction.importWallet, label: 'Import wallet'), | ||
| ]; | ||
|
|
||
| if (FeatureFlags.isFeatureEnabled('keystone_hardware_wallet')) { |
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.
Let's make the flag an enum
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.
I just wanted to fit in the system. As I am concerned we could just have some booleans. Not sure why we have this fancy system with strings.
Not related to this PR - should be follow up PR refactor.
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.
Okay, we can do either way, using string just ain't safe enough
mobile-app/lib/features/main/screens/send/qr_scanner_screen.dart
Outdated
Show resolved
Hide resolved
| @@ -1,5 +1,6 @@ | |||
| class AppConstants { | |||
| static const globalDebug = false; | |||
| static const debugHardwareWallet = false; | |||
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.
Is this used at all?
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.
Yes. Not sure we should leave this debug code in there now that it works. Hmm...
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.
Moved this to the end of the file and added a comment since this is quite intricate.
GEMINI PRO 3 REVIEWThe branch Summary of Changes
Code ReviewPositives:
Issues / Suggestions:
RecommendationThe implementation looks solid. I recommend removing the unused import in The branch Key Changes
Review ActionsI found one minor issue and fixed it:
The rest of the code looks well-structured, with clear separation between UI, SDK logic, and Rust crypto primitives. The feature is safely flagged for incremental rollout. |
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.
I have manually reviewed it and also use gemini pro 3 to review.
| } | ||
|
|
||
| int getSelectedWalletIndex(List<Account> accounts) { | ||
| final selectedWallet = _selectedWalletIndex ?? (accounts.isNotEmpty ? accounts.first.walletIndex : 0); |
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.
I mean can we not use ternary? We can do if else instead.
if (_selectedWalletIndex != null) {
return _selectedWalletIndex
} else if (accounts.isNotEmpty) {
return accounts.first.walletIndex
} else {
return 0
}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.
whats all this hate for ternary?
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.
I mean to me this if statement is less clear than the original... I have to think about it more
The original is simply this
if it's defined, return it
if it's not defined, return some sort of reasonable default...
if there are wallets, return the first wallet index
if not, return 0 - but this case never happens here, thats just there to make this function return an int thats' not optional.
dewabisma
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.
LGTM! Just have one non blocking request for change
Overview
Demo
Demo: https://youtu.be/r1nDWzD6-x8
Caveats
The send overlay is super complicated now, follow up PR will refactor this to make it smaller and factor our things into a hardware wallet service.
Will be fixed here:
#356