Skip to content

feat(nodectl): settings mutations via REST API#91

Merged
Keshoid merged 7 commits into
release/nodectl/v0.4.0from
feature/sma-70-centralize-config-management-part3
Apr 15, 2026
Merged

feat(nodectl): settings mutations via REST API#91
Keshoid merged 7 commits into
release/nodectl/v0.4.0from
feature/sma-70-centralize-config-management-part3

Conversation

@Keshoid
Copy link
Copy Markdown
Contributor

@Keshoid Keshoid commented Apr 15, 2026

Summary

Part 3 of centralizing config management. Moves all settings mutation commands to REST API endpoints.

  • Unified POST /v1/elections/settings replaces /v1/stake_strategy, individual tick-interval and max-factor endpoints. Accepts any combination of policy, tick_interval, max_factor in one request.
  • Unified POST /v1/ton-http-api with append flag replaces separate set/add endpoints
  • POST /v1/log for log settings mutation
  • Added save_to_file() to elections exclude/include handlers for persistence consistency
  • Removed config stake-policy CLI alias (use config elections stake-policy)
  • Service loop rebuilds caches (wallets, pools, RPC client) immediately on entity/endpoint mutations via Notify + force_reload()

Breaking changes

  • config ton-http-api: renamed --url option to --endpoint (disambiguates from the root config --url option). Update any scripts/automation invoking nodectl config ton-http-api --url ... to use --endpoint.

CI script

  • Moved config ton-http-api set from phase 2 (before service) to phase 8 (after service + auth). Service starts with default URL from config generate.

Notes

  • Linear: SMA-70

Closes SMA-70

Keshoid added 3 commits April 15, 2026 14:33
- Unified POST /v1/elections/settings for stake-policy, tick-interval, max-factor
- Unified POST /v1/ton-http-api with append flag for set/add
- POST /v1/log for log settings
- Added save_to_file to elections exclude/include handlers
- Removed config stake-policy alias, moved vault_secret_missing to utils
- CI: moved ton-http-api set from phase 2 to phase 8
…ions

Entity CRUD and ton-http-api handlers signal config_changed after
save_to_file so the service loop calls force_reload to rebuild
wallets, pools, RPC client and restart tasks immediately.
Copilot AI review requested due to automatic review settings April 15, 2026 11:49
@linear
Copy link
Copy Markdown

linear Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Migrates multiple nodectl config “settings mutation” commands (elections, log, ton-http-api, include/exclude) from direct file edits to REST API calls, and wires REST-side structural mutations to trigger cache rebuilds so changes take effect immediately.

Changes:

  • Add REST mutation endpoints for elections settings, ton-http-api, and log; migrate old stake-strategy endpoint to elections settings.
  • Introduce config_changed notification path so structural REST mutations trigger RuntimeConfigStore::force_reload() and task restarts.
  • Consolidate mutation persistence by switching remaining update_with/save_to_file call sites to update_and_save.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/node/tests/test_run_net_py/run_singlehost_nodectl.py Moves ton-http-api set to a phase where REST/auth is available.
src/node-control/service/src/service_main_task.rs Adds config_changed Notify and force_reload + restart on structural REST mutations.
src/node-control/service/src/runtime_config.rs Adds force_reload() helper; continues migration toward update_and_save.
src/node-control/service/src/http/http_server_task.rs Replaces stake-strategy route with elections settings mutation route; updates tests and uses update_and_save.
src/node-control/service/src/http/config_handlers.rs Implements REST mutation handlers for elections settings, ton-http-api, and log; emits config_changed for structural changes.
src/node-control/service/src/http/auth_tests.rs Updates auth tests to target new elections settings mutation endpoint and adds config_changed to test state.
src/node-control/commands/src/commands/nodectl/utils.rs Adds shared vault_secret_missing helper for CLI commands.
src/node-control/commands/src/commands/nodectl/service_api_cmd.rs Updates stake policy mutation to call /v1/elections/settings with a mirrored request DTO.
src/node-control/commands/src/commands/nodectl/config_wallet_cmd.rs Reuses shared vault helper; removes local duplicate.
src/node-control/commands/src/commands/nodectl/config_ton_http_api_cmd.rs Migrates ton-http-api set/add to REST calls.
src/node-control/commands/src/commands/nodectl/config_node_cmd.rs Reuses shared vault helper; removes local duplicate.
src/node-control/commands/src/commands/nodectl/config_log_cmd.rs Migrates log set to REST call and renders response.
src/node-control/commands/src/commands/nodectl/config_elections_cmd.rs Migrates elections mutations (policy/tick/max-factor/include/exclude) to REST calls.
src/node-control/commands/src/commands/nodectl/config_cmd.rs Removes legacy stake-policy shortcut and routes ton-http-api/log/elections mutations through REST-aware commands.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +136 to +141
if let Err(e) = runtime_cfg.force_reload().await {
tracing::error!("cache rebuild after config mutation failed: {e:#}");
}
for task in tasks.values() {
let _ = task.restart().await;
}
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

In the config_changed branch, tasks are restarted even when runtime_cfg.force_reload() fails. Since update_and_save mutates only the config snapshot (not the cached wallets/pools/RPC client), a failed force_reload leaves caches inconsistent with the new config; restarting tasks in that state can cause hard-to-debug runtime errors. Consider restarting tasks only after a successful force_reload, and otherwise leaving tasks running on the previous caches (and/or reverting the config change / returning an error to the caller).

Suggested change
if let Err(e) = runtime_cfg.force_reload().await {
tracing::error!("cache rebuild after config mutation failed: {e:#}");
}
for task in tasks.values() {
let _ = task.restart().await;
}
match runtime_cfg.force_reload().await {
Ok(()) => {
for task in tasks.values() {
let _ = task.restart().await;
}
}
Err(e) => {
tracing::error!(
"cache rebuild after config mutation failed; skipping task restart: {e:#}"
);
}
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +1234 to +1245
if append {
let existing = cfg.ton_http_api.endpoints();
for url in &urls {
if !existing.iter().any(|e| e == url) {
let entry = match &api_key {
Some(key) => {
EndpointEntry::WithKey { url: url.clone(), api_key: key.clone() }
}
None => EndpointEntry::Url(url.clone()),
};
cfg.ton_http_api.urls.push(entry);
}
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

In append mode, existing is computed once before the loop, so duplicates within the incoming urls list (or duplicates added earlier in the loop) can still be pushed into cfg.ton_http_api.urls. This can bloat the stored config even though endpoints() dedupes for display. Consider tracking a mutable set of seen URLs (initialized from resolved_endpoints()/endpoints()) and inserting into it as you push new entries.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +1286 to +1293
let level = req
.level
.as_deref()
.map(|l| {
tracing::Level::from_str(l)
.map_err(|_| AppError::bad_request(format!("invalid log level: '{l}'")))
})
.transpose()?;
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

req.level is parsed as-is; values with leading/trailing whitespace (e.g. " info ") will be rejected even though they’re semantically valid. Consider trimming the string before Level::from_str to make the API more robust.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +1295 to +1301
// Pre-validate: file/all output requires a path (check against current + incoming)
if let Some(ref output) = req.output {
if matches!(output, LogOutput::File | LogOutput::All) {
let has_path = req.path.is_some()
|| state.runtime_cfg.get().log.as_ref().and_then(|l| l.path.as_ref()).is_some();
if !has_path {
let mode = match output {
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

The "output requires a path" pre-validation treats req.path.is_some() as sufficient, but an empty string path would pass this check and later be persisted as PathBuf(""). Consider trimming/validating req.path and treating empty/whitespace-only values as unset (or returning 400).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +357 to 371
/// Client-side mirror of `ElectionsSettingsUpdateRequest` in `service::http::config_handlers`.
/// Must stay in sync with the server-side definition.
#[derive(Clone, Default, serde::Serialize)]
struct ElectionsSettingsRequest {
#[serde(skip_serializing_if = "Option::is_none")]
policy: Option<StakePolicy>,
#[serde(skip_serializing_if = "Option::is_none")]
node: Option<String>,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
reset: bool,
#[serde(skip_serializing_if = "Option::is_none")]
tick_interval: Option<u64>,
#[serde(skip_serializing_if = "Option::is_none")]
max_factor: Option<f32>,
}
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

There are now multiple client-side request structs mirroring the same server DTO (ElectionsSettingsUpdateRequest) across commands (e.g. here and in config_elections_cmd.rs). This is easy to let drift over time. Consider defining a single shared request type in one module (or a common crate) and reusing it for all callers of /v1/elections/settings.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — this is real duplication. Tracking as a follow-up in v0.5.0: a refactor to extract shared REST request/response DTOs into a common module reused by both the service handlers and the CLI commands. Out of scope for this PR.

@Keshoid Keshoid merged commit 61dfa32 into release/nodectl/v0.4.0 Apr 15, 2026
5 of 7 checks passed
@Keshoid Keshoid deleted the feature/sma-70-centralize-config-management-part3 branch April 15, 2026 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants