God Object - IndexerConfig mixes concerns across multiple domains

[nachog00]

God Object - IndexerConfig mixes concerns across multiple domains

Labels: configs, tech-debt, architecture

Problem

IndexerConfig is a God Object that violates the Single Responsibility Principle by mixing configuration for entirely different subsystems in a single struct.

Current Problematic Code

pub struct IndexerConfig {
    // Backend selection
    pub backend: zaino_state::BackendType,
    
    // JSON-RPC server settings
    pub enable_json_server: bool,
    pub json_rpc_listen_address: SocketAddr,
    pub enable_cookie_auth: bool,
    pub cookie_dir: Option<PathBuf>,
    
    // gRPC server settings  
    pub grpc_listen_address: SocketAddr,
    pub grpc_tls: bool,
    pub tls_cert_path: Option<String>,
    pub tls_key_path: Option<String>,
    
    // Validator connection settings
    pub validator_listen_address: SocketAddr,
    pub validator_grpc_listen_address: SocketAddr,
    pub validator_cookie_auth: bool,
    pub validator_cookie_path: Option<String>,
    pub validator_user: Option<String>,
    pub validator_password: Option<String>,
    
    // Cache settings
    pub map_capacity: Option<usize>,
    pub map_shard_amount: Option<usize>,
    
    // Database settings
    pub zaino_db_path: PathBuf,
    pub zebra_db_path: PathBuf,
    pub db_size: Option<usize>,
    
    // Network and debug
    pub network: String,
    pub no_sync: bool,
    pub no_db: bool,
    pub slow_sync: bool,
}

Impact

• Unclear Responsibilities: What is IndexerConfig actually responsible for?
• Change Amplification: Adding a new server option affects the "main" config
• Documentation Overload: Single struct needs to document 6+ different concepts
• Testing Complexity: Can't test server config without creating entire IndexerConfig
• Poor Cohesion: Fields that change together are scattered
• Hidden Dependencies: Which fields are used together isn't clear

Solution

Split into focused, domain-specific configuration objects:

// Top-level config just orchestrates
pub struct IndexerConfig {
    pub network: Network,
    pub backend: BackendConfig,
    pub server: ServerConfig,
    pub storage: StorageConfig,
    pub debug: DebugConfig,
}

// Server owns its configuration
pub struct ServerConfig {
    pub grpc: GrpcConfig,
    pub json_rpc: Option<JsonRpcConfig>,  // Optional server
}

pub struct GrpcConfig {
    pub listen_address: SocketAddr,
    pub tls: TlsConfig,
}

pub struct JsonRpcConfig {
    pub listen_address: SocketAddr,
    pub auth: JsonRpcAuth,
}

// Backend config variants encode valid combinations
pub enum BackendConfig {
    LocalZebra {
        rpc_address: SocketAddr,
        grpc_address: SocketAddr,
        auth: ZebradAuth,
        state_config: ZebraStateConfig,
    },
    RemoteZcashd {
        rpc_address: SocketAddr,
        auth: ZcashdAuth,
    },
    // Each variant has exactly what it needs
}

// Storage is its own concern
pub struct StorageConfig {
    pub cache: Option<CacheConfig>,
    pub database: DatabaseConfig,
}

Usage becomes clearer:

# TOML structure mirrors the code structure
network = "mainnet"

[backend]
type = "local_zebra"
rpc_address = "127.0.0.1:8232"
grpc_address = "127.0.0.1:9000"
auth = { cookie = { path = ".cookie" }}

[server.grpc]
listen_address = "0.0.0.0:8080"
tls = { enabled = { cert = "cert.pem", key = "key.pem" }}

[server.json_rpc]
listen_address = "0.0.0.0:8545"
auth = "disabled"

[storage.database]
path = "./zaino_db"
size = { gb = 10 }

Benefits:

• Each struct has a single, clear responsibility
• Related fields are grouped together
• Can test/modify subsystems independently
• Documentation is focused and relevant
• Changes are localized to affected subsystem
• Configuration structure mirrors system architecture

[idky137]

Overall this looks good. I think the backend config could do with some tweaking.

The zebra option looks good, it the zcashd option that I think isn't quite correct. In a few months this option will be purely backed by zainod, this isn't really a remote option, we added some hacks so it can be run remote to the validator but it is not a practice we want to promote, also there are a lot of local processes that run with this option that make it not a true remote option.

The remote backend will be the grpc backend added in milestone 3 of the zaino completion grant. This will Fe a full remote backend (minimal local processes).

I think this field should also make note of the actual technology powering each option as this is an important distinction.

Eg. ReadStateService, JsonRPC, gRPC.

[nachog00]

well, we can work on the namings of course. The key point of the backendconfig enum is we enumerate the possible combinations of validators+(state/fetch)... i tried to encode state/fetch as local vs remotes, as it sounded a bit more user friendly. Again, we can figure out the names anyways, but the key aspect it we get just what we need on each variant.

regarding the backing tech, the idea is for each variant to inherently come with a certain set of those. I would add it as documentation of each variant. Having it on the name could make it cluttered... but we should explore it anyways.

[idky137]

My point is that I think Zaino and Zcashd use different auth (with Zainod following Zebrad, not Zcashd) So the variant you have currently called "ZcashAuth" may actually fail when running with zaino (the intended backend for that choice).

So as much as we do need to make sure names are correct we also need to make sure we are not locking functionality with these changes.

[AloeareV]

(sidenote: is LocalZebra supposed to correspond to a StateService backend? If so, there exists the possibility for a FetchService-based Zebra backend, and we should reflect that. I have in this example)

What about something like

pub enum BackendConfig {
    LocalZebra {
        rpc_address: SocketAddr,
        grpc_address: SocketAddr,
        auth: ZebradAuth,
        state_config: ZebraStateConfig,
    },
    Remote {
        rpc_address: SocketAddr,
        backend: RemoteBackendConfig,
    },
    // Each variant has exactly what it needs
}

pub enum RemoteBackendConfig {
    Zcash { auth: ZcashAuth }, // These are only single fields at the moment, is authorization the only way in which
                               // different fetch backends will differ in config?
    Zebra { auth: ZebraAuth },
    Zaino { auth: ZebraAuth }, // Is Zainod's auth expected to be completely identical to Zebrad's?
}

[idky137]

This looks good.

There is a decision we need to make with this though.

Previously the implementation plans we have had would lead to a point that zebrad would no longer be able to serve the fetchservice as some of the zcash jsonrpc services are being implemented in zaino rather than zebrad (where zcashd used to serve the complete set).

We could implement the fetch service to always be backable by zebrad but I'm not sure what consequences this will have, it might mean that we would have to feature gate some capability out.

This is why I think we need to think whether we want to support zebra in the fetch service.

[AloeareV]

This looks good.

There is a decision we need to make with this though.

Previously the implementation plans we have had would lead to a point that zebrad would no longer be able to serve the fetchservice as some of the zcash jsonrpc services are being implemented in zaino rather than zebrad (where zcashd used to serve the complete set).

We could implement the fetch service to always be backable by zebrad but I'm not sure what consequences this will have, it might mean that we would have to feature gate some capability out.

This is why I think we need to think whether we want to support zebra in the fetch service.

Gotcha. In that case I will go forward based off of the fact that we currently support zebra fetch. As we add features that zebra fetch can't support I suggest that we implement them as some sort of 'operation not supported with zebra-fetch' error (or feature-gate)..we could also remove the fetch-service variant at that point, but given that there are entire categories that are fully supported by fetch service I'm not sold on removing it entirely. That doesn't need to be decided now, though, removing it from the config later will be straight-forward if we go that route.

[nachog00]

My point is that I think Zaino and Zcashd use different auth (with Zainod following Zebrad, not Zcashd) So the variant you have currently called "ZcashAuth" may actually fail when running with zaino (the intended backend for that choice).

So as much as we do need to make sure names are correct we also need to make sure we are not locking functionality with these changes.

Hold on, i just listed two options there on the example, but we can have many. I actually contemplate ZebraState, ZebraFetch, ZcashFetch and ZainoFetch. Each of those can have their own custom {X}Auth variant.

[nachog00]

(sidenote: is LocalZebra supposed to correspond to a StateService backend? If so, there exists the possibility for a FetchService-based Zebra backend, and we should reflect that. I have in this example)

What about something like

pub enum BackendConfig {
LocalZebra {
rpc_address: SocketAddr,
grpc_address: SocketAddr,
auth: ZebradAuth,
state_config: ZebraStateConfig,
},
Remote {
rpc_address: SocketAddr,
backend: RemoteBackendConfig,
},
// Each variant has exactly what it needs
}

pub enum RemoteBackendConfig {
Zcash { auth: ZcashAuth }, // These are only single fields at the moment, is authorization the only way in which
// different fetch backends will differ in config?
Zebra { auth: ZebraAuth },
Zaino { auth: ZebraAuth }, // Is Zainod's auth expected to be completely identical to Zebrad's?
}

This also makes sense. Although one crucial thing to consider is, how the toml will have to look for this config to be parsed. You get some wiggle room with serde tags like "untagged" or "flatten".. but i think the struct i proposed maps to:

[backend]
type = "local_zebra"
rpc_address = "127.0.0.1:8232"
grpc_address = "127.0.0.1:9000"
auth = { cookie = { path = ".cookie" }}
// or
[backend]
type = "remote_zcashd"
rpc_address = "127.0.0.1:8232"
auth = { password = { username = "xxx", password="xxx" }}

and I think this one you propose would be:

[backend]
type = "remote"
rpc_address = "127.0.0.1:8232"

[backend.backend]
type = "Zcash"
auth = { password = { username = "xxx", password="xxx" }}

OR you could have RemoteBackendConfig have the serde "untagged" directive, in which case, there would be no need for the 'type', and the deserialiser would try to parse the passed auth fields into the different variants one by one, until one fits... but that sounds like a poor and implicit way of picking the backing validator... especially because their auth configs could be the same and we would get missmatches.

[idky137]

This looks good.

There is a decision we need to make with this though.

Previously the implementation plans we have had would lead to a point that zebrad would no longer be able to serve the fetchservice as some of the zcash jsonrpc services are being implemented in zaino rather than zebrad (where zcashd used to serve the complete set).

We could implement the fetch service to always be backable by zebrad but I'm not sure what consequences this will have, it might mean that we would have to feature gate some capability out.

This is why I think we need to think whether we want to support zebra in the fetch service.

Gotcha. In that case I will go forward based off of the fact that we currently support zebra fetch. As we add features that zebra fetch can't support I suggest that we implement them as some sort of 'operation not supported with zebra-fetch' error (or feature-gate)..we could also remove the fetch-service variant at that point, but given that there are entire categories that are fully supported by fetch service I'm not sold on removing it entirely. That doesn't need to be decided now, though, removing it from the config later will be straight-forward if we go that route.

Ah I wasn't saying we would completely remove the fetch service, just that at some point unless we change our plans the only server that will be able to serve it will zainod.

But yes sounds good. We can make that choice when we get back to the jsonrpc work.

[idky137]

One other thing I have realised is that we turn off Auth entirely for our tests so we will need a "deactivated" option as well.

[idky137]

(sidenote: is LocalZebra supposed to correspond to a StateService backend? If so, there exists the possibility for a FetchService-based Zebra backend, and we should reflect that. I have in this example)

What about something like

pub enum BackendConfig {
LocalZebra {
rpc_address: SocketAddr,
grpc_address: SocketAddr,
auth: ZebradAuth,
state_config: ZebraStateConfig,
},
Remote {
rpc_address: SocketAddr,
backend: RemoteBackendConfig,
},
// Each variant has exactly what it needs
}

pub enum RemoteBackendConfig {
Zcash { auth: ZcashAuth }, // These are only single fields at the moment, is authorization the only way in which
// different fetch backends will differ in config?
Zebra { auth: ZebraAuth },
Zaino { auth: ZebraAuth }, // Is Zainod's auth expected to be completely identical to Zebrad's?
}

This also makes sense. Although one crucial thing to consider is, how the toml will have to look for this config to be parsed. You get some wiggle room with serde tags like "untagged" or "flatten".. but i think the struct i proposed maps to:

[backend]
type = "local_zebra"
rpc_address = "127.0.0.1:8232"
grpc_address = "127.0.0.1:9000"
auth = { cookie = { path = ".cookie" }}
// or
[backend]
type = "remote_zcashd"
rpc_address = "127.0.0.1:8232"
auth = { password = { username = "xxx", password="xxx" }}

and I think this one you propose would be:

[backend]
type = "remote"
rpc_address = "127.0.0.1:8232"

[backend.backend]
type = "Zcash"
auth = { password = { username = "xxx", password="xxx" }}

OR you could have RemoteBackendConfig have the serde "untagged" directive, in which case, there would be no need for the 'type', and the deserialiser would try to parse the passed auth fields into the different variants one by one, until one fits... but that sounds like a poor and implicit way of picking the backing validator... especially because their auth configs could be the same and we would get missmatches.

Ye maybe if we change the "backend" field to "Auth" so the toml reads"

[Backend.auth]

Instead of

[Backend.backend]

It would look nicer.