feat: bip85 alias, revoke, activate, copy and UI improvements

[ethicnology]

Closes #1629
Closes #1212

[leesalminen]

@claude please review this PR

[claude]

Claude finished @leesalminen's task in 2m 28s —— View job

---

Code Review for PR #1639 ✅

I've completed a thorough review of the BIP85 alias, revoke, activate, copy and UI improvements feature. Overall, the implementation is well-structured and follows clean architecture principles. Here are my findings:

---

✅ Strengths

1. Clean Architecture: The PR properly separates concerns with dedicated use cases, repository, and datasource layers
2. Consistent Patterns: New use cases follow the existing codebase patterns
3. Good Refactoring: The change from passing String path to Bip85DerivationEntity in revoke() and activate() methods improves type safety
4. UI Enhancements: Added loading states, copy functionality, and improved layout with ListView
5. Naming Consistency: Renamed reactivate() to activate() for better clarity

---

🔴 Critical Issues

1. Security Risk: Sensitive Data Exposure in Widget (lib/core/widgets/bip85_derivation_widget.dart:67-70)

final data = bip85.Bip85Entropy.deriveFromHardenedPath(
  xprvBase58: widget.xprvBase58,
  path: bip85.Bip85HardenedPath(widget.derivation.path),
);

Problem: The sensitive entropy data is re-derived on every widget rebuild. This is inefficient and exposes the master xprv to the widget layer unnecessarily.

Recommendation: Consider deriving the entropy once in the cubit/use case layer and passing it as a parameter, or cache it to avoid repeated derivations.

2. Widget State Not Updated After Alias Change (lib/core/widgets/bip85_derivation_widget.dart:46-50)

Future<void> _saveAlias() async {
  if (widget.onAliasChanged != null) {
    await widget.onAliasChanged!(widget.derivation, _aliasController.text);
    setState(() => _isEditingAlias = false);
  }
}

Problem: When the alias is saved, the widget exits edit mode but still displays the old alias text from widget.derivation.alias. The widget needs to be rebuilt with the updated derivation entity to show the new alias.

Current Flow:

1. User edits alias → saves
2. _saveAlias() calls cubit.aliasDerivation()
3. Cubit updates DB and calls fetchAllDerivations()
4. Widget exits edit mode but displays old data until parent rebuilds

Issue: The _aliasController.text is saved, but if the parent widget doesn't rebuild immediately, the displayed text shows the old alias value.

---

🟡 Major Issues

3. Missing Error Handling in Widget Callbacks (lib/core/widgets/bip85_derivation_widget.dart:46-63)

The async callback methods (_saveAlias, _revokeDerivation, _activateDerivation) don't handle errors. If a use case throws an exception, it will propagate uncaught.

Recommendation: Wrap in try-catch and show user-friendly error messages via SnackBar or similar.

4. Unnecessary Try-Catch with Rethrow

Multiple locations just catch and rethrow without adding value:

• lib/core/bip85/domain/activate_bip85_derivation_usecase.dart:11-15
• lib/core/bip85/domain/revoke_bip85_derivation_usecase.dart:10-15
• lib/core/bip85/data/bip85_datasource.dart:126-133
• lib/core/bip85/data/bip85_datasource.dart:136-143
• lib/core/bip85/data/bip85_datasource.dart:146-153

Recommendation: Remove these unnecessary try-catch blocks since they don't transform or log errors. Let exceptions propagate naturally.

5. Loading State Management Issue (lib/features/bip85_entropy/presentation/cubit.dart:60-64)

Future<void> fetchAllDerivations() async {
  emit(state.copyWith(isLoading: true));
  final derivations = await _fetchAllBip85DerivationsUsecase.execute();
  emit(state.copyWith(derivations: derivations, isLoading: false));
}

Problem: This method is called from init(), deriveNextMnemonic(), deriveNextHex(), and other methods that also set isLoading: true. This creates redundant loading state emissions.

Recommendation: Consider making fetchAllDerivations() not emit loading states since the callers handle it, or refactor to avoid duplicate emissions.

6. Potential Memory Leak (lib/core/widgets/bip85_derivation_widget.dart:157)

child: TextField(
  controller: TextEditingController(text: data),
  obscureText: _isObscured,
  readOnly: true,
  ...
),

Problem: Creating a new TextEditingController on every build without disposing it can cause memory leaks.

Recommendation: Either use a managed controller or use the Text widget since the field is read-only.

---

🟢 Minor Issues / Suggestions

7. Inconsistent Error Handling in AliasBip85DerivationUsecase (lib/core/bip85/domain/alias_bip85_derivation_usecase.dart:10-15)

This use case doesn't wrap the call in try-catch, while ActivateBip85DerivationUsecase and RevokeBip85DerivationUsecase do. This inconsistency is confusing.

Recommendation: Be consistent - either all use cases handle errors or none do.

8. Missing Input Validation for Alias (lib/core/bip85/data/bip85_datasource.dart:146-154)

The alias can be set to any string, including empty strings or extremely long strings.

Recommendation: Add validation for alias length and content (e.g., max 50 characters, no special characters if needed).

9. UI: Cramped Header Row (lib/core/widgets/bip85_derivation_widget.dart:85-110)

The header row displays path, application, status, and action buttons all in one row with MainAxisAlignment.spaceBetween. This could overflow on small screens or with long paths.

Recommendation: Consider a more responsive layout, perhaps wrapping text or using a Column for better spacing.

10. No Confirmation Dialog for Revoke (lib/core/widgets/bip85_derivation_widget.dart:53-57)

Revoking a derivation is a significant action but has no confirmation dialog.

Recommendation: Add a confirmation dialog before revoking to prevent accidental revocations.

11. Hard-Coded SnackBar Text (lib/core/widgets/bip85_derivation_widget.dart:176)

content: Text('Copied to clipboard'),

Recommendation: Use localized strings from context.loc for better i18n support.

12. Loading Indicator Positioning (lib/features/bip85_entropy/bip85_home_page.dart:27-29)

The FadingLinearProgress shows when either xprvBase58 is empty OR isLoading is true. During initialization, both conditions might be true simultaneously.

Recommendation: Simplify the trigger to just state.isLoading for clarity.

---

📝 Code Quality Observations

1. Formatting: Some formatting changes in the diff appear to be auto-formatter adjustments (e.g., in bip85_datasource.dart:90-107). These are fine but make the diff larger.

2. MainAxisAlignment Syntax: Noticed .start and .spaceBetween syntax (lib/core/widgets/bip85_derivation_widget.dart:83, 86). This is correct Dart 3 shorthand syntax.

3. State Management: The cubit properly manages loading states and error states. Good use of BLoC pattern.

---

🧪 Testing Recommendations

1. Unit Tests: Add tests for the new use cases (alias, activate, revoke)
2. Widget Tests: Test the edit mode, revoke/activate buttons, and copy functionality
3. Integration Tests: Verify the full flow of aliasing, revoking, and activating derivations
4. Edge Cases: Test behavior with:
   • Empty alias
   • Very long alias
   • Rapid clicking of revoke/activate buttons
   • Editing alias while derivation list is loading

---

📋 Summary

Approve with Changes: This PR adds valuable functionality and follows good architectural patterns. However, the critical issues (especially #1 and #2) should be addressed before merging. The major issues are important for robustness but could be addressed in a follow-up PR if needed.

Priority fixes:

1. Fix alias display update issue (Sync in Isolate #2)
2. Add error handling to widget callbacks (Create new wallet using existing mnemonic and new bip39 passphrase #3)
3. Fix memory leak from TextEditingController (Add app version at bottom of settings page #6)
4. Add confirmation dialog for revoke action (Create default wallet when app is first installed #10)

---