Support AMMClawback amendment #802
Conversation
WalkthroughThe changes introduce support for the new AMMClawback amendment. A new changelog section documents this support. The integration tests now allow enabling a trustline clawback flag when creating an AMM pool, and a new integration test case verifies a positive workflow for AMM clawback operations. In addition, unit tests were added to validate the AMMClawback transaction model. Core and model files have been updated to include the new AMMClawback transaction type, its associated enum member, and validation logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Client as XRPL Client
participant Issuer as Issuer Wallet
participant Holder as Holder Wallet
Test->>Client: create_amm_pool(issuer_account_enable_trustline_clawback=True)
Client->>Issuer: Enable ASF_ALLOW_TRUSTLINE_CLAWBACK flag
Test->>Client: AMMDeposit transaction (holder deposits assets)
Client-->>Test: Confirm deposit success
Test->>Client: AMMClawback transaction (issuer initiates clawback)
Client-->>Test: Confirm clawback success
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![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 (3)
xrpl/models/transactions/amm_clawback.py (1)
90-106: String concatenation for multiple errors could be more maintainable.While returning concatenated error messages works, using a list-based accumulation might improve clarity and consistency for future expansions. For example:
def _validate_wallet_and_amount_fields(self: Self) -> Optional[str]: - errors = "" + errors_list = [] if self.account == self.holder: - errors += "Issuer and holder wallets must be distinct." + errors_list.append("Issuer and holder wallets must be distinct.") # Additional checks... return " ".join(errors_list) if errors_list else NoneThis is only a minor maintainability improvement suggestion.
tests/integration/transactions/test_amm_clawback.py (2)
1-22: Remove unused imports.The following imports are not used in the code:
fund_wallet_asyncWALLETAMMInfoResponseStatusAccountSetAccountSetAsfFlagAMMCreateWalletApply this diff to remove the unused imports:
-from tests.integration.it_utils import ( - create_amm_pool_async, - fund_wallet_async, - sign_and_reliable_submission_async, - test_async_and_sync, -) +from tests.integration.it_utils import ( + create_amm_pool_async, + sign_and_reliable_submission_async, + test_async_and_sync, +) -from tests.integration.reusable_values import WALLET -from xrpl.models.requests.amm_info import AMMInfo -from xrpl.models.response import ResponseStatus -from xrpl.models.transactions import ( - AccountSet, - AccountSetAsfFlag, - AMMCreate, - AMMDeposit, -) +from xrpl.models.transactions import AMMDeposit -from xrpl.wallet import Wallet🧰 Tools
🪛 Ruff (0.8.2)
4-4:
tests.integration.it_utils.fund_wallet_asyncimported but unusedRemove unused import:
tests.integration.it_utils.fund_wallet_async(F401)
8-8:
tests.integration.reusable_values.WALLETimported but unusedRemove unused import:
tests.integration.reusable_values.WALLET(F401)
12-12:
xrpl.models.requests.amm_info.AMMInfoimported but unusedRemove unused import:
xrpl.models.requests.amm_info.AMMInfo(F401)
13-13:
xrpl.models.response.ResponseStatusimported but unusedRemove unused import:
xrpl.models.response.ResponseStatus(F401)
15-15:
xrpl.models.transactions.AccountSetimported but unusedRemove unused import
(F401)
16-16:
xrpl.models.transactions.AccountSetAsfFlagimported but unusedRemove unused import
(F401)
17-17:
xrpl.models.transactions.AMMCreateimported but unusedRemove unused import
(F401)
22-22:
xrpl.wallet.Walletimported but unusedRemove unused import:
xrpl.wallet.Wallet(F401)
🪛 GitHub Actions: Unit test
[warning] 2-2: 'tests.integration.it_utils.fund_wallet_async' imported but unused
[warning] 8-8: 'tests.integration.reusable_values.WALLET' imported but unused
[warning] 12-12: 'xrpl.models.requests.amm_info.AMMInfo' imported but unused
[warning] 13-13: 'xrpl.models.response.ResponseStatus' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AccountSet' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AccountSetAsfFlag' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AMMCreate' imported but unused
[warning] 22-22: 'xrpl.wallet.Wallet' imported but unused
27-73: Enhance test coverage.While the test successfully validates the positive workflow, consider adding:
- Validation of state changes after deposit and clawback operations.
- Negative test cases to verify error handling.
Would you like me to help generate additional test cases to improve coverage?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)tests/integration/it_utils.py(5 hunks)tests/integration/transactions/test_amm_clawback.py(1 hunks)tests/unit/models/transactions/test_amm_clawback.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(2 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/amm_clawback.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration/transactions/test_amm_clawback.py
4-4: tests.integration.it_utils.fund_wallet_async imported but unused
Remove unused import: tests.integration.it_utils.fund_wallet_async
(F401)
8-8: tests.integration.reusable_values.WALLET imported but unused
Remove unused import: tests.integration.reusable_values.WALLET
(F401)
12-12: xrpl.models.requests.amm_info.AMMInfo imported but unused
Remove unused import: xrpl.models.requests.amm_info.AMMInfo
(F401)
13-13: xrpl.models.response.ResponseStatus imported but unused
Remove unused import: xrpl.models.response.ResponseStatus
(F401)
15-15: xrpl.models.transactions.AccountSet imported but unused
Remove unused import
(F401)
16-16: xrpl.models.transactions.AccountSetAsfFlag imported but unused
Remove unused import
(F401)
17-17: xrpl.models.transactions.AMMCreate imported but unused
Remove unused import
(F401)
22-22: xrpl.wallet.Wallet imported but unused
Remove unused import: xrpl.wallet.Wallet
(F401)
🪛 GitHub Actions: Unit test
tests/integration/transactions/test_amm_clawback.py
[warning] 2-2: 'tests.integration.it_utils.fund_wallet_async' imported but unused
[warning] 8-8: 'tests.integration.reusable_values.WALLET' imported but unused
[warning] 12-12: 'xrpl.models.requests.amm_info.AMMInfo' imported but unused
[warning] 13-13: 'xrpl.models.response.ResponseStatus' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AccountSet' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AccountSetAsfFlag' imported but unused
[warning] 14-14: 'xrpl.models.transactions.AMMCreate' imported but unused
[warning] 22-22: 'xrpl.wallet.Wallet' imported but unused
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (17)
xrpl/models/transactions/amm_clawback.py (5)
1-8: Good initial setup and documentation.The overall docstring clearly states the purpose of the transaction model, and the imports (including
FlagInterface, etc.) align well with the XRPL architecture. No issues detected.
21-30: Flag definition is consistent with XRPL standards.The
AMMClawbackFlagenum provides a straightforward way to represent the optional clawing back of both assets. The descriptive docstring helps clarify the behavior for maintainers. No issues found.
32-41: Interface structure is appropriate.
AMMClawbackFlagInterfacenicely inherits fromFlagInterface, ensuring typed usage of the new AMM clawback flags. This follows a familiar pattern in the XRPL codebase, promoting consistency.
43-79: Accurate representation of AMMClawback transaction fields.The
AMMClawbackdataclass:
- Correctly uses
KW_ONLY_DATACLASSwith frozen semantics for immutability.- Enforces required attributes (
holder,asset,asset2) and setstransaction_typetoTransactionType.AMM_CLAWBACKby default.- Provides thorough docstrings for each field, matching the typical XRPL style.
Implementation looks well-structured and consistent with XRPL transaction models.
80-89: Validation error gathering is neatly structured.Collecting both superclass and subclass validation errors in
_get_errorsprovides a clean approach to consolidated validation. This helps ensure that consumers see all relevant errors at once.xrpl/models/transactions/types/transaction_type.py (1)
13-13: New transaction type aligns with AMMClawback support.The addition of
AMM_CLAWBACK = "AMMClawback"integrates seamlessly with the rest of the AMM transaction types. Looks good.tests/unit/models/transactions/test_amm_clawback.py (5)
1-8: Imports and setup look good.All relevant XRPL models and exceptions are imported correctly, and constants are defined in a clear, self-documenting style.
19-31: Direct and thorough test for identical holder and issuer.This test properly verifies that the transaction raises an exception when
account == holder, matching the model's validation rules.
32-45: Enforces correct issuer for the asset.Verifies that the
asset.issuermust matchaccount, ensuring alignment with the XRPL's requirement for issuer-based transactions. Good coverage.
46-64: Test for mismatched asset amount fields is accurate.Checks that
amountmust share the same currency and issuer asasset, guaranteeing consistency in transaction-defined resources.
65-74: Positive test case covers valid usage with flags.Confirms that a correct setup with the
TF_CLAW_TWO_ASSETSflag passes validation. This nicely rounds out the test coverage.tests/integration/transactions/test_amm_clawback.py (1)
25-25: LGTM!The test class follows good practices by inheriting from
IntegrationTestCaseand using the@test_async_and_syncdecorator to test both async and sync implementations.xrpl/models/transactions/__init__.py (1)
14-14: LGTM!The changes correctly import and export the
AMMClawbackclass, following the module's established patterns.Also applies to: 122-122
tests/integration/it_utils.py (2)
316-319: LGTM!The changes to
create_amm_poolcorrectly implement the trustline clawback functionality:
- New parameter with appropriate default value.
- Clear comments explaining the purpose.
- Proper implementation of the flag setting.
- Return value updated to include necessary wallet.
Also applies to: 335-344, 396-397
400-403: LGTM!The changes to
create_amm_pool_asyncmirror the changes in the sync version, maintaining consistency between the implementations.Also applies to: 419-428, 480-481
CHANGELOG.md (1)
10-11: LGTM!The changelog entry follows the Keep a Changelog format and correctly documents the addition of AMMClawback amendment support.
xrpl/core/binarycodec/definitions/definitions.json (1)
3054-3111: New Transaction Type 'AMMClawback' AddedThe new transaction type
"AMMClawback": 31has been correctly inserted into theTRANSACTION_TYPESsection. Please make sure that:
- The numeric value
31does not conflict with other transaction types (note that"Clawback"is defined as30).- All associated models, validations, and tests reference this new type consistently.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Omar Khan <khancodegt@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
xrpl/models/transactions/amm_clawback.py (1)
90-110: Improve error message formatting.Consider these improvements to the validation method:
- Add spaces between concatenated error messages
- Use f-strings instead of string concatenation
def _validate_wallet_and_amount_fields(self: Self) -> Optional[str]: errors = "" if self.account == self.holder: - errors += "Issuer and holder wallets must be distinct." + errors += "Issuer and holder wallets must be distinct. " if self.account != self.asset.issuer: - errors += ( - "Asset.issuer and AMMClawback transaction sender must be identical." - ) + errors += "Asset.issuer and AMMClawback transaction sender must be identical. " if self.amount is not None and ( self.amount.issuer != self.asset.issuer or self.amount.currency != self.asset.currency ): - errors += ( - "Amount.issuer and Amount.currency must match corresponding Asset " - + "fields." - ) + errors += "Amount.issuer and Amount.currency must match corresponding Asset fields." return errors if errors else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/transactions/test_amm_clawback.py(1 hunks)xrpl/models/transactions/amm_clawback.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/transactions/test_amm_clawback.py
🧰 Additional context used
🪛 GitHub Actions: Unit test
xrpl/models/transactions/amm_clawback.py
[warning] 66-66: Black would make changes.
[warning] 66-66: Blank line contains whitespace.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (4)
xrpl/models/transactions/amm_clawback.py (4)
1-19: LGTM!The imports are well-organized and the module docstring is clear and concise.
21-30: LGTM!The enum is well-documented and follows the standard pattern for flag enums.
32-41: Duplicate docstring noted.The docstring is duplicated from
AMMClawbackFlag. While this was discussed in previous reviews and deemed a matter of preference, consider using a reference to theAMMClawbackFlagdocumentation instead.
51-73: Move optional fields after required fields.Following the previous review suggestion, reorder the fields to group required fields together.
🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 66-66: Black would make changes.
[warning] 66-66: Blank line contains whitespace.
| Specifies the other asset in the AMM's pool. In JSON, this is an object with | ||
| currency and issuer fields (omit issuer for XRP). | ||
| """ | ||
|
|
There was a problem hiding this comment.
Fix formatting issues.
Remove the whitespace on this blank line to resolve the pipeline failures.
-
+📝 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.
🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 66-66: Black would make changes.
[warning] 66-66: Blank line contains whitespace.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xrpl/models/transactions/amm_clawback.py (1)
90-109: Improve error message handling.Consider these improvements to the error handling:
- Use a list to collect errors and join them at the end for better readability.
- Simplify the string concatenation in the amount validation error message.
def _validate_wallet_and_amount_fields(self: Self) -> Optional[str]: - errors = "" + errors = [] if self.account == self.holder: - errors += "Issuer and holder wallets must be distinct." + errors.append("Issuer and holder wallets must be distinct.") if self.account != self.asset.issuer: - errors += ( + errors.append( "Asset.issuer and AMMClawback transaction sender must be identical." ) if self.amount is not None and ( self.amount.issuer != self.asset.issuer or self.amount.currency != self.asset.currency ): - errors += ( - "Amount.issuer and Amount.currency must match corresponding Asset " - + "fields." + errors.append( + "Amount.issuer and Amount.currency must match corresponding Asset fields." ) - return errors if errors else None + return " ".join(errors) if errors else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)tests/integration/it_utils.py(5 hunks)tests/integration/transactions/test_amm_clawback.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(1 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/amm_clawback.py(1 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- xrpl/models/transactions/types/transaction_type.py
- CHANGELOG.md
- xrpl/models/transactions/init.py
- tests/integration/transactions/test_amm_clawback.py
- xrpl/core/binarycodec/definitions/definitions.json
- tests/integration/it_utils.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.8)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (4)
xrpl/models/transactions/amm_clawback.py (4)
1-19: LGTM!The imports are well-organized, and all imported types and classes are used appropriately in the code.
21-30: LGTM!The flag is well-defined with a clear docstring explaining its purpose and behavior.
32-41: LGTM!The flag interface is correctly implemented, following the standard pattern used in the codebase.
43-110: LGTM!The AMMClawback transaction class is well-implemented with:
- Clear field definitions and docstrings
- Proper validation of wallet and amount fields
- Correct transaction type configuration
|
Thank you for all the reviews @khancode @achowdhry-ripple @mvadari |
High Level Overview of Change
Documentation for the said amendment: https://xrpl.org/docs/references/protocol/transactions/types/ammclawback
C++ implementation PR: https://github.com/XRPLF/rippled/pull/5142/files
Context of Change
This PR implements the xrpl-py support for the said amendment.
Type of Change
Did you update CHANGELOG.md?