Implement account permission delegation - XLS-74d and XLS-75d amendments #840
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…permission-delegation
WalkthroughThis update introduces support for account permission delegation in the XRPL Python SDK. It adds new transaction and ledger entry types, models, and binary codec definitions for permission delegation. The changes include validation logic, test coverage for the new features, and updates to configuration, changelog, and public APIs to reflect the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant XRPL Network
Client->>SDK: Create DelegateSet transaction (authorize, permissions)
SDK->>SDK: Validate permissions (length, duplicates, delegatable)
SDK->>XRPL Network: Submit DelegateSet transaction
XRPL Network-->>SDK: Confirm transaction, update ledger with Delegate entry
SDK-->>Client: Return transaction result and ledger state
Client->>SDK: Submit delegated transaction (signed by delegate)
SDK->>XRPL Network: Submit transaction
XRPL Network-->>SDK: Check permissions, accept or reject
SDK-->>Client: Return result (success or tecNO_PERMISSION)
Poem
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 (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🔇 Additional comments (2)
✨ 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
|
| PERMISSION_MAX_LENGTH = 10 | ||
|
|
||
|
|
||
| class DelegatableTransaction(str, Enum): |
There was a problem hiding this comment.
This should use the Transaction class to make maintenance easier. You can check for the invalid tests in the _get_errors function instead of at the type level.
There was a problem hiding this comment.
@mvadari Do you mean taking in TransactionType from user and checking for the presence of any of these AccountSet, SetRegularKey, SignerListSet, DelegateSet and AccountDelete transactions in _get_errors and throwing an error?
I first thought of this approach but, it seemed that we are allowing an invalid Transaction Type as a valid input and then later throwing an error stating its invalid.
There was a problem hiding this comment.
Not everything has to be checked at the typing level IMO. This will be much more difficult to maintain, and require a separate set of imports.
There was a problem hiding this comment.
I agree with @mvadari and rather not use this approach. Reason is each time a new transaction is added, then this module will have to be updated to keep it accurate which makes it harder to maintain.
| for k, v in _GRANULAR_PERMISSIONS.items(): | ||
| if v == value: | ||
| return k |
There was a problem hiding this comment.
Is there a better way to do this? Perhaps it should use a helper function like the others?
![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 (7)
xrpl/models/transactions/__init__.py (1)
149-156: Reorder all entries for alphabetical consistencyTo keep the
__all__list alphabetized, move the new entries beforeDepositPreauthand arrange them in proper order. For example:- "CredentialDelete", - "DepositPreauth", - "DelegateSet", - "DelegatableTransaction", - "GranularPermission", - "DIDDelete", + "CredentialDelete", + "DelegatableTransaction", + "DelegateSet", + "DepositPreauth", + "DIDDelete", + "GranularPermission",This ensures
DelegatableTransactionandDelegateSetappear beforeDepositPreauth, andGranularPermissionfollows its alphabetical slot.xrpl/core/binarycodec/definitions/__init__.py (1)
35-36: Reorder__all__for logical groupingFor consistency with the other
get_functions, consider moving the newget_permission_value_type_*entries adjacent to the otherget_transaction_*andget_ledger_entry_*exports. For instance:- "get_transaction_type_name", - "get_permission_value_type_code", - "get_permission_value_type_name", ] + "get_permission_value_type_code", + "get_permission_value_type_name", + "get_transaction_type_name", ]This groups all permission-related functions together and maintains the export list’s logical flow.
xrpl/core/binarycodec/definitions/definitions.json (2)
1964-1972:DelegateAccountID entry – collision check & completenessNice to see
nth: 12continuing the AccountID sequence.
Two small checks worth running:
- Verify no other AccountID already uses 12.
- Confirm accompanying test-vectors cover serialization/deserialization of this new field (to avoid silent endian/length issues).
If either is missing, please add.
3150-3153: TransactionTypes – keep enum & codec in sync
"DelegateSet": 64is added here.
Please ensure:
xrpl/models/transactions/types/transaction_type.pycontainsDELEGATE_SET = 64.- Binary codec integer ↔︎ string mapping includes the new entry.
Missing either will cause
ValueErroron (de)serialization.tests/integration/transactions/test_delegate_set.py (2)
60-69: Avoid hard-coding list indices when asserting permissionsThe assertions later in the test depend on the permissions being exactly two and in a fixed order.
Iterating defensively avoidsIndexErrorif the on-ledger array order ever changes:- granted_permission.add( - entry["Permissions"][0]["Permission"]["PermissionValue"] - ) - granted_permission.add( - entry["Permissions"][1]["Permission"]["PermissionValue"] - ) + for perm in entry["Permissions"]: + granted_permission.add(perm["Permission"]["PermissionValue"])
170-196:ledger_indexvariable overwritten inside pagination loopInside the pagination while-loop you mutate
ledger_indexwith the server-returned value.
This makes the next request use an explicit numeric ledger rather than"validated", which is harmless but unnecessary and can complicate flaky-ledger scenarios.Simply omit the assignment and keep the original
"validated"literal:- ledger_index = ledger_data_response.result["ledger_index"]xrpl/models/transactions/delegate_set.py (1)
154-163:_get_errors– dict-merge creates keys even when valid
_get_errorscurrently injects"permissions"and"account_addresses"keys whose values may beNone, then filters them out.
Minor, but generating fewer temporaries is cleaner & fractionally faster:- return { - key: value - for key, value in { - **super()._get_errors(), - "permissions": self._get_permissions_error(), - "account_addresses": self._validate_account_addresses(), - }.items() - if value is not None - } + errors = super()._get_errors() + if (perm_err := self._get_permissions_error()): + errors["permissions"] = perm_err + if (addr_err := self._validate_account_addresses()): + errors["account_addresses"] = addr_err + return errors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_delegate_set.py(1 hunks)tests/unit/core/binarycodec/fixtures/data/serialized-dict-fixtures.json(1 hunks)tests/unit/core/binarycodec/types/test_serialized_dict.py(1 hunks)tests/unit/models/transactions/test_delegate_set.py(1 hunks)tests/unit/models/transactions/test_payment.py(2 hunks)xrpl/core/binarycodec/definitions/__init__.py(2 hunks)xrpl/core/binarycodec/definitions/definitions.json(6 hunks)xrpl/core/binarycodec/definitions/definitions.py(3 hunks)xrpl/core/binarycodec/types/st_object.py(3 hunks)xrpl/models/requests/ledger_entry.py(4 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/core/binarycodec/definitions/__init__.py (1)
xrpl/core/binarycodec/definitions/definitions.py (2)
get_permission_value_type_code(281-294)get_permission_value_type_name(297-310)
xrpl/models/transactions/__init__.py (1)
xrpl/models/transactions/delegate_set.py (3)
DelegatableTransaction(20-72)DelegateSet(137-179)GranularPermission(75-115)
tests/unit/core/binarycodec/types/test_serialized_dict.py (2)
xrpl/core/binarycodec/binary_wrappers/binary_parser.py (1)
BinaryParser(31-261)xrpl/core/binarycodec/types/st_object.py (4)
STObject(91-247)from_value(128-228)to_json(230-247)from_parser(95-125)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.9)
🔇 Additional comments (36)
.ci-config/rippled.cfg (1)
199-199: Enable PermissionDelegation amendment in CI configThe
PermissionDelegationflag has been added under the[features]stanza to enable the new account permission delegation amendment during CI testing. Ensure that the rippled server version used in your CI pipeline includes support for this amendment to avoid startup failures.xrpl/models/transactions/types/transaction_type.py (1)
25-25: Add DELEGATE_SET transaction typeIntroduces
DELEGATE_SET = "DelegateSet"to theTransactionTypeenum, aligning the enum with the newly implementedDelegateSettransaction model. The placement maintains the existing alphabetical ordering of transaction types.CHANGELOG.md (1)
17-17: Update changelog for Account Permission DelegationDocuments the addition of “Account Permission” and “Account Permission Delegation” (XLS-74d, XLS-75d) under the Unreleased section. This correctly captures the new features introduced by this PR.
xrpl/models/transactions/__init__.py (1)
35-39: Expose DelegateSet models in transactions packageImports
DelegatableTransaction,DelegateSet, andGranularPermissionfrom the newdelegate_setmodule, making these types available as part of the public transactions API. The import position follows the existing alphabetical grouping by module name.xrpl/core/binarycodec/definitions/__init__.py (1)
9-10: Import new permission value functionsAdds
get_permission_value_type_codeandget_permission_value_type_nameto the import list, exposing the mappings between permission names and their binary codes in the definitions API. This is required for serializing and deserializing granular permissions.xrpl/core/binarycodec/types/st_object.py (3)
16-17: Good addition of permission value type imports.The imports properly extend the list of definitions for field type conversion, maintaining consistency with the existing pattern for other field types.
73-74: Correctly implemented permission value string-to-enum conversion.This implementation follows the established pattern used for other field types in the method, ensuring consistent handling of the new PermissionValue field type.
86-87: Correctly implemented permission value enum-to-string conversion.This implementation properly handles the reverse conversion from enum code to string name for the PermissionValue field type, maintaining symmetry with the corresponding _str_to_enum method.
xrpl/models/transactions/transaction.py (3)
28-29: Style improvement with trailing comma.The addition of a trailing comma is good practice as it makes future additions cleaner and reduces diff noise.
256-258: Good implementation of delegate account field.The optional delegate field is properly defined with clear documentation on its purpose, enabling the account permission delegation functionality.
270-278: Proper validation to prevent identical account and delegate.The validation logic correctly prevents setting the same address for both account and delegate, which would defeat the purpose of delegation. The error message is clear, and the code properly handles both cases - when there are existing errors and when there aren't.
tests/unit/core/binarycodec/types/test_serialized_dict.py (5)
1-3: Good addition of necessary imports.The imports for
jsonandosare required for loading test fixtures from an external file.
8-12: Excellent shift to data-driven testing.This refactor improves test maintainability by moving test data to an external fixture file, making it easier to add more test cases, especially for new transaction types like DelegateSet.
19-24: Well-implemented data-driven test for serialization.The test now iterates through all test cases in the fixture file, validating that STObject.from_value produces the expected binary output for each case, including the new DelegateSet transaction type.
26-30: Well-implemented data-driven test for serialization and JSON conversion.The test properly validates that serializing and then converting back to JSON matches the original JSON input for all test cases.
32-37: Well-implemented data-driven test for deserialization.The test correctly validates that parsing binary data and converting to JSON produces the expected output for all test cases, ensuring proper round-trip conversions.
tests/unit/models/transactions/test_payment.py (3)
9-10: Good addition of delegate account constant.Adding a constant for the delegate account test value follows the existing pattern in the file and improves maintainability.
194-206: Well-written test for duplicate account and delegate validation.This test properly verifies that an exception is raised when the account and delegate addresses are the same, with the correct error message.
208-216: Good test coverage for valid delegate usage.This test ensures that a Payment transaction with a different delegate address is considered valid, providing complete test coverage for the new feature.
tests/unit/core/binarycodec/fixtures/data/serialized-dict-fixtures.json (1)
1-47: New test fixtures look good!The new JSON fixture file provides appropriate test data for both existing "OfferCreate" and the newly added "DelegateSet" transaction types. The DelegateSet fixture (lines 21-46) properly includes the required fields such as
Account,Authorize, andPermissionsarray with granular permission values, matching the functionality being implemented for account permission delegation.xrpl/models/requests/ledger_entry.py (4)
33-33: LGTM! New LedgerEntryType correctly added.Adding the "delegate" type to the LedgerEntryType enum properly extends the available ledger entry types to support querying delegate objects.
70-91: Implementation follows established patterns.The new Delegate class correctly follows the same pattern as other ledger entry data classes, with proper documentation and required fields (account and authorize) for querying delegate objects.
331-331: LGTM! Field correctly added to LedgerEntry.The new delegate field in the LedgerEntry class is properly typed as Optional[Union[str, Delegate]], consistent with the pattern used for other similar fields.
366-366: Validation properly updated.The _get_errors method is correctly updated to include the new delegate field in the query parameter validation logic, ensuring consistent validation behavior.
tests/unit/models/transactions/test_delegate_set.py (5)
12-26: Test constants look good.The test accounts and permissions list constants are properly defined. The
_MORE_THAN_10_PERMISSIONSlist correctly includes a mix of GranularPermission and DelegatableTransaction values, which is useful for testing the maximum permissions list length constraint.
29-48: Basic validation tests look good.The tests appropriately verify that both transaction-level permissions and granular permissions can be used in a DelegateSet transaction. This ensures all permission types are properly supported.
49-64: Maximum permissions validation test is comprehensive.The test correctly verifies that the model enforces the maximum number of permissions constraint, using the predefined list of more than 10 permissions to trigger the validation error.
66-79: Duplicate permission validation test is well-designed.The test properly verifies that the model detects duplicate permission values in the permissions list, ensuring data integrity.
81-96: Validation of distinct account addresses is properly tested.The test correctly verifies that the account and authorize fields must contain different addresses, which is an important constraint for the delegate relationship.
xrpl/core/binarycodec/definitions/definitions.py (4)
12-25: Granular permissions mapping looks good.The _GRANULAR_PERMISSIONS dictionary correctly maps permission names to unique integer codes starting from 65537, which provides a clear separation from transaction type codes. The permissions cover various authorization and setting operations, aligning with the XLS-74d and XLS-75d amendments.
76-78: Reverse mapping is correctly implemented.The reverse mapping from permission codes to names follows the same pattern used for other similar mappings in the file, maintaining code consistency.
281-295: Well-designed permission code conversion function.The function correctly handles both granular permissions (using the _GRANULAR_PERMISSIONS map) and transaction-type permissions (using transaction type codes with an offset of +1). The approach efficiently reuses existing transaction type codes while ensuring they're distinct from granular permission codes.
297-310: Bidirectional conversion is properly implemented.The conversion from permission codes back to name strings correctly handles both granular permissions and transaction-type permissions, ensuring proper round-trip serialization and deserialization of permission values.
xrpl/core/binarycodec/definitions/definitions.json (2)
673-682:✅ Verification successful
Ensure
nthuniqueness within the UInt32 namespace
PermissionValueis assignednth: 52, which looks sequentially correct afterOracleDocumentID (51).
However, duplicatenthvalues inside the same type‐family silently break the codec at runtime.The script echoes duplicates; expect no output.
Please run it and confirm the new value does not collide.
🏁 Script executed:
#!/usr/bin/env bash # Fail if *any* UInt32 field re-uses an nth already taken. jq -r '.FIELDS[] | select(.[1].type=="UInt32") | .[1].nth' \ xrpl/core/binarycodec/definitions/definitions.json | sort | uniq -dLength of output: 128
No duplicate
nthvalues for UInt32 fields – verification passed.
Ran the provided jq script againstxrpl/core/binarycodec/definitions/definitions.json; it returned no duplicates.
2921-2923:✅ Verification successful
LedgerEntryTypes – verify code 131 is unused
Code 131 looks free, but a quick automated scan prevents surprises across future merges.
Expect exactly one hit (this insertion).
🏁 Script executed:
#!/usr/bin/env bash grep -R --line-number '"Delegate": *131' xrpl/core/binarycodec/definitions/definitions.jsonLength of output: 119
Delegate (code 131) – single definition verified
- xrpl/core/binarycodec/definitions/definitions.json:2922 contains
"Delegate": 131- No other occurrences of code 131 in that mapping
xrpl/models/transactions/delegate_set.py (1)
165-168:⚠️ Potential issueAddress comparison should be case-insensitive
X-addresses/Base58 are case-insensitive; defensive lower-casing avoids false
negatives if callers supply mixed-case values.- if self.authorize == self.account: + if self.authorize.lower() == self.account.lower():Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/unit/models/transactions/test_delegate_set.py (3)
12-26: Consider adding documentation to test constants.The constants are used throughout the tests but their meaning isn't documented. Consider adding comments explaining what these accounts represent in the test context and why the list contains more than 10 permissions.
49-65: Consider adding a boundary test case.The test verifies that exceeding PERMISSION_MAX_LENGTH fails validation, but there's no test for exactly PERMISSION_MAX_LENGTH permissions, which should pass. Consider adding this boundary case.
+ def test_max_length_permissions_list(self): + tx = DelegateSet( + account=_ACCOUNT, + authorize=_DELEGATED_ACCOUNT, + permissions=[ + Permission(permission_value=_MORE_THAN_10_PERMISSIONS[i]) + for i in range(PERMISSION_MAX_LENGTH) + ], + ) + self.assertTrue(tx.is_valid())
1-115: Consider adding a test for empty permissions list.There is no test for an empty permissions list. If this is a valid case, add a test to verify it passes validation. If it should be invalid, add a test to verify it fails with the appropriate error message.
+ def test_empty_permissions_list(self): + # If empty list should be valid: + tx = DelegateSet( + account=_ACCOUNT, + authorize=_DELEGATED_ACCOUNT, + permissions=[], + ) + self.assertTrue(tx.is_valid()) + + # Or if empty list should be invalid: + # with self.assertRaises(XRPLModelException) as error: + # DelegateSet( + # account=_ACCOUNT, + # authorize=_DELEGATED_ACCOUNT, + # permissions=[], + # ) + # self.assertEqual( + # error.exception.args[0], + # "{'permissions': 'Permissions list cannot be empty.'}", + # )xrpl/models/transactions/delegate_set.py (4)
71-84: Permission dataclass is minimalistic and focused.The Permission class correctly implements a nested model with a single required field. The docstring could provide more examples of valid permission values to help developers.
class Permission(NestedModel): """Represents one entry in a Permissions list used in DelegateSet transaction. + + The permission_value can be either a TransactionType.value or a + GranularPermission value. For example: + + - TransactionType.PAYMENT.value (transaction-level permission) + - GranularPermission.PAYMENT_MINT (granular permission) """
121-139: Consider breaking down the permissions validation method.The _get_permissions_error method performs multiple validations. Consider breaking it down into smaller, focused methods for better readability and maintainability.
- 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}." - ) - - entered_permissions = [ - permission.permission_value for permission in self.permissions - ] - if len(entered_permissions) != len(set(entered_permissions)): - return "Duplicate permission value in `permissions` list." - - if set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS: - return ( - f"Non delegatable transactions found in `permissions` list: " - f"{set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS}." - ) - - return None + def _get_permissions_error(self: Self) -> Optional[str]: + """Validate the permissions list and return error if any.""" + # Check for length constraints + length_error = self._validate_permissions_length() + if length_error: + return length_error + + # Extract permission values for further validation + entered_permissions = [ + permission.permission_value for permission in self.permissions + ] + + # Check for duplicates + duplicate_error = self._validate_no_duplicate_permissions(entered_permissions) + if duplicate_error: + return duplicate_error + + # Check for non-delegatable transactions + delegatable_error = self._validate_delegatable_permissions(entered_permissions) + if delegatable_error: + return delegatable_error + + return None + + def _validate_permissions_length(self: Self) -> Optional[str]: + """Validate that permissions list doesn't exceed maximum length.""" + if len(self.permissions) > PERMISSION_MAX_LENGTH: + return f"Length of `permissions` list is greater than {PERMISSION_MAX_LENGTH}." + return None + + def _validate_no_duplicate_permissions(self: Self, permissions: List[str]) -> Optional[str]: + """Validate that there are no duplicate permission values.""" + if len(permissions) != len(set(permissions)): + return "Duplicate permission value in `permissions` list." + return None + + def _validate_delegatable_permissions(self: Self, permissions: List[str]) -> Optional[str]: + """Validate that all permissions are delegatable.""" + non_delegatable = set(permissions) & NON_DELEGATABLE_TRANSACTIONS + if non_delegatable: + return f"Non delegatable transactions found in `permissions` list: {non_delegatable}." + return None
123-126: Use consistent string formatting.The error message formatting in this file uses both f-strings and string concatenation with + operator. Consider using f-strings consistently for better readability.
- return ( - f"Length of `permissions` list is greater than {PERMISSION_MAX_LENGTH}." - ) + return f"Length of `permissions` list is greater than {PERMISSION_MAX_LENGTH}."
134-137: Use consistent string formatting here too.Similar to the previous comment, use f-strings consistently for error messages.
- return ( - f"Non delegatable transactions found in `permissions` list: " - f"{set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS}." - ) + return f"Non delegatable transactions found in `permissions` list: {set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS}."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/integration/transactions/test_delegate_set.py(1 hunks)tests/unit/models/transactions/test_delegate_set.py(1 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/delegate_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- xrpl/models/transactions/init.py
- tests/integration/transactions/test_delegate_set.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
🔇 Additional comments (11)
tests/unit/models/transactions/test_delegate_set.py (6)
1-11: All necessary imports are present.The imports include all required components for testing the DelegateSet transaction model including exceptions, model classes, and permission enums.
28-40: Test for basic DelegateSet creation looks good.This test properly verifies that a DelegateSet transaction with both granular and transaction type permissions can be created and validated.
41-48: Granular permission test is appropriately focused.This test correctly validates that a DelegateSet can be created with just a granular permission.
66-80: Duplicate permission test is thorough.This test correctly verifies that duplicate permissions in the list are properly detected and rejected.
81-97: Account validation test is appropriate.This test correctly verifies that using the same address for both account and authorize fields is rejected.
98-114: Non-delegatable transactions test is thorough.This test properly verifies that including non-delegatable transactions in the permissions list is rejected with the appropriate error message.
xrpl/models/transactions/delegate_set.py (5)
1-16: Imports and module docstring are appropriate.All necessary imports are present and the module docstring clearly indicates the purpose of this file.
17-26: Constants are well-defined with clear purpose.The PERMISSION_MAX_LENGTH and NON_DELEGATABLE_TRANSACTIONS constants are clear and appropriate for the implementation.
28-69: GranularPermission enum is well-documented.The enum provides comprehensive documentation for each granular permission type, which is excellent for developer understanding.
105-114: Error aggregation looks good.The _get_errors method properly aggregates validation errors from parent class and specific validations.
116-120: Account address validation is simple and effective.The _validate_account_addresses method correctly checks that authorize and account fields differ.
| PERMISSION_MAX_LENGTH = 10 | ||
|
|
||
|
|
||
| class DelegatableTransaction(str, Enum): |
There was a problem hiding this comment.
I agree with @mvadari and rather not use this approach. Reason is each time a new transaction is added, then this module will have to be updated to keep it accurate which makes it harder to maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
xrpl/models/transactions/delegate_set.py (4)
19-25: Prefer an immutablefrozensetfor global constants
NON_DELEGATABLE_TRANSACTIONSis intended to be a read-only lookup table.
Using afrozensetprevents accidental run-time mutation and has a (minor) performance benefit for membership tests.-NON_DELEGATABLE_TRANSACTIONS = { +NON_DELEGATABLE_TRANSACTIONS: frozenset[str] = frozenset( TransactionType.ACCOUNT_SET.value, TransactionType.SET_REGULAR_KEY.value, TransactionType.SIGNER_LIST_SET.value, TransactionType.DELEGATE_SET.value, TransactionType.ACCOUNT_DELETE.value, -} +)
109-118: Possible loss of error details due to key shadowing
super()._get_errors()might already contain a"permissions"entry in the future.
By blindly overwriting withNone, valuable diagnostics could be dropped.Safest pattern:
- **super()._get_errors(), - "permissions": self._get_permissions_error(), + **super()._get_errors(), + "permissions": ( + self._get_permissions_error() + or super()._get_errors().get("permissions") + ),This keeps upstream errors if the local check yields no issues.
(If the base never emits a"permissions"key today, the change is still forward-compatible.)
126-138: Duplicate-detection message could identify offending entriesWhen the list is large it’s hard to spot which item is repeated.
Returning the duplicates helps the caller fix the payload quickly.- if len(entered_permissions) != len(set(entered_permissions)): - return "Duplicate permission value in `permissions` list." + duplicates = { + p for p in entered_permissions if entered_permissions.count(p) > 1 + } + if duplicates: + return ( + "Duplicate permission value(s) in `permissions` list: " + f"{sorted(duplicates)}." + )
139-143: Unordered set in error message may lead to flaky assertionsStringifying a
setproduces an arbitrary order, which can make test expectations brittle.
Return a sorted, deterministic representation instead:- return ( - f"Non-delegatable transactions found in `permissions` list: " - f"{set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS}." - ) + offenders = sorted(set(entered_permissions) & NON_DELEGATABLE_TRANSACTIONS) + return ( + "Non-delegatable transactions found in `permissions` list: " + f"{offenders}." + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_delegate_set.py(1 hunks)xrpl/models/transactions/delegate_set.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_delegate_set.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
xrpl/models/transactions/delegate_set.py (4)
xrpl/models/nested_model.py (1)
NestedModel(20-72)xrpl/models/transactions/transaction.py (3)
Transaction(161-519)_get_errors(108-121)_get_errors(259-274)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-63)xrpl/models/utils.py (1)
require_kwargs_on_init(110-163)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (1)
xrpl/models/transactions/delegate_set.py (1)
78-82: Validatepermission_valueearly to avoid malformed transactionsRight now any arbitrary string will pass through, leading to a failed submit after the network round-trip.
Consider a__post_init__or a dedicated validator that:
- Normalises incoming values (
Enum→.value,str→ stripped).- Verifies the value is either:
• a delegatableTransactionType, or
• aGranularPermission.Failing fast improves DX and keeps client / CI feedback tight.
[ suggest_essential_refactor ]
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as off-topic.
This comment was marked as off-topic.
| ledger_index = "validated" | ||
| marker = None | ||
| granted_permission = set() | ||
| while True: |
There was a problem hiding this comment.
Same as in JS - why is this necessary? Much more efficient to search via account_objects.
There was a problem hiding this comment.
@mvadari I wanted to test for ledger_data api call. So I added this. But, I should also test for account_objects as your review comment pointed out missing DELEGATE enum type in AccountObjectType
There was a problem hiding this comment.
I'm not sure this test is worth it - it'll take a while and be pretty inefficient.
There was a problem hiding this comment.
I see, removed ledger_data and added account_objects test.
| "TransactionType": "OfferCreate", | ||
| "TxnSignature": "304502202ABE08D5E78D1E74A4C18F2714F64E87B8BD57444AFA5733109EB3C077077520022100DB335EE97386E4C0591CAC024D50E9230D8F171EEB901B5E5E4BD6D1E0AEF98C" | ||
| }, | ||
| "buffer": "120007220000000024000195F964400000170A53AC2065D5460561EC9DE000000000000000000000000000494C53000000000092D705968936C419CE614BF264B5EEB1CEA47FF468400000000000000A7321028472865AF4CB32AA285834B57576B7290AA8C31B459047DB27E16F418D6A71667447304502202ABE08D5E78D1E74A4C18F2714F64E87B8BD57444AFA5733109EB3C077077520022100DB335EE97386E4C0591CAC024D50E9230D8F171EEB901B5E5E4BD6D1E0AEF98C811439408A69F0895E62149CFCC006FB89FA7D1E6E5D" |
There was a problem hiding this comment.
To match the other fixture files, this field should be called binary.
| }, | ||
| "buffer": "120040210000F7E0228000000024000009186840000000000000C87321ED510865F867CDFCB944D435812ACF23D231E5C14534B146BCE46A2F794D198B777440D05A89D0B489DEC1CECBE0D33BA656C929CDCCC75D4D41B282B378544975B87A70C3E42147D980D1F6E2E4DC6316C99D7E2D4F6335F147C71C0DAA0D6516150D8114DB9157872FA63FAF7432CD300911A43B981B85A28514EBA79C385B47C50D52445DF2676EEC0231F732CEF01DEF203400000001E1EF203400000015E1F1" | ||
| } | ||
| ] |
There was a problem hiding this comment.
nit: newline at the end of the file
| }, | ||
| { | ||
| "description": "DelegateSet Transaction", | ||
| "json": { |
There was a problem hiding this comment.
This should be added to the other fixtures file, not this one - codec-fixtures.json
| _DELEGATABLE_PERMISSIONS_STR_TO_CODE_MAP: Dict[str, int] = { | ||
| **{key: value + 1 for (key, value) in _DEFINITIONS["TRANSACTION_TYPES"].items()}, | ||
| **_GRANULAR_PERMISSIONS, | ||
| } | ||
| _DELEGATABLE_PERMISSIONS_CODE_TO_STR_MAP: Dict[int, str] = { | ||
| **{value + 1: key for (key, value) in _DEFINITIONS["TRANSACTION_TYPES"].items()}, | ||
| **{value: key for (key, value) in _GRANULAR_PERMISSIONS.items()}, | ||
| } |
There was a problem hiding this comment.
nit (I think this is slightly better but it's fine if you don't want to change this):
| _DELEGATABLE_PERMISSIONS_STR_TO_CODE_MAP: Dict[str, int] = { | |
| **{key: value + 1 for (key, value) in _DEFINITIONS["TRANSACTION_TYPES"].items()}, | |
| **_GRANULAR_PERMISSIONS, | |
| } | |
| _DELEGATABLE_PERMISSIONS_CODE_TO_STR_MAP: Dict[int, str] = { | |
| **{value + 1: key for (key, value) in _DEFINITIONS["TRANSACTION_TYPES"].items()}, | |
| **{value: key for (key, value) in _GRANULAR_PERMISSIONS.items()}, | |
| } | |
| _tx_delegations = {key: value + 1 for (key, value) in _DEFINITIONS["TRANSACTION_TYPES"].items()} | |
| _DELEGATABLE_PERMISSIONS_STR_TO_CODE_MAP: Dict[str, int] = { | |
| **_tx_delegations, | |
| **_GRANULAR_PERMISSIONS, | |
| } | |
| _DELEGATABLE_PERMISSIONS_CODE_TO_STR_MAP: Dict[int, str] = { | |
| **{value: key for (key, value) in _DELEGATABLE_PERMISSIONS_STR_TO_CODE_MAP.items()} | |
| } |
…permission-delegation
|
Nice job |
High Level Overview of Change
This PR is continuation of #833 as there are more changes related to binary codec involved.
This PR implements changes pertaining to the XLS-74d and XLS-75d amendments.
Here is the associated cpp PR: XRPLF/rippled#5354.
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added unit and integration tests.