Skip to content

Pre-lending protocol refactoring 2: STParsedJSON #5591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 24 commits into
base: ximinez/lending-refactoring-1
Choose a base branch
from
Open

Pre-lending protocol refactoring 2: STParsedJSON #5591

wants to merge 24 commits into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Jul 22, 2025

High Level Overview of Change

This PR is number two of four which contain several small changes that are used as the baseline of #5270. Since they are significant and useful, I created this separate PR to get them merged sooner.

The base of this branch is #5590. The expectation is that they'll be merged in order.

(This one is a pure refactor, and does not add or change functionality.)

Context of Change

#5270 is large and complicated, and I had to lay some groundwork before I could implement the feature.

Type of Change

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

Before / After

Highlights:

  • Refactors parseLeaf to break the handlers for STI_UINT16 and STI_UINT32 into separate helper functions. This is mainly to improve readability and add some reusability. It was going to be the base for functionality that I abandoned as unnecessary.

@ximinez ximinez requested a review from a team as a code owner July 22, 2025 00:45
@ximinez
Merge branch 'ximinez/lending-refactoring-1' into ximinez/lending-ref…
72f33d8 
…actoring-2

* ximinez/lending-refactoring-1: (57 commits)
  Fix formatting
  Remove `include(default)` from libxrpl profile (#5587)
  refactor: Change boost::shared_mutex to std::shared_mutex (#5576)
  Fix macos runner (#5585)
  Remove the type filter from "ledger" RPC command (#4934)
  refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567)
  test: Run unit tests regardless of 'Supported' amendment status (#5537)
  Retire Flow Cross amendment (#5562)
  chore: Update CI to use Conan 2 (#5556)
  fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513)
  chore: Add gcc-12 workaround (#5554)
  Add MPT related txns into issuer's account history  (#5530)
  chore: Remove unused headers (#5526)
  fix: add allowTrustLineLocking flag for account_info (#5525)
  Downgrade required CMake version for Antithesis SDK (#5548)
  fix: Link with boost libraries explicitly (#5546)
  chore: Fix compilation error with clang-20 and cleanup (#5543)
  test: Remove circular jtx.h dependencies (#5544)
  Decouple CredentialHelpers from xrpld/app/tx (#5487)
  fix: crash when trace-logging in tests (#5529)
  ...
@ximinez ximinez changed the title Pre-lending protocol refactoring 2 Pre-lending protocol refactoring 2: STParsedJSON Jul 22, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 22, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 97.29730% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (ximinez/lending-refactoring-1@0b833e1). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/libxrpl/protocol/STParsedJSON.cpp 97.3% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                       @@
##             ximinez/lending-refactoring-1   #5591   +/-   ##
===============================================================
  Coverage                                 ?   78.7%           
===============================================================
  Files                                    ?     816           
  Lines                                    ?   71827           
  Branches                                 ?    8468           
===============================================================
  Hits                                     ?   56507           
  Misses                                   ?   15320           
  Partials                                 ?       0
Files with missing lines Coverage Δ
src/libxrpl/protocol/STParsedJSON.cpp 72.3% <97.3%> (ø)

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 requested a review from Bronek July 22, 2025 19:12
dangell7
dangell7 requested changes Aug 4, 2025
View reviewed changes
Copy link
Collaborator

@dangell7 dangell7 left a comment

Choose a reason for hiding this comment

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

Missing Code Coverage.

This patch will add a new STParseJSON_test.cpp and should give you 100% on your changes. https://gist.github.com/dangell7/a129e1032a88d6c13e64c57e42b2427a

If you don't want the full refactor its the last 3 tests in testParseJSONEdgeCases in STParseJSON_test.

@ximinez
Copy link
Collaborator Author

ximinez commented Aug 5, 2025

This patch will add a new STParseJSON_test.cpp and should give you 100% on your changes. gist.github.com/dangell7/a129e1032a88d6c13e64c57e42b2427a

Thanks for the patch!

I verified that the code was moved over with no changes. I also renamed the file and class to STParsedJSON_test to match the existing class.

(In the future, it may save us both time if you push a commit to a new branch, and just give me the commit ID, which I can then cherry-pick.)

@ximinez @dangell7
Add code coverage for STParsedJSON edge cases
f89c88d 
- Move several tests from `STObject_test` to new `STParsedJSON_test`
  unchanged.
- Add 3 test cases to cover edge cases for UInt16 values.

Co-authored-by: Denis Angell <[email protected]>
@ximinez ximinez requested a review from dangell7 August 5, 2025 16:46
@dangell7
Copy link
Collaborator

dangell7 commented Aug 5, 2025

This patch will add a new STParseJSON_test.cpp and should give you 100% on your changes. gist.github.com/dangell7/a129e1032a88d6c13e64c57e42b2427a

Thanks for the patch!

I verified that the code was moved over with no changes. I also renamed the file and class to STParsedJSON_test to match the existing class.

(In the future, it may save us both time if you push a commit to a new branch, and just give me the commit ID, which I can then cherry-pick.)

Is if (value.isString()) inside of parseUnsigned a possible code path? It looks like its not but maybe we want to keep it for defensive purposes?

If its not testable and you want to keep it can we add an exclude?

// LCOV_EXCL_START
if (value.isString())
...
// LCOV_EXCL_STOP

Bronek
Bronek reviewed Aug 22, 2025
View reviewed changes
src/libxrpl/protocol/STParsedJSON.cpp
else
{
ret = detail::make_stvar<STResult>(
field, beast::lexicalCastThrow<Integer>(value.asString()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be field, safe_cast<typename STResult::value_type>(beast::lexicalCastThrow<Integer>( . . . ?

Bronek
Bronek approved these changes Aug 22, 2025
View reviewed changes
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Minor comment aside (which I do not feel strongly about, but some response would be appreciated) this is good.

@kennyzlei kennyzlei requested a review from yinyiqian1 August 29, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bronek Bronek Bronek approved these changes

@dangell7 dangell7 Awaiting requested review from dangell7

@yinyiqian1 yinyiqian1 Awaiting requested review from yinyiqian1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ximinez @dangell7 @Bronek