chore: Update default values of base and owner reserve to 1/0.2 - #6382
Conversation
mvadari
left a comment
There was a problem hiding this comment.
Should also change the sample config
done |
|
@xVet would you be able to fix the test failures? A quick scan shows two issues: Failure to build on some distros: Failure to test on other distros: I'll update your branch with the latest from develop, as we've added more static code analyzers recently (with more to come), so it might be that when you take a look you'll see other results. |
|
If only the most recent commit is unsigned, you can run:
If multiple commits are unsigned, you can run:
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
XRPAmount only defines operator*(value_type), so 0.2 * DROPS_PER_XRP truncates 0.2 to 0 (long), leaving owner_reserve = 0 drops. On strict builds this is caught at compile time (-Werror=literal-conversion); on lenient builds it slips through and trips the assertion in calculateOwnerReserveFee that increment > base * 100. Initialize owner_reserve directly with 200_000 drops (= 0.2 XRP).
c267be8 to
1502514
Compare
|
|
||
| /** The account reserve requirement in drops. */ | ||
| XRPAmount account_reserve{10 * DROPS_PER_XRP}; | ||
| XRPAmount account_reserve{1 * DROPS_PER_XRP}; |
There was a problem hiding this comment.
@mvadari would it be more elegant to change this to XRPAmount account_reserve{1'000'000}; // 1 XRP?
We can also wait until the value changes in the future again in a small amount up or down, but it'd be easier to change then and aesthetically it'd be consistent with the owner reserve below.
|
@xVet it looks like the adjustment results in a test failure: The relevant code is here: Since the account reserve went from 10 XRP to 1 XRP, I suppose it tried to send 0 drops, hence the error. It might be worthwhile changing this to use the |
Ahh yes, i see, thank you! |
The test paid (accountReserve - XRP(1)) to verify a too-small payment is rejected with tecNO_DST_INSUF_XRP. With the new 1 XRP account reserve, that becomes 0 drops and is rejected earlier with temBAD_AMOUNT. Subtract 1 drop instead of 1 XRP so the payment lands just below the reserve regardless of its absolute value.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6382 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 1010 1010
Lines 76024 76024
Branches 7383 7369 -14
=======================================
+ Hits 62401 62402 +1
+ Misses 13623 13622 -1
🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
We recently merged a refactor to One-time setupIf 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 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-tidy2. Reconfigure conan/cmake so 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 called4. 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/developExtraRun 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 |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
@xVet ready to merge? (I see some builds are flaky - I'll retry them once all pipelines complete.) |
Ready! |
…F#6382) Co-authored-by: Bart <bthomee@users.noreply.github.com>
High Level Overview of Change
Updated the old base reserve 10 XRP and owner reserve 2 XRP to the current network levels 1 XRP and 0.2 XRP respectively.
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)