Fix failed integration tests due to removal of ammendaments #882
Conversation
WalkthroughThis change disables five amendments (NonFungibleTokensV1, NonFungibleTokensV1_1, fixNFTokenRemint, fixNonFungibleTokensV1_2, and PermissionDelegation) in the CI configuration. New helper functions are added to check amendment status at runtime, and three delegation tests now include pre-flight checks that skip execution if the PermissionDelegation amendment is disabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/it_utils.py (1)
666-667: Consider narrowing exception handling.Catching all exceptions might mask legitimate errors like network issues or malformed responses. While the current approach provides robustness for tests, consider catching specific exceptions (e.g.,
XRPLException,RequestException) or at minimum logging unexpected errors for debugging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.ci-config/rippled.cfg(0 hunks)tests/integration/it_utils.py(2 hunks)tests/integration/transactions/test_delegate_set.py(4 hunks)
💤 Files with no reviewable changes (1)
- .ci-config/rippled.cfg
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
📚 Learning: 2024-10-30T20:45:02.301Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Applied to files:
tests/integration/transactions/test_delegate_set.py
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Applied to files:
tests/integration/transactions/test_delegate_set.py
📚 Learning: 2024-11-01T16:20:50.192Z
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Applied to files:
tests/integration/transactions/test_delegate_set.py
📚 Learning: 2024-10-17T17:45:46.828Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Applied to files:
tests/integration/it_utils.py
📚 Learning: 2025-06-04T22:17:47.164Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Applied to files:
tests/integration/it_utils.py
📚 Learning: 2025-09-18T18:20:02.896Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 866
File: xrpl/models/transactions/loan_broker_set.py:105-110
Timestamp: 2025-09-18T18:20:02.896Z
Learning: The xrpl-py codebase contains many string-typed numeric fields in transaction models (like loan_origination_fee, maximum_amount, signature_reward, debt_maximum, etc.) and consistently uses direct int() calls for validation without try-catch blocks, allowing ValueError exceptions to bubble up naturally for clear error communication.
Applied to files:
tests/integration/it_utils.py
🧬 Code graph analysis (2)
tests/integration/transactions/test_delegate_set.py (1)
tests/integration/it_utils.py (1)
is_amendment_enabled_async(641-667)
tests/integration/it_utils.py (4)
xrpl/models/requests/feature.py (1)
Feature(14-22)xrpl/asyncio/clients/async_client.py (2)
AsyncClient(12-29)request(19-29)xrpl/clients/sync_client.py (2)
request(21-31)SyncClient(14-31)xrpl/models/response.py (1)
is_successful(74-82)
🪛 Ruff (0.14.3)
tests/integration/it_utils.py
659-659: Loop control variable feature_id not used within loop body
Rename unused feature_id to _feature_id
(B007)
665-665: Consider moving this statement to an else block
(TRY300)
666-666: Do not catch blind exception: Exception
(BLE001)
688-688: Loop control variable feature_id not used within loop body
Rename unused feature_id to _feature_id
(B007)
694-694: Consider moving this statement to an else block
(TRY300)
695-695: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
tests/integration/transactions/test_delegate_set.py (1)
4-4: LGTM! Amendment checks properly guard DelegateSet tests.The pre-flight checks correctly skip tests when PermissionDelegation is unavailable, allowing the test suite to run against servers without this amendment. The pattern is consistent across all three test methods.
Also applies to: 26-32, 60-66, 142-148
| async def is_amendment_enabled_async( | ||
| client: AsyncClient, | ||
| amendment_name: str, | ||
| ) -> bool: | ||
| """ | ||
| Check if a specific amendment is enabled on the XRPL server. | ||
| Args: | ||
| client: The async client to use for the request. | ||
| amendment_name: The name of the amendment to check. | ||
| Returns: | ||
| True if the amendment is enabled, False otherwise. | ||
| """ | ||
| try: | ||
| response = await client.request(Feature()) | ||
| if response.is_successful() and "features" in response.result: | ||
| features = response.result["features"] | ||
| for feature_id, feature_data in features.items(): | ||
| if ( | ||
| isinstance(feature_data, dict) | ||
| and feature_data.get("name") == amendment_name | ||
| ): | ||
| return feature_data.get("enabled", False) | ||
| return False | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate unused loop variable.
The feature_id loop variable is never used. Consider iterating over .values() directly for cleaner code.
Apply this diff:
- for feature_id, feature_data in features.items():
+ for feature_data in features.values():
if (
isinstance(feature_data, dict)
and feature_data.get("name") == amendment_name📝 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.
| async def is_amendment_enabled_async( | |
| client: AsyncClient, | |
| amendment_name: str, | |
| ) -> bool: | |
| """ | |
| Check if a specific amendment is enabled on the XRPL server. | |
| Args: | |
| client: The async client to use for the request. | |
| amendment_name: The name of the amendment to check. | |
| Returns: | |
| True if the amendment is enabled, False otherwise. | |
| """ | |
| try: | |
| response = await client.request(Feature()) | |
| if response.is_successful() and "features" in response.result: | |
| features = response.result["features"] | |
| for feature_id, feature_data in features.items(): | |
| if ( | |
| isinstance(feature_data, dict) | |
| and feature_data.get("name") == amendment_name | |
| ): | |
| return feature_data.get("enabled", False) | |
| return False | |
| except Exception: | |
| return False | |
| async def is_amendment_enabled_async( | |
| client: AsyncClient, | |
| amendment_name: str, | |
| ) -> bool: | |
| """ | |
| Check if a specific amendment is enabled on the XRPL server. | |
| Args: | |
| client: The async client to use for the request. | |
| amendment_name: The name of the amendment to check. | |
| Returns: | |
| True if the amendment is enabled, False otherwise. | |
| """ | |
| try: | |
| response = await client.request(Feature()) | |
| if response.is_successful() and "features" in response.result: | |
| features = response.result["features"] | |
| for feature_data in features.values(): | |
| if ( | |
| isinstance(feature_data, dict) | |
| and feature_data.get("name") == amendment_name | |
| ): | |
| return feature_data.get("enabled", False) | |
| return False | |
| except Exception: | |
| return False |
🧰 Tools
🪛 Ruff (0.14.3)
659-659: Loop control variable feature_id not used within loop body
Rename unused feature_id to _feature_id
(B007)
665-665: Consider moving this statement to an else block
(TRY300)
666-666: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In tests/integration/it_utils.py around lines 641 to 667, the for-loop iterates
over features.items() but never uses the feature_id variable; change the loop to
iterate over features.values() (e.g., for feature_data in features.values():)
and remove the unused feature_id binding so the body uses feature_data as before
and returns feature_data.get("enabled", False).
| def is_amendment_enabled( | ||
| client: SyncClient, | ||
| amendment_name: str, | ||
| ) -> bool: | ||
| """ | ||
| Check if a specific amendment is enabled on the XRPL server. | ||
| Args: | ||
| client: The sync client to use for the request. | ||
| amendment_name: The name of the amendment to check. | ||
| Returns: | ||
| True if the amendment is enabled, False otherwise. | ||
| """ | ||
| try: | ||
| response = client.request(Feature()) | ||
| if response.is_successful() and "features" in response.result: | ||
| features = response.result["features"] | ||
| for feature_id, feature_data in features.items(): | ||
| if ( | ||
| isinstance(feature_data, dict) | ||
| and feature_data.get("name") == amendment_name | ||
| ): | ||
| return feature_data.get("enabled", False) | ||
| return False | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply the same refactoring to the sync version.
The sync function has the same unused loop variable issue as the async version.
Apply this diff:
- for feature_id, feature_data in features.items():
+ for feature_data in features.values():
if (
isinstance(feature_data, dict)
and feature_data.get("name") == amendment_name📝 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.
| def is_amendment_enabled( | |
| client: SyncClient, | |
| amendment_name: str, | |
| ) -> bool: | |
| """ | |
| Check if a specific amendment is enabled on the XRPL server. | |
| Args: | |
| client: The sync client to use for the request. | |
| amendment_name: The name of the amendment to check. | |
| Returns: | |
| True if the amendment is enabled, False otherwise. | |
| """ | |
| try: | |
| response = client.request(Feature()) | |
| if response.is_successful() and "features" in response.result: | |
| features = response.result["features"] | |
| for feature_id, feature_data in features.items(): | |
| if ( | |
| isinstance(feature_data, dict) | |
| and feature_data.get("name") == amendment_name | |
| ): | |
| return feature_data.get("enabled", False) | |
| return False | |
| except Exception: | |
| return False | |
| def is_amendment_enabled( | |
| client: SyncClient, | |
| amendment_name: str, | |
| ) -> bool: | |
| """ | |
| Check if a specific amendment is enabled on the XRPL server. | |
| Args: | |
| client: The sync client to use for the request. | |
| amendment_name: The name of the amendment to check. | |
| Returns: | |
| True if the amendment is enabled, False otherwise. | |
| """ | |
| try: | |
| response = client.request(Feature()) | |
| if response.is_successful() and "features" in response.result: | |
| features = response.result["features"] | |
| for feature_data in features.values(): | |
| if ( | |
| isinstance(feature_data, dict) | |
| and feature_data.get("name") == amendment_name | |
| ): | |
| return feature_data.get("enabled", False) | |
| return False | |
| except Exception: | |
| return False |
🧰 Tools
🪛 Ruff (0.14.3)
688-688: Loop control variable feature_id not used within loop body
Rename unused feature_id to _feature_id
(B007)
694-694: Consider moving this statement to an else block
(TRY300)
695-695: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In tests/integration/it_utils.py around lines 670 to 696, the sync
is_amendment_enabled function uses an unused loop variable (feature_id) in "for
feature_id, feature_data in features.items()"; change the loop to iterate over
values only (for feature_data in features.values()) so the unused variable is
removed while preserving behavior; keep the rest of the logic intact and return
False on exceptions as before.
|
Hello,
I could not find references to this amendment in the rippled codebase. However, rippled does not post any complaints about this amendment either.
The above three amendments have been retired from the rippled codebase. However, rippled executable does not complain about their presence in the configuration file. Hence, I decided to retain them.
The The latest state of all the amendments can be found in this rippled source code file Based on the observations above, I feel the changes in this PR are not warranted at the present time. Perhaps, we can reconsider this PR after 2-3 months after the rippled team has completed retiring all the old amendments. I expect more of these problems in the coming weeks. |
High Level Overview of Change
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan