Sponsored Content
Skip to content

fix: account_tx limit parameter validation for malformed values - #5891

Merged
bthomee merged 1 commit into
developfrom
copilot/fix-account-tx-error-handling
Oct 28, 2025
Merged

fix: account_tx limit parameter validation for malformed values#5891
bthomee merged 1 commit into
developfrom
copilot/fix-account-tx-error-handling

Conversation

Copilot AI commented Oct 14, 2025

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Fixed account_tx RPC method to properly validate malformed limit parameter values. Previously, invalid values like 0, 1.2, "10", true, false, -1, [], {}, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return invalidParams error.

Fixes #5138

Context of Change

This is a bug fix for improper parameter validation in the account_tx RPC method. The bug has existed since the method was implemented - the original code directly called asUInt() on the limit parameter without proper type validation, which bypassed type checking and caused implicit conversions or internal errors.

The fix follows the established pattern used by other RPC methods (like account_lines) by:

  1. Adding proper type validation to reject non-integer values
  2. Explicitly rejecting invalid values like 0
  3. Using the existing readLimitField helper function for consistent behavior

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)

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)

Before / After

Before (incorrect behavior):

Request with limit = 0:

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 0}]}

Response: Success with transactions (incorrect - should reject invalid limit)

Request with limit = "10":

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": "10"}]}

Response: Success (incorrect - should reject non-integer)

Request with limit = -1:

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": -1}]}

Response: Internal error (incorrect error type)

After (correct behavior):

Request with limit = 0:

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 0}]}

Response:

{
    "result": {
        "error": "invalidParams",
        "error_code": 31,
        "error_message": "Invalid parameters.",
        "status": "error"
    }
}

Request with limit = "10", 1.2, true, false, -1, [], {}, etc.:

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": "10"}]}

Response:

{
    "result": {
        "error": "invalidParams",
        "error_code": 31,
        "error_message": "Invalid field 'limit', not unsigned integer.",
        "status": "error"
    }
}

Valid request with limit = 10:

{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 10}]}

Response: Success with up to 10 transactions

Test Plan

Added comprehensive test coverage in AccountTx_test.cpp for all malformed limit values from the issue:

  • limit = 0 → returns rpcINVALID_PARAMS
  • limit = 1.2, "10", true, false, -1, [], {}, "malformed", ["limit"], {"limit": 10} → return expected_field_error with message "Invalid field 'limit', not unsigned integer."
  • limit = 10 (valid) → works correctly

The tests verify that:

  1. Type validation correctly rejects non-integer types
  2. Value validation correctly rejects 0
  3. Valid integer values are accepted and processed correctly

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@mvadari
mvadari marked this pull request as ready for review October 14, 2025 21:09
Copilot AI changed the title [WIP] Fix account_tx to handle malformed limit values Fix account_tx limit parameter validation for malformed values Oct 14, 2025
@mvadari
mvadari requested a review from a team October 14, 2025 21:09
Copilot AI requested a review from mvadari October 14, 2025 21:09
@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

Copilot AI requested a review from mvadari October 14, 2025 22:05
@mvadari mvadari changed the title Fix account_tx limit parameter validation for malformed values fix: account_tx limit parameter validation for malformed values Oct 14, 2025
@mvadari mvadari changed the title fix: account_tx limit parameter validation for malformed values fix: account_tx limit parameter validation for malformed values Oct 14, 2025
@codecov

codecov Bot commented Oct 14, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.3%. Comparing base (d9960d5) to head (9b31907).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5891   +/-   ##
=======================================
  Coverage     78.3%   78.3%           
=======================================
  Files          817     817           
  Lines        68981   68982    +1     
  Branches      8333    8321   -12     
=======================================
+ Hits         54021   54023    +2     
+ Misses       14960   14959    -1     
Files with missing lines Coverage Δ
src/xrpld/rpc/detail/RPCHelpers.cpp 82.3% <100.0%> (+0.2%) ⬆️
src/xrpld/rpc/detail/Tuning.h 100.0% <ø> (ø)
src/xrpld/rpc/handlers/AccountChannels.cpp 81.4% <ø> (-0.4%) ⬇️
src/xrpld/rpc/handlers/AccountLines.cpp 88.3% <ø> (+0.6%) ⬆️
src/xrpld/rpc/handlers/AccountOffers.cpp 89.5% <ø> (-0.3%) ⬇️
src/xrpld/rpc/handlers/AccountTx.cpp 87.6% <100.0%> (+0.1%) ⬆️

Impacted file tree graph

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

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

Overall this is fine. I believe in Clio we have the same behaviour.

Comment thread src/xrpld/rpc/handlers/AccountTx.cpp Outdated
if (auto err = RPC::readLimitField(limit, RPC::Tuning::accountTx, context))
return *err;

args.limit = limit;

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.

nit: Why not directly use args.limit?

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.

Fair point - fixed

Comment thread src/xrpld/rpc/handlers/AccountTx.cpp Outdated
}

unsigned int limit;
if (auto err = RPC::readLimitField(limit, RPC::Tuning::accountTx, context))

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.

nit: err can be const

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.

Good catch - fixed

@bthomee bthomee added the Needs additional review PR requires at least one more code review approval before it can be merged label Oct 20, 2025
Comment thread src/xrpld/rpc/handlers/AccountTx.cpp Outdated
Comment thread src/xrpld/rpc/handlers/AccountTx.cpp Outdated
@mvadari
mvadari force-pushed the copilot/fix-account-tx-error-handling branch from 782630f to 24a8364 Compare October 22, 2025 21:23
Comment thread src/xrpld/rpc/detail/RPCHelpers.cpp

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

👍

@mvadari mvadari added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Needs additional review PR requires at least one more code review approval before it can be merged labels Oct 28, 2025
@mvadari mvadari added Needs additional review PR requires at least one more code review approval before it can be merged and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Oct 28, 2025
This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.
@bthomee
bthomee force-pushed the copilot/fix-account-tx-error-handling branch from bf58766 to 9b31907 Compare October 28, 2025 17:20
@bthomee bthomee added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Needs additional review PR requires at least one more code review approval before it can be merged labels Oct 28, 2025
@bthomee
bthomee enabled auto-merge October 28, 2025 17:21
@bthomee
bthomee added this pull request to the merge queue Oct 28, 2025
Merged via the queue into develop with commit 7d5ed0c Oct 28, 2025
3 checks passed
@bthomee
bthomee deleted the copilot/fix-account-tx-error-handling branch October 28, 2025 18:07
@mactoys1980

Copy link
Copy Markdown

3907f32

bthomee added a commit that referenced this pull request Apr 7, 2026
This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
ximinez pushed a commit that referenced this pull request Apr 10, 2026
This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
@ximinez ximinez modified the milestones: 3.1.3, 3.1.3 (develop) Apr 15, 2026
ximinez pushed a commit that referenced this pull request Apr 22, 2026
This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
…F#5891)

This change fixes the `account_tx` RPC method to properly validate malformed limit parameter values. Previously, invalid values like `0`, `1.2`, `"10"`, `true`, `false`, `-1`, `[]`, `{}`, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly return the `invalidParams` error.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
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.

[account_tx with malformed values of limit returns either no error or not expected error] (Version: [2.3.0-b1])

7 participants