feat: Account Permissions XLS-74d #807
Conversation
specify LedgerEntry RPC for AcctPerm; include integ tests unit tests for AccountPermissionsSet transaction
|
Warning Rate limit exceeded@ckeshava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce support for account permissions in the system. A new configuration option ( Changes
Suggested reviewers
Poem
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:
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
|
|
The C++ implementation allows developers to specify a string value for the I invite your opinion on this design. Do we need to mirror this in the client libraries? At the moment, I have allowed for If the client libraries are expected to accept enum-like string inputs, then it adds maintenance overhead. xrpl-py and xrpl.js need to ensure that: (1) validate that the specified transaction/granular permission is correct (2) maintain a mirror of all transaction-level and granular permissions Regarding (2), it needs to be updated with every new transaction and upgrades to the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
xrpl/models/transactions/account_permission_set.py (1)
19-31: Consider validating boundary forpermission_value.If there's a domain restriction on valid permission values (e.g., they must be non-negative), you might want to explicitly validate that in the
Permissionclass.tests/unit/models/transactions/test_account_permission_set.py (1)
23-39: Good test coverage for oversized permission list.This confirms the model's ability to reject lists exceeding
PERMISSION_MAX_SIZE. Consider adding a boundary test for exactly 10 permissions to ensure no off-by-one errors.tests/integration/transactions/test_account_permission_set.py (3)
18-52: Consider adding sequence number assertions.The test correctly validates that a transaction fails with
tecNO_PERMISSIONwhen attempted without necessary permissions. Consider adding assertions to verify that alice's sequence number remains unchanged after the failed transaction.alice_account_info_response = await client.request( AccountInfo( account=alice.address, ) ) self.assertTrue(alice_account_info_response.is_successful()) +initial_sequence = int(alice_account_info_response.result["account_data"]["Sequence"]) # Use bob's account to execute a transaction on behalf of alice payment = Payment( account=bob.address, amount=xrp_to_drops(1), destination=carol.address, on_behalf_of=alice.address, delegating_seq=int( alice_account_info_response.result["account_data"]["Sequence"] ), ) response = await sign_and_reliable_submission_async( payment, bob, client, check_fee=False ) self.assertEqual(response.status, ResponseStatus.SUCCESS) # The lack of AccountPermissionSet transaction will result in a tecNO_PERMISSION self.assertEqual(response.result["engine_result"], "tecNO_PERMISSION") +# Verify sequence number remains unchanged +alice_account_info_response = await client.request( + AccountInfo( + account=alice.address, + ) +) +self.assertTrue(alice_account_info_response.is_successful()) +self.assertEqual( + int(alice_account_info_response.result["account_data"]["Sequence"]), + initial_sequence, + "Sequence number should remain unchanged after failed transaction", +)
54-100: Improve clarity of permission value calculation.The test correctly validates the account permission workflow. However, the permission value calculation
1 + 0could be more explicit about its purpose.+# Define constants for permission flags +PERMISSION_ENABLED = 1 +PAYMENT_TRANSACTION_TYPE = 0 account_permission_set = AccountPermissionSet( account=alice.address, authorize=bob.address, # Authorize bob account to execute Payment transactions on # behalf of alice's account. # Note: Payment transaction has a TransactionType of 0 - permissions=[Permission(permission_value=(1 + 0))], + permissions=[Permission(permission_value=(PERMISSION_ENABLED + PAYMENT_TRANSACTION_TYPE))], )
102-139: Extract common setup code.The test correctly validates the ledger entry retrieval. However, the setup code for creating and funding wallets and setting permissions is duplicated across tests.
Consider extracting the common setup code into helper methods:
+async def setup_wallets(self, client): + """Create and fund test wallets.""" + alice = Wallet.create() + await fund_wallet_async(alice) + bob = Wallet.create() + await fund_wallet_async(bob) + return alice, bob + +async def setup_account_permission(self, client, alice, bob): + """Set up account permission for testing.""" + PERMISSION_ENABLED = 1 + PAYMENT_TRANSACTION_TYPE = 0 + account_permission_set = AccountPermissionSet( + account=alice.address, + authorize=bob.address, + permissions=[Permission(permission_value=(PERMISSION_ENABLED + PAYMENT_TRANSACTION_TYPE))], + ) + response = await sign_and_reliable_submission_async( + account_permission_set, alice, client, check_fee=False + ) + self.assertEqual(response.status, ResponseStatus.SUCCESS) + self.assertEqual(response.result["engine_result"], "tesSUCCESS") + return response async def test_fetch_account_permission_ledger_entry(self, client): - # Note: Using WALLET, DESTINATION accounts could pollute the test results - alice = Wallet.create() - await fund_wallet_async(alice) - bob = Wallet.create() - await fund_wallet_async(bob) + alice, bob = await self.setup_wallets(client) - account_permission_set = AccountPermissionSet( - account=alice.address, - authorize=bob.address, - permissions=[Permission(permission_value=(1 + 0))], - ) - response = await sign_and_reliable_submission_async( - account_permission_set, alice, client, check_fee=False - ) - self.assertEqual(response.status, ResponseStatus.SUCCESS) - self.assertEqual(response.result["engine_result"], "tesSUCCESS") + await self.setup_account_permission(client, alice, bob)xrpl/models/transactions/transaction.py (1)
259-261: Add documentation for delegation sequence fields.The
delegating_seqanddelegating_ticket_seqfields lack documentation explaining their purpose and usage.Add docstrings for these fields:
delegating_seq: Optional[int] = None + """ + The sequence number of the transaction that granted delegation permissions. + """ delegating_ticket_seq: Optional[int] = None + """ + The ticket sequence number of the transaction that granted delegation permissions. + Used in place of delegating_seq when using tickets. + """
📜 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_account_permission_set.py(1 hunks)tests/unit/models/transactions/test_account_permission_set.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(6 hunks)xrpl/models/requests/ledger_entry.py(4 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/account_permission_set.py(1 hunks)xrpl/models/transactions/transaction.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (15)
xrpl/models/transactions/account_permission_set.py (4)
1-2: Looks Good.The docstring clearly states the purpose of this module.
34-50: Well-structured transaction class.Using
transaction_type=TransactionType.ACCOUNT_PERMISSION_SETensures clarity about the nature of this transaction. The required fields (authorize,permissions) reflect the intended usage.
52-60: Concise error aggregation.The
_get_errorsmethod effectively merges superclass errors with local validation results, promoting reuse and maintainability.
62-74: Robust checks for list length and duplicates.The
_get_permissions_errormethod correctly enforces a maximum size and prevents duplicate permission values. This ensures minimal risk of conflicting or oversized permission sets.tests/unit/models/transactions/test_account_permission_set.py (2)
15-22: Positive test scenario is well covered.Verifying
is_valid()with a straightforward, valid permission set confirms correct default behavior.
40-54: Duplicate detection test is thorough.Validates that duplicate permission values trigger an error as expected. This helps ensure data integrity.
xrpl/models/transactions/types/transaction_type.py (1)
16-16: New transaction type is appropriately defined.Adding
ACCOUNT_PERMISSION_SETto theTransactionTypeenum is consistent with the introduced feature..ci-config/rippled.cfg (1)
199-199: LGTM!The
AccountPermissionfeature is correctly added to the[features]section, aligning with the PR objectives to introduce account permission functionality.xrpl/models/transactions/__init__.py (1)
7-7: LGTM!The
AccountPermissionSetmodel is correctly imported and exported, making it available for use by other modules.Also applies to: 121-121
xrpl/models/requests/ledger_entry.py (3)
28-28: LGTM!The
ACCOUNT_PERMISSIONenum value is correctly added toLedgerEntryType, following the established pattern.
52-59: LGTM!The
AccountPermissiondata class is well-structured with requiredaccountandauthorizefields, following the established pattern of other ledger entry classes.
314-314: LGTM!The
account_permissionfield is correctly added to theLedgerEntryclass and properly integrated into the query parameter validation.Also applies to: 349-349
xrpl/models/transactions/transaction.py (1)
256-258: LGTM! Clear documentation foron_behalf_offield.The field is well-documented with a clear description of its purpose.
CHANGELOG.md (1)
10-11: LGTM! Clear and concise changelog entry.The changelog entry follows the Keep a Changelog format and properly documents the new feature.
xrpl/core/binarycodec/definitions/definitions.json (1)
673-702: LGTM! Binary codec definitions are consistent and well-structured.The new fields, objects, and types are properly defined following the binary codec format and align with the XLS-74d specification.
Also applies to: 1983-1992, 2203-2212, 2602-2610, 2924-2924, 3143-3143
| on_behalf_of: Optional[str] = None | ||
| """The account that the transaction is being sent on behalf of.""" | ||
|
|
||
| delegating_seq: Optional[int] = None | ||
|
|
||
| delegating_ticket_seq: Optional[int] = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for delegation fields.
The new delegation fields should be validated to ensure they are used correctly. For example, delegating_seq and delegating_ticket_seq should not be used simultaneously.
Add validation in the _get_errors method:
def _get_errors(self: Self) -> Dict[str, str]:
errors = super()._get_errors()
+ if self.delegating_seq is not None and self.delegating_ticket_seq is not None:
+ errors["Transaction"] = "Cannot use both delegating_seq and delegating_ticket_seq"
+ if self.on_behalf_of is not None and (
+ self.delegating_seq is None and self.delegating_ticket_seq is None
+ ):
+ errors["Transaction"] = "Must provide delegating_seq or delegating_ticket_seq when using on_behalf_of"
return errors📝 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.
| on_behalf_of: Optional[str] = None | |
| """The account that the transaction is being sent on behalf of.""" | |
| delegating_seq: Optional[int] = None | |
| delegating_ticket_seq: Optional[int] = None | |
| def _get_errors(self: Self) -> Dict[str, str]: | |
| errors = super()._get_errors() | |
| if self.delegating_seq is not None and self.delegating_ticket_seq is not None: | |
| errors["Transaction"] = "Cannot use both delegating_seq and delegating_ticket_seq" | |
| if self.on_behalf_of is not None and ( | |
| self.delegating_seq is None and self.delegating_ticket_seq is None | |
| ): | |
| errors["Transaction"] = "Must provide delegating_seq or delegating_ticket_seq when using on_behalf_of" | |
| return errors |
There was a problem hiding this comment.
Can you also come up with unit tests for this new patch of code?
There was a problem hiding this comment.
🧩 Analysis chain
@ckeshava Here are the unit tests for the new validation logic:
def test_delegation_validation(self):
"""Test validation of delegation fields."""
# Valid cases
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
"on_behalf_of": "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW",
"delegating_seq": 123,
})
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
"on_behalf_of": "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW",
"delegating_ticket_seq": 456,
})
# Invalid: both delegating_seq and delegating_ticket_seq
with self.assertRaises(XRPLModelException) as context:
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
"delegating_seq": 123,
"delegating_ticket_seq": 456,
})
self.assertEqual(
str(context.exception),
"Cannot use both delegating_seq and delegating_ticket_seq",
)
# Invalid: on_behalf_of without delegating_seq or delegating_ticket_seq
with self.assertRaises(XRPLModelException) as context:
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
"on_behalf_of": "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW",
})
self.assertEqual(
str(context.exception),
"Must provide delegating_seq or delegating_ticket_seq when using on_behalf_of",
)
def test_delegation_none_values(self):
"""Test delegation fields with None values."""
# Valid: all fields None
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
})
# Valid: explicit None values
Transaction.from_dict({
"account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"transaction_type": "Payment",
"on_behalf_of": None,
"delegating_seq": None,
"delegating_ticket_seq": None,
})These tests cover:
- Valid combinations of delegation fields
- Invalid combinations that should raise exceptions
- Edge cases with None values
The tests use sample XRP Ledger addresses and sequence numbers. Add these tests to the existing test class in tests/unit/models/test_transaction.py.
🏁 Script executed:
#!/bin/bash
# Find the test file for Transaction class
fd "test_transaction.py" tests/
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Check the content of the unit test file
cat tests/unit/models/transactions/test_transaction.py
Length of output: 8306
@ckeshava Here are the unit tests that match the existing test file's style and patterns:
# Add to the constants at the top of the file
_DELEGATE_ACCOUNT = "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW"
_DELEGATING_SEQ = 12345
_DELEGATING_TICKET_SEQ = 67890
# Add these test methods to the TestTransaction class
def test_valid_delegation_with_seq(self):
"""Test valid transaction with on_behalf_of and delegating_seq."""
tx = Transaction(
account=_ACCOUNT,
fee=_FEE,
sequence=_SEQUENCE,
transaction_type=TransactionType.PAYMENT,
on_behalf_of=_DELEGATE_ACCOUNT,
delegating_seq=_DELEGATING_SEQ,
)
self.assertTrue(tx.is_valid())
def test_valid_delegation_with_ticket_seq(self):
"""Test valid transaction with on_behalf_of and delegating_ticket_seq."""
tx = Transaction(
account=_ACCOUNT,
fee=_FEE,
sequence=_SEQUENCE,
transaction_type=TransactionType.PAYMENT,
on_behalf_of=_DELEGATE_ACCOUNT,
delegating_ticket_seq=_DELEGATING_TICKET_SEQ,
)
self.assertTrue(tx.is_valid())
def test_throws_when_both_delegation_seq_types(self):
"""Test that using both delegating_seq and delegating_ticket_seq raises an error."""
with self.assertRaises(XRPLModelException) as context:
Transaction(
account=_ACCOUNT,
fee=_FEE,
sequence=_SEQUENCE,
transaction_type=TransactionType.PAYMENT,
delegating_seq=_DELEGATING_SEQ,
delegating_ticket_seq=_DELEGATING_TICKET_SEQ,
)
self.assertEqual(
str(context.exception),
"Cannot use both delegating_seq and delegating_ticket_seq",
)
def test_throws_when_on_behalf_of_without_seq(self):
"""Test that using on_behalf_of without a delegation sequence raises an error."""
with self.assertRaises(XRPLModelException) as context:
Transaction(
account=_ACCOUNT,
fee=_FEE,
sequence=_SEQUENCE,
transaction_type=TransactionType.PAYMENT,
on_behalf_of=_DELEGATE_ACCOUNT,
)
self.assertEqual(
str(context.exception),
"Must provide delegating_seq or delegating_ticket_seq when using on_behalf_of",
)
def test_valid_transaction_without_delegation(self):
"""Test that transaction without any delegation fields is valid."""
tx = Transaction(
account=_ACCOUNT,
fee=_FEE,
sequence=_SEQUENCE,
transaction_type=TransactionType.PAYMENT,
)
self.assertTrue(tx.is_valid())These tests:
- Follow the existing file's style and naming conventions
- Use the same pattern for constants
- Test both valid and invalid cases
- Include proper docstrings
- Use consistent error handling patterns
- Cover all the new validation rules
Add these tests to tests/unit/models/transactions/test_transaction.py right after the existing test methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/transactions/test_transaction.py (1)
230-290: Consider adding more edge cases and negative tests.To strengthen the test suite, consider adding:
- Tests for invalid account addresses in
on_behalf_of- Tests for negative or zero values in
delegating_seq- Tests for boundary values in
delegating_ticket_seqAlso, consider extracting error messages into constants to avoid duplication in
test_throws_when_both_delegation_seq_typesandtest_throws_when_on_behalf_of_without_seq.class TestTransaction(TestCase): + # Error message constants + _ERR_BOTH_SEQ_TYPES = ( + "{'Transaction': 'Cannot use both delegating_seq and delegating_ticket_seq'}" + ) + _ERR_MISSING_SEQ = ( + "{'Transaction': 'Must provide delegating_seq or delegating_ticket_seq when" + + " using on_behalf_of'}" + ) + + def test_throws_when_invalid_delegate_account(self): + """Test that using an invalid account address in on_behalf_of raises an error.""" + with self.assertRaises(XRPLModelException): + Transaction( + account=_ACCOUNT, + fee=_FEE, + sequence=_SEQUENCE, + transaction_type=TransactionType.PAYMENT, + on_behalf_of="invalid_address", + delegating_seq=_DELEGATING_SEQ, + ) + + def test_throws_when_negative_delegating_seq(self): + """Test that using a negative delegating_seq raises an error.""" + with self.assertRaises(XRPLModelException): + Transaction( + account=_ACCOUNT, + fee=_FEE, + sequence=_SEQUENCE, + transaction_type=TransactionType.PAYMENT, + on_behalf_of=_DELEGATE_ACCOUNT, + delegating_seq=-1, + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_transaction.py(2 hunks)xrpl/models/transactions/transaction.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xrpl/models/transactions/transaction.py
🔇 Additional comments (2)
tests/unit/models/transactions/test_transaction.py (2)
21-23: LGTM! Well-structured test constants.The new constants for delegation testing follow the established naming convention and provide clear test values.
230-290: LGTM! Comprehensive test coverage for delegation validation.The test suite effectively validates the core delegation scenarios with clear test cases and assertions.
|
Please include new transactions and requests added in the PR description. |
| from xrpl.models.transactions.types import TransactionType | ||
| from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init | ||
|
|
||
| PERMISSION_MAX_SIZE = 10 |
There was a problem hiding this comment.
| PERMISSION_MAX_SIZE = 10 | |
| PERMISSIONS_MAX_LENGTH = 10 |
| permission_value: int = REQUIRED # type: ignore | ||
| """ | ||
| Integer representation of the transaction-level or granular permission. | ||
|
|
||
| :meta hide-value: | ||
| """ |
There was a problem hiding this comment.
We should make this a union of int and enum type PermissionValue (map integer to transaction type) so it's usage is developer friendly.
| permission_value: int = REQUIRED # type: ignore | |
| """ | |
| Integer representation of the transaction-level or granular permission. | |
| :meta hide-value: | |
| """ | |
| permission_value: Union[int, PermissionValue] = REQUIRED # type: ignore | |
| def __post_init__(self): | |
| """Ensure permission_value is always stored as an integer.""" | |
| if isinstance(self.permission_value, PermissionValue): | |
| # Convert Enum to its integer value | |
| object.__setattr__(self, "permission_value", int(self.permission_value)) | |
| elif not isinstance(self.permission_value, int): | |
| raise ValueError(f"Invalid permission_value: {self.permission_value}")``` |
| delegating_seq: Optional[int] = None | ||
|
|
||
| delegating_ticket_seq: Optional[int] = None |
| if self.delegating_seq is not None and self.delegating_ticket_seq is not None: | ||
| errors["Transaction"] = ( | ||
| "Cannot use both delegating_seq and delegating_ticket_seq" | ||
| ) | ||
| if self.on_behalf_of is not None and ( | ||
| self.delegating_seq is None and self.delegating_ticket_seq is None | ||
| ): | ||
| errors["Transaction"] = ( | ||
| "Must provide delegating_seq or delegating_ticket_seq when using" | ||
| + " on_behalf_of" | ||
| ) |
There was a problem hiding this comment.
Can a user just solely provide on_behalf_of and not any delegating fields? If not, then it's missing a case for on_behalf_of must be set if either delegating_seq or delegating_ticket_seq are defined:
self.on_behalf_of is None and (self.delegating_seq is not None or self.delegating_ticket_seq is not None)
| # Note: Payment transaction has a TransactionType of 0 | ||
| permissions=[Permission(permission_value=(1 + 0))], |
There was a problem hiding this comment.
We should allow users to also use enums to make it more developer friendly rather than passing in raw integers.
khancode
changed the title
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
High Level Overview of Change
Specification for this amendment: XRPLF/XRPL-Standards#217, XRPLF/XRPL-Standards#218
Cpp implementation: XRPLF/rippled#5164
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Integration and unit tests have been added