Sponsored Content
Skip to content

refactor: Delete SecretKey compare op from library and move it to tests module - #6503

Merged
bthomee merged 9 commits into
developfrom
pratik/Move-key-equals-functions-to-test-files
Mar 16, 2026
Merged

refactor: Delete SecretKey compare op from library and move it to tests module#6503
bthomee merged 9 commits into
developfrom
pratik/Move-key-equals-functions-to-test-files

Conversation

@pratikmankawde

@pratikmankawde pratikmankawde commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Delete the SecretKey equality/inequality operators from the public library header and move the comparison logic into test-only code.

The operator== and operator!= free functions on SecretKey were removed from include/xrpl/protocol/SecretKey.h and replaced with explicitly deleted member functions to prevent accidental use in production code. A named test::equal() helper was added in src/test/unit_test/utils.h for test assertions that need to compare secret keys.

Context of Change

Comparing secret keys with == in production code is a security concern — constant-time comparison should be used instead. By deleting the operator and restricting comparison to test code only, we prevent accidental timing-side-channel vulnerabilities in the library.

The previous free-function operator==/operator!= overloads in SecretKey.h were available to all consumers of libxrpl. Now, the deleted member operators cause a compile error if anyone tries to compare SecretKey objects outside of test code. Test files that need comparison use the explicit test::equal() named function, which bypasses the deleted-member-function lookup issue that would affect any free-function operator== overload.

Type of Change

  • Refactor (non-breaking change that only restructures code)

API Impact

  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)

Before / After

Before: SecretKey had public operator== / operator!= free functions in the library header, usable by any code that included SecretKey.h.

// Production or test code — both compiled fine
BEAST_EXPECT(k.keys->secretKey == tokenSecretKey);

After: SecretKey::operator== is deleted. Production code gets a compile error. Test code uses the named helper:

// Production code — compile error (good)
// k.keys->secretKey == tokenSecretKey;

// Test code — explicit opt-in
BEAST_EXPECT(test::equal(k.keys->secretKey, tokenSecretKey));

Test Plan

  • Build passes with zero errors
  • All existing SecretKey comparison test assertions updated to use test::equal()
  • Affected test files: SecretKey_test.cpp, ValidatorKeys_test.cpp, Manifest_test.cpp

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
@codecov

codecov Bot commented Mar 9, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6503     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          878     878             
  Lines        67887   67885      -2     
  Branches      7551    7553      +2     
=========================================
- Hits         54206   54195     -11     
- Misses       13681   13690      +9     
Files with missing lines Coverage Δ
include/xrpl/protocol/SecretKey.h 66.7% <ø> (-4.8%) ⬇️

... and 6 files 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.

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Comment thread src/test/unit_test/utils.h
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
@pratikmankawde pratikmankawde changed the title Delete compare op from library and moved it to tests module chore: Delete compare op from library and moved it to tests module Mar 9, 2026
@pratikmankawde pratikmankawde changed the title chore: Delete compare op from library and moved it to tests module refactor: Delete compare op from library and moved it to tests module Mar 9, 2026
Comment thread include/xrpl/protocol/SecretKey.h
@ximinez
ximinez self-requested a review March 10, 2026 01:25
Comment thread src/test/unit_test/utils.h Outdated
pratikmankawde and others added 2 commits March 16, 2026 10:28
@pratikmankawde pratikmankawde 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 16, 2026
@bthomee bthomee changed the title refactor: Delete compare op from library and moved it to tests module refactor: Delete SecretKey compare op from library and move it to tests module Mar 16, 2026
@bthomee
bthomee enabled auto-merge (squash) March 16, 2026 10:34
@bthomee
bthomee merged commit 1738a69 into develop Mar 16, 2026
3 checks passed
@bthomee
bthomee deleted the pratik/Move-key-equals-functions-to-test-files branch March 16, 2026 10:55
@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
…ests module (XRPLF#6503)

This change deletes the `SecretKey` equality/inequality operators from the public library header and moves the comparison logic into test-only code.

Specifically, the `operator==` and `operator!=` free functions on `SecretKey` have been removed from `include/xrpl/protocol/SecretKey.h` and have been replaced with explicitly deleted member functions to prevent accidental use in production code. A named `test::equal()` helper has also been added in `src/test/unit_test/utils.h` for test assertions that need to compare secret keys.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants