Skip to content

Conversation

@qustavo
Copy link
Contributor

@qustavo qustavo commented Nov 21, 2025

The main goal of this PR is to improve the flag visibility by document the usage within arkd. Now flags can be set with -- syntax, while keeping compatibility with previous env var mechanic.

Flags are defined by cli and viper has been removed.
A comprehensive list of flags and its usage can be displayed as follows:

   NAME:
      arkd start - Starts the arkd server

   USAGE:
      arkd start [command options]

   OPTIONS:
      --datadir value                                         Directory to store data (default: "/home/gustavo/.arkd") [$ARKD_DATADIR]
      --port value                                            Port (public) to listen on (default: 7070) [$ARKD_PORT]
      --admin-port value                                      Admin port (private) to listen on, fallback to service port if 0 (default: 7071) [$ARKD_ADMIN_PORT]
      --log-level value                                       Logging level (0-6, where 6 is trace) (default: 4) [$ARKD_LOG_LEVEL]
      --session-duration value                                How long a batch session lasts (in seconds) before timing out once it started (default: 30) [$ARKD_SESSION_DURATION]
      --db-type value                                         Database type (postgres, sqlite, badger) (default: "postgres") [$ARKD_DB_TYPE]
      --pg-db-url value                                       Postgres connection url if ARKD_DB_TYPE is set to postgres [$ARKD_PG_DB_URL]
      --event-db-type value                                   Event database type (postgres, badger) (default: "postgres") [$ARKD_EVENT_DB_TYPE]
      --pg-event-db-url value                                 Postgres connection url if ARKD_EVENT_DB_TYPE is set to postgres [$ARKD_PG_EVENT_DB_URL]
      --tx-builder-type value                                 Transaction builder type (default: "covenantless") [$ARKD_TX_BUILDER_TYPE]
      --live-store-type value                                 Cache service type (redis, inmemory) (default: "redis") [$ARKD_LIVE_STORE_TYPE]
      --redis-url value                                       Redis db connection url if ARKD_LIVE_STORE_TYPE is set to redis [$ARKD_REDIS_URL]
      --redis-num-of-retries value                            Maximum number of retries for Redis write operations in case of conflicts (default: 10) [$ARKD_REDIS_NUM_OF_RETRIES]
      --vtxo-tree-expiry value                                VTXO tree expiry in seconds (default: 604672 (~7 days)) [$ARKD_VTXO_TREE_EXPIRY]
      --unilateral-exit-delay value                           Unilateral exit delay in seconds (default: 86400 (~24 hours)) [$ARKD_UNILATERAL_EXIT_DELAY]
      --public-unilateral-exit-delay value                    Public unilateral exit delay in seconds (default: 86400 (~24 hours)) [$ARKD_PUBLIC_UNILATERAL_EXIT_DELAY]
      --boarding-exit-delay value                             Boarding exit delay in seconds (default: 7776000 (~3 months)) [$ARKD_BOARDING_EXIT_DELAY]
      --esplora-url value                                     Esplora API URL (default: "https://blockstream.info/api") [$ARKD_ESPLORA_URL]
      --wallet-addr value                                     The arkd wallet address to connect to in the form host:port [$ARKD_WALLET_ADDR]
      --signer-addr value                                     The signer address to connect to in the form host:port (default: value of `ARKD_WALLET_ADDR`) [$ARKD_SIGNER_ADDR]
      --no-macaroons                                          Disable Macaroons authentication (default: false) [$ARKD_NO_MACAROONS]
      --no-tls                                                Disable TLS (default: true) [$ARKD_NO_TLS]
      --unlocker-type value                                   Wallet unlocker type (env, file) to enable auto-unlock [$ARKD_UNLOCKER_TYPE]
      --unlocker-file-path value                              Path to unlocker file [$ARKD_UNLOCKER_FILE_PATH]
      --unlocker-password value                               Wallet unlocker password [$ARKD_UNLOCKER_PASSWORD]
      --round-max-participants-count value                    Maximum number of participants per round (default: 128) [$ARKD_ROUND_MAX_PARTICIPANTS_COUNT]
      --round-min-participants-count value                    Minimum number of participants per round (default: 1) [$ARKD_ROUND_MIN_PARTICIPANTS_COUNT]
      --utxo-max-amount value                                 The maximum allowed amount for boarding or collaborative exit (default: -1 unset) [$ARKD_UTXO_MAX_AMOUNT]
      --utxo-min-amount value                                 The minimum allowed amount for boarding or collaborative exit (default: -1 dust) [$ARKD_UTXO_MIN_AMOUNT]
      --utxo-min-amount value                                 The minimum allowed amount for boarding or collaborative exit (default: -1 dust) [$ARKD_UTXO_MIN_AMOUNT]
      --vtxo-max-amount value                                 The maximum allowed amount for vtxos (default: -1 unset) [$ARKD_VTXO_MAX_AMOUNT]
      --vtxo-min-amount value                                 The minimum allowed amount for vtxos (default: -1 dust) [$ARKD_VTXO_MIN_AMOUNT]
      --ban-duration value                                    Ban duration in seconds (default: 300) [$ARKD_BAN_DURATION]
      --ban-threshold value                                   Number of crimes to trigger a ban (default: 3) [$ARKD_BAN_THRESHOLD]
      --scheduler-type value                                  Scheduler type (gocron, block) (default: "gocron") [$ARKD_SCHEDULER_TYPE]
      --checkpoint-exit-delay value                           Checkpoint exit delay in seconds (default: 86400) [$ARKD_CHECKPOINT_EXIT_DELAY]
      --tls-extra-ip value [ --tls-extra-ip value ]           Extra IP addresses for TLS (comma-separated) [$ARKD_TLS_EXTRA_IP]
      --tls-extra-domain value [ --tls-extra-domain value ]   Extra domains for TLS (comma-separated) [$ARKD_TLS_EXTRA_DOMAIN]
      --note-uri-prexi value                                  Note URI prefix [$ARKD_NOTE_URI_PREFIX]
      --scheduled-session-start-time value                    Scheduled session start time (Unix timestamp) (default: 0) [$ARKD_SCHEDULED_SESSION_START_TIME]
      --scheduled-session-end-time value                      Scheduled session end time (Unix timestamp) (default: 0) [$ARKD_SCHEDULED_SESSION_END_TIME]
      --scheduled-session-period value                        Scheduled session period in minutes (default: 0) [$ARKD_SCHEDULED_SESSION_PERIOD]
      --scheduled-session-duration value                      Scheduled session duration in seconds (default: 0) [$ARKD_SCHEDULED_SESSION_DURATION]
      --scheduled-session-min-round-participants-count value  Min participants for scheduled sessions [$ARKD_SCHEDULED_SESSION_MIN_ROUND_PARTICIPANTS_COUNT]
      --scheduled-session-max-round-participants-count value  Max participants for scheduled sessions [$ARKD_SCHEDULED_SESSION_MAX_ROUND_PARTICIPANTS_COUNT]
      --collector-endpoint value                              OpenTelemetry collector endpoint [$ARKD_COLLECTOR_ENDPOINT]
      --otel-push-internal value                              OpenTelemetry push interval in seconds (default: 10) [$ARKD_OTEL_PUSH_INTERVAL]
      --allow-csv-block-type                                  Allow CSV block type (default: false) [$ARKD_ALLOW_CSV_BLOCK_TYPE]
      --heartbeat-interval value                              Heartbeat interval in seconds (default: 60) [$ARKD_HEARTBEAT_INTERVAL]
      --round-report-enabled                                  Enable round report service (default: false) [$ARKD_ROUND_REPORT_ENABLED]
      --settlement-min-expiry-gap value                       (default: 0 disabled) [$ARKD_SETTLEMENT_MIN_EXPIRY_GAP]
      --vtxo-no-csv-validation-cutoff-date value              (default: 0 disabled) [$ARKD_VTXO_NO_CSV_VALIDATION_CUTOFF_DATE]
      --onchain-output-fee value                              (default: 0) [$ARKD_ONCHAIN_OUTPUT_FEE]
      --alert-manager-url value                                [$ARKD_ALERT_MANAGER_URL]
      --help, -h                                              show help

Closes #782

Summary by CodeRabbit

  • New Features

    • Most application configuration options are now exposed as command-line flags for the arkd start command, allowing runtime configuration via flags or environment variables.
  • Documentation

    • Setup instructions updated to reflect the new arkd start usage.
  • Chores

    • Internal configuration system refactored to centralize flag/env handling and configuration loading; a dependency representation was adjusted.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

Replaces viper-based configuration with urfave/cli v2 flags, exposes those flags on the arkd start command, updates LoadConfig/initDatadir to accept a *cli.Context, moves viper to an indirect require in go.mod, and updates README to call arkd start.

Changes

Cohort / File(s) Summary
Configuration System Migration
internal/config/config.go
Replaces viper with urfave/cli v2 flags: introduces ~40+ exported *cli.Flag declarations and Flags []cli.Flag. Changes LoadConfig()LoadConfig(c *cli.Context) (*Config, error) and initDatadir()initDatadir(c *cli.Context) error. Loads config via c.String/c.Int/c.Bool/..., creates datadir from context, and validates derived settings.
CLI Command Wiring
cmd/arkd/commands.go, cmd/arkd/main.go
cmd/arkd/commands.go imports internal/config and assigns config.Flags to the start command; startAction now accepts c *cli.Context. main.go calls config.LoadConfig(c).
Dependency Management
go.mod
Moves github.com/spf13/viper v1.20.1 from direct to indirect require.
Documentation
README.md
Updates setup instructions to use arkd start instead of arkd.

Sequence Diagram(s)

sequenceDiagram
    participant User as User CLI
    participant CLI as urfave/cli
    participant StartCmd as arkd start
    participant Config as internal/config.LoadConfig
    participant Datadir as initDatadir
    participant App as App init

    User->>CLI: run `arkd start` with flags/env
    CLI->>StartCmd: invoke start command
    StartCmd->>Config: LoadConfig(c *cli.Context)
    note right of Config `#D6EAF8`: Reads flags via c.String/c.Int/c.Bool...
    Config->>Datadir: initDatadir(c)
    Datadir-->>Config: datadir created/validated
    Config-->>StartCmd: returns *Config or error
    StartCmd->>App: initialize services with Config
    App-->>StartCmd: started / error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on internal/config/config.go (many new exported flags, types, defaults, EnvVars mapping).
  • Verify all call sites updated for LoadConfig(c *cli.Context) and initDatadir(c *cli.Context).
  • Check config.Flags wiring in cmd/arkd/commands.go and CLI help/usage output.

Possibly related PRs

Suggested reviewers

  • louisinger
  • Kukks

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Improve flag visibility' accurately reflects the main change in this PR, which migrates from viper-based config to CLI flags with explicit visibility.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from issue #782: flags are now defined as CLI flags (visible in help), moved from mainAction to start subcommand, and environment-variable compatibility is preserved.
Out of Scope Changes check ✅ Passed All changes are in scope: the viper-to-CLI-flags migration, command restructuring, and config loading updates directly support the issue objectives.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qustavo qustavo force-pushed the use_cli_env_flags branch 3 times, most recently from e29f92c to d277e9c Compare November 21, 2025 22:24
@qustavo qustavo marked this pull request as ready for review November 21, 2025 22:24
@qustavo qustavo force-pushed the use_cli_env_flags branch 3 times, most recently from bce47d5 to 00c5503 Compare November 21, 2025 22:34
The main goal of this PR is to improve the flag visibility by document the usage within arkd.
Now flags can be set with `--` syntax, while keeping compatibility with previous env var mechanic.

Flags are defined by cli and viper has been removed.
A comprehensive list of flags and its usage can be displayed as follows:
```bash
NAME:
   arkd start - Starts the arkd server

USAGE:
   arkd start [command options]

OPTIONS:
   --datadir value                                         Directory to store data (default: "/home/gustavo/.arkd") [$ARKD_DATADIR]
   --port value                                            Port (public) to listen on (default: 7070) [$ARKD_PORT]
   --admin-port value                                      Admin port (private) to listen on, fallback to service port if 0 (default: 7071) [$ARKD_ADMIN_PORT]
   --log-level value                                       Logging level (0-6, where 6 is trace) (default: 4) [$ARKD_LOG_LEVEL]
   --session-duration value                                How long a batch session lasts (in seconds) before timing out once it started (default: 30) [$ARKD_SESSION_DURATION]
   --db-type value                                         Database type (postgres, sqlite, badger) (default: "postgres") [$ARKD_DB_TYPE]
   --pg-db-url value                                       Postgres connection url if ARKD_DB_TYPE is set to postgres [$ARKD_PG_DB_URL]
   --event-db-type value                                   Event database type (postgres, badger) (default: "postgres") [$ARKD_EVENT_DB_TYPE]
   --pg-event-db-url value                                 Postgres connection url if ARKD_EVENT_DB_TYPE is set to postgres [$ARKD_PG_EVENT_DB_URL]
   --tx-builder-type value                                 Transaction builder type (default: "covenantless") [$ARKD_TX_BUILDER_TYPE]
   --live-store-type value                                 Cache service type (redis, inmemory) (default: "redis") [$ARKD_LIVE_STORE_TYPE]
   --redis-url value                                       Redis db connection url if ARKD_LIVE_STORE_TYPE is set to redis [$ARKD_REDIS_URL]
   --redis-num-of-retries value                            Maximum number of retries for Redis write operations in case of conflicts (default: 10) [$ARKD_REDIS_NUM_OF_RETRIES]
   --vtxo-tree-expiry value                                VTXO tree expiry in seconds (default: 604672 (~7 days)) [$ARKD_VTXO_TREE_EXPIRY]
   --unilateral-exit-delay value                           Unilateral exit delay in seconds (default: 86400 (~24 hours)) [$ARKD_UNILATERAL_EXIT_DELAY]
   --public-unilateral-exit-delay value                    Public unilateral exit delay in seconds (default: 86400 (~24 hours)) [$ARKD_PUBLIC_UNILATERAL_EXIT_DELAY]
   --boarding-exit-delay value                             Boarding exit delay in seconds (default: 7776000 (~3 months)) [$ARKD_BOARDING_EXIT_DELAY]
   --esplora-url value                                     Esplora API URL (default: "https://blockstream.info/api") [$ARKD_ESPLORA_URL]
   --wallet-addr value                                     The arkd wallet address to connect to in the form host:port [$ARKD_WALLET_ADDR]
   --signer-addr value                                     The signer address to connect to in the form host:port (default: value of `ARKD_WALLET_ADDR`) [$ARKD_SIGNER_ADDR]
   --no-macaroons                                          Disable Macaroons authentication (default: false) [$ARKD_NO_MACAROONS]
   --no-tls                                                Disable TLS (default: true) [$ARKD_NO_TLS]
   --unlocker-type value                                   Wallet unlocker type (env, file) to enable auto-unlock [$ARKD_UNLOCKER_TYPE]
   --unlocker-file-path value                              Path to unlocker file [$ARKD_UNLOCKER_FILE_PATH]
   --unlocker-password value                               Wallet unlocker password [$ARKD_UNLOCKER_PASSWORD]
   --round-max-participants-count value                    Maximum number of participants per round (default: 128) [$ARKD_ROUND_MAX_PARTICIPANTS_COUNT]
   --round-min-participants-count value                    Minimum number of participants per round (default: 1) [$ARKD_ROUND_MIN_PARTICIPANTS_COUNT]
   --utxo-max-amount value                                 The maximum allowed amount for boarding or collaborative exit (default: -1 unset) [$ARKD_UTXO_MAX_AMOUNT]
   --utxo-min-amount value                                 The minimum allowed amount for boarding or collaborative exit (default: -1 dust) [$ARKD_UTXO_MIN_AMOUNT]
   --vtxo-max-amount value                                 The maximum allowed amount for vtxos (default: -1 unset) [$ARKD_VTXO_MAX_AMOUNT]
   --vtxo-min-amount value                                 The minimum allowed amount for vtxos (default: -1 dust) [$ARKD_VTXO_MIN_AMOUNT]
   --ban-duration value                                    Ban duration in seconds (default: 300) [$ARKD_BAN_DURATION]
   --ban-threshold value                                   Number of crimes to trigger a ban (default: 3) [$ARKD_BAN_THRESHOLD]
   --scheduler-type value                                  Scheduler type (gocron, block) (default: "gocron") [$ARKD_SCHEDULER_TYPE]
   --checkpoint-exit-delay value                           Checkpoint exit delay in seconds (default: 86400) [$ARKD_CHECKPOINT_EXIT_DELAY]
   --tls-extra-ip value [ --tls-extra-ip value ]           Extra IP addresses for TLS (comma-separated) [$ARKD_TLS_EXTRA_IP]
   --tls-extra-domain value [ --tls-extra-domain value ]   Extra domains for TLS (comma-separated) [$ARKD_TLS_EXTRA_DOMAIN]
   --note-uri-prexi value                                  Note URI prefix [$ARKD_NOTE_URI_PREFIX]
   --scheduled-session-start-time value                    Scheduled session start time (Unix timestamp) (default: 0) [$ARKD_SCHEDULED_SESSION_START_TIME]
   --scheduled-session-end-time value                      Scheduled session end time (Unix timestamp) (default: 0) [$ARKD_SCHEDULED_SESSION_END_TIME]
   --scheduled-session-period value                        Scheduled session period in minutes (default: 0) [$ARKD_SCHEDULED_SESSION_PERIOD]
   --scheduled-session-duration value                      Scheduled session duration in seconds (default: 0) [$ARKD_SCHEDULED_SESSION_DURATION]
   --scheduled-session-min-round-participants-count value  Min participants for scheduled sessions [$ARKD_SCHEDULED_SESSION_MIN_ROUND_PARTICIPANTS_COUNT]
   --scheduled-session-max-round-participants-count value  Max participants for scheduled sessions [$ARKD_SCHEDULED_SESSION_MAX_ROUND_PARTICIPANTS_COUNT]
   --collector-endpoint value                              OpenTelemetry collector endpoint [$ARKD_COLLECTOR_ENDPOINT]
   --otel-push-internal value                              OpenTelemetry push interval in seconds (default: 10) [$ARKD_OTEL_PUSH_INTERVAL]
   --allow-csv-block-type                                  Allow CSV block type (default: false) [$ARKD_ALLOW_CSV_BLOCK_TYPE]
   --heartbeat-interval value                              Heartbeat interval in seconds (default: 60) [$ARKD_HEARTBEAT_INTERVAL]
   --round-report-enabled                                  Enable round report service (default: false) [$ARKD_ROUND_REPORT_ENABLED]
   --settlement-min-expiry-gap value                       (default: 0 disabled) [$ARKD_SETTLEMENT_MIN_EXPIRY_GAP]
   --vtxo-no-csv-validation-cutoff-date value              (default: 0 disabled) [$ARKD_VTXO_NO_CSV_VALIDATION_CUTOFF_DATE]
   --onchain-output-fee value                              (default: 0) [$ARKD_ONCHAIN_OUTPUT_FEE]
   --alert-manager-url value                                [$ARKD_ALERT_MANAGER_URL]
   --help, -h                                              show help
```

Closes arkade-os#782

Signed-off-by: Gustavo Chain <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
README.md (1)

176-179: Start command example is consistent; consider hinting at --help for flags

Switching the setup step to arkd start matches the new dedicated start subcommand while arkd remains the default action, so this example looks good.

Given this PR’s goal of surfacing flags via CLI help, you might optionally add a short note near the “Configuration Options” table that arkd start --help prints the full list of flags, defaults and env vars, so operators learn to rely on the CLI as the source of truth.

internal/config/config.go (2)

192-553: Centralized CLI/env flag definitions look solid

The new env() helper plus the typed *cli.Flag declarations give a clear, maintainable mapping between --flags, defaults, and ARKD_* environment variables. Grouping everything into the exported Flags slice is a nice way to keep cmd/arkd thin and avoid config drift.

For the internal/advanced flags where Usage is intentionally empty (e.g., SettlementMinExpiryGap, VtxoNoCsvValidationCutoffDate, OnchainOutputFee, AlertManagerURL, EnablePprof), consider adding at least a one‑line description later so arkd start --help is self-explanatory even without consulting source. This can be done incrementally as those options are documented.


613-724: Minor cleanup: redundant dbUrl assignment in LoadConfig

In the postgres branch of the DB config you assign dbUrl twice from the same source:

if c.String(DbType.Name) == "postgres" {
    dbUrl = c.String(DbUrl.Name)
    dbUrl = c.String(DbUrl.Name)
    if dbUrl == "" {
        return nil, fmt.Errorf("db type set to 'postgres' but db url is missing")
    }
}

The second assignment is redundant and can be dropped without changing behavior:

-   if c.String(DbType.Name) == "postgres" {
-       dbUrl = c.String(DbUrl.Name)
-       dbUrl = c.String(DbUrl.Name)
+   if c.String(DbType.Name) == "postgres" {
+       dbUrl = c.String(DbUrl.Name)
        if dbUrl == "" {
            return nil, fmt.Errorf("db type set to 'postgres' but db url is missing")
        }
    }

Purely a readability/cleanup nit; runtime behavior is already correct.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7afe52a and 00c5503.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • cmd/arkd/commands.go (2 hunks)
  • cmd/arkd/main.go (1 hunks)
  • go.mod (1 hunks)
  • internal/config/config.go (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T08:12:00.266Z
Learnt from: altafan
Repo: arkade-os/arkd PR: 659
File: .github/workflows/release.yaml:183-201
Timestamp: 2025-07-08T08:12:00.266Z
Learning: The arkd wallet Dockerfile in the arkade-os/arkd repository is named `arkdwallet.Dockerfile` (without hyphens), not `arkd-wallet.Dockerfile`.

Applied to files:

  • README.md
🧬 Code graph analysis (3)
cmd/arkd/main.go (1)
internal/config/config.go (1)
  • LoadConfig (613-725)
cmd/arkd/commands.go (1)
internal/config/config.go (1)
  • Flags (555-611)
internal/config/config.go (2)
pkg/arkd-wallet/config/config.go (8)
  • Datadir (25-25)
  • Port (24-24)
  • LogLevel (26-26)
  • OtelCollectorEndpoint (30-30)
  • OtelPushInterval (31-31)
  • PyroscopeServerURL (32-32)
  • LoadConfig (41-82)
  • Config (84-97)
internal/interface/grpc/config.go (1)
  • Config (13-23)
🔇 Additional comments (3)
cmd/arkd/commands.go (1)

12-25: Wiring start command to config.Flags looks correct

Importing internal/config here and delegating startCmd.Flags to config.Flags is a clean way to centralize configuration flags and expose them via arkd start --help without touching other commands.

cmd/arkd/main.go (1)

26-31: Passing the CLI context into LoadConfig aligns correctly with the new flag model

Updating startAction to accept c *cli.Context and calling config.LoadConfig(c) is the right way to wire the new CLI-based configuration into startup; the rest of the boot sequence remains unchanged and backwards-compatible.

go.mod (1)

183-183: Viper correctly marked as indirect; verification confirms tidy state

Reclassifying github.com/spf13/viper as an indirect dependency is consistent with the server moving to urfave/cli/v2-based config. Verification confirms:

  • No direct viper imports remain in cmd/ or internal/
  • go.mod is tidy (no changes needed)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
cmd/arkd/commands.go (1)

12-26: Wiring start flags through config.Flags looks good; verify UX around global vs command flags

Using config.Flags on startCmd cleanly centralizes runtime configuration and makes arkd start --help the single place to see all server flags. That aligns well with the PR goal.

One thing to double‑check: with config flags now scoped to the start command (and only urlFlag, datadirFlag, macaroonFlag kept as app‑level globals in main.go), invocations that previously relied on global config flags like arkd --port=... may now require arkd start --port=.... If backward compatibility with arkd --FLAG is important, you may want to confirm real‑world usages and, if needed, mirror config.Flags into app‑level flags or document the change explicitly.

internal/config/config.go (4)

192-279: Env helper and basic flag wiring are clean; verify env-name compatibility with previous viper setup

Using an env() helper to prefix all env var names with ARKD_ keeps the flag definitions concise and makes it easy to see the mapping between CLI flags and env vars. The basic flags here (Datadir, Port, AdminPort, LogLevel, SessionDuration, DbType, DbUrl, EventDbType, EventDbUrl, TxBuilderType, LiveStoreType, RedisUrl, RedisTxNumOfRetries) look consistent with the Config fields and with the PR description.

One thing worth double‑checking against the old viper config is that env variable names remain fully backward compatible (e.g., PG_DB_URL vs any historic DB_URL, PG_EVENT_DB_URL, etc.). A mismatch here would silently break existing deployments that rely purely on env vars.


280-409: Duration‑like numeric flags are wired correctly; consider migrating to typed duration/timestamp flags over time

The numeric timing flags (VtxoTreeExpiry, UnilateralExitDelay, PublicUnilateralExitDelay, BoardingExitDelay) and amount limits (Utxo*/Vtxo*) are all declared with types consistent with how they’re consumed in LoadConfig and Validate (converted via c.Int64(...) and then fed into determineLocktimeType or stored as int64s).

Given the existing TODOs, it would be a worthwhile follow‑up (not blocking this PR) to gradually move these to cli.DurationFlag/cli.TimestampFlag where appropriate, to avoid users having to manually reason about “seconds” vs “blocks” and to align more closely with the time semantics already enforced in Validate().


410-552: Scheduler/round/telemetry/advanced flags look consistent; document the intentionally “undocumented” ones

The rest of the flag block (ban, scheduler, TLS, note URI, scheduled session, telemetry, CSV/block, heartbeat, round report, settlement gap, cutoff date, onchain fee, alerts, pyroscope, pprof) appears to be in 1:1 correspondence with fields on Config and with the logic in Validate() and the various *Service() methods later in the file.

You’ve explicitly left Usage: "" on some advanced/experimental flags (e.g., SettlementMinExpiryGap, VtxoNoCsvValidationCutoffDate, OnchainOutputFee, AlertManagerURL, PyroscopeServerURL, EnablePprof), which is reasonable to keep help output terse. Just ensure this is deliberate and captured somewhere (issue or internal docs), so future maintainers know these are semi‑internal knobs and not an omission.


613-723: LoadConfig should match wallet config pattern: explicitly create dbPath directory or add clarifying comment

Verification confirms the discrepancy: wallet configs (pkg/arkd-wallet/config/config.go:61) explicitly call makeDirectoryIfNotExists(dbPath) after loading, but LoadConfig at lines 613–723 does not.

However, testing shows all supported DB backends handle this automatically:

  • Badgerhold (badger backend): automatically creates the directory if missing
  • SQLite (sqlite backend): explicitly creates via os.MkdirAll in its initialization
  • Postgres (postgres backend): uses DbUrl, no directory needed

While not a functional bug, consider either:

  • Adding if err := makeDirectoryIfNotExists(dbPath); err != nil { return nil, err } after line 618 to match the wallet config pattern for consistency, or
  • Adding a code comment explaining that backend initialization handles directory creation

The explicit approach in wallet configs (pkg/arkd-wallet/config/config.go:61) is clearer for future maintainers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00c5503 and 429468c.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • cmd/arkd/commands.go (2 hunks)
  • cmd/arkd/main.go (1 hunks)
  • go.mod (1 hunks)
  • internal/config/config.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • README.md
  • go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/arkd/main.go (1)
internal/config/config.go (1)
  • LoadConfig (613-724)
cmd/arkd/commands.go (1)
internal/config/config.go (1)
  • Flags (555-611)
internal/config/config.go (3)
pkg/arkd-wallet/config/config.go (8)
  • Datadir (25-25)
  • Port (24-24)
  • LogLevel (26-26)
  • OtelCollectorEndpoint (30-30)
  • OtelPushInterval (31-31)
  • PyroscopeServerURL (32-32)
  • LoadConfig (41-82)
  • Config (84-97)
pkg/arkd-wallet-btcwallet/config/config.go (6)
  • Datadir (18-18)
  • Port (17-17)
  • LogLevel (19-19)
  • EsploraURL (21-21)
  • LoadConfig (38-86)
  • Config (88-102)
internal/interface/grpc/config.go (1)
  • Config (13-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: integration tests
🔇 Additional comments (3)
cmd/arkd/main.go (1)

26-32: Config loading via cli.Context is correct; confirm where cfg.Validate() is called

Passing the *cli.Context into config.LoadConfig(c) is the right way to bind the new flag set to the server config, and the error wrapping remains clear.

Given that Config.Validate() is responsible for both validation and wiring of internal services (DB, wallet, scheduler, etc.), please confirm that it is still invoked somewhere in the startup path (either inside LoadConfig, here in startAction, or within grpcservice.NewService). If it’s no longer called, misconfigurations might only fail later at runtime instead of up front during startup.

internal/config/config.go (2)

555-611: Central Flags slice cleanly exposes the server configuration surface

The Flags slice correctly aggregates all the runtime server configuration flags that LoadConfig expects. This is exactly what cmd/arkd/commands.go needs to wire startCmd.Flags = config.Flags and is a nice single source of truth for future additions.

When adding new config fields, this makes it straightforward to ensure they:

  1. Have a flag with env binding, and
  2. Are referenced in LoadConfig and (if needed) in Validate().

No changes needed here; just keep this list and the Config struct in sync going forward.


726-736: Datadir initialization via cli.Context is straightforward and sufficient

initDatadir(c) now derives the path from the Datadir flag/env and ensures it exists using makeDirectoryIfNotExists. That’s a clean replacement for the old viper‑based approach and keeps all configuration flowing through the CLI context.

No changes needed here; just ensure any future logic that depends on additional subdirectories (like db/) either calls makeDirectoryIfNotExists or is handled by the downstream component.

Copy link
Member

@tiero tiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not against per se, but better to not conflate style and functional changes in the same commit for better trail. Even if it means having three small commits around the same code area

func makeDirectoryIfNotExists(path string) error {
if _, err := os.Stat(path); os.IsNotExist(err) {
return os.MkdirAll(path, os.ModeDir|0755)
return os.MkdirAll(path, os.ModeDir|0o755)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would separate this into his own commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, this actually made by my linter automatically, I wonder if it's best to remove from this PR as it is not related.

EnablePprof,
}

func LoadConfig(c *cli.Context) (*Config, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why moving into using c.String instead of viper.GetString?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viper.GetString will get the environment variable only as the flag has not being defined by viper it self. On the other hand, when we c.String we let urfave/cli to retrieve the value which can be defined as a flag (--flag) or as an environment variable. Note that urfave/cli will prioritize flags and fallback to env vars.

@altafan
Copy link
Collaborator

altafan commented Dec 5, 2025

@qustavo please solve conflicts.

I'd prefer to refactor the cli and use cobra instead of urfave, rather than refactoring config and remove viper as the former is much less "sensible" code than the latter.

@qustavo
Copy link
Contributor Author

qustavo commented Dec 6, 2025

@qustavo please solve conflicts.

I'd prefer to refactor the cli and use cobra instead of urfave, rather than refactoring config and remove viper as the former is much less "sensible" code than the latter.

I understand your point, so I made a branch to try it out and see how it would look to keep cobra instead of urfave/cli. Given your concern and to minimize the impact of such a change, master...qustavo:arkd:move_start_cmd_to_cobra implements a hybrid approach, where I only migrated the start sub command to cobra (in the future all should be consistent and moved to cobra).

Before moving on and create a PR, please have a look and think if this is an approach that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve main action flag visibility

3 participants