Amber integration #48
Conversation
WalkthroughAdds NIP-55 (Amber) external signer support across Android native, Flutter/Dart, and Rust: Android plugin and manifest query, Dart service/hook and UI/provider wiring, FRB bindings for Dart-backed signer callbacks, and Rust signer/account APIs including external-account login and AccountType. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FlutterUI as Flutter UI (HomeScreen)
participant AmberHook as useAmber / AmberSignerService
participant AndroidPlugin as Android Plugin (AmberSignerPlugin)
participant AmberApp as Amber App
participant RustBackend as Rust Backend (signer_api)
User->>FlutterUI: Tap "Login with Amber"
FlutterUI->>AmberHook: connect() / isAvailable?
AmberHook-->>FlutterUI: available=true
FlutterUI->>AmberHook: getPublicKey(permissions)
AmberHook->>AndroidPlugin: platform channel getPublicKey
AndroidPlugin->>AmberApp: Launch Intent (nostrsigner://...) / ContentResolver
AmberApp-->>AndroidPlugin: Return pubkey + packageName
AndroidPlugin-->>AmberHook: { result, packageName, id, event? }
AmberHook-->>FlutterUI: pubkey
FlutterUI->>RustBackend: loginWithExternalSignerAndCallbacks(pubkey, callbacks)
RustBackend->>RustBackend: Create DartSigner (wrap callbacks)
RustBackend->>RustBackend: Create account, register signer, publish key-package
RustBackend-->>FlutterUI: Account (pubkey, accountType=external)
FlutterUI->>User: Navigate to ChatList
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt`:
- Around line 181-190: Replace the explicit size check in
isExternalSignerInstalled() with Kotlin's idiomatic collection check: instead of
returning infos.size > 0, return infos.isNotEmpty(); update the return
expression in the isExternalSignerInstalled function that uses the infos result
from context.packageManager.queryIntentActivities(intent, 0) to use
isNotEmpty().
- Around line 212-218: Replace the broad Exception catch around
currentActivity.startActivityForResult(intent, REQUEST_CODE_SIGNER) with a more
specific handler: catch android.content.ActivityNotFoundException first and call
pendingResult = null and result.error("LAUNCH_ERROR", "Signer activity not
found: " + e.message, null), then add a narrower fallback catch for other
RuntimeException/Throwable if you still want a safety net; ensure you reference
REQUEST_CODE_SIGNER, pendingResult and result.error in the handlers so error
semantics are clearer.
- Around line 311-316: The catch block in AmberSignerPlugin.kt that swallows
Exception when trying the ContentResolver should log the caught exception for
debugging; update the catch (e: Exception) in the method that resolves URIs (the
exception handling inside AmberSignerPlugin class, e.g., the content-resolver
URI resolution function) to call the Android logger with the exception (e.g.,
Log.e(TAG, "ContentResolver lookup failed, falling back to Intent", e) or use
the existing plugin logger) so the error message and stack trace are recorded
while preserving the current fallback behavior.
- Around line 460-465: The catch block in AmberSignerPlugin.kt that swallows
Exception (in the method handling ContentResolver fallback) should log the
exception for debugging: add a Log.e(TAG, "ContentResolver fallback failed", e)
(or use the plugin's logger if present) inside the catch (e: Exception) { ... }
so the thrown exception is recorded before returning null; reference the catch
block in the AmberSignerPlugin class and the surrounding method that attempts to
use the ContentResolver.
In `@lib/providers/auth_provider.dart`:
- Around line 62-136: The loginWithAmber method doesn't disconnect Amber if an
unexpected exception occurs after amberNotifier.connect(); update the try/catch
to ensure amberNotifier.disconnect() is called in all failure cases: either add
a finally block that calls await amberNotifier.disconnect() when login did not
complete successfully, or set a success flag (e.g., loggedIn) and call
disconnect in finally if not successful; modify loginWithAmber (and the existing
ApiError and AmberSignerException handlers) to avoid double-disconnects by only
disconnecting in the common cleanup path while keeping the current rethrows.
- Around line 138-147: The getLoginMethod() function silently falls back to
LoginMethod.nsec when the stored value is unrecognized; update it to detect when
orElse is used and log a warning containing the raw stored value and
_loginMethodKey so corruption or future-version values are visible. Use the same
secureStorageProvider/ref.read call to obtain the raw method string, and before
returning the fallback from getLoginMethod(), emit a warning (via your app
logger) that includes the unknown value and that nsec is being used as a
fallback.
In `@lib/screens/home_screen.dart`:
- Around line 35-43: Replace the generic catch that sets amberError.value =
e.toString() with logic that maps unexpected exceptions to a non-technical,
user-friendly message (e.g., "An unexpected error occurred. Please try again.")
while still logging the original error for debugging; update the catch block
that follows the AmberSignerException handling (the one using amberError and
isAmberLoading) to set amberError.value to the friendly message and ensure the
original exception is recorded via a logger or debug print rather than shown to
the user.
In `@lib/services/amber_signer_service.dart`:
- Around line 118-120: The log call in AmberSignerResponse.fromMap handling can
throw a RangeError when response.result is null or shorter than 8 characters;
update the logging in the method that calls AmberSignerResponse.fromMap so it
safely previews the key (check for null/empty and use response.result.length to
take min(8, length) or a conditional fallback like '<invalid>' before calling
substring), e.g. around the place that currently does _logger.info('Got public
key from signer: ${response.result?.substring(0, 8)}...') replace with a guarded
preview using response.result and _logger.info to avoid calling substring on
too-short or null strings.
In `@rust/src/api/signer.rs`:
- Around line 69-88: The Dart callback futures (e.g., the cloned sign function
in sign_event) return Result<String, E> but your code awaits them as if they
always succeed, silently dropping Dart errors; update sign_event (and the other
callback-using methods nip04_encrypt, nip04_decrypt, nip44_encrypt,
nip44_decrypt) to match on the awaited callback result and convert any Err into
SignerError::backend (for example: let signed_json = match
sign_fn(unsigned_json).await { Ok(json) => json, Err(e) => return
Err(SignerError::backend(e)), };), then continue parsing the JSON on Ok.
| /// Gets the current login method. | ||
| Future<LoginMethod?> getLoginMethod() async { | ||
| final storage = ref.read(secureStorageProvider); | ||
| final method = await storage.read(key: _loginMethodKey); | ||
| if (method == null) return null; | ||
| return LoginMethod.values.firstWhere( | ||
| (e) => e.name == method, | ||
| orElse: () => LoginMethod.nsec, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent fallback to nsec may mask storage corruption.
If the stored login method is an unrecognized value (perhaps from a future version or corruption), the code silently defaults to nsec. Consider logging a warning when the orElse branch is taken.
♻️ Suggested improvement
return LoginMethod.values.firstWhere(
(e) => e.name == method,
- orElse: () => LoginMethod.nsec,
+ orElse: () {
+ _logger.warning('Unknown login method in storage: $method, defaulting to nsec');
+ return LoginMethod.nsec;
+ },
);🤖 Prompt for AI Agents
In `@lib/providers/auth_provider.dart` around lines 138 - 147, The
getLoginMethod() function silently falls back to LoginMethod.nsec when the
stored value is unrecognized; update it to detect when orElse is used and log a
warning containing the raw stored value and _loginMethodKey so corruption or
future-version values are visible. Use the same secureStorageProvider/ref.read
call to obtain the raw method string, and before returning the fallback from
getLoginMethod(), emit a warning (via your app logger) that includes the unknown
value and that nsec is being used as a fallback.
rust/src/api/signer.rs
Outdated
| fn sign_event( | ||
| &self, | ||
| unsigned: UnsignedEvent, | ||
| ) -> BoxedFuture<'_, Result<Event, SignerError>> { | ||
| let sign_fn = self.sign_event.clone(); | ||
| Box::pin(async move { | ||
| // Serialize the unsigned event to JSON | ||
| let unsigned_json = serde_json::to_string(&unsigned) | ||
| .map_err(SignerError::backend)?; | ||
|
|
||
| // Call the Dart callback to sign | ||
| let signed_json = sign_fn(unsigned_json).await; | ||
|
|
||
| // Parse the signed event | ||
| let event: Event = serde_json::from_str(&signed_json) | ||
| .map_err(SignerError::backend)?; | ||
|
|
||
| Ok(event) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
flutter_rust_bridge DartFnFuture error handling Dart exceptions to Rust
💡 Result:
Short answer
- When Rust calls a Dart callback via a DartFnFuture and the Dart callback throws, the generated bridge turns that into an error visible to Rust when you .await the DartFnFuture. Handle it on the Rust side by matching the awaited result and converting/propagating the error (or reporting it with a custom ErrorHandler). (cjycode.com)
What to do (example)
-
Await the callback and match the result; convert it into your return Result or anyhow::Error:
pub async fn rust_fn(cb: impl Fn(String) -> DartFnFuture) -> anyhow::Result {
match cb("hi".into()).await {
Ok(s) => Ok(s),
Err(e) => Err(anyhow::anyhow!("Dart callback failed: {}", e)),
}
} -
Or if you want global handling/reporting, implement a custom Handler / ErrorHandler to capture and forward Dart errors from the bridge. (cjycode.com)
Notes / pointers
- The bridge converts Rust Results/anyhow::Error to Dart exceptions and converts uncaught Dart exceptions into an error visible to Rust when a DartFnFuture resolves; treat it like any other async error and propagate or handle accordingly. (cjycode.com)
If you want, tell me which generated Rust signature you have and I’ll give an exact snippet to handle the Dart exception and preserve stack/message information.
Citations:
- 1: https://cjycode.com/flutter_rust_bridge/guides/direction/rust-call-dart?utm_source=openai
- 2: https://cjycode.com/flutter_rust_bridge/guides/direction/rust-call-dart?utm_source=openai
- 3: https://cjycode.com/flutter_rust_bridge/guides/types/translatable/return?utm_source=openai
🏁 Script executed:
# Check the repository structure and find FRB-related code
git ls-files | grep -E "(Cargo|bridge|dart)" | head -20Repository: marmot-protocol/sloth
Length of output: 667
🏁 Script executed:
# Search for DartFnFuture definition and usage
rg "DartFnFuture" --type rustRepository: marmot-protocol/sloth
Length of output: 2004
🏁 Script executed:
# Check the signer.rs file context to understand the callback types
cat -n rust/src/api/signer.rs | head -100Repository: marmot-protocol/sloth
Length of output: 4521
🏁 Script executed:
# Get the complete signer.rs file to see all methods and error handling
cat -n rust/src/api/signer.rs | sed -n '90,145p'Repository: marmot-protocol/sloth
Length of output: 2273
Dart callback errors are silently suppressed instead of being properly converted to SignerError.
DartFnFuture<String> returns a Result<String, E> which must be matched and handled on the Rust side. The current code treats the awaited value as a bare String, silently discarding any Dart callback errors. This affects all callback methods: sign_event (line 80), nip04_encrypt (line 99), nip04_decrypt (line 113), nip44_encrypt (line 127), and nip44_decrypt (line 141).
Wrap each callback invocation in a match statement to convert Dart errors to SignerError:
let signed_json = match sign_fn(unsigned_json).await {
Ok(json) => json,
Err(e) => return Err(SignerError::backend(e)),
};🤖 Prompt for AI Agents
In `@rust/src/api/signer.rs` around lines 69 - 88, The Dart callback futures
(e.g., the cloned sign function in sign_event) return Result<String, E> but your
code awaits them as if they always succeed, silently dropping Dart errors;
update sign_event (and the other callback-using methods nip04_encrypt,
nip04_decrypt, nip44_encrypt, nip44_decrypt) to match on the awaited callback
result and convert any Err into SignerError::backend (for example: let
signed_json = match sign_fn(unsigned_json).await { Ok(json) => json, Err(e) =>
return Err(SignerError::backend(e)), };), then continue parsing the JSON on Ok.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt`:
- Around line 202-214: The code overwrites pendingResult and currentRequestId
when a new signer request starts while one is already in-flight; add an
in-flight guard at the start of the request handlers (the blocks that set
pendingResult, currentRequestId and call currentActivity.startActivityForResult
with REQUEST_CODE_SIGNER) that checks if pendingResult or currentRequestId is
non-null and, if so, immediately returns an explicit error to the caller (e.g.,
call result.error with a clear "request_in_flight" code/message) instead of
proceeding; ensure the guard exists in the same places as shown (the block using
Intent + startActivityForResult and the other similar blocks around the later
ranges) so you never overwrite pendingResult/currentRequestId or start a new
activity while one is pending.
♻️ Duplicate comments (3)
lib/providers/auth_provider.dart (1)
143-151: Handle unknown login methods without crashing.Throwing a
StateErrorhere can break startup/login flows if storage contains a future/invalid value. Prefer logging + clearing the key and returning null (or a safe default) to keep the app usable.🛠️ Proposed fix
- final loginMethod = LoginMethod.values.where((e) => e.name == method).firstOrNull; - if (loginMethod == null) { - throw StateError('Unknown login method in storage: $method'); - } - return loginMethod; + final loginMethod = LoginMethod.values.where((e) => e.name == method).firstOrNull; + if (loginMethod == null) { + _logger.warning('Unknown login method in storage: $method. Clearing $_loginMethodKey.'); + await storage.delete(key: _loginMethodKey); + return null; + } + return loginMethod;lib/screens/home_screen.dart (1)
35-38: Avoid surfacing raw Amber error details to users.
'Amber error: ${e.message}'can expose internal details. Map to a friendly message and keep technical details in logs.🛠️ Proposed fix
- amberError.value = e.code == 'USER_REJECTED' - ? 'Login cancelled' - : 'Amber error: ${e.message}'; + amberError.value = e.code == 'USER_REJECTED' + ? 'Login cancelled' + : 'Unable to connect to Amber. Please try again.';Based on learnings, user-facing errors should be friendly and non-technical.
lib/services/amber_signer_service.dart (1)
119-121: Null-safe pubkey preview logging.
response.result!is evaluated insidemin(...)before the null-aware call, so a null response throws unexpectedly and bypasses the intended error handling. Validate first, then compute the preview.🛠️ Proposed fix
- final response = AmberSignerResponse.fromMap(result); - final preview = response.result?.substring(0, min(8, response.result!.length)) ?? ''; - _logger.info('Got public key from signer: $preview...'); - - if (response.result == null || response.result!.isEmpty) { - throw const AmberSignerException('NO_PUBKEY', 'Signer did not return a public key'); - } - - return response.result!; + final response = AmberSignerResponse.fromMap(result); + final pubkey = response.result; + if (pubkey == null || pubkey.isEmpty) { + throw const AmberSignerException('NO_PUBKEY', 'Signer did not return a public key'); + } + + final preview = pubkey.substring(0, min(8, pubkey.length)); + _logger.info('Got public key from signer: $preview...'); + return pubkey;
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt`:
- Around line 533-543: getSignerPackageName() and setSignerPackageName()
currently use activity which may be null; switch them to use the plugin's
application context field instead so SharedPreferences persist and won't
silently fail during configuration/background changes. Locate the two functions
(getSignerPackageName and setSignerPackageName) and replace usage of activity
with the existing application context variable (the plugin's context), keeping
the same preference name "amber_signer_prefs" and key "signer_package_name" and
preserving the getString and prefs.edit().putString(...).apply() behavior.
- Around line 411-420: The intent creation here is missing URI-encoding for
content and treats the "id" extra inconsistently compared to signEvent; update
the Intent construction to use a URI-encoded content (e.g., wrap content with
Uri.encode(...)) when building Uri.parse("$NOSTRSIGNER_SCHEME:$content"), and
always call intent.putExtra("id", id) (remove the conditional) so the "id" extra
is present even when empty to match signEvent behavior; keep the current_user
conditional as-is. Use the symbols NOSTRSIGNER_SCHEME, Uri.parse/Uri.encode, and
intent.putExtra("id", ...) to locate and modify the code in
AmberSignerPlugin.kt.
- Around line 266-273: The intent URI is built with raw eventJson which can
contain reserved chars; encode the payload per NIP-55 by applying Android's
RFC-3986 percent-encoding before parsing the URI. In AmberSignerPlugin.kt update
the intent creation to use Uri.encode(eventJson) (i.e., build the Uri with
"$NOSTRSIGNER_SCHEME:${Uri.encode(eventJson)}") while keeping the same intent
extras ("type", "id", "current_user") and flags; this ensures the
NOSTRSIGNER_SCHEME payload is properly percent-encoded.
android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt`:
- Around line 181-189: The isExternalSignerInstalled function is shadowing the
class-level context by using activity ?: return false, causing false negatives
when activity is null; change it to use the class-level application context
property (the existing context field) instead of activity, build the Intent the
same way and call context.packageManager.queryIntentActivities(...), ensuring
you don't shadow the context variable and that the method returns
infos.isNotEmpty() as before.
♻️ Duplicate comments (1)
android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt (1)
411-420: Keepidextras consistent across signer intents.
signEventalways includes theidextra, butlaunchSignerIntentomits it when empty. Aligning behavior avoids inconsistent signer handling.♻️ Proposed fix
- if (id.isNotEmpty()) { - intent.putExtra("id", id) - } + intent.putExtra("id", id)
| private fun isExternalSignerInstalled(): Boolean { | ||
| val context = activity ?: return false | ||
| val intent = | ||
| Intent().apply { | ||
| action = Intent.ACTION_VIEW | ||
| data = Uri.parse("$NOSTRSIGNER_SCHEME:") | ||
| } | ||
| val infos = context.packageManager.queryIntentActivities(intent, 0) | ||
| return infos.isNotEmpty() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full file structure and find the relevant methods
wc -l android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 135
🏁 Script executed:
# Read the file to understand the class structure and context
cat -n android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt | head -50Repository: marmot-protocol/sloth
Length of output: 2418
🏁 Script executed:
# Check around lines 181-189 for the isExternalSignerInstalled method
cat -n android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt | sed -n '175,195p'Repository: marmot-protocol/sloth
Length of output: 780
🏁 Script executed:
# Check if there's a context property in the class
cat -n android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt | grep -n "context" | head -20Repository: marmot-protocol/sloth
Length of output: 564
🏁 Script executed:
# Check the launchSignerIntent method around lines 411-420
cat -n android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt | sed -n '405,425p'Repository: marmot-protocol/sloth
Length of output: 932
🏁 Script executed:
# Search for signEvent method and how id is handled in different places
rg "signEvent|putExtra.*id" android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt -n -A 2 -B 2Repository: marmot-protocol/sloth
Length of output: 996
🏁 Script executed:
# Get broader view around the methods that use context
cat -n android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt | sed -n '290,300p'Repository: marmot-protocol/sloth
Length of output: 505
Use application context for signer availability checks.
The method currently shadows the class-level context property with activity ?: return false at line 182. This causes a false negative when the activity is null during background operations or lifecycle changes, even though the class maintains an application context that remains available. Align with the pattern used elsewhere in the codebase (lines 293, 443, 534, 540) by using the application context instead.
♻️ Proposed fix
private fun isExternalSignerInstalled(): Boolean {
- val context = activity ?: return false
+ val ctx = context ?: return false
val intent =
Intent().apply {
action = Intent.ACTION_VIEW
data = Uri.parse("$NOSTRSIGNER_SCHEME:")
}
- val infos = context.packageManager.queryIntentActivities(intent, 0)
+ val infos = ctx.packageManager.queryIntentActivities(intent, 0)
return infos.isNotEmpty()
}🤖 Prompt for AI Agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt` around
lines 181 - 189, The isExternalSignerInstalled function is shadowing the
class-level context by using activity ?: return false, causing false negatives
when activity is null; change it to use the class-level application context
property (the existing context field) instead of activity, build the Intent the
same way and call context.packageManager.queryIntentActivities(...), ensuring
you don't shadow the context variable and that the method returns
infos.isNotEmpty() as before.
|
@dannym-arx I realized that last commits have no CI checks beside code rabbit's... can you please rebase master and push to trigger CI checks? I don't really know why checks didn't run... it's weird |
|
@josefinalliende I'll do that tomorrow morning! |
dannym-arx
force-pushed
the
amber-integration
branch
from
a4bcb48 to
578cce4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/Cargo.toml (1)
1-7: Address CI failure: Rust formatting.The pipeline reports a Rust fmt check failure. Run the following to fix:
cd rust && cargo fmtThen commit the formatted files.
🤖 Fix all issues with AI agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt`:
- Around line 306-310: The cursor handling currently returns null if the
"rejected" column exists rather than checking its value; inside the cursor?.use
block in AmberSignerPlugin (the query/result-processing code that reads the
"rejected" column) first get the column index for "rejected", and if index >= 0
then read the column value (use getInt or getString per your schema) and only
return null when that value indicates rejection (e.g., 1 or "true"); otherwise
continue normal processing. Apply the same change to the other cursor?.use
occurrence that currently checks only existence (the one around the second
occurrence noted in the review).
In `@lib/providers/amber_provider.dart`:
- Around line 149-224: Create a centralized mapper that converts
AmberSignerException codes into user-facing messages (e.g., implement a function
like mapAmberSignerError(String code) or add a userMessage getter on
AmberSignerException) that covers USER_REJECTED -> "Login cancelled",
NOT_CONNECTED -> "Not connected to Amber", NO_RESPONSE -> "No response from
Amber", NO_PUBKEY -> "Missing public key", NO_RESULT -> "No result returned",
NO_EVENT -> "No event returned" and a sensible fallback for unknown codes; then
update the UI error handling (where AmberSignerException is caught, e.g., in
home_screen.dart) to pass exception.code into this mapper and display the
returned friendly string instead of the raw technical message.
- Around line 86-103: The permission list passed to service.getPublicKey in
amber_provider.dart is missing NIP-04 permissions, so add AmberPermission
entries for nip04_encrypt and nip04_decrypt to the permissions array (alongside
the existing nip44 entries) so Amber is granted NIP-04 encryption/decryption
upfront; this change will cover calls to nip04Encrypt and nip04Decrypt (also
used in auth_provider.dart) and prevent repeated runtime prompts.
In `@rust/src/api/signer.rs`:
- Around line 1-6: The file rust/src/api/signer.rs has rustfmt/formatting
violations around the module header and subsequent items (e.g., the
NostrSigner-related code and Amber integration comments); run rustfmt (cargo
fmt) in the rust workspace or format this file to fix wrapping/whitespace so the
doc comment and the NostrSigner bridge code conform to rustfmt rules, then
re-run CI; specifically locate symbols like the module doc header, NostrSigner
trait bridge and any Amber/NIP-55 related functions in signer.rs and ensure
their comments and signatures are line-wrapped and indented according to
rustfmt.
In `@rust/src/frb_generated.rs`:
- Around line 28-36: The generated import block has rustfmt mismatches (e.g. the
flutter_rust_bridge imports like
flutter_rust_bridge::for_generated::byteorder::{NativeEndian, ReadBytesExt,
WriteBytesExt}, flutter_rust_bridge::for_generated::{transform_result_dco,
Lifetimeable, Lockable}, and flutter_rust_bridge::{Handler, IntoIntoDart});
re-run the FRB generator to regenerate rust/src/frb_generated.rs and then run
cargo fmt (or rustfmt) to fix formatting so CI passes; if you must edit by hand,
adjust the use statements to match rustfmt style (combine/split lines as rustfmt
expects) and verify no other generated blocks (e.g. similar flutter_rust_bridge
imports later) remain unformatted.
- Around line 3062-3133: The decode functions
decode_DartFn_Inputs_String_Output_String_AnyhowException and
decode_DartFn_Inputs_String_String_Output_String_AnyhowException currently call
.expect(...) which panics when Dart throws; instead change the Rust API and
generated bindings so the callback return type is a Result (e.g., Result<String,
ApiError>), update the codegen so the async body returns Result<String,
ApiError> (or the chosen error type) and propagates the deserialized Err branch
as Err rather than calling expect, and adjust convert_into_dart_fn_future usage
to accept futures resolving to Result; after updating the generator and the
DartSigner callback signatures (impl Fn(...) -> DartFnFuture<Result<String,
ApiError>>), regenerate the bindings so runtime Dart exceptions map into Rust
Err values instead of panics.
♻️ Duplicate comments (3)
rust/src/api/signer.rs (1)
69-88: Add error handling to Dart callbacks—exceptions from Dart will cause panics.Awaiting
DartFnFuturein flutter_rust_bridge 2.11.1 returns aResult; Dart exceptions become theErrvariant. The current code ignores this and will panic on Dart-side failures. Use.map_err()to convert toSignerErrorin all five callback invocations:
sign_event(line 80)nip04_encrypt(line 99)nip04_decrypt(line 113)nip44_encrypt(line 127)nip44_decrypt(line 141)Fix pattern for all callbacks
- let signed_json = sign_fn(unsigned_json).await; + let signed_json = sign_fn(unsigned_json) + .await + .map_err(|e| SignerError::backend(format!("Dart callback failed: {e}")))?;android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt (2)
181-189: Use application context for signer availability checks.
activitycan be null during lifecycle transitions, causing false negatives even though the application context is available.🛠️ Proposed fix
- val context = activity ?: return false + val ctx = context ?: return false @@ - val infos = context.packageManager.queryIntentActivities(intent, 0) + val infos = ctx.packageManager.queryIntentActivities(intent, 0) return infos.isNotEmpty()
411-420: Keep “id” extra consistent across signer intents.
signEventalways includesid, but encryption/decryption only adds it when non‑empty. This inconsistency can affect signer behavior.🛠️ Proposed fix
- if (id.isNotEmpty()) { - intent.putExtra("id", id) - } + intent.putExtra("id", id)
| cursor?.use { | ||
| // Check if rejected | ||
| if (it.getColumnIndex("rejected") >= 0) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the AmberSignerPlugin.kt file
fd -t f "AmberSignerPlugin.kt"Repository: marmot-protocol/sloth
Length of output: 131
🏁 Script executed:
# Check the file size first
wc -l android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 135
🏁 Script executed:
# Read the context around lines 306-310 and 467-469
sed -n '300,320p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 835
🏁 Script executed:
# Also check the second occurrence around lines 467-469
sed -n '460,475p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 599
🏁 Script executed:
# Get more context to understand what this code is doing
sed -n '290,325p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1296
🏁 Script executed:
# Also check the context for the second location
sed -n '455,480p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 939
🏁 Script executed:
# Search for content provider definitions
rg -i "content.*rejected" android/app/src/main/kotlinRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Look for schema/contract definitions
rg -i "rejected" android/app/src/main/kotlinRepository: marmot-protocol/sloth
Length of output: 793
🏁 Script executed:
# Search for where these constants are defined
rg "eventJson|content.*=|pubkey.*=" android/app/src/main/kotlin | head -20Repository: marmot-protocol/sloth
Length of output: 2325
🏁 Script executed:
# Look at the full context of these methods to understand the flow
sed -n '295,330p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1319
🏁 Script executed:
# Check if there's any documentation or comments about the rejected column
sed -n '1,100p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 3709
🏁 Script executed:
# Look for content provider definitions in the Android project
fd -t f "ContentProvider\|Contract\|Provider" android/Repository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Search for any test files that might show expected behavior
fd -t f -e "Test.kt" -e "test.kt" android/Repository: marmot-protocol/sloth
Length of output: 402
🏁 Script executed:
# Look for documentation or README files
fd -t f -e "md" -e "txt" android/Repository: marmot-protocol/sloth
Length of output: 402
🏁 Script executed:
# Search for where the "rejected" column might be defined or documented
rg "rejected.*=" android/ --type kotlin -B 2 -A 2Repository: marmot-protocol/sloth
Length of output: 1079
🏁 Script executed:
# Check if there are any gradle files or build.gradle mentioning the signer package
fd -t f "build.gradle\|build.gradle.kts" android/Repository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Look at the full implementation of the problematic methods
sed -n '295,350p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1841
🏁 Script executed:
# Check the second occurrence location more precisely
sed -n '450,500p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1622
🏁 Script executed:
# Search for README or documentation files
find . -name "README*" -o -name "*.md" | head -20Repository: marmot-protocol/sloth
Length of output: 299
🏁 Script executed:
# Search for any comments about the schema or protocol
rg "NIP-55\|Amber\|schema" --type kotlinRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Check if there's an external reference or package documentation
rg "SIGN_EVENT" android/Repository: marmot-protocol/sloth
Length of output: 200
🏁 Script executed:
# Look for any version control history or comments
git log --oneline android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt 2>/dev/null | head -10Repository: marmot-protocol/sloth
Length of output: 87
🏁 Script executed:
# Search for implementation details of content resolver expectations
sed -n '250,300p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1857
🏁 Script executed:
# Look for README files that might explain the protocol
cat README.md | head -100Repository: marmot-protocol/sloth
Length of output: 2808
🏁 Script executed:
# Search for any comments about the protocol or expected behavior
sed -n '1,50p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 2068
🏁 Script executed:
# Look for any external documentation references or links
rg -i "nip.?55\|amber.*signer\|content.*resolver" . --type markdownRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Check if there are any tests or examples showing how this should work
fd -t f . android/app/src/test --type kotlinRepository: marmot-protocol/sloth
Length of output: 238
🏁 Script executed:
# Look at the comment about the method more carefully
sed -n '277,290p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 551
🏁 Script executed:
# Search for any protocol or schema documentation
rg -i "protocol\|schema\|contract" README.mdRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Check the git history for more context about this code
git log --all --oneline | grep -i "amber\|signer\|nip" | head -10Repository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Look at the exact implementation of how other columns are handled
sed -n '300,330p' android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.ktRepository: marmot-protocol/sloth
Length of output: 1143
🏁 Script executed:
# Search for any documentation about Amber or NIP-55
find . -name "*.md" -exec grep -l "Amber\|NIP-55\|NIP.55" {} \;Repository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
# Search for any references to how rejected should be handled
web_search
NIP-55 Nostr signer protocol Amber content providerRepository: marmot-protocol/sloth
Length of output: 160
Check the value of the "rejected" column, not just its presence.
The current code checks if the "rejected" column exists and immediately returns null. If the content provider always includes this column, the method will always fail regardless of the actual rejection status. The fix should verify both column existence and its value:
Suggested fix (adjust type per schema)
- if (it.getColumnIndex("rejected") >= 0) {
- return null
- }
+ val rejectedIndex = it.getColumnIndex("rejected")
+ if (rejectedIndex >= 0 && it.getInt(rejectedIndex) != 0) {
+ return null
+ }Also applies to: 467-469
🤖 Prompt for AI Agents
In `@android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt` around
lines 306 - 310, The cursor handling currently returns null if the "rejected"
column exists rather than checking its value; inside the cursor?.use block in
AmberSignerPlugin (the query/result-processing code that reads the "rejected"
column) first get the column index for "rejected", and if index >= 0 then read
the column value (use getInt or getString per your schema) and only return null
when that value indicates rejection (e.g., 1 or "true"); otherwise continue
normal processing. Apply the same change to the other cursor?.use occurrence
that currently checks only existence (the one around the second occurrence noted
in the review).
lib/providers/amber_provider.dart
Outdated
| final amberState = state.value; | ||
| if (amberState == null || !amberState.isEnabled) { | ||
| throw const AmberSignerException('NOT_CONNECTED', 'Not connected to Amber'); | ||
| } | ||
|
|
||
| final service = ref.read(amberSignerServiceProvider); | ||
| return service.signEvent( | ||
| eventJson: eventJson, | ||
| id: id, | ||
| currentUser: amberState.pubkey, | ||
| ); | ||
| } | ||
|
|
||
| /// Encrypts text using NIP-04 via Amber. | ||
| Future<String> nip04Encrypt({ | ||
| required String plaintext, | ||
| required String recipientPubkey, | ||
| }) async { | ||
| final amberState = state.value; | ||
| if (amberState == null || !amberState.isEnabled) { | ||
| throw const AmberSignerException('NOT_CONNECTED', 'Not connected to Amber'); | ||
| } | ||
|
|
||
| final service = ref.read(amberSignerServiceProvider); | ||
| return service.nip04Encrypt( | ||
| plaintext: plaintext, | ||
| pubkey: recipientPubkey, | ||
| currentUser: amberState.pubkey, | ||
| ); | ||
| } | ||
|
|
||
| /// Decrypts text using NIP-04 via Amber. | ||
| Future<String> nip04Decrypt({ | ||
| required String encryptedText, | ||
| required String senderPubkey, | ||
| }) async { | ||
| final amberState = state.value; | ||
| if (amberState == null || !amberState.isEnabled) { | ||
| throw const AmberSignerException('NOT_CONNECTED', 'Not connected to Amber'); | ||
| } | ||
|
|
||
| final service = ref.read(amberSignerServiceProvider); | ||
| return service.nip04Decrypt( | ||
| encryptedText: encryptedText, | ||
| pubkey: senderPubkey, | ||
| currentUser: amberState.pubkey, | ||
| ); | ||
| } | ||
|
|
||
| /// Encrypts text using NIP-44 via Amber. | ||
| Future<String> nip44Encrypt({ | ||
| required String plaintext, | ||
| required String recipientPubkey, | ||
| }) async { | ||
| final amberState = state.value; | ||
| if (amberState == null || !amberState.isEnabled) { | ||
| throw const AmberSignerException('NOT_CONNECTED', 'Not connected to Amber'); | ||
| } | ||
|
|
||
| final service = ref.read(amberSignerServiceProvider); | ||
| return service.nip44Encrypt( | ||
| plaintext: plaintext, | ||
| pubkey: recipientPubkey, | ||
| currentUser: amberState.pubkey, | ||
| ); | ||
| } | ||
|
|
||
| /// Decrypts text using NIP-44 via Amber. | ||
| Future<String> nip44Decrypt({ | ||
| required String encryptedText, | ||
| required String senderPubkey, | ||
| }) async { | ||
| final amberState = state.value; | ||
| if (amberState == null || !amberState.isEnabled) { | ||
| throw const AmberSignerException('NOT_CONNECTED', 'Not connected to Amber'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "AmberSignerException|NOT_CONNECTED|NO_SIGNER|REQUEST_IN_PROGRESS" -g "*.dart"Repository: marmot-protocol/sloth
Length of output: 3820
🏁 Script executed:
sed -n '120,145n' lib/providers/auth_provider.dartRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
sed -n '25,50n' lib/screens/home_screen.dartRepository: marmot-protocol/sloth
Length of output: 47
🏁 Script executed:
fd -e dart -e json | xargs rg -l "NOT_CONNECTED|user.*friendly|error.*message" | head -20Repository: marmot-protocol/sloth
Length of output: 389
🏁 Script executed:
cat -n lib/providers/auth_provider.dart | sed -n '120,145p'Repository: marmot-protocol/sloth
Length of output: 1357
🏁 Script executed:
cat -n lib/screens/home_screen.dart | sed -n '25,50p'Repository: marmot-protocol/sloth
Length of output: 1036
🏁 Script executed:
cat -n lib/src/rust/api/error.dart | head -100Repository: marmot-protocol/sloth
Length of output: 2299
🏁 Script executed:
rg -n "catch.*AmberSignerException|on.*AmberSignerException" -A 3 -B 1 lib/Repository: marmot-protocol/sloth
Length of output: 7984
Map AmberSignerException error codes to user-friendly messages.
Currently only USER_REJECTED is mapped to friendly text ('Login cancelled') in home_screen.dart. Other error codes like NOT_CONNECTED, NO_RESPONSE, NO_PUBKEY, NO_RESULT, and NO_EVENT surface raw technical messages to users. Implement centralized error message mapping for all Amber exception codes before displaying to end users.
🤖 Prompt for AI Agents
In `@lib/providers/amber_provider.dart` around lines 149 - 224, Create a
centralized mapper that converts AmberSignerException codes into user-facing
messages (e.g., implement a function like mapAmberSignerError(String code) or
add a userMessage getter on AmberSignerException) that covers USER_REJECTED ->
"Login cancelled", NOT_CONNECTED -> "Not connected to Amber", NO_RESPONSE -> "No
response from Amber", NO_PUBKEY -> "Missing public key", NO_RESULT -> "No result
returned", NO_EVENT -> "No event returned" and a sensible fallback for unknown
codes; then update the UI error handling (where AmberSignerException is caught,
e.g., in home_screen.dart) to pass exception.code into this mapper and display
the returned friendly string instead of the raw technical message.
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
@dannym-arx I’m not an amber user, but I used an android I have for testing purposes, downloaded amber, set a nsec of a real nostr account, tried granting permissions on amber but I couldn’t go past the Home Screen in all the attempts I made… maybe I am doing something wrong?
Maybe we could have a call to go through this PR? I am having a bit of trouble understanding how it works to give a useful review
lib/screens/home_screen.dart
Outdated
| crossAxisAlignment: CrossAxisAlignment.stretch, | ||
| children: [ | ||
| // Show Amber login button if Amber is available | ||
| if (amberState.value?.isAvailable ?? false) ...[ |
There was a problem hiding this comment.
I think that the amber option should appear in login screen and not as a third option in Home Screen.... @vladimir-krstic WDYT?
| import 'package:sloth/providers/amber_provider.dart' show amberProvider; | ||
| import 'package:sloth/providers/auth_provider.dart' show authProvider; | ||
| import 'package:sloth/routes.dart' show Routes; | ||
| import 'package:sloth/theme.dart'; |
There was a problem hiding this comment.
Small issue with rebase here. I think this commit fixes it (so that build apk works) f59f009
There was a problem hiding this comment.
Some questions that we can talk in a call:
- This flow could be useful/generalized for other signers? Or each signer will work differently?
- Do we need to handle the case when amber stops being available after the user logged in ?
- Event kinds: event kinds inside group messages (reactions, deletions, etc) don’t need amber permission? I’m a bit lost there.
- I don’t understand how is this is going to affect all other actions. Eg: accepting an invite, sending a message. I don’t see changes related to this parts of the app but I think we need them, right?
- How will this amber integration work with blossom? (Sloth doesn’t have media yet but will have soon)
- At this point I’m thinking that rust crate may do behind lots of things that require signing… that now the flutter app has no idea about cause rust crate exposes a friendly API. Maybe we could have a stream or another way that allows the rust crate to request to flutter side to sign requests with amber?
Updates with what I understood (please correct me @dannym-arx if I got something wrong 😅 )
|
|
2. I don't know what we should do in that case, all signing will fail... but technically you can read previous conversations... I'm not sure what the course of action should be, what do you think? there isn't much we can do, either we delete the account, or we keep it in a non functional state. or I guess we can present a ui to switch it to a local user, tho I'm not 100% sure if we can differentiate between the app being deleted and other reasons for amber not launching, so this may cause unforeseen issues. 5. to clarify on this, we're not 100% sure it works with blossom (tho it should), keyword missing there haha I will address the other things we discussed in the call and fix the issues you found :) Just to make those visible to everyone:
|
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
Update: for the account I was having problems, the error message was "Account has no key package relays configured"
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/hooks/use_amber.dart`:
- Around line 23-24: amberService and storage are being recreated on every hook
invocation; memoize their instances by replacing the direct instantiations of
AmberSignerService and FlutterSecureStorage with hook memoization (e.g.,
useMemoized(() => const AmberSignerService()) and useMemoized(() => const
FlutterSecureStorage())) and assign those results to the amberService and
storage variables so they remain referentially stable across rebuilds.
In `@lib/providers/auth_provider.dart`:
- Around line 167-171: Extract the hardcoded relay list from auth_provider.dart
(the const defaultRelays array) into a shared configuration constant (e.g.,
DEFAULT_RELAYS or kDefaultRelays) in a central config file or constants class,
export it, then replace the local const defaultRelays with an import and
reference to that shared constant in the AuthProvider code (lookup uses inside
any methods or constructors referencing defaultRelays to ensure the symbol is
replaced). Ensure you add the necessary import for the config/ constants module
and keep the list values identical to preserve behavior.
In `@rust/src/api/signer.rs`:
- Around line 165-188: The API currently orchestrates multiple Whitenoise calls
(Whitenoise::get_instance(), DartSigner::new(...), login_with_external_signer,
register_external_signer, publish_key_package_for_account_with_signer) which
leaks business flow into rust/src/api; move this sequence into the Whitenoise
implementation and expose a single high-level method (e.g.,
Whitenoise::login_and_register_external_signer(pubkey, signer_callbacks) or
similar) that takes the public key and signer callbacks and internally performs
login → register → publish; then simplify signer.rs to call
Whitenoise::get_instance()? and that single new method, constructing the
DartSigner only if needed or letting Whitenoise create/own it, so the API layer
becomes a thin orchestration wrapper.
♻️ Duplicate comments (1)
rust/src/frb_generated.rs (1)
3062-3133: Generated Dart-callback decoder panics on Dart exceptions.Line 3083 and Line 3122:
.expect(...)will panic when the Dart callback throws, crashing the app. Update the API signatures to returnResultand regenerate the bindings so errors propagate instead of panicking (avoid hand-editing this generated file).flutter_rust_bridge DartFnFuture error handling when Dart callback throws
| let whitenoise = whitenoise::Whitenoise::get_instance()?; | ||
| let pubkey = PublicKey::parse(&pubkey)?; | ||
|
|
||
| // Create the signer - DartSigner is Clone since it uses Arc internally | ||
| let signer = DartSigner::new( | ||
| pubkey, | ||
| sign_event, | ||
| nip04_encrypt, | ||
| nip04_decrypt, | ||
| nip44_encrypt, | ||
| nip44_decrypt, | ||
| ); | ||
|
|
||
| // Login with the external signer (this creates the account without publishing) | ||
| let account = whitenoise.login_with_external_signer(pubkey).await?; | ||
|
|
||
| // Register a clone of the signer for ongoing use (e.g., giftwrap decryption) | ||
| whitenoise.register_external_signer(pubkey, signer.clone()); | ||
|
|
||
| // Now publish the key package using our Dart signer | ||
| // This requires the signer to sign the key package event | ||
| whitenoise | ||
| .publish_key_package_for_account_with_signer(&account, signer) | ||
| .await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move external-signer login orchestration into whitenoise.
Lines 165-188: the API layer coordinates login → register → publish. Consider moving this sequence into whitenoise and exposing a single call here to keep the API layer thin. Based on learnings, keep rust/src/api as translation/orchestration-only wrappers.
🤖 Prompt for AI Agents
In `@rust/src/api/signer.rs` around lines 165 - 188, The API currently
orchestrates multiple Whitenoise calls (Whitenoise::get_instance(),
DartSigner::new(...), login_with_external_signer, register_external_signer,
publish_key_package_for_account_with_signer) which leaks business flow into
rust/src/api; move this sequence into the Whitenoise implementation and expose a
single high-level method (e.g.,
Whitenoise::login_and_register_external_signer(pubkey, signer_callbacks) or
similar) that takes the public key and signer callbacks and internally performs
login → register → publish; then simplify signer.rs to call
Whitenoise::get_instance()? and that single new method, constructing the
DartSigner only if needed or letting Whitenoise create/own it, so the API layer
becomes a thin orchestration wrapper.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/hooks/use_amber.dart`:
- Around line 41-79: In connect(), the success log uses pubkey.substring(0, 8)
which will throw for short/invalid pubkeys; change the logging to safely produce
a preview (e.g., check pubkey != null and pubkey.length > 8 before calling
substring, or use a helper preview function that returns the whole pubkey if
shorter) and log that safe preview in the _logger.info call (still return pubkey
and keep saving to storage as-is); ensure this avoids any thrown exception
during logging and preserves existing storage.write and error handling behavior.
In `@lib/providers/auth_provider.dart`:
- Around line 190-199: The getLoginMethod function uses firstOrNull (involving
LoginMethod.values and _loginMethodKey) which requires the collection extension;
either add the missing import "import 'package:collection/collection.dart';" at
the top of the file, or replace the firstOrNull usage with a standard Dart
lookup (e.g., use .firstWhere with orElse to return null) so the code compiles
without the extension. Ensure the change preserves the current behavior of
throwing a StateError when no matching LoginMethod is found.
In `@lib/services/amber_signer_service.dart`:
- Around line 63-88: The default case in the userFriendlyMessage getter
currently returns the raw platform message (variable message); change it to
return a generic, non-technical user-facing string (e.g., "An error occurred
with the signer. Please try again.") and stop returning message directly. Also
ensure the original raw message is recorded to logs for debugging (use your app
logger or print) when handling Amber errors so developers can see the detailed
message while users only see the generic text. Keep the other switch cases
unchanged and update references in the userFriendlyMessage getter and any
callers expecting the fallback text.
♻️ Duplicate comments (3)
rust/src/api/signer.rs (1)
69-139: Dart callback errors are silently suppressed — handleResultfromDartFnFuture.
DartFnFuture<String>.awaitreturns aResult<String, E>, but the code at lines 76, 94, 108, 122, and 136 treats the awaited value as a bareString, discarding any Dart-side errors.Wrap each callback invocation to convert errors to
SignerError:let signed_json = sign_fn(unsigned_json) .await .map_err(|e| SignerError::backend(e))?;Apply the same pattern to
nip04_encrypt,nip04_decrypt,nip44_encrypt, andnip44_decrypt.
,android/app/src/main/kotlin/com/example/sloth/AmberSignerPlugin.kt (2)
181-189: Use application context for signer availability checks.Using
activitycan return false negatives when the activity is temporarily null; the plugin already stores application context.🛠️ Suggested fix
private fun isExternalSignerInstalled(): Boolean { - val context = activity ?: return false + val ctx = context ?: return false val intent = Intent().apply { action = Intent.ACTION_VIEW data = Uri.parse("$NOSTRSIGNER_SCHEME:") } - val infos = context.packageManager.queryIntentActivities(intent, 0) + val infos = ctx.packageManager.queryIntentActivities(intent, 0) return infos.isNotEmpty() }
306-312: Check therejectedflag value, not just nullability.If the column is always present with
0/false, the current check treats every response as rejected. Read the value and only reject when it indicates true (type depends on schema).🛠️ Suggested fix (apply to both resolver blocks)
- val rejectedIndex = it.getColumnIndex("rejected") - if (rejectedIndex >= 0 && !it.isNull(rejectedIndex)) { - return null - } + val rejectedIndex = it.getColumnIndex("rejected") + if (rejectedIndex >= 0 && it.getInt(rejectedIndex) != 0) { + return null + }Amber NIP-55 content provider rejected column value type (int/bool/string)?Also applies to: 469-472
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/providers/auth_provider.dart`:
- Around line 190-200: In getLoginMethod(), avoid throwing StateError for
unknown stored values; instead catch the firstWhere failure, log the unexpected
method value and return null so callers like isUsingAmber() and logout can
handle a missing/invalid method gracefully; update the catch block in
getLoginMethod() (which reads from secureStorageProvider using _loginMethodKey)
to record the unknown method (using your existing logging mechanism) and return
null rather than throwing.
In `@lib/services/amber_signer_service.dart`:
- Around line 135-161: The getPublicKey method currently logs and returns the
public key but doesn't persist the signer package name; update getPublicKey to
check response.packageName and save it using the same mechanism used by
getSignerPackageName (e.g., set the backing field or call the persistence helper
used elsewhere in AmberSignerService), so when response.packageName != null or
not empty you persist it before returning; ensure this uses the existing symbols
response.packageName, getPublicKey, and getSignerPackageName (or the service's
internal persistence helper) so later calls to getSignerPackageName can retrieve
the stored value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/services/amber_signer_service.dart`:
- Around line 195-201: In signEvent, after creating
AmberSignerResponse.fromMap(result) validate that the response contains either a
non-empty signature or a non-empty signedEvent (e.g., check
AmberSignerResponse.signature and AmberSignerResponse.signedEvent) and if both
are missing/empty throw an AmberSignerException (e.g., 'NO_SIGNATURE' or
similar) instead of returning the empty response; update the _logger message to
reflect success only when a valid signature/signedEvent is present so downstream
code won’t receive empty payloads.
| if (result == null) { | ||
| throw const AmberSignerException('NO_RESPONSE', 'No response from signer'); | ||
| } | ||
|
|
||
| final response = AmberSignerResponse.fromMap(result); | ||
| _logger.fine('Got signature from signer'); | ||
| return response; |
There was a problem hiding this comment.
Guard against empty signer responses in signEvent.
signEvent returns a response even if it contains neither a signature nor a signed event. That pushes failures downstream and bypasses AmberSignerException handling. Validate the payload before returning.
🐛 Proposed fix
final response = AmberSignerResponse.fromMap(result);
- _logger.fine('Got signature from signer');
+ final hasSignature = response.result != null && response.result!.isNotEmpty;
+ final hasEvent = response.event != null && response.event!.isNotEmpty;
+ if (!hasSignature && !hasEvent) {
+ throw const AmberSignerException(
+ 'NO_RESULT',
+ 'Signer did not return a signature or event',
+ );
+ }
+ _logger.fine('Got signer response');
return response;📝 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.
| if (result == null) { | |
| throw const AmberSignerException('NO_RESPONSE', 'No response from signer'); | |
| } | |
| final response = AmberSignerResponse.fromMap(result); | |
| _logger.fine('Got signature from signer'); | |
| return response; | |
| if (result == null) { | |
| throw const AmberSignerException('NO_RESPONSE', 'No response from signer'); | |
| } | |
| final response = AmberSignerResponse.fromMap(result); | |
| final hasSignature = response.result != null && response.result!.isNotEmpty; | |
| final hasEvent = response.event != null && response.event!.isNotEmpty; | |
| if (!hasSignature && !hasEvent) { | |
| throw const AmberSignerException( | |
| 'NO_RESULT', | |
| 'Signer did not return a signature or event', | |
| ); | |
| } | |
| _logger.fine('Got signer response'); | |
| return response; |
🤖 Prompt for AI Agents
In `@lib/services/amber_signer_service.dart` around lines 195 - 201, In signEvent,
after creating AmberSignerResponse.fromMap(result) validate that the response
contains either a non-empty signature or a non-empty signedEvent (e.g., check
AmberSignerResponse.signature and AmberSignerResponse.signedEvent) and if both
are missing/empty throw an AmberSignerException (e.g., 'NO_SIGNATURE' or
similar) instead of returning the empty response; update the _logger message to
reflect success only when a valid signature/signedEvent is present so downstream
code won’t receive empty payloads.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.