Conversation
…back_test_url-references chore: remove FEEDBACK_TEST_URL references
Co-authored-by: charl <charl@vanstaden.info>
…logout-dropdown fix(ui): close dropdown on window resize
…fine (#3037) * refactor: simplify Docker env vars and build command logic - Remove unnecessary environment variables from Docker container - Keep only GITHUB_API_PUBLIC_READONLY_TOKEN as env var - Replace hardcoded build logic with conditional credential handling - Add proper validation for Trello and Cloudflare feedback services - Use dart-define flags instead of environment variables for build configs - Add warning messages for incomplete credential sets * fix executable rights on build.sh * remove the redundant FEEDBACK_TEST_URL, as it will no longer be used in the GUI * swap BUILD_COMMAND and BUILD_CMD (important) BUILD_CMD, which is built dynamically with --dart-define= arguments, should be called last (not during the assets-fetch stage) to ensure the app is actually built with the required defines and that all String.fromEnvironment(...) statements in the Dart code work as expected.
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* remove dgph binaries * Ignore .dgph build artifacts in iOS
* refactor: drop balance override from CoinBalance * chore: remove unused import Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(address-card): revert change to coinbalance and use extensions * feat(coin-addresses): migrate to stream-based pubkey watch function TODO: bump SDK version once merged --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor duplicated translations * apply consistent "dex address" pill style * adds missing bracket
* fix missing price and change data on coin page, sync with main wallet page * chore: remove unreferenced coin list and fiat widgets * refactor(coin-details-info-fiat): move widget builder functions to widgets https://stackoverflow.com/a/53234826 * adds Individual error handling for price and 24hr data sourcing * cache 24hr data from CoinsBlock until SDK offers it * fix layout issue (renderbox constraint violations) * dispose of scrollbars thoughtfully * Show skeleton for assets list addresses (#2990) * feat(ui): show loading skeleton for coin addresses * Reduce address placeholder items from 3 to 1 * fix: apply Cursor suggestions --------- Co-authored-by: Francois <takenagain@users.noreply.github.com> Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
* chore: roll SDK packages and fix non-exhaustive MnemonicFailedReason switch\n\n- Ran .github/scripts/roll_sdk_packages.sh\n- Updated git refs for SDK packages\n- Added handling for invalidWord and invalidChecksum in wallet_simple_import\n- Generated SDK_CHANGELOG.md * feat: more specific mnemonic validation errors (invalid word, checksum, length)\n\n- Added locale keys and messages\n- Mapped MnemonicFailedReason to specific messages\n- Regenerated localization keys * chore: normalize en.json EOF newline * chore: re-roll SDK to latest refs (d5b5c3) and update SDK_CHANGELOG.md * fix: patch breaking SDK changes * chore: remove generated SDK changelog * fix: apply cursor coin addresses bug fixes TODO(@takenagain): Please revisit these change when you complete your PR for full integration of the latest SDK version with the pubkey changes.
* limit memo field to supporting protocols * add TODO for unused component * removes obsolete `MemoField` * Add helper function to check memo support for specific protocols Co-authored-by: charl <charl@vanstaden.info> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: charl <charl@vanstaden.info> Co-authored-by: CharlVS <77973576+CharlVS@users.noreply.github.com>
* adds `isHiddenElse` to `HiddenWithoutWallet` * only hide if `isHiddenElse` is `true`
Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
…indow; prevent zombie processes (#3063) * Implement single instance enforcement for Komodo Wallet on Windows Co-authored-by: charl <charl@vanstaden.info> * Improve window handling and application reopening on Linux and macOS Co-authored-by: charl <charl@vanstaden.info> * Disconnect window signals and clear main window pointer in dispose method Co-authored-by: charl <charl@vanstaden.info> * fix(windows): single-instance mutex: handle ERROR_ACCESS_DENIED when detecting existing instance; focus existing window (#3063) * fix(linux): replace deprecated application flag Replace deprecated application flag for ensuring only a single instance can be open at any time. * fix(windows): correct single-instance detection in mutex logic Remove impossible ERROR_ACCESS_DENIED check when CreateMutexW returns a valid handle. GetLastError() returns ERROR_ACCESS_DENIED only if CreateMutexW fails and returns nullptr. Treat only ERROR_ALREADY_EXISTS as indication of existing instance. * fix(linux): disconnect destroy handler by user_data to fix Linux build error --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…ck to `G_APPLICATION_FLAGS_NONE` (#3105) * Use `GLIB_CHECK_VERSION(2,74,0)` to select `G_APPLICATION_DEFAULT_FLAGS` when available, otherwise use `G_APPLICATION_FLAGS_NONE`. * Resolves build failures with older GLib headers while keeping the preferred constant on newer systems. * No functional changes; improves cross-distro compatibility.
…3108) * chore: update various deps references * chore(deps): Upgrade SDK deps to pub.dev hosted * chore: upgrade flutter version references * chore(sdk): support Option 3 (hosted) SDK deps in roll script and docs * chore(scripts): update roll_sdk_packages.sh * chore(scripts): accept CLI flags (-a/--upgrade-all, -t/--target-branch, --help) * feat(scripts): add --major-sdk-only to allow major upgrades for SDK hosted deps * fix(scripts): preserve pubspec.yaml structure when using --major-sdk-only by backup/restore before inline updates * fix(scripts): delete *.bak and *.bak_major even when no rolls detected * chore: roll sdk * chore: add Cursor ignore * Fix shell array expansion in package upgrade script Co-authored-by: charl <charl@vanstaden.info> * Improve SDK package roll script with robust error handling and file processing Co-authored-by: charl <charl@vanstaden.info> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
#3110) * chore: update various deps references * chore(deps): Upgrade SDK deps to pub.dev hosted * chore: upgrade flutter version references * chore(sdk): support Option 3 (hosted) SDK deps in roll script and docs * chore(scripts): update roll_sdk_packages.sh * chore(scripts): accept CLI flags (-a/--upgrade-all, -t/--target-branch, --help) * feat(scripts): add --major-sdk-only to allow major upgrades for SDK hosted deps * fix(scripts): preserve pubspec.yaml structure when using --major-sdk-only by backup/restore before inline updates * fix(scripts): delete *.bak and *.bak_major even when no rolls detected * chore: roll sdk * chore: add Cursor ignore * build(sdk): add sdk as git submodule and update deps to path overrides * ci: add recursive submodules option to checkout steps * docs: document submodule init step and related notes * chore(submodules): pin SDK to recorded commit and update docs/docker - Set sdk submodule to track dev with update=checkout and on-demand fetch - Document explicit update flow and CI pinned behavior - Ensure Docker builds initialize submodule to pinned commit * docs(pubspec): clarify Option 1 uses sdk submodule (pinned; no symlink) * docs(pubspec): reference SDK submodule docs for Option 1 (pinned setup) * build(docker): bump pinned Flutter version to 3.35.1 * chore(build): bump Flutter to 3.35.2 in docs, workflows, & docker --------- Co-authored-by: CharlVS <77973576+CharlVS@users.noreply.github.com>
* add GitHub Actions workflow to validate Linux build script Introduce docker-linux-build.yml workflow that runs .docker/build.sh for Linux builds inside Docker. This allows us to continuously verify that the build script works as expected and that the Linux application can be successfully built in a clean environment. By having this check in CI, we ensure that end users will also be able to build the application themselves using .docker/build.sh, which is the only officially recommended build path. * CI: touch .docker to trigger PR workflow * remove temp .ci-touch file --------- Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
* test: remove cex market data unit tests API providers already moved to SDK, with market metrics soon to follow * test: explicitly skip failing tests that require SDK internals mocking * fix: conflicting cex and dex RPC imports legacy and local import names conflicted with SDK exports when importing without the `show` option to limit imports. * fix(sparkline): update to use AssetId and Di via RepositoryProvider * fix(cex-price): migrate to local, deprecated model with Decimal Double conversion used for widget display-only purposes, and Rational/Decimal defaulted to for internal calculations (e.g. repositories an blocs) * fix(market-metrics): migrate to SDK MarketDataManager interface * fix(price-chart): migrate to SDK MarketDataManager interface * chore: migrate KW to mono-repo workspace * fix: exhaustive dependency overrides for sdk packages * build(deps): move SDK package overrides to pubspec_overrides.yaml - Remove local path overrides from pubspec.yaml - Add pubspec_overrides.yaml with ./sdk package paths * fix(review): improve rational/decimal conversions and null check safety * test(get-fiat-amount): re-enable fiat amount unit test * fix(profit-loss-calculator): normalise timezones to UTC * chore: sync native dependencies * chore(ios): apply Flutter auto-migrations * fix(macos): fix broken runner config paths * feat(settings): add new SDK methods for coin config commit hashes * fix(wallet-overview): prefer current balance over portfolio balance calc * fix(app-version-info): switch to timer-based polling approach - update wallet overview comments - simplify total balance data source and calculations - use valuegetter for copyWith function * chore(app-version-info): bump polling interval back to 5min --------- Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
#3115) Previously, when logging apiCommitHash, the code printed the function reference (e.g. Closure: () => String) instead of the actual hash value. Example: ``` 2025-8-29: 18:38:32.869T+:0:00:00.036927 [LOG] {message: INFO: MM2 API version loaded successfully - Version: 2.5.1-beta_80766b0, Commit: Closure: () => String - null, path: VersionInfoBloc, app_version: 0.9.2, mm2_version: 2.5.1-beta_80766b0, os_language: en-US, screen_size: , timestamp: 1756492712864, date: 2025-08-29 20:38:32.864631}__metadata: {appVersion: null, mm2Version: 2.5.1-beta_80766b0, appLanguage: , platform: linux Linux 6.8.0-65-generic #68~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Jul 15 18:06:34 UTC 2, osLanguage: en-US, screenSize: } ```
…iner (#3114) * build(devcontainer): reuse .docker images and fix cross-platform paths Use komodo/komodo-wallet image; prebuild base images in initializeCommand; remove USB mount; macOS/WSL compatible. Delete obsolete .devcontainer/Dockerfile. * build(devcontainer): revert to Linux-only flow and validate on ubuntu CI Build from .docker Dockerfile with USB mount; add GH workflow to build images on hosted runners. * ci: remove devcontainer build workflow; use GitHub prebuilds instead * build(devcontainer): init and pin submodules in postCreate * build(devcontainer): ignore dirty state for sdk submodule; pin only commits * build(devcontainer): move postCreate steps to script to avoid exec errors * refactor(devcontainer): remove chown, don't ignore failures, disable privileged flag by default --------- Co-authored-by: Francois <takenagain@users.noreply.github.com>
) * feat: add remember me wallet login * feat(auth): implement one-click remember me sign-in functionality - Add automatic sign-in dialog for last logged-in wallet on app start - Store and retrieve wallet information in local storage when remember me is enabled - Implement remember me checkbox across all wallet flows (login, creation, import) - Add session-based dialog prevention to avoid multiple prompts - Support backward compatibility for existing stored wallet data Technical changes: - Introduce AppDialog as replacement for deprecated PopupDispatcher - Add WalletId JSON storage with pubkey hash for precise wallet matching - Update wallet manager widgets to support remember me state - Add translations for new UI elements - Implement proper storage cleanup when remember me is disabled This enhancement streamlines the user experience by allowing quick access to previously used wallets while maintaining security through password requirements. * fix: reword quick login text * Verify wallet identity using pubkey hash MD5 comparison Co-authored-by: charl <charl@vanstaden.info> * fix: prevent blank screen after login (#3043) * fix(auth): avoid context usage after login dialog closes (#3044) * bug(login): fix quick login dialog showing multiple times * fix: various sign-in remember-me fixes TODO(@takenagain): Refactor remember-me feature to bloc * fix: use stable context in connect wallet dispatcher * fix: navigator bug * fix: add missing password reset autocomplete * Fix: Use root navigator in remembered wallet dialog and reset flag on failure * Improve password field auto-submit with paste detection logic Co-authored-by: charl <charl@vanstaden.info> * feat: integrate password manager to password reset * docs: add wallet manager TODO * fix(auth,autofill): finalize autofill context only after successful login to restore password manager saving\n\n- Remove premature TextInput.finishAutofillContext(shouldSave: true) on submit\n- Call finishAutofillContext(shouldSave: true) in onLogIn() after AuthBloc signals logged-in\n\nThis resolves the regression introduced with one-click login that prevented credential managers from saving/recalling credentials. * fix(auth,dialog): guard dialog close and tolerate sign-out errors to prevent Android grey screen - Make AppDialog close idempotent and use navigator.maybePop() to avoid 'Bad state: No element' during 1-click login transitions - Wrap KDF signOut in try/catch so UI resets even if KDF stop times out - Align deprecated PopupDispatcher.close() to use maybePop() --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…ement (#3111) * fix(macos): show file picker by adding user-selected read-only entitlement - Add com.apple.security.files.user-selected.read-only to DebugProfile and Release entitlements to enable file_picker dialog on macOS.\n- Bump iOS deployment target to 13.0 (project settings and AppFrameworkInfo.plist) to align with plugin requirements.\n- Update file_picker to ^10.3.2 and refresh lockfiles (iOS/macOS).\n\nRefs: https://github.com/miguelpruivo/flutter_file_picker/wiki/Setup * chore: roll native ios/macos deps * fix(macos): enable save dialog by switching to user-selected read-write entitlement - Replace com.apple.security.files.user-selected.read-only with read-write in DebugProfile and Release entitlements to allow NSSavePanel. Refs: https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_security_files_user-selected_read-write, #3111 (comment)
* Add exact amount button and improve large number parsing Co-authored-by: charl <charl@vanstaden.info> * Auto-fill sell amount when order is selected in taker form Co-authored-by: charl <charl@vanstaden.info> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
* fix impossible nested condition logic * handle uninitialized logger exceptions gratefully * caches `typeName` to avoid unneccesary `getCoinTypeName` calls * adds caching for `isParent` * set parents as native; use cached coin values * special case to cover ARB * chore: roll SDK (#3036) Roll SDK for critical KMD API pricing fix * chore: remove FEEDBACK_TEST_URL references * fix(ui): close dropdown on window resize * Fix null safety issues in UiDropdown widget Co-authored-by: charl <charl@vanstaden.info> * fix(build): correct env vars passing to Docker and Dart via --dart-define (#3037) * refactor: simplify Docker env vars and build command logic - Remove unnecessary environment variables from Docker container - Keep only GITHUB_API_PUBLIC_READONLY_TOKEN as env var - Replace hardcoded build logic with conditional credential handling - Add proper validation for Trello and Cloudflare feedback services - Use dart-define flags instead of environment variables for build configs - Add warning messages for incomplete credential sets * fix executable rights on build.sh * remove the redundant FEEDBACK_TEST_URL, as it will no longer be used in the GUI * swap BUILD_COMMAND and BUILD_CMD (important) BUILD_CMD, which is built dynamically with --dart-define= arguments, should be called last (not during the assets-fetch stage) to ensure the app is actually built with the required defines and that all String.fromEnvironment(...) statements in the Dart code work as expected. * fix(nft): add IPFS gateway resolution, retry, and fallback to improve NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(build): add --no-web-resources-cdn to the web build in build.sh (#3055) * Remove .dgph build artifacts from iOS project (#3058) * remove dgph binaries * Ignore .dgph build artifacts in iOS * fix arb parent ticker * include qtum parent * chore(sdk): bump submodule to 170aab4 for parent display name suffix Pulls in AssetId.displayName and CoinSubClass.tokenStandardSuffix * feat(ui): disambiguate parent chain names via SDK displayName - Use AssetId.displayName for top-level assets (SDK)\n- Update UI to show e.g. Ethereum (ARB20) for parents\n- Keep protocol pill consistent using coin.typeName (NATIVE for parents)\n- Search compares against displayName\n\nRefs: #2988 (comment) --------- Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: charl <charl@vanstaden.info> Co-authored-by: DeckerSU <deckersu@protonmail.com> Co-authored-by: Francois <takenagain@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…lls on user input changes (#3125) * chore(deps): bump sdk to 3f503d2 and fix breaking sparkline change * perf(fiat-onramp): add debouncer to fiat amount input field reduce API requests sent on user input. This was previously managed via `bloc_concurrency` debounce transformer, which was changed to `restartable` after incorrect state updates were reported in testing * refactor(header): use SDK balance for header balance value continuation of migration away from portfolio balance in previous PR * refactor(review): explicit imports, clear flag, and use `.value` factory --------- Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
* docs(setup): add ruby installation step for ios and macos builds * docs(setup): replace specific versions with notes about using latest specific versions are redundant, as there are no currently known version constraints, and the team is generally using the latest available versions of all tools * build(android): bump ndk, platform, and build-tools to latest releases * docs(setup): remove sudo from cocoapods install and fix misc issues
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Just want to note that the current release uses Before the release, p.s. It should look like this: The first part matches Jenkins releases (for example, |
|
If we are not using |
|
@CharlVS we don't have any webpack stuff right now and won't/don't need cache busting as we have a service worker. we can safely remove the highlighted nodejs stuff imo |
|
All new and updated dependencies introduced in this release have been reviewed. No malicious or suspicious components were found. The integrity and authenticity of external packages have been verified where applicable (meaning the versions served from dart.pub and flutter.pub were compared with their corresponding GitHub sources). This review covers only third-party dependencies and external code, not the application’s internal logic or functionality. From an operational security standpoint, the dependency changes LGTM. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/mobile-builds.yml (1)
34-35: Submodules: recursive — OKConsistent with the repo-wide submodule adoption. No further action; prior discussion on shallow depth stands.
🧹 Nitpick comments (3)
.github/workflows/docker-android-build.yml (1)
47-58: Harden GITHUB_TOKEN permissionsYou're exporting the default GITHUB_TOKEN (aliased here as GITHUB_API_PUBLIC_READONLY_TOKEN). For least privilege, explicitly set workflow/job permissions to read-only (contents: read) unless writes are required.
Example (top-level):
permissions: contents: read.github/workflows/desktop-builds.yml (2)
73-76: Reduce GITHUB_TOKEN privilegesThe env variable name suggests read-only, but the default GITHUB_TOKEN may have broader scopes. Explicitly lock down permissions:
At workflow top:
permissions: contents: readTighten further per platform if needed.
85-86: Analytics config OK; consider disabling for CI artifactsMATOMO_URL and MATOMO_SITE_ID are safe to embed. If CI artifacts might run outside controlled environments, consider setting ANALYTICS_DISABLED=true in CI to avoid noisy telemetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/desktop-builds.yml(4 hunks).github/workflows/docker-android-build.yml(2 hunks).github/workflows/docker-linux-build.yml(1 hunks).github/workflows/mobile-builds.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/docker-linux-build.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: osv_scan
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
- GitHub Check: build-linux-docker
- GitHub Check: Build Desktop (windows)
- GitHub Check: build_and_preview
- GitHub Check: Build Desktop (linux)
- GitHub Check: unit_tests
- GitHub Check: validate_code_guidelines
- GitHub Check: Test web-app-linux-profile
- GitHub Check: build-android-docker
- GitHub Check: Test web-app-macos
- GitHub Check: Build Desktop (macos)
- GitHub Check: build_and_deploy
🔇 Additional comments (5)
.github/workflows/docker-android-build.yml (1)
62-66: Good: deterministic artifact suffixShort SHA computation is correct and scoped to bash. No concerns.
.github/workflows/mobile-builds.yml (1)
86-90: Good: short SHA suffix for artifactsStep is correct and consistent across workflows.
.github/workflows/desktop-builds.yml (3)
6-6: Branch filter expansion LGTMAdding fix/* and add/* makes sense for coverage.
89-92: Good: short SHA computationMatches other workflows; no issues.
98-99: Artifact namingUsing .zip in the artifact name is fine; upload-artifact will compress regardless. Just ensure consumers don’t double-zip on download.
| # Optional feedback provider secrets to embed dart-defines | ||
| TRELLO_API_KEY: ${{ secrets.TRELLO_API_KEY }} | ||
| TRELLO_TOKEN: ${{ secrets.TRELLO_TOKEN }} | ||
| TRELLO_BOARD_ID: ${{ secrets.TRELLO_BOARD_ID }} | ||
| TRELLO_LIST_ID: ${{ secrets.TRELLO_LIST_ID }} | ||
| FEEDBACK_API_KEY: ${{ secrets.FEEDBACK_API_KEY }} | ||
| FEEDBACK_PRODUCTION_URL: ${{ secrets.FEEDBACK_PRODUCTION_URL }} | ||
| MATOMO_URL: ${{ secrets.MATOMO_URL }} | ||
| MATOMO_SITE_ID: ${{ secrets.MATOMO_SITE_ID }} |
There was a problem hiding this comment.
🧩 Analysis chain
Do not embed sensitive tokens into the APK
Passing TRELLO_TOKEN (and similar) as build envs risks compiling secrets into the app artifact and leaking them via public artifacts/logs. Matomo URL/site ID are fine as public config; Trello token is not.
Recommended:
- Do not inject TRELLO_TOKEN (or any privileged token) into client builds. Move such calls server-side, or gate usage to CI-only actions that do not end up in the binary.
- If you must keep them for internal QA, avoid uploading those artifacts publicly and gate injection behind a condition (e.g., internal branches) and strip before upload.
Please confirm these secrets are not compiled into the APK and that uploaded artifacts cannot expose them.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== dart-define usage ==="
rg -n 'dart-define' -C3 .github/workflows/docker-android-build.yml || echo "No dart-define found"
echo "=== Flutter build commands ==="
rg -n 'flutter build' -C3 .github/workflows/docker-android-build.yml || echo "No flutter build found"
echo "=== DART_DEFINES usage ==="
rg -n 'DART_DEFINES' -C3 .github/workflows/docker-android-build.yml || echo "No DART_DEFINES found"Length of output: 503
🏁 Script executed:
#!/bin/bash
set -e
echo "=== docker-android-build.yml lines 1-200 ==="
sed -n '1,200p' .github/workflows/docker-android-build.ymlLength of output: 2224
🏁 Script executed:
#!/bin/bash
set -e
echo "=== .docker/build.sh (first 200 lines) ==="
sed -n '1,200p' .docker/build.sh || echo "File not found or empty"
echo "=== dart-define usage in .docker directory ==="
rg -n 'dart-define' -C3 .dockerLength of output: 8481
Do not embed privileged tokens in client builds
- TRELLO_TOKEN (and TRELLO_API_KEY) are passed via --dart-define in .github/workflows/docker-android-build.yml → .docker/build.sh, embedding them into the APK. Move these credentials to server-side or CI-only steps that never end up in the binary.
Confirm removal of all privileged secrets from the build and ensure public artifacts cannot expose them.
| - name: Fetch packages, generate assets, and build for ${{ matrix.platform }} | ||
| env: | ||
| GITHUB_API_PUBLIC_READONLY_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: ${{ matrix.build_command }} | ||
| uses: ./.github/actions/generate-assets | ||
| with: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| TRELLO_API_KEY: ${{ secrets.TRELLO_API_KEY }} | ||
| TRELLO_TOKEN: ${{ secrets.TRELLO_TOKEN }} | ||
| TRELLO_BOARD_ID: ${{ secrets.TRELLO_BOARD_ID }} | ||
| TRELLO_LIST_ID: ${{ secrets.TRELLO_LIST_ID }} | ||
| FEEDBACK_API_KEY: ${{ secrets.FEEDBACK_API_KEY }} | ||
| FEEDBACK_PRODUCTION_URL: ${{ secrets.FEEDBACK_PRODUCTION_URL }} | ||
| MATOMO_URL: ${{ secrets.MATOMO_URL }} | ||
| MATOMO_SITE_ID: ${{ secrets.MATOMO_SITE_ID }} | ||
| BUILD_COMMAND: ${{ matrix.build_command }} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Avoid embedding privileged secrets in mobile apps; consider disabling analytics in CI
- Risk: TRELLO_TOKEN (and related) may end up compiled into iOS/Android artifacts and then uploaded. This is sensitive and should not be client-embedded. Matomo config is safe to embed.
- Actions:
- Remove TRELLO_TOKEN (and any privileged API tokens) from client build inputs; handle server-side or CI-only.
- Gate secret injection to internal builds only and skip artifact upload for such builds, or strip secrets before upload.
- Optionally add ANALYTICS_DISABLED=true for CI/test builds to avoid sending analytics from CI artifacts if they ever run.
Please confirm these secrets are not embedded in the produced artifacts and that artifact visibility will not expose them.
🏁 Script executed:
#!/bin/bash
set -e
echo "Listing generate-assets action files:"
ls -R .github/actions/generate-assets
echo -e "\nInspecting action.yml:"
sed -n '1,200p' .github/actions/generate-assets/action.yml
echo -e "\nSearching for TRELLO_TOKEN in action code:"
rg -n 'TRELLO_TOKEN' -C3 .github/actions/generate-assetsLength of output: 10389
Remove privileged secrets from Flutter build inputs
- The composite action injects
TRELLO_API_KEY,TRELLO_TOKEN,TRELLO_BOARD_ID, andTRELLO_LIST_IDinto theflutter buildvia--dart-define(action.yml lines 114–118), which embeds them in the app binary and can be extracted. - Move Trello feedback operations to a backend/CI-only process or strip these variables before the production build.
- Apply the same gating to
FEEDBACK_API_KEY(and any other sensitive tokens), or setANALYTICS_DISABLED=truefor CI/test builds to prevent unintended analytics from CI artifacts.
…ith occasional flickering (#3187) * fix(maker-bot): sell coin leading widget takes up entire width * fix(maker-bot): remove duplicate event and reduce emit frequency race condition with sell coin selection event emission resulted in sell coin flickering between current and previous coin * chore(sdk): update SDK to 448eecc to switch to KDF staging branch * refactor(maker-bot): use intermediate state and clamp sell amount * fix(maker-bot): add debounce to percentage slider * revert: emit coin selection event early before preimage check prevents input delays while waiting for preimage error checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/bloc/market_maker_bot/market_maker_trade_form/market_maker_trade_form_bloc.dart (2)
134-172: Consider consolidating emissions to reduce state churn.This method emits state up to 3 times in quick succession:
- Line 136: Initial coin selection update
- Line 156: Buy amount update
- Line 170: Preimage error update (if applicable)
While this provides incremental UI updates, rapid successive emissions can cause unnecessary widget rebuilds and potential flickering. Consider whether some of these can be consolidated, especially the buy amount calculation (lines 150-163) which could potentially be included in the initial emission if the calculation is fast enough.
474-479: Handle zero‐value sell amounts in _getMaxSellAmountFromPreImage
Clamping prevents negatives, but"0"still passes.isEmptyand yields a zero‐value trade. Add a numeric<= 0validation (e.g. enforce a dust limit or emit an error) to reject or properly handle zero‐amount trades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(2 hunks)lib/bloc/market_maker_bot/market_maker_trade_form/market_maker_trade_form_bloc.dart(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Format Dart code using
dart format .before committing
Files:
lib/bloc/market_maker_bot/market_maker_trade_form/market_maker_trade_form_bloc.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: Test web-app-macos
- GitHub Check: Cursor Bugbot
- GitHub Check: validate_code_guidelines
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (windows)
- GitHub Check: unit_tests
- GitHub Check: build_and_preview
- GitHub Check: build-android-docker
- GitHub Check: build-linux-docker
- GitHub Check: build_and_deploy
🔇 Additional comments (4)
lib/bloc/market_maker_bot/market_maker_trade_form/market_maker_trade_form_bloc.dart (4)
86-94: LGTM: Improved UI responsiveness.Calculating
newBuyAmountupfront and including it in the initial emission provides faster feedback to the user.
114-121: Verify preimage error handling doesn't mask activation failures.If
_autoActivateCointhrows an error (line 112), the preimage check at line 116 will still execute. Consider whether activation failures should prevent preimage checks or be surfaced to the user.
184-224: LGTM: Clear separation of immediate update and async validation.The immediate emission at line 196 provides responsive UI feedback, while the async preimage validation at lines 206-224 ensures correctness. The pattern is clear and maintainable.
109-112: Clarify or remove the TODO comment—it incorrectly describes the coin filtering logic.The sell coin dropdown filters by USD balance only, not by active status. The code at lines 177–181 of
lib/views/market_maker_bot/market_maker_bot_form_content.dartshows:List<Coin> _coinsWithUsdBalance(List<Coin> coins) { return coins .where((coin) => (coin.lastKnownUsdBalance(context.sdk) ?? 0) > 0) .toList(); }There is no
isActivefilter. The TODO's assumption that "only enabled coins with a balance are displayed" is inaccurate. Since coins without active status could theoretically have a USD balance, the_autoActivateCoin()call at line 112 is a necessary defensive activation. Either remove the comment entirely (if the activation is warranted), or update it to accurately reflect that coins are filtered by balance, not active status, and explain why pre-activation is still needed.
… reactivation (#3184) * fix(zcoin): show zcoin on reactivation zcoin activation function simply discarded coins that were already active without notifying listeners to display the coin in UI * style(zhtlc): move block size and scan interval to advanced section * feat(zhtlc): add config modification option to coin details page * fix(zhtlc): simplify the config update and reactivation * chore(sdk): update SDK to 448eecc to switch to KDF staging branch * chore(zhtlc): reword dialog title and apply suggestions * fix(zhtlc): coin incorrectly added to list on dialog cancel * fix(zcoin): rethrow if user action is not mismatch of assumptions: activateArrr returns an activation error result rather than throwing an exception, so it has to be rethrown in the error handler for it to bubble up. there is also a mismatch between single and multiple activations, but that is fine for now since single activations are all that are present --------- Co-authored-by: Nitride <77973576+CharlVS@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/bloc/coins_bloc/coins_repo.dart (2)
95-114: Existing subscriptions not cancelled when assets become blocked.The blocking check prevents NEW subscriptions for blocked assets (lines 99-102). However, if an asset becomes blocked AFTER activation, the existing subscription continues to run, wasting resources by watching untradable assets.
Consider adding a method to reactively cancel subscriptions when trading status changes:
+ /// Cancel balance subscription for an asset + void _cancelBalanceSubscription(AssetId assetId) { + _balanceWatchers[assetId]?.cancel(); + _balanceWatchers.remove(assetId); + } + /// Subscribe to balance updates for an asset using the SDK's balance manager void _subscribeToBalanceUpdates(Asset asset) { // Cancel any existing subscription for this asset _balanceWatchers[asset.id]?.cancel(); if (_tradingStatusService.isAssetBlocked(asset.id)) { _log.info('Asset ${asset.id.id} is blocked. Skipping balance updates.'); return; } // Start a new subscription _balanceWatchers[asset.id] = _kdfSdk.balances.watchBalance(asset.id).listen( (balanceInfo) { // Update the balance cache with the new values _balancesCache[asset.id.id] = ( balance: balanceInfo.total.toDouble(), spendable: balanceInfo.spendable.toDouble(), ); }, ); }Then, listen to trading status changes and cancel subscriptions for newly-blocked assets:
// In constructor or initialization _tradingStatusService.watchBlockedAssets().listen((blockedAssets) { for (final assetId in blockedAssets) { _cancelBalanceSubscription(assetId); } });
625-633: Guard against empty asset list.If
assetsis empty,boundedChunkSizebecomes 0 andassets.slices(0)throws anArgumentError, breakingfetchCurrentPriceswhenever no assets are available.Apply this diff to guard against empty assets:
Future<void> _fetchAssetPricesInChunks( List<Asset> assets, { int chunkSize = 12, }) async { + if (assets.isEmpty) { + return; + } final boundedChunkSize = min(assets.length, chunkSize); final chunks = assets.slices(boundedChunkSize); for (final chunk in chunks) { await Future.wait(chunk.map(_fetchAssetPrice), eagerError: false); } }
🧹 Nitpick comments (5)
lib/services/arrr_activation/arrr_activation_service.dart (3)
120-127: Consider reducing the configuration timeout duration.The 15-minute timeout for configuration requests is quite long. Most user interactions should complete within 1-5 minutes. A long timeout can leave resources held unnecessarily if users abandon the configuration flow.
Consider reducing the timeout to 3-5 minutes or making it configurable:
+ /// Default timeout for configuration requests + static const _configurationTimeout = Duration(minutes: 5); + try { config = await completer.future.timeout( - const Duration(minutes: 15), + _configurationTimeout, onTimeout: () {
383-406: Consider improving UI listener detection.The polling approach works but could be more efficient. Additionally, continuing execution when no listeners are detected (line 394-398) could result in lost configuration requests that are emitted but never received.
Consider:
- Using a Completer that gets completed when the first listener subscribes
- Throwing an error if no listeners are detected after the timeout, rather than continuing
Future<void> _waitForUIListeners(AssetId assetId) async { const maxWaitTime = Duration(seconds: 10); const checkInterval = Duration(milliseconds: 100); final stopwatch = Stopwatch()..start(); while (!_configRequestController.hasListener && stopwatch.elapsed < maxWaitTime) { _log.info('Waiting for UI listeners to be ready for ${assetId.id}...'); await Future.delayed(checkInterval); } if (!_configRequestController.hasListener) { - _log.warning( + final message = 'No UI listeners detected after ${maxWaitTime.inSeconds} seconds for ${assetId.id}. ' - 'Make sure ZhtlcConfigurationHandler is in the widget tree.', + 'Make sure ZhtlcConfigurationHandler is in the widget tree.'; - ); + _log.severe(message); + throw StateError(message); } else { _log.info( 'UI listeners ready for ${assetId.id} after ${stopwatch.elapsed.inMilliseconds}ms', ); } stopwatch.stop(); }
154-216: Consider distinguishing retryable from non-retryable errors.The retry logic currently treats all activation failures as retryable by wrapping them in
_RetryableZhtlcActivationException(line 192). Some errors may not benefit from retries, such as invalid configuration, permission errors, or service unavailability. Retrying these wastes time and resources.Consider analyzing the error type or message to determine if retry is appropriate:
if (lastActivationProgress?.isSuccess ?? false) { await _cacheActivationComplete(asset.id); return ArrrActivationResultSuccess( Stream.value( ActivationProgress( status: 'Activation completed successfully', progressPercentage: 100, isComplete: true, progressDetails: ActivationProgressDetails( currentStep: ActivationStep.complete, stepCount: 1, ), ), ), ); } final errorMessage = lastActivationProgress?.errorMessage ?? 'Unknown activation error'; + + // Determine if the error is retryable based on the error message + final isRetryable = !errorMessage.contains('invalid') && + !errorMessage.contains('permission') && + !errorMessage.contains('unauthorized'); + + if (!isRetryable) { + // Don't retry for permanent errors + throw Exception(errorMessage); + } + throw _RetryableZhtlcActivationException(errorMessage);lib/bloc/coins_bloc/coins_repo.dart (2)
752-815: Consider extracting common post-activation logic.The method handles already-activated and newly-activated assets separately, but both paths perform similar post-activation tasks (broadcasting state, subscribing to balances, registering icons). This duplication increases maintenance burden.
Extract common logic into a helper method:
+ /// Common post-activation tasks for ZHTLC assets + void _handleZhtlcPostActivation( + Asset asset, + Coin coin, { + bool notifyListeners = true, + }) { + // Broadcast active state + if (notifyListeners) { + _broadcastAsset(coin.copyWith(state: CoinState.active)); + if (coin.id.parentId != null) { + final parentCoin = _assetToCoinWithoutAddress( + _kdfSdk.assets.available[coin.id.parentId]!, + ); + _broadcastAsset(parentCoin.copyWith(state: CoinState.active)); + } + } + + // Subscribe to balance updates + _subscribeToBalanceUpdates(asset); + if (coin.id.parentId != null) { + final parentAsset = _kdfSdk.assets.available[coin.id.parentId]; + if (parentAsset != null) { + _subscribeToBalanceUpdates(parentAsset); + } + } + + // Register custom icon + if (coin.logoImageUrl?.isNotEmpty ?? false) { + AssetIcon.registerCustomIcon(coin.id, NetworkImage(coin.logoImageUrl!)); + } + } Future<void> _activateZhtlcAssets(...) async { for (final asset in assets) { final coin = coins.firstWhere((coin) => coin.id == asset.id); final isAlreadyActivated = activatedAssets.any((a) => a.id == asset.id); if (isAlreadyActivated) { if (addToWalletMetadata) { await _addAssetsToWalletMetdata([asset.id]); } - // Broadcast, subscribe, register icon... + _handleZhtlcPostActivation(asset, coin, notifyListeners: notifyListeners); } else { await _activateZhtlcAsset(...); } } }
876-892: User cancellation detection is fragile.The code detects user cancellation by checking if the error message contains "cancelled by user" (line 883). This string-matching approach is fragile and will break if the error message format changes in
ArrrActivationService.Consider using a dedicated exception type or error code for user cancellations:
In
lib/services/arrr_activation/arrr_activation_service.dart:class UserCancelledException implements Exception { const UserCancelledException(this.message); final String message; }Then in
ArrrActivationService, throw this specific exception:return ArrrActivationResultError( 'Configuration cancelled by user or timed out', isUserCancellation: true, // Add this field );And in
_activateZhtlcAsset, check the type:error: (message) { _log.severe( 'ZHTLC asset activation failed: ${asset.id.id} - $message', ); - final isUserCancellation = message.contains('cancelled by user'); + final isUserCancellation = result.isUserCancellation ?? false; if (notifyListeners && !isUserCancellation) { _broadcastAsset(coin.copyWith(state: CoinState.suspended)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (4)
assets/translations/en.json(11 hunks)lib/bloc/coins_bloc/coins_repo.dart(20 hunks)lib/main.dart(3 hunks)lib/services/arrr_activation/arrr_activation_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Format Dart code using
dart format .before committing
Files:
lib/main.dartlib/bloc/coins_bloc/coins_repo.dartlib/services/arrr_activation/arrr_activation_service.dart
🧠 Learnings (2)
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: In the Komodo Wallet project, part files share imports with their parent files. The import for `app_config.dart` in `coins_bloc.dart` is necessary because the part file `coins_state.dart` uses `excludedAssetList` from that package.
Applied to files:
lib/main.dartlib/bloc/coins_bloc/coins_repo.dart
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: The `excludedAssetList` from `app_config.dart` is used in the `_filterExcludedAssets` method in `coins_state.dart`. Since `coins_state.dart` is a part file of `coins_bloc.dart`, the import needs to be in the parent file even though it's not directly used there. In Dart, part files share the namespace and imports of their parent files.
Applied to files:
lib/bloc/coins_bloc/coins_repo.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: build-android-docker
- GitHub Check: build_and_preview
- GitHub Check: Test web-app-macos
- GitHub Check: Test web-app-linux-profile
- GitHub Check: validate_code_guidelines
- GitHub Check: unit_tests
- GitHub Check: build-linux-docker
- GitHub Check: Cursor Bugbot
- GitHub Check: build_and_deploy
🔇 Additional comments (7)
lib/services/arrr_activation/arrr_activation_service.dart (2)
16-19: LGTM!The constructor properly initializes dependencies and immediately subscribes to authentication state changes, ensuring the service responds to auth lifecycle events.
516-536: LGTM!The disposal logic is thorough and well-ordered. It prevents new operations, cancels subscriptions, completes pending operations with errors, and properly closes resources.
lib/main.dart (1)
63-84: LGTM!The initialization sequence is well-ordered, ensuring dependencies are initialized before their consumers. The comment explaining Sparkline's Hive dependency is helpful for maintainability.
lib/bloc/coins_bloc/coins_repo.dart (4)
34-47: LGTM!The constructor properly accepts and stores the new service dependencies while maintaining the existing initialization pattern.
257-279: LGTM!The parameter rename from
notifytonotifyListenersimproves clarity and is consistently applied throughout the method signature and documentation.
287-309: LGTM!The separation of ZHTLC and regular assets is well-structured. Processing ZHTLC assets first ensures proper handling of their unique configuration requirements before proceeding with standard activations.
545-572: LGTM!The expanded deprecation message provides clear guidance for migration and the implementation properly handles edge cases like multiple assets matching a single config ID.
| } | ||
|
|
||
| if (!isUserCancellation) { | ||
| throw Exception("zcoin activaiton failed: $message"); |
There was a problem hiding this comment.
Fix typo in error message.
Line 890 contains a typo: "zcoin activaiton" should be "zcoin activation".
Apply this fix:
if (!isUserCancellation) {
- throw Exception("zcoin activaiton failed: $message");
+ throw Exception("zcoin activation failed: $message");
}📝 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.
| throw Exception("zcoin activaiton failed: $message"); | |
| if (!isUserCancellation) { | |
| throw Exception("zcoin activation failed: $message"); | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around line 890, fix the typo in the
thrown exception message: change "zcoin activaiton failed: $message" to "zcoin
activation failed: $message" so the error text reads correctly; update only the
string literal in the Exception constructor.
| child: MultiRepositoryProvider( | ||
| providers: [ | ||
| RepositoryProvider.value(value: komodoDefiSdk), | ||
| RepositoryProvider.value(value: mm2Api), | ||
| RepositoryProvider.value(value: arrrActivationService), | ||
| RepositoryProvider.value(value: coinsRepo), | ||
| RepositoryProvider.value(value: walletsRepository), | ||
| RepositoryProvider.value(value: sparklineRepository), | ||
| RepositoryProvider.value(value: tradingStatusRepository), | ||
| RepositoryProvider.value(value: tradingStatusService), | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify disposal handling for new services.
The provider configuration correctly exposes all newly initialized services. However, ensure that services requiring disposal (like ArrrActivationService which has a dispose() method) are properly disposed when the app terminates.
Verify disposal handling for the new services:
🏁 Script executed:
#!/bin/bash
# Check if ArrrActivationService has disposal handling
rg -n "dispose.*arrrActivationService|ArrrActivationService.*dispose" --type dart
# Check if there's a global disposal mechanism for services
ast-grep --pattern $'class $_App {
$$$
@override
void dispose() {
$$$
}
$$$
}'Length of output: 256
Implement disposal for ArrrActivationService (and any other services with a dispose() method) in the root widget’s dispose().
Override your app’s dispose() (e.g., in MyApp) to call arrrActivationService.dispose() (and dispose additional services as needed).
🤖 Prompt for AI Agents
In lib/main.dart around lines 98 to 108, ArrrActivationService (and possibly
other services provided via RepositoryProvider) exposes a dispose() method but
the root widget does not override dispose() to call it; override your app's root
StatefulWidget (e.g., MyApp) dispose() and call arrrActivationService.dispose()
(and invoke dispose() on any other provided services that implement dispose),
ensuring you obtain the service instances (e.g., from fields or
RepositoryProvider) and clean them up before calling super.dispose().
| return MultiBlocProvider( | ||
| providers: [ | ||
| BlocProvider<AuthBloc>( | ||
| create: (_) { | ||
| final bloc = AuthBloc( | ||
| komodoDefiSdk, walletsRepository, SettingsRepository()); | ||
| komodoDefiSdk, | ||
| walletsRepository, | ||
| SettingsRepository(), | ||
| tradingStatusService, | ||
| ); | ||
| bloc.add(const AuthLifecycleCheckRequested()); | ||
| return bloc; | ||
| }, | ||
| ), | ||
| ], | ||
| child: BetterFeedback( | ||
| feedbackBuilder: CustomFeedbackForm.feedbackBuilder, | ||
| themeMode: ThemeMode.light, | ||
| darkTheme: _feedbackThemeData(theme), | ||
| theme: _feedbackThemeData(theme), | ||
| child: AppFeedbackWrapper( | ||
| child: AnalyticsLifecycleHandler( | ||
| child: WindowCloseHandler( | ||
| child: app_bloc_root.AppBlocRoot( | ||
| storedPrefs: _storedSettings!, | ||
| komodoDefiSdk: komodoDefiSdk, | ||
| child: ScreenshotSensitivity( | ||
| controller: sensitivityController, | ||
| child: app_bloc_root.AppBlocRoot( | ||
| storedPrefs: _storedSettings!, | ||
| komodoDefiSdk: komodoDefiSdk, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify ScreenshotSensitivityController disposal.
The widget tree structure is well-organized with proper dependency injection. The ScreenshotSensitivityController is created at line 165 but there's no visible disposal. Ensure the controller is properly disposed when MyApp is disposed to prevent resource leaks.
Check if ScreenshotSensitivityController requires disposal:
🏁 Script executed:
#!/bin/bash
# Check the controller's implementation
ast-grep --pattern $'class ScreenshotSensitivityController {
$$$
dispose() {
$$$
}
$$$
}'
# Check if StatelessWidget needs conversion to StatefulWidget for disposal
rg -n "class MyApp extends.*Widget" lib/main.dartLength of output: 41
---
🏁 Script executed:
```shell
#!/bin/bash
# Find ScreenshotSensitivityController definition and implementation
rg -n "class ScreenshotSensitivityController" --type dart
# Find ScreenshotSensitivity widget and its build method
rg -n "class ScreenshotSensitivity" --type dart -A 20
# Check if controller is passed to a provider or inherited widget
rg -n "ScreenshotSensitivityController" lib/main.dart -B 2 -A 2
Length of output: 3673
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for dispose calls on sensitivityController
rg -n "sensitivityController" lib/main.dart
# Check if there's a dispose method in ChangeNotifier or StatefulWidget managing this
rg -n "dispose\(\)" lib/main.dart -B 2 -A 2
# Check complete MyApp class structure
rg -n "class MyApp" lib/main.dart -A 50
Length of output: 1611
Convert MyApp to StatefulWidget and dispose ScreenshotSensitivityController
Refactor MyApp (lib/main.dart) from StatelessWidget to StatefulWidget, move sensitivityController instantiation into initState, and override dispose() to call sensitivityController.dispose() to avoid ChangeNotifier leaks.
🤖 Prompt for AI Agents
In lib/main.dart around lines 166 to 194, MyApp is currently a StatelessWidget
and creates a ScreenshotSensitivity controller inline causing a ChangeNotifier
leak; convert MyApp to a StatefulWidget, promote sensitivityController to a late
final field on the State class, instantiate it in initState(), and override
dispose() to call sensitivityController.dispose(); update references in the
build method to use the State's controller instance so the controller lifecycle
is properly managed.
| Future<void> _cleanupOnSignOut() async { | ||
| _log.info('User signed out - cleaning up active ZHTLC activations'); | ||
|
|
||
| // Cancel all pending configuration requests | ||
| final pendingAssets = _configCompleters.keys.toList(); | ||
| for (final assetId in pendingAssets) { | ||
| final completer = _configCompleters[assetId]; | ||
| if (completer != null && !completer.isCompleted) { | ||
| _log.info('Cancelling pending configuration request for ${assetId.id}'); | ||
| completer.complete(null); | ||
| } | ||
| } | ||
| _configCompleters.clear(); | ||
|
|
||
| // Clear activation cache as it's user-specific | ||
| var activeAssets = <AssetId>[]; | ||
| await _activationCacheMutex.protectWrite(() async { | ||
| activeAssets = _activationCache.keys.toList(); | ||
| for (final assetId in activeAssets) { | ||
| _log.info('Clearing activation status for ${assetId.id}'); | ||
| } | ||
| _activationCache.clear(); | ||
| }); | ||
|
|
||
| _log.info( | ||
| 'Cleanup completed - cancelled ${pendingAssets.length} pending configs and cleared ${activeAssets.length} activation statuses', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify ongoing activations are properly handled on sign-out.
The cleanup logic properly cancels pending configuration requests and clears the activation cache. However, it doesn't explicitly handle the ongoing activation futures stored in _ongoingActivations (line 34). While these futures will eventually complete or error out, they might continue consuming resources after sign-out.
Verify whether ongoing activation futures need explicit cancellation or if they naturally terminate when the auth state changes. Run the following to check if activations reference auth-dependent resources:
🏁 Script executed:
#!/bin/bash
# Check if activation methods access auth-dependent resources
ast-grep --pattern $'_activateArrrInternal($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate declaration and usage of _ongoingActivations
rg -n '_ongoingActivations' -C3 lib/services/arrr_activation/arrr_activation_service.dart
rg -n '_ongoingActivations' -C3 .Length of output: 3349
Clear ongoing activations on sign-out
Add _ongoingActivations.clear() inside _cleanupOnSignOut() to drop in-flight activation futures and prevent reuse of stale futures across user sessions.
🤖 Prompt for AI Agents
In lib/services/arrr_activation/arrr_activation_service.dart around lines 425 to
452, the cleanup routine does not clear the _ongoingActivations map/collection
so in-flight activation futures can be reused across sessions; add a call to
_ongoingActivations.clear() inside _cleanupOnSignOut() (preferably after
clearing _activationCache within the protected write block or immediately after
it) to drop any in-flight activation futures and ensure no stale futures remain,
keeping the operation thread-safe by performing the clear while still in the
mutex-protected section or right after it before the final log statement.
| Future<void> updateZhtlcConfig( | ||
| Asset asset, | ||
| ZhtlcUserConfig newConfig, | ||
| ) async { | ||
| if (_isDisposing || _configRequestController.isClosed) { | ||
| throw StateError('ArrrActivationService has been disposed'); | ||
| } | ||
|
|
||
| _log.info('Updating ZHTLC configuration for ${asset.id.id}'); | ||
|
|
||
| try { | ||
| // Cancel any pending configuration requests | ||
| final completer = _configCompleters[asset.id]; | ||
| if (completer != null && !completer.isCompleted) { | ||
| _log.info( | ||
| 'Cancelling pending configuration request for ${asset.id.id}', | ||
| ); | ||
| completer.complete(null); | ||
| _configCompleters.remove(asset.id); | ||
| } | ||
|
|
||
| // 2. Disable the coin if it's currently active | ||
| await _disableCoin(asset.id.id); | ||
|
|
||
| // 3. Store the new configuration | ||
| _log.info('Saving new configuration for ${asset.id.id}'); | ||
| await _configService.saveZhtlcConfig(asset.id, newConfig); | ||
| } catch (e, stackTrace) { | ||
| _log.severe( | ||
| 'Failed to update ZHTLC configuration for ${asset.id.id}', | ||
| e, | ||
| stackTrace, | ||
| ); | ||
| await _cacheActivationError(asset.id, e.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix error handling and ensure atomic configuration updates.
The method has two issues:
- Swallowed exceptions: Exceptions are logged and cached (line 492) but not rethrown, so callers don't know the update failed.
- Non-atomic updates: If
saveZhtlcConfigfails after_disableCoin, the coin remains disabled without a valid configuration, leaving it in a broken state.
Apply this diff to fix error handling and add transaction-like semantics:
Future<void> updateZhtlcConfig(
Asset asset,
ZhtlcUserConfig newConfig,
) async {
if (_isDisposing || _configRequestController.isClosed) {
throw StateError('ArrrActivationService has been disposed');
}
_log.info('Updating ZHTLC configuration for ${asset.id.id}');
try {
// Cancel any pending configuration requests
final completer = _configCompleters[asset.id];
if (completer != null && !completer.isCompleted) {
_log.info(
'Cancelling pending configuration request for ${asset.id.id}',
);
completer.complete(null);
_configCompleters.remove(asset.id);
}
+ // Save new configuration BEFORE disabling the coin
+ // This ensures we don't leave the coin in a broken state
+ _log.info('Saving new configuration for ${asset.id.id}');
+ await _configService.saveZhtlcConfig(asset.id, newConfig);
+
// 2. Disable the coin if it's currently active
await _disableCoin(asset.id.id);
-
- // 3. Store the new configuration
- _log.info('Saving new configuration for ${asset.id.id}');
- await _configService.saveZhtlcConfig(asset.id, newConfig);
} catch (e, stackTrace) {
_log.severe(
'Failed to update ZHTLC configuration for ${asset.id.id}',
e,
stackTrace,
);
await _cacheActivationError(asset.id, e.toString());
+ rethrow; // Let caller know the update failed
}
}🤖 Prompt for AI Agents
In lib/services/arrr_activation/arrr_activation_service.dart around lines 459 to
494, change updateZhtlcConfig so failures are not swallowed and the update is
atomic: first persist the newConfig via _configService.saveZhtlcConfig, then
disable the coin via _disableCoin; if disabling fails, attempt to roll back by
re-enabling the coin (or delete/restore the saved config as appropriate) and
propagate the original error; always log and cache the error, then rethrow the
exception so callers see the failure; ensure any pending completer is still
cancelled/cleaned up in a finally block to avoid leaked state.
Roll SDK to update KDF version to the latest v2.5.2-beta RC. TODO! Update to the GH KDF release upon its release.
…racking (#3194) * chore(analytics): remove scroll attempt event and update status * Update lib/analytics/required_analytics_events.csv Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/analytics/analytics_factory.dart (1)
408-417: Broken reference confirmed: Remove unused factory methodThe class
ScrollAttemptOutsideContentEventdoes not exist in the codebase. The factory method at lines 408-417 references a non-existent class and will cause a compile error. Since no callers ofscrollAttemptOutsideContent()exist, remove the entire method.Apply:
- /// Scroll attempt outside content - static ScrollAttemptOutsideContentEvent scrollAttemptOutsideContent({ - required String screenContext, - required double scrollDelta, - }) { - return ScrollAttemptOutsideContentEvent( - screenContext: screenContext, - scrollDelta: scrollDelta, - ); - }
♻️ Duplicate comments (3)
lib/bloc/analytics/matomo_analytics_api.dart (2)
299-307: Initialization failure is swallowed after retry cap; rethrow to surface failureAfter exhausting retries, initialize completes without error, leaving provider unusable but callers unaware. Rethrow after completing the error to fail fast.
Apply:
} else { // Maximum retries exceeded if (!_initCompleter.isCompleted) { _initCompleter.completeError(e); } + rethrow; }
227-241: Prevent unbounded queue growth when Matomo is permanently disabled (no config)With missing MATOMO_URL/SITE_ID, sendEvent() enqueues forever; there is no path to activation, risking OOM. Track config presence and drop events early when invalid.
Apply:
class MatomoAnalyticsApi implements AnalyticsApi { late MatomoTracker _instance; final Completer<void> _initCompleter = Completer<void>(); bool _isInitialized = false; bool _isEnabled = false; + bool _hasValidConfig = false; int _initRetryCount = 0; @@ - final bool hasConfig = matomoUrl.isNotEmpty && matomoSiteId.isNotEmpty; + final bool hasConfig = matomoUrl.isNotEmpty && matomoSiteId.isNotEmpty; + _hasValidConfig = hasConfig; if (!hasConfig) { @@ Future<void> sendEvent(AnalyticsEventData event) async { + // If Matomo is hard-disabled by missing config, drop the event. + if (!_hasValidConfig) { + if (kDebugMode) { + log( + 'Matomo disabled (missing config); dropping ${event.name}', + path: 'analytics -> MatomoAnalyticsApi -> sendEvent', + ); + } + return; + } // If not initialized or disabled, enqueue for later if (!_isInitialized || !_isEnabled) { _eventQueue.add(event); return; }Optional: bound the queue even when temporarily disabled (e.g., keep last N=1000) to guard against long disable windows.
Also applies to: 320-325
lib/analytics/events/misc_events.dart (1)
115-117: Parameter key rename breaks consistency; revert to 'asset_symbol'Factory emits 'asset_symbol', but this class sends 'asset'. This divergence can break analytics expecting the original key (e.g., Firebase, CSV contracts).
Apply:
JsonMap get parameters => { 'query_length': queryLength, - if (assetSymbol != null) 'asset': assetSymbol!, + if (assetSymbol != null) 'asset_symbol': assetSymbol!, };Matomo normalization already maps asset_symbol → asset, so this preserves downstream expectations without losing Matomo data. Based on learnings.
🧹 Nitpick comments (1)
lib/analytics/analytics_factory.dart (1)
20-24: Unify parameter type to JsonMap for consistencyOther events use JsonMap; this one uses Map<String, Object>. Align for consistency and easier refactors.
Apply:
- @override - Map<String, Object> get parameters => { + @override + JsonMap get parameters => { 'timeframe': timeframe, 'realized_pnl': realizedPnl, 'unrealized_pnl': unrealizedPnl, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/analytics/required_analytics_events.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
docs/ANALYTICS_EVENT_IMPLEMENTATION_PLAN.md(1 hunks)lib/analytics/analytics_factory.dart(30 hunks)lib/analytics/events/misc_events.dart(7 hunks)lib/bloc/analytics/matomo_analytics_api.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Format Dart code using
dart format .before committing
Files:
lib/bloc/analytics/matomo_analytics_api.dartlib/analytics/analytics_factory.dartlib/analytics/events/misc_events.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Cursor Bugbot
- GitHub Check: build_and_preview
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (windows)
- GitHub Check: validate_code_guidelines
- GitHub Check: Test web-app-linux-profile
- GitHub Check: Test web-app-macos
- GitHub Check: unit_tests
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
- GitHub Check: build-android-docker
- GitHub Check: build-linux-docker
- GitHub Check: build_and_deploy
| // Include backend API key header similarly to feedback feature | ||
| customHeaders: { | ||
| if (const String.fromEnvironment('FEEDBACK_API_KEY').isNotEmpty) | ||
| 'X-KW-KEY': const String.fromEnvironment('FEEDBACK_API_KEY'), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Do not send FEEDBACK_API_KEY as a header to analytics endpoint
Passing X-KW-KEY derived from FEEDBACK_API_KEY to Matomo risks leaking a backend secret to a third-party endpoint (or separate infra). The Matomo Tracking API does not require such a key for client-side tracking.
Apply:
await MatomoTracker.instance.initialize(
siteId: matomoSiteId,
url: matomoUrl,
dispatchSettings: const DispatchSettings.persistent(),
- // Include backend API key header similarly to feedback feature
- customHeaders: {
- if (const String.fromEnvironment('FEEDBACK_API_KEY').isNotEmpty)
- 'X-KW-KEY': const String.fromEnvironment('FEEDBACK_API_KEY'),
- },
);If authentication is required for a private Matomo, use a Matomo-specific public token intended for tracking, not a backend API key, and only when the endpoint is under your control. Based on learnings.
📝 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.
| // Include backend API key header similarly to feedback feature | |
| customHeaders: { | |
| if (const String.fromEnvironment('FEEDBACK_API_KEY').isNotEmpty) | |
| 'X-KW-KEY': const String.fromEnvironment('FEEDBACK_API_KEY'), | |
| }, | |
| ); | |
| await MatomoTracker.instance.initialize( | |
| siteId: matomoSiteId, | |
| url: matomoUrl, | |
| dispatchSettings: const DispatchSettings.persistent(), | |
| ); |
🤖 Prompt for AI Agents
In lib/bloc/analytics/matomo_analytics_api.dart around lines 247–252, remove
sending the backend FEEDBACK_API_KEY as the X-KW-KEY header to the Matomo
endpoint (this leaks a backend secret). Instead, do not include that header; if
authentication is required for a private Matomo instance, read a dedicated
Matomo public/tracking token from a separate environment variable (e.g.
MATOMO_PUBLIC_TOKEN) and send that token (or the appropriate Matomo tracking
parameter) only when the endpoint is under your control; ensure the env var is
intended for client-side use and document it so backend secrets are never
exposed.
* chore(sdk): roll to origin/bugfix/activation-asset-grouping for asset type tagging fix * chore(sdk): roll to origin/fix/asset-chain-tagging * fix(asset-tagging): set SMART_CHAIN ticker; improve parsing - Sanitize ticker parsing for exact/partial matches\n- Restore partial ticker matching to preserve behavior\n- Prefer UTXO on exact "UTXO" to avoid mislabeling\n- Catch StateError specifically and deduplicate enum name extraction\n- Confirm UTXO formatted label is "Native" * chore(submodule): roll SDK to ec40963 (fix SMART_CHAIN ticker, parsing) * Delete run.sh
…n signing support for standalone distribution (#3185) * change product bundle identifier (com.komodo.wallet) * Set sdk submodule to chore/sdk-universal-macos-cdn-pods #3175 * XCode: add production scheme and corresponding configurations * make Release-production sign with Developer ID Application * sdk: temp following submodules for patch-macos-release-profile new https://github.com/KomodoPlatform/komodo-defi-sdk-flutter/tree/patch-macos-release-profile branch for macos Developer ID Application release preparations. * sdk: update submodules * add OTHER_CODE_SIGN_FLAGS --options=runtime --timestamp * fix developement team for Release-production after this - kdf binary is signed (but still without timestamp) * fix codesigning for Pods (now CP-User signing in Pods receive correct env) 'Release-production'=> :release, - CocoaPods will create this config in Pods project * add --options=runtime --timestamp for codesigning pods * sdk: pin version with code_sign_if_enabled in cp-user sh script * sdk: bump to new location of KDF binary under MacOS * macos: XCode project update / Pods_Runner.framework in Frameworks * build(macos): set CODE_SIGN_INJECT_BASE_ENTITLEMENTS to NO for Release need to remove below from Entitlements "com.apple.security.get-task-allow" = 1; To check: codesign -d --entitlements :- "build/macos/Build/Products/Release-production/Komodo Wallet.app" | plutil -p - Read more: https://developer.apple.com/documentation/security/resolving-common-notarization-issues#3087731 * update .gitignore to exclude dist/ directory * feat: add make-dmg.sh script for building DMG files on macOS This script automates the process of creating a DMG file for the Komodo Wallet application, including setting up the Finder layout and handling background images. It requires macOS and utilizes hdiutil, osascript, and ditto for the operations. * feat: add test-sign-timestamp.sh script for verifying code signing and timestamping on macOS This script checks the code signing and timestamping of the Komodo Wallet application, providing feedback on the presence of timestamps and the authority of the timestamping service. It simplifies the verification process for macOS app builds. * feat: enhance make-dmg.sh script with default app path and usage instructions * chore: update Podfile.lock to include device_info_plus and update dependencies * chore: update subproject commit reference in sdk * feat: add README.md for contrib scripts and enhance test-sign-timestamp.sh * chore(sdk): roll submodule to latest dev - Update .gitmodules to track dev - Sync sdk to origin/dev --------- Co-authored-by: Nitride <77973576+CharlVS@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contrib/make-dmg.sh (2)
16-30: Consider input validation for numeric parameters (optional refactor).Parameters like
WIN_W,WIN_H,APP_X,APP_Yare substituted directly into AppleScript (line 165) via shell variable expansion. While default values are numeric, custom values passed via environment variables are not validated. Non-numeric input would cause AppleScript syntax errors without clear diagnostics.If caller responsibility is acceptable, this is fine. If robustness is preferred, consider a lightweight validation step:
# Optional validation function validate_numeric() { local var="$1" if ! [[ "$var" =~ ^[0-9]+$ ]]; then echo >&2 "ERROR: $var must be numeric, got: $var" exit 1 fi } # Call before using: # validate_numeric "$WIN_W" # validate_numeric "$WIN_H" # validate_numeric "$APP_X" # validate_numeric "$APP_Y"
140-206: AppleScript error handling could be more explicit (optional improvement).The AppleScript block uses multiple
try...end tryblocks without reporting errors if they occur (lines 155–205). Silent failures can mask issues like missing .background folder, icon positioning problems, or window sizing failures. While the pragmatic approach assumes Finder will handle missing operations gracefully, explicit error logging would improve debuggability during DMG staging.Current approach is functional, but consider adding error reporting if debugging time becomes an issue:
osascript <<OSAEOF 2>&1 | while read line; do [[ "$line" =~ error|Error ]] && echo "WARNING: AppleScript: $line" done # ... rest of osascript block
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
macos/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.gitignore(3 hunks)contrib/README.md(1 hunks)contrib/make-dmg.sh(1 hunks)contrib/test-sign-timestamp.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Cursor Bugbot
- GitHub Check: Test web-app-macos
- GitHub Check: Test web-app-linux-profile
- GitHub Check: build_and_preview
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Desktop (windows)
- GitHub Check: unit_tests
- GitHub Check: Build Desktop (linux)
- GitHub Check: validate_code_guidelines
- GitHub Check: build-android-docker
- GitHub Check: build-linux-docker
- GitHub Check: build_and_deploy
🔇 Additional comments (2)
contrib/test-sign-timestamp.sh (1)
1-63: Well-structured macOS code signing validation utility.The script provides comprehensive signature and timestamping verification with solid error handling, portable POSIX regex patterns, and user-friendly color-coded output. Defensive programming throughout (early validation, suppressed optional errors, explicit checks on critical operations).
contrib/make-dmg.sh (1)
1-14: Well-structured DMG automation with solid error handling.The script demonstrates professional Bash practices: early error handling with
set -euo pipefail, cleanup trap, mount/unmount retries, and defensive directory checks. Parameter defaults with environment variable override pattern is idiomatic.
…egacy (#3189) * fix(icons): update native platform app icons and desktop entry - Replace Linux icon: remove SVG, add PNG, update .desktop - Update macOS AppIcon assets (16-1024 px) - Update Windows app icon (.ico) - Adjust Linux runner code for app icon * fix(icons): align iOS/Android app icons with legacy build * fix(splash): mirror startup screen from legacy build (iOS LaunchScreen, Android launch_background) * fix(ios): align AppLogo.imageset Contents.json with legacy images * fix(desktop): mirror macOS AppIcon and Linux icon from legacy desktop * chore(sdk): bump submodule to GLEECBTC/komodo-defi-sdk-flutter@717fe64 * feat(assets): refresh app logo assets across platforms\n\n- Replace PNG logo with WebP variants for app and web\n- Update iOS AppLogo images and Contents.json\n- Remove deprecated app_logo-1x/2x assets\n- Update web/index.html references to new logo\n- Format changed Dart file in ramp fiat provider\n\nRefs: fix/native-platform-app-icons * fix(web): correct logo asset path in index.html for production hosting Update prefetch and img src to /assets/assets/app_icon/logo_icon.webp to match Flutter web bundling and fix 404s on Firebase Hosting rewrites. * fix(web,android): update app icon and favicon refs; ensure correct launcher XML for adaptive icons * chore(sdk): update submodule to latest dev * chore(macos/ios): role native dependencies lock files * fix(icons): correct Linux app icon config and update web index references
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_bloc.dart (2)
209-224: Bug: stale polling snapshot in _waitForOrdersToBeCancelled causes spurious timeouts.
ordersis fetched once and never refreshed inside the loop; the condition never sees updates. Refresh each iteration.- final orders = await _orderRepository.getTradePairs(); - while (orders.any((order) => order.order != null)) { + while (true) { + final orders = await _orderRepository.getTradePairs(); + final allCancelled = orders.every((o) => o.order == null); + if (allCancelled) break; if (DateTime.now().difference(start) > timeout) { if (fatalTimeout) { - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'timeout_cancelling', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('timeout_cancelling', strategyType: 'simple'); throw TimeoutException('Failed to cancel orders in time'); } return; } await Future<void>.delayed(const Duration(milliseconds: 100)); - } + }
117-126: State consistency: revert settings if update flow fails after optimistic add.
addTradePairToStoredSettingshappens before cancel/update; on failure, the pair remains in settings while the bot is stopped — inconsistent UX. Track the optimistic add and roll it back incatch.@@ - // Add the trade pair to stored settings immediately to provide feedback - // and updates to the user. - await _botRepository.addTradePairToStoredSettings(event.tradePair); + // Optimistic settings update; roll back on failure. + var addedToSettings = false; + await _botRepository.addTradePairToStoredSettings(event.tradePair); + addedToSettings = true; @@ - final stoppingState = const MarketMakerBotState.stopping().copyWith( + final stoppingState = const MarketMakerBotState.stopping().copyWith( error: e.toString(), ); emit(stoppingState); - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'update_failed', - strategyType: 'simple', - ), - ); + if (addedToSettings) { + // Roll back optimistic settings change + await _botRepository.removeTradePairsFromStoredSettings( + [event.tradePair].toList(), + ); + } + _safeLogMarketbotError('update_failed', strategyType: 'simple'); await _botRepository.stop(botId: event.botId); emit(stoppingState.copyWith(status: MarketMakerBotStatus.stopped));Also applies to: 137-150
♻️ Duplicate comments (2)
lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_bloc.dart (2)
74-76: Analytics accuracy: avoid hardcodingstrategyType: 'simple'.Derive the strategy type from the event/repo/settings to prevent mislabelled analytics. This was flagged earlier; please fix at source.
Would you confirm the available source for strategy type and payload fields?
#!/bin/bash # Locate event payload and available fields rg -nP 'class\s+MarketbotErrorEventData|strategyType|failureDetail' -C2 # Check market maker bot events for a strategy context rg -nP '(class|sealed)\s+MarketMakerBot.*Event|strategy(Type|)\b' -C3Also applies to: 98-100, 144-146, 191-193, 216-218
71-77: Guard analytics lookups; don’t let logging crash error paths.Direct
GetIt.I<AnalyticsRepo>()will throw when analytics is disabled/not registered; this breaks failure handling. Guard withisRegisteredand swallow logging errors. Also centralize via a small helper to de-duplicate.Apply these diffs to replace each call:
@@ - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'start_failed', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('start_failed', strategyType: 'simple'); @@ - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'stop_failed', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('stop_failed', strategyType: 'simple'); @@ - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'update_failed', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('update_failed', strategyType: 'simple'); @@ - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'cancel_failed', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('cancel_failed', strategyType: 'simple'); @@ - // Log bot error - GetIt.I<AnalyticsRepo>().queueEvent( - MarketbotErrorEventData( - failureDetail: 'timeout_cancelling', - strategyType: 'simple', - ), - ); + _safeLogMarketbotError('timeout_cancelling', strategyType: 'simple');Add this helper (place near the bottom of the file):
void _safeLogMarketbotError(String failureDetail, {required String strategyType}) { try { final getIt = GetIt.I; if (getIt.isRegistered<AnalyticsRepo>()) { getIt<AnalyticsRepo>().queueEvent( MarketbotErrorEventData( failureDetail: failureDetail, strategyType: strategyType, ), ); } } catch (_) { // Ignore analytics failures } }Based on learnings.
Also applies to: 95-101, 142-147, 189-194, 213-219
🧹 Nitpick comments (5)
lib/bloc/fiat/ramp/ramp_fiat_provider.dart (2)
65-74: Add validation for invalid sourceAmount.If
sourceAmountcontains an invalid number string,Decimal.tryParsereturnsnull, which will be sent to the API. Consider validating the input or throwing a descriptive error.Apply this diff to add validation:
) => apiRequest( 'POST', apiEndpoint, queryParams: {'endpoint': '/onramp/quote/all'}, body: { 'fiatCurrency': source, 'cryptoAssetSymbol': getFullCoinCode(target), - 'fiatValue': Decimal.tryParse(sourceAmount)?.toDouble(), + 'fiatValue': (Decimal.tryParse(sourceAmount) ?? + (throw ArgumentError.value(sourceAmount, 'sourceAmount', 'Invalid decimal value'))) + .toDouble(), }, );
186-217: Consider simplifying nested ternary operations for readability.The nested ternary expressions on lines 206-215 for min/max purchase amounts reduce readability. Consider extracting them into local variables or helper methods.
Example refactor:
quoteResult.paymentMethods.forEach(( String key, RampQuoteResultForPaymentMethod value, ) { _log.fine('Processing payment method key $key: $value'); final feeAmount = value.baseRampFee / sourceAmountValue; final minAmount = asset.hasValidMinPurchaseAmount() ? asset.minPurchaseAmount : coins.minPurchaseAmount; final maxAmount = asset.hasValidMaxPurchaseAmount() ? asset.maxPurchaseAmount : coins.maxPurchaseAmount; final method = { 'id': key, 'name': _formatMethodName(key), 'transaction_fees': [ { 'fees': [ {'amount': feeAmount.toDouble()}, ], }, ], 'transaction_limits': [ { 'fiat_code': source, 'min': minAmount.toString(), 'max': maxAmount.toString(), }, ], 'price_info': { 'coin_amount': getFormattedCryptoAmount( value.cryptoAmount.toString(), asset.decimals, ), 'fiat_amount': value.fiatValue.toString(), }, }; paymentMethodsList.add(FiatPaymentMethod.fromJson(method)); });lib/bloc/nft_withdraw/nft_withdraw_bloc.dart (3)
21-23: Consider injecting AnalyticsRepo for consistency and testability.The bloc constructor already injects
NftWithdrawRepo,KomodoDefiSdk, andCoinsRepoas dependencies, butAnalyticsRepois accessed viaGetIt.I<AnalyticsRepo>()throughout the file (lines 103, 129, 140, 179, 199). This inconsistency makes unit testing harder, as you cannot easily mock the analytics repository.Apply this diff to inject
AnalyticsRepo:class NftWithdrawBloc extends Bloc<NftWithdrawEvent, NftWithdrawState> { NftWithdrawBloc({ required NftWithdrawRepo repo, required NftToken nft, required KomodoDefiSdk kdfSdk, required CoinsRepo coinsRepository, + required AnalyticsRepo analyticsRepo, }) : _repo = repo, _coinsRepository = coinsRepository, _kdfSdk = kdfSdk, + _analyticsRepo = analyticsRepo, super(NftWithdrawFillState.initial(nft)) {Then declare the field:
final NftWithdrawRepo _repo; final KomodoDefiSdk _kdfSdk; final CoinsRepo _coinsRepository; + final AnalyticsRepo _analyticsRepo;And replace all
GetIt.I<AnalyticsRepo>()calls with_analyticsRepo.
99-109: Extract wallet type retrieval to reduce duplication.The wallet type retrieval logic
(await _kdfSdk.auth.currentUser)?.type ?? 'unknown'appears twice in this file (lines 99 and 175). Extracting this to a helper method would improve maintainability and ensure consistency.Consider adding a helper method:
Future<String> _getWalletType() async { return (await _kdfSdk.auth.currentUser)?.type ?? 'unknown'; }Then replace both occurrences:
- final walletType = (await _kdfSdk.auth.currentUser)?.type ?? 'unknown'; + final walletType = await _getWalletType();
128-147: Extract failure logging to reduce code duplication.The failure logging code in the
ApiErrorcatch block (lines 129-135) and theTransportErrorcatch block (lines 140-146) is nearly identical. A similar pattern also appears in_onConfirmSend(lines 179-185). Extracting this to a helper method would reduce duplication and make future updates easier.Consider adding a helper method:
void _logNftTransferFailure({ required String collectionName, required String failureDetail, required String hdType, }) { _analyticsRepo.queueEvent( NftTransferFailureEventData( collectionName: collectionName, failureDetail: failureDetail, hdType: hdType, ), ); }Then replace the duplicated blocks:
} on ApiError catch (e) { - // Log failure - GetIt.I<AnalyticsRepo>().queueEvent( - NftTransferFailureEventData( - collectionName: - state.nft.collectionName ?? state.nft.symbol ?? 'unknown', - failureDetail: e.message, - hdType: walletType, - ), - ); + _logNftTransferFailure( + collectionName: state.nft.collectionName ?? state.nft.symbol ?? 'unknown', + failureDetail: e.message, + hdType: walletType, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
android/app/src/main/res/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/launcher_icon_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/launcher_icon_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/launcher_icon_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/launcher_icon_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/launcher_icon_foreground.pngis excluded by!**/*.pngassets/app_icon/logo_icon.pngis excluded by!**/*.pngios/Podfile.lockis excluded by!**/*.lockios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-1024x1024@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-83.5x83.5@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppLogo.imageset/AppLogo.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppLogo.imageset/AppLogo@2x.pngis excluded by!**/*.png
📒 Files selected for processing (9)
android/app/src/main/res/drawable/launch_background.xml(1 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml(1 hunks)android/app/src/main/res/values/colors.xml(1 hunks)ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json(1 hunks)ios/Runner/Assets.xcassets/AppLogo.imageset/Contents.json(1 hunks)ios/Runner/Base.lproj/LaunchScreen.storyboard(1 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(7 hunks)lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_bloc.dart(9 hunks)lib/bloc/nft_withdraw/nft_withdraw_bloc.dart(9 hunks)
✅ Files skipped from review due to trivial changes (4)
- android/app/src/main/res/values/colors.xml
- android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml
- ios/Runner/Assets.xcassets/AppLogo.imageset/Contents.json
- ios/Runner/Base.lproj/LaunchScreen.storyboard
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Format Dart code using
dart format .before committing
Files:
lib/bloc/fiat/ramp/ramp_fiat_provider.dartlib/bloc/nft_withdraw/nft_withdraw_bloc.dartlib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_bloc.dart
🧠 Learnings (1)
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: In the Komodo Wallet project, part files share imports with their parent files. The import for `app_config.dart` in `coins_bloc.dart` is necessary because the part file `coins_state.dart` uses `excludedAssetList` from that package.
Applied to files:
lib/bloc/nft_withdraw/nft_withdraw_bloc.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
- GitHub Check: build-android-docker
- GitHub Check: build-linux-docker
- GitHub Check: Build Desktop (windows)
🔇 Additional comments (5)
ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json (1)
3-116: Asset catalog configuration looks correct.The additions provide comprehensive icon coverage for iOS (iPhone and iPad) across all required sizes and scales:
- iPhone: 20×20, 29×29, 40×40, 60×60 with appropriate scales (2×, 3×)
- iPad: 20×20, 29×29, 40×40, 76×76, 83.5×83.5 with appropriate scales (1×, 2×)
- App Store: 1024×1024 marked as
ios-marketing(correct idiom for the App Store icon)The idiom change for the 1024×1024 entry from
universaltoios-marketingis appropriate and follows Xcode's Asset Catalog conventions. JSON structure and naming are consistent.android/app/src/main/res/drawable/launch_background.xml (1)
6-8: Centered bitmap approach is appropriate for launch screen.Using a
<bitmap>element withandroid:gravity="center"is the standard Android pattern for maintaining aspect ratio and centering imagery on the launch screen. This implementation is sound.lib/bloc/fiat/ramp/ramp_fiat_provider.dart (1)
15-15: WebP logo asset verified—URL is accessible.The asset exists and is available at the new URL (HTTP 200). The logo format change from PNG to WebP is confirmed functional and will provide improved performance through better compression.
lib/bloc/nft_withdraw/nft_withdraw_bloc.dart (1)
196-207: Success event logging looks good.The fee calculation safely handles nullable values and parse failures with appropriate fallbacks. Using
doubleprecision for analytics purposes is reasonable here, as this is for tracking rather than financial calculations.lib/bloc/market_maker_bot/market_maker_bot/market_maker_bot_bloc.dart (1)
124-126: LGTM: clearer typing around status stream.Explicit
Stream<MarketMakerBotStatus>improves readability.
- Update `sdk` submodule pointer to latest `dev` commit - No app code changes Refs: none
Komodo Wallet v0.9.3 Release Notes
This release sharpens sign-in convenience, analytics coverage, operational resilience, and ARRR support. Highlights include one-click remember-me login, a dual analytics pipeline with Matomo support, a hardened feedback workflow, and improved compliance guardrails across platforms.
🚀 New Features
🎨 UI & UX
CoinSubClass.tokenStandardSuffixfor clearer parent chain identification.🔒 Security & Compliance
⚡ Performance & Reliability
🐛 Bug Fixes
🛠️ Build & Developer Experience
📚 Documentation
^3.9.0and Flutter version constraint is>=3.35.0 <3.36.0📦 SDK Changes
SDK Version: Updated from
68429b23dac43eddd53434dda1bd23296523f27dto72c9de3b370f1b4169ebbb3150e8adedf4ed3b23Full Changelog: 0.9.2...0.9.3
Dependency Changes
Details
Note that added
hive_ce/hive_ce_flutterare community forks of the (no longer maintained)hivepackages. They performed significant refactors for performance enhancements, but this may only represent a small portion of the entire package's codebase.Changed Dependencies Summary:
Note
Introduces one-click quick-login, robust IPFS-backed NFT media loading, ZHTLC (ARRR) activation/config UI with status bar, refined trading/geo filters, and cross-platform build/signing improvements, alongside analytics, UX, and dependency upgrades.
imageUrl.Written by Cursor Bugbot for commit 28c9734. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Build/CI
Documentation