Feature flags system

[dewabisma]

Summary

• Added feature flag json file
• Added endpoint to get the feature flags
• Watch feature flag file change to hot reload without redeployment

[n13]

Here's my review of PR #61 - Feature Flags System:

---

Overall Assessment

Solid, well-structured feature. The architecture follows existing codebase patterns nicely (handler/route/service layering, SuccessResponse wrapper, Arc wrapping in AppState). The file watcher for hot-reload is a great operational choice. A few items worth addressing:

---

Issues

1. start_server error propagation with ? on WalletFeatureFlagsError

start_server returns Result<(), Box<dyn std::error::Error>>, so the ? on WalletFeatureFlagsService::new(...) works at runtime. However, WalletFeatureFlagsError is never integrated into AppError, which means if you ever want to use this service from a handler that returns AppError, you'll need an impl From<WalletFeatureFlagsError> for AppError. Right now the handler doesn't hit this path (it just reads from RwLock), so it's fine today but worth noting.

2. RwLock poisoning panic in production [should be fixed]

self.wallet_feature_flags.read().expect("RwLock poisoned").clone()

std::sync::RwLock can poison if a writer panics. The expect("RwLock poisoned") will crash the entire server if that ever happens. Consider returning a Result or using read().unwrap_or_else(|e| e.into_inner()) to gracefully recover with stale data — consistent with the "keep last known good flags" philosophy already used in the watcher.

3. File path is relative — depends on the working directory [would be good to fix this]

WalletFeatureFlagsService::new receives a relative path like "wallet_feature_flags/default_feature_flags.json". This works only if the binary's current working directory is the project root. In production containers or systemd services the cwd might differ. Consider resolving the path relative to a known base (e.g., the config file directory or an env var) to make it more robust.

4. Missing test_feature_flags.json file [need to fix]

config/test.toml points to wallet_feature_flags/test_feature_flags.json, but that file isn't included in the PR — only default_feature_flags.json is. This will break tests that use create_test_app_state() (which reads test.toml). CI is currently pending — I expect it will fail on this.

5. No authentication on the endpoint

The route GET /feature-flags/wallet has no auth middleware. Other routes in the codebase use auth guards. Is this intentional? Feature flags are typically non-sensitive, but worth confirming — especially if flags like enable_high_security could expose security posture information.

6. _state parameter unused in route constructor [nice to have]

pub fn wallet_feature_flags_routes(_state: AppState) -> Router<AppState> {

The _state parameter is unused. Other routes that accept state use it to apply middleware (auth layers, etc.). If no auth is intended, consider removing the parameter to match the actual usage — or adding the appropriate auth if it's needed.

---

Minor / Nitpicks

• Missing newline at EOF in config/default.toml and config/test.toml — the files end without a trailing newline. Not critical but can cause noisy diffs later.
• The _watch_task: JoinHandle<()> is stored but never cancelled on drop. If WalletFeatureFlagsService is dropped, the spawned task will keep running until the next recv() returns None. In practice this is fine since AppState lives for the server's lifetime, but worth documenting if the service is ever used in tests with shorter lifetimes.

---

What Looks Good

• Clean separation: config / service / handler / route all in their own files following existing patterns
• SuccessResponse wrapper used correctly, consistent with rest of the API
• File watcher approach is solid — watches the parent directory, filters by filename, handles atomic saves
• Graceful degradation on bad JSON (keeps last known good flags, logs warning)
• Good test coverage: initial load, hot-reload, and invalid-JSON resilience
• Arc<RwLock<T>> for the shared state is the right choice for read-heavy, rarely-written data

---

Summary

The main blocker is the missing test_feature_flags.json file which will likely fail CI. The RwLock poisoning behavior and relative path resolution are worth fixing before merge. The rest is solid work.

[n13]

See above - I commented on each item if it's actually needed or not, nothing major

[n13]

Findings

• Low — Example config still points to a non-existent file.
  config/example.toml references ../wallet_feature_flags/example_feature_flags.json, but the PR currently ships only default_feature_flags.json and test_feature_flags.json.
  Copying example config as suggested will fail on startup unless that file is added or the path is changed.

[feature_flags]
wallet_feature_flags_config_file = "../wallet_feature_flags/example_feature_flags.json"

Re-check outcome

• Previous blocker (test_feature_flags typo) is fixed.
• CI is now green (Test & Migrate passed).
• The earlier fixes (RwLock panic handling, relative path resolution, unused route param removal) are present and look correct.

Assumptions / residual risk

• I’m assuming the feature-flags endpoint is intentionally public.
• Residual gap: no integration-level test validating config/example usability; only runtime/test config paths are effectively covered.

Reference: PR #61

[n13]

Ok these hard-coded feature flags are better than nothing but why not have a key/value system with just strings?

Meaning we can just define new feature flags in the wallet without deploying a new server?

Or with a local way to change the feature flag settings?

server get_setting("flag", "default")

Then we actually need a way to change feature flags on the server too
server set_flag(auth, "flag", "new_value")

Either way these are two different and unrelated concerns:

1. Can we make feature flags be soft, in that any string/string combination can be set and requested? So these aren't hard-coded into the codebase?

2. How do we set new values? Even if we have hard-coded values here, we probably don't want to deploy a new server every time we change the flags.

Hmmm... small admin page?

Anyway this PR is good for what it is doing

[n13]

why do we have these hard-coded values?

If we manage these by editing the JSON file, we can just have anything in there right?

[dewabisma]

we just change the json file we don't have to redeploy just edit it using nano/vim or other text editor

[n13]

Ok I reviewed, just some questions on how it works

• So editing json file is the way to deploy new values?
• If so why do we even have hard-coded variables in the codebase? See above.

[dewabisma]

Ok I reviewed, just some questions on how it works

• So editing json file is the way to deploy new values?
• If so why do we even have hard-coded variables in the codebase? See above.

1. Yes we can edit json file to add more value or update existing or delete value it will directly reflected.
2. My bad, I forgot that we want freedom to not have to redeploy. I will update it. This is really good comment.

[n13]

Good, I now have the full picture. Here's my review of PR #61 — Feature Flags System.

---

Overall

Nice work. The architecture is clean — handler/route/service layering follows existing codebase patterns. The switch to serde_json::Value (from a hardcoded struct) in the latest commit addresses the earlier feedback well, giving the wallet team full flexibility without server redeployments. The file watcher for hot-reload is a solid operational choice, and the graceful degradation on bad JSON is exactly the right behavior. CI is green.

That said, there are a few items I'd flag:

---

Issues

1. Missing example_feature_flags.json (should fix)

config/example.toml references ../wallet_feature_flags/example_feature_flags.json, but that file doesn't exist in the PR — only default_feature_flags.json and test_feature_flags.json are shipped. Anyone copying example.toml to bootstrap a new environment will hit a startup crash. Either add the file or update the path in example.toml to point to default_feature_flags.json.

4. Watcher fallback path is a magic string

let parent_dir = Path::new(&file_path)
    .parent()
    .unwrap_or(Path::new("wallet_feature_flags"));

If file_path has no parent (bare filename), this silently falls back to watching a directory called wallet_feature_flags which may or may not exist. This is unlikely to happen given the config-based path resolution, but returning an error here would be more defensible than a silent wrong-directory watch.

5. _watch_task is never cancelled on drop [probably OK]

The spawned JoinHandle is stored but never aborted when WalletFeatureFlagsService is dropped. In practice this is fine since AppState lives for the server's lifetime, but if the service is ever used in tests with shorter lifetimes or re-created, the orphaned task will keep running until the channel closes. Consider implementing Drop to abort the task, or at minimum document this as a known constraint.

6. Test temp files not cleaned up on panic [i think its fine as is]

The tests call std::fs::remove_file(path).ok() at the end, but if the test panics before that line (e.g. the wait_until timeout), the temp file leaks. A guard struct that deletes on Drop (or using tempfile crate) would be more robust.

---

What Looks Good

• Clean separation: config / service / handler / route in their own files, matching existing patterns
• SuccessResponse wrapper used correctly, consistent with the rest of the API
• File watcher approach is solid — watches parent directory, filters by filename, handles atomic saves
• Graceful degradation on bad JSON (keeps last known good flags, logs warning)
• Good test coverage: initial load, hot-reload, and invalid-JSON resilience
• Arc<RwLock<Value>> is the right choice for read-heavy, rarely-written data
• The move from hardcoded struct to serde_json::Value gives full flexibility without redeployment
• Proper error type integration into AppError with From impl

---

Summary

The main actionable item is the missing example_feature_flags.json file. The auth question should be explicitly confirmed. Everything else is minor hardening. Solid PR overall.