Sponsored Content
Skip to content

feat: Enforce feature name lengths and character set - #5555

Merged
bthomee merged 20 commits into
developfrom
Bronek/maximum_feature_name_size
Mar 13, 2026
Merged

feat: Enforce feature name lengths and character set#5555
bthomee merged 20 commits into
developfrom
Bronek/maximum_feature_name_size

Conversation

@Bronek

@Bronek Bronek commented Jul 11, 2025

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR enforces a maximum length of 63 characters on feature names, as well as not permitting an exactly 32 character long feature name to avoid confusion with those that use a uint256 hex representation, as that is an alternative way to specify a feature. This change further prevents the use of Unicode characters in feature names, because some can be confused with regular ASCII characters despite being valid in identifiers.

Context of Change

Useful for WASM interop; improved security.

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

@Bronek
Bronek requested a review from mvadari July 11, 2025 14:03
Comment thread src/test/rpc/Feature_test.cpp Outdated
Comment thread src/libxrpl/protocol/Feature.cpp Outdated
@Bronek
Bronek force-pushed the Bronek/maximum_feature_name_size branch from 1c57402 to ba2ac69 Compare July 14, 2025 16:32
@mvadari

mvadari commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

If it makes more sense, this PR can be merged into ripple/smart-escrow instead of directly into develop

@intelliot

Copy link
Copy Markdown
Contributor

@Bronek is there a reason this is still in draft? Just wondering

@intelliot intelliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

notes: this limits feature (amendment) names to <= 63 and prevents names that are exactly 32-33 characters long, because those names would be ambiguous - is it feature name or uint256 hash?

WASM code will use 32-byte strings to select features without ambiguity.

disallowing 33 bytes prevents potential off-by-one mistake since a 32 character string is 33 bytes total (with 1 null terminator byte)

@ximinez
ximinez marked this pull request as ready for review March 3, 2026 19:13
@ximinez ximinez added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 3, 2026
@ximinez

ximinez commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

I'm marking this as "Ready to merge" on @Bronek's behalf. It's got two trusted reviews, and doesn't seem to have bit-rotted.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 compile-time enforcement of feature/amendment name length limits to prevent ambiguous selection (e.g., names colliding with alternate uint256-based selection), and updates the RPC feature unit test to compute/verify the feature ID hash.

Changes:

  • Add maxFeatureNameSize and reservedFeatureNameSize constants and enforce them during feature registration via compile-time static_asserts.
  • Update XRPL_FEATURE / XRPL_FIX macro expansions to run name literals through the new size-enforcement helper.
  • Adjust Feature_test to compute the expected feature ID via sha512Half while still asserting against the fixed known hash.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
include/xrpl/protocol/Feature.h Introduces public constants for feature name size limits (and documents reserved lengths).
src/libxrpl/protocol/Feature.cpp Adds compile-time validation helper and wires it into feature/fix registration macros.
src/test/rpc/Feature_test.cpp Computes expected feature hash dynamically and verifies it matches the known constant.

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

Comment thread src/libxrpl/protocol/Feature.cpp Outdated
Comment thread include/xrpl/protocol/Feature.h Outdated
@ximinez ximinez removed the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 3, 2026
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Mar 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (7a3bf16) to head (0d38c57).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5555     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          878     878             
  Lines        67887   67887             
  Branches      7548    7550      +2     
=========================================
- Hits         54207   54205      -2     
- Misses       13680   13682      +2     
Files with missing lines Coverage Δ
include/xrpl/protocol/Feature.h 97.8% <ø> (ø)
src/libxrpl/protocol/Feature.cpp 94.4% <ø> (ø)

... 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.

@ximinez
ximinez self-requested a review March 6, 2026 19:31
@Bronek Bronek changed the title Enforce maximum feature name size feat: Enforce maximum feature name size Mar 11, 2026
@Bronek Bronek changed the title feat: Enforce maximum feature name size feat: Enforce maximum feature name size and character set Mar 11, 2026
@Bronek

Bronek commented Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

@bthomee @ximinez I added a check to fail compilation if a feature name contains an Unicode character - otherwise we may end up with features which are difficult to work with, but may also look (in many but not all tools) like regular text.

@bthomee bthomee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking good and very comprehensive. I would just like to see some additional comments in places that would not be intuitive for the standard developer.

Comment thread include/xrpl/protocol/Feature.h Outdated
Comment thread include/xrpl/protocol/Feature.h
@bthomee bthomee changed the title feat: Enforce maximum feature name size and character set feat: Enforce feature name lengths and character set Mar 13, 2026
@bthomee
bthomee merged commit 0446bef into develop Mar 13, 2026
2 of 3 checks passed
@bthomee
bthomee deleted the Bronek/maximum_feature_name_size branch March 13, 2026 17:41
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
This change enforces a maximum length of 63 characters on feature names, as well as not permitting an exactly 32 character long feature name to avoid confusion with those that use a `uint256` hex representation, as that is an alternative way to specify a feature. This change further prevents the use of Unicode characters in feature names, because some can be confused with regular ASCII characters despite being valid in identifiers.
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.

6 participants