-
Notifications
You must be signed in to change notification settings - Fork 5
Nip46 event signer #207
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
Nip46 event signer #207
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #207 +/- ##
==========================================
+ Coverage 71.23% 71.50% +0.26%
==========================================
Files 119 124 +5
Lines 4178 4393 +215
==========================================
+ Hits 2976 3141 +165
- Misses 1202 1252 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1-leo
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.
Interesting approach using a separate NDK instance. Actually has some benefits.
Have you thought about integrating it directly? Not possible?
I think it makes sense to move the code into the NDK package. We can still use a separate NDK if it makes sense architecturally.
I marked one problem with generateRandomString().
Thx for implementing!
|
When merging, we should also add a method to the accounts usecase to make it even more integrated. |
1-leo
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.
Noticed some minor issues. Overall, very solid!
We can fine-tune the API in another PR if you need this feaure soon.
On Wednesday we should discuss whether to make getPublicKeyAsync the 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.
Probably good to split it into several steps.
Like:
connectionSettings first time
- send login request
- auth with url param
- create and store connectionSettings
connectionSettings already stored
- read connection settings
- create signer
- login with ndk
In general what do you think about automating this further. Like providing connectionSettins storage and (semi)autoLogin if a connectionSetting is stored?
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 think it's a good idea to persist everything.
| @@ -0,0 +1,41 @@ | |||
| import 'package:ndk/shared/nips/nip01/helpers.dart'; | |||
|
|
|||
| enum BunkerRequestMethods { | |||
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.
you could use connect('connect') or getPublicKey('get_public_key') here, making methodToString redundant.
|
|
||
| late Bip340EventSigner localEventSigner; | ||
|
|
||
| Nip46EventSigner({required this.connectionSettings}) { |
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.
a good thing to have would be to pass in other EventVerifiers (like rust verifier) as an optional parameter
frnandu
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.
see #213
|
Nip46 mock remote signer and nip46 tests updated
No description provided.