Skip to content

Conversation

@mvadari
Copy link
Collaborator

@mvadari mvadari commented Nov 17, 2025

High Level Overview of Change

This PR fixes an indexing typo in the book_offers CLI processing. This does not affect the HTTPS/WS RPC processing.

It's a fairly straightforward fix - there is a length check on whether the list length is >= 5, but the 5u index is taken instead of the 4u index (as the last in the list).

Context of Change

Reported as a part of the Lending Protocol Attackathon

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

There is a minor fix in the book_offers CLI. This does not affect the HTTPS/WS RPC processing.

Test Plan

CI passes.

@mvadari mvadari requested a review from a team as a code owner November 17, 2025 10:45
@mvadari mvadari requested a review from vvysokikh1 November 17, 2025 10:46
@mvadari mvadari changed the title Fix index for iLimit in RPCCall.cpp fix: correct index for limit in book_offers CLI Nov 17, 2025
@mvadari mvadari 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 Nov 17, 2025
@mvadari mvadari 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 Nov 17, 2025
@Tapanito Tapanito self-requested a review November 17, 2025 12:01
@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.6%. Comparing base (6ff495f) to head (532c30e).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/rpc/detail/RPCCall.cpp 72.7% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6043   +/-   ##
=======================================
  Coverage     78.6%   78.6%           
=======================================
  Files          818     818           
  Lines        68983   68989    +6     
  Branches      8246    8241    -5     
=======================================
+ Hits         54196   54210   +14     
+ Misses       14787   14779    -8     
Files with missing lines Coverage Δ
src/xrpld/rpc/detail/RPCCall.cpp 93.7% <72.7%> (-0.4%) ⬇️

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

@mvadari
Copy link
Collaborator Author

mvadari commented Nov 17, 2025

#6044 is a version of this PR that covers more cases and is broader. We may want to merge this first or go straight to that one, unsure.

@bthomee
Copy link
Collaborator

bthomee commented Nov 19, 2025

@Tapanito are there any additional things that need to be addressed, or is this good to go?

@Tapanito
Copy link
Collaborator

@Tapanito are there any additional things that need to be addressed, or is this good to go?

Good to go @bthomee

@mvadari mvadari 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 Nov 20, 2025
@bthomee bthomee merged commit a3d4be4 into develop Nov 20, 2025
25 of 26 checks passed
@bthomee bthomee deleted the mvadari/book-offers-cli-fix branch November 20, 2025 11:37
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