-
Notifications
You must be signed in to change notification settings - Fork 133
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
Nostr Preferences #1186
base: master
Are you sure you want to change the base?
Nostr Preferences #1186
Conversation
sarthak13gupta
commented
Aug 11, 2023
- Nostr Screen for Creating a New Account / Importing Private Key.
- Publishing User Defined Relays.
- Added Primal(Nostr client).
- Added support for Nip04 encrypt and decrypt operations.
…ggle_comments_check
…upta/breezmobile into comments_platform
…into nostr_screen
…zmobile into comments_login_logout
…zmobile into comments_login_logout
…/breezmobile into comments_platform
…into comments_platform
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.
Have some NIT comments.
I'll get into reviewing bloc logic & UI at a later time.
var nostrSetttingsModel = NostrSettings.fromJson(settings); | ||
_nostrSettingsController.add(nostrSetttingsModel.copyWith( | ||
enableNostr: nostrSetttingsModel.enableNostr, | ||
isRememberPubKey: nostrSetttingsModel.isRememberPubKey, | ||
isRememberSignEvent: nostrSetttingsModel.isRememberSignEvent, | ||
isLoggedIn: nostrSetttingsModel.isLoggedIn, | ||
relayList: nostrSetttingsModel.relayList, | ||
)); |
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 we're not changing any of the NostrSettings
values, we can add it directly as:
var nostrSetttingsModel = NostrSettings.fromJson(settings); | |
_nostrSettingsController.add(nostrSetttingsModel.copyWith( | |
enableNostr: nostrSetttingsModel.enableNostr, | |
isRememberPubKey: nostrSetttingsModel.isRememberPubKey, | |
isRememberSignEvent: nostrSetttingsModel.isRememberSignEvent, | |
isLoggedIn: nostrSetttingsModel.isLoggedIn, | |
relayList: nostrSetttingsModel.relayList, | |
)); | |
_nostrSettingsController.add(NostrSettings.fromJson(settings)); |
lib/bloc/nostr/nostr_bloc.dart
Outdated
final defaultRelaysList = [ | ||
"wss://relay.damus.io", | ||
"wss://nostr1.tunnelsats.com", | ||
"wss://nostr-pub.wellorder.net", | ||
"wss://relay.nostr.info", | ||
"wss://nostr-relay.wlvs.space", | ||
"wss://nostr.bitcoiner.social", | ||
"wss://nostr-01.bolt.observer", | ||
"wss://relayer.fiatjaf.com", | ||
]; | ||
|
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.
Can't we read these from NostSettings
? There should be one source of this list.
lib/home_page.dart
Outdated
import 'bloc/marketplace/nostr_settings.dart'; | ||
import 'bloc/nostr/nostr_actions.dart'; |
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.
We're trying to avoid relative path imports as they are not git/refactor-friendly. Would you replace these and any other imports in the new files introduced to use absolute paths on imports?
Will be working on these changes, also I found that the nip04 operations need a bit of correction, will do it in the upcoming commits. |
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 @sarthak13gupta !
Done initial pass. Dropped some comments and questions.
It seems to me that it will be easier to manage all nostr part in one bloc (NostrBloc) instead of splitting it into several blocs that depends on each other.
The marketplace bloc might depend on the NostrBloc (for the setting purposes).
isRememberPubKey: false, | ||
isRememberSignEvent: false, | ||
// start should be done by retrieving the values set in sharedPreferences | ||
NostrSettings.start({ |
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: rename start
to initial
lib/home_page.dart
Outdated
commentBloc.signEventStream.listen((event) async { | ||
final nostrPrivateKey = widget.nostrBloc.nostrPrivateKey; | ||
|
||
widget.nostrBloc.actionsSink.add(SignEvent(event, nostrPrivateKey)); |
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.
It seems that it is more correct to not pass the private key to the SignEvent since the private key is already saved in the bloc.
All operations should not read (and pass) the private key which should be private field on the bloc and used there when needed.
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, will make the changes.
lib/bloc/nostr/nostr_bloc.dart
Outdated
@@ -39,6 +54,14 @@ class NostrBloc with AsyncActionsHandler { | |||
nostrPrivateKey = await _secureStorage.read(key: "nostrPrivateKey"); |
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.
It seems that we are storing the private key in the golang library but read it from the secure storage. Can you explain that?
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.
We are storing and fetching the private key in the golang library but only for the first time when a nostrBloc instance is made . Otherwise, if a new instance is made the private key is fetched from the secure storage(where it was stored the first time the key was created and fetched from the golang library).
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.
If it is stored in the golang library why use secure storage 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.
okay got it, will remove the secure storage part.
@@ -32,3 +14,29 @@ Map<int, String> eventKind = { | |||
10002: 'RelayList', | |||
30078: 'Application Specific Data' | |||
}; | |||
|
|||
Event mapToEvent(Map<String, dynamic> eventObject) { |
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: in dart the best practice is to use toJson
and fromJson
lib/home_page.dart
Outdated
@@ -127,6 +140,41 @@ class HomeState extends State<Home> with WidgetsBindingObserver { | |||
}); | |||
}); | |||
|
|||
commentBloc = Provider.of<NostrCommentBloc>(context, listen: 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.
I don't see the NostrCommentBloc in the codebase. Did you forget to push?
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.
It is a part of any time, you will find it here breez/anytime_podcast_player#48
lib/home_page.dart
Outdated
@@ -127,6 +140,41 @@ class HomeState extends State<Home> with WidgetsBindingObserver { | |||
}); | |||
}); | |||
|
|||
commentBloc = Provider.of<NostrCommentBloc>(context, listen: false); | |||
|
|||
widget.marketplaceBloc.nostrSettingsStream.listen((event) { |
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.
All these wiring between blocs should not be in the UI layer. More than that it indicates there it too much coupling between these blocs and maybe consider unify them to one NostrBloc. I need to see the NostrCommentBloc before I suggest anything.
If this is not an option we can always pass one bloc stream to another and this stream dependency can happen inside the bloc.
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.
Actually, it is to connect the signevent method and getpubkey method of the nostrBloc to the nostrCommentBloc . It might not be the best place to do this, I wanted to be able to fetch both nostrBloc and nostrCommentBloc in one place to be able to connect them.