Skip to content

Conversation

@ckeshava
Copy link
Collaborator

High Level Overview of Change

The title of the erstwhile PermissionDelegation amendment has been updated in the latest version of the rippled code. Here is the source.

A corresponding change has been made in the xrpl-py integration tests as well.terNO_DELEGATE_PERMISSION error code is used instead of tecNO_DELEGATE_PERMISSION.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update CHANGELOG.md?

  • Yes
  • No, this change does not impact library users

Test Plan

The existing integ tests have been updated to accomodate the new behavior of rippled.

…Use terNO_DELEGATE_PERMISSION error in the integ tests;
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 10, 2025

Walkthrough

Configuration amendment list updated to remove NonFungibleTokensV1 and rename PermissionDelegation to PermissionDelegationV1_1. Test expectations updated to reflect error code change from tecNO_DELEGATE_PERMISSION to terNO_DELEGATE_PERMISSION for delegation permission validation.

Changes

Cohort / File(s) Summary
Configuration amendments
\.ci-config/rippled.cfg
Removed NonFungibleTokensV1 amendment; renamed PermissionDelegation to PermissionDelegationV1_1
Test error code expectations
tests/integration/transactions/test_delegate_set.py
Updated engine_result expectations from tecNO_DELEGATE_PERMISSION to terNO_DELEGATE_PERMISSION in two test cases validating delegation permission failures

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Configuration changes are straightforward removals and renames
  • Test updates are localized, matching a single error code change across two assertions

Possibly related PRs

Suggested reviewers

  • pdp2121
  • khancode
  • mvadari

Poem

🐰 Amendment gardens trimmed with care,
NonFungible fades to air,
PermissionDelegation takes a new name,
Error codes shift in this gentle refactored game,
Tests now pass with terNO_DELEGATE's claim! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Fix failing Integration Test workflow' is vague and generic, using non-descriptive language that doesn't convey the specific nature of the changes (amendment rename and error code updates). Use a more specific title that reflects the actual changes, such as 'Update integration tests for PermissionDelegation amendment rename and error code change' or 'Update integration tests for rippled amendment and error code updates'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the main changes, context, and type of change, though the 'Context of Change' section is minimal and 'Test Plan' is brief but present.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 020eb78 and a9c55a8.

📒 Files selected for processing (2)
  • .ci-config/rippled.cfg (1 hunks)
  • tests/integration/transactions/test_delegate_set.py (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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: 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.
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
⏰ 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.11)
  • GitHub Check: Integration test (3.10)
  • GitHub Check: Integration test (3.12)
  • GitHub Check: Integration test (3.8)
  • GitHub Check: Integration test (3.13)
  • GitHub Check: semgrep-cloud-platform/scan
  • GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
tests/integration/transactions/test_delegate_set.py (2)

103-103: LGTM!

The error code update is consistent with the change at Line 47 and correctly validates that unauthorized delegation operations fail with the new error code.


46-47: Verification complete—no outstanding issues.

Error code migration from tecNO_DELEGATE_PERMISSION to terNO_DELEGATE_PERMISSION is complete across the test file. No old error code references remain, and the new error code is consistently applied in both the comment and assertions (lines 46, 47, and 103).

.ci-config/rippled.cfg (1)

204-204: Verify amendment name against rippled version target—discrepancy detected.

Web search results indicate rippled 2.5.0 uses the amendment name "PermissionDelegation", while a later fixed revision will use "PermissionDelegationV1_1". The config change uses PermissionDelegationV1_1, which may not align with rippled 2.5.0. Confirm whether the PR targets rippled 2.5.0 or a later version—if targeting 2.5.0, the amendment name should likely be PermissionDelegation instead.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ckeshava
Copy link
Collaborator Author

This PR is a precursor to #882. Please review this PR first.

cc @achowdhry-ripple @pdp2121

@ckeshava
Copy link
Collaborator Author

Thanks for the quick reviews @pdp2121 @achowdhry-ripple

@ckeshava ckeshava merged commit 839127a into XRPLF:main Nov 12, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants