feat: add Account Permission Delegation (XLS-74d, XLS-75d) #2981
Conversation
WalkthroughThis change introduces support for account permissions and permission delegation in accordance with XLS-74d and XLS-75d standards. It adds new transaction and ledger entry types, validation logic, configuration flags, and comprehensive tests to enable and verify delegated permissions on the XRPL. Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| try { | ||
| await testTransaction(testContext.client, tx, bob) | ||
| } catch (err: unknown) { | ||
| const assertErr = err as AssertionError | ||
| assert.equal( | ||
| assertErr.message, | ||
| "No permission to perform requested operation.: expected 'tecNO_PERMISSION' to equal 'tesSUCCESS'", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Can the framework be adjusted to take other optional error codes instead of needing this?
There was a problem hiding this comment.
Do you mean adding an optional param like this: await testTransaction(testContext.client, tx, bob, errCode)
There was a problem hiding this comment.
Nevermind, got it working in 2f1f6a2
There was a problem hiding this comment.
Shouldn't that be used here?
There was a problem hiding this comment.
Ah yes, missed the update here. Done in a192433
There was a problem hiding this comment.
Oh actually, since tecNO_PERMISSION is already being tested in base flow, I removed it from here.
packages/xrpl/test/integration/transactions/delegateSet.test.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/delegateSet.test.ts (2)
49-57: Consider using the new errCode parameter for consistency.The test catches an AssertionError to verify the
tecNO_PERMISSIONerror, but it would be more consistent to use the newerrCodeparameter that was added specifically for this purpose.- try { - await testTransaction(testContext.client, tx, bob) - } catch (err: unknown) { - const assertErr = err as AssertionError - assert.equal( - assertErr.message, - "No permission to perform requested operation.: expected 'tecNO_PERMISSION' to equal 'tesSUCCESS'", - ) - } + await testTransaction(testContext.client, tx, bob, undefined, 'tecNO_PERMISSION')
40-41: Consider a more descriptive test name.The current test name "no permission" could be more descriptive to clearly communicate what's being tested.
- 'no permission', + 'rejects delegated transaction without prior authorization',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/xrpl/test/integration/transactions/delegateSet.test.ts(1 hunks)packages/xrpl/test/integration/utils.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: unit (18.x)
🔇 Additional comments (5)
packages/xrpl/test/integration/utils.ts (1)
235-236: Appropriate enhancement for handling expected failures.The addition of the
errCodeparameter enables more efficient testing of cases where transactions are expected to fail with specific error codes. This is particularly valuable for testing permission-related features like the new account delegation functionality.Also applies to: 248-248, 263-266
packages/xrpl/test/integration/transactions/delegateSet.test.ts (4)
33-35: Good use of test context wallet to reduce funding time.Using
testContext.walletfor Alice instead of generating a new wallet is an efficient approach that reduces test initialization time.
78-91: Well-structured verification of Delegate ledger entry.The test properly verifies the creation of the Delegate ledger entry with all the expected fields, ensuring the delegation was properly recorded.
93-106: Thorough validation of delegated payment transaction.The test comprehensively verifies that Bob can successfully execute a payment on behalf of Alice, checking that the transaction includes the correct Account (Alice), Delegate (Bob), and is properly signed with Bob's key.
108-123: Good negative test case for insufficient permissions.The test correctly verifies that Bob cannot perform an AccountSet operation for which he hasn't been authorized, using the new errCode parameter to check for the expected tecNO_PERMISSION error.
|
Nice job |
High Level Overview of Change
Adds support for Account Permission Delegation (XLS-74d, XLS-75d)
Context of Change
rippled PR: XRPLF/rippled#5354.
Type of Change
Did you update HISTORY.md?
Test Plan
Adds unit/integ tests.