Skip to content

Refactor: checking Insufficient reserve to use helper function#5844

Open
tequdev wants to merge 13 commits into
XRPLF:developfrom
tequdev:refactor-insufficient-reserve-check
Open

Refactor: checking Insufficient reserve to use helper function#5844
tequdev wants to merge 13 commits into
XRPLF:developfrom
tequdev:refactor-insufficient-reserve-check

Conversation

@tequdev

@tequdev tequdev commented Oct 3, 2025

Copy link
Copy Markdown
Member

High Level Overview of Change

Added helper function checkInsufficientReserve() for tecINSUFFICIENT_RESERVE check

Type of Change

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

@Tapanito Tapanito requested review from mvadari and vvysokikh1 October 3, 2025 15:05
@bthomee bthomee added this to the 3.1.0 milestone Oct 3, 2025
@codecov

codecov Bot commented Oct 3, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.28571% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.3%. Comparing base (173f9f7) to head (ba69dcd).

Files with missing lines Patch % Lines
src/xrpld/app/tx/detail/PayChan.cpp 92.9% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5844   +/-   ##
=======================================
  Coverage     78.3%   78.3%           
=======================================
  Files          816     816           
  Lines        68887   68923   +36     
  Branches      8303    8301    -2     
=======================================
+ Hits         53943   53989   +46     
+ Misses       14944   14934   -10     
Files with missing lines Coverage Δ
include/xrpl/ledger/View.h 100.0% <ø> (ø)
src/libxrpl/ledger/View.cpp 94.4% <100.0%> (+<0.1%) ⬆️
src/xrpld/app/tx/detail/AMMCreate.cpp 92.1% <100.0%> (+0.1%) ⬆️
src/xrpld/app/tx/detail/AMMDeposit.cpp 96.3% <100.0%> (+0.1%) ⬆️
src/xrpld/app/tx/detail/AMMWithdraw.cpp 95.8% <100.0%> (+<0.1%) ⬆️
src/xrpld/app/tx/detail/CreateCheck.cpp 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/CreateTicket.cpp 98.0% <100.0%> (ø)
src/xrpld/app/tx/detail/Credentials.cpp 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/DID.cpp 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/DelegateSet.cpp 100.0% <100.0%> (ø)
... and 12 more

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

@Tapanito Tapanito added the Triaged Issue/PR has been triaged for viability, liveliness, etc. label Oct 3, 2025
@mvadari

mvadari commented Oct 3, 2025

Copy link
Copy Markdown
Collaborator

Can you add tests for the unchecked codecov lines as well? I know it's not technically relevant to this PR, but while we're here we may as well, since it's pretty easy.

Comment thread src/libxrpl/ledger/View.cpp Outdated
Comment thread src/libxrpl/ledger/View.cpp Outdated
Comment thread src/test/app/PayChan_test.cpp Outdated
Comment thread src/xrpld/app/tx/detail/PayChan.cpp Outdated
@vvysokikh1 vvysokikh1 self-requested a review October 9, 2025 09:59

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

taking off my approval for now

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

check for amendment gating potentially required

@mvadari

mvadari commented Oct 9, 2025

Copy link
Copy Markdown
Collaborator

check for amendment gating potentially required

@vvysokikh1 Some context that may be useful: these refactors were originally included in the XLS-68 PR (to be opened). I suggested pulling some out and putting them in other PRs, as that has been a useful strategy in other amendments.

Perhaps we can comment out the changes wherever we think an amendment would be needed? Or are you concerned about all calls to this helper function?

@vvysokikh1

Copy link
Copy Markdown
Contributor

check for amendment gating potentially required

@vvysokikh1 Some context that may be useful: these refactors were originally included in the XLS-68 PR (to be opened). I suggested pulling some out and putting them in other PRs, as that has been a useful strategy in other amendments.

Perhaps we can comment out the changes wherever we think an amendment would be needed? Or are you concerned about all calls to this helper function?

I'm concerned about the changes in the last commit. Previous commits where pretty obvious to me, but this last change is quite interesting. I'm not yet sure there's no change in behavior and I wanted to pull back my approval, it's just unfortunate that I don't know any other way except requesting a change :)

@vvysokikh1 vvysokikh1 self-requested a review October 29, 2025 11:02
Comment thread src/libxrpl/ledger/View.cpp Outdated
Comment thread src/xrpld/app/tx/detail/AMMCreate.cpp
@tequdev tequdev force-pushed the refactor-insufficient-reserve-check branch from d3fa030 to d024dab Compare December 18, 2025 15:42
@vvysokikh1 vvysokikh1 self-requested a review January 5, 2026 12:26
@ximinez ximinez modified the milestones: 3.1.0, 3.2.0 Jan 28, 2026
Comment on lines +121 to 125
auto const accountSle = ctx.view.read(keylet::account(accountID));
if (auto const ret = checkInsufficientReserve(
ctx.view, accountSle, accountSle->getFieldAmount(sfBalance), 1);
!isTesSuccess(ret))
{

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.

could you please add a check for accountSle being valid in every place you changed xrpLiquid to checkInsufficientReserve

@github-actions

Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@godexsoft

Copy link
Copy Markdown
Contributor

We recently merged a refactor to develop that enables clang-tidy's readability-identifier-naming. Your branch now has heavy conflicts that are largely mechanical. Below is a workflow that aligns your branch's naming with develop before merging, which should minimize the merge conflicts.

One-time setup

If you don't already have clang-tidy working in your env, on macOS:

brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.

Workflow on your branch (before merging develop)

1. Grab the new .clang-tidy from develop without pulling anything else. Sync your fork on GitHub first, then:

git remote -v   # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy

2. Reconfigure conan/cmake so compile_commands.json is fresh.

3. Apply renames for the files modified in your PR:

git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
  | grep -E '\.(cpp|h|hpp|ipp)$' \
  | xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called

4. Build + test, then commit as a single dedicated commit:

cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"

5. Now merge develop:

git merge upstream/develop

Extra

Run clang-tidy once more after the merge to catch any stragglers introduced from develop's side:

run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called

@bthomee bthomee modified the milestones: 3.2.0, 3.3.0 May 6, 2026
@bthomee

bthomee commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

@tequdev what's the status of this PR? Would you be able to resolve the conflicts in case it is still relevant?

@mvadari

mvadari commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

@bthomee I think this is getting superseded by #7350

@bthomee

bthomee commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

@bthomee I think this is getting superseded by #7350

I'll let Tequ close this PR then in confirmation of the supersedence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: has conflicts Triaged Issue/PR has been triaged for viability, liveliness, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants