Feature/sma 55 add cli commands for adaptive staking parameters#128
Conversation
There was a problem hiding this comment.
Pull request overview
Extends node-control’s CLI and service API to manage adaptive staking/elections timing parameters, improves TONCore pool slot status handling when chain/RPC data is missing, and aligns TONCore validator deposit behavior with the pool’s fixed 1 TON processing fee while documenting the semantics.
Changes:
- Add
sleep_period_pct/waiting_period_pctsupport to elections settings API + CLI commands, including validation and tests. - Improve TONCore pool slot listing to treat uninitialized accounts as “not deployed” and merge deploy params from local config when on-chain reads fail.
- Centralize TONCore
deposit_validatorfixed fee constant and update CLI deposit message value calculation + documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/node-control/service/src/http/http_server_task.rs | Adds HTTP route tests for adaptive elections timing validation and updates. |
| src/node-control/service/src/http/config_handlers_tests.rs | Updates pool slot tests to expect config fallback deploy params in responses. |
| src/node-control/service/src/http/config_handlers.rs | Implements TONCore slot state improvements + config fallback merge; adds elections timing fields to settings DTO/update flow. |
| src/node-control/contracts/src/nominator/messages.rs | Introduces a shared constant for TONCore validator deposit fixed fee and documents message value semantics. |
| src/node-control/common/src/app_config.rs | Makes elections timing validation callable across crates for API-side validation. |
| src/node-control/commands/src/commands/nodectl/service_api_cmd.rs | Extends client-side elections settings update payload to include adaptive timing fields. |
| src/node-control/commands/src/commands/nodectl/config_pool_cmd.rs | Adds --validator-share-percent, adjusts TONCore slot state display, and fixes validator deposit message value to include the fixed fee. |
| src/node-control/commands/src/commands/nodectl/config_elections_cmd.rs | Adds CLI commands to set adaptive sleep/wait timing fractions and displays them in show. |
| src/node-control/README.md | Documents new elections CLI flags and TONCore deposit fee semantics; updates pool add/ls behavior docs. |
Comments suppressed due to low confidence (1)
src/node-control/commands/src/commands/nodectl/config_pool_cmd.rs:532
display_toncore_slot_row()can return the literal "not deployed" which is longer than the{:<10}width used for the "State" column in the TONCore table. This will misalign the table output once undeployed slots are shown. Consider either widening the State column formatting, or shortening the displayed label (e.g. "not-deployed") so the table stays aligned.
for s in slots {
let display_addr = display_ton_address(s.address.as_deref());
let display_state = display_toncore_slot_row(s);
let display_balance =
s.balance.map(|b| display_tons(b).white()).unwrap_or_else(|| "-".red());
let noms = match (s.nominators_count, s.max_nominators) {
(Some(n), Some(m)) => format!("{n}/{m}"),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Keshoid
left a comment
There was a problem hiding this comment.
PR review — 6 issues found (details in inline comments).
…mands-for-adaptive-staking-parameters
Keshoid
left a comment
There was a problem hiding this comment.
Review of code quality / Rust idiom. Most are cleanup; (1) and (3) are the two I'd want addressed before merge.
…mands-for-adaptive-staking-parameters
Keshoid
left a comment
There was a problem hiding this comment.
Second pass — three issues plus two minor wording nits. (1) is a real display bug Copilot flagged in the first round but never got addressed.
Resolve conflicts: combine elections adaptive settings with automation/voting handlers and REST docs; merge HTTP integration tests in order. Co-authored-by: Cursor <cursoragent@cursor.com>
Keshoid
left a comment
There was a problem hiding this comment.
Rust idiom pass — three narrow API/style nits on the new code, all easy fixes.
| } | ||
| } | ||
|
|
||
| fn from_wire(s: &str) -> Result<Self, ()> { |
There was a problem hiding this comment.
Result<Self, ()> → Option<Self>.
() as the error variant is meaningless — there's no information to convey beyond "it failed". The sole caller (deserialize_toncore_pool_slot_wire_state) immediately discards it with .map_err(|_| de::Error::custom(...)), which is the standard tell that the unit error is dead weight.
The idiomatic shape is Option<Self>:
fn from_wire(s: &str) -> Option<Self> {
Some(match s {
"" | "not deployed" => Self::NotDeployed,
"active" => Self::Active,
"frozen" => Self::Frozen,
"error" => Self::Error,
_ => return None,
})
}Caller becomes:
TonCorePoolSlotWireState::from_wire(&s)
.ok_or_else(|| de::Error::custom(format!("unknown TONCore pool slot state: {s:?}")))Drops one Result layer for no information loss.
| Error, | ||
| } | ||
|
|
||
| impl Default for TonCorePoolSlotWireState { |
There was a problem hiding this comment.
The whole Default + Serialize + custom deserializer block (~40 lines) collapses to a 6-line derive.
Since Rust 1.62, #[derive(Default)] works on enums via #[default] on a variant. And serde handles "not deployed" (with the space) via #[serde(rename = ...)]:
#[derive(
Clone, Copy, Debug, Default, PartialEq, Eq,
serde::Serialize, serde::Deserialize,
)]
#[serde(rename_all = "lowercase")]
enum TonCorePoolSlotWireState {
#[serde(rename = "not deployed")]
#[default]
NotDeployed,
Active,
Frozen,
Error,
}Then on the field:
#[serde(default)]
state: TonCorePoolSlotWireState,Drops impl Default, the custom Serialize impl, as_wire / from_wire helpers, and deserialize_toncore_pool_slot_wire_state. The only behavior you lose is the "" → NotDeployed fallback, which is defensive coding for a case the server never produces (it always serializes one of the four wire strings). If you want to keep that, #[serde(alias = "")] on NotDeployed brings it back in one line.
| /// to **`< 10_000`**: a 100 % validator share would leave nominators with no | ||
| /// pool rewards, which is almost always operator error rather than intent. | ||
| fn validate_validator_share_bp(bp: u16) -> anyhow::Result<u16> { | ||
| anyhow::ensure!( |
There was a problem hiding this comment.
First ensure! is subsumed by the second.
bp != 10_000 accepts everything in 0..10_000 ∪ 10_001..=u16::MAX, then bp < 10_000 accepts only 0..10_000. Anything the first rejects (bp == 10_000) the second also rejects — so the first guard only exists for a friendlier error message at that specific value, which is fine intent but reads as dead validation on first scan.
A single match makes the intent obvious and drops one branch:
fn validate_validator_share_bp(bp: u16) -> anyhow::Result<u16> {
match bp {
10_000 => anyhow::bail!(
"--validator-share 10000 bp is 100%: nominators would receive no pool rewards — use a value below 10000 bp"
),
bp if bp > 10_000 => anyhow::bail!(
"validator_share must be in 0..10000 basis points (<100%; got {bp})"
),
bp => Ok(bp),
}
}
Summary
Changes