Major codebase cleanup #415

mocodesmo · 2025-01-05T11:21:01Z

INFO: further clean up, rearranging and renaming will be done after this PR is merged

Removed all/most bloc-to-bloc communications

removed passing walletblocs via routes
removed walletbloc property in blocstate
removed passing currencycubit into blocs
removed passing networkcubit into blocs

WALLET SERVICE

all wallet operations now handled by walletservice
exposes a stream of walletdata which is listened to by walletbloc

APP WALLET REPOSITORY

main repository that maintains all walletservices (which contains each wallet)
handles loading wallets from storage
handles getters for wallet/walletservice retrieval

NETWORK REPOSITORY

handles network connections and updates
exposes a stream of networkrepodata which is listened to by networkkbloc

APP WALLETS BLOC

created in locator on appstart
handles the list of walletblocs which is used for retrieval across the app

…t for wallet services

kumulynja

Some general remarks:

Good first chunk of refactoring to remove some Cubit in Cubit’s already and start with repository classes.
But I think the scope of the current repositories is still too big and should be limited as much as possible to just storing and retrieving data, for example the NetworkRepository now also creates a blockchain object for the wallet. I think a cleaner flow would be: NetworkRepository is only responsible for storing and retrieving the settings concerning the network, then a WalletRepository or WalletService is responsible for creating the blockchain instance it needs. Both the NetworkRepository as the WalletRepository/Service can be injected into the BloC or Service that needs to initialise the wallets. In that BLoC, the NetworkRepository can then be used to retrieve the current network settings from local storage and these can then be passed to the WalletRepository/Service to initialise it with those correct settings. This way they are both decoupled from each other and the BLoCs or Services can use both. Now having the creation of the bdk Blockchain instance in the NetworkRepository is too tightly coupled code. In general I think we should improve by keeping the repositories as limited as possible to just storing and retrieving data. Initializing things or using this data should move to higher layers like Services for global reusable things or to BLoCs for feature specific logic.
If the scope of repositories is more bounded, we can organise the architecture a lot better in different layers and make sure there is a clear pattern and consistency in responsibilities of every layer, what is injected into what and how data and calls flow between them. For example, normally repositories get injected to services so that the services can build functionality that needs data from different repositories, but the AppWalletsRepository keeps a list of WalletServices, and the repository creates the services, that’s kind of the other way around. If we keep a more consistent pattern of layers and how injections, data and calls flow through them, the code will become more understandable and easier to extend on.
Another practice I think we should apply more in the future is the use of abstract classes/interfaces. This to also create less coupled and more reusable code and try to keep external dependencies like bdk limited to only a secure BitcoinWalletService class, while implementing a general WalletService class for all wallet types. Using abstract classes and then implementing concrete types can also eliminate the need to check for isLiquid, isBitcoin etc, which is repeated a lot in the code.
I think we can still improve a lot in how the BLoC pattern is applied:
- The list of WalletBloc's in the AppWalletBlocs cubit state is still a Bloc in another Bloc, which is an anti-pattern and should also be able to be done differently.
- States still do not really have different states in the sense of having a Status enum with the different states this BloC can be in (e.g. inital, loading, success, failure) or having a sealed class for shared properties and subclasses for every distinct state/step a feature or BLoC can be in. Without this, the BLoC pattern can not really be used as effective and more code and if/else’s are needed to figure out what state the BloC is really in now which leads to less maintainable and understandable code.
- The CurrencyCubit still holds state like amounts that logically should be part of other BloCs like the amount to send should be managed by the SendBloc and the amount to receive should be managed by the ReceiveBloc. The CurrencyCubit’s state should be limited to the available and selected currencies and the exchange rate in my opinion, they should not manage state of different features. I think this is a general problem that various Cubits are defined on a global level, while their State and its values are logically more part of other BLoC’s of other features.

I also think that to continue this BLoC refactoring we can do it more gradually bloc per bloc, starting with the easiest maybe like the NetworkBloc, then the CurrencyCubit and everything involved with that (currency repository, Currency cubit state, separating the amount from it and adding it to the send and receive state itself etc.), then the rest of the ReceiveCubit, then the rest of the SendCubit etc…
This PR was pretty big to review all at once and pretty difficult to really understand all implications and changes.

kumulynja · 2025-01-06T12:48:01Z

lib/_model/wallet.dart

@@ -515,6 +516,10 @@ class Wallet with _$Wallet {
  List<SwapTx> swapsToProcess() {
    return swaps.where((swap) => swap.proceesTx() && !swap.failed()).toList();
  }
+
+  int balanceSats() => balance ?? 0;


Why not change the balance field/parameter to balanceSats directly with a default value of 0 instead of having balance which is unclear if it is in sats or btc or fiat and then having a function for balanceSats which just returns balance or 0… I think this can be clearer by just replacing the balance field for balanceSats directly with a default 0 value.

kumulynja · 2025-01-06T12:52:00Z

lib/_model/wallet.dart

+
+  int balanceSats() => balance ?? 0;
+
+  String balanceStr() => ((balance ?? 0) / 100000000).toStringAsFixed(8);


The balanceStr seems to be the balance in btc and as a String then, but the fact that it is in btc is not clear from the name. balanceBtc would be a better name I think, and I would leave it as a numeric value (preferably Decimal type) and leave String formatting up to the presentation/UI layer and not the model. The intl package number formatting capabilities (NumberFormat.currency) can be used in the Widgets for a formatted String of a currency balance like this.

kumulynja · 2025-01-06T13:55:26Z

lib/_pkg/deep_link.dart

@@ -39,8 +40,8 @@ class DeepLink {
  Future<Err?> handleUri({
    required String link,
    // required SettingsCubit settingsCubit,
-    required HomeCubit homeCubit,
-    required NetworkCubit networkCubit,
+    required HomeBloc homeCubit,


HomeBloc is renamed, but the variable is still named homeCubit.
Same for networkCubit.

kumulynja · 2025-01-06T13:58:47Z

lib/_pkg/storage/storage.dart

@@ -13,6 +13,7 @@ class StorageKeys {
  static const wallets = 'wallets';
  static const settings = 'settings';
  static const network = 'network';
+  static const networkReposity = 'networkReposity';


Not clear why a repository itself should have a key to be stored itself...?

kumulynja · 2025-01-06T14:03:20Z

lib/_repository/app_wallets_repository.dart

+
+    final List<Wallet> walletsWithEnoughBalance = [];
+
+    for (final walletBloc in wallets) {


Rename walletBloc to wallet since it is not a Bloc anymore.

kumulynja · 2025-01-06T14:31:23Z

lib/_repository/wallet_service.dart

+
+class WalletService {
+  WalletService({
+    required Wallet wallet,


I think it would be clearer if a WalletService creates a Wallet instance itself in its constructor or some create or init function, instead of already getting one injected. Definitely if it also gets the WalletStorageRepository passed, which is good. Having that repository should be what the service needs to retrieve the Wallet instead of getting injected both.

kumulynja · 2025-01-06T14:35:58Z

lib/_repository/wallet_service.dart

+    required WalletSync walletSync,
+    required WalletBalance walletBalance,
+    required WalletAddress walletAddress,
+    required WalletCreate walletCreate,
+    required WalletTx walletTransaction,


Instead of having the WalletCreate, WalletSync and WalletAddress as “global” instances through locator being passed into the service, wouldn't it be clearer if the WalletService itself is a Global instance and it just has the functions from WalletSync etc in it instead of getting them injected in? It could create and keep a Wallet instance in memory instead of having to pass it to everyone of these classes.

kumulynja · 2025-01-06T14:51:35Z

lib/address/bloc/address_cubit.dart

 import 'package:bb_mobile/address/bloc/address_state.dart';
-import 'package:bb_mobile/wallet/bloc/event.dart';
-import 'package:bb_mobile/wallet/bloc/wallet_bloc.dart';
 import 'package:flutter_bloc/flutter_bloc.dart';

 class AddressCubit extends Cubit<AddressState> {


I feel like this Cubit can be separated and replaced with repositories and wallet services to make things more straight forward.

kumulynja · 2025-01-06T14:57:00Z

lib/home/home_page.dart

@@ -35,6 +37,12 @@ import 'package:gap/gap.dart';
 import 'package:go_router/go_router.dart';
 import 'package:google_fonts/google_fonts.dart';

+class AppWalletBlocs extends Cubit<List<WalletBloc>> {


This is still a WalletBloc inside another Cubit, this is really an anti-pattern.
Why not have methods in the HomeBloc itself to clearWallets or update the Wallet’s that are available? If this is possible to do from the home page by the user, this should just be part of the HomeBloc I think.

kumulynja · 2025-01-06T15:01:50Z

lib/home/listeners.dart

+
+    for (final bloc in blocs) {
+      context
+          .read<HomeLoadingCubit>()


Instead of having a HomeLoadingCubit , shouldn’t we just have a loading state in the HomeBloc state itself?
If we pass the wallet repositories to the HomeBloc, the Bloc itself can listen to events of it and change its state from and to Loading.

mocodesmo added 30 commits December 31, 2024 14:39

rearrange

9edf8ba

updates

eed8f3f

updates

c03421b

clean

36804d2

updates

057aae6

clean

1b24bbd

updates

ca32965

clean

2c34342

clean

1696b64

clean

48cc1fe

clean

42f814d

rename

08a8978

clean

567d3cf

clean

8d44e72

updates

87fdb76

network cubit to bloc

3f5df40

clean

f8be66f

clean

118fde6

fixes

0589659

fixes

26bd7d7

fixes

fcbcf65

clean

dee040b

fixes

94ca64a

fixes

cf46efa

fixes

ea2303a

fixes

96a0fd4

fixes

2323997

network fixes

5a1b0ed

refactor AppWalletsRepository to use a list instead of BehaviorSubjec…

01ff02b

…t for wallet services

slow route fix

5d0c4ba

mocodesmo added 2 commits January 5, 2025 16:12

send fixes

d018d34

Merge branch 'main' into bloc_cleanup

01b186f

kumulynja reviewed Jan 6, 2025

View reviewed changes

ethicnology changed the base branch from main to refactoring-bloc January 7, 2025 16:09

ethicnology merged commit 5140ece into refactoring-bloc Jan 7, 2025
1 of 3 checks passed

ethicnology pushed a commit that referenced this pull request Jan 7, 2025

refactor: Major codebase cleanup (#415)

7343ba4

ethicnology mentioned this pull request Jan 7, 2025

refactor: Major codebase cleanup #418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major codebase cleanup #415

Major codebase cleanup #415

mocodesmo commented Jan 5, 2025

kumulynja left a comment •

edited

Loading

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025

kumulynja Jan 6, 2025


		int balanceSats() => balance ?? 0;

		String balanceStr() => ((balance ?? 0) / 100000000).toStringAsFixed(8);


		final List<Wallet> walletsWithEnoughBalance = [];

		for (final walletBloc in wallets) {

Major codebase cleanup #415

Major codebase cleanup #415

Conversation

mocodesmo commented Jan 5, 2025

kumulynja left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumulynja left a comment •

edited

Loading