fix: restoreWithMnemonic returns per-unit balances instead of single BigInt - #123
fix: restoreWithMnemonic returns per-unit balances instead of single BigInt#123Delgado74 wants to merge 10 commits into
Conversation
…BigInt - Change restoreWithMnemonic() return type from BigInt to Map<String, BigInt> - Update RecoverTokensModal to display each unit separately - Fix widget_test.dart to use ElCajuApp instead of MyApp Resolves issue Forte11Cuba#104
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
📝 WalkthroughWalkthroughMnemonic recovery now returns balances grouped by mint and unit, with corresponding per-unit UI formatting. The widget test uses ChangesWallet Recovery
APK Release Workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/widget_test.dart (1)
16-28:⚠️ Potential issue | 🟠 MajorWrap
ElCajuAppwith providers and replace stale counter assertions.
ElCajuAppcallscontext.watch<SettingsProvider>()andcontext.read<WalletProvider>(), which require ancestor providers. The test will fail during initialization with aProviderNotFoundException. Additionally,ElCajuApproutes toSplashScreen(showing a loading screen), not the Flutter counter sample, so the counter assertions (expect(find.text('0')...,find.byIcon(Icons.add)) are stale and will fail.Wrap
ElCajuAppinMultiProviderwith all four providers (SettingsProvider, WalletProvider, P2PKProvider, PriceProvider), and replace the counter assertions with expectations that match whatSplashScreenactually renders (e.g.,find.byType(CircularProgressIndicator)or logo/text queries). Add imports forMultiProvider,ChangeNotifierProvider, and the specific provider classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/widget_test.dart` around lines 16 - 28, The test fails because ElCajuApp reads SettingsProvider and WalletProvider and routes to SplashScreen rather than the Flutter counter sample; wrap the app in a MultiProvider that supplies ChangeNotifierProvider instances for SettingsProvider, WalletProvider, P2PKProvider and PriceProvider (ensure you add imports for MultiProvider and ChangeNotifierProvider and the provider classes), then replace the stale counter-based expectations and the tap on Icons.add with assertions that match SplashScreen output (e.g., expect(find.byType(CircularProgressIndicator), findsOneWidget) or queries for the app logo/text rendered by SplashScreen) so the test initializes with the required providers and asserts the actual initial UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/screens/8_settings/recover_tokens_modal.dart`:
- Around line 687-688: The call to l10n.recoveredTokens currently passes
recoveredDetails.length (count of unit buckets) which is incorrect; change the
second argument to the count of distinct mints (use recoveredMap.keys.length or
mintsRecovered) so l10n.recoveredTokens(recoveredDetails.join(", "),
recoveredMap.keys.length) (or use mintsRecovered) reports mint count rather than
unit bucket count.
---
Outside diff comments:
In `@test/widget_test.dart`:
- Around line 16-28: The test fails because ElCajuApp reads SettingsProvider and
WalletProvider and routes to SplashScreen rather than the Flutter counter
sample; wrap the app in a MultiProvider that supplies ChangeNotifierProvider
instances for SettingsProvider, WalletProvider, P2PKProvider and PriceProvider
(ensure you add imports for MultiProvider and ChangeNotifierProvider and the
provider classes), then replace the stale counter-based expectations and the tap
on Icons.add with assertions that match SplashScreen output (e.g.,
expect(find.byType(CircularProgressIndicator), findsOneWidget) or queries for
the app logo/text rendered by SplashScreen) so the test initializes with the
required providers and asserts the actual initial UI.
🪄 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: 0f0bab7a-64e7-4667-ab69-44f855d315ae
📒 Files selected for processing (3)
lib/providers/wallet_provider.dartlib/screens/8_settings/recover_tokens_modal.darttest/widget_test.dart
- Change return type to Map<String, Map<String, BigInt>> (mintUrl -> unit -> balance) - Count mints with tokens > 0 instead of unit count - Now shows correct mint count (e.g., '2 mints' not '5 units')
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/providers/wallet_provider.dart`:
- Around line 1995-2008: Before calling ourWallet.receive(...), persist the
transfer token so it can be retried if receive() throws: after
tempWallet.send(...) succeeds, call a helper like
persistPendingToken(result.token, mintUrl, unit) (or equivalent persistence
mechanism) immediately, then call ourWallet.receive(token: result.token); on
successful receive remove the persisted token via deletePendingToken(mintUrl,
unit, result.token) and update results as you already do; if receive throws, do
not delete the persisted token so it remains retryable and ensure any exception
handling/logging does not lose the token information. Use the existing symbols
tempWallet.prepareSend, tempWallet.send, ourWallet.receive and results to locate
where to add persistence and cleanup.
🪄 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: d3646e61-4e48-4b32-86f2-b9acdfae0401
📒 Files selected for processing (2)
lib/providers/wallet_provider.dartlib/screens/8_settings/recover_tokens_modal.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/screens/8_settings/recover_tokens_modal.dart
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build-apk.yml (1)
15-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor to use a matrix strategy.
The three build jobs (
build-arm64,build-armv7,build-x86-64) share identical steps, differing only in the target architectures and artifact names. Consolidating them into a single matrix job will significantly reduce code duplication and simplify future maintenance.♻️ Proposed refactor using a matrix strategy
jobs: build-apk: name: Build ${{ matrix.name }} runs-on: ubuntu-latest timeout-minutes: 60 strategy: matrix: include: - name: ARM64 (armV8) rust_target: aarch64-linux-android flutter_platform: android-arm64 artifact_name: elcaju-arm64-v8a apk_path: app-arm64-v8a-release.apk # Adjust if the actual output is app-release.apk - name: ARMv7 (armV7) rust_target: armv7-linux-androideabi flutter_platform: android-arm artifact_name: elcaju-armeabi-v7a apk_path: app-armeabi-v7a-release.apk - name: x86_64 rust_target: x86_64-linux-android flutter_platform: android-x64 artifact_name: elcaju-x86-64 apk_path: app-x86_64-release.apk steps: - uses: actions/checkout@v4 with: ref: ${{ inputs.branch }} persist-credentials: false - name: Setup Java 17 uses: actions/setup-java@v4 with: distribution: 'zulu' java-version: '17' - name: Setup Flutter uses: subosito/flutter-action@v2 with: channel: stable - name: Setup Rust toolchain uses: dtolnay/rust-toolchain@stable with: targets: ${{ matrix.rust_target }} - name: Install Android NDK run: echo "y" | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager "ndk;27.0.12077973" - name: Install dependencies run: flutter pub get - name: Generate localizations run: flutter gen-l10n - name: Build APK run: flutter build apk --release --target-platform ${{ matrix.flutter_platform }} - name: Upload APK uses: actions/upload-artifact@v4 with: name: ${{ matrix.artifact_name }} path: build/app/outputs/flutter-apk/${{ matrix.apk_path }}🤖 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 @.github/workflows/build-apk.yml around lines 15 - 147, Replace the duplicated build-arm64, build-armv7, and build-x86-64 jobs with one build-apk matrix job. Define each architecture’s name, Rust target, Flutter target platform, artifact name, and APK path under matrix.include, then reuse those values in the setup, build, and upload steps while preserving the existing shared workflow behavior.
🤖 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 @.github/workflows/build-apk.yml:
- Around line 22-24: Disable credential persistence for every checkout action in
.github/workflows/build-apk.yml at lines 22-24, 66-68, and 110-112 by adding
persist-credentials: false to each action's with configuration.
- Around line 51-59: Update the ARM64 build and upload steps at
.github/workflows/build-apk.yml lines 51-59, the corresponding site at lines
95-103, and the corresponding site at lines 139-147 so each build generates
split-per-ABI APKs by adding the required Flutter build option, preserving the
existing ABI-specific artifact paths and names.
---
Nitpick comments:
In @.github/workflows/build-apk.yml:
- Around line 15-147: Replace the duplicated build-arm64, build-armv7, and
build-x86-64 jobs with one build-apk matrix job. Define each architecture’s
name, Rust target, Flutter target platform, artifact name, and APK path under
matrix.include, then reuse those values in the setup, build, and upload steps
while preserving the existing shared workflow behavior.
🪄 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: f737f8e0-893f-464b-aac8-ffc3aaa0a2d9
📒 Files selected for processing (1)
.github/workflows/build-apk.yml
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ inputs.branch }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Disable credential persistence in checkout.
To prevent potential credential leakage through GitHub Actions artifacts, set persist-credentials: false in the checkout action, as flagged by static analysis.
.github/workflows/build-apk.yml#L22-L24: Addpersist-credentials: falseto the checkout action..github/workflows/build-apk.yml#L66-L68: Addpersist-credentials: falseto the checkout action..github/workflows/build-apk.yml#L110-L112: Addpersist-credentials: falseto the checkout action.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 22-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 1 file
.github/workflows/build-apk.yml#L22-L24(this comment).github/workflows/build-apk.yml#L66-L68.github/workflows/build-apk.yml#L110-L112
🤖 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 @.github/workflows/build-apk.yml around lines 22 - 24, Disable credential
persistence for every checkout action in .github/workflows/build-apk.yml at
lines 22-24, 66-68, and 110-112 by adding persist-credentials: false to each
action's with configuration.
Source: Linters/SAST tools
1 participant
Summary
restoreWithMnemonic()return type fromBigInttoMap<String, BigInt>to preserve unit informationRecoverTokensModalto display each unit separately (e.g., "1000 sat, 5 USD, 10 EUR")ElCajuAppinstead ofMyAppProblem
restoreWithMnemonic()was returning a singleBigInt, losing unit information. This caused incorrect display when tokens were recovered from mints with different units (sat, USD, EUR).Example: Recovering 1000 sat + 5 USD would display as "1005 sat" or "1005 USD" depending on active unit.
Solution
Now returns
Map<String, BigInt>(unit → balance), consistent with other restore methods:restoreFromMint()→Map<String, BigInt>restoreAllMints()→Map<String, Map<String, BigInt>>restoreWithMnemonic()→Map<String, BigInt>Files Changed
lib/providers/wallet_provider.dart- Change return type and accumulatorlib/screens/8_settings/recover_tokens_modal.dart- Update display logictest/widget_test.dart- Fix class nameTesting
Resolves fix: restoreWithMnemonic should return per-unit balances instead of a single BigInt #104
Summary by CodeRabbit