-
Notifications
You must be signed in to change notification settings - Fork 1
N13/high security implementation 2 #363
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
also added theme colors for the tags
fix data types and names which were wrong in the mock implementation
sanitized data types even more
remove a lot of text, add human checkphrase
didn't rename since we don't support multiple chains anyway
| } | ||
|
|
||
| // Wait for accounts to reload | ||
| await ref.read(accountsProvider.notifier).stream.firstWhere((state) => !state.isLoading); |
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.
Since accountsProvider is an AsyncValue. If we just want to wait for the future to resolve, we should just use await ref.read(accountsProvider.future); because listening to stream of state.isLoading seems not so reliable, there is a chance isLoading never flip to false. Better go with simpler and more reliable solution.
mobile-app/lib/features/main/screens/high_security/guardian_account_info_sheet.dart
Outdated
Show resolved
Hide resolved
mobile-app/lib/features/main/screens/high_security/high_security_confirmation_sheet.dart
Outdated
Show resolved
Hide resolved
mobile-app/lib/features/main/screens/high_security/high_security_details_screen.dart
Outdated
Show resolved
Hide resolved
| const HighSecurityDetailsScreen({super.key, required this.account}); | ||
|
|
||
| // Shared style accessors | ||
| static TextStyle text16(BuildContext context) => TextStyle( |
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.
Shouldn't this style go to theme file?
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.
No these are just some helpers for this specific screen...
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.
Oh okay, all good then
| ), | ||
| ), | ||
| data: (data) { | ||
| if (data == null) { |
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.
Will we ever see this? or this is just to complete the possible data type case?
mobile-app/lib/features/main/screens/high_security/high_security_details_screen.dart
Outdated
Show resolved
Hide resolved
mobile-app/lib/features/main/screens/high_security/high_security_details_screen.dart
Outdated
Show resolved
Hide resolved
| import 'package:resonance_network_wallet/providers/high_security_form_provider.dart'; | ||
| import 'package:resonance_network_wallet/providers/wallet_providers.dart'; | ||
|
|
||
| final highSecurityEstimatedFeeProvider = FutureProvider.family<BigInt, Account>((ref, account) async { |
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.
Same like before, shouldn't we just put in provider folder.
mobile-app/lib/features/main/screens/high_security/high_security_get_started_screen.dart
Show resolved
Hide resolved
mobile-app/lib/features/main/screens/high_security/high_security_guardian_wizard.dart
Outdated
Show resolved
Hide resolved
mobile-app/lib/features/main/screens/high_security/high_security_guardian_wizard.dart
Outdated
Show resolved
Hide resolved
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. There are some questions I have. Overall looking great to me.
Seems, can;t really review it using gemini 3 pro because the changes is so huge.
| _SafeguardWindowSection(safeguardWindow: data.safeguardWindow), | ||
| const SizedBox(height: 20), | ||
| const _RemindersSection(reminders: reminders), | ||
| // const SizedBox(height: 20), |
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.
Why only comment? will it be used later?
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.
yeah once we have reminders
idk where to store them
on chain? task master? on the local?
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 unsure about the watching stream if it's reliable or not. As long as you have tested it all good then
Features added
Next PR will be about guardian accounts being able to intercept transactions.