Skip to content

Comments

P2Pool Config Integration.#9

Closed
R27-pixel wants to merge 0 commit intop2poolv2:mainfrom
R27-pixel:feature/p2pool-config
Closed

P2Pool Config Integration.#9
R27-pixel wants to merge 0 commit intop2poolv2:mainfrom
R27-pixel:feature/p2pool-config

Conversation

@R27-pixel
Copy link
Contributor

P2Pool Config Integration and config tests

@codecov
Copy link

codecov bot commented Jan 4, 2026

Codecov Report

❌ Patch coverage is 97.63092% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/main.rs 95.87% 8 Missing ⚠️
src/components/config_editor.rs 96.71% 5 Missing ⚠️
src/components/legacy_p2pool_parser.rs 85.71% 5 Missing ⚠️
src/ui.rs 98.21% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@R27-pixel R27-pixel marked this pull request as draft January 4, 2026 17:13
@R27-pixel R27-pixel marked this pull request as ready for review January 8, 2026 10:31
Copy link
Contributor

@pool2win pool2win left a comment

Choose a reason for hiding this comment

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

Nice start. Left some comments. We need to use rust's config.rs so we don't end up writing parsing code. Also, ratatui navigation block is becoming too big, please check if we can compose that somehow from sub sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add comments to all functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to p2poolv2_config_parser.rs

Copy link
Contributor

Choose a reason for hiding this comment

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

Use config.rs crate and we won't have to do string split on = to get key/value pairs. We will also get the sections.

See how we do it here: https://github.com/p2poolv2/p2poolv2/blob/main/p2poolv2_lib/src/config.rs

src/main.rs Outdated
// File Selected!
app.bitcoin_conf_path = Some(path);
app.toggle_menu(); // Go back to main screen
if let Some(path) = app.explorer.select().filter(|p| p.is_file()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a fear this navigation is going to become a HUGE case statement. Can we compose this from smaller case statements per section? See if ratatui provides support for this.

Copy link
Contributor

@pool2win pool2win left a comment

Choose a reason for hiding this comment

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

It is a large PR. It'll be really nice to split it into two. UI changes with the refactors you mentioned and one for reading/updating/writing config file.

We also need to clean up the legacy config format. We never used it in p2pool. IDK why you have that here TBH.

use super::p2poolv2_config_parser::{ConfigField, ConfigSection};

/// Parses legacy p2pool.conf (key=value) format into UI sections
pub fn parse_legacy_p2pool_conf<P: AsRef<Path>>(path: P) -> io::Result<Vec<ConfigSection>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the legacy conf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added this because one test was failing maybe when a non-toml file was selected, so i intriduced key=value parser to make the test pass.
and yeah the fallback dosent correspond to the real config format, i just introduced to make the test pass
ill make it strictly TOML/schema-driven

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test case is wrong , and because of it i had to add it just to make it pass . ill remove it .
thanks

pub fn parse_p2poolv2_config<P: AsRef<Path>>(path: P) -> io::Result<Vec<ConfigSection>> {
let path = path.as_ref();

// Try TOML first
Copy link
Contributor

Choose a reason for hiding this comment

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

We only have toml config files, so no need to fall back to legacy. Lets clean that up.

use crate::components::p2poolv2_config_schema::Config;

#[derive(Clone, Debug, PartialEq)]
pub struct ConfigField {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only have toml config file, I think we might also not need this struct.

@pool2win
Copy link
Contributor

pool2win commented Jan 14, 2026 via email

@R27-pixel
Copy link
Contributor Author

Interesting. If someone selects a non-toml file, the config.rs parser will raise an error. We should depend on the error and show a meaningful error to the user.

On Wed, 14 Jan 2026 at 15:57, R27 @.> wrote: @.* commented on this pull request. ------------------------------ In src/components/legacy_p2pool_parser.rs <#9 (comment)>: > @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: 2024 PDM Authors +// SPDX-License-Identifier: AGPL-3.0-or-later + +use std::fs::File; +use std::io::{self, BufRead}; +use std::path::Path; + +use super::p2poolv2_config_parser::{ConfigField, ConfigSection}; + +/// Parses legacy p2pool.conf (key=value) format into UI sections +pub fn parse_legacy_p2pool_conf<P: AsRef>(path: P) -> io::Result<Vec> { i added this because one test was failing maybe when a non-toml file was selected, so i intriduced key=value parser to make the test pass. and yeah the fallback dosent correspond to the real config format, i just introduced to make the test pass ill make it strictly TOML/schema-driven — Reply to this email directly, view it on GitHub <#9 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASU3SYKKSPFC2XRSG7QELA34GZKNNAVCNFSM6AAAAACQU6VLHCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNRRGE3TINJXG4 . You are receiving this because you commented.Message ID: @.***>

Got it
I’ll remove the fallback and make the UI depend on config.rs errors and givea proper validation error to the user instead.

@R27-pixel R27-pixel closed this Jan 15, 2026
@R27-pixel R27-pixel force-pushed the feature/p2pool-config branch from e5ca31d to dafc799 Compare January 15, 2026 08:04
@R27-pixel R27-pixel deleted the feature/p2pool-config branch January 15, 2026 08:08
@R27-pixel R27-pixel restored the feature/p2pool-config branch January 18, 2026 06:59
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.

2 participants