Skip to content

fix: Check network ID in transactionSignFor#7102

Merged
bthomee merged 4 commits into
XRPLF:developfrom
kuznetsss:Check_network_id_in_sign_for
May 13, 2026
Merged

fix: Check network ID in transactionSignFor#7102
bthomee merged 4 commits into
XRPLF:developfrom
kuznetsss:Check_network_id_in_sign_for

Conversation

@kuznetsss
Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR adds NetworkID field validation to sign_for RPC.

Context of Change

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)

Copy link
Copy Markdown
Contributor

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

Adds NetworkID validation to the sign_for RPC path so callers can’t generate multi-signature payloads for the wrong network, and adjusts Expected::error() to support moving errors out of temporaries.

Changes:

  • Add checkNetworkID() validation and invoke it from transactionSignFor().
  • Add ref-qualified Expected::error() overloads, including an rvalue overload to enable move-out of error values.

Reviewed changes

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

File Description
src/xrpld/rpc/detail/TransactionSign.cpp Introduces and applies NetworkID validation for sign_for requests.
include/xrpl/basics/Expected.h Adds ref-qualified error() overloads (including rvalue) to support moving error payloads.

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

Comment thread src/xrpld/rpc/detail/TransactionSign.cpp
Comment thread src/xrpld/rpc/detail/TransactionSign.cpp
Comment thread src/xrpld/rpc/detail/TransactionSign.cpp
Comment thread include/xrpl/basics/Expected.h Outdated
Comment thread include/xrpl/basics/Expected.h Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

❌ Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (6340c98) to head (b39c9cc).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/rpc/detail/TransactionSign.cpp 46.2% 7 Missing ⚠️
include/xrpl/basics/Expected.h 66.7% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #7102     +/-   ##
=========================================
- Coverage     82.2%   82.1%   -0.0%     
=========================================
  Files         1010    1010             
  Lines        76147   76162     +15     
  Branches      7377    7381      +4     
=========================================
- Hits         62556   62555      -1     
- Misses       13591   13607     +16     
Files with missing lines Coverage Δ
include/xrpl/basics/Expected.h 96.2% <66.7%> (-3.8%) ⬇️
src/xrpld/rpc/detail/TransactionSign.cpp 89.1% <46.2%> (-1.0%) ⬇️

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

@kuznetsss kuznetsss force-pushed the Check_network_id_in_sign_for branch from ce14648 to f2d4c01 Compare May 11, 2026 15:07
@kuznetsss kuznetsss added this to the 3.1.3 (develop) milestone May 12, 2026
@bthomee bthomee changed the title fix: Check network ID in transactionSignFor fix: Check network ID in transactionSignFor May 12, 2026
@kuznetsss kuznetsss 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 May 12, 2026
@bthomee bthomee enabled auto-merge May 13, 2026 14:31
@bthomee bthomee added this pull request to the merge queue May 13, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 13, 2026
@bthomee bthomee added this pull request to the merge queue May 13, 2026
Merged via the queue into XRPLF:develop with commit 977e5a7 May 13, 2026
59 of 62 checks passed
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.

5 participants