feat: Introduce lending 1.1 amendment and add MemoData field to VaultDelete transaction#6324
Conversation
Tapanito
force-pushed
the
tapanito/lending-fix-amendment
branch
2 times, most recently
from
cb45787 to
3422c11
Compare
Tapanito
added
the
DraftRunCI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6324 +/- ##
=========================================
- Coverage 82.3% 82.3% -0.0%
=========================================
Files 1024 1024
Lines 78384 78397 +13
Branches 8978 8978
=========================================
- Hits 64501 64498 -3
- Misses 13874 13890 +16
Partials 9 9
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Tapanito
changed the title
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
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. |
Tapanito
changed the title
MemoData field to VaultDelete transaction
Tapanito
removed
the
DraftRunCI
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Had a look at this
One correctness bug: missing env.close() after enableFeature means the "data too large" and "data empty" sub-cases never actually test the enabled-feature path — see inline.
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Tapanito
added
the
Ready to merge
There was a problem hiding this comment.
Pull request overview
This PR introduces the featureLendingProtocolV1_1 amendment and uses it to gate a new optional sfMemoData field on the VaultDelete transaction, allowing a bounded “deletion reason” payload to be included and validated at preflight time.
Changes:
- Registers the new
LendingProtocolV1_1amendment infeatures.macro. - Adds optional
sfMemoDatatoVaultDelete(macro + protocol_autogen wrapper/builder + protocol_autogen tests). - Implements amendment guarding and payload length validation in
VaultDelete::preflight, with corresponding app-level unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libxrpl/tx/transactors/vault/VaultDelete.cpp | Adds amendment gate + sfMemoData size validation in preflight. |
| include/xrpl/protocol/detail/transactions.macro | Declares sfMemoData as an optional field on ttVAULT_DELETE. |
| include/xrpl/protocol/detail/features.macro | Registers the new LendingProtocolV1_1 amendment. |
| include/xrpl/protocol_autogen/transactions/VaultDelete.h | Updates generated wrapper/builder with getMemoData() / setMemoData(). |
| src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp | Extends generated transaction wrapper tests for the new optional field. |
| src/test/app/Vault_test.cpp | Adds integration-style test coverage for disabled/invalid/valid sfMemoData cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Summary
Introduces the
featureLendingProtocolV1_1amendment and a new feature it gates:sfMemoDataonVaultDelete: Allows callers to record a deletion reason (up tokMaxDataPayloadLengthbytes) when closing a vault. The field is validated for non-zero length inpreflight; submitting it without the amendment enabled returnstemDISABLED. Autogen accessor/builder updated accordingly.Changes
features.macrofeatureLendingProtocolV1_1(Supported::No,DefaultNo)transactions.macro+ autogensfMemoData(optional) toVaultDeleteVaultDelete.cpppreflightvalidatessfMemoDatalength and amendment guardVault_test.cpptestVaultDeleteData— covers disabled/oversized/empty/valid casesHigh Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)