Skip to content

Conversation

@mvadari
Copy link
Collaborator

@mvadari mvadari commented Jan 13, 2026

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Copilot AI review requested due to automatic review settings January 13, 2026 15:02
@mvadari mvadari marked this pull request as draft January 13, 2026 15:02
@mvadari mvadari added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Jan 13, 2026
Copy link

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

This PR adds experimental support for YAML-based configuration files as an alternative to the existing INI format. The implementation includes YAML parsing, conversion to the internal INI structure, automatic format detection based on file extension, and comprehensive unit tests for the YAML functionality.

Changes:

  • Adds YAML configuration parsing and conversion utilities with support for the existing INI-based config structure
  • Integrates YAML support into the Config class with automatic format detection based on file extension
  • Provides example YAML configuration files and a conversion script for migrating from INI to YAML format

Reviewed changes

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

Show a summary per file
File Description
include/xrpl/basics/YamlConfig.h Defines public API for YAML parsing and conversion functions
src/libxrpl/basics/YamlConfig.cpp Implements YAML parsing, conversion to IniFileSections, and file type detection
src/xrpld/core/Config.h Adds loadFromYamlString and loadFromIniFileSections method declarations
src/xrpld/core/detail/Config.cpp Implements YAML config loading and integrates format detection into existing load flow
src/test/basics/YamlConfig_test.cpp Comprehensive unit tests for YAML parsing and conversion functions
conanfile.py Adds yaml-cpp/0.8.0 dependency
conan.lock Updates dependency lock file with yaml-cpp package hashes
cmake/XrplCore.cmake Links yaml-cpp library to xrpl.libxrpl.basics module
CMakeLists.txt Adds yaml-cpp package discovery and linking
cfg/xrpld-example.yaml Example YAML configuration file demonstrating the YAML format
cfg/validators-example.yaml Example YAML validators configuration file
cfg/ini2yaml.sh Bash script for converting INI config files to YAML format

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

Comment on lines 142 to 146
if (sectionValue.IsScalar())
{
// Single value section (e.g., database_path: "/path")
lines.push_back(sectionValue.as<std::string>());
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The YAML to INI conversion does not handle null/undefined YAML values. When a YAML section has a null value (e.g., empty_section: null or empty_section:), the code will attempt to call as<std::string>() on a null node at line 145, which could cause a runtime error or unexpected behavior. Consider adding a check for IsNull() before processing scalar values.

Copilot uses AI. Check for mistakes.

// Regular mapping section (e.g., node_db: {type: "NuDB", ...})
processMapping(sectionValue, lines, j);
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The yamlToIniFileSections function does not handle null or undefined values in the main conversion loop. After checking the node type (IsScalar, IsSequence, IsMap), if none match but the node is null, an empty section will be added without any indication. Consider handling the IsNull case explicitly to provide clearer semantics or logging.

Suggested change
}
}
else if (!sectionValue.IsDefined() || sectionValue.IsNull())
{
// Explicitly handle null/undefined section values to avoid
// silently creating empty sections.
JLOG(j.warn()) << "YAML section '" << sectionName
<< "' has null/undefined value; skipping.";
continue;
}

Copilot uses AI. Check for mistakes.
Comment on lines 474 to 484
void
Config::loadFromYamlString(std::string const& fileContents)
{
auto const yamlNode = parseYamlString(fileContents, j_);
if (!yamlNode)
{
Throw<std::runtime_error>("Failed to parse YAML configuration");
}
IniFileSections secConfig = yamlToIniFileSections(*yamlNode, j_);
loadFromIniFileSections(secConfig);
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

Missing test coverage for the Config class integration with YAML format. While YamlConfig_test.cpp thoroughly tests the YAML parsing and conversion functions, there are no tests that verify the actual Config class can load and parse YAML configuration files through the loadFromYamlString method. Consider adding integration tests in Config_test.cpp that verify YAML configuration files are properly loaded and converted to the expected Config state.

Copilot uses AI. Check for mistakes.
cfg/ini2yaml.sh Outdated
Comment on lines 55 to 57
# Escape any existing quotes and wrap in quotes if it contains special chars
if [[ "$value" =~ [\:\#\[\]\{\}\,\&\*\!\|\>\<\%\@\`] ]] || [[ "$value" =~ ^[[:space:]] ]] || [[ "$value" =~ [[:space:]]$ ]]; then
echo "\"$value\""
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The shell script does not properly escape double quotes within string values. At line 57, if a value contains double quotes, they should be escaped before wrapping the value in double quotes to avoid malformed YAML output. Consider adding escape logic for quotes and other special characters that could break YAML syntax.

Suggested change
# Escape any existing quotes and wrap in quotes if it contains special chars
if [[ "$value" =~ [\:\#\[\]\{\}\,\&\*\!\|\>\<\%\@\`] ]] || [[ "$value" =~ ^[[:space:]] ]] || [[ "$value" =~ [[:space:]]$ ]]; then
echo "\"$value\""
# Escape any existing quotes and backslashes, and wrap in quotes if it contains special chars
if [[ "$value" =~ [\:\#\[\]\{\}\,\&\*\!\|\>\<\%\@\`] ]] || [[ "$value" =~ ^[[:space:]] ]] || [[ "$value" =~ [[:space:]]$ ]]; then
local escaped="$value"
escaped="${escaped//\\/\\\\}"
escaped="${escaped//\"/\\\"}"
echo "\"$escaped\""

Copilot uses AI. Check for mistakes.
Comment on lines 50 to 120
void
processMapping(
YAML::Node const& node,
std::vector<std::string>& lines,
beast::Journal j)
{
for (auto const& pair : node)
{
auto const key = pair.first.as<std::string>();
auto const& value = pair.second;

if (value.IsScalar())
{
lines.push_back(key + "=" + value.as<std::string>());
}
else if (value.IsSequence())
{
// For sequences in a mapping, join values with comma
std::string combined;
for (std::size_t i = 0; i < value.size(); ++i)
{
if (i > 0)
combined += ",";
combined += value[i].as<std::string>();
}
lines.push_back(key + "=" + combined);
}
else
{
JLOG(j.warn()) << "Skipping nested mapping for key '" << key << "'";
}
}
}

// Helper to process a YAML sequence into multiple lines
void
processSequence(
YAML::Node const& node,
std::vector<std::string>& lines,
beast::Journal j)
{
for (auto const& item : node)
{
if (item.IsScalar())
{
lines.push_back(item.as<std::string>());
}
else if (item.IsMap())
{
// For sequences of maps, convert each map item to key=value
// This handles cases like ips: [{host: "x", port: 51235}]
// Convert to "x 51235" format for compatibility
auto hostIt = item["host"];
auto portIt = item["port"];
if (hostIt && portIt)
{
lines.push_back(
hostIt.as<std::string>() + " " + portIt.as<std::string>());
}
else
{
// Generic map handling: key=value on separate lines
processMapping(item, lines, j);
}
}
else if (item.IsSequence())
{
// Nested sequences are not supported in INI format
JLOG(j.warn()) << "Skipping nested sequence in YAML config";
}
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The YAML::Node::asstd::string() calls throughout this function (lines 58, 63, 73, 95, 107, etc.) can throw YAML::TypedBadConversion exceptions if the node cannot be converted to the target type. While the parseYamlString function catches YAML exceptions, these conversion errors in yamlToIniFileSections are not caught and could cause the program to terminate unexpectedly. Consider wrapping these conversions in try-catch blocks or validating node types more thoroughly before conversion.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 64.74820% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (f3627fb) to head (bb7aa50).

Files with missing lines Patch % Lines
src/libxrpl/basics/TomlConfig.cpp 67.2% 38 Missing ⚠️
src/xrpld/core/detail/Config.cpp 52.2% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6210     +/-   ##
=========================================
- Coverage     79.9%   79.8%   -0.0%     
=========================================
  Files          839     840      +1     
  Lines        65528   65660    +132     
  Branches      7270    7314     +44     
=========================================
+ Hits         52327   52409     +82     
- Misses       13201   13251     +50     
Files with missing lines Coverage Δ
src/xrpld/core/Config.h 84.6% <ø> (ø)
src/xrpld/core/detail/Config.cpp 73.2% <52.2%> (-1.5%) ⬇️
src/libxrpl/basics/TomlConfig.cpp 67.2% <67.2%> (ø)

... and 1 file with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mvadari mvadari changed the title experimental: add support for YAML-based config experimental: add support for TOML-based config Jan 28, 2026
@mvadari mvadari added the Needs Draft XLS The corresponding draft XLS must have been merged before this PR is merged. label Jan 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DraftRunCI Normally CI does not run on draft PRs. This opts in. Needs Draft XLS The corresponding draft XLS must have been merged before this PR is merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant