Feat/nip55 external signer#657
Open
nogringo wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR replaces Amber Flutter with NIP-55 external signer protocol. Changes span Android native plugin (Kotlin/Gradle), Dart bridge (MethodChannel data models), event signer repository, account persistence, login UI, localization (13 locales), sample app, and build configuration. ChangesNIP-55 External Signer Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
⚔️ Resolve merge conflicts
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.90% -0.05%
==========================================
Files 210 210
Lines 10698 10698
==========================================
- Hits 7697 7692 -5
- Misses 3001 3006 +5 ☔ 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
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:
|
1-leo
approved these changes
Jun 12, 2026
1-leo
left a comment
1-leo
left a comment
Contributor
There was a problem hiding this comment.
tested and looks good!
Because its a braking change we should publish a migration guide in our docs (basically just renaming stuff) wdyt?
Also amberflutter is still mentioned in some codedocs and skills.md
frnandu
requested changes
Jun 12, 2026
Collaborator
There was a problem hiding this comment.
-
Sample app AmberPage class name -- The class is still called AmberPage and the variable is amber. Since this is now generic, renaming to SignerAppPage / signerApp would be cleaner
-
isAppInstalled hardcodes Amber's package name
- nip55_signer.dart:77 always checks com.greenart7c3.nostrsigner. The whole point of this PR is to be signer-generic, yet install detec
tion is Amber-specific. If the user has Primal/Aegis but not Amber, isAppInstalled() returns false, and the UI redirects to Amber's GitHub ins
tead of letting them proceed.
- The Kotlin side has isExternalSignerInstalled() which checks the nostrsigner: scheme (generic), but the Dart side doesn't call it wit
h packageName: null -- it always passes the Amber package. -
isAppInstalled logic in Kotlin has surprising OR
- DartNdkPlugin.kt:186: isPackageInstalled(...) || isExternalSignerInstalled(_context) -- if the specific package is not installed but ANY external signer is, this returns true. For isAppInstalled called with a specific package, this seems misleading. It may cause the UI to think a specific signer is available when it's actually a different one.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ndk_flutter/lib/main/ndk_flutter.dart (1)
244-249:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd logging for silently suppressed account-switch errors.
The
try/catchimproves startup resilience by preventing crashes when the stored logged account cannot be restored, butcatch (_)silently suppresses all errors with no logging. This makes debugging restoration failures in production difficult.Consider adding a log statement to capture these errors for debugging:
📊 Proposed logging addition
try { ndk.accounts.switchAccount(pubkey: accounts.loggedAccount!); - } catch (_) { + } catch (e) { // stored logged account could not be restored (e.g. stale/corrupted // state); ignore rather than crash app startup. + debugPrint('Failed to restore logged account: $e'); }🤖 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/main/ndk_flutter.dart` around lines 244 - 249, The catch block around ndk.accounts.switchAccount(pubkey: accounts.loggedAccount!) currently suppresses all errors; update it to log the caught exception and optionally the stacktrace so restoration failures are observable in production (e.g., use the existing logger or at minimum print) while still swallowing the error to avoid crashing; reference ndk.accounts.switchAccount and accounts.loggedAccount when adding the log entry.packages/sample-app/lib/nip55_signer_page.dart (2)
119-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove redundant decrypt calls.
The "Nip 04 Decrypt" button makes three identical
nip04Decryptcalls (lines 120, 132, 144) with the same parameters, differing only in the display suffix ("1", "2", "3"). This appears to be leftover debug/test code.For a sample app, one call is sufficient. The redundant calls waste resources and may confuse developers learning from the example.
🧹 Suggested cleanup
FilledButton( onPressed: () async { nip55Signer .nip04Decrypt( ciphertext: _cipherText, currentUser: _npub, pubKey: _pubkeyHex, ) .then((value) { setState(() { - _text = '$value 1'; + _text = '$value'; }); }); - // ; - nip55Signer - .nip04Decrypt( - ciphertext: _cipherText, - currentUser: _npub, - pubKey: _pubkeyHex, - ) - .then((value) { - setState(() { - _text = '$value 2'; - }); - }); - // , - nip55Signer - .nip04Decrypt( - ciphertext: _cipherText, - currentUser: _npub, - pubKey: _pubkeyHex, - ) - .then((value) { - setState(() { - _text = '$value 3'; - }); - }); }, child: const Text('Nip 04 Decrypt'), ),🤖 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/nip55_signer_page.dart` around lines 119 - 157, Remove the redundant duplicate nip55Signer.nip04Decrypt calls in the onPressed handler and keep a single asynchronous call that uses the existing parameters (ciphertext: _cipherText, currentUser: _npub, pubKey: _pubkeyHex); when it completes call setState to update _text with the decrypted value (remove the extra calls that append "1" and "2"/"3" so only one nip04Decrypt invocation remains in the onPressed block).
28-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequest all necessary permissions at login for silent operation.
The sample only requests
nip04_encryptandnip04_decryptpermissions, but later buttons invokesignEvent(lines 60, 86),nip44Encrypt(line 160), andnip44Decrypt(line 177) without pre-authorizing those operations. This means every sign/nip44 request will require user interaction instead of being answered silently via ContentResolver.For a sample app demonstrating best practices, consider using
Nip55Signer.defaultPermissionsor explicitly including all operations used in the demo.🔐 Suggested fix to request complete permissions
nip55Signer.getPublicKey( permissions: [ + const Nip55Permission( + type: "sign_event", + ), const Nip55Permission( type: "nip04_encrypt", ), const Nip55Permission( type: "nip04_decrypt", ), + const Nip55Permission( + type: "nip44_encrypt", + ), + const Nip55Permission( + type: "nip44_decrypt", + ), ],Alternatively, use the default set:
- nip55Signer.getPublicKey( - permissions: [ - const Nip55Permission( - type: "nip04_encrypt", - ), - const Nip55Permission( - type: "nip04_decrypt", - ), - ], - ).then((value) { + nip55Signer.login().then((result) { + if (result == null) return; + _npub = 'npub...'; // or extract from result.pubkey + _pubkeyHex = result.pubkey; setState(() { - _text = '$value'; + _text = 'Logged in: ${result.pubkey}'; }); });🤖 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/nip55_signer_page.dart` around lines 28 - 36, The code only requests nip04_encrypt and nip04_decrypt in nip55Signer.getPublicKey which causes later calls (signEvent, nip44Encrypt, nip44Decrypt) to prompt the user; update the permission request to include all operations used in the demo by either replacing the explicit Nip55Permission list with Nip55Signer.defaultPermissions or adding explicit Nip55Permission entries for signEvent (signing) and nip44Encrypt/nip44Decrypt (nip44_encrypt/nip44_decrypt) so that nip55Signer.getPublicKey is called with the full set of required permissions for silent operation.
🧹 Nitpick comments (2)
packages/sample-app/lib/nip55_signer_page.dart (2)
28-43: 💤 Low valueConsider using consistent async patterns.
This button uses
.then()callback style, while the "Verify signature" button (line 86) usesawait. For consistency and readability in a sample app, prefer one pattern throughout—modern Dart/Flutter code typically favorsasync/await.🤖 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/nip55_signer_page.dart` around lines 28 - 43, The nip55Signer.getPublicKey call uses .then() while other handlers use async/await; change the button handler to be async, await nip55Signer.getPublicKey(...), assign _npub and _pubkeyHex (using Nip19.decode) and call setState() afterward, and wrap the await in try/catch to handle errors consistently—update the code that references nip55Signer.getPublicKey, _npub, _pubkeyHex, and setState to follow this async/await pattern.
19-19: ⚡ Quick winConsider demonstrating the package-targeting pattern.
The sample instantiates
const Nip55Signer()without a package, which means every request will let Android route to any compatible signer. The recommended NIP-55 pattern is to capture thepackagefrom the login result and create a newNip55Signer(package: capturedPackage)instance, enabling silent ContentResolver-based answers for pre-authorized permissions.This would better demonstrate the optimal integration flow for developers using this sample.
🤖 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/nip55_signer_page.dart` at line 19, Replace the unconditional const Nip55Signer() instantiation with a package-targeted signer by capturing the package identifier from the login result and creating Nip55Signer(package: capturedPackage) (use the same variable name nip55Signer or replace its declaration where it's defined) so subsequent requests use the package-specific signer and enable silent ContentResolver-based responses for pre-authorized permissions.
🤖 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.
Outside diff comments:
In `@packages/ndk_flutter/lib/main/ndk_flutter.dart`:
- Around line 244-249: The catch block around ndk.accounts.switchAccount(pubkey:
accounts.loggedAccount!) currently suppresses all errors; update it to log the
caught exception and optionally the stacktrace so restoration failures are
observable in production (e.g., use the existing logger or at minimum print)
while still swallowing the error to avoid crashing; reference
ndk.accounts.switchAccount and accounts.loggedAccount when adding the log entry.
In `@packages/sample-app/lib/nip55_signer_page.dart`:
- Around line 119-157: Remove the redundant duplicate nip55Signer.nip04Decrypt
calls in the onPressed handler and keep a single asynchronous call that uses the
existing parameters (ciphertext: _cipherText, currentUser: _npub, pubKey:
_pubkeyHex); when it completes call setState to update _text with the decrypted
value (remove the extra calls that append "1" and "2"/"3" so only one
nip04Decrypt invocation remains in the onPressed block).
- Around line 28-36: The code only requests nip04_encrypt and nip04_decrypt in
nip55Signer.getPublicKey which causes later calls (signEvent, nip44Encrypt,
nip44Decrypt) to prompt the user; update the permission request to include all
operations used in the demo by either replacing the explicit Nip55Permission
list with Nip55Signer.defaultPermissions or adding explicit Nip55Permission
entries for signEvent (signing) and nip44Encrypt/nip44Decrypt
(nip44_encrypt/nip44_decrypt) so that nip55Signer.getPublicKey is called with
the full set of required permissions for silent operation.
---
Nitpick comments:
In `@packages/sample-app/lib/nip55_signer_page.dart`:
- Around line 28-43: The nip55Signer.getPublicKey call uses .then() while other
handlers use async/await; change the button handler to be async, await
nip55Signer.getPublicKey(...), assign _npub and _pubkeyHex (using Nip19.decode)
and call setState() afterward, and wrap the await in try/catch to handle errors
consistently—update the code that references nip55Signer.getPublicKey, _npub,
_pubkeyHex, and setState to follow this async/await pattern.
- Line 19: Replace the unconditional const Nip55Signer() instantiation with a
package-targeted signer by capturing the package identifier from the login
result and creating Nip55Signer(package: capturedPackage) (use the same variable
name nip55Signer or replace its declaration where it's defined) so subsequent
requests use the package-specific signer and enable silent ContentResolver-based
responses for pre-authorized permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e0cc9a2-67f4-45f7-bec5-fed6f3a47681
📒 Files selected for processing (7)
packages/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/nip55_signer.dartpackages/ndk_flutter/lib/data_layer/repositories/signers/nip55_event_signer.dartpackages/ndk_flutter/lib/main/ndk_flutter.dartpackages/sample-app/README.mdpackages/sample-app/lib/nip55_signer_page.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ndk_flutter/lib/data_layer/repositories/signers/nip55_event_signer.dart
- packages/ndk_flutter/lib/data_layer/data_sources/nip55_signer.dart
- packages/ndk_flutter/android/src/main/kotlin/relaystr/ndk/DartNdkPlugin.kt
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
3 participants
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
UI Updates
Infrastructure