feat(nodectl): TONCore per-slot details in 'config pool ls' + REST add#95
Conversation
There was a problem hiding this comment.
Pull request overview
Enhances node-control pool management by exposing TONCore pools as per-slot entries (even/odd) with on-chain details in both config pool ls and GET /v1/pools, and adds a dedicated REST endpoint to create TONCore pool slots (POST /v1/pools/core) so CLI no longer edits local config directly.
Changes:
- Update
GET /v1/poolsto return TONCore pools asslots[]with per-slot state/balance and on-chain pool parameters fetched in parallel. - Add
POST /v1/pools/coreREST handler with validation to add/create TONCore pool slots. - Update
nodectl config pool add coreto call the new REST endpoint and updateconfig pool lsoutput formatting; add focused HTTP handler unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/node-control/service/src/http/mod.rs |
Registers new HTTP handler test module. |
src/node-control/service/src/http/http_server_task.rs |
Wires new /v1/pools/core route and adds DTOs/requests to OpenAPI schema. |
src/node-control/service/src/http/config_handlers_tests.rs |
Adds unit tests covering TONCore per-slot listing behavior and /v1/pools/core validation/mutation paths. |
src/node-control/service/src/http/config_handlers.rs |
Implements TONCore per-slot DTOs, parallel slot fetching for /v1/pools, and the /v1/pools/core handler. |
src/node-control/commands/src/commands/nodectl/config_pool_cmd.rs |
Switches TONCore “add” to REST, and renders pool listing as separate SNP and TONCore tables with per-slot rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Ok(slot) = joined { | ||
| slots.push(slot); |
There was a problem hiding this comment.
JoinSet::join_next() errors are currently ignored (if let Ok(slot) = joined { ... }). If a task panics/cancels, the corresponding slot silently disappears from the response, breaking the “one entry per configured slot” invariant. Handle Err(join_error) by pushing a slot DTO with state="error" (and slot id/address if known) or by converting the join error into an explicit handler error.
| if let Ok(slot) = joined { | |
| slots.push(slot); | |
| match joined { | |
| Ok(slot) => slots.push(slot), | |
| Err(join_error) => return Err(join_error.into()), |
There was a problem hiding this comment.
That joined is the JoinHandle result — it only errors if the spawned task panicked (a Tokio join error), not if RPC failed.
| ) -> TonCorePoolSlotDto { | ||
| let slot_name = if slot_idx == 0 { "even" } else { "odd" }; | ||
| let address = match &cached { | ||
| Some(w) => w.address().await.ok(), |
There was a problem hiding this comment.
Address resolution prioritizes the cached wrapper, but if w.address().await fails it does not fall back to the config address (it just returns None and the slot becomes not deployed). Consider attempting to parse/use config_address when the cached wrapper can’t provide an address, so transient cache/wrapper errors don’t misclassify a deployed slot.
| Some(w) => w.address().await.ok(), | |
| Some(w) => w | |
| .address() | |
| .await | |
| .ok() | |
| .or_else(|| config_address.as_deref().and_then(|a| MsgAddressInt::from_str(a).ok())), |
| if let Ok(d) = wrapper.get_pool_data().await { | ||
| dto.validator_share = Some(d.pool_config.validator_reward_share); | ||
| dto.max_nominators = Some(d.pool_config.max_nominators_count); | ||
| dto.min_validator_stake = Some(d.pool_config.min_validator_stake); | ||
| dto.min_nominator_stake = Some(d.pool_config.nominator_stake_threshold); | ||
| dto.nominators_count = Some(d.nominators_count); | ||
| dto.stake_amount_sent = Some(d.stake_amount_sent); | ||
| dto.validator_amount = Some(d.validator_amount); | ||
| dto.pool_state = Some(d.state); | ||
| dto.last_election_id = Some(d.stake_at); |
There was a problem hiding this comment.
If get_pool_data() fails for an active slot, the DTO is returned with state="active" but with all on-chain parameter fields left empty, which is indistinguishable from “pool data unavailable”. Since the PR goal is to encode per-slot RPC failures into the slot DTO, consider setting state="error" (or adding an explicit error field) when get_pool_data() errors so clients can reliably detect partial failures.
| if let Ok(d) = wrapper.get_pool_data().await { | |
| dto.validator_share = Some(d.pool_config.validator_reward_share); | |
| dto.max_nominators = Some(d.pool_config.max_nominators_count); | |
| dto.min_validator_stake = Some(d.pool_config.min_validator_stake); | |
| dto.min_nominator_stake = Some(d.pool_config.nominator_stake_threshold); | |
| dto.nominators_count = Some(d.nominators_count); | |
| dto.stake_amount_sent = Some(d.stake_amount_sent); | |
| dto.validator_amount = Some(d.validator_amount); | |
| dto.pool_state = Some(d.state); | |
| dto.last_election_id = Some(d.stake_at); | |
| match wrapper.get_pool_data().await { | |
| Ok(d) => { | |
| dto.validator_share = Some(d.pool_config.validator_reward_share); | |
| dto.max_nominators = Some(d.pool_config.max_nominators_count); | |
| dto.min_validator_stake = Some(d.pool_config.min_validator_stake); | |
| dto.min_nominator_stake = Some(d.pool_config.nominator_stake_threshold); | |
| dto.nominators_count = Some(d.nominators_count); | |
| dto.stake_amount_sent = Some(d.stake_amount_sent); | |
| dto.validator_amount = Some(d.validator_amount); | |
| dto.pool_state = Some(d.state); | |
| dto.last_election_id = Some(d.stake_at); | |
| } | |
| Err(_) => { | |
| dto.state = "error".to_string(); | |
| } |
There was a problem hiding this comment.
added log on error, but logic is the same.
| match cfg.pools.get_mut(&name) { | ||
| Some(PoolConfig::TONCore { pools }) => { | ||
| pools[slot_idx] = Some(slot_cfg); | ||
| } | ||
| Some(PoolConfig::SNP { .. }) => { | ||
| // Pre-validated above; race-narrow window only — leave | ||
| // the existing entry untouched. | ||
| } |
There was a problem hiding this comment.
Inside update_and_save, the Some(PoolConfig::SNP { .. }) branch is intentionally a no-op (for a “race-narrow window”), but the handler still returns 200 OK even though it didn’t apply the requested change. To avoid misleading success responses, capture a flag inside the closure (e.g., Arc<AtomicBool>) to confirm the mutation happened and return a 409/400 if it didn’t, or re-check the config after the update and error if the slot wasn’t set.
| name: &self.name, | ||
| slot: slot_name, | ||
| address: address.as_deref(), | ||
| validator_share: self.validator_share, |
There was a problem hiding this comment.
I would suggest adding validation to the handler for max_nominators (more than 1 and less than 40) and validator_share (no more than 10000)
There was a problem hiding this comment.
This check is implemented on server-side, but I also added in cli.
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub min_nominator_stake: Option<u64>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub nominators_count: Option<u32>, |
There was a problem hiding this comment.
mb not no, but not all of params have the same types as in contract (for example, nominators count is u16 in contract,state is u8 and so on) . It comes from the first wrapper version, but we don't have any problems because of it. Not urgent
| pub name: String, | ||
| /// `"SNP"` or `"Core"` — selects which of the field groups below applies. | ||
| pub kind: String, | ||
| /// SNP only: on-chain balance in nanotons. Always `None` for TONCore. |
There was a problem hiding this comment.
but toncore has a balance too
There was a problem hiding this comment.
mb add a little bit more information to avoid misunderstanding?
| dto.stake_amount_sent = Some(d.stake_amount_sent); | ||
| dto.validator_amount = Some(d.validator_amount); | ||
| dto.pool_state = Some(d.state); | ||
| dto.last_election_id = Some(d.stake_at); |
There was a problem hiding this comment.
Why? stake_at in the pool contract storage is the election ID — it records which election round the pool staked at.
The DTO doc comment makes the mapping explicit:
/// Last election id the pool staked at (stake_at in pool storage).
| path = "/v1/pools/core", | ||
| request_body = PoolAddCoreRequest, | ||
| responses( | ||
| (status = 200, description = "TONCore pool slot added", body = EntityRefResponse), |
There was a problem hiding this comment.
maybe 201? 200 - success, 201 usually means "successfully created"
| } | ||
| if let Some(mn) = req.max_nominators { | ||
| if mn == 0 { | ||
| return Err(AppError::bad_request("max_nominators must be > 0")); |
…-ls-toncore-pools-display-is-incomplete
config pool lsandGET /v1/poolsnow render TONCore pools as a per-slot table with state (active / uninit / not deployed / error), balance, and on-chain pool params (validator share, max nominators, min stakes, nominators count, staked amount, pool state, last election id). Slot fetches run in parallel; per-slot RPC failures are encoded into the slot DTO instead of failing the whole response. SNP shape unchanged.Also adds
POST /v1/pools/coresoconfig pool add coregoes through REST like the rest of v0.4.0 entity CRUD instead of writing the local config file. Validatesvalidator_share(0..=10000 bp),max_nominators > 0, sibling deploy params requirevalidator_share, rejects SNP-name collisions and already-configured slots.Closes SMA-78