chore(release): v0.9.0 - HD support, Unified codebase, Custom tokens, and SDK migration #2526
Conversation
* CI self-hosted config test #1 * Fix incorrect duplicated job ID * CI test #2 * CI Test #3 Results of test #2: integration tests work when using GitHub-hosted Actions latest ubuntu * CI Test #4 * Test unit/integration tests on hosted runner Test unit/integration tests on the hosted runner to determine if the cause of the failing tests is related to differences between self-hosted and hosted environment. * Update CI conditional statements of self-hosted runners Update CI conditional statements that referenced variables only applicable to self-hosted runners. Change to the equivalent for hosted runners. * Remove very_good_analysis linting dependency Flutter is not fetching dependencies for sub-packages in the `packages` directory, so the very_good_analysis files are missing until `flutter pub get` is ran for each package * Add composite actions for asset generation and build validation * Remove chromedriver from unit tests workflow * Add missing `shell` property to the `generate-assets` action Also remove the default value, and unset the environment variable if it does not exist * Delete the `build/` directory between builds to clear AssetManifest.bin Rebuilds do not appear to consistently update AssetManifest.bin, even if there are new assets generated between builds (icons, coin configs) * Add code coverage report as artefact upload Temporary until link with 3rd-party sources like CodeCov is setup * Only fail the coverage step if the report does not exist The flutter test command failing or passing should not affect the test coverage step success/failure * Fix profit/loss unit tests Binance closing price introduces some variability in the calculated profit/loss that has to be accounted for. The margin of error can be reduced by improved price and timestamp matching, but this should suffice for now * Fix code coverage generation action I forgot the spaces in the bash condition :(. It should be "if [ ... ]; then" * Revert to using `strings` before `grep` to be platform independent macOS handles binary files differently to Linux, meaning that `grep -i ...` works with binary files on Linux systems, but not macOS * Remove trailing spaces in zip generation command Trailing spaces after the backslash in a multi-line command causes it to fail with "command not found" * Prefix artefact with runner name & move coverage step to end of ui tests * Update job names and add descriptions * Switch build_transformer over to the new sdk repo * Remove old build_transformer package * Revert changes to web/src/index.html * Add index.html to .gitignore * Move webpack JS files out of web/src * Revert changes to index.html once more :( * Untrack index.html & update build transformer package * Use default GITHUB_TOKEN to make authenticated requests * Add test case for failing binance requests * demo data generator: check if coin is supported before fetching * Add validation step for build_config.json * Fix silently failing asset manifest validation check * Add json validity checks for coin configs * Update commit sha of build transformer package * Use abstract repository class in generate demo data * Install Chrome alongside chromedriver on linux for UI tests * Skip generate demo test data unit tests * Bump build transformer to dev branch * Switch UI and unit test workflows back to self-hosted runners * Restrict coverage report to linux-based runners * Improve npm build cross-platform compatibility '&&' is invalid in Windows PowerShell. ';' works on Windows, macOS, and Linux * Move Flutter and NodeJS installation to composite action * Add github token to UI and unit test steps Seeing 403 responses on self-hosted runners * Fix UI integration tests on self-hosted macos * Export logs for safaridriver in workflows Use the --diagnose flag when starting safaridriver * Fix failing restore wallet integration test * Add verbose flag to integration test runner * Fix issues with pumpAndSettle timeout Add pumpNFrames and use that instead. This is based on the recommendation in the docs: https://api.flutter.dev/flutter/flutter_test/WidgetTester/pumpAndSettle.html * Fix missing balance overview and back button * Fix wallet & wallet manager integration tests * Fix maker & taker integration tests * Fix misc integration tests * Fix analyse warnings and remove support page test * Pin chromedriver to the self-hosted runner version 116.0.5845.96 with path /opt/google/chrome/chrome * Use mock binance repository for unit testing Using the live Binance endpoints resulted in some variance of the closing price & final calculation that caused the unit tests to fail on occasion. * Add note about the possibility of 403 error --------- Co-authored-by: CharlVS <[email protected]> Co-authored-by: Francois <[email protected]>
* add enable test coins to settings * test coins hide if toggled off in settings
…uilds (#163) * Update Flutter to 3.24.3 (stable) Only web build works with this configuration * Bump Flutter version in workflows to `3.24.x` * Remove `desktop_webview_window` dependency * Switch to in-app webview in popup-window * Upgrade `file_picker` to 8.1.2 to fix ios&macos build error win32 v5 removes references to deprecated APIs, which jonataslaw/get_cli#263 * Upgrade `url_launcher` to 6.3.0 to fix iOS build error * Skip web defi fetch step if target is iOS * Bump CI Flutter version to `3.24.x`
* router fix uri query parameters were lost at parse * router add dexroute parameters * router dex state notifyListeners for new fields * router dex process order_type param * router dex clean params without notifying * router dex maker process ticker and amount params * router dex fix typo * dex TakerSetSellCoin event add setOnlyIfNotSet * router dex maker consumes only their own params * router dex allow order_type being maker by default * router dex taker process from/to currencies * dex repository waitOrderbookAvailability function * router dex form wait for orderbooks before process * router dex taker process from_amount * router dex code cleanup * mobile/widget layout taker form dropdown position * maker form parse error check
* change extension to `.gz` on native platforms * add single-file zip archive support credit for zip.dart goes to gpt-4o
* Update Flutter to 3.24.3 (stable) Only web build works with this configuration * Bump Flutter version in workflows to `3.24.x` * Remove `desktop_webview_window` dependency * Switch to in-app webview in popup-window * Upgrade `file_picker` to 8.1.2 to fix ios&macos build error win32 v5 removes references to deprecated APIs, which jonataslaw/get_cli#263 * Upgrade `url_launcher` to 6.3.0 to fix iOS build error * Skip web defi fetch step if target is iOS * Bump CI Flutter version to `3.24.x` * Fix mobile coin details buttons layout - Also fix Bitrefill button in preparation for cross-platform fiat onramp in a similar fashion * Add fullscreen in-app-webview for native platforms * Close the browser if redirected to web app This is a failure condition for the `checkout_status_redirect.html` page * Move payment status events to BLoC The async onConsole/onMessage callback used by the `flutter_inappwebview` package is incompatible with the previous watcher implementation. * Migrate bitrefill provider & watcher to package:web * refactor fullscreen webview to webview dialog * add fiat onramp html page fixes issues with reading `onmessage` and `window.console` from an iframe or another window * apply patch to `web_support.js` to fix web callbacks from pichillilorenzo/flutter_inappwebview#2058 * migrate remaining fiat & bitrefill html references to package:web * WIP: add initial fiat onramp bloc implementation * fix arb merge issue: add arb to currency class * fix type conversion bugs and add more error logging add stacktrace to logs when in kDebug mode * replace onCheckoutComplete callback with BlocListener * improve form state management and add default payment methods list for initial user input * fix cross-platform compilation with conditional imports package:web and js_interop only work on web, so use conditional exports * fix cocoapods build warnings * use url instead of proxy page on native platforms only web requires the proxy page because of CORS restrictions. onConsole, and onMessage works on native platforms * fix hive runtime init exception `Hive.initFlutter` failed on macOS and appears to be a web-specific function that produces an exception on native platforms * add error parsing for banxa order creation * localize fiat error and popup messages * fix status message parsing in wrapper html page * move getCoinAddress to coins bloc * update testing and setup docs add example launch.json and iOS crash logs location add notes about linux setup * fix fiat amount injecting decimals removed unnecessary string `error` field, since the status fields suffice * fix onramp error on linux & banxa parsing bug * fix fiat form overlapping issues on mobile - use autoscrolltext, expand, and align - fix fiat icon errors when scrolling quickly - fix webview platform check * fix flutter analyze warnings * add fiat onramp form integration test * bump build transformer package commit * improve status parsing on native platforms - parsing errors from escaped json strings on Windows - re-enable banxa order status watching * fix coins bloc and Hive init race condition on macos Runtime updates Hive boxes have to be initialised on native platforms before coins bloc executes, which was not happening consistently across all platforms * move confirmation prompt behind conditional import `web` and `js_interop` package imports do not compile on native platforms, so they have to be hidden behind conditional imports to allow for cross-platform support
* kdf show_priv_key api * display private keys * private keys QR code dialog * private keys list title * private key dialog width * private key share instead of copy * Revert "private key share instead of copy" This reverts commit a623444ea75e4d074419bcc9689585ca819beceb. * private key clipboard warning
* Fallback to remote images CDN for missing coin icons * Add coin icon fallback for all references to icons Add coin icon fallback for other widgets referencing the local image icons. Now all coin icons in the app share the same widget. * Prevent flicker for fallback coin icons Prevent flicker for fallback coin icons by caching the status of the existence on the CDN. * Further coin icon fallback bug fixes
… created (#184) * fix trading bot tab order * fix trading bot history onclick event * show estimate trade volume while creating new order
* add system clock repository with fallback urls * add directly connected peers rpc as fallback * improve utc parsing and request exception handling * revert changes to build_config
* add komodo_defi_sdk to dependencies update required dependencies, and fix build errors * WIP: replace mm2 classes with defi sdk * replace kdf rpc status checks with isSignedIn The new SDK abstracts away the KDF functionality behind an authentication class, so the previous status checks are not possible, and the isSignedIn is the closest alternative without removing the logic entirely (possibly breaking change) * bump kdf version * load coin assets from sdk package komodo_defi_framework package already downloads the configs and icons, so load them from the external package instead of redownloading and loading the same assets twice * ci: update paths in validate action * replace dynamic index.html with static version the drawbacks of bundling with webpack outweighed the negligible size reduction in the kdf wasm files, so it was dropped in favour of a static index.html in the sdk * fix failing unit tests caused by the `rational` and `decimal` package updates * update docs remove nodejs, api update script, and add fvm as flutter installation alternative * fix breaking changes to rpc method return types sdk returns different types which have to be accounted for, sometimes on a per-rpc basis * fix validation warnings * WIP: fix integration tests temporarily disable suspended assets test. The `coins_config.json` is no longer editable as it is obtained from an external package, so alternative means of invalidating the electrum URLs for an asset are required * re-enable suspended assets test on chrome * block electrum urls in chrome for suspended assets test * update logs location in ui-tests workflow * fix taker order integration test * change from sdk to komodo_defi_framework * fix debug utils & withdraw test * remove driver start step from ui test workflow browser driver startup is now handled by integration test runner * improve driver management & fix `test_withdraw` test add and refactor integration test utility functions * add pub get flag flutter drive runs `flutter pub get` before each test by default, which slows down the current implementation, which runs each set of tests independently * add profile mode step to ui-tests-on-pr * add verbose logging to integration tests steps some errors, like pumpAndSettle timeouts, do not produce useful stacktraces, so adding print statements is necessary unless we run the integration tests in debug mode (not recommended) * fix cex_prices integration test * fix theme switching on web * fix nft and trezor RPC call type errors type conversions missed during initial migration, but caught by integration tests * migrate web file_loader to js_interop and package:web * change file loader to conditional import structure js_interop causes builds to fail once again * fix seed file upload & add keep-running flag safaridriver logs do not include console logs, so we have to keep the browser window open to read console logs in the event of failure * fix intermittent test failures * bump sdk version & add debug statements * increase flutter drive timeout & remove tests.dart group structure is better suited to the new integration_test format rather than flutter_drive. `await app.main` fails after the second test when in the same group * fix merge error * fix validation warnings & flaky dex taker test * add port option & fix flaky dex ui test steps * tests: add longer wait after taker/maker confirmation button click * fix misc integration test errors when switching theme * fix trading bot order count in tab bar * fix analyze warnings and increase tab refresh rate * restore missing error check throw exception if error response is received from API. Market maker bot was failing to start/stop after this was removed as part of the sdk integration * fix trading bot & dex order list onClick events * fix merge issues & trade bot empty list check list null = empty * fix best orders and connected peers parsing v2 uses orderaddress object for address field instead of string
Add coin icon fallback for other widgets referencing the local image icons. Now all coin icons in the app share the same widget. Fallback to remote images CDN for missing coin icons Further coin icon fallback bug fixes Prevent flicker for fallback coin icons Prevent flicker for fallback coin icons by caching the status of the existence on the CDN.
* fix zero balance issue by fallback to my_balance * update kdf wasm version * Remove undefined KDF build configs * deploy preview on PR to main * max_taker_vol fallback to my_balance * ci branches add release/* * max_taker_vol balance parse rational --------- Co-authored-by: CharlVS <[email protected]> Co-authored-by: Francois <[email protected]>
* add support for map in hd balance response * bump kdf release hash and wasm checksum
* add komodo_defi_sdk to dependencies update required dependencies, and fix build errors * WIP: replace mm2 classes with defi sdk * replace kdf rpc status checks with isSignedIn The new SDK abstracts away the KDF functionality behind an authentication class, so the previous status checks are not possible, and the isSignedIn is the closest alternative without removing the logic entirely (possibly breaking change) * bump kdf version * load coin assets from sdk package komodo_defi_framework package already downloads the configs and icons, so load them from the external package instead of redownloading and loading the same assets twice * ci: update paths in validate action * replace dynamic index.html with static version the drawbacks of bundling with webpack outweighed the negligible size reduction in the kdf wasm files, so it was dropped in favour of a static index.html in the sdk * fix failing unit tests caused by the `rational` and `decimal` package updates * update docs remove nodejs, api update script, and add fvm as flutter installation alternative * fix breaking changes to rpc method return types sdk returns different types which have to be accounted for, sometimes on a per-rpc basis * fix validation warnings * WIP: fix integration tests temporarily disable suspended assets test. The `coins_config.json` is no longer editable as it is obtained from an external package, so alternative means of invalidating the electrum URLs for an asset are required * re-enable suspended assets test on chrome * block electrum urls in chrome for suspended assets test * update logs location in ui-tests workflow * fix taker order integration test * coin addresses bloc init * coin addresses init list in coin details * coin addresses separate status for creation * create addresses cleanup * change from sdk to komodo_defi_framework * coin addresses realistic fake address * coin addresses hide middle part of the address * fix debug utils & withdraw test * remove driver start step from ui test workflow browser driver startup is now handled by integration test runner * coin addresses improve ui * coin addresses special exception type * coin addresses copy button functionality * coin addresses swap address tag * coin addresses bloc hide zero balance * coin addresses styling and hide 0 balance * improve driver management & fix `test_withdraw` test add and refactor integration test utility functions * add pub get flag flutter drive runs `flutter pub get` before each test by default, which slows down the current implementation, which runs each set of tests independently * add profile mode step to ui-tests-on-pr * add verbose logging to integration tests steps some errors, like pumpAndSettle timeouts, do not produce useful stacktraces, so adding print statements is necessary unless we run the integration tests in debug mode (not recommended) * coin addresses extract widget to file * coin addresses QR code * coin addresses extract widgets * coin addresses mobile view init * coin addresses extract widgets * coin addresses improve mobile view layout * coin addresses slightly larger swap address text * coin addresses improve QR dialog * coin addresses localize texts with existing keys * coin addresses localize texts * coin addresses localize texts * coin addresses polish * fix cex_prices integration test * fix theme switching on web * fix nft and trezor RPC call type errors type conversions missed during initial migration, but caught by integration tests * migrate web file_loader to js_interop and package:web * change file loader to conditional import structure js_interop causes builds to fail once again * fix seed file upload & add keep-running flag safaridriver logs do not include console logs, so we have to keep the browser window open to read console logs in the event of failure * fix intermittent test failures * bump sdk version & add debug statements * increase flutter drive timeout & remove tests.dart group structure is better suited to the new integration_test format rather than flutter_drive. `await app.main` fails after the second test when in the same group * fix merge error * fix validation warnings & flaky dex taker test * add port option & fix flaky dex ui test steps * tests: add longer wait after taker/maker confirmation button click * fix misc integration test errors when switching theme * fix trading bot order count in tab bar * fix analyze warnings and increase tab refresh rate * restore missing error check throw exception if error response is received from API. Market maker bot was failing to start/stop after this was removed as part of the sdk integration * fix trading bot & dex order list onClick events * fix merge issues & trade bot empty list check list null = empty * fix best orders and connected peers parsing v2 uses orderaddress object for address field instead of string * replace auth methods with KomodoDefiSdk * fix global variable initialization order and async issues Migrating away from the global variables in blocs.dart will involve a considerable number of code changes, so leaving as-is for now * use sdk in current wallet "bloc" default values used for hasBackup * move global bloc variables to repositoryprovider async sdk/kdf initialization does not work with the current global variable structure, so moved the global repositories to repositoryprovider * revert breaking wallet restore and import changes * skip orders, and swaps RPCs when not logged in also bump kdf sdk version * fix login to existing wallet and remove onlogout deactivate * catch and log uncaught (or async) errors * sdk txs disable tests and mock repository * sdk txs cex market data charts * sdk txs transactions import change * fix onlogout balance clearing throw exceptions with stacktraces * sdk txs use the new sdk Transaction model * sdk txs use the new sdk Transaction model * sdk txs use the new sdk Transaction model * sdk txs use the new sdk Transaction model * sdk txs change types import * tests: use faucet for doc/marty if balance is insufficient * fix maker form auth state listener * sdk txs fee coin * fix validation warnings and remove gha timeout at step * sdk txs fetch from sdk * coin details hide addresses if tx is selected * sdk txs always show transactions if filled * fix transactions missing one row during loading * sdk txs getSdkAsset temp helper function * sdk access added to CoinsRepo * sdk replace max_maker_vol with sdk * sdk getSdkPubkeys helper function * add wallet metadata and disable unsupported features - add `has_backup` and `WalletType` metadata fields to KdfUser - disable delete wallet feature - * sdk addresses port * replace current wallet stream with auth bloc * rename coins "bloc" and move more repos to DI * use new asset class for trezor coin activation trezor activation responds to 'UserActionRequired' events, which are not propagated through asset manager * migrate coins manager from legacy coins bloc to repository and new coins bloc * migrate legacy coins bloc to coins repostory * remove runtime coin updates `KomodoCoins` fetches coins from github cdn at runtime, so runtime updates are no longer necessary * remove coin activation rpc models and migrate legacy coins bloc refs * add activated coins metadata * refactor: migrate fiat bloc from legacy coins bloc * migrate remaining legacy coins bloc references to coins repo * migrate current wallet references to auth bloc * fix swap page validation issues * fix market metrics and transaction history loading * fix trading bot validation errors coins bloc emits not refreshing the widgets at the bottom of the tree * fix bridge page missing protocols * fix coin activation error handling * fix merge issues * fix timestamp in unit test transaction generation * sdk show pubkey addresses for all coins * sdk addresses active swap address tag * sdk addresses display balance * sdk addresses polish balance display * fix view seed and re-login flow * fix coin activation via coins manager activating coins were only displayed on the next balance refresh instead of immediately after activation * add parent coin to conversion * sdk addresses hide zero balances checkbox * sdk addresses reload after create * sdk remove coin addresses repository * fix portfolio growth concurrent modification * fix coin details tab controller error * sdk getSdkAsset use findAssetsByTicker * sdk addresses max 3 empty addresses * sdk addresses split variables * sdk txs add address * sdk txs add address mobile * sdk txs load 200 * fix sporadic activation in coins list * add legacy wallet migration * sdk asset getPubkeys * sdk use assetsFromTicker * pubspec upgrade * coin details history -> lastTransactions * coin details transactions better spacing * coin details polish transactions section * upgrade to flutter 3.27.0 * coin details redesign tx history section * coin details redesign tx history section mobile * fix new validation warnings from flutter upgrade * fix trezor login login does not work in debug mode because of an assert statement in the SDK * bump flutter version in workflows * fix macos and ios builds * decouple balance fetching & coin activation * sdk addresses use CantCreateNewAddressReason * coin details remove createAddressAllowed field * sdk localize CantCreateNewAddressReason texts * trim unused methods from coin model * fix coin balances persisting to another wallet login * fix initial trezor coin activation list * fix trezor coin add asset page * call faucet on all addresses should implement a more robust solution to HD wallet support for faucet * improve login activated coins metadata storing * upgrade sdk --------- Co-authored-by: Francois <[email protected]>
* ci(sdk): Create script/action for SDK auto roll * CI Test 1 * CI Test 2 * CI Test 3
![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
🧹 Nitpick comments (4)
.github/scripts/roll_sdk_packages.sh (3)
38-96: Separate declaration and assignment to preserve return codes.
ShellCheck flags combininglocalwith command substitution (SC2155), which can mask return statuses. For robustness, declare variables first, then assign:- local start_line=$(grep -n "^ $package_name:" "$lock_file" | cut -d: -f1) + local start_line + start_line=$(grep -n "^ $package_name:" "$lock_file" | cut -d: -f1) - local end_line=$(tail -n +$((start_line+1)) "$lock_file" | grep -n "^[^ ]" | head -1 | cut -d: -f1) + local end_line + end_line=$(tail -n +$((start_line+1)) "$lock_file" | grep -n "^[^ ]" | head -1 | cut -d: -f1) - local package_section=$(sed -n "${start_line},${end_line}p" "$lock_file" | sed '$d') + local package_section + package_section=$(sed -n "${start_line},${end_line}p" "$lock_file" | sed '$d') - local version=$(echo "$package_section" | grep " version:" | head -1 | sed 's/.*version: *"\([^"]*\)".*/\1/') + local version + version=$(echo "$package_section" | grep " version:" | head -1 | sed 's/.*version: *"\([^"]*\)".*/\1/') - local source=$(echo "$package_section" | grep " source:" | head -1 | sed 's/.*source: *\([^ ]*\).*/\1/') + local source + source=$(echo "$package_section" | grep " source:" | head -1 | sed 's/.*source: *\([^ ]*\).*/\1/')🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 49-49: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 71-71: Declare and assign separately to avoid masking return values.
(SC2155)
109-113: Handle file paths with spaces.
Looping overPUBSPEC_FILES=$(find ...)with a simplefor PUBSPEC in $PUBSPEC_FILEScan break on paths containing spaces. Consider usingfind … -print0and reading withwhile IFS= read -r -d '' PUBSPEC; do … done.
253-257: Quote output file targets to prevent word splitting.
Redirecting into$GITHUB_OUTPUTwithout quotes may misbehave if the path contains spaces.- echo "updates_found=true" >> $GITHUB_OUTPUT + echo "updates_found=true" >> "$GITHUB_OUTPUT"Apply the same quoting wherever
$GITHUB_OUTPUTis used as a redirection target..github/workflows/roll-sdk-packages.yml (1)
60-62: Quote redirection targets to avoid word splitting.
In theDetermine configurationstep, the script writes to$GITHUB_ENVwithout quotes. Wrap the variable in quotes to be safe:- echo "TARGET_BRANCH=$TARGET_BRANCH" >> $GITHUB_ENV + echo "TARGET_BRANCH=$TARGET_BRANCH" >> "$GITHUB_ENV" - echo "UPGRADE_ALL=$UPGRADE_ALL" >> $GITHUB_ENV + echo "UPGRADE_ALL=$UPGRADE_ALL" >> "$GITHUB_ENV"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/assets/sdk-rolls.pngis excluded by!**/*.png
📒 Files selected for processing (3)
.github/actions/flutter-deps/action.yml(1 hunks).github/scripts/roll_sdk_packages.sh(1 hunks).github/workflows/roll-sdk-packages.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/flutter-deps/action.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/scripts/roll_sdk_packages.sh
[warning] 49-49: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 66-66: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 71-71: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 actionlint (1.7.4)
.github/workflows/roll-sdk-packages.yml
36-36: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
51-51: shellcheck reported issue in this script: SC2086:info:9:40: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:17:36: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:20:52: Double quote to prevent globbing and word splitting
(shellcheck)
79-79: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
79-79: shellcheck reported issue in this script: SC2086:info:5:31: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (1)
.github/workflows/roll-sdk-packages.yml (1)
36-39: Skip outdated actionlint warning.
actionlint suggests updatingactions/checkout@v3, but v3 is the latest stable release and fully supported. No change required.🧰 Tools
🪛 actionlint (1.7.4)
36-36: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| #!/usr/bin/env bash | ||
|
|
||
| # Script to roll SDK packages in all Flutter/Dart projects | ||
| # Designed to handle git-based dependencies and work with the GitHub CI workflow | ||
|
|
||
| set -e | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add stricter error handling.
Currently the script uses set -e, which stops on the first error, but pipelines can still mask failures. It's recommended to also enable pipefail (and optionally nounset) to catch errors in piped commands and unset variables.
- set -e
+ set -euo pipefail📝 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.
| #!/usr/bin/env bash | |
| # Script to roll SDK packages in all Flutter/Dart projects | |
| # Designed to handle git-based dependencies and work with the GitHub CI workflow | |
| set -e | |
| #!/usr/bin/env bash | |
| # Script to roll SDK packages in all Flutter/Dart projects | |
| # Designed to handle git-based dependencies and work with the GitHub CI workflow | |
| set -euo pipefail |
| # Initialize changes file | ||
| echo "# SDK Package Rolls" > "$CHANGES_FILE" | ||
| echo "" >> "$CHANGES_FILE" | ||
| echo "**Date:** $CURRENT_DATE" >> "$CHANGES_FILE" | ||
| echo "**Target Branch:** $TARGET_BRANCH" >> "$CHANGES_FILE" | ||
| echo "**Upgrade Mode:** $([ "$UPGRADE_ALL_PACKAGES" = "true" ] && echo "All Packages" || echo "SDK Packages Only")" >> "$CHANGES_FILE" | ||
| echo "" >> "$CHANGES_FILE" | ||
| echo "The following SDK packages were rolled to newer versions:" >> "$CHANGES_FILE" |
There was a problem hiding this comment.
Prevent generating changelog when no rolls occurred.
The script unconditionally initializes SDK_CHANGELOG.md at the start, so the workflow will always detect a changelog file—even if no SDK packages were rolled. Move the header initialization block inside the if [ "$ROLLS_MADE" = true ] check (after detecting changes), or remove the file when ROLLS_MADE is false.
Suggested adjustment:
- # Initialize changes file
- echo "# SDK Package Rolls" > "$CHANGES_FILE"
- ...
- echo "" >> "$CHANGES_FILE"
+ # Defer initializing the changelog until a roll is detected.Committable suggestion skipped: line range outside the PR's diff.
…Ls of existing providers (#2611) * feat(system-health): add NTP and HTTP header time providers * test(system-health): add unit tests for system clock providers * chore(deps): add ntp package dependency and reorder imports * refactor(system-health): rename events & move logging init to bootstrap * fix(system-health): add binance time provider for web CORS rules block accessing the `date` header on web from other domains unless it's been explicitly allowed. * refactor: change method return type and implement ai review suggestions * fix(system-health): show banner if clock invalid even if peers >= 2 previous check was a regression that wouldn't show the banner for incorrect clock if there were still peers connected * refactor: simplify datetime parsing and revert exception regression * refactor: remove shared http client to avoid issues with multiple closes
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/bloc/app_bloc_root.dart (1)
391-398:⚠️ Potential issueError handling in _precacheCoinIcons still needs to be addressed.
The comment on lines 394-396 acknowledges a known issue where
completeErrorthrows an uncaught exception, but the fix is not implemented. This causes UI tests to fail when switching between light and dark theme.Implement proper error handling when aborting an ongoing precache operation:
if (_currentPrecacheOperation != null && !_currentPrecacheOperation!.isCompleted) { - // completeError throws an uncaught exception, which causes the UI - // tests to fail when switching between light and dark theme - log('New request to precache icons started.'); - _currentPrecacheOperation!.complete(); + log('Cancelling previous precache operation'); + try { + // Signal cancellation with a specific error + _currentPrecacheOperation!.completeError( + StateError('Precache cancelled by a subsequent request') + ); + } catch (e) { + log('Error while cancelling previous precache operation: $e'); + } }
🧹 Nitpick comments (3)
lib/bloc/app_bloc_root.dart (3)
120-125: Inconsistent transaction history repository implementation.The code still uses
MockTransactionHistoryRepoconditionally while using the new SDK-basedSdkTransactionHistoryRepositoryin production mode. Consider creating a mock that extends the new repository for consistency.For better consistency with the SDK migration:
final transactionsRepo = performanceMode != null - ? MockTransactionHistoryRepo( - performanceMode: performanceMode, - demoDataGenerator: DemoDataCache.withDefaults(), - ) + ? MockSdkTransactionHistoryRepository( + performanceMode: performanceMode, + demoDataGenerator: DemoDataCache.withDefaults(), + sdk: komodoDefiSdk, + ) : SdkTransactionHistoryRepository(sdk: komodoDefiSdk);
150-153: Document startup behavior changes.The code now sets
MainMenuValue.dexas the initial menu and callstradingEntitiesBloc.runUpdate()at startup, but lacks documentation explaining this change in behavior.Add comments explaining the purpose of these startup operations:
-// startup bloc run steps +// Initialize trading entities and set the default navigation tab to DEX +// This ensures market data is loaded immediately when the app starts tradingEntitiesBloc.runUpdate(); routingState.selectedMenu = MainMenuValue.dex;
333-337: Consider refactoring debug initialization to a separate method.The debug initialization code is embedded directly in the
initStatemethod, which reduces readability. Consider extracting it to a dedicated method.@override void initState() { final coinsBloc = context.read<CoinsBloc>(); _routeInformationParser = RootRouteInformationParser(coinsBloc); _airDexBackButtonDispatcher = AirDexBackButtonDispatcher(_routerDelegate); routingState.selectedMenu = MainMenuValue.defaultMenu(); unawaited(_hideAppLoader()); + _initDebugDataIfNeeded(); + super.initState(); +} + +void _initDebugDataIfNeeded() { if (kDebugMode) { final walletsRepo = RepositoryProvider.of<WalletsRepository>(context); final authBloc = context.read<AuthBloc>(); initDebugData(authBloc, walletsRepo).ignore(); } - - super.initState(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/bloc/app_bloc_root.dart(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/app_bloc_root.dart (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: unit_tests
- GitHub Check: validate_code_guidelines
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: build_and_preview
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: build_and_deploy
🔇 Additional comments (1)
lib/bloc/app_bloc_root.dart (1)
72-78: Constructor refactored to use KomodoDefiSdk.The constructor now requires a
KomodoDefiSdkinstance instead ofruntimeUpdateConfig, aligning with the SDK migration.
| final RuntimeUpdateConfig runtimeUpdateConfig; | ||
| final KomodoDefiSdk komodoDefiSdk; | ||
|
|
||
| // TODO: Refactor to clean up the bloat in this main file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment regarding bloat in main file.
The TODO comment on line 80 indicates awareness of code bloat in this file, but no action has been taken to address it.
Consider breaking this file into smaller components:
- Separate repository providers and bloc providers into dedicated factory methods
- Extract initialization logic to separate helper classes
- Group related bloc providers by feature domain
This will improve maintainability and testability of the application initialization process.
lib/bloc/app_bloc_root.dart
Outdated
| final coins = coinsRepo.getKnownCoinsMap().keys; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update coins retrieval method consistently across codebase.
The method to retrieve known coins has changed from getKnownCoins() to getKnownCoinsMap().keys. Ensure this change is applied consistently throughout the codebase.
🏁 Script executed:
#!/bin/bash
# Check for other uses of getKnownCoins() that may need to be updated
rg "getKnownCoins\(\)" --type dartLength of output: 609
Update all getKnownCoins() calls to getKnownCoinsMap().keys
We’ve introduced getKnownCoinsMap().keys in app_bloc_root.dart; other parts of the code are still using the old getKnownCoins() method. Please replace these call sites to ensure consistency:
• lib/bloc/nft_transactions/nft_txn_repository.dart
• lib/bloc/nfts/nft_main_repo.dart
• lib/bloc/bridge_form/bridge_repository.dart
• lib/views/dex/simple/form/taker/taker_form_content.dart
For example, change:
final coins = _coinsRepo.getKnownCoins();to:
final coins = _coinsRepo.getKnownCoinsMap().keys;* fix(dex): crash when clicking on the buy coin dropdown KDF returns ARRR in the best orders response, which is currently excluded from the state which caused the exception to be thrown in grouped list view * fix(dex): revert removing ARRR from excluded assets & filter best orders * fix(dex): filter excluded assets out from add assets and fiat onramp
…n to komodo button (#2608) * refactor(fiat-onramp): improve json parsing, logging, and doc comments included coderabbitai suggestions in main release PR * refactor(webiew-dialog): improve view mode checks, and JS listener * refactor(fiat-form): move url and webview mode checks to bloc * fix(fiat-onramp): split initial and pending payment status * feat(webview): add CSP blockers with trusted domain filters * fix(fiat-onramp): open banxa in new tab rather than dialog Banxa "Return to Komodo" button attempts to set `window.parent.location`, which is not allowed in sandboxed iframes, and is not desired behaviour. In doing so, it would require the user to log in again after the redirect * fix(fiat-onramp): banxa status parsing and normalisation Use FiatOrderStatus enum, add the missing status, and normalise the remaining strings given that the input string is lowercased * refactor: implement github and coderabbit ai suggestions * fix(fiat-onramp): close checkout status redirect Assumption: redirect occurs in new tab or popup webview dialog, so the desired behaviour is to return to the KW tab rather than navigating back to a home page that requires the user to sign in again * fix(fiat-onramp): coin ticker abbr/ticker regression - increases the number of coins available for onramp, mostly from the banxa supported coins list - revert regression in the `getAbbr` function, although a better approach would be a good idea when migrating to the SDK * feat(fiat-onramp): show raw provider error message on buy failure * refactor(fiat-widget): add charset metadata and revert nav in handler * fix(fiat-onramp): add coin min purchase amount field as validation check * fix(fiat-onramp): temporarily disable coins with address format errors * fix(fix-onramp): ramp asset parsing and divide by zero error
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
lib/bloc/fiat/fiat_repository.dart (2)
32-40:⚠️ Potential issuePrevent
null-dereference when provider is absent
yield* Stream.error(...)does not terminate the generator, so execution continues andprovider!is dereferenced, throwing at runtime.
Add an early return (or wrap the rest of the block in anelse) to guarantee safety.- if (provider == null) yield* Stream.error('Provider not found'); - - yield* provider!.watchOrderStatus(orderId); + if (provider == null) { + yield* Stream.error('Provider not found'); + return; // stop further execution + } + + yield* provider.watchOrderStatus(orderId);
219-233: 🛠️ Refactor suggestionShield
getPaymentMethodsListagainst flaky providersSame fast-fail concern as earlier: one broken provider stops all others. Consider wrapping each provider call:
final paymentMethods = await provider .getPaymentMethodsList(source, target, sourceAmount) .catchError((e, s) { _log.warning('Provider ${provider.getProviderId()} failed', e, s); return <FiatPaymentMethod>[]; });This improves resilience and end-user experience.
♻️ Duplicate comments (3)
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
98-102: Deprecated getFirstPubkey calls have been replacedThe code now properly uses
_sdk.pubkeys.getPubkeys()instead of the deprecatedgetFirstPubkeymethod, addressing a previously identified issue in the codebase.lib/bloc/fiat/fiat_order_status.dart (1)
42-47: 🛠️ Refactor suggestionAdd common synonyms returned by Banxa
Banxa occasionally replies with
"completed"(note the “d”). Mapping this
prevents incorrectly downgrading a successful order to in progress.- case 'complete': + case 'complete': + case 'completed': return FiatOrderStatus.success;lib/bloc/fiat/ramp/ramp_fiat_provider.dart (1)
55-58: Decimal → double cast still risks precision loss
This point has been raised in a previous review and remains unresolved.
🧹 Nitpick comments (8)
lib/bloc/coins_manager/coins_manager_bloc.dart (1)
132-143: Updated coin selection logic to use synchronous activation methodsSimilar to the batch operation, individual coin selection now uses synchronous activation methods. This code would be more maintainable if the activation/deactivation logic was extracted to a separate method to avoid duplication.
Consider refactoring the repeated activation/deactivation code into a separate method:
- if (state.action == CoinsManagerAction.add) { - _coinsRepo.deactivateCoinsSync([event.coin]); - } else { - _coinsRepo.activateCoinsSync([event.coin]); - } + _toggleCoinActivation([event.coin], isActive: !state.action == CoinsManagerAction.add);Where you would define:
void _toggleCoinActivation(List<Coin> coins, {required bool isActive}) { if (isActive) { _coinsRepo.activateCoinsSync(coins); } else { _coinsRepo.deactivateCoinsSync(coins); } }lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (3)
66-113: Robust pubkey fetching with proper error handlingThe coin selection implementation includes proper error handling for pubkey retrieval with informative logging. However, there's a potential issue when using
firstOrNullon the pubkeys map.When getting the first pubkey with
assetPubkeys.keys.firstOrNull, consider checking if the map is empty first, or adding more robust fallback handling. This pattern appears in multiple places throughout the code.- final address = assetPubkeys.keys.firstOrNull; + final address = assetPubkeys.isNotEmpty ? assetPubkeys.keys.first : null;
242-284: Robust pubkey updating with retry mechanismThe
_updateAssetPubkeysmethod implements a good retry mechanism with exponential backoff. However, there's a hardcoded retry delay value.Consider extracting the retry delay calculation to a constant or configurable parameter:
- await Future<void>.delayed(Duration(milliseconds: 500 * attempts)); + final baseRetryDelay = const Duration(milliseconds: 500); + await Future<void>.delayed(baseRetryDelay * attempts);
545-547: Hardcoded default amount valueThe method contains a hardcoded value of '10000' for the source amount when there's no valid fiat amount.
Consider extracting the default amount value to a named constant at the class or file level:
+ static const String defaultSourceAmount = '10000'; String _getSourceAmount() { - return _hasValidFiatAmount() ? state.fiatAmount.value : '10000'; + return _hasValidFiatAmount() ? state.fiatAmount.value : defaultSourceAmount; }lib/bloc/fiat/fiat_order_status.dart (3)
37-41: Out-of-date docstring – method no longer throws
fromStringnow returns a fallback status instead of throwing, but the
documentation still warns about an exception. Updating the comment prevents
confusion for future maintainers.- /// Parses the fiat order status form string - /// Throws [Exception] if the string is not a valid status + /// Parses a provider-specific string into a [FiatOrderStatus]. + /// + /// If the value is not recognised the method logs a warning and + /// returns [FiatOrderStatus.inProgress] instead of throwing.
28-33: TreatwindowCloseRequestedas a terminal stateA user-initiated close ends the flow just like a success/failure. Not marking
it as terminal may leave UI components (spinners, buttons) in an indeterminate
state.bool get isTerminal => - this == FiatOrderStatus.success || this == FiatOrderStatus.failed; + this == FiatOrderStatus.success || + this == FiatOrderStatus.failed || + this == FiatOrderStatus.windowCloseRequested;
42-44: Normalise withtrim()before lowering caseStray spaces or line-feeds have been observed in webhook payloads. A quick
trim()eliminates this class of bugs.- final normalized = status.toLowerCase(); + final normalized = status.trim().toLowerCase();lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (1)
115-122:canSubmitbecomes true afterwindowCloseRequestedWhen the provider window is closed (
fiatOrderStatus.windowCloseRequested) the
order is effectively aborted, yetcanSubmitre-enables the submit button
becauseisSubmittingisfalse.- !fiatOrderStatus.isSubmitting && + !fiatOrderStatus.isSubmitting && + !fiatOrderStatus.isTerminal &&Prevents accidental double-submissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (18)
assets/web_pages/checkout_status_redirect.html(1 hunks)assets/web_pages/fiat_widget.html(1 hunks)lib/app_config/app_config.dart(7 hunks)lib/bloc/bridge_form/bridge_bloc.dart(7 hunks)lib/bloc/coins_manager/coins_manager_bloc.dart(7 hunks)lib/bloc/fiat/banxa_fiat_provider.dart(9 hunks)lib/bloc/fiat/base_fiat_provider.dart(5 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(1 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(1 hunks)lib/bloc/fiat/fiat_order_status.dart(1 hunks)lib/bloc/fiat/fiat_repository.dart(8 hunks)lib/bloc/fiat/models/fiat_buy_order_error.dart(1 hunks)lib/bloc/fiat/models/fiat_buy_order_info.dart(1 hunks)lib/bloc/fiat/models/fiat_price_info.dart(1 hunks)lib/bloc/fiat/models/i_currency.dart(1 hunks)lib/bloc/fiat/payment_status_type.dart(1 hunks)lib/bloc/fiat/ramp/models/ramp_asset_info.dart(1 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- lib/bloc/fiat/payment_status_type.dart
- lib/bloc/bridge_form/bridge_bloc.dart
- lib/bloc/fiat/models/fiat_price_info.dart
- lib/bloc/fiat/ramp/models/ramp_asset_info.dart
- assets/web_pages/fiat_widget.html
- lib/app_config/app_config.dart
- lib/bloc/fiat/models/fiat_buy_order_info.dart
- lib/bloc/fiat/models/i_currency.dart
- lib/bloc/fiat/base_fiat_provider.dart
- lib/bloc/fiat/banxa_fiat_provider.dart
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: unit_tests
🔇 Additional comments (35)
lib/bloc/coins_manager/coins_manager_bloc.dart (13)
3-7: Updated imports to use the KDF Flutter SDKThe imports have been updated to use the new Komodo DeFi SDK (
komodo_defi_sdk.dart) and repository pattern withCoinsRepoinstead of direct bloc dependencies. This aligns with the PR objective of migrating to the new KDF Flutter SDK.
11-12: Switched to part files for events and statesThe bloc structure has been updated to use the part file pattern, which provides better organization and separation of concerns.
19-24: Modified constructor to use CoinsRepo and KomodoDefiSdkThe constructor now properly requires
CoinsRepoandKomodoDefiSdkinstances, supporting the SDK migration. The initial state now properly initializes with an empty coin list, with actual coins fetched asynchronously.
35-36: Updated class fields to use SDK dependenciesFields have been updated to store references to the injected SDK and repository objects.
55-64: Refactored _onCoinsUpdate to use asynchronous SDK methodsThe method has been correctly updated to use asynchronous SDK methods, fetching coin lists through the repository and SDK.
77-78: Added explicit action parameter to state updateThe state update now includes the current action, which ensures proper state tracking across asynchronous operations.
80-92: Added async _onCoinsListReset methodThis new method provides a clean way to reset and reload the coin list asynchronously. The implementation correctly handles state transitions during the async operation.
104-104: Updated event dispatching with current actionEvent dispatching has been updated to include the current action state, maintaining consistency throughout the component lifecycle.
162-162: Updated event dispatching to include action parameterEvent dispatching has been updated consistently throughout the class to include the current action state.
Also applies to: 170-170
207-214: Refactored _getOriginalCoinList to use SDK methodsThe function now properly integrates with the SDK to fetch the current wallet type, handling the null case correctly.
217-219: Updated coin fetching logic with SDK methodsThe code now appropriately delegates to coin fetching methods based on the action type, using repository and SDK methods.
225-234: Implemented _getDeactivatedCoins with SDK asset integrationThis method correctly implements the logic to filter disabled coins based on the SDK's enabled coins list and excluded assets list.
237-243: Added support for HD walletsThe code now properly supports both legacy
iguanaand newhdwallettypes with the same coin handling logic, which aligns with the PR objective of adding HD wallet support.assets/web_pages/checkout_status_redirect.html (4)
3-7: Improved HTML formattingThe HTML document structure has been cleaned up with proper spacing and indentation.
12-18: Refactored params extraction functionThe function that extracts URL parameters has been refactored with cleaner variable declarations and consistent formatting.
26-37: Improved conditional logic and commentaryThe conditional logic for checking the opener window has been improved with better indentation and explanatory comments about the Banxa checkout temporary workaround.
39-41: Updated window behavior to always closeInstead of redirecting to the home page when the opener is closed or unavailable, the script now always closes the current window. This is explained with a clear comment about how Ramp and Banxa checkouts work.
lib/bloc/fiat/models/fiat_buy_order_error.dart (6)
1-9: Well-structured error model class with required propertiesThe
FiatBuyOrderErrorclass is well-designed with required properties for error code, status, and title, extending Equatable for proper value equality comparisons.
11-17: Robust JSON deserialization with null handlingThe
fromJsonfactory method properly handles potential null values and type casting with theassertIntutility function.
19-24: Convenient named constructors for common error statesThe class provides useful named constructors for common error states (
noneandparsing) which simplify error creation throughout the codebase.
26-30: Utility getter and clear property definitionsThe class includes a helpful
isNoneutility getter and clearly defined properties.
32-38: Consistent JSON serializationThe
toJsonmethod provides consistent serialization matching the deserialization pattern.
40-42: Proper Equatable implementationThe
propsgetter correctly includes all relevant properties for equality comparison.lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (8)
1-28: Well-organized imports and class structure using part filesThe imports are properly organized with a clear separation between standard Flutter, third-party, and app-specific imports. The bloc uses part files for clean separation of event and state definitions.
29-65: Comprehensive event handling with proper event transformer usageThe bloc implements a wide range of event handlers with appropriate transformers (like
restartable()) to handle concurrent events properly. The class follows best practices by requiring dependencies through constructor injection.
141-206: Thorough form submission logic with proper validationThe form submission implementation includes comprehensive validation, error handling, and state management. The URL construction and web view mode determination are well-implemented.
208-225: Platform-aware WebView mode determination logicThe method for determining the appropriate WebView mode based on platform and provider is well-designed and commented.
442-453: Form validation with TODO commentThe form validation logic includes another TODO comment about UI display and localization of error messages.
This TODO indicates the error messages should be localized and displayed in the UI, rather than just used internally. Consider creating a tracking issue for this enhancement.
455-489: Careful amount input validation with minimum purchase handlingThe method for validating and bounding amount input is thorough and handles the complex requirements of minimum purchase amounts across different payment providers.
592-601: Good default state managementThe method that sets up default payment methods handles the state updates properly, ensuring a consistent initial state.
430-440:✅ Verification successful
TODO comment for error handling improvement
There's a TODO comment about Banxa error handling that should be addressed in future updates.
This TODO comment suggests there's potential for better error handling specific to Banxa errors. Consider creating a tracking issue to address this in a future update.
🌐 Web query:
What is the recommended way to handle Banxa error codes in cryptocurrency applications?💡 Result:
The recommended way to handle Banxa error codes in cryptocurrency applications generally involves the following steps:
Identify Error Codes: Familiarize yourself with the specific error codes that Banxa may return. These codes typically correspond to various issues like validation errors, payment failures, or system errors.
Consult Banxa Documentation: Banxa provides detailed documentation on their API, including error codes and their meanings. Always refer to their official documentation to understand what each error code means and the recommended resolutions.
Implement Robust Error Handling Logic: In your application, implement logic to catch and handle these error codes gracefully. For example:
- Retry mechanisms for transient errors.
- User-friendly error messages for validation errors.
- Logging errors for monitoring and debugging.
User Notifications: Inform users about the error in a clear and helpful manner. If the error is recoverable (e.g., invalid input), guide the user on how to fix it. If it is a system error, advise them to try again later or contact support.
Fallback Procedures: Depending on the severity of the error, consider fallback procedures such as alternative payment methods, or queuing the transaction for later processing.
Security Considerations: Ensure sensitive information is not exposed in error messages.
If you want, I can help you find the latest Banxa error codes and their handling guidelines from their official resources or developer documentation. Would you like me to do that?
Add a tracking issue for Banxa‐specific error handling
The existing TODO is a valid reminder to improve error handling for Banxa’s text‐based error codes. Rather than blocking this PR, let’s:
• Create a GitHub issue (e.g. “Support Banxa error codes in fiat_onramp_form”)
• In that issue, list the known Banxa error codes (validation, payment, system errors) from their API docs
• Define how we’ll handle each (retry logic, user‐friendly messages, fallbacks)This keeps the code moving and ensures we track a concrete plan for future updates.
lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (2)
99-102:firstOrNullrequirespackage:collection
transactionLimituses thefirstOrNullextension.
Ensure that some file in the same library imports:import 'package:collection/collection.dart';to avoid a compilation error.
123-165: Value-getter pattern for nullable resets – great!Using
ValueGetter<T?>to let callers intentionally clear optional fields is a
clean solution that avoids extra boolean flags. Nicely done.lib/bloc/fiat/ramp/ramp_fiat_provider.dart (1)
332-337: UnimplementedwatchOrderStatusThe method always throws
UnsupportedError, yet higher layers may expect a
stream. Surface this limitation clearly (e.g., via repository capabilities) so
UI code can disable polling for Ramp orders instead of crashing.lib/bloc/fiat/fiat_repository.dart (1)
117-134:decimalPointsargument is ignored
_calculateCoinAmountacceptsdecimalPointsbut never applies it, so callers cannot control rounding / precision.- return coinAmount.toDecimal( - scaleOnInfinitePrecision: scaleOnInfinitePrecision, - ); + // round/scale to the requested number of decimal places + return coinAmount.toDecimal( + scaleOnInfinitePrecision: scaleOnInfinitePrecision, + precision: decimalPoints, // <-- ensure argument is honoured + );Please double-check the exact
DecimalAPI; ifprecisionis not available, fall back tocoinAmount.round(scale: decimalPoints)ortoStringAsFixed(decimalPoints)then parse back.
| final Future<void> switchingFuture = state.action == CoinsManagerAction.add | ||
| ? _coinsRepo.activateCoins(selectedCoins) | ||
| : _coinsRepo.deactivateCoins(selectedCoins); | ||
| ? _coinsRepo.activateCoinsSync(selectedCoins) | ||
| : _coinsRepo.deactivateCoinsSync(selectedCoins); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Updated to use synchronous coin activation methods
The code now uses synchronous activation methods from CoinsRepo instead of asynchronous calls, which could potentially lead to race conditions if not handled properly.
Please ensure that the synchronous methods in CoinsRepo handle any internal asynchronous operations appropriately and don't block the UI thread.
🏁 Script executed:
#!/bin/bash
# Search for the implementation of activateCoinsSync and deactivateCoinsSync methods
rg -A 10 'activateCoinsSync|deactivateCoinsSync' --type dartLength of output: 10361
Clarify async behavior and loading‐state handling in CoinsManagerBloc
The methods activateCoinsSync/deactivateCoinsSync in CoinsRepo are asynchronous (Future<void>) and internally await each coin operation, so they won’t block the UI thread. However, in CoinsManagerBloc the UI loading flag (isSwitching) is being cleared before these operations finish, which can prematurely hide loading indicators and lead to race conditions.
Please adjust the emission in lib/bloc/coins_manager/coins_manager_bloc.dart:
• File: lib/bloc/coins_manager/coins_manager_bloc.dart
Lines: around 114–116
Suggested diff:
- emit(state.copyWith(selectedCoins: [], isSwitching: false));
- await switchingFuture;
+ await switchingFuture;
+ emit(state.copyWith(selectedCoins: [], isSwitching: false));📝 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.
| final Future<void> switchingFuture = state.action == CoinsManagerAction.add | |
| ? _coinsRepo.activateCoins(selectedCoins) | |
| : _coinsRepo.deactivateCoins(selectedCoins); | |
| ? _coinsRepo.activateCoinsSync(selectedCoins) | |
| : _coinsRepo.deactivateCoinsSync(selectedCoins); | |
| final Future<void> switchingFuture = state.action == CoinsManagerAction.add | |
| ? _coinsRepo.activateCoinsSync(selectedCoins) | |
| : _coinsRepo.deactivateCoinsSync(selectedCoins); | |
| await switchingFuture; | |
| emit(state.copyWith(selectedCoins: [], isSwitching: false)); |
🤖 Prompt for AI Agents
In lib/bloc/coins_manager/coins_manager_bloc.dart around lines 114 to 116, the
code calls asynchronous methods activateCoinsSync and deactivateCoinsSync but
clears the isSwitching loading flag before these operations complete, causing
premature hiding of loading indicators. To fix this, await the switchingFuture
before emitting the state that clears isSwitching, ensuring the UI loading state
remains active until the async operations finish.
| final queryString = payload.entries.map((entry) { | ||
| return '${Uri.encodeComponent(entry.key)}=${Uri.encodeComponent(entry.value.toString())}'; | ||
| return '${Uri.encodeComponent(entry.key)}=${Uri.encodeComponent(entry.value)}'; | ||
| }).join('&'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uri.encodeComponent expects String
entry.value is dynamic; passing a non-string (e.g., Decimal) will throw
a TypeError in Dart 3. Convert to String first.
- return '${Uri.encodeComponent(entry.key)}=${Uri.encodeComponent(entry.value)}';
+ return '${Uri.encodeComponent(entry.key)}=${Uri.encodeComponent(entry.value.toString())}';🤖 Prompt for AI Agents
In lib/bloc/fiat/ramp/ramp_fiat_provider.dart around lines 323 to 325, the code
passes entry.value directly to Uri.encodeComponent, but entry.value is dynamic
and may not be a String, causing a TypeError in Dart 3. Fix this by converting
entry.value to a String before encoding it, ensuring Uri.encodeComponent always
receives a String argument.
| Decimal _getPaymentMethodFee(FiatPaymentMethod paymentMethod) { | ||
| return paymentMethod.transactionFees.first.fees.first.amount; | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against empty fee collections
transactionFees.first.fees.first will throw RangeError if either list is
empty. Fail gracefully instead of crashing:
- return paymentMethod.transactionFees.first.fees.first.amount;
+ if (paymentMethod.transactionFees.isEmpty ||
+ paymentMethod.transactionFees.first.fees.isEmpty) {
+ _log.warning('No fee data for payment method ${paymentMethod.id}');
+ return Decimal.zero;
+ }
+ return paymentMethod.transactionFees.first.fees.first.amount;📝 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.
| Decimal _getPaymentMethodFee(FiatPaymentMethod paymentMethod) { | |
| return paymentMethod.transactionFees.first.fees.first.amount; | |
| } | |
| Decimal _getPaymentMethodFee(FiatPaymentMethod paymentMethod) { | |
| if (paymentMethod.transactionFees.isEmpty || | |
| paymentMethod.transactionFees.first.fees.isEmpty) { | |
| _log.warning('No fee data for payment method ${paymentMethod.id}'); | |
| return Decimal.zero; | |
| } | |
| return paymentMethod.transactionFees.first.fees.first.amount; | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/fiat/ramp/ramp_fiat_provider.dart around lines 230 to 233, the code
accesses the first elements of transactionFees and fees lists without checking
if they are empty, which can cause a RangeError. Modify the method to first
check if transactionFees and fees are not empty before accessing their first
elements, and return a default value or handle the empty case gracefully to
avoid crashing.
| knownCoinAbbreviations = knownCoins.map((coin) => coin.abbr).toSet(); | ||
| } | ||
| final currencyMap = <String, ICurrency>{}; | ||
| final knownCoins = _coinsRepo.getKnownCoinsMap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate provider-level failures individually
Future.wait(futures) will fail fast if a single provider throws, masking prices from the remaining providers. Wrap each call in try/catch (or use Future.wait(..., eagerError: false)) and log failures instead of aborting the whole request.
🤖 Prompt for AI Agents
In lib/bloc/fiat/fiat_repository.dart around lines 46 to 49, the current use of
Future.wait on the futures list will cause the entire operation to fail if any
single provider throws an error, hiding results from other providers. To fix
this, wrap each call to getList in a try/catch block or use Future.wait with
eagerError set to false to prevent fast failure. Log any individual provider
errors instead of aborting the whole request, ensuring partial results from
successful providers are still processed.
| bool isCoinSegwitKnown(String coinTicker) => | ||
| currency.isCrypto && knownCoins.containsKey('$coinTicker-segwit'); | ||
| if (isCoinSegwitKnown(currency.getAbbr())) { | ||
| final segwitCoin = knownCoins['${currency.getAbbr()}-segwit']; | ||
| final segwitCurrency = (currency as CryptoCurrency).copyWith( |
There was a problem hiding this comment.
Parameter is unused & shadowed – leads to wrong segwit-ticker check
isCoinSegwitKnown ignores its argument and captures the outer currency, producing false negatives for non-crypto items and confusing future maintainers.
-bool isCoinSegwitKnown(String coinTicker) =>
- currency.isCrypto && knownCoins.containsKey('$coinTicker-segwit');
-if (isCoinSegwitKnown(currency.getAbbr())) {
+bool _isSegwitVariantKnown(ICurrency c) =>
+ c.isCrypto && knownCoins.containsKey('${c.getAbbr()}-segwit');
+
+if (_isSegwitVariantKnown(currency)) {📝 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.
| bool isCoinSegwitKnown(String coinTicker) => | |
| currency.isCrypto && knownCoins.containsKey('$coinTicker-segwit'); | |
| if (isCoinSegwitKnown(currency.getAbbr())) { | |
| final segwitCoin = knownCoins['${currency.getAbbr()}-segwit']; | |
| final segwitCurrency = (currency as CryptoCurrency).copyWith( | |
| // Replace the old, buggy helper: | |
| - bool isCoinSegwitKnown(String coinTicker) => | |
| - currency.isCrypto && knownCoins.containsKey('$coinTicker-segwit'); | |
| - if (isCoinSegwitKnown(currency.getAbbr())) { | |
| + bool _isSegwitVariantKnown(ICurrency c) => | |
| + c.isCrypto && knownCoins.containsKey('${c.getAbbr()}-segwit'); | |
| + | |
| + if (_isSegwitVariantKnown(currency)) { | |
| final segwitCoin = knownCoins['${currency.getAbbr()}-segwit']; | |
| final segwitCurrency = (currency as CryptoCurrency).copyWith( |
🤖 Prompt for AI Agents
In lib/bloc/fiat/fiat_repository.dart around lines 62 to 66, the function
isCoinSegwitKnown has a parameter coinTicker that is not used because the
function references the outer variable currency instead. To fix this, modify
isCoinSegwitKnown to use its coinTicker parameter inside the function body
instead of the outer currency variable, ensuring the check is performed
correctly based on the passed argument.
| Decimal _calculateSpotPriceIncludingFee(FiatPaymentMethod paymentMethod) { | ||
| // Use the previous coin and fiat amounts to estimate the spot price | ||
| // including fee. | ||
| final coinAmount = | ||
| double.parse(paymentMethod['price_info']['coin_amount'] as String); | ||
| final fiatAmount = | ||
| double.parse(paymentMethod['price_info']['fiat_amount'] as String); | ||
| final coinAmount = paymentMethod.priceInfo.coinAmount; | ||
| final fiatAmount = paymentMethod.priceInfo.fiatAmount; | ||
| final spotPriceIncludingFee = fiatAmount / coinAmount; | ||
| return spotPriceIncludingFee.toString(); | ||
| return spotPriceIncludingFee.toDecimal( | ||
| scaleOnInfinitePrecision: scaleOnInfinitePrecision, | ||
| ); |
There was a problem hiding this comment.
Division-by-zero guard
fiatAmount / coinAmount will throw if coinAmount == 0. Providers occasionally return zero when a quote fails, so add a sanity check.
- final spotPriceIncludingFee = fiatAmount / coinAmount;
+ if (coinAmount == Decimal.zero) {
+ _log.warning(
+ 'Spot-price calculation skipped: coinAmount is zero for '
+ '${paymentMethod.id}',
+ );
+ return Decimal.zero;
+ }
+ final spotPriceIncludingFee = fiatAmount / coinAmount;📝 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.
| Decimal _calculateSpotPriceIncludingFee(FiatPaymentMethod paymentMethod) { | |
| // Use the previous coin and fiat amounts to estimate the spot price | |
| // including fee. | |
| final coinAmount = | |
| double.parse(paymentMethod['price_info']['coin_amount'] as String); | |
| final fiatAmount = | |
| double.parse(paymentMethod['price_info']['fiat_amount'] as String); | |
| final coinAmount = paymentMethod.priceInfo.coinAmount; | |
| final fiatAmount = paymentMethod.priceInfo.fiatAmount; | |
| final spotPriceIncludingFee = fiatAmount / coinAmount; | |
| return spotPriceIncludingFee.toString(); | |
| return spotPriceIncludingFee.toDecimal( | |
| scaleOnInfinitePrecision: scaleOnInfinitePrecision, | |
| ); | |
| Decimal _calculateSpotPriceIncludingFee(FiatPaymentMethod paymentMethod) { | |
| // Use the previous coin and fiat amounts to estimate the spot price | |
| // including fee. | |
| final coinAmount = paymentMethod.priceInfo.coinAmount; | |
| final fiatAmount = paymentMethod.priceInfo.fiatAmount; | |
| if (coinAmount == Decimal.zero) { | |
| _log.warning( | |
| 'Spot-price calculation skipped: coinAmount is zero for ' | |
| '${paymentMethod.id}', | |
| ); | |
| return Decimal.zero; | |
| } | |
| final spotPriceIncludingFee = fiatAmount / coinAmount; | |
| return spotPriceIncludingFee.toDecimal( | |
| scaleOnInfinitePrecision: scaleOnInfinitePrecision, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/fiat/fiat_repository.dart around lines 140 to 148, the division
fiatAmount / coinAmount can throw a division-by-zero error if coinAmount is
zero. Add a guard clause to check if coinAmount is zero before performing the
division. If coinAmount is zero, return a default Decimal value or handle the
case gracefully to avoid the exception.
| final coinAmounts = _paymentMethodsList! | ||
| .map((method) => double.parse(method['price_info']['coin_amount'])) | ||
| .map((method) => method.priceInfo.coinAmount) | ||
| .toList(); | ||
| final maxCoinAmount = coinAmounts.reduce((a, b) => a > b ? a : b); | ||
| return _paymentMethodsList!.map((method) { | ||
| final coinAmount = double.parse(method['price_info']['coin_amount']); | ||
| if (coinAmount == 0) { | ||
| final coinAmount = method.priceInfo.coinAmount; | ||
| if (coinAmount == Decimal.zero) { | ||
| return method; | ||
| } | ||
| if (coinAmount == maxCoinAmount) { | ||
| return { | ||
| ...method, | ||
| 'relative_percent': null, | ||
| }; | ||
| return method.copyWith(relativePercent: Decimal.zero); | ||
| } | ||
|
|
||
| final relativeValue = | ||
| (coinAmount - maxCoinAmount) / (maxCoinAmount).abs(); | ||
| (coinAmount - maxCoinAmount) / maxCoinAmount.abs(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the passed list instead of the global cache & fix percent sign
_addRelativePercentField ignores its paymentMethodsList parameter and works on _paymentMethodsList!, making the method order-dependent and side-effect prone.
- final coinAmounts = _paymentMethodsList!
+ final coinAmounts = paymentMethodsList
.map((method) => method.priceInfo.coinAmount)
.toList();
...
- return _paymentMethodsList!.map((method) {
+ return paymentMethodsList.map((method) {Additionally, (coinAmount - max)/max returns negative values (best = 0, others < 0).
If you meant “how much less favourable this offer is”, negate the numerator or document the negative semantics.
🤖 Prompt for AI Agents
In lib/bloc/fiat/fiat_repository.dart around lines 291 to 306, the method
_addRelativePercentField incorrectly uses the global _paymentMethodsList instead
of the passed paymentMethodsList parameter, causing side effects and order
dependency. Fix this by replacing all references to _paymentMethodsList with the
local paymentMethodsList parameter. Also, correct the calculation of
relativeValue so it returns positive percentages by negating the numerator or
adjusting the formula to (maxCoinAmount - coinAmount) / maxCoinAmount.abs(),
ensuring the relativePercent reflects how much less favorable each offer is
compared to the max.
|
Last issue(s) blocking approval: https://github.com/KomodoPlatform/komodo-wallet/issues?q=is%3Aissue%20state%3Aopen%20label%3AP0 A solution which would accept the updated coins file needs to remove the This is a required change to continue allowing rolling coins file updates on app launch, otherwise users will lose the ability to activate EVM coins when https://github.com/KomodoPlatform/coins/pull/1351 is merged. Related: GLEECBTC/komodo-defi-sdk-flutter#55 |
DeckerSU
left a comment
DeckerSU
left a comment
There was a problem hiding this comment.
LGTM.
From a security standpoint, everything generally looks fine. The dependencies have been reviewed — some of them even line by line — and no malicious inclusions or known CVEs were found. The source code itself has also been reviewed for areas that could potentially be targeted by attackers.
The one concern is on the Windows platform: we’re launching the kdf binary with the wallet password (wallet_password) inside a command-line argument. An attacker could easily read that password from the process arguments — for example, using tools like Process Explorer from Sysinternals — decrypt the wallet, and access the seed phrase.
Of course, if an attacker already has access to the system, nothing can truly be considered secure — the system is already compromised. But we should still make such attacks as difficult as possible and avoid transferring plain-text passwords through mechanisms where they can be easily exposed.
p.s. From my point of view, the goal is to operate the "encrypted wallet" under the assumption that the password can't be read from anywhere — whether it's process arguments, memory, text files, or any other easily accessible source.
* removes redundant readme mentioning remplate.html * updates meta title, description tags for SEO
* fix: fix SDK roll CI script issues - Fix SDK roll CI script issues - Roll SDK version * CI Test #1 * build: update remaining SDK commit refs in lockfiles * CI Test 2 * fix(icon-pre-caching): exclude excluded assets to remove NFT_ coins `NFT_` prefixed coins do not have icons and should not be shown to users, so filter them out from the icon pre-caching * chore: roll SDK * chore(deps): upgrade SDK to include fee info fix GLEECBTC/komodo-defi-sdk-flutter#60 --------- Co-authored-by: Francois <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
lib/bloc/app_bloc_root.dart (1)
416-419:⚠️ Potential issueReplace deprecated CoinIcon with AssetIcon.
The pipeline indicates thatCoinIconis deprecated. Replace it withAssetIconfrom the SDK'skomodo_uipackage.- await CoinIcon.precacheCoinIcon(context, abbr) - .onError((_, __) => debugPrint('Error precaching coin icon $abbr')); + await AssetIcon.precacheAssetIcon(context, abbr) + .onError((_, __) => debugPrint('Error precaching asset icon $abbr'));🧰 Tools
🪛 GitHub Actions: CharlVS is validating code guidelines 🚀
[warning] 417-417: 'CoinIcon' is deprecated and shouldn't be used. Use AssetIcon from the SDK's
komodo_uipackage instead.
♻️ Duplicate comments (5)
.github/scripts/roll_sdk_packages.sh (2)
16-16: 🛠️ Refactor suggestionAdd stricter error handling.
Currently the script usesset -e, which stops on the first error, but pipelines can still mask failures. It's recommended to also enablepipefail(and optionallynounset) to catch errors in piped commands and unset variables.- set -e + set -euo pipefail
148-155: 🛠️ Refactor suggestionPrevent generating changelog when no rolls occurred.
The script unconditionally initializesSDK_CHANGELOG.mdat the start, so the workflow will always detect a changelog file—even if no SDK packages were rolled. Move the header initialization block inside theif [ "$ROLLS_MADE" = true ]check (after detecting changes), or remove the file whenROLLS_MADEis false.Suggested adjustment:
- # Initialize changes file - echo "# SDK Package Rolls" > "$CHANGES_FILE" - echo "" >> "$CHANGES_FILE" - echo "**Date:** $CURRENT_DATE" >> "$CHANGES_FILE" - echo "**Target Branch:** $TARGET_BRANCH" >> "$CHANGES_FILE" - echo "**Upgrade Mode:** $([ "$UPGRADE_ALL_PACKAGES" = "true" ] && echo "All Packages" || echo "SDK Packages Only")" >> "$CHANGES_FILE" - echo "" >> "$CHANGES_FILE" - echo "The following SDK packages were rolled to newer versions:" >> "$CHANGES_FILE" - echo "" >> "$CHANGES_FILE" + # Defer initializing the changelog until a roll is detectedlib/bloc/app_bloc_root.dart (2)
391-398: 🛠️ Refactor suggestionFix race condition when aborting an ongoing precache.
Completing the previous_currentPrecacheOperationwithout signalling error loses information about why the task was interrupted and may leave upstreamawaiters in an inconsistent state.if (_currentPrecacheOperation != null && !_currentPrecacheOperation!.isCompleted) { - // completeError throws an uncaught exception, which causes the UI - // tests to fail when switching between light and dark theme - log('New request to precache icons started.'); - _currentPrecacheOperation!.complete(); + log('Cancelling previous precache operation'); + try { + _currentPrecacheOperation!.completeError( + StateError('Precache cancelled by a subsequent request'), + ); + } catch (e) { + // Safely handle potential exceptions during cancellation + log('Error while cancelling previous precache: $e'); + } }
80-80: 🛠️ Refactor suggestionAddress TODO comment regarding bloat in main file.
The TODO comment on line 80 indicates awareness of code bloat in this file, but no action has been taken to address it.Consider breaking this file into smaller components:
- Separate repository providers and bloc providers into dedicated factory methods
- Extract initialization logic to separate helper classes
- Group related bloc providers by feature domain
This will improve maintainability and testability of the application initialization process.
lib/bloc/coins_bloc/coins_repo.dart (1)
300-307:⚠️ Potential issueIncorrect asset passed to
activateAsset– only the first coin is activatedInside the loop you call
activateAsset(assets.single)which always
targets the first element of the original list.- final progress = await _kdfSdk.assets.activateAsset(assets.single).last; + final progress = await _kdfSdk.assets.activateAsset(asset).last;Without this fix every iteration but the first becomes a no-op.
🧹 Nitpick comments (7)
.github/scripts/roll_sdk_packages.sh (1)
260-260: Remove or use the unused variable.
ThePACKAGE_UPDATE_FAILEDvariable is set but never used. Either use it for proper error handling or remove it.- PACKAGE_UPDATE_FAILED=true + # Either use this variable to track failures: + PACKAGE_UPDATE_FAILED=true + # Later check and handle the failed packages + # Or simply remove it if not needed🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 260-260: PACKAGE_UPDATE_FAILED appears unused. Verify use (or export if used externally).
(SC2034)
docs/SDK_DEPENDENCY_MANAGEMENT.md (2)
90-91: Fix list formatting for script parameters.
The bullet points in this list have loose punctuation which affects readability. Add consistent spacing after the bullet points.-The script accepts the following environment variables: - -- `UPGRADE_ALL_PACKAGES`: Set to `true` to upgrade all packages, not just SDK packages. Defaults to `false`. -- `TARGET_BRANCH`: The target branch for the changelog information. Defaults to `dev`. +The script accepts the following environment variables: + +- `UPGRADE_ALL_PACKAGES`: Set to `true` to upgrade all packages, not just SDK packages. Defaults to `false`. +- `TARGET_BRANCH`: The target branch for the changelog information. Defaults to `dev`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: Loose punctuation mark.
Context: ...ent variables: -UPGRADE_ALL_PACKAGES: Set totrueto upgrade all packages, ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~91-~91: Loose punctuation mark.
Context: .... Defaults tofalse. -TARGET_BRANCH: The target branch for the changelog inf...(UNLIKELY_OPENING_PUNCTUATION)
108-111: Fix list formatting for exit codes.
Similar to the previous issue, the bullet points in this list need consistent spacing.-The script uses the following exit codes: - -- `0`: Success - SDK packages were rolled successfully -- `100`: No updates needed - the script ran correctly but no packages needed updating -- Any other code: An error occurred during the execution +The script uses the following exit codes: + +- `0`: Success - SDK packages were rolled successfully +- `100`: No updates needed - the script ran correctly, but no packages needed updating +- Any other code: An error occurred during the execution🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ...pt uses the following exit codes: -0: Success - SDK packages were rolled succ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...pdates needed - the script ran correctly but no packages needed updating - Any other...(COMMA_COMPOUND_SENTENCE_2)
.github/workflows/roll-sdk-packages.yml (2)
91-91: Remove trailing whitespace.
There's a trailing whitespace on this line that should be removed for consistency.- +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 91-91: trailing spaces
(trailing-spaces)
129-134: Consider using GitHub API for branch deletion.
When dealing with protected branches, the current approach may fail. Using the GitHub API via the gh CLI could be more reliable.- git push origin --delete ${{ env.PR_BRANCH_NAME }} || { - echo "::warning::Failed to delete existing branch ${{ env.PR_BRANCH_NAME }} - it may be protected" - } + # Try to delete with git first + git push origin --delete ${{ env.PR_BRANCH_NAME }} 2>/dev/null || { + # If git push fails, try using GitHub API via gh CLI + echo "::notice::Attempting to delete branch via GitHub API..." + gh api -X DELETE repos/{owner}/{repo}/git/refs/heads/${{ env.PR_BRANCH_NAME }} --silent || { + echo "::warning::Failed to delete existing branch ${{ env.PR_BRANCH_NAME }} - it may be protected" + } + }lib/bloc/coins_bloc/coins_repo.dart (2)
412-421:getUsdPriceByAmountstill depends on the deprecated helperReplace the call to
getCoinwith the non-deprecated alternatives:- final Coin? coin = getCoin(coinAbbr); + final Coin? coin = getCoinFromId(AssetId(coinAbbr));(or inject the coin information from the caller).
This keeps the method usable once
getCoinis finally removed.
474-483: Add timeout & retry logic to external price requests
http.get(pricesUrlV3)blocks indefinitely on network stalls.
A short timeout (e.g., 10 s) with exponential back-off would make
the wallet more resilient to connectivity issues.Example:
- res = await http.get(pricesUrlV3); + res = await http + .get(pricesUrlV3) + .timeout(const Duration(seconds: 10));Consider wrapping the call in a retry helper to respect transient failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/actions/flutter-deps/action.yml(1 hunks).github/scripts/roll_sdk_packages.sh(1 hunks).github/workflows/roll-sdk-packages.yml(1 hunks)README.md(1 hunks)docs/SDK_DEPENDENCY_MANAGEMENT.md(1 hunks)lib/app_config/app_config.dart(7 hunks)lib/bloc/app_bloc_root.dart(11 hunks)lib/bloc/coins_bloc/coins_repo.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- .github/actions/flutter-deps/action.yml
- lib/app_config/app_config.dart
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/app_bloc_root.dart (1)
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.
🪛 GitHub Actions: CharlVS is validating code guidelines 🚀
lib/bloc/coins_bloc/coins_repo.dart
[warning] 79-238: 'enabledType', 'getCoin', and 'getFirstPubkey' are deprecated and shouldn't be used. Use SDK WalletManager and pubkeys.getPubkeys.
[warning] 324-370: 'CoinIcon' is deprecated and shouldn't be used. Use AssetIcon from the SDK's komodo_ui package instead.
[warning] 394-634: 'call' and 'getCoin' are deprecated and shouldn't be used. Use KomodoDefiSdk.client.rpc or KomodoDefiSdk.client.executeRpc and assets or getCoinFromId.
lib/bloc/app_bloc_root.dart
[warning] 417-417: 'CoinIcon' is deprecated and shouldn't be used. Use AssetIcon from the SDK's komodo_ui package instead.
🪛 Shellcheck (0.10.0)
.github/scripts/roll_sdk_packages.sh
[warning] 99-99: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 106-106: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 116-116: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 120-120: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 121-121: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 258-258: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 260-260: PACKAGE_UPDATE_FAILED appears unused. Verify use (or export if used externally).
(SC2034)
🪛 actionlint (1.7.7)
.github/workflows/roll-sdk-packages.yml
53-53: shellcheck reported issue in this script: SC2086:info:9:40: Double quote to prevent globbing and word splitting
(shellcheck)
53-53: shellcheck reported issue in this script: SC2086:info:17:36: Double quote to prevent globbing and word splitting
(shellcheck)
53-53: shellcheck reported issue in this script: SC2086:info:20:52: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:9:31: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:11:33: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 YAMLlint (1.37.1)
.github/workflows/roll-sdk-packages.yml
[error] 91-91: trailing spaces
(trailing-spaces)
🪛 LanguageTool
docs/SDK_DEPENDENCY_MANAGEMENT.md
[uncategorized] ~90-~90: Loose punctuation mark.
Context: ...ent variables: - UPGRADE_ALL_PACKAGES: Set to true to upgrade all packages, ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~91-~91: Loose punctuation mark.
Context: .... Defaults to false. - TARGET_BRANCH: The target branch for the changelog inf...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ...pt uses the following exit codes: - 0: Success - SDK packages were rolled succ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...pdates needed - the script ran correctly but no packages needed updating - Any other...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (4)
docs/SDK_DEPENDENCY_MANAGEMENT.md (1)
1-14: Documentation is clear and well-structured.
The documentation provides a comprehensive overview of SDK dependencies and management processes. It clearly identifies the packages from the external repository and their purpose.lib/bloc/app_bloc_root.dart (3)
72-74: Good SDK integration.
The refactoring to require theKomodoDefiSdkinstance in the constructor aligns well with the PR's goal of migrating to the KDF Flutter SDK.
403-404: Good implementation of getKnownCoinsMap.
The switch fromgetKnownCoins()togetKnownCoinsMap(excludeExcludedAssets: true).keysis correct and consistent with other changes in the codebase.
120-126:❌ Incorrect review comment
Update MockTransactionHistoryRepo to be compatible with SDK.
The current implementation still usesMockTransactionHistoryRepofor demo mode but usesSdkTransactionHistoryRepositoryotherwise. Consider updating theMockTransactionHistoryRepoto work with the new SDK architecture for consistency.
🏁 Script executed:
#!/bin/bash # Check implementation of MockTransactionHistoryRepo rg -A 10 "class MockTransactionHistoryRepo" --type dartLength of output: 1187
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of MockTransactionHistoryRepo implementation to inspect how it generates data sed -n '1,200p' lib/bloc/cex_market_data/mockup/mock_transaction_history_repository.dartLength of output: 928
MockTransactionHistoryRepo is intentionally a separate demo implementation
TheMockTransactionHistoryRepoimplements the sameTransactionHistoryRepointerface asSdkTransactionHistoryRepositorybut usesDemoDataCacheto generate mock data in demo/performance mode. This decoupling is by design, so no changes are needed to make it “compatible” with the SDK architecture.•
MockTransactionHistoryRepo
– ImplementsTransactionHistoryRepo
– UsesdemoDataGenerator.loadTransactionsDemoData(...)for demo data
•SdkTransactionHistoryRepositoryremains the production implementationNo updates required.
Likely an incorrect or invalid review comment.
| # Upgrade all SDK packages at once | ||
| if [ ${#SDK_PACKAGES_FOUND[@]} -gt 0 ]; then | ||
| log_info "Upgrading packages: ${SDK_PACKAGES_FOUND[*]}" | ||
| if ! flutter pub upgrade --unlock-transitive ${SDK_PACKAGES_FOUND[@]}; then |
There was a problem hiding this comment.
Fix potential array splitting issue.
When expanding arrays in bash, it's important to quote them to prevent word splitting and globbing.
- if ! flutter pub upgrade --unlock-transitive ${SDK_PACKAGES_FOUND[@]}; then
+ if ! flutter pub upgrade --unlock-transitive "${SDK_PACKAGES_FOUND[@]}"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! flutter pub upgrade --unlock-transitive ${SDK_PACKAGES_FOUND[@]}; then | |
| if ! flutter pub upgrade --unlock-transitive "${SDK_PACKAGES_FOUND[@]}"; then |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 258-258: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
In .github/scripts/roll_sdk_packages.sh at line 258, the array
SDK_PACKAGES_FOUND is expanded without quotes, which can cause word splitting
and globbing issues. To fix this, enclose the array expansion in double quotes
like "${SDK_PACKAGES_FOUND[@]}" to ensure each element is treated as a separate
argument and prevent unintended splitting or globbing.
| id: roll_packages | ||
| run: | | ||
| # Run the script and capture exit code | ||
| UPGRADE_ALL_PACKAGES=${{ env.UPGRADE_ALL }} TARGET_BRANCH=${{ env.TARGET_BRANCH }} .github/scripts/roll_sdk_packages.sh || EXIT_CODE=$? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error variable handling.
The current method of capturing exit codes may not reliably capture all failure scenarios. Use a more reliable approach to capture the exit code.
- UPGRADE_ALL_PACKAGES=${{ env.UPGRADE_ALL }} TARGET_BRANCH=${{ env.TARGET_BRANCH }} .github/scripts/roll_sdk_packages.sh || EXIT_CODE=$?
+ UPGRADE_ALL_PACKAGES=${{ env.UPGRADE_ALL }} TARGET_BRANCH=${{ env.TARGET_BRANCH }} .github/scripts/roll_sdk_packages.sh
+ EXIT_CODE=$?📝 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.
| UPGRADE_ALL_PACKAGES=${{ env.UPGRADE_ALL }} TARGET_BRANCH=${{ env.TARGET_BRANCH }} .github/scripts/roll_sdk_packages.sh || EXIT_CODE=$? | |
| UPGRADE_ALL_PACKAGES=${{ env.UPGRADE_ALL }} TARGET_BRANCH=${{ env.TARGET_BRANCH }} .github/scripts/roll_sdk_packages.sh | |
| EXIT_CODE=$? |
🤖 Prompt for AI Agents
In .github/workflows/roll-sdk-packages.yml at line 83, the current method of
capturing the exit code using '|| EXIT_CODE=$?' may not reliably capture all
failure scenarios. Modify the script execution line to run the script normally,
then immediately capture the exit code in a separate step or command by
assigning '$?' to EXIT_CODE right after the script finishes. This ensures the
exit code is accurately captured regardless of how the script exits.
| @Deprecated('Use SDK pubkeys.getPubkeys instead and let the user ' | ||
| 'select from the available options.') | ||
| Future<String?> getFirstPubkey(String coinId) async { | ||
| final asset = _kdfSdk.assets.findAssetsByConfigId(coinId).single; | ||
| final pubkeys = await _kdfSdk.pubkeys.getPubkeys(asset); | ||
| if (pubkeys.keys.isEmpty) { | ||
| return null; | ||
| } | ||
| return pubkeys.keys.first.address; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Delete getFirstPubkey and inline the SDK call
The helper is itself deprecated and only wraps the modern pubkeys.getPubkeys.
Remove the method, migrate callers to:
final pubkeys = await _kdfSdk.pubkeys.getPubkeys(asset);
final firstAddress =
pubkeys.keys.isNotEmpty ? pubkeys.keys.first.address : null;🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart lines 401 to 410, the getFirstPubkey
method is deprecated and simply wraps the SDK call _kdfSdk.pubkeys.getPubkeys.
Remove this method entirely and update all its callers to directly use the SDK
call by fetching pubkeys with _kdfSdk.pubkeys.getPubkeys(asset) and then
checking if pubkeys.keys is not empty to get the first address or return null.
| final currentUser = await _kdfSdk.auth.currentUser; | ||
| if (currentUser != null) { | ||
| coin.enabledType = currentUser.wallet.config.type; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop mutating the deprecated enabledType field
coin.enabledType has been marked deprecated (see pipeline warnings 79-238).
Persisting this value here locks the model to a legacy API and forces every downstream consumer to keep it alive.
- if (currentUser != null) {
- coin.enabledType = currentUser.wallet.config.type;
- }
+ // ⚠️ `enabledType` is deprecated – remove the mutation and let
+ // callers read the wallet type directly from `currentUser`.Removing the mutation avoids noise during static-analysis and helps the gradual migration to WalletManager.
📝 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.
| final currentUser = await _kdfSdk.auth.currentUser; | |
| if (currentUser != null) { | |
| coin.enabledType = currentUser.wallet.config.type; | |
| } | |
| final currentUser = await _kdfSdk.auth.currentUser; | |
| // ⚠️ `enabledType` is deprecated – remove the mutation and let | |
| // callers read the wallet type directly from `currentUser`. |
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around lines 77 to 81, the code mutates
the deprecated field `coin.enabledType`. To fix this, remove the assignment to
`coin.enabledType` entirely to avoid using the legacy API and reduce
static-analysis warnings. Instead, rely on the new `WalletManager` for managing
wallet types and related logic.
| CoinIcon.registerCustomIcon( | ||
| coin.id.id, | ||
| NetworkImage(coin.logoImageUrl!), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated CoinIcon with AssetIcon
CoinIcon is flagged as deprecated by the pipeline.
Switch to the SDK’s AssetIcon.register helper:
- CoinIcon.registerCustomIcon(
- coin.id.id,
- NetworkImage(coin.logoImageUrl!),
- );
+ AssetIcon.register(
+ id: coin.id,
+ imageProvider: NetworkImage(coin.logoImageUrl!),
+ );📝 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.
| CoinIcon.registerCustomIcon( | |
| coin.id.id, | |
| NetworkImage(coin.logoImageUrl!), | |
| ); | |
| } | |
| } | |
| AssetIcon.register( | |
| id: coin.id, | |
| imageProvider: NetworkImage(coin.logoImageUrl!), | |
| ); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: CharlVS is validating code guidelines 🚀
[warning] 324-370: 'CoinIcon' is deprecated and shouldn't be used. Use AssetIcon from the SDK's komodo_ui package instead.
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around lines 324 to 329, replace the
deprecated CoinIcon.registerCustomIcon call with the SDK's AssetIcon.register
method. Update the code to use AssetIcon.register, passing the coin's id and the
NetworkImage of the coin's logo URL accordingly to conform with the new API.
| Future<void> _disableCoin(String coinId) async { | ||
| try { | ||
| await _mm2.call(DisableCoinReq(coin: coinId)); | ||
| } catch (e, s) { | ||
| _log.shout('Error disabling $coinId', e, s); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use SDK RPC façade instead of deprecated _mm2.call
The direct call on MM2 is deprecated.
Leverage the unified RPC surface that ships with the SDK:
- await _mm2.call(DisableCoinReq(coin: coinId));
+ await _kdfSdk.client.executeRpc(DisableCoinReq(coin: coinId));This removes one more deprecation warning and keeps the repo aligned with the new transport layer.
📝 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.
| Future<void> _disableCoin(String coinId) async { | |
| try { | |
| await _mm2.call(DisableCoinReq(coin: coinId)); | |
| } catch (e, s) { | |
| _log.shout('Error disabling $coinId', e, s); | |
| return; | |
| } | |
| } | |
| Future<void> _disableCoin(String coinId) async { | |
| try { | |
| await _kdfSdk.client.executeRpc(DisableCoinReq(coin: coinId)); | |
| } catch (e, s) { | |
| _log.shout('Error disabling $coinId', e, s); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around lines 392 to 399, replace the
deprecated direct call to _mm2.call with the new SDK RPC façade method for
disabling a coin. Identify the appropriate RPC method provided by the SDK's
unified RPC surface to disable the coin and use it instead of _mm2.call,
ensuring the code no longer triggers deprecation warnings and aligns with the
updated transport layer.
| Coin? getCoinFromId(AssetId id) { | ||
| final asset = _kdfSdk.assets.available[id]; | ||
| if (asset == null) return null; | ||
| return _assetToCoinWithoutAddress(asset); | ||
| } | ||
|
|
||
| final List<Coin> unmodifiableCoins = List.unmodifiable(coins); | ||
| _cachedKnownCoins = unmodifiableCoins; | ||
| return unmodifiableCoins; | ||
| @Deprecated('Use KomodoDefiSdk assets or getCoinFromId instead.') | ||
| Coin? getCoin(String coinId) { | ||
| if (coinId.isEmpty) return null; | ||
|
|
||
| try { | ||
| final assets = _kdfSdk.assets.assetsFromTicker(coinId); | ||
| if (assets.isEmpty || assets.length > 1) { | ||
| _log.warning( | ||
| 'Coin "$coinId" not found. ${assets.length} results returned', | ||
| ); | ||
| return null; | ||
| } | ||
| return _assetToCoinWithoutAddress(assets.single); | ||
| } catch (_) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retire the deprecated getCoin() helper
getCoin() still relies on a ticker lookup that:
- Collides when several assets share the same ticker (e.g.
KMD,KMD-BEP20). - Keeps the deprecated API alive, triggering warnings on every call.
Replace all invocations with getCoinFromId() or direct SDK look-ups and delete this method.
-@Deprecated('Use KomodoDefiSdk assets or getCoinFromId instead.')
-Coin? getCoin(String coinId) { … }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart between lines 155 and 177, the getCoin()
method is deprecated and causes issues due to ticker collisions and triggers
warnings. Remove the entire getCoin() method and replace all its usages in the
codebase with getCoinFromId() or direct calls to the SDK's asset lookup methods
to ensure consistent and warning-free asset retrieval.
| try { | ||
| response = await _mm2.call(request) as Map<String, dynamic>?; | ||
| } catch (e, s) { | ||
| _log.shout('Error withdrawing ${request.params.coin}', e, s); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Withdraw flow still uses deprecated RPC call
Same recommendation as above – swap _mm2.call for the SDK client:
- response = await _mm2.call(request) as Map<String, dynamic>?;
+ response =
+ await _kdfSdk.client.executeRpc(request) as Map<String, dynamic>?;📝 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.
| try { | |
| response = await _mm2.call(request) as Map<String, dynamic>?; | |
| } catch (e, s) { | |
| _log.shout('Error withdrawing ${request.params.coin}', e, s); | |
| } | |
| try { | |
| response = | |
| await _kdfSdk.client.executeRpc(request) as Map<String, dynamic>?; | |
| } catch (e, s) { | |
| _log.shout('Error withdrawing ${request.params.coin}', e, s); | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around lines 633 to 637, the withdraw
flow uses the deprecated RPC call _mm2.call. Replace this call with the
appropriate method from the SDK client to ensure the code uses the updated and
supported API. Adjust the code to await the SDK client's withdraw method and
handle the response accordingly, maintaining the existing error logging.
* fix: fix SDK roll CI script issues - Fix SDK roll CI script issues - Roll SDK version * CI Test #1 * build: update remaining SDK commit refs in lockfiles * CI Test 2 * fix(icon-pre-caching): exclude excluded assets to remove NFT_ coins `NFT_` prefixed coins do not have icons and should not be shown to users, so filter them out from the icon pre-caching * chore: roll SDK * docs(analytics): add event implementation plan * feat(analytics): log onboarding and app lifecycle events * feat(analytics): add send and swap event callbacks * feat(analytics): log events for hd address and page metrics * feat(analytics): add portfolio and asset event logging * fix(analytics): Fix analytics type errors * fix(analytics): fix missing import * chore(deps): upgrade SDK to include fee info fix GLEECBTC/komodo-defi-sdk-flutter#60 * feat(analytics): add high priority events * fix: update alpha version warning * feat(analytics): add cross-chain events and triggers * feat(analytics): trigger new event logs * fix: various analytics fixes/enhancements * refactor(analytics): log events directly * fix: various analytics fixes * feat: analytics event queuing --------- Co-authored-by: Francois <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
lib/bloc/coins_manager/coins_manager_bloc.dart (2)
127-183: 🛠️ Refactor suggestionOptimize analytics logging and add error handling
The current implementation has potential performance and reliability issues:
- Performance: Multiple
await _sdk.auth.currentUsercalls during rapid coin selection- Error handling: No try-catch blocks for SDK calls that could fail
- Blocking operations: Analytics logging could block the UI thread
Consider this optimization:
+ String? _cachedWalletType; + Future<void> _onCoinSelect( CoinsManagerCoinSelect event, Emitter<CoinsManagerState> emit, ) async { + try { + _cachedWalletType ??= (await _sdk.auth.currentUser)?.wallet.config.type.name; + } catch (e) { + // Handle SDK errors gracefully + _cachedWalletType = ''; + } + // ... existing logic ... if (state.action == CoinsManagerAction.add) { _coinsRepo.deactivateCoinsSync([event.coin]); - _analyticsBloc.logEvent( - AssetDisabledEventData( - assetSymbol: coin.abbr, - assetNetwork: coin.protocolType, - walletType: (await _sdk.auth.currentUser)?.wallet.config.type.name ?? '', - ), - ); + // Fire and forget analytics to avoid blocking UI + _logAnalyticsEvent(AssetDisabledEventData( + assetSymbol: coin.abbr, + assetNetwork: coin.protocolType, + walletType: _cachedWalletType ?? '', + )); } } + + void _logAnalyticsEvent(dynamic event) { + // Non-blocking analytics logging + _analyticsBloc.logEvent(event); + }
244-284: 💡 Verification agent🧩 Analysis chain
Add comprehensive error handling for SDK operations
The helper methods lack robust error handling for SDK operations that could fail.
Consider adding proper error handling:
Future<List<Coin>> _getOriginalCoinList( CoinsRepo coinsRepo, CoinsManagerAction action, KomodoDefiSdk sdk, ) async { + try { final WalletType? walletType = (await sdk.currentWallet())?.config.type; if (walletType == null) return []; + } catch (e) { + // Log error and return empty list to prevent crashes + debugPrint('Failed to get wallet type: $e'); + return []; + } switch (action) { case CoinsManagerAction.add: return _getDeactivatedCoins(coinsRepo, sdk, walletType); case CoinsManagerAction.remove: return coinsRepo.getWalletCoins(); case CoinsManagerAction.none: return []; } }Also verify that
WalletType.hdwallet(line 274) is the correct enum value for HD wallets in the new SDK.
🏁 Script executed:
#!/bin/bash # Search for WalletType enum definition rg "enum WalletType" -A5 # Search for KomodoDefiSdk class declaration rg "class KomodoDefiSdk" -A5 # Search for currentWallet method signature rg "currentWallet" -A5Length of output: 34329
Add comprehensive error handling for SDK operations
Wrap both
sdk.currentWallet()andsdk.assets.getEnabledCoins()calls intry/catchto prevent unhandled exceptions and log failures.WalletType.hdwalletis valid per the enum.– File: lib/bloc/coins_manager/coins_manager_bloc.dart
• In_getOriginalCoinList, catch errors aroundsdk.currentWallet()
• In_getDeactivatedCoins, catch errors aroundsdk.assets.getEnabledCoins()Suggested diff:
Future<List<Coin>> _getOriginalCoinList( CoinsRepo coinsRepo, CoinsManagerAction action, KomodoDefiSdk sdk, ) async { - final WalletType? walletType = (await sdk.currentWallet())?.config.type; - if (walletType == null) return []; + WalletType? walletType; + try { + walletType = (await sdk.currentWallet())?.config.type; + if (walletType == null) return []; + } catch (e, st) { + debugPrint('Failed to get wallet type: $e\n$st'); + return []; + } switch (action) { case CoinsManagerAction.add: return _getDeactivatedCoins(coinsRepo, sdk, walletType); @@ Future<List<Coin>> _getDeactivatedCoins( CoinsRepo coinsRepo, KomodoDefiSdk sdk, WalletType walletType, ) async { - final Iterable<String> enabledCoins = await sdk.assets.getEnabledCoins(); + Iterable<String> enabledCoins; + try { + enabledCoins = await sdk.assets.getEnabledCoins(); + } catch (e, st) { + debugPrint('Failed to fetch enabled coins: $e\n$st'); + return []; + } final Map<String, Coin> disabledCoins = coinsRepo.getKnownCoinsMap() ..removeWhere((coinId, _) => enabledCoins.contains(coinId)) ..removeWhere((coinId, _) => excludedAssetList.contains(coinId)); …
♻️ Duplicate comments (5)
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart (1)
78-127: 🛠️ Refactor suggestionImprove error handling and USD calculation robustness.
- The generic exception for missing network asset could be more specific (as noted in previous review).
- The USD price calculation using string parsing is fragile.
if (networkAsset == null) { - throw Exception('Network asset ${state.network.formatted} not found'); + throw AssetNotFoundException('Network asset ${state.network.formatted} not found'); }For the USD calculation, consider handling the conversion more robustly:
- final usdBalance = - _coinsRepo.getUsdPriceByAmount(balance.toString(), tokenData.id.id); + final usdBalance = _coinsRepo.getUsdPriceByAmount( + balance.toString(), + tokenData.id.id, + ) ?? Decimal.zero; emit( state.copyWith( formStatus: FormStatus.success, tokenData: () => tokenData, tokenBalance: balance, - tokenBalanceUsd: - Decimal.tryParse(usdBalance?.toString() ?? '0.0') ?? Decimal.zero, + tokenBalanceUsd: usdBalance, formErrorMessage: '', ), );This assumes
getUsdPriceByAmountreturns aDecimal?directly, which would be cleaner than string conversions.lib/bloc/bridge_form/bridge_bloc.dart (1)
107-107:⚠️ Potential issueReplace usage of deprecated
getCoinmethod.This line still uses the deprecated
getCoinmethod that was flagged in a previous review. Please update to use the recommendedKomodoDefiSdk.assetsorgetCoinFromIdapproach.- final Coin? defaultTickerCoin = _coinsRepository.getCoin(event.ticker); + final Coin? defaultTickerCoin = _coinsRepository.getCoinFromId(event.ticker);lib/bloc/app_bloc_root.dart (2)
83-83: Address TODO comment regarding code organization.The TODO comment indicates awareness of code bloat, but this major migration has added even more complexity to this file.
421-422: Inconsistent coin retrieval method usage.The method has been updated to use
getKnownCoinsMap().keys, but other parts of the codebase still use the oldgetKnownCoins()method.lib/bloc/coins_manager/coins_manager_bloc.dart (1)
119-124:⚠️ Potential issueFix loading state race condition (duplicate issue)
The loading state (
isSwitching) is still being cleared before the async operations complete, causing premature hiding of loading indicators. This is the same issue identified in previous reviews.Apply this fix to ensure proper loading state management:
- emit(state.copyWith(selectedCoins: [], isSwitching: false)); - await switchingFuture; + await switchingFuture; + emit(state.copyWith(selectedCoins: [], isSwitching: false));
🧹 Nitpick comments (3)
docs/ANALYTICS.md (3)
26-36: Add language specification to fenced code block.The code block showing the directory structure should specify a language for proper syntax highlighting.
-``` +```text lib/analytics/events/ ├── user_engagement_events.dart ├── user_acquisition_events.dart ├── security_events.dart ├── portfolio_events.dart ├── transaction_events.dart ├── data_sync_events.dart ├── performance_events.dart └── ui_usability_events.dart<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 26-26: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `459-459`: **Add missing period for consistency.** The list item should end with a period to maintain consistent formatting throughout the document. ```diff - - Repository pattern provides a clean API + - Repository pattern provides a clean API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~459-~459: A period might be missing here.
Context: ... - Repository pattern provides a clean API 2. OOP-based Design - Events a...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
473-473: Add missing period for consistency.The last list item should end with a period to maintain consistent formatting.
- - Mock implementations are simpler + - Mock implementations are simpler.🧰 Tools
🪛 LanguageTool
[uncategorized] ~473-~473: A period might be missing here.
Context: ...isolation - Mock implementations are simpler(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
📜 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 (25)
Fetching(1 hunks)assets/translations/en.json(15 hunks)docs/ANALYTICS.md(1 hunks)docs/ANALYTICS_EVENT_IMPLEMENTATION_PLAN.md(1 hunks)lib/analytics/README.md(1 hunks)lib/analytics/analytics_factory.dart(1 hunks)lib/analytics/events.dart(1 hunks)lib/analytics/events/cross_chain_events.dart(1 hunks)lib/analytics/events/market_bot_events.dart(1 hunks)lib/analytics/events/misc_events.dart(1 hunks)lib/analytics/events/nft_events.dart(1 hunks)lib/analytics/events/portfolio_events.dart(1 hunks)lib/analytics/events/reward_events.dart(1 hunks)lib/analytics/events/security_events.dart(1 hunks)lib/analytics/events/transaction_events.dart(1 hunks)lib/analytics/events/user_acquisition_events.dart(1 hunks)lib/analytics/events/user_engagement_events.dart(1 hunks)lib/analytics/widgets/analytics_lifecycle_handler.dart(1 hunks)lib/bloc/analytics/analytics_bloc.dart(1 hunks)lib/bloc/analytics/analytics_repo.dart(2 hunks)lib/bloc/app_bloc_root.dart(11 hunks)lib/bloc/bridge_form/bridge_bloc.dart(8 hunks)lib/bloc/coin_addresses/bloc/coin_addresses_bloc.dart(1 hunks)lib/bloc/coins_manager/coins_manager_bloc.dart(6 hunks)lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- Fetching
- lib/analytics/README.md
- lib/analytics/events/reward_events.dart
- lib/analytics/events/user_acquisition_events.dart
- lib/analytics/events/nft_events.dart
- lib/analytics/events/misc_events.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/bloc/coin_addresses/bloc/coin_addresses_bloc.dart
- lib/bloc/analytics/analytics_repo.dart
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/app_bloc_root.dart (1)
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.
🪛 GitHub Actions: CharlVS is validating code guidelines 🚀
lib/bloc/app_bloc_root.dart
[warning] 17-17: Unused import: 'package:web_dex/bloc/analytics/analytics_event.dart' (unused_import)
🪛 LanguageTool
docs/ANALYTICS.md
[uncategorized] ~459-~459: A period might be missing here.
Context: ... - Repository pattern provides a clean API 2. OOP-based Design - Events a...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
[uncategorized] ~473-~473: A period might be missing here.
Context: ...isolation - Mock implementations are simpler
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
docs/ANALYTICS_EVENT_IMPLEMENTATION_PLAN.md
[uncategorized] ~56-~56: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...page | Log when user chooses light/dark/auto theme. | | page_interactive_delay | Page lo...
(AUTO_HYPHEN)
🪛 markdownlint-cli2 (0.17.2)
docs/ANALYTICS.md
26-26: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: unit_tests
🔇 Additional comments (30)
lib/bloc/analytics/analytics_bloc.dart (2)
59-62: Good architectural improvement - delegates responsibility appropriately.The change from conditional
sendDatato unconditionalqueueEventsimplifies the bloc and delegates state management to the repository level, which is a cleaner separation of concerns.
65-70: Useful convenience extension for cleaner event logging.The
AnalyticsBlocEventLoggerextension provides a clean API for logging events without requiring callers to manually wrap events inAnalyticsSendDataEvent.lib/analytics/events/user_engagement_events.dart (2)
8-25: Well-structured analytics event data implementation.The
AppOpenedEventDataclass properly implements theAnalyticsEventDatainterface with appropriate event name and parameters. The implementation follows consistent patterns for analytics events.
27-38: Clean event wrapper implementation.The
AnalyticsAppOpenedEventclass provides a clean wrapper that properly constructs the data object and passes it to the parent class.docs/ANALYTICS_EVENT_IMPLEMENTATION_PLAN.md (2)
1-11: Excellent comprehensive documentation for analytics implementation.This documentation provides clear, actionable guidance for implementing analytics events throughout the application. The structured approach with trigger locations and implementation notes will ensure consistent analytics integration.
56-56: Minor: Static analysis false positive about hyphen.The static analysis tool flagged "light/dark/auto" as needing a hyphen, but this is a false positive. In UI contexts, "light/dark/auto" (referring to theme options) is commonly written without hyphens and is perfectly clear.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...page | Log when user chooses light/dark/auto theme. | |page_interactive_delay| Page lo...(AUTO_HYPHEN)
lib/analytics/widgets/analytics_lifecycle_handler.dart (2)
51-56: Good use of postFrameCallback for initial event logging.Using
addPostFrameCallbackensures the initial app opened event is logged after the widget tree is fully built, which is the correct approach for lifecycle-dependent operations.
59-63: Proper cleanup of lifecycle observer.The widget correctly removes the observer in the dispose method, preventing memory leaks.
lib/analytics/events/security_events.dart (2)
5-42: LGTM! Well-structured analytics event classes.The implementation follows a clean pattern for analytics events with proper separation between data and event classes. Parameter naming is consistent with JSON conventions.
44-76: Clean implementation of backup skip tracking.The event structure is consistent with the backup completion event and properly captures the stage at which backup was skipped.
lib/analytics/events/cross_chain_events.dart (2)
5-47: Well-structured bridge initiation event.The event properly captures the essential information for tracking bridge transfers with clear parameter names.
98-140: Bridge failure event looks good.The event structure properly captures bridge failure information. Ensure that error messages are sanitized to avoid exposing sensitive technical details in analytics.
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart (2)
36-59: Clean form reset implementation.The method properly resets all state fields and correctly filters EVM protocols from available coin types.
129-168: Import logic and analytics integration look good.The method properly handles the import flow and correctly logs the analytics event with appropriate parameters.
lib/analytics/events/market_bot_events.dart (1)
1-136: LGTM! Well-structured market bot analytics events.The implementation follows excellent practices:
- Consistent event naming with snake_case
- Proper separation of event data and wrapper classes
- Comprehensive coverage of market bot lifecycle (setup, execution, errors)
- Appropriate parameters for each event type
- Clean integration with the analytics framework
The event IDs (E27-E30) and parameter choices align well with business analytics needs for market bot feature tracking.
lib/bloc/bridge_form/bridge_bloc.dart (2)
521-575: Excellent analytics integration for swap lifecycle tracking.The comprehensive analytics logging covers all critical swap events:
- Swap initiation with chain and asset details
- Success tracking with amount and wallet type
- Failure tracking with error details
The implementation properly handles async wallet type retrieval and includes appropriate error handling. This provides valuable insights for swap performance and success rates.
269-272: Good defensive programming for unsupported assets.The filtering of unsupported coins like ARRR prevents downstream errors and aligns with the app configuration. The comment clearly explains the reasoning.
lib/analytics/events.dart (1)
1-181: Well-organized core analytics events with excellent utility function.The implementation demonstrates strong architectural design:
- Clear categorization: Events are logically grouped by functional area (HD Wallet, UI, Data Sync, Performance)
- Consistent patterns: All events follow the established data class + wrapper class pattern
- Comprehensive documentation: Each event includes business category and insights explanation
- Useful utility: The
parseDerivationPathfunction provides clean extraction of account/address indices for HD wallet analyticsThe event parameters are well-chosen for their respective analytics purposes, especially the performance metrics for UI responsiveness and data sync monitoring.
lib/analytics/events/transaction_events.dart (1)
1-286: Well-structured analytics event system for transactions.The implementation follows a clean, consistent pattern with data classes and event wrappers. The event naming, parameter structure, and business documentation are well-designed and will provide valuable insights for transaction funnel analysis.
lib/analytics/events/portfolio_events.dart (1)
1-295: Excellent consistency with transaction events architecture.This file maintains the same clean pattern as
transaction_events.dartwith well-documented business categories and consistent naming conventions. The portfolio events cover all key user interactions for asset management.lib/bloc/app_bloc_root.dart (3)
76-81: Approve SDK-centric constructor refactor.The migration from
runtimeUpdateConfigtoKomodoDefiSdkaligns well with the overall architectural changes in this PR and establishes a cleaner dependency pattern.
183-312: Comprehensive bloc migration to SDK architecture.The extensive updates to bloc initialization properly integrate the new KomodoDefiSdk, repository patterns, and analytics system. The migration maintains dependency injection while modernizing the architecture.
441-447: Well-implemented analytics event logging.The addition of
CoinsDataUpdatedEventDatawith timing and count metrics provides valuable telemetry for monitoring coin data updates.lib/analytics/analytics_factory.dart (1)
457-871: Event data classes follow good structure.The individual event data classes are well-structured with consistent naming and proper parameter mapping, even though the overall file architecture differs from other analytics event files.
lib/bloc/coins_manager/coins_manager_bloc.dart (1)
3-28: LGTM - Clean SDK integrationThe constructor and import updates properly integrate the new KomodoDefiSdk and AnalyticsBloc dependencies. The changes align well with the architectural migration described in the PR objectives.
assets/translations/en.json (5)
6-11: LGTM - Improved singular/plural translation keysThe separation of "asset" (singular) and "assets" (plural) provides better translation granularity. The addition of "lastTransactions" supports new UI features mentioned in the PR.
117-119: LGTM - Enhanced wallet import messagingThe updated wallet import messages properly clarify seed file handling and password creation. The new HD wallet error message aligns with the multi-address wallet support mentioned in the PR objectives.
Also applies to: 131-131, 205-205
407-415: LGTM - Comprehensive HD wallet mode supportThe new translation keys properly support the multi-address wallet functionality mentioned in the PR objectives. The messages clearly explain the implications and requirements of HD wallet mode.
583-592: LGTM - Complete fiat payment flow coverageThe new fiat payment translation keys provide comprehensive coverage for minimum/maximum amounts and all payment status states (success, failure, in-progress). This aligns with the enhanced fiat integration mentioned in the PR objectives.
623-636: LGTM - Custom token import supportThe new translation keys properly support the custom token import functionality mentioned in the PR objectives. The messaging includes appropriate warnings and clear instructions for token management.
| _analyticsRepo.queueEvent( | ||
| AppOpenedEventData( | ||
| platform: platform, | ||
| appVersion: appVersion, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using AnalyticsBloc instead of direct repository access.
The widget bypasses the AnalyticsBloc and calls the repository directly. This breaks the established architecture pattern where the bloc manages analytics events and could lead to inconsistent behavior.
Consider using the bloc extension for consistency:
- _analyticsRepo.queueEvent(
- AppOpenedEventData(
- platform: platform,
- appVersion: appVersion,
- ),
- );
+ // Get the analytics bloc and use the extension method
+ final analyticsBloc = context.read<AnalyticsBloc>();
+ analyticsBloc.logEvent(
+ AppOpenedEventData(
+ platform: platform,
+ appVersion: appVersion,
+ ),
+ );This approach maintains architectural consistency and ensures the event goes through the proper bloc pipeline.
📝 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.
| _analyticsRepo.queueEvent( | |
| AppOpenedEventData( | |
| platform: platform, | |
| appVersion: appVersion, | |
| ), | |
| ); | |
| // Get the analytics bloc and use the extension method | |
| final analyticsBloc = context.read<AnalyticsBloc>(); | |
| analyticsBloc.logEvent( | |
| AppOpenedEventData( | |
| platform: platform, | |
| appVersion: appVersion, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In lib/analytics/widgets/analytics_lifecycle_handler.dart around lines 81 to 86,
the code directly calls the analytics repository to queue an event, bypassing
the AnalyticsBloc. To maintain architectural consistency and ensure events are
processed correctly, refactor the code to dispatch the AppOpenedEventData event
through the AnalyticsBloc instead of calling the repository directly. Use the
bloc's event dispatching method or extension to send the event, aligning with
the established pattern.
| // Get the analytics repository from GetIt | ||
| try { | ||
| _analyticsRepo = GetIt.I<AnalyticsRepo>(); | ||
| } catch (e) { | ||
| log('AnalyticsLifecycleHandler: Failed to get AnalyticsRepo - $e'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix potential null reference after GetIt exception.
If GetIt.I<AnalyticsRepo>() throws an exception, _analyticsRepo remains uninitialized but the code later attempts to use it in _logAppOpenedEvent(). This will cause a LateInitializationError.
Consider this fix to handle the failure case:
/// Reference to the analytics repository
- late final AnalyticsRepo _analyticsRepo;
+ AnalyticsRepo? _analyticsRepo;
@override
void initState() {
super.initState();
// Get the analytics repository from GetIt
try {
_analyticsRepo = GetIt.I<AnalyticsRepo>();
} catch (e) {
log('AnalyticsLifecycleHandler: Failed to get AnalyticsRepo - $e');
+ return; // Early return if analytics repo is unavailable
}And update _logAppOpenedEvent():
void _logAppOpenedEvent() {
+ if (_analyticsRepo == null) return;
+
try {
final platform = PlatformInfo.getInstance().platform;
final appVersion = packageInformation.packageVersion ?? 'unknown';
- _analyticsRepo.queueEvent(
+ _analyticsRepo!.queueEvent(📝 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.
| // Get the analytics repository from GetIt | |
| try { | |
| _analyticsRepo = GetIt.I<AnalyticsRepo>(); | |
| } catch (e) { | |
| log('AnalyticsLifecycleHandler: Failed to get AnalyticsRepo - $e'); | |
| } | |
| /// Reference to the analytics repository | |
| - late final AnalyticsRepo _analyticsRepo; | |
| + AnalyticsRepo? _analyticsRepo; | |
| @override | |
| void initState() { | |
| super.initState(); | |
| // Get the analytics repository from GetIt | |
| try { | |
| _analyticsRepo = GetIt.I<AnalyticsRepo>(); | |
| } catch (e) { | |
| log('AnalyticsLifecycleHandler: Failed to get AnalyticsRepo - $e'); | |
| + return; // Early return if analytics repo is unavailable | |
| } | |
| WidgetsBinding.instance.addObserver(this); | |
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| _logAppOpenedEvent(); | |
| }); | |
| } | |
| void _logAppOpenedEvent() { | |
| + if (_analyticsRepo == null) return; | |
| try { | |
| final platform = PlatformInfo.getInstance().platform; | |
| final appVersion = packageInformation.packageVersion ?? 'unknown'; | |
| - _analyticsRepo.queueEvent( | |
| + _analyticsRepo!.queueEvent( | |
| AppOpenedEvent(platform: platform, appVersion: appVersion), | |
| ); | |
| } catch (e) { | |
| log('AnalyticsLifecycleHandler: Error logging app opened event - $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/analytics/widgets/analytics_lifecycle_handler.dart around lines 44 to 50,
the code catches exceptions when retrieving AnalyticsRepo from GetIt but does
not initialize _analyticsRepo afterward, leading to a potential
LateInitializationError when _logAppOpenedEvent() uses it. To fix this,
initialize _analyticsRepo to null or a safe default in the catch block and
update _logAppOpenedEvent() to check if _analyticsRepo is null before using it,
avoiding null reference errors.
| /// E21: Bridge completed | ||
| /// Business category: Cross-Chain. | ||
| class BridgeSucceededEventData implements AnalyticsEventData { | ||
| const BridgeSucceededEventData({ | ||
| required this.fromChain, | ||
| required this.toChain, | ||
| required this.asset, | ||
| required this.amount, | ||
| required this.walletType, | ||
| }); | ||
|
|
||
| final String fromChain; | ||
| final String toChain; | ||
| final String asset; | ||
| final double amount; | ||
| final String walletType; | ||
|
|
||
| @override | ||
| String get name => 'bridge_success'; | ||
|
|
||
| @override | ||
| JsonMap get parameters => { | ||
| 'from_chain': fromChain, | ||
| 'to_chain': toChain, | ||
| 'asset': asset, | ||
| 'amount': amount, | ||
| 'wallet_type': walletType, | ||
| }; | ||
| } | ||
|
|
||
| /// E21: Bridge completed | ||
| class AnalyticsBridgeSucceededEvent extends AnalyticsSendDataEvent { | ||
| AnalyticsBridgeSucceededEvent({ | ||
| required String fromChain, | ||
| required String toChain, | ||
| required String asset, | ||
| required double amount, | ||
| required String walletType, | ||
| }) : super( | ||
| BridgeSucceededEventData( | ||
| fromChain: fromChain, | ||
| toChain: toChain, | ||
| asset: asset, | ||
| amount: amount, | ||
| walletType: walletType, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider using Decimal instead of double for amount field.
Using double for financial amounts can lead to precision issues. Consider using Decimal type for accurate representation of cryptocurrency amounts.
- final double amount;
+ final Decimal amount;You'll also need to handle the serialization appropriately in the parameters map:
@override
JsonMap get parameters => {
'from_chain': fromChain,
'to_chain': toChain,
'asset': asset,
- 'amount': amount,
+ 'amount': amount.toDouble(), // or amount.toString() depending on backend requirements
'wallet_type': walletType,
};And update the constructor parameter:
AnalyticsBridgeSucceededEvent({
required String fromChain,
required String toChain,
required String asset,
- required double amount,
+ required Decimal amount,
required String walletType,
}) : super(Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/analytics/events/cross_chain_events.dart between lines 49 and 96, the
amount field is currently a double, which can cause precision errors for
financial data. Change the amount field type in BridgeSucceededEventData and the
constructor of AnalyticsBridgeSucceededEvent to Decimal. Update the parameters
getter to serialize the Decimal amount properly, for example by converting it to
a string. Ensure all references to amount use the Decimal type consistently.
| import 'package:web_dex/app_config/app_config.dart'; | ||
| import 'package:web_dex/bloc/analytics/analytics_bloc.dart'; | ||
| import 'package:web_dex/bloc/analytics/analytics_repo.dart'; | ||
| import 'package:web_dex/bloc/analytics/analytics_event.dart'; |
There was a problem hiding this comment.
Remove unused import to fix pipeline warning.
The pipeline indicates this import is unused and should be removed.
-import 'package:web_dex/bloc/analytics/analytics_event.dart';🧰 Tools
🪛 GitHub Actions: CharlVS is validating code guidelines 🚀
[warning] 17-17: Unused import: 'package:web_dex/bloc/analytics/analytics_event.dart' (unused_import)
🤖 Prompt for AI Agents
In lib/bloc/app_bloc_root.dart at line 17, the import statement for
'package:web_dex/bloc/analytics/analytics_event.dart' is unused and causing a
pipeline warning. Remove this import line entirely to resolve the warning and
clean up the code.
| class AnalyticsEvents { | ||
| const AnalyticsEvents._(); | ||
|
|
||
| /// Portfolio P&L viewed event | ||
| static PortfolioPnlViewedEvent portfolioPnlViewed({ | ||
| required String timeframe, | ||
| required double realizedPnl, | ||
| required double unrealizedPnl, | ||
| }) { | ||
| return PortfolioPnlViewedEvent( | ||
| timeframe: timeframe, | ||
| realizedPnl: realizedPnl, | ||
| unrealizedPnl: unrealizedPnl, | ||
| ); | ||
| } | ||
|
|
||
| /// App opened / foregrounded event | ||
| static AppOpenedEvent appOpened({ | ||
| required String platform, | ||
| required String appVersion, | ||
| }) { | ||
| return AppOpenedEvent(platform: platform, appVersion: appVersion); | ||
| } | ||
|
|
||
| /// Onboarding started event | ||
| static OnboardingStartedEvent onboardingStarted({ | ||
| required String method, | ||
| String? referralSource, | ||
| }) { | ||
| return OnboardingStartedEvent( | ||
| method: method, | ||
| referralSource: referralSource, | ||
| ); | ||
| } | ||
|
|
||
| /// Wallet created event | ||
| static WalletCreatedEvent walletCreated({ | ||
| required String source, | ||
| required String walletType, | ||
| }) { | ||
| return WalletCreatedEvent(source: source, walletType: walletType); | ||
| } | ||
|
|
||
| /// Wallet imported event | ||
| static WalletImportedEvent walletImported({ | ||
| required String source, | ||
| required String importType, | ||
| required String walletType, | ||
| }) { | ||
| return WalletImportedEvent( | ||
| source: source, | ||
| importType: importType, | ||
| walletType: walletType, | ||
| ); | ||
| } | ||
|
|
||
| /// Seed backup completed event | ||
| static BackupCompletedEvent backupCompleted({ | ||
| required int backupTime, | ||
| required String method, | ||
| required String walletType, | ||
| }) { | ||
| return BackupCompletedEvent( | ||
| backupTime: backupTime, | ||
| method: method, | ||
| walletType: walletType, | ||
| ); | ||
| } | ||
|
|
||
| /// Backup skipped event | ||
| static BackupSkippedEvent backupSkipped({ | ||
| required String stageSkipped, | ||
| required String walletType, | ||
| }) { | ||
| return BackupSkippedEvent( | ||
| stageSkipped: stageSkipped, | ||
| walletType: walletType, | ||
| ); | ||
| } | ||
|
|
||
| /// Bridge initiated event | ||
| static BridgeInitiatedEvent bridgeInitiated({ | ||
| required String fromChain, | ||
| required String toChain, | ||
| required String asset, | ||
| }) { | ||
| return BridgeInitiatedEvent( | ||
| fromChain: fromChain, | ||
| toChain: toChain, | ||
| asset: asset, | ||
| ); | ||
| } | ||
|
|
||
| /// Bridge success event | ||
| static BridgeSuccessEvent bridgeSuccess({ | ||
| required String fromChain, | ||
| required String toChain, | ||
| required String asset, | ||
| required double amount, | ||
| }) { | ||
| return BridgeSuccessEvent( | ||
| fromChain: fromChain, | ||
| toChain: toChain, | ||
| asset: asset, | ||
| amount: amount, | ||
| ); | ||
| } | ||
|
|
||
| /// Bridge failure event | ||
| static BridgeFailureEvent bridgeFailure({ | ||
| required String fromChain, | ||
| required String toChain, | ||
| required String failError, | ||
| }) { | ||
| return BridgeFailureEvent( | ||
| fromChain: fromChain, | ||
| toChain: toChain, | ||
| failError: failError, | ||
| ); | ||
| } | ||
|
|
||
| /// NFT gallery opened event | ||
| static NftGalleryOpenedEvent nftGalleryOpened({ | ||
| required int nftCount, | ||
| required int loadTimeMs, | ||
| }) { | ||
| return NftGalleryOpenedEvent( | ||
| nftCount: nftCount, | ||
| loadTimeMs: loadTimeMs, | ||
| ); | ||
| } | ||
|
|
||
| /// NFT transfer initiated | ||
| static NftTransferInitiatedEvent nftTransferInitiated({ | ||
| required String collectionName, | ||
| required String tokenId, | ||
| required String hdType, | ||
| }) { | ||
| return NftTransferInitiatedEvent( | ||
| collectionName: collectionName, | ||
| tokenId: tokenId, | ||
| hdType: hdType, | ||
| ); | ||
| } | ||
|
|
||
| /// NFT transfer success | ||
| static NftTransferSuccessEvent nftTransferSuccess({ | ||
| required String collectionName, | ||
| required String tokenId, | ||
| required double fee, | ||
| required String hdType, | ||
| }) { | ||
| return NftTransferSuccessEvent( | ||
| collectionName: collectionName, | ||
| tokenId: tokenId, | ||
| fee: fee, | ||
| hdType: hdType, | ||
| ); | ||
| } | ||
|
|
||
| /// NFT transfer failure | ||
| static NftTransferFailureEvent nftTransferFailure({ | ||
| required String collectionName, | ||
| required String failReason, | ||
| required String hdType, | ||
| }) { | ||
| return NftTransferFailureEvent( | ||
| collectionName: collectionName, | ||
| failReason: failReason, | ||
| hdType: hdType, | ||
| ); | ||
| } | ||
|
|
||
| /// Marketbot setup started | ||
| static MarketbotSetupStartEvent marketbotSetupStart({ | ||
| required String strategyType, | ||
| required int pairsCount, | ||
| }) { | ||
| return MarketbotSetupStartEvent( | ||
| strategyType: strategyType, | ||
| pairsCount: pairsCount, | ||
| ); | ||
| } | ||
|
|
||
| /// Marketbot setup complete | ||
| static MarketbotSetupCompleteEvent marketbotSetupComplete({ | ||
| required String strategyType, | ||
| required double baseCapital, | ||
| }) { | ||
| return MarketbotSetupCompleteEvent( | ||
| strategyType: strategyType, | ||
| baseCapital: baseCapital, | ||
| ); | ||
| } | ||
|
|
||
| /// Marketbot trade executed | ||
| static MarketbotTradeExecutedEvent marketbotTradeExecuted({ | ||
| required String pair, | ||
| required double tradeSize, | ||
| required double profitUsd, | ||
| }) { | ||
| return MarketbotTradeExecutedEvent( | ||
| pair: pair, | ||
| tradeSize: tradeSize, | ||
| profitUsd: profitUsd, | ||
| ); | ||
| } | ||
|
|
||
| /// Marketbot error | ||
| static MarketbotErrorEvent marketbotError({ | ||
| required String errorCode, | ||
| required String strategyType, | ||
| }) { | ||
| return MarketbotErrorEvent( | ||
| errorCode: errorCode, | ||
| strategyType: strategyType, | ||
| ); | ||
| } | ||
|
|
||
| /// Reward claim initiated | ||
| static RewardClaimInitiatedEvent rewardClaimInitiated({ | ||
| required String asset, | ||
| required double expectedRewardAmount, | ||
| }) { | ||
| return RewardClaimInitiatedEvent( | ||
| asset: asset, | ||
| expectedRewardAmount: expectedRewardAmount, | ||
| ); | ||
| } | ||
|
|
||
| /// Reward claim success | ||
| static RewardClaimSuccessEvent rewardClaimSuccess({ | ||
| required String asset, | ||
| required double rewardAmount, | ||
| }) { | ||
| return RewardClaimSuccessEvent( | ||
| asset: asset, | ||
| rewardAmount: rewardAmount, | ||
| ); | ||
| } | ||
|
|
||
| /// Reward claim failure | ||
| static RewardClaimFailureEvent rewardClaimFailure({ | ||
| required String asset, | ||
| required String failReason, | ||
| }) { | ||
| return RewardClaimFailureEvent( | ||
| asset: asset, | ||
| failReason: failReason, | ||
| ); | ||
| } | ||
|
|
||
| /// DApp connected | ||
| static DappConnectEvent dappConnect({ | ||
| required String dappName, | ||
| required String network, | ||
| }) { | ||
| return DappConnectEvent( | ||
| dappName: dappName, | ||
| network: network, | ||
| ); | ||
| } | ||
|
|
||
| /// Settings change | ||
| static SettingsChangeEvent settingsChange({ | ||
| required String settingName, | ||
| required String newValue, | ||
| }) { | ||
| return SettingsChangeEvent( | ||
| settingName: settingName, | ||
| newValue: newValue, | ||
| ); | ||
| } | ||
|
|
||
| /// Error displayed | ||
| static ErrorDisplayedEvent errorDisplayed({ | ||
| required String errorCode, | ||
| required String screenContext, | ||
| }) { | ||
| return ErrorDisplayedEvent( | ||
| errorCode: errorCode, | ||
| screenContext: screenContext, | ||
| ); | ||
| } | ||
|
|
||
| /// App shared | ||
| static AppShareEvent appShare({ | ||
| required String channel, | ||
| }) { | ||
| return AppShareEvent(channel: channel); | ||
| } | ||
|
|
||
| /// Scroll attempt outside content | ||
| static ScrollAttemptOutsideContentEvent scrollAttemptOutsideContent({ | ||
| required String screenContext, | ||
| required double scrollDelta, | ||
| }) { | ||
| return ScrollAttemptOutsideContentEvent( | ||
| screenContext: screenContext, | ||
| scrollDelta: scrollDelta, | ||
| ); | ||
| } | ||
|
|
||
| /// Searchbar input | ||
| static SearchbarInputEvent searchbarInput({ | ||
| required int queryLength, | ||
| String? assetSymbol, | ||
| }) { | ||
| return SearchbarInputEvent( | ||
| queryLength: queryLength, | ||
| assetSymbol: assetSymbol, | ||
| ); | ||
| } | ||
|
|
||
| /// Theme selected | ||
| static ThemeSelectedEvent themeSelected({ | ||
| required String themeName, | ||
| }) { | ||
| return ThemeSelectedEvent(themeName: themeName); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider unifying analytics event patterns across the codebase.
This file uses a factory method pattern with AnalyticsEvents class, while transaction_events.dart and portfolio_events.dart use wrapper classes extending AnalyticsSendDataEvent. This inconsistency could lead to confusion and maintenance issues.
Consider either:
- Converting this file to use the wrapper class pattern for consistency
- Or updating the other files to use the factory pattern
🤖 Prompt for AI Agents
In lib/analytics/analytics_factory.dart from lines 135 to 455, the
AnalyticsEvents class uses a factory method pattern to create event instances,
which is inconsistent with transaction_events.dart and portfolio_events.dart
that use wrapper classes extending AnalyticsSendDataEvent. To fix this, choose
one pattern for all analytics event definitions: either refactor this file to
use wrapper classes extending AnalyticsSendDataEvent like the others, or
refactor the other files to adopt the factory method pattern used here. This
unification will improve code consistency and maintainability.
| class PortfolioPnlViewedEvent extends AnalyticsEventData { | ||
| PortfolioPnlViewedEvent({ | ||
| required this.timeframe, | ||
| required this.realizedPnl, | ||
| required this.unrealizedPnl, | ||
| }); | ||
|
|
||
| @override | ||
| String get name => 'portfolio_pnl_viewed'; | ||
|
|
||
| final String timeframe; | ||
| final double realizedPnl; | ||
| final double unrealizedPnl; | ||
|
|
||
| @override | ||
| Map<String, Object> get parameters => { | ||
| 'timeframe': timeframe, | ||
| 'realized_pnl': realizedPnl, | ||
| 'unrealized_pnl': unrealizedPnl, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent return type in parameters getter.
The PortfolioPnlViewedEvent uses Map<String, Object> while other events in the file use JsonMap. This should be consistent.
@override
- Map<String, Object> get parameters => {
+ JsonMap get parameters => {🤖 Prompt for AI Agents
In lib/analytics/analytics_factory.dart between lines 5 and 25, the parameters
getter in PortfolioPnlViewedEvent returns Map<String, Object> which is
inconsistent with other events that use JsonMap. Change the return type of the
parameters getter to JsonMap and ensure the returned map matches the expected
JsonMap type for consistency across the file.
* docs: Implement security advisory and changelog template * docs: add v0.9.0 release notes and blog post * docs: expand v0.9.0 changelog
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
AGENTS.md (5)
3-3: Use American English for consistency
Change “afterwards” to “afterward” to align with American English usage.- ... dependencies pre-fetched during setup, but network access is disabled afterwards. ... + ... dependencies pre-fetched during setup, but network access is disabled afterward. ...🧰 Tools
🪛 LanguageTool
[locale-violation] ~3-~3: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...g setup, but network access is disabled afterwards. Use the commands below to work with th...(AFTERWARDS_US)
25-25: Add missing period and refine sentence
The sentence ends abruptly; add a period and consider clarifying it.- Unit tests and integration tests are currently failing. Instead of running tests to validate fixes, do a thorough code review of the changes and static analysis + Unit tests and integration tests are currently failing. Instead of running tests to validate fixes, thoroughly review the code changes and rely on static analysis.
31-31: Standardize spelling to American English
Change “Familiarise” to “Familiarize” for consistency.- Familiarise yourself with the BLoC conventions and Conventional Commits standards included at the end of this document. + Familiarize yourself with the BLoC conventions and Conventional Commits standards included at the end of this document.
41-41: Adjust heading level for consistency
Use a second-level heading (##) for “Komodo DeFi Flutter SDK” to match other section levels.- # Komodo DeFi Flutter SDK + ## Komodo DeFi Flutter SDK
45-45: Refine wording for clarity
Consider replacing “make up” with a more precise verb, e.g., “comprise” or “constitute.”- The repository consists of a suite of packages (in the `packages` directory) which make up a Flutter SDK package `komodo_defi_sdk` used for implementing Komodo DeFi into Flutter apps. + The repository comprises a suite of packages in the `packages` directory that constitute the `komodo_defi_sdk` Flutter package for implementing Komodo DeFi into Flutter apps.🧰 Tools
🪛 LanguageTool
[style] ~45-~45: Try using a synonym here to elevate your writing.
Context: ...ges (in thepackagesdirectory) which make up a Flutter SDK packagekomodo_defi_sdk...(CONSTITUTE_COMPRISE)
docs/RELEASE_NOTES_TEMPLATE.md (4)
1-9: Consider using YAML frontmatter for metadata
The top-of-file metadata (Tags, Application, Platform, Created, Updated, Version) could be expressed as standard YAML frontmatter for clearer semantics and better support by static site generators. For example:--- tags: [release-notes, changelog, documentation, versioning] application: Komodo Wallet platform: Flutter (Web, Desktop, Mobile) created: 2025-05-19 updated: 2025-05-19 version: 1.0 ---
20-24: Enhance input variables section with a table
Turning the list of input variables into a markdown table can improve readability and maintain consistency with the rest of the docs. For example:| Variable | Description | |-----------------------------|------------------------------------------------------------------------| | **{VERSION_NUMBER}** | The version being released (e.g., "1.0.0") | | **{PREVIOUS_RELEASE_NOTES}**| Example of previous release notes to match style and format | | **{CHANGELOG_CONTENT}** | Raw changelog entries or commit messages to be organized | | **{COMMITS_WITH_AUTHORS}** | List of commits with author information for attribution | | **{REFERENCE_VERSION}** | A reference version with particularly good release notes format (e.g., "v0.9.0") |
64-64: Convert placeholder emphases to section headings
Using bold emphasis (**...**) for placeholder markers can be confusing and triggers markdownlint’s MD036. Consider replacing these with subheadings, e.g.:### Previous Release Notes {PREVIOUS_RELEASE_NOTES} ### Changelog Content {CHANGELOG_CONTENT} ### Commits with Authors {COMMITS_WITH_AUTHORS}Also applies to: 68-68, 72-72
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
64-64: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
155-160: Replace empty or placeholder links with real URLs or clear placeholders
Lines like[migration guide](#),[user guide](#), and[API documentation](#)use#as the href, which may trigger markdownlint’s MD042 “No empty links.” Either provide actual URLs or replace#with a clear placeholder such as<URL>orTBD.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
155-155: No empty links
null(MD042, no-empty-links)
159-159: No empty links
null(MD042, no-empty-links)
160-160: No empty links
null(MD042, no-empty-links)
docs/BUILD_SECURITY_ADVISORY.md (3)
17-17: Nitpick: Add comma for clarity.
Consider adding a comma after “pubspec.lock file” for improved readability:This flag ensures that dependencies are installed exactly as specified in the `pubspec.lock` file, with no automatic updates.🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...ly** as specified in thepubspec.lockfile with no automatic updates. **Why this ...(AI_HYDRA_LEO_MISSING_COMMA)
8-11: Optional: Clarify flag usage in context.
Listing standalone flags can be ambiguous. To make this section self-contained, you might show the full commands here:- --enforce-lockfile # When running 'flutter pub get' - --no-pub # When running 'flutter build' - --no-web-resources-cdn # When building for web + flutter pub get --enforce-lockfile + flutter build apk --no-pub + flutter build web --no-web-resources-cdn
41-44: Nitpick: Improve phrasing for regulatory context.
The phrase “operate under regulations restricting external resource loading” is a bit wordy. Consider tightening it, for example:- **Regulatory restrictions**: Many financial applications must bundle resources locally due to regulatory requirements.🧰 Tools
🪛 LanguageTool
[style] ~44-~44: ‘under regulations’ might be wordy. Consider a shorter alternative.
Context: ...**: Many financial applications operate under regulations restricting external resource loading. ...(EN_WORDINESS_PREMIUM_UNDER_REGULATIONS)
CHANGELOG.md (3)
28-28: Nitpick: Add definite article.
In the UI/UX section, consider adding “the” for smoother reading:- improving experience for international users. + improving the experience for international users.🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: Possible missing article found.
Context: ...ore text is now translatable, improving experience for international users. - **Add Assets...(AI_HYDRA_LEO_MISSING_THE)
30-31: Optional: Include PR/issue reference.
The Wallet Only Logout Confirmation entry currently has no PR or issue number. Please add the appropriate reference (e.g.,[@naezith], #<issue>).
75-80: Optional: Highlight new documentation.
Since you’ve introduceddocs/BUILD_SECURITY_ADVISORY.md, consider adding a 📖 Documentation section to this changelog:## 📖 Documentation - **Build Security Advisory**: Added security guidelines for enforcing lockfiles and build flags (`docs/BUILD_SECURITY_ADVISORY.md`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.md(1 hunks)CHANGELOG.md(1 hunks)docs/BUILD_RELEASE.md(4 hunks)docs/BUILD_SECURITY_ADVISORY.md(1 hunks)docs/RELEASE_NOTES_TEMPLATE.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/BUILD_RELEASE.md
🧰 Additional context used
🪛 LanguageTool
docs/BUILD_SECURITY_ADVISORY.md
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...ly** as specified in the pubspec.lock file with no automatic updates. **Why this ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ... for engine resources when building for web. Why this matters: - **Eliminates...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~44-~44: ‘under regulations’ might be wordy. Consider a shorter alternative.
Context: ...**: Many financial applications operate under regulations restricting external resource loading. ...
(EN_WORDINESS_PREMIUM_UNDER_REGULATIONS)
CHANGELOG.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...for enhanced security and features. - HD Withdrawals & Portfolio Overview ([@C...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~28-~28: Possible missing article found.
Context: ...ore text is now translatable, improving experience for international users. - **Add Assets...
(AI_HYDRA_LEO_MISSING_THE)
[grammar] ~49-~49: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...VS]) - Resolved display issues with the receive payment button. - **Coin Balance Calcul...
(PREPOSITION_VERB)
AGENTS.md
[locale-violation] ~3-~3: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...g setup, but network access is disabled afterwards. Use the commands below to work with th...
(AFTERWARDS_US)
[style] ~45-~45: Try using a synonym here to elevate your writing.
Context: ...ges (in the packages directory) which make up a Flutter SDK package komodo_defi_sdk...
(CONSTITUTE_COMPRISE)
docs/RELEASE_NOTES_TEMPLATE.md
[uncategorized] ~49-~49: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...egorized - Changes are described clearly and concisely - Proper attribution is given...
(COMMA_COMPOUND_SENTENCE_2)
🪛 markdownlint-cli2 (0.17.2)
docs/RELEASE_NOTES_TEMPLATE.md
64-64: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
72-72: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
155-155: No empty links
null
(MD042, no-empty-links)
159-159: No empty links
null
(MD042, no-empty-links)
160-160: No empty links
null
(MD042, no-empty-links)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (1)
AGENTS.md (1)
37-37: Verify documentation path
Ensure thatdocs/BUILD_RUN_APP.mdexists and the path matches the repository structure.
…2620) * fix(auth): wallet password validation in creation and update flows * feat(auth): allow weak password toggle in settings * fix(password-validator): use Characters.length for better unicode support * chore(deps): bump SDK hash 9519157 * fix(auth): revert fallback to legacy validator do not perform any validation if allow_weak_passwords is toggled
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
assets/translations/en.json (1)
413-413: Inconsistent withdrawal preview key naming.You have both
"withdrawPreview"(line 413) and"previewWithdrawal"(line 671). This duplication can cause confusion and inconsistent UI text.Consider standardizing on one naming convention. Since
"withdrawPreview"appears first and follows the pattern of other keys, consider removing the duplicate:- "previewWithdrawal": "Preview Withdrawal",Or if both are needed for different contexts, ensure they have distinct, descriptive names.
Also applies to: 671-671
🧹 Nitpick comments (2)
assets/translations/en.json (2)
129-129: Password validation message could be more user-friendly.The password format error message is quite long and technical. Consider breaking it into smaller, more digestible validation messages that can be shown individually.
The current approach with individual password validation messages (lines 369-376) is much better for UX. Consider using those individual messages instead of the comprehensive one on line 129.
347-347: Alpha version warning could be more concise.The alpha version warning is quite lengthy and might overwhelm users. Consider shortening it while maintaining the essential legal disclaimers.
Break the warning into smaller, more digestible paragraphs or consider highlighting the key risks more prominently.
📜 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 (2)
assets/translations/en.json(16 hunks)lib/bloc/auth_bloc/auth_bloc.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Test web-app-macos
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (2)
lib/bloc/auth_bloc/auth_bloc.dart (1)
67-76:✅ Verification successful
Verify legacy wallet detection logic and security implications.
The legacy wallet detection and seed extraction logic needs careful consideration:
- The
isLegacyWalletproperty should be thoroughly validatedgetLegacySeed()method exposes sensitive seed data - ensure proper security measures
🏁 Script executed:
#!/bin/bash # Description: Verify legacy wallet handling and seed security across the codebase # Search for isLegacyWallet property usage and implementation ast-grep --pattern 'isLegacyWallet' # Search for getLegacySeed method implementation and usage ast-grep --pattern 'getLegacySeed($_)' # Check for any security measures around seed handling rg -A 5 -B 5 "getLegacySeed|isLegacyWallet"Length of output: 5970
Legacy wallet detection and seed handling are implemented correctly
isLegacyWalletis a straightforward getter onconfig.isLegacyWallet, which is reliably set inwallets_repositorywhen loading legacy wallets.getLegacySeed(password)usesEncryptionTool().decryptDatato decrypt the stored seed phrase. No seeds are logged or persisted in plaintext.- AuthBloc branches on
isLegacyWalletand passes the seed only to the restoration flow, then immediately deletes the legacy wallet to avoid duplication.No changes required in
AuthBloc—ensure that the underlyingEncryptionToolemploys a strong KDF and encryption algorithm, and audit it if necessary.assets/translations/en.json (1)
669-669:✅ Verification successful
Potential duplicate "transactionSuccessful" usage.
While the past review comments indicate the duplicate
transactionSuccessfulkeys were addressed, line 669 shows this key again. Verify this isn't a reintroduction of the duplicate.Also applies to: 51-51
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining duplicate keys in the translation file # Search for duplicate keys in the JSON file cat assets/translations/en.json | jq -r 'keys[]' | sort | uniq -d # Search specifically for transactionSuccessful occurrences rg -n "transactionSuccessful" assets/translations/en.jsonLength of output: 184
No duplicate “transactionSuccessful” key found
Verified with
jqandrgthattransactionSuccessfulappears only once inassets/translations/en.json(line 669), and no duplicate keys exist. No further action required.
| // Delete legacy wallet on successful restoration & login to avoid | ||
| // duplicates in the wallet list | ||
| if (event.wallet.isLegacyWallet) { | ||
| await _kdfSdk.addActivatedCoins(event.wallet.config.activatedCoins); | ||
| await _walletsRepository.deleteWallet(event.wallet); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Legacy wallet deletion after restoration could cause data loss.
Deleting the legacy wallet immediately after successful restoration might be premature. If there are any issues with the SDK wallet or coin activation, the user could lose access to their funds.
Consider a safer approach:
- // Delete legacy wallet on successful restoration & login to avoid
- // duplicates in the wallet list
- if (event.wallet.isLegacyWallet) {
- await _kdfSdk.addActivatedCoins(event.wallet.config.activatedCoins);
- await _walletsRepository.deleteWallet(event.wallet);
- }
+ // Mark legacy wallet for deletion after successful SDK integration
+ if (event.wallet.isLegacyWallet) {
+ await _kdfSdk.addActivatedCoins(event.wallet.config.activatedCoins);
+ // Consider marking as migrated rather than immediate deletion
+ // await _walletsRepository.markWalletMigrated(event.wallet);
+ }📝 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.
| // Delete legacy wallet on successful restoration & login to avoid | |
| // duplicates in the wallet list | |
| if (event.wallet.isLegacyWallet) { | |
| await _kdfSdk.addActivatedCoins(event.wallet.config.activatedCoins); | |
| await _walletsRepository.deleteWallet(event.wallet); | |
| } | |
| // Mark legacy wallet for deletion after successful SDK integration | |
| if (event.wallet.isLegacyWallet) { | |
| await _kdfSdk.addActivatedCoins(event.wallet.config.activatedCoins); | |
| // Consider marking as migrated rather than immediate deletion | |
| // await _walletsRepository.markWalletMigrated(event.wallet); | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/auth_bloc/auth_bloc.dart around lines 224 to 229, avoid deleting the
legacy wallet immediately after restoration to prevent potential data loss if
SDK wallet or coin activation fails. Instead, implement a verification step to
confirm successful coin activation and wallet functionality before deleting the
legacy wallet. This ensures the user retains access to their funds until the new
wallet is fully operational.
| void _listenToAuthStateChanges() { | ||
| _authChangesSubscription?.cancel(); | ||
| _authChangesSubscription = _kdfSdk.auth.authStateChanges.listen((user) { | ||
| final AuthorizeMode event = | ||
| user != null ? AuthorizeMode.logIn : AuthorizeMode.noLogin; | ||
| add(AuthModeChanged(mode: event, currentUser: user)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling in auth state listener.
The _listenToAuthStateChanges method doesn't handle potential errors in the stream, which could cause the app to miss important auth state changes.
Add error handling to the stream subscription:
void _listenToAuthStateChanges() {
_authChangesSubscription?.cancel();
_authChangesSubscription = _kdfSdk.auth.authStateChanges.listen(
(user) {
final AuthorizeMode event =
user != null ? AuthorizeMode.logIn : AuthorizeMode.noLogin;
add(AuthModeChanged(mode: event, currentUser: user));
},
+ onError: (error, stackTrace) {
+ _log.shout('Error in auth state changes stream', error, stackTrace);
+ // Consider emitting an error state or attempting to reconnect
+ },
);
}📝 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.
| void _listenToAuthStateChanges() { | |
| _authChangesSubscription?.cancel(); | |
| _authChangesSubscription = _kdfSdk.auth.authStateChanges.listen((user) { | |
| final AuthorizeMode event = | |
| user != null ? AuthorizeMode.logIn : AuthorizeMode.noLogin; | |
| add(AuthModeChanged(mode: event, currentUser: user)); | |
| }); | |
| } | |
| void _listenToAuthStateChanges() { | |
| _authChangesSubscription?.cancel(); | |
| _authChangesSubscription = _kdfSdk.auth.authStateChanges.listen( | |
| (user) { | |
| final AuthorizeMode event = | |
| user != null ? AuthorizeMode.logIn : AuthorizeMode.noLogin; | |
| add(AuthModeChanged(mode: event, currentUser: user)); | |
| }, | |
| onError: (error, stackTrace) { | |
| _log.shout('Error in auth state changes stream', error, stackTrace); | |
| // Consider emitting an error state or attempting to reconnect | |
| }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/bloc/auth_bloc/auth_bloc.dart around lines 295 to 302, the
_listenToAuthStateChanges method subscribes to the authStateChanges stream but
lacks error handling for the stream. Modify the listen call to include an
onError callback that handles errors appropriately, such as logging the error or
dispatching an error event, to ensure the app can respond to stream errors and
maintain reliable auth state updates.
| await _kdfSdk.auth.register( | ||
| password: event.password, | ||
| walletName: event.wallet.name, | ||
| options: AuthOptions( | ||
| derivationMethod: event.wallet.config.type == WalletType.hdwallet | ||
| ? DerivationMethod.hdWallet | ||
| : DerivationMethod.iguana, | ||
| allowWeakPassword: weakPasswordsAllowed, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication between register and restore methods.
Both _onRegister and _onRestore have nearly identical SDK registration calls with similar error handling patterns.
Extract the common registration logic into a helper method:
+ Future<void> _performRegistration({
+ required String password,
+ required String walletName,
+ required WalletType walletType,
+ Mnemonic? mnemonic,
+ }) async {
+ final weakPasswordsAllowed = await _areWeakPasswordsAllowed();
+
+ await _kdfSdk.auth.register(
+ password: password,
+ walletName: walletName,
+ mnemonic: mnemonic,
+ options: AuthOptions(
+ derivationMethod: walletType == WalletType.hdwallet
+ ? DerivationMethod.hdWallet
+ : DerivationMethod.iguana,
+ allowWeakPassword: weakPasswordsAllowed,
+ ),
+ );
+ }Then use this helper in both register and restore methods to reduce duplication.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/bloc/auth_bloc/auth_bloc.dart around lines 153 to 162, the registration
logic in _onRegister and _onRestore methods is duplicated. Extract the common
SDK registration call and its error handling into a separate private helper
method that accepts necessary parameters like password, wallet name, and
options. Then replace the duplicated code in both _onRegister and _onRestore
with calls to this new helper method to reduce redundancy and improve
maintainability.
| emit(AuthBlocState.loading()); | ||
| if (await _didSignInExistingWallet(event.wallet, event.password)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential infinite recursion in wallet registration.
The _didSignInExistingWallet method dispatches AuthSignInRequested event, which could create an infinite loop if the sign-in process repeatedly fails and triggers registration again.
Consider adding a flag or counter to prevent infinite recursion:
- if (await _didSignInExistingWallet(event.wallet, event.password)) {
- return;
- }
+ if (await _didSignInExistingWallet(event.wallet, event.password)) {
+ _log.info('Existing wallet found, sign-in attempt dispatched');
+ return;
+ }Also ensure _didSignInExistingWallet has proper error handling to avoid recursive registration attempts.
🤖 Prompt for AI Agents
In lib/bloc/auth_bloc/auth_bloc.dart around lines 144 to 147, the call to
_didSignInExistingWallet can cause infinite recursion because it dispatches
AuthSignInRequested, potentially triggering repeated sign-in attempts and
registration. To fix this, introduce a flag or counter to track the number of
sign-in attempts and prevent repeated retries beyond a safe limit. Additionally,
add proper error handling in _didSignInExistingWallet to catch failures and
avoid triggering recursive registration attempts.
@ca333 Acknowledged. I've started with some changes needed in #2642 (localisations) and #2675 (JS). Will look into the rest as well. Some of those packages relate to storage, which we can refactor to KDF. I'll handle all the low-hanging items in time for v0.9.1 RC and then give you feedback on what it will take for the remainder. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Komodo Wallet v0.9.0 Release
We are excited to announce Komodo Wallet v0.9.0. This release introduces HD wallet functionality, cross-platform fiat on-ramp improvements, a new feedback provider, and numerous bug fixes and dependency upgrades.
Under the hood, the app has undergone a major rewrite to migrate to our new KDF Flutter SDK. This allows developers to quickly and easily build their own DeFi applications in hours instead of months. See the SDK package for more information.
The codebase is now unified across all web, desktop, and mobile platforms. This change allows for more consistent development and easier maintenance.
🚀 New Features
🎨 UI/UX Improvements
⚡ Performance Enhancements
🐛 Bug Fixes
🔒 Security Updates
💻 Platform-specific Changes
iOS & macOS
Web/Desktop/Mobile
Full Changelog: 0.8.3...0.9.0
Changelog
TODO
KDF SDK
This app migrates most of our codebase to the new KDF Flutter SDK, which showcases how even hobbyist developers can build their own GUI apps using our simple tools and avoiding the months of headache and time invested in starting from scratch.
Due to this dual benefit, we appreciate your patience with any teething issues or regressions of existing features.
OPSEC
Dependencies Diff Summary
See the generated diff summary in Google Drive for more info and a patch of the code changes.
The directory includes the following:
The diff summary is generated using our diff-summary-generator.
Package Dependencies Summary (Click to Expand/Collapse)
QA
cc @KomodoPlatform/qa
While HD support is the most notable user-facing change, this is an entire rewrite "under the hood", so all features are affected. Please test widely. The follow-up release will focus on any major issues identified after release as well as general performance optimisations (especially those which have regressed since pre-SDK migration).
Please also note that any issues already reported which are not patched will be addressed in a future update.
Auto-reviews:
NB: The CodeRabbit summary below is incomplete and only considers the incremental changes between the commit when the branch was updated and the current commit because it exceeded the max length when opening the PR. In the future, we will always keep a PR open for
dev->mainto avoid this limit.Summary by CodeRabbit
New Features
CustomTokenImportBlocand repository interface.CoinAddressesBlocfor managing coin addresses.FiatFormBlocfor fiat on-ramp form management.FaucetBlocto manage faucet requests.Improvements
KomodoDefiSdkandCoinsRepofor improved architecture and type safety.ThemeExtensionfor custom dark and light themes.WithdrawFormBloc,MarketMakerBotBloc, andNftWithdrawBloc.BanxaFiatProvider,RampFiatProvider) with stronger typing and error handling.CacheMissExceptionfor portfolio growth repository.CoinsBlocand related repositories.KomodoDefiSdkacross multiple blocs.TextThemeExtensionin the theme.Fixes