Skip to content

feat: add PermissionedDomains support (XLS-80)#157

Draft
e-desouza wants to merge 25 commits into
mainfrom
feat/xls-80-permissioned-domains
Draft

feat: add PermissionedDomains support (XLS-80)#157
e-desouza wants to merge 25 commits into
mainfrom
feat/xls-80-permissioned-domains

Conversation

@e-desouza

@e-desouza e-desouza commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements XLS-80 (PermissionedDomains) support for xrpl-rust.

New transaction types

  • PermissionedDomainSet — Create or update a permissioned domain with a list of AcceptedCredentials
  • PermissionedDomainDelete — Delete an existing permissioned domain by DomainID

New ledger entry type

  • PermissionedDomain (LedgerEntryType = 0x0082) — On-ledger permissioned domain object with Owner, AcceptedCredentials, Sequence, and audit fields

Shared types

  • Credential — Nested STObject (via serde_with_tag!) with required Issuer and CredentialType fields

Validation

  • Credential.issuer and Credential.credential_type are required (non-optional String fields)
  • PermissionedDomainSet.get_errors() rejects empty issuer or credential_type values

Registration

  • TransactionType::PermissionedDomainSet, TransactionType::PermissionedDomainDelete added to enum
  • LedgerEntryType::PermissionedDomain (0x0082) added to enum
  • Module re-exports and integration tests included

Test plan

  • Serde roundtrip tests for PermissionedDomainSet, PermissionedDomainDelete, PermissionedDomain
  • Builder pattern and new() constructor tests
  • get_transaction_type() variant tests
  • Validation: empty credential fields rejected
  • Multiple credentials in AcceptedCredentials array
  • Integration tests in tests/transactions/
  • cargo fmt, cargo clippy --all-features, cargo test --release all pass
  • All 9 feature-matrix builds compile cleanly

…r XLS-80

Add the Credential serde_with_tag type for wrapping issuer/credential_type
pairs, and add PermissionedDomainSet and PermissionedDomainDelete variants
to the TransactionType enum. Module declarations for the new transaction
types are included.
Implement the PermissionedDomainSet transaction for creating and updating
permissioned domains. Supports optional DomainID for updates, a list of
accepted Credential entries, and the full CommonTransactionBuilder pattern
with builder methods for domain-specific fields.
Implement the PermissionedDomainDelete transaction for removing an
existing permissioned domain from the ledger. Takes a required DomainID
field identifying the domain to delete.
Add the PermissionedDomain ledger object representing a permissioned
domain on the XRP Ledger, with owner, accepted_credentials, sequence,
owner_node, previous_txn_id, and previous_txn_lgr_seq fields. Register
as LedgerEntryType 0x0082 and add the LedgerEntry enum variant.
Add integration test stubs for PermissionedDomainSet and
PermissionedDomainDelete transactions. These tests verify transaction
construction, signing, and submission against a running XRPL node,
accepting temDISABLED when the amendment is not yet enabled.

@e-desouza e-desouza left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a read through

Required Credential fields typed as Option without validation — flagged inline.


Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

The XLS-80 spec requires both `issuer` and `credential_type` on every
Credential entry.  Having them typed as `Option<String>` allowed callers
to construct silently invalid transactions.

- Change `Credential.issuer` and `Credential.credential_type` from
  `Option<String>` to `String`, matching the Signer struct pattern.
- Add `get_errors()` validation in `PermissionedDomainSet` that rejects
  empty issuer or credential_type with `MissingField`.
- Add two tests covering the new validation paths.
- Update all call sites (unit tests, integration test, ledger object
  tests) to use non-optional field construction.
The CredentialType field is a Blob type in the XRPL binary codec
definitions, which requires hex-encoded input. The test was passing
the raw ASCII string "KYC" instead of its hex representation "4B5943",
causing a TryFromStrError during binary serialization.
The rippleci/rippled:develop image updated after 2026-04-01 and broke
integration tests across all PRs (container exits before becoming healthy,
causing Connection refused on localhost:5005).

Pin to the last known-good digest and replace the simple until loop with
a bounded retry that checks container liveness, prints status per attempt,
and dumps container logs on failure.
- Enforce XLS-80 AcceptedCredentials bounds (1..=10) in
  PermissionedDomainSet::get_errors.
- Add validate_credential helper that rejects CredentialType values
  that are not valid hex Blob: non-empty, even length, ASCII hex only,
  and at most 64 hex chars (rippled MaxCredentialTypeLength = 32 bytes).
- Validate DomainID in PermissionedDomainDelete as exactly 64 hex chars.
- Tighten integration-test engine_result assertions to a strict
  allowlist rather than substring matches that could swallow unrelated
  tec codes.
- Extend PermissionedDomain ledger-object serialize test to assert
  PascalCase JSON keys so silent field-name drift is caught.
- Update all affected unit tests to use valid hex CredentialType and
  64-char DomainID values.

Deferred: optional Credential field rework (required-field degrades
deserialization) is lower priority and risks breaking callers.
pdp2121 pushed a commit that referenced this pull request Apr 21, 2026
… #3270) (#291)

## Summary

The `rippled` binary was renamed to `xrpld` upstream, and the
`rippleci/rippled` image stopped receiving updates. Our integration
tests across every open PR started failing because the published
`develop` image exited before becoming healthy (`Connection refused` on
`localhost:5005`, **0 passed / 41 failed**).

This PR mirrors the upstream fix in xrpl.js:
[XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270).
Switching to `rippleci/xrpld:develop` is the **actual root-cause fix**
rather than pinning an old digest of the deprecated image.

## Changes

`.github/workflows/integration_test.yml`:
- `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE:
rippleci/xrpld:develop`.
- `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image
handles `mkdir` + launch internally; no more `bash -c "mkdir -p
/var/lib/rippled/db/ && rippled -a"` wrapper).
- Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`.
- Container name: `rippled-service` -> `xrpld-service`.
- Removed the docker `--health-cmd` (which shelled out to the renamed
`rippled` CLI and always failed) in favour of a direct JSON-RPC poll
against `http://localhost:5005/`.
- Always dump container logs on the stop step for post-mortem
visibility.

`.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`:
- `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`.
- `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`.
- `[debug_logfile] /var/log/rippled/debug.log` ->
`/var/log/xrpld/debug.log`.

## Verification

Validated on throwaway PR #292 (now closed): **Integration Test green in
2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check
also pass.

## Related follow-up

The 7 in-flight PRs (#130, #131, #151, #153, #156, #157, #158) currently
carry a stopgap commit pinning `rippleci/rippled:develop` to a specific
digest. After this PR merges to `main`, those branches should:
1. Rebase on `main` to pick up the xrpld switch, or
2. Cherry-pick this commit and drop the stopgap digest pin.

## Test plan

- [x] Validated end-to-end on PR #292
- [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass
- [ ] Merge and confirm subsequent PRs inherit the fix without manual
cherry-pick

## Credit

Approach lifted from @ckeshava's
[xrpl.js#3270](XRPLF/xrpl.js#3270).
Comment thread .github/workflows/integration_test.yml Outdated
Comment on lines +46 to +55
if ! docker ps -q -f name=rippled-service | grep -q .; then
echo "Container exited unexpectedly"
docker logs rippled-service 2>&1 || true
exit 1
fi
STATUS=$(docker inspect --format='{{.State.Health.Status}}' rippled-service 2>/dev/null || echo "unknown")
echo "Attempt $i/30: $STATUS"
if [ "$STATUS" = "healthy" ]; then
exit 0
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is insufficient. Often, the xrpld executable throws a runtime exception due to a misconfigured parameter and the xrpld process is left hanging / non-terminated state.

However, this error is not reflected in the health of the Docker container. From the Docker daemon's point-of-view, the Docker container is healthy and has not experienced any problems (OOMKilled, Terminated, etc)

hence, this check is not informative.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5564a56. Removed the custom Docker health-check logic and restored the current xrpld integration workflow from main.

Comment on lines +173 to +190
fn test_empty_credentials() {
let domain = PermissionedDomain::new(
None,
None,
Cow::from("rOwner"),
vec![],
10,
None,
Cow::from("AABB00112233445566778899AABB00112233445566778899AABB001122334455AA"),
200,
);

assert!(domain.accepted_credentials.is_empty());

let serialized = serde_json::to_string(&domain).unwrap();
let deserialized: PermissionedDomain = serde_json::from_str(&serialized).unwrap();
assert_eq!(domain, deserialized);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a PermissionedDomain ledger object has empty Credentials list, that is the violation of the invariant. This serde test is not useful, it is conveying an anti-pattern to readers. This type of a PD object with empty credentials list should not be possible on the XRPL.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4ab5ad6 and 712b0ec. Removed the invalid empty-credentials ledger fixture and kept remaining PermissionedDomain fixtures valid per XLS-80 invariants.

Comment on lines +129 to +151
fn test_serialize_without_owner_node() {
let domain = PermissionedDomain::new(
Some(Cow::from("IndexHash")),
None,
Cow::from("rOwnerAccount123"),
vec![Credential {
issuer: "rIssuer".to_string(),
credential_type: "4B5943".to_string(), // hex("KYC")
}],
5,
None,
Cow::from("DEADBEEF01234567DEADBEEF01234567DEADBEEF01234567DEADBEEF01234567"),
500,
);

let serialized = serde_json::to_string(&domain).unwrap();

// OwnerNode should not appear when None
assert!(!serialized.contains("OwnerNode"));

let deserialized: PermissionedDomain = serde_json::from_str(&serialized).unwrap();
assert_eq!(domain, deserialized);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you have a PermissionedDomain object without a corresponding OwnerNode? Each ledger-object on the XRPL must have a corresponding Owner. I prefer this test to be removed because it conveys an incorrect semantic meaning to the reader.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba3b83a. OwnerNode is now required on PermissionedDomain, matching XLS-80, xrpl.js, and rippled SoeRequired; the invalid “without OwnerNode” test was removed.

pub previous_txn_lgr_seq: u32,
}

impl<'a> Model for PermissionedDomain<'a> {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the validations provided by this Model used anywhere?

This Model trait is not useful for a Ledger-Object. All Ledger-Objects are constructed by the rippled node. We can assume that such objects are always correct. A user is not expected to "construct" these ledger-objects, hence this validation is not useful.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5a08859. Removed the unused Model implementation from the PermissionedDomain ledger object.

Comment on lines +350 to +374
fn test_multiple_memos() {
let txn = PermissionedDomainDelete {
common_fields: CommonFields {
account: "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh".into(),
transaction_type: TransactionType::PermissionedDomainDelete,
..Default::default()
},
domain_id: "AABB00112233445566778899AABB00112233445566778899AABB00112233445A".into(),
}
.with_memo(Memo {
memo_data: Some("reason 1".into()),
memo_format: None,
memo_type: Some("text".into()),
})
.with_memo(Memo {
memo_data: Some("reason 2".into()),
memo_format: None,
memo_type: Some("text".into()),
})
.with_fee("10".into())
.with_sequence(1);

assert_eq!(txn.common_fields.memos.as_ref().unwrap().len(), 2);
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is more appropriate in a Transaction unit-test section, instead of PDDelete.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6f9149f. Removed the common memo-builder coverage from the PDDelete unit-test area.

Comment on lines +19 to +31
// Use a placeholder domain ID -- on a real network this would be an existing domain.
let mut tx = PermissionedDomainDelete::new(
wallet.classic_address.clone().into(),
None,
None,
None,
None,
None,
None,
None,
None,
"A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2".into(),
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that a real PD-Ledger-Object is deleted, instead of a placeholder. This test does not tell us "why" the PDDelete transaction might fail. There could be an error in the transaction construction by xrpl-rust and we would never see such errors.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7fc4460. The integration test now creates a real PermissionedDomain, extracts its ledger object ID from account_objects, and deletes that created domain instead of using a placeholder.

Comment on lines +46 to +48
"temDISABLED",
"tecNO_PERMISSION",
"tecDUPLICATE",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the construction of a new PD object should always be successful. We should not relax these conditions because errors like tecNO_PERMISSION could also occur due to potential mistakes in the client library

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 16c6952. The PDSet integration test now only accepts tesSUCCESS when the amendment is enabled, with temDISABLED retained only for amendment-disabled environments.

@ckeshava ckeshava left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-checked the diff against rippled develop. Five findings — three correctness bugs (#1 most impactful: valid 64-byte credential types would be rejected), one required-as-Option, and two missing protocol-level validations. Inline comments below.

"Credential.CredentialType".into(),
));
}
if ct.len() > 64 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CredentialType length limit is half what rippled enforces. The SDK rejects when ct.len() > 64 (hex chars), treating the limit as 32 bytes. rippled's kMaxCredentialTypeLength = 64 is bytes, so the hex-encoded form is up to 128 chars. Valid 64-byte credential types are rejected today.

Source: include/xrpl/protocol/Protocol.hconstexpr std::size_t kMaxCredentialTypeLength = 64; — applied as ct.size() > kMaxCredentialTypeLength in CredentialHelpers.cpp::checkArray, where ct.size() is the blob byte length.

XLS-80 §2.1.2 also says "The maximum length is 64 bytes (as per XLS-70)."

Fix: change the limit to 128 hex chars; also fix the doc comment on lines 78–79 and the expected: string on line 100 (currently says 32 bytes / 64 chars).

Tests demonstrating the bug (replace test_credential_type_too_long_rejected):

#[test]
fn test_credential_type_64_bytes_accepted() {
    // 64 bytes hex-encoded = 128 chars; this is the rippled maximum.
    let cred = Credential { issuer: "rIssuer".to_string(), credential_type: "A".repeat(128) };
    assert!(validate_credential(&cred).is_ok());
}

#[test]
fn test_credential_type_over_64_bytes_rejected() {
    let cred = Credential { issuer: "rIssuer".to_string(), credential_type: "A".repeat(130) };
    assert!(matches!(validate_credential(&cred), Err(XRPLModelException::ValueTooLong { max: 128, .. })));
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3fbe1b9. CredentialType validation now allows 128 hex chars / 64 bytes, matching XLS-80, xrpl.js, and rippled. Added tests for max-size accepted and over-max rejected.

pub sequence: u32,
/// A hint indicating which page of the owner directory links to this object,
/// in case the directory consists of multiple pages.
pub owner_node: Option<Cow<'a, str>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OwnerNode is SoeRequired on the PermissionedDomain ledger entry — should not be Option.

Source: include/xrpl/protocol/detail/ledger_entries.macro:

LEDGER_ENTRY(ltPERMISSIONED_DOMAIN, 0x0082, PermissionedDomain, permissioned_domain, ({
    {sfOwner,               SoeRequired},
    {sfSequence,            SoeRequired},
    {sfAcceptedCredentials, SoeRequired},
    {sfOwnerNode,           SoeRequired},  // <--
    {sfPreviousTxnID,       SoeRequired},
    {sfPreviousTxnLgrSeq,   SoeRequired},
}))

rippled always sets it on insert: slePd->setFieldU64(sfOwnerNode, *page); in PermissionedDomainSet.cpp. XLS-80 §2.1 also marks it required.

Every other SoeRequired OwnerNode in this codebase is Cow<'a, str>, not Option (see signer_list.rs:62, offer.rs:54, check.rs:38, pay_channel.rs:45, ticket.rs:32, escrow.rs:52, deposit_preauth.rs:36). Re-serializing a PermissionedDomain constructed with owner_node: None would produce a malformed ledger object.

Fix: make it pub owner_node: Cow<'a, str>,, update new() to take it by value, and remove test_serialize_without_owner_node (the field has no "without" case).

// After the fix this should fail to compile:
let _ = PermissionedDomain::new(None, None, "r".into(), vec![], 1, /* owner_node */ None, "...".into(), 1);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ba3b83a. OwnerNode is now required on PermissionedDomain, matching XLS-80, xrpl.js, and rippled SoeRequired; new() now takes it by value and the invalid “without OwnerNode” test was removed.

validate_credential(credential)?;
}
self.validate_currencies()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing all-zero DomainID rejection. rippled preflight rejects temMALFORMED when DomainID is present and equal to beast::kZero. The SDK doesn't validate DomainID at all on Set (only checks length / hex on Delete — see related comment).

Source: PermissionedDomainSet.cpp::preflight:

auto const domain = ctx.tx.at(~sfDomainID);
if (domain && *domain == beast::kZero)
    return temMALFORMED;

Fix: in get_errors, after the credential loop, check domain_id is Some(s) with s not all-'0' and a valid 64-char hex string. Mirror the validation already in PermissionedDomainDelete::get_errors so the two stay in sync.

Test demonstrating the bug:

#[test]
fn test_set_all_zero_domain_id_rejected() {
    let txn = PermissionedDomainSet {
        common_fields: CommonFields {
            account: "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh".into(),
            transaction_type: TransactionType::PermissionedDomainSet,
            ..Default::default()
        },
        domain_id: Some("0".repeat(64).into()),
        accepted_credentials: vec![Credential { issuer: "rIssuer".to_string(), credential_type: "4B5943".to_string() }],
    };
    assert!(matches!(txn.get_errors(), Err(XRPLModelException::InvalidValue { .. })));
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c2f3a21. PermissionedDomainSet now validates optional DomainID as a non-zero 64-character hex string, mirroring rippled preflight behavior.

// DomainID is the 32-byte hash of the PermissionedDomain ledger entry,
// serialized as 64 uppercase hex chars.
let domain_id = self.domain_id.as_ref();
if domain_id.len() != 64 || !domain_id.chars().all(|c| c.is_ascii_hexdigit()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same all-zero DomainID gap as PermissionedDomainSet. This check accepts a 64-char hex string of all '0', but rippled rejects temMALFORMED.

Source: PermissionedDomainDelete.cpp::preflight:

auto const domain = ctx.tx.getFieldH256(sfDomainID);
if (domain == beast::kZero)
    return temMALFORMED;

Fix: reject domain_id if every character is '0' (cheap superset of the rippled check that doesn't require parsing the hex).

Test demonstrating the bug:

#[test]
fn test_delete_all_zero_domain_id_rejected() {
    let txn = PermissionedDomainDelete {
        common_fields: CommonFields {
            account: "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh".into(),
            transaction_type: TransactionType::PermissionedDomainDelete,
            ..Default::default()
        },
        domain_id: "0".repeat(64).into(),
    };
    assert!(matches!(txn.get_errors(), Err(XRPLModelException::InvalidValue { .. })));
}

The current test_valid_domain_id_accepted happens to use "A".repeat(64) so it doesn't expose this — "0".repeat(64) returns Ok(()) today.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e676bd1. PermissionedDomainDelete now rejects all-zero DomainID values while preserving the existing 64-character hex validation.

}
for credential in &self.accepted_credentials {
validate_credential(credential)?;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing duplicate-credentials rejection. rippled rejects temMALFORMED if any two entries in AcceptedCredentials share the same (Issuer, CredentialType) pair. The SDK accepts duplicates.

Source: CredentialHelpers.cpp::checkArray:

auto [it, ins] = duplicates.insert(sha512Half(issuer, ct));
if (!ins) {
    JLOG(j.trace()) << "Malformed transaction: duplicates in credentials.";
    return temMALFORMED;
}

The loop hashes (issuer, credentialType) and rejects on first collision.

Fix: after the existing validate_credential loop, collect (issuer, credential_type) tuples into a BTreeSet (no extra deps needed — already in alloc::collections) and return an error on duplicate. The error variant can stay InvalidValue since rippled lumps this into temMALFORMED.

Test demonstrating the bug:

#[test]
fn test_duplicate_credentials_rejected() {
    let dup = Credential { issuer: "rIssuer".to_string(), credential_type: "4B5943".to_string() };
    let txn = PermissionedDomainSet {
        common_fields: CommonFields {
            account: "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh".into(),
            transaction_type: TransactionType::PermissionedDomainSet,
            ..Default::default()
        },
        domain_id: None,
        accepted_credentials: vec![dup.clone(), dup],
    };
    assert!(txn.get_errors().is_err());
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f833e64. PermissionedDomainSet now rejects duplicate (Issuer, CredentialType) pairs, matching xrpl.js and rippled behavior.

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.39336% with 22 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@8e1cc7f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/models/transactions/permissioned_domain_set.rs 97.50% 13 Missing ⚠️
.../models/transactions/permissioned_domain_delete.rs 96.13% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #157   +/-   ##
=======================================
  Coverage        ?   84.05%           
=======================================
  Files           ?      222           
  Lines           ?    23148           
  Branches        ?        0           
=======================================
  Hits            ?    19456           
  Misses          ?     3692           
  Partials        ?        0           
Flag Coverage Δ
integration 71.04% <ø> (?)
unit 84.83% <97.39%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/models/ledger/objects/mod.rs 84.09% <ø> (ø)
src/models/ledger/objects/permissioned_domain.rs 100.00% <100.00%> (ø)
src/models/transactions/mod.rs 50.00% <ø> (ø)
.../models/transactions/permissioned_domain_delete.rs 96.13% <96.13%> (ø)
src/models/transactions/permissioned_domain_set.rs 97.50% <97.50%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

realonbebeto pushed a commit to realonbebeto/xrpl-rust that referenced this pull request Jun 5, 2026
… #3270) (XRPLF#291)

## Summary

The `rippled` binary was renamed to `xrpld` upstream, and the
`rippleci/rippled` image stopped receiving updates. Our integration
tests across every open PR started failing because the published
`develop` image exited before becoming healthy (`Connection refused` on
`localhost:5005`, **0 passed / 41 failed**).

This PR mirrors the upstream fix in xrpl.js:
[XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270).
Switching to `rippleci/xrpld:develop` is the **actual root-cause fix**
rather than pinning an old digest of the deprecated image.

## Changes

`.github/workflows/integration_test.yml`:
- `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE:
rippleci/xrpld:develop`.
- `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image
handles `mkdir` + launch internally; no more `bash -c "mkdir -p
/var/lib/rippled/db/ && rippled -a"` wrapper).
- Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`.
- Container name: `rippled-service` -> `xrpld-service`.
- Removed the docker `--health-cmd` (which shelled out to the renamed
`rippled` CLI and always failed) in favour of a direct JSON-RPC poll
against `http://localhost:5005/`.
- Always dump container logs on the stop step for post-mortem
visibility.

`.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`:
- `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`.
- `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`.
- `[debug_logfile] /var/log/rippled/debug.log` ->
`/var/log/xrpld/debug.log`.

## Verification

Validated on throwaway PR XRPLF#292 (now closed): **Integration Test green in
2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check
also pass.

## Related follow-up

The 7 in-flight PRs (XRPLF#130, XRPLF#131, XRPLF#151, XRPLF#153, XRPLF#156, XRPLF#157, XRPLF#158) currently
carry a stopgap commit pinning `rippleci/rippled:develop` to a specific
digest. After this PR merges to `main`, those branches should:
1. Rebase on `main` to pick up the xrpld switch, or
2. Cherry-pick this commit and drop the stopgap digest pin.

## Test plan

- [x] Validated end-to-end on PR XRPLF#292
- [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass
- [ ] Merge and confirm subsequent PRs inherit the fix without manual
cherry-pick

## Credit

Approach lifted from @ckeshava's
[xrpl.js#3270](XRPLF/xrpl.js#3270).
@ckeshava

Copy link
Copy Markdown
Collaborator

@e-desouza please rebase with the main branch, that should run the CI tests

None,
None,
None, // No domain_id means create new domain
vec![Credential {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e-desouza I think Permissioned Domains depends on existence of Credentials first on the ledger. I think we can either merge #154 first or point this PR to #154 so that we have ability to create credentials first and then write integration tests for Permissioned Domains.

pub deliver_min: Option<Amount<'a>>,
/// The PermissionedDomain ledger entry ID to use for permissioned DEX pathfinding.
#[serde(rename = "DomainID")]
pub domain_id: Option<Cow<'a, str>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change and the one in offer_create.rs pertains to XLS-0081-permissioned-dex. If we are merging both XLS-80 and XLS-81 in one PR, then we need to update the Offer ledger object as well as other RPCs as mentioned in XLS-81, or we can log an issue and handle Permissioned DEX in separate PR.

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