Remote feature flags

[dewabisma]

Summary

• Remove old way of constant feature flags
• Add remote feature flags provider
• Add new method to task master service to get feature flags

Note

We shouldn't merge this before we deploy the new task master version

[n13]

Ok this once approved can you also merge this into the new_testnet_branch?

Also please deploy new taskmaster, we can do this independently of new testnet.

• merge into new testnet once approved
• deploy new taskmaster

[dewabisma]

Ok this once approved can you also merge this into the new_testnet_branch?

Also please deploy new taskmaster, we can do this independently of new testnet.

• merge into new testnet once approved
• deploy new taskmaster

Okay I can help merging this also to new_testnet_branch, I will deploy taskmaster meanwhile

[n13]

Good, I now have a thorough understanding of the PR. Here's my review:

---

PR Review: Remote Feature Flags (#425)

Overall: This is a solid refactor -- moving from compile-time constants to remote feature flags is the right direction. The architecture is clean: SDK model, service with caching, Riverpod provider. A few issues worth addressing though:

---

1. Race condition at startup (critical)

In main(), FeatureFlagsService is instantiated directly (outside Riverpod) to fetch flags before the widget tree exists:

  if (FeatureFlags.enableRemoteNotifications) {
    await Firebase.initializeApp(options: DefaultFirebaseOptions.getOptionsForEnvironment());
  }

The PR changes this to:

final featureFlags = await FeatureFlagsService().getFlagsWithFallback();
if (featureFlags.enableRemoteNotifications) {

Then in AppInitializer, the FeatureFlagsNotifier constructor also calls syncFlags(), and _initialize() calls syncFlags() again:

  FeatureFlagsNotifier(this._service) : super(FeatureFlagsModel.defaults) {
    syncFlags();
  }

  Future<void> syncFlags() async {
    final flags = await _service.getFlagsWithFallback();

That's three network requests for feature flags on every cold start: one in main(), one from the notifier constructor, and one from _AppInitializerState._initialize(). The notifier constructor call is fire-and-forget too, so the provider state could flip after widgets have already read the defaults.

Suggestion: Remove the syncFlags() call from the constructor. The explicit call in _initialize() is sufficient and has proper sequencing (await).

---

2. Default values differ from the old constants

The old FeatureFlags:

static const bool enableHighSecurity = true;
static const bool enableSwap = true;

The new FeatureFlagsModel.defaults:

  static const FeatureFlagsModel defaults = FeatureFlagsModel(
    enableTestButtons: false,
    enableKeystoneHardwareWallet: false,
    enableHighSecurity: false,
    enableRemoteNotifications: true,
    enableSwap: false,
  );

Both enableHighSecurity and enableSwap are now false by default instead of true. If the remote fetch fails on first launch (no cache yet), these features will be disabled. Is that intentional? If users lose swap access because of a transient network failure, that's a noticeable regression.

---

3. getWalletFeatureFlags doesn't require auth

  Future<FeatureFlagsModel> getWalletFeatureFlags() async {
    final Uri uri = Uri.parse('${AppConstants.taskMasterEndpoint}/feature-flags/wallet');

    final http.Response response = await http.get(uri, headers: {'Content-Type': 'application/json'});

This is the only TaskmasterService method that doesn't call ensureIsLoggedIn() and sends no auth header. That makes sense if the endpoint is public, but it's worth confirming this is intentional and not an oversight -- especially since the method lives on TaskmasterService where every other method authenticates.

---

4. Redundant type check

    if (data is! Map) {
      throw Exception('Feature flags response body is invalid. Body: ${response.body}');
    }

data is already typed as Map<String, dynamic>? from the line above. The only way it's not a Map is if it's null. A null check (if (data == null)) would be clearer.

---

5. _readBool is a top-level function

bool? _readBool(dynamic value) {
  if (value is bool) return value;
  if (value is num) return value != 0;
  if (value is String) {

This is fine for internal use, but it's a top-level private function in the library file. If the SDK exports this file, _readBool won't be accessible outside but it does pollute the library's top-level namespace. Consider making it a static method on FeatureFlagsModel instead.

---

6. Minor: print for error logging

The service uses print() for errors in several places. The rest of the codebase uses debugPrint() in AppInitializer. Might want to keep that consistent, or even use a proper logger since these are real failure paths (network errors, parse errors).

---

Summary

Area	Verdict
Architecture (remote flags + cache + provider)	Good
Migration of callsites	Clean
Triple-fetch at startup	Should fix
Default value changes (highSecurity, swap)	Needs confirmation
No auth on feature flags endpoint	Needs confirmation
Code quality	Minor nits

The big thing to address before merge is the triple network call and the default value discrepancies. The rest are small.

[n13]

Adding some real specs - this is modeled after Firebase Remote Config and how that works.

It's basically a cache with default values and background updates.

• Feature flag service is initialized with a static initializer which reads feature flags from cache; if no cache exists, there's a default value for each flag.
• Feature flags are always ready to use even at startup
• Fetching updates never causes a wait
• Updated flags check whether anything has changed (they keep track) and only notify the provider if there are changes
• Feature flags use a String/String array for parsing and don't need "fallbacks" for non string values. Or if you prefer since it's JSON and json has boolean, int, and string types, we could also just use these native json types. However, each value has a type definition and something that's supposed to be boolean won't be "int" - if it is, it's a crash. No fallbacks, just a clear definition.
• Update / Refresh is triggered on App startup and resume. It's a background update, we don't wait for this, but if anything changes, we trigger the provider update.

Sorry for not specifying these things earlier, I kind of assumed saying "Like Firebase remote config" would be good enough. This is how Firebase works, more or less. But would have been better to think about this beforehand and provide a good spec. Mea culpa.

[n13]

hmm why can't we use initialized for this?

would rename this to hasSetupHandlers... taphandler sounds like it's a gesture listener, which it's not...

[n13]

I deleted some things that are intentional like no auth...

PR #425 Review: Remote Feature Flags

Architecture Summary

The PR replaces compile-time constants (FeatureFlags) with a remote system: an SDK model (FeatureFlagsModel), a service layer (FeatureFlagsService) with SharedPreferences caching, a Riverpod StateNotifier provider, and a Taskmaster endpoint for remote fetching. Flags refresh on startup and on app resume from background.

The overall direction is good. The architecture is clean: SDK model, service with caching, Riverpod provider. That said, there are several issues ranging from spec-compliance problems to bugs.

---

1. fromJson crashes on missing keys (critical)

The spec says: "each value has a type definition and something that's supposed to be boolean won't be 'int' - if it is, it's a crash. No fallbacks." That's fine. But _readBool also crashes on null:

static bool _readBool(dynamic value) {
    if (value is! bool) throw Exception('Invalid boolean value: $value');
    return value;
}

If the server adds a new flag tomorrow and the cached JSON doesn't have it, json?['newFlag'] passes null to _readBool, which crashes. Similarly, if the server omits a key from the response, same crash. The model should use defaults for missing keys from the cache, while strictly requiring correct types when present. Something like:

static bool _readBool(dynamic value, {required bool defaultValue}) {
    if (value == null) return defaultValue;
    if (value is! bool) throw Exception('Expected bool, got ${value.runtimeType}: $value');
    return value;
}

This way type mismatches crash (per spec), but missing keys gracefully fall back to defaults, which is exactly how Firebase Remote Config works.

---

2. Provider unconditionally overwrites state (spec violation)

The spec says: "Updated flags check whether anything has changed (they keep track) and only notify the provider if there are changes."

Currently in FeatureFlagsNotifier.syncFlags():

final remote = await _service.readRemoteFlags();
if (remote == null) return;
_service.cacheFlags(remote.toCacheJson());
state = remote;  // always sets, always notifies

This always assigns state, which triggers a Riverpod rebuild of every listener even if nothing changed. The model needs an equality check (implement == and hashCode or use Equatable), and the notifier should only assign state when values actually differ:

if (remote != state) {
  _service.cacheFlags(remote.toCacheJson());
  state = remote;
}

---

3. readLocalFlags() writes to cache on first read (side effect in a read)

FeatureFlagsModel readLocalFlags() {
    final jsonString = _settingsService.getString(featureFlagsCacheKey);
    if (jsonString == null || jsonString.isEmpty) {
      cacheFlags(FeatureFlagsModel.defaults.toCacheJson());  // fire-and-forget async write
      return FeatureFlagsModel.defaults;
    }
    ...
}

A method named readLocalFlags shouldn't have the side effect of writing. It's also calling an async cacheFlags without awaiting, which is fine for fire-and-forget but inconsistent. Just return the defaults without writing -- the cache will be populated on the first successful remote fetch anyway.

---

4. FeatureFlagsModel lacks == / hashCode

Without these, the equality check recommended in point 2 won't work (Dart uses identity comparison for plain classes). Either add operator == + hashCode, or use Equatable. This is essential for the "only notify on change" requirement.

---

6. Double syncFlags() call on startup

This seems like a mistake, I think it should only be called in one place? Not sure. Need to understand this better.

The FeatureFlagsNotifier constructor calls syncFlags():

FeatureFlagsNotifier(this._service) : super(_service.readLocalFlags()) {
    syncFlags();
}

And then AppLifecycleManager also calls syncFlags() on resume. The constructor call is the initial startup fetch, so that's correct. But the _isRefreshingRemote guard means if the constructor-triggered fetch is still in flight when resume fires, the resume call is silently dropped. That's acceptable behavior, just noting it.

However, the listenManual callback in AppInitializer._initialize() will fire _enableRemoteNotificationsIfNeeded() whenever the state changes, and _initialize also checks enableRemoteNotifications immediately after registering the listener. The _isEnablingRemoteNotifications guard handles the double-call, so this is fine functionally but a bit convoluted.

---

7. navigatorKey moved to app_initializer.dart

// app_initializer.dart
final GlobalKey<NavigatorState> navigatorKey = GlobalKey<NavigatorState>();

This was previously in app.dart. The app.dart file still uses navigatorKey (in the MaterialApp and in _setupNotifications), so it now imports it from app_initializer.dart. That creates a dependency where the app's root widget imports from a child widget. Consider moving navigatorKey to a shared location instead of tying it to the initializer.

---

9. _hasSetupTapHandlers naming

The reviewer comment on the PR is valid -- _hasSetupTapHandlers sounds like gesture tap handlers. Something like _hasSetupNotificationHandlers or _hasRegisteredHandlers would be clearer. Also, _isInitialized could potentially serve this purpose if the semantic is the same.

---

Summary Table

Area	Verdict
Architecture (remote flags + cache + provider)	Good
Synchronous init from cache at startup	Good (spec-compliant)
Background refresh, never blocks	Good (spec-compliant)
Refresh on startup + resume	Good (spec-compliant)
Strict typing (bool must be bool)	Good (spec-compliant)
Crash on null/missing keys	Needs fix -- will crash on cache/server schema drift
Only notify on change	Missing -- always sets state, no ==/hashCode
No auth on feature flags endpoint	Needs confirmation
readLocalFlags side effect	Minor -- remove the write
print vs debugPrint	Minor consistency
navigatorKey location	Minor architectural nit

The two things that should be fixed before merge are the missing equality check (spec requires only-notify-on-change) and the null crash in _readBool (cache will be stale when new flags are added server-side). Everything else is minor.

[n13]

Question: Why did you not just add a separate function called "flagsHaveChanged" or whatever and use that for the same-ness comparison.

I usually prefer doing this over overriding the == / equals / hash functions because it doesn't have any side effects, it's constrained to that one check which we want to do... it's basically a more targeted code piece that does exactly what we need it to do. This way if == is used elsewhere in the code under different circumstances we don't override that...

Also whichever way you do it, please make a unit test that will trigger when we changed something...

ie what usually happens is, you add some flag... and then forget to update the == override, and then it's mostly working but when only that flag changes it doesn't work

There's a way to structure to the code to make sure such errors are caught at compile time or at least at testing...

maybe we can put all compare values in an array or something like that... not sure...

[dewabisma]

I added CI test and remove one unit test in send screen logic test because seems it's irrelevant anymore now the value 0 is not counted as error in the function maybe it's what you did to adjust new design not sure. If not we can sill count zero as error.

[n13]

LGTM! 🚀