Transaction model, unit and integ tests for Delegate XLS-74d amendment #833
Conversation
WalkthroughThis update introduces support for delegate-based account permissions in the XRPL Python SDK. It adds new transaction and ledger entry types for delegation, including the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant XRPL SDK
participant Ledger
Client->>XRPL SDK: Create DelegateSet transaction (authorize, permissions)
XRPL SDK->>Ledger: Submit DelegateSet transaction
Ledger-->>XRPL SDK: DelegateSet transaction result
Client->>XRPL SDK: Submit Payment (signed by delegate)
XRPL SDK->>Ledger: Process Payment (check delegate permissions)
Ledger-->>XRPL SDK: Payment result (success/failure)
sequenceDiagram
participant Client
participant XRPL SDK
participant Ledger
Client->>XRPL SDK: Query LedgerEntry (delegate)
XRPL SDK->>Ledger: Fetch delegate ledger entry
Ledger-->>XRPL SDK: Return delegate ledger entry details
XRPL SDK-->>Client: Delegate ledger entry response
Suggested reviewers
Poem
📜 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 (6)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
xrpl/models/transactions/transaction.py (1)
27-29: Consider adding parentheses for multi-line function parameters.Adding a trailing comma to the parameter list is good practice for future extensibility, but consider also enclosing the parameter in parentheses for better readability of multi-line function signatures.
- def transaction_json_to_binary_codec_form( - dictionary: Dict[str, XRPL_VALUE_TYPE], - ) -> Dict[str, XRPL_VALUE_TYPE]: + def transaction_json_to_binary_codec_form( + ( + dictionary: Dict[str, XRPL_VALUE_TYPE], + ) + ) -> Dict[str, XRPL_VALUE_TYPE]:tests/integration/transactions/test_delegate_set.py (1)
84-122: LGTM: Delegate ledger entry query test.This test properly validates that delegate ledger entries can be queried using the LedgerEntry request with the new Delegate model, and checks that the returned data contains the expected fields.
Pipeline error note: The test failure appears to be unrelated to your code implementation and is likely due to connection issues with the test environment.
xrpl/models/transactions/delegate_set.py (1)
88-99: Consider validating permission values against known permissions.The current implementation checks for duplicate permissions and enforces a maximum list length, which is good. However, it doesn't validate whether the
permission_valuevalues are known/valid according to theGRANULAR_PERMISSIONSdictionary.Consider adding validation to ensure only recognized permission values are used, or document if arbitrary values are intentionally allowed.
def _get_permissions_error(self: Self) -> Optional[str]: if len(self.permissions) > PERMISSION_MAX_LENGTH: return ( f"Length of `permissions` list is greater than {PERMISSION_MAX_LENGTH}." ) permission_unique_values = set() for permission in self.permissions: + # Optional: Validate against known permissions + if permission.permission_value not in GRANULAR_PERMISSIONS.values(): + return f"Unknown permission value: {permission.permission_value}" + if permission.permission_value in permission_unique_values: return "Duplicate permission value in `permissions` list." permission_unique_values.add(permission.permission_value) return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_delegate_set.py(1 hunks)tests/unit/models/transactions/test_delegate_set.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(6 hunks)xrpl/models/requests/ledger_entry.py(3 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/delegate_set.py(1 hunks)xrpl/models/transactions/transaction.py(3 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
xrpl/models/transactions/__init__.py (1)
xrpl/models/transactions/delegate_set.py (1)
DelegateSet(55-99)
tests/unit/models/transactions/test_delegate_set.py (3)
xrpl/models/exceptions.py (1)
XRPLModelException(6-9)xrpl/models/transactions/delegate_set.py (2)
DelegateSet(55-99)Permission(40-50)xrpl/models/base_model.py (1)
is_valid(292-299)
tests/integration/transactions/test_delegate_set.py (8)
tests/integration/integration_test_case.py (1)
IntegrationTestCase(9-18)tests/integration/it_utils.py (3)
fund_wallet_async(117-126)sign_and_reliable_submission_async(188-220)test_async_and_sync(265-360)xrpl/models/requests/ledger_entry.py (2)
LedgerEntry(316-394)Delegate(71-89)xrpl/models/response.py (2)
ResponseStatus(23-27)is_successful(74-82)xrpl/models/transactions/delegate_set.py (2)
DelegateSet(55-99)Permission(40-50)xrpl/models/transactions/payment.py (1)
Payment(69-182)xrpl/utils/xrp_conversions.py (1)
xrp_to_drops(25-70)xrpl/wallet/main.py (3)
Wallet(16-290)create(120-134)address(24-29)
🪛 GitHub Actions: Unit test
tests/unit/models/transactions/test_delegate_set.py
[error] 77-77: flake8: line too long (90 > 88 characters) (E501)
xrpl/models/transactions/delegate_set.py
[error] 19-19: flake8: trailing whitespace (W291)
🪛 GitHub Actions: Integration test
tests/unit/models/transactions/test_delegate_set.py
[error] 1-20: Setup failure: ConnectionRefusedError: Connect call failed to 127.0.0.1:6006 during websocket client open.
tests/integration/transactions/test_delegate_set.py
[error] 12-12: ConnectionRefusedError: Connect call failed ('127.0.0.1', 6006) during WebSocket client open in setUpClass.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (21)
.ci-config/rippled.cfg (1)
199-199: LGTM: Added PermissionDelegation amendmentThe configuration has been correctly updated to include the new PermissionDelegation amendment, which enables the account delegation features introduced in this PR.
xrpl/models/transactions/types/transaction_type.py (1)
25-25: LGTM: Added DELEGATE_SET transaction typeThe TransactionType enum has been properly extended with the new DELEGATE_SET entry, allowing for DelegateSet transactions in the XRPL.
CHANGELOG.md (1)
17-17: LGTM: Added changelog entry for new featuresThe changelog has been properly updated to document the addition of Account Permission and Account Permission Delegation features with appropriate references to XLS-74d and XLS-75d.
xrpl/models/transactions/__init__.py (2)
35-35: LGTM: Added DelegateSet importThe DelegateSet class is now properly imported from its module.
145-145: LGTM: Added DelegateSet to exportsDelegateSet has been correctly added to the all list, making it publicly accessible when importing from the package.
tests/unit/models/transactions/test_delegate_set.py (2)
1-79: Comprehensive test coverage for DelegateSet transactionsThe test suite thoroughly validates the DelegateSet transaction model's core functionality and constraints, including basic operations, granular permissions, and various validation scenarios.
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 77-77: flake8: line too long (90 > 88 characters) (E501)
🪛 GitHub Actions: Integration test
[error] 1-20: Setup failure: ConnectionRefusedError: Connect call failed to 127.0.0.1:6006 during websocket client open.
1-20:❓ Verification inconclusive
Note the integration test connection failure
There is a pipeline failure in the integration tests showing a connection error to the websocket client. This might be an infrastructure issue rather than a code problem.
Verify that the rippled server configured with the PermissionDelegation amendment is running and accessible during integration tests:
🏁 Script executed:
#!/bin/bash # Check if the rippled server is running and accessible curl -s http://127.0.0.1:5005 -X POST -H "Content-Type: application/json" \ -d '{"method": "server_info", "params": [{}]}' | grep -i "error\|success" # Check if PermissionDelegation amendment is enabled curl -s http://127.0.0.1:5005 -X POST -H "Content-Type: application/json" \ -d '{"method": "feature", "params": [{"feature": "PermissionDelegation"}]}' | grep -i "status\|enabled"Length of output: 329
Ensure rippled node is running on port 5005 with PermissionDelegation enabled
The integration tests are failing with a connection error, and the previous
curlchecks produced no output. Please verify the following before rerunning the pipeline:
- The
rippledprocess is up and listening on 127.0.0.1:5005 (e.g.lsof -i:5005ornetstat -an | grep 5005).- You can reach the server via RPC:
Expect to see acurl -s http://127.0.0.1:5005 \ -X POST -H "Content-Type: application/json" \ -d '{"method": "server_info","params":[{}]}' | jq"status":"success"response.- The
PermissionDelegationamendment is enabled in yourrippled.cfg:Look forcurl -s http://127.0.0.1:5005 \ -X POST -H "Content-Type: application/json" \ -d '{"method": "feature","params":[{"feature":"PermissionDelegation"}]}' | jq"enabled":true.Once confirmed, re-run the integration tests.
🧰 Tools
🪛 GitHub Actions: Integration test
[error] 1-20: Setup failure: ConnectionRefusedError: Connect call failed to 127.0.0.1:6006 during websocket client open.
xrpl/models/transactions/transaction.py (1)
256-258: LGTM: New delegate field added to support delegate transactions.This new optional field properly supports the delegate functionality being added in the XLS-74d amendment, allowing transactions to be signed by a delegate account.
xrpl/core/binarycodec/definitions/definitions.json (6)
673-682: LGTM: New PermissionValue field definition added.This properly defines the PermissionValue field as a UInt32 type with appropriate serialization and signing flags for the delegation feature.
1963-1972: LGTM: New Delegate field definition added.This properly defines the Delegate field as an AccountID type with appropriate serialization and signing flags for the delegation feature.
2183-2192: LGTM: New Permission object definition added.This properly defines the Permission field as an STObject type with appropriate serialization and signing flags for the delegation feature.
2582-2590: LGTM: New Permissions array definition added.This properly defines the Permissions field as an STArray type with appropriate serialization and signing flags for the delegation feature.
2910-2910: LGTM: New Delegate ledger entry type definition added.The Delegate ledger entry type is properly defined with a unique code (131) that doesn't conflict with existing ledger entry types.
3131-3131: LGTM: New DelegateSet transaction type definition added.The DelegateSet transaction type is properly defined with a unique code (64) that doesn't conflict with existing transaction types.
xrpl/models/requests/ledger_entry.py (3)
69-90: LGTM: New Delegate model for ledger entry queries.The Delegate class is properly implemented as a dataclass with required account and authorize fields, matching the structure of the delegate ledger entry. The documentation clearly explains its purpose.
330-330: LGTM: Added delegate field to LedgerEntry class.This properly adds the optional delegate field to the LedgerEntry class, allowing for querying delegate ledger entries.
365-365: LGTM: Updated validation logic to include delegate field.The _get_errors method is properly updated to include the delegate field in the list of mutually exclusive query parameters, maintaining the validation that exactly one ledger entry identifier is provided.
tests/integration/transactions/test_delegate_set.py (3)
1-14: LGTM: Appropriate imports for delegate feature tests.All necessary imports are included for testing the delegate feature, including the new DelegateSet transaction type, Permission model, and Delegate ledger entry model.
🧰 Tools
🪛 GitHub Actions: Integration test
[error] 12-12: ConnectionRefusedError: Connect call failed ('127.0.0.1', 6006) during WebSocket client open in setUpClass.
16-41: LGTM: Test for unauthorized delegation.This test correctly verifies that a delegate cannot execute a transaction without proper permission, validating the security aspect of the delegate feature.
42-83: LGTM: Complete delegate workflow test.This test case properly validates the complete delegate flow:
- Setting permissions with DelegateSet transaction
- Executing a Payment transaction as a delegate
- Verifying the transaction was properly signed by the delegate
The permission value (1 + 0) corresponds to the Payment transaction type (0) with a permission bit (1).
xrpl/models/transactions/delegate_set.py (1)
1-100: Overall implementation looks solid.The
DelegateSettransaction model and supportingPermissionclass are well-structured and follow the design patterns of the XRPL SDK. The implementation includes appropriate validation logic to ensure:
- The authorizing account is different from the transaction account
- No more than 10 permissions are included
- No duplicate permissions exist
The utility dictionary of granular permissions provides good documentation for developers implementing this feature.
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 19-19: flake8: trailing whitespace (W291)
| ) | ||
| self.assertEqual( | ||
| error.exception.args[0], | ||
| "{'account_addresses': 'Field `authorize` and `account` must be different.'}", |
There was a problem hiding this comment.
Fix line length to comply with flake8
The line exceeds the maximum length limit (90 > 88 characters).
- "{'account_addresses': 'Field `authorize` and `account` must be different.'}",
+ "{'account_addresses': 'Field `authorize` and `account` must be different.'}",Consider either:
- Breaking this line into multiple lines, or
- Using a shorter error message in the test
- "{'account_addresses': 'Field `authorize` and `account` must be different.'}",
+ "{'account_addresses': 'Field `authorize` and `account` must be "
+ "different.'}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "{'account_addresses': 'Field `authorize` and `account` must be different.'}", | |
| "{'account_addresses': 'Field `authorize` and `account` must be " | |
| "different.'}", |
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 77-77: flake8: line too long (90 > 88 characters) (E501)
There was a problem hiding this comment.
The linter (and flake8) haven't thrown any errors over this line. This is not a necessary update.
There was a problem hiding this comment.
@ckeshava I understand you're not seeing this error locally, but the GitHub Actions CI shows a flake8 error for this line: "flake8: line too long (90 > 88 characters) (E501)".
This discrepancy might be due to different flake8 configurations between your local environment and the CI pipeline. The CI is enforcing an 88-character limit, which this line exceeds at 90 characters.
Would you like me to help investigate the flake8 configuration in the project to resolve this inconsistency?
There was a problem hiding this comment.
updated in 1d4b3d8
| f"Length of `permissions` list is greater than {PERMISSION_MAX_LENGTH}." | ||
| ) | ||
|
|
||
| permission_unique_values = set() |
There was a problem hiding this comment.
Probably can just use if self.permissions != set(self.permissions)
There was a problem hiding this comment.
thanks. updated in 4b1491b
xrpl/models/requests/ledger_entry.py
Outdated
|
|
||
| authorize: str = REQUIRED # type: ignore | ||
| """ | ||
| This field is required. |
There was a problem hiding this comment.
Could you add description for this field?
There was a problem hiding this comment.
added description in 4b1491b
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@pdp2121 @khancode Please hold for some time reviewing this PR as I am going to raise a new PR with a few changes with respect to I'll raise a PR once the cpp PR is merged in develop and we have a docker image to write integration tests. |
High Level Overview of Change
This PR implements changes pertaining to the XLS-74d and XLS-75d amendments.
Here is the associated cpp implementation.
Note: This PR succeeds https://github.com/XRPLF/xrpl-py/pull/807/files. I chose to create a new PR owing to the major updates to the specification.
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Appropriate unit and integration tests have been added to this PR. The integration tests are validated by running a local rippled executable compiled from the attached feature-branch. As of today, this executable is not available as a docker container because the cpp-implementation hasn't been merged into
developbranch ofrippledrepository.