-
Notifications
You must be signed in to change notification settings - Fork 105
Transaction model, unit and integ tests for Delegate XLS-74d amendment #833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ckeshava
wants to merge
5
commits into
XRPLF:main
Choose a base branch
from
ckeshava:acctPerm_v1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d2ee762
Transaction model, unit and integ tests for Delegate XLS-74d amendment
ckeshava 63f7dd3
fix linter errors
ckeshava e42af9f
Update xrpl/models/transactions/transaction.py
ckeshava 4b1491b
address PR comments from Phu and Rabbit
ckeshava 1d4b3d8
fix linter errors
ckeshava File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
from tests.integration.integration_test_case import IntegrationTestCase | ||
from tests.integration.it_utils import ( | ||
fund_wallet_async, | ||
sign_and_reliable_submission_async, | ||
test_async_and_sync, | ||
) | ||
from xrpl.models.requests import LedgerEntry | ||
from xrpl.models.requests.ledger_entry import Delegate | ||
from xrpl.models.response import ResponseStatus | ||
from xrpl.models.transactions import DelegateSet, Payment | ||
from xrpl.models.transactions.delegate_set import Permission | ||
from xrpl.utils import xrp_to_drops | ||
from xrpl.wallet.main import Wallet | ||
|
||
|
||
class TestDelegateSet(IntegrationTestCase): | ||
@test_async_and_sync(globals()) | ||
async def test_delegation_with_no_permission(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) | ||
carol = Wallet.create() | ||
await fund_wallet_async(carol) | ||
|
||
# Use bob's account to execute a transaction on behalf of alice | ||
payment = Payment( | ||
account=alice.address, | ||
amount=xrp_to_drops(1), | ||
destination=carol.address, | ||
delegate=bob.address, | ||
) | ||
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") | ||
|
||
@test_async_and_sync(globals()) | ||
async def test_delegate_set_workflow(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) | ||
carol = Wallet.create() | ||
await fund_wallet_async(carol) | ||
|
||
delegate_set = DelegateSet( | ||
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))], | ||
) | ||
response = await sign_and_reliable_submission_async( | ||
delegate_set, alice, client, check_fee=False | ||
) | ||
self.assertEqual(response.status, ResponseStatus.SUCCESS) | ||
self.assertEqual(response.result["engine_result"], "tesSUCCESS") | ||
|
||
# Use the bob's account to execute a transaction on behalf of alice | ||
payment = Payment( | ||
account=alice.address, | ||
amount=xrp_to_drops(1), | ||
destination=carol.address, | ||
delegate=bob.address, | ||
) | ||
response = await sign_and_reliable_submission_async( | ||
payment, bob, client, check_fee=False | ||
) | ||
self.assertEqual(response.status, ResponseStatus.SUCCESS) | ||
self.assertEqual(response.result["engine_result"], "tesSUCCESS") | ||
|
||
# Validate that the transaction was signed by bob | ||
self.assertEqual(response.result["tx_json"]["Account"], alice.address) | ||
self.assertEqual(response.result["tx_json"]["Delegate"], bob.address) | ||
self.assertEqual(response.result["tx_json"]["SigningPubKey"], bob.public_key) | ||
|
||
@test_async_and_sync(globals()) | ||
async def test_fetch_delegate_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) | ||
|
||
delegate_set = DelegateSet( | ||
account=alice.address, | ||
authorize=bob.address, | ||
# Authorize bob's 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))], | ||
) | ||
response = await sign_and_reliable_submission_async( | ||
delegate_set, alice, client, check_fee=False | ||
) | ||
self.assertEqual(response.status, ResponseStatus.SUCCESS) | ||
self.assertEqual(response.result["engine_result"], "tesSUCCESS") | ||
|
||
ledger_entry_response = await client.request( | ||
LedgerEntry( | ||
delegate=Delegate( | ||
account=alice.address, | ||
authorize=bob.address, | ||
), | ||
) | ||
) | ||
self.assertTrue(ledger_entry_response.is_successful()) | ||
self.assertEqual( | ||
ledger_entry_response.result["node"]["LedgerEntryType"], | ||
"Delegate", | ||
) | ||
self.assertEqual(ledger_entry_response.result["node"]["Account"], alice.address) | ||
self.assertEqual(ledger_entry_response.result["node"]["Authorize"], bob.address) | ||
self.assertEqual(len(ledger_entry_response.result["node"]["Permissions"]), 1) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
from unittest import TestCase | ||
|
||
from xrpl.models.exceptions import XRPLModelException | ||
from xrpl.models.transactions import DelegateSet | ||
from xrpl.models.transactions.delegate_set import ( | ||
GRANULAR_PERMISSIONS, | ||
PERMISSION_MAX_LENGTH, | ||
Permission, | ||
) | ||
|
||
_ACCOUNT = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ" | ||
_DELEGATED_ACCOUNT = "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW" | ||
|
||
|
||
class TestAccountPermissionSet(TestCase): | ||
def test_delegate_set(self): | ||
tx = DelegateSet( | ||
account=_ACCOUNT, | ||
authorize=_DELEGATED_ACCOUNT, | ||
permissions=[Permission(permission_value=1)], | ||
) | ||
self.assertTrue(tx.is_valid()) | ||
|
||
def test_delegate_set_granular_permission(self): | ||
tx = DelegateSet( | ||
account=_ACCOUNT, | ||
authorize=_DELEGATED_ACCOUNT, | ||
permissions=[ | ||
Permission(permission_value=GRANULAR_PERMISSIONS["PaymentMint"]) | ||
], | ||
) | ||
self.assertTrue(tx.is_valid()) | ||
|
||
def test_long_permissions_list(self): | ||
with self.assertRaises(XRPLModelException) as error: | ||
DelegateSet( | ||
account=_ACCOUNT, | ||
authorize=_DELEGATED_ACCOUNT, | ||
permissions=[ | ||
Permission(permission_value=i) | ||
for i in range(PERMISSION_MAX_LENGTH + 1) | ||
], | ||
) | ||
self.assertEqual( | ||
error.exception.args[0], | ||
"{'permissions': 'Length of `permissions` list is greater than " | ||
+ str(PERMISSION_MAX_LENGTH) | ||
+ ".'}", | ||
) | ||
|
||
def test_duplicate_permission_value(self): | ||
with self.assertRaises(XRPLModelException) as error: | ||
DelegateSet( | ||
account=_ACCOUNT, | ||
authorize=_DELEGATED_ACCOUNT, | ||
permissions=[ | ||
Permission(permission_value=1), | ||
Permission(permission_value=1), | ||
], | ||
) | ||
self.assertEqual( | ||
error.exception.args[0], | ||
"{'permissions': 'Duplicate permission value in `permissions` list.'}", | ||
) | ||
|
||
def test_account_and_delegate_are_the_same(self): | ||
with self.assertRaises(XRPLModelException) as error: | ||
DelegateSet( | ||
account=_ACCOUNT, | ||
authorize=_ACCOUNT, | ||
permissions=[ | ||
Permission(permission_value=1), | ||
], | ||
) | ||
self.assertEqual( | ||
error.exception.args[0], | ||
"{'account_addresses': 'Field `authorize` and `account` must be different.'}", | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,29 @@ class Credential(BaseModel): | |
"""The type of the credential, as issued.""" | ||
|
||
|
||
@require_kwargs_on_init | ||
@dataclass(frozen=True, **KW_ONLY_DATACLASS) | ||
class Delegate(BaseModel): | ||
""" | ||
Required fields for requesting a Delegate ledger object if not querying by | ||
object ID. | ||
""" | ||
|
||
account: str = REQUIRED # type: ignore | ||
""" | ||
This field is required. | ||
|
||
:meta hide-value: | ||
""" | ||
|
||
authorize: str = REQUIRED # type: ignore | ||
""" | ||
This field is required. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add description for this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added description in 4b1491b |
||
|
||
:meta hide-value: | ||
""" | ||
|
||
|
||
@require_kwargs_on_init | ||
@dataclass(frozen=True, **KW_ONLY_DATACLASS) | ||
class DepositPreauth(BaseModel): | ||
|
@@ -304,6 +327,7 @@ class LedgerEntry(Request, LookupByLedgerRequest): | |
account_root: Optional[str] = None | ||
check: Optional[str] = None | ||
credential: Optional[Union[str, Credential]] = None | ||
delegate: Optional[Union[str, Delegate]] = None | ||
deposit_preauth: Optional[Union[str, DepositPreauth]] = None | ||
did: Optional[str] = None | ||
directory: Optional[Union[str, Directory]] = None | ||
|
@@ -338,6 +362,7 @@ def _get_errors(self: Self) -> Dict[str, str]: | |
self.account_root, | ||
self.check, | ||
self.credential, | ||
self.delegate, | ||
self.deposit_preauth, | ||
self.did, | ||
self.directory, | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix line length to comply with flake8
The line exceeds the maximum length limit (90 > 88 characters).
Consider either:
📝 Committable suggestion
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 77-77: flake8: line too long (90 > 88 characters) (E501)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in 1d4b3d8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ckeshava Thank you for confirming that you've addressed the line length issue in commit 1d4b3d8. Great work on resolving this flake8 warning!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!