Skip to content

Added account permission delegation#689

Open
cybele-ripple wants to merge 30 commits intomainfrom
account-permission-delegation
Open

Added account permission delegation#689
cybele-ripple wants to merge 30 commits intomainfrom
account-permission-delegation

Conversation

@cybele-ripple
Copy link
Collaborator

Adds support for account permission delegation (XLS-0075)

/**
* Set of transaction types that cannot be delegated.
*/
Set<String> NON_DELEGABLE_TRANSACTIONS = Sets.newHashSet(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use TransactionType enum so that string value remains consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing this fix implemented. Can we keep comments unresolved until they're pushed (to aid in code review)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@Patel-Raj11 Patel-Raj11 Feb 20, 2026

Choose a reason for hiding this comment

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

I resolved this because the values were at least using TransactionType enum to get string value. But, yes we should do this - Set<TransactionType> NON_DELEGABLE_TRANSACTIONS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use Set NON_DELEGABLE_TRANSACTIONS

transactionWithSignature = CredentialDelete.builder().from((CredentialDelete) transaction)
.transactionSignature(signature)
.build();
} else if (DelegateSet.class.isAssignableFrom(transaction.getClass())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Since we are adding tests in SignatureUtilsTest for all transaction types, we should add it for DelegateSet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this comment as resolved, but when I run SignatureUtilsTest.java with coverage, it shows this as still uncovered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added coverage for this line and reran tests with coverage to confirm

sappenin and others added 7 commits February 18, 2026 15:32
* Introduce `transactionFlags()` in a manner that doesn’t break immutables.
* Add unit tests to all transactions to verify functionality.
* Note this addition is required for Batch functionality to verify inner transaction flags in a generic manner.
First of three PRs for adding support for Batch transactions (XLS-56)

* Update tfInnerBatchTxn to all transactions
* Increase coverage of BatchFlags
…r permission object with different string vals
* change.</p>
*/
@Beta
BATCH("Batch"),
Copy link
Collaborator

@sappenin sappenin Feb 20, 2026

Choose a reason for hiding this comment

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

Note: This branch was updated via GH to pull-in changes from main. Because Batch is now in main, this enum is a duplicate, and breaks the build. We'll want to remove this one to get the build working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed enum

…lign with enum, removed package-lock, removed beta batch (becuse batch now in main)
Copy link
Collaborator

@Patel-Raj11 Patel-Raj11 left a comment

Choose a reason for hiding this comment

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

Looks good for beta release.

/**
* Set of transaction types that cannot be delegated.
*/
Set<String> NON_DELEGABLE_TRANSACTIONS = Sets.newHashSet(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing this fix implemented. Can we keep comments unresolved until they're pushed (to aid in code review)?

@Test
public void testAllNonDelegatableTransactionTypesWithEnum() {
// Test all non-delegatable transaction types using TransactionType enum
TransactionType[] nonDelegatableTypes = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we instead reuse DelegateSet#NON_DELEGABLE_TRANSACTIONS here?

Overall, I worry too about future transaction added to the library that eventually become untested, either here or in testAllDelegatableTransactionTypes. Maybe we should first take all transactions in the enum, and remove the undelegateble ones? That way, if anyone adds a new TransactionType in the future, it will get included in here automatically.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test now takes all TransactionType vals
As of now, new delegable transaction types just need to be added to the TransactionType enum. Adding non delegable transactions just involves an additional change to a single file so I think the current implementation is fine.

@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

❌ Patch coverage is 98.21429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.99%. Comparing base (1063883) to head (7cc6652).

Files with missing lines Patch % Lines
...j/codec/binary/definitions/DefinitionsService.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #689      +/-   ##
============================================
+ Coverage     92.91%   92.99%   +0.07%     
- Complexity     2220     2265      +45     
============================================
  Files           437      443       +6     
  Lines          6025     6135     +110     
  Branches        536      549      +13     
============================================
+ Hits           5598     5705     +107     
- Misses          279      281       +2     
- Partials        148      149       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

<artifactId>xrpl4j-parent</artifactId>
<groupId>org.xrpl</groupId>
<version>HEAD-SNAPSHOT</version>
<version>HEAD-SNAPSOT</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a typo.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants