Sponsored Content
Skip to content

fix: Mark SAV and Lending transactions as NotDelegable - #6489

Merged
bthomee merged 6 commits into
XRPLF:developfrom
yinyiqian1:disable-new-feature-delegation
Mar 11, 2026
Merged

fix: Mark SAV and Lending transactions as NotDelegable#6489
bthomee merged 6 commits into
XRPLF:developfrom
yinyiqian1:disable-new-feature-delegation

Conversation

@yinyiqian1

Copy link
Copy Markdown
Contributor

Any new transactions should be marked as NotDelegable, unless the interactions are fully tested and confirmed.

High Level Overview of Change

Context of Change

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

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@yinyiqian1
yinyiqian1 requested review from mvadari and ximinez March 5, 2026 21:35

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

I would not object if you added the "Trivial" label to this PR.

@yinyiqian1
yinyiqian1 force-pushed the disable-new-feature-delegation branch 2 times, most recently from 5a03e2d to 9faf4f2 Compare March 5, 2026 22:03
@yinyiqian1

Copy link
Copy Markdown
Contributor Author

I would not object if you added the "Trivial" label to this PR.

It looks like some vault tests are related to delegation. I’ve removed them for now—please let me know if that’s okay with you. We can still add back if we want to support in the future.

@yinyiqian1
yinyiqian1 force-pushed the disable-new-feature-delegation branch from 9faf4f2 to 8013a0a Compare March 5, 2026 22:34
Comment on lines 1577 to 1585
testTxRequireFeatures(FeatureBitset features)
{
testcase("test delegate disabled tx");
using namespace jtx;

// map of tx and required feature.
// non-delegable tx are not included.
// NFTokenMint, NFTokenBurn, NFTokenCreateOffer, NFTokenCancelOffer,
// NFTokenAcceptOffer are not included, they are tested separately.

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.

nit: would recommend adding a comment to this test to let others know that when they add a tx to this list that they make sure they have unit tested delegation with their feature

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just forced a delegable tx count check in a separate test case, which will contain those comments:
483a545
Any change to either adding/deleting a delegable tx, will trigger failure.

@codecov

codecov Bot commented Mar 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (ce9ccf8) to head (a7a46e6).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6489   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          862     862           
  Lines        67883   67883           
  Branches      7550    7547    -3     
=======================================
+ Hits         54200   54203    +3     
+ Misses       13683   13680    -3     
Files with missing lines Coverage Δ
include/xrpl/protocol/detail/transactions.macro 100.0% <ø> (ø)

... and 2 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 commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

@ximinez should this go into Lending 1.1? If so, should the PR be against tapanito/lending-fix-amendment ?

@ximinez

ximinez commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

should this go into Lending 1.1? If so, should the PR be against tapanito/lending-fix-amendment ?

@Tapanito It could, but I don't think it needs to.

  1. It is controlled by the delegation amendment, not lending 1.1. Because delegation is not yet supported, it can be changed without a separate amendment.
  2. They're both going to the same place (develop). If lending-fix-amendment ends up not going to develop, it'll be moot because the delegation amendment is unsupported.
  3. @yinyiqian1 wrote this under the auspices of delegation. It seems to be an inappropriate mixing of the two projects to put it under lending-fix-amendment.
  4. Finally, if @yinyiqian1 has more delegation-related stuff to do, it will probably be completely irrelevant to lending. Having this fix off to the side in another branch is only going to cause unnecessary complexity.

@yinyiqian1 yinyiqian1 added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Mar 10, 2026
@yinyiqian1

Copy link
Copy Markdown
Contributor Author

@ximinez Can we merge it if it looks good to you?

@kennyzlei kennyzlei 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 Mar 10, 2026
@bthomee bthomee changed the title fix: SAV and Lending as NotDelegable fix: Marks SAV and Lending transactions as NotDelegable Mar 11, 2026
@bthomee bthomee changed the title fix: Marks SAV and Lending transactions as NotDelegable fix: Mark SAV and Lending transactions as NotDelegable Mar 11, 2026
@bthomee
bthomee enabled auto-merge (squash) March 11, 2026 21:19
@bthomee
bthomee merged commit 46d5c67 into XRPLF:develop Mar 11, 2026
3 checks passed
@yinyiqian1
yinyiqian1 deleted the disable-new-feature-delegation branch May 7, 2026 18:44
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
New transactions should be marked as `NotDelegable`, until the interactions with other transactions have been fully tested and validated.
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. Trivial Simple change with minimal effect, or already tested. Only needs one approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants