Feat/nip55 external signer#657
Open
nogringo wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR replaces the Amber Flutter external signer integration with a new NIP-55 (standardized protocol) implementation across native Android plugin, Dart data layer, event signing, account persistence, UI controllers, and localization strings for 13 languages. ChangesNIP-55 External Signer Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #657 +/- ##
=======================================
Coverage 71.94% 71.95%
=======================================
Files 210 210
Lines 10698 10698
=======================================
+ Hits 7697 7698 +1
+ Misses 3001 3000 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/DartNdkPlugin.kt (1)
34-37:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCorrelate results per request instead of using one shared
_resultand request code0.With concurrent calls, Line 58 overwrites
_resultbefore prior activity responses arrive, and Line 36 uses a fixed request code (0), so responses can be delivered to the wrong Dart caller or dropped.Suggested direction
- private lateinit var _result: MethodChannel.Result - private val _intentRequestCode = 0 + private val _pendingResults = mutableMapOf<Int, MethodChannel.Result>() + private var _nextRequestCode = 10000 ... - _result = MethodResultWrapper(result) + val wrapped = MethodResultWrapper(result) ... - activity.startActivityForResult(intent, _intentRequestCode) + val requestCode = _nextRequestCode++ + _pendingResults[requestCode] = wrapped + activity.startActivityForResult(intent, requestCode) ... override fun onActivityResult(requestCode: Int, resultCode: Int, intent: Intent?): Boolean { - if (requestCode == _intentRequestCode) { + val pendingResult = _pendingResults.remove(requestCode) ?: return false + if (resultCode == Activity.RESULT_OK && intent != null) { ... - _result.success(dataMap) + pendingResult.success(dataMap) return true - } - return false + } + pendingResult.success(HashMap<String, String?>()) + return true }Also applies to: 58-59, 140-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/DartNdkPlugin.kt` around lines 34 - 37, The plugin currently stores a single shared _result and uses a fixed _intentRequestCode which causes races and mis-delivered responses; change to correlate each outbound intent with its own request id by introducing an AtomicInteger (or similar) to generate unique request codes and a concurrent map (e.g., MutableMap<Int, MethodChannel.Result>) that stores MethodChannel.Result instances keyed by that request code; when starting an activity use the generated code instead of _intentRequestCode and put the corresponding Result into the map, and in your activity result handler (the methods around where _result is referenced, including lines ~58-59 and ~140-165) look up and remove the Result from the map by requestCode before invoking success/error so each Dart caller receives the correct response and no results are overwritten or leaked.packages/sample-app/lib/amber_page.dart (1)
27-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle both
resultandsignaturekeys for NIP-55 public key responses.After switching to
Nip55Signer, Line 38 still reads onlyvalue['signature']. Some signer responses useresult, so this can leave_npubempty and breakNip19.decode.Suggested fix
- ).then((value) { - _npub = value['signature'] ?? ''; + ).then((value) { + _npub = (value['result'] ?? value['signature'] ?? '') as String; _pubkeyHex = Nip19.decode(_npub); setState(() { _text = '$value'; }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sample-app/lib/amber_page.dart` around lines 27 - 40, The onPressed handler currently extracts the NIP-55 public key from value['signature'] which can be empty for newer Nip55Signer responses that use value['result']; update the amber.getPublicKey .then callback to prefer value['result'] and fall back to value['signature'] (or vice versa) when assigning _npub, then continue to decode with Nip19.decode(_npub) and call setState as before; update references in this callback (amber.getPublicKey, Nip55Permission, _npub, _pubkeyHex, Nip19.decode) so _npub is always populated from either key before decoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ndk_flutter/lib/widgets/login/login_controller.dart`:
- Around line 102-107: The code currently calls signer.isAppInstalled() and, if
false, immediately opens the Amber GitHub URL (launchUrl) which hard-codes Amber
as the only acceptable NIP-55 signer; update the logic to detect any compliant
NIP-55 signer and avoid redirecting to Amber. Specifically, modify or extend the
signer/Nip55Signer API to either (a) expose the actually installed package id
(e.g., signer.getInstalledPackage() or signer.installedPackageName) or (b)
accept a list of known package IDs (e.g., isAppInstalled(List<String>
candidates)) and use that to determine installation; then change the fallback
from launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')) to a neutral
flow (show an in-app chooser, an installation help dialog, or open a
configurable signers discovery URL obtained from signer metadata) so that any
valid NIP-55 signer can be used and Amber is not hard-coded. Ensure changes
touch the isAppInstalled() call site and any redirect/launchUrl usage in
login_controller.dart and the Nip55Signer implementation.
- Around line 97-127: In loginWithExternalSigner, the flag
isWaitingForExternalSigner is set true but not guaranteed to be cleared if
signer.isAppInstalled() or signer.login() throws; wrap the main flow in a
try/finally (set isWaitingForExternalSigner = true before the try) and move the
existing "isWaitingForExternalSigner = false" into the finally block so it
always runs, preserving the current early-return logic (e.g., after launchUrl or
null loginResult) inside the try; update references to Nip55Signer,
Nip55EventSigner, ndk.accounts.loginExternalSigner, and loggedIn to remain
unchanged.
---
Outside diff comments:
In `@packages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/DartNdkPlugin.kt`:
- Around line 34-37: The plugin currently stores a single shared _result and
uses a fixed _intentRequestCode which causes races and mis-delivered responses;
change to correlate each outbound intent with its own request id by introducing
an AtomicInteger (or similar) to generate unique request codes and a concurrent
map (e.g., MutableMap<Int, MethodChannel.Result>) that stores
MethodChannel.Result instances keyed by that request code; when starting an
activity use the generated code instead of _intentRequestCode and put the
corresponding Result into the map, and in your activity result handler (the
methods around where _result is referenced, including lines ~58-59 and ~140-165)
look up and remove the Result from the map by requestCode before invoking
success/error so each Dart caller receives the correct response and no results
are overwritten or leaked.
In `@packages/sample-app/lib/amber_page.dart`:
- Around line 27-40: The onPressed handler currently extracts the NIP-55 public
key from value['signature'] which can be empty for newer Nip55Signer responses
that use value['result']; update the amber.getPublicKey .then callback to prefer
value['result'] and fall back to value['signature'] (or vice versa) when
assigning _npub, then continue to decode with Nip19.decode(_npub) and call
setState as before; update references in this callback (amber.getPublicKey,
Nip55Permission, _npub, _pubkeyHex, Nip19.decode) so _npub is always populated
from either key before decoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afda8421-ee26-45f4-8909-5d4cc23fb950
⛔ Files ignored due to path filters (1)
packages/sample-app/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
packages/ndk_flutter/android/build.gradlepackages/ndk_flutter/android/settings.gradlepackages/ndk_flutter/android/src/main/AndroidManifest.xmlpackages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/DartNdkPlugin.ktpackages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/Nip55Constants.ktpackages/ndk_flutter/lib/data_layer/data_sources/amber_flutter.dartpackages/ndk_flutter/lib/data_layer/data_sources/nip55_signer.dartpackages/ndk_flutter/lib/data_layer/repositories/signers/nip55_event_signer.dartpackages/ndk_flutter/lib/l10n/app_de.arbpackages/ndk_flutter/lib/l10n/app_en.arbpackages/ndk_flutter/lib/l10n/app_es.arbpackages/ndk_flutter/lib/l10n/app_fi.arbpackages/ndk_flutter/lib/l10n/app_fr.arbpackages/ndk_flutter/lib/l10n/app_it.arbpackages/ndk_flutter/lib/l10n/app_ja.arbpackages/ndk_flutter/lib/l10n/app_localizations.dartpackages/ndk_flutter/lib/l10n/app_localizations_de.dartpackages/ndk_flutter/lib/l10n/app_localizations_en.dartpackages/ndk_flutter/lib/l10n/app_localizations_es.dartpackages/ndk_flutter/lib/l10n/app_localizations_fi.dartpackages/ndk_flutter/lib/l10n/app_localizations_fr.dartpackages/ndk_flutter/lib/l10n/app_localizations_it.dartpackages/ndk_flutter/lib/l10n/app_localizations_ja.dartpackages/ndk_flutter/lib/l10n/app_localizations_pl.dartpackages/ndk_flutter/lib/l10n/app_localizations_pt.dartpackages/ndk_flutter/lib/l10n/app_localizations_ru.dartpackages/ndk_flutter/lib/l10n/app_localizations_zh.dartpackages/ndk_flutter/lib/l10n/app_pl.arbpackages/ndk_flutter/lib/l10n/app_pt.arbpackages/ndk_flutter/lib/l10n/app_pt_BR.arbpackages/ndk_flutter/lib/l10n/app_ru.arbpackages/ndk_flutter/lib/l10n/app_zh.arbpackages/ndk_flutter/lib/main/ndk_flutter.dartpackages/ndk_flutter/lib/models/accounts.dartpackages/ndk_flutter/lib/ndk_flutter.dartpackages/ndk_flutter/lib/ndk_platform_interface.dartpackages/ndk_flutter/lib/widgets/login/login_controller.dartpackages/ndk_flutter/lib/widgets/login/n_login.dartpackages/ndk_flutter/lib/widgets/pending_requests/n_pending_requests.dartpackages/ndk_flutter/pubspec.yamlpackages/sample-app/android/app/build.gradlepackages/sample-app/android/build.gradlepackages/sample-app/android/gradle.propertiespackages/sample-app/android/gradle/wrapper/gradle-wrapper.propertiespackages/sample-app/android/settings.gradlepackages/sample-app/lib/amber_page.dartpackages/sample-app/lib/main.dartpackages/sample-app/lib/widgets_demo_page.dartpackages/sample-app/pubspec.yaml
💤 Files with no reviewable changes (2)
- packages/ndk_flutter/lib/data_layer/data_sources/amber_flutter.dart
- packages/sample-app/pubspec.yaml
Comment on lines
+61
to
+64
| if (paramsMap == null) { | ||
| Log.d("onMethodCall", "paramsMap is null") | ||
| return | ||
| } |
There was a problem hiding this comment.
Complete the MethodChannel result on every exit path.
On Line 63, Line 116, and the non-OK branch in Line 141+, execution can exit without calling success/error/notImplemented, which leaves the Dart caller hanging indefinitely.
Suggested fix
val paramsMap = call.arguments as? HashMap<*, *>
if (paramsMap == null) {
Log.d("onMethodCall", "paramsMap is null")
+ _result.error("invalid_args", "Expected map arguments", null)
return
}
...
- try {
- _activity?.startActivityForResult(intent, _intentRequestCode)
+ val activity = _activity
+ if (activity == null) {
+ _result.success(HashMap<String, String?>())
+ return
+ }
+ try {
+ activity.startActivityForResult(intent, _intentRequestCode)
} catch (e: Exception) {
Log.d("onMethodCall", "startActivityForResult failed for '$signerPackage': ${e.message}")
_result.success(HashMap<String, String?>())
}
...
override fun onActivityResult(requestCode: Int, resultCode: Int, intent: Intent?): Boolean {
if (requestCode == _intentRequestCode) {
if (resultCode == Activity.RESULT_OK && intent != null) {
...
_result.success(dataMap)
return true
}
+ _result.success(HashMap<String, String?>())
+ return true
}
return false
}Also applies to: 116-120, 141-167
Comment on lines
+97
to
127
| Future<void> loginWithExternalSigner() async { | ||
| isWaitingForExternalSigner = true; | ||
|
|
||
| final amber = Amberflutter(); | ||
| const signer = Nip55Signer(); | ||
|
|
||
| final isAmberInstalled = await amber.isAppInstalled(); | ||
| final isInstalled = await signer.isAppInstalled(); | ||
|
|
||
| if (!isAmberInstalled) { | ||
| if (!isInstalled) { | ||
| isWaitingForExternalSigner = false; | ||
| launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')); | ||
| return; | ||
| } | ||
|
|
||
| final amberFlutterDS = AmberFlutterDS(amber); | ||
|
|
||
| final amberResponse = await amber.getPublicKey(); | ||
|
|
||
| final npub = amberResponse['signature']; | ||
| final pubkey = Nip19.decode(npub); | ||
| final loginResult = await signer.login(); | ||
| if (loginResult == null) { | ||
| isWaitingForExternalSigner = false; | ||
| return; | ||
| } | ||
|
|
||
| final amberSigner = AmberEventSigner( | ||
| publicKey: pubkey, | ||
| amberFlutterDS: amberFlutterDS, | ||
| final externalSigner = Nip55EventSigner( | ||
| publicKey: loginResult.pubkey, | ||
| // pin the signer captured at login so later requests can be silent | ||
| nip55Signer: Nip55Signer(package: loginResult.package), | ||
| ); | ||
|
|
||
| ndk.accounts.loginExternalSigner(signer: amberSigner); | ||
| ndk.accounts.loginExternalSigner(signer: externalSigner); | ||
|
|
||
| isWaitingForAmber = false; | ||
| isWaitingForExternalSigner = false; | ||
|
|
||
| await loggedIn(); | ||
| } |
There was a problem hiding this comment.
Reset isWaitingForExternalSigner in a finally block.
If signer.isAppInstalled() or signer.login() throws, Line 98 sets waiting state to true and it is never cleared, leaving the login button disabled until rebuild/restart.
Suggested fix
Future<void> loginWithExternalSigner() async {
- isWaitingForExternalSigner = true;
-
- const signer = Nip55Signer();
-
- final isInstalled = await signer.isAppInstalled();
-
- if (!isInstalled) {
- isWaitingForExternalSigner = false;
- launchUrl(Uri.parse('https://github.com/greenart7c3/Amber'));
- return;
- }
-
- final loginResult = await signer.login();
- if (loginResult == null) {
- isWaitingForExternalSigner = false;
- return;
- }
-
- final externalSigner = Nip55EventSigner(
- publicKey: loginResult.pubkey,
- // pin the signer captured at login so later requests can be silent
- nip55Signer: Nip55Signer(package: loginResult.package),
- );
-
- ndk.accounts.loginExternalSigner(signer: externalSigner);
-
- isWaitingForExternalSigner = false;
-
- await loggedIn();
+ isWaitingForExternalSigner = true;
+ try {
+ const signer = Nip55Signer();
+ final isInstalled = await signer.isAppInstalled();
+
+ if (!isInstalled) {
+ await launchUrl(Uri.parse('https://github.com/greenart7c3/Amber'));
+ return;
+ }
+
+ final loginResult = await signer.login();
+ if (loginResult == null) return;
+
+ final externalSigner = Nip55EventSigner(
+ publicKey: loginResult.pubkey,
+ nip55Signer: Nip55Signer(package: loginResult.package),
+ );
+
+ ndk.accounts.loginExternalSigner(signer: externalSigner);
+ await loggedIn();
+ } finally {
+ isWaitingForExternalSigner = false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Future<void> loginWithExternalSigner() async { | |
| isWaitingForExternalSigner = true; | |
| final amber = Amberflutter(); | |
| const signer = Nip55Signer(); | |
| final isAmberInstalled = await amber.isAppInstalled(); | |
| final isInstalled = await signer.isAppInstalled(); | |
| if (!isAmberInstalled) { | |
| if (!isInstalled) { | |
| isWaitingForExternalSigner = false; | |
| launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')); | |
| return; | |
| } | |
| final amberFlutterDS = AmberFlutterDS(amber); | |
| final amberResponse = await amber.getPublicKey(); | |
| final npub = amberResponse['signature']; | |
| final pubkey = Nip19.decode(npub); | |
| final loginResult = await signer.login(); | |
| if (loginResult == null) { | |
| isWaitingForExternalSigner = false; | |
| return; | |
| } | |
| final amberSigner = AmberEventSigner( | |
| publicKey: pubkey, | |
| amberFlutterDS: amberFlutterDS, | |
| final externalSigner = Nip55EventSigner( | |
| publicKey: loginResult.pubkey, | |
| // pin the signer captured at login so later requests can be silent | |
| nip55Signer: Nip55Signer(package: loginResult.package), | |
| ); | |
| ndk.accounts.loginExternalSigner(signer: amberSigner); | |
| ndk.accounts.loginExternalSigner(signer: externalSigner); | |
| isWaitingForAmber = false; | |
| isWaitingForExternalSigner = false; | |
| await loggedIn(); | |
| } | |
| Future<void> loginWithExternalSigner() async { | |
| isWaitingForExternalSigner = true; | |
| try { | |
| const signer = Nip55Signer(); | |
| final isInstalled = await signer.isAppInstalled(); | |
| if (!isInstalled) { | |
| await launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')); | |
| return; | |
| } | |
| final loginResult = await signer.login(); | |
| if (loginResult == null) return; | |
| final externalSigner = Nip55EventSigner( | |
| publicKey: loginResult.pubkey, | |
| nip55Signer: Nip55Signer(package: loginResult.package), | |
| ); | |
| ndk.accounts.loginExternalSigner(signer: externalSigner); | |
| await loggedIn(); | |
| } finally { | |
| isWaitingForExternalSigner = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk_flutter/lib/widgets/login/login_controller.dart` around lines 97
- 127, In loginWithExternalSigner, the flag isWaitingForExternalSigner is set
true but not guaranteed to be cleared if signer.isAppInstalled() or
signer.login() throws; wrap the main flow in a try/finally (set
isWaitingForExternalSigner = true before the try) and move the existing
"isWaitingForExternalSigner = false" into the finally block so it always runs,
preserving the current early-return logic (e.g., after launchUrl or null
loginResult) inside the try; update references to Nip55Signer, Nip55EventSigner,
ndk.accounts.loginExternalSigner, and loggedIn to remain unchanged.
Comment on lines
+102
to
107
| final isInstalled = await signer.isAppInstalled(); | ||
|
|
||
| if (!isAmberInstalled) { | ||
| if (!isInstalled) { | ||
| isWaitingForExternalSigner = false; | ||
| launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')); | ||
| return; |
There was a problem hiding this comment.
Hard-coded install gate restricts “signer app” login to Amber package only.
Line 102 uses Nip55Signer.isAppInstalled(), and the upstream implementation checks only com.greenart7c3.nostrsigner. If a different NIP-55 signer is installed, this path fails and Line 106 redirects to Amber, blocking valid signer-app logins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk_flutter/lib/widgets/login/login_controller.dart` around lines
102 - 107, The code currently calls signer.isAppInstalled() and, if false,
immediately opens the Amber GitHub URL (launchUrl) which hard-codes Amber as the
only acceptable NIP-55 signer; update the logic to detect any compliant NIP-55
signer and avoid redirecting to Amber. Specifically, modify or extend the
signer/Nip55Signer API to either (a) expose the actually installed package id
(e.g., signer.getInstalledPackage() or signer.installedPackageName) or (b)
accept a list of known package IDs (e.g., isAppInstalled(List<String>
candidates)) and use that to determine installation; then change the fallback
from launchUrl(Uri.parse('https://github.com/greenart7c3/Amber')) to a neutral
flow (show an in-app chooser, an installation help dialog, or open a
configurable signers discovery URL obtained from signer metadata) so that any
valid NIP-55 signer can be used and Amber is not hard-coded. Ensure changes
touch the isAppInstalled() call site and any redirect/launchUrl usage in
login_controller.dart and the Nip55Signer implementation.
Collaborator
Author
|
Metioned in nip55 and not implemented:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features