Skip to content

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

Open
e-desouza wants to merge 14 commits into
mainfrom
feat/xls-80
Open

feat: add PermissionedDomains support (XLS-80)#331
e-desouza wants to merge 14 commits into
mainfrom
feat/xls-80

Conversation

@e-desouza

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

  • PermissionedDomainSet.get_errors() rejects: empty credentials list, duplicate (Issuer, CredentialType) pairs (case-normalised), invalid issuer address, malformed CredentialType hex, all-zero DomainID, list exceeding 10 entries
  • PermissionedDomainDelete.get_errors() rejects all-zero DomainID
  • ledger_entry PermissionedDomain selector validated: Index must be 64-char hex; Object requires valid classic address and u32-range seq

Registration

  • TransactionType::PermissionedDomainSet, TransactionType::PermissionedDomainDelete added to enum
  • LedgerEntryType::PermissionedDomain (0x0082) added to enum
  • XRPLRequest::LedgerEntry boxed to fix large_enum_variant clippy

CI

  • Integration test health check polls xrpld /server_info directly instead of relying on Docker container health status

Test plan

  • Serde roundtrip tests for PermissionedDomainSet, PermissionedDomainDelete, PermissionedDomain
  • Validation: empty creds, duplicate creds, invalid issuer, bad hex, zero DomainID, oversized list
  • ledger_entry selector tests: Index (valid + invalid hex), Object (valid + invalid address)
  • Integration tests: full lifecycle (Set → fetch → Delete), error codes (temARRAY_EMPTY, tecNO_ISSUER), account_objects filter, ledger_entry by index and by account+seq
  • cargo fmt, cargo clippy --all-features -D warnings, cargo test --release pass
  • no_std build clean

Supersedes #157.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.31835% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.15%. Comparing base (e4fab93) to head (a0f5fc1).

Files with missing lines Patch % Lines
src/models/ledger/objects/permissioned_domain.rs 86.33% 25 Missing ⚠️
src/models/transactions/permissioned_domain_set.rs 97.29% 16 Missing ⚠️
.../models/transactions/permissioned_domain_delete.rs 95.98% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   85.77%   86.15%   +0.38%     
==========================================
  Files         235      238       +3     
  Lines       26828    27891    +1063     
==========================================
+ Hits        23012    24030    +1018     
- Misses       3816     3861      +45     
Flag Coverage Δ
integration 71.32% <ø> (-0.20%) ⬇️
unit 86.85% <95.31%> (+0.38%) ⬆️

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/requests/account_objects.rs 92.30% <ø> (ø)
src/models/requests/ledger_entry.rs 94.17% <100.00%> (+3.66%) ⬆️
src/models/requests/mod.rs 0.00% <ø> (ø)
src/models/transactions/mod.rs 58.38% <ø> (+2.01%) ⬆️
.../models/transactions/permissioned_domain_delete.rs 95.98% <95.98%> (ø)
src/models/transactions/permissioned_domain_set.rs 97.29% <97.29%> (ø)
src/models/ledger/objects/permissioned_domain.rs 86.33% <86.33%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@e-desouza e-desouza force-pushed the feat/xls-80 branch 2 times, most recently from b4669b1 to 2906cde Compare June 23, 2026 23:42
Comment thread tests/transactions/permissioned_domain_set.rs Outdated
Comment thread src/models/requests/ledger_entry.rs Outdated
Comment on lines +183 to +207
if let Some(pd) = &self.permissioned_domain {
match pd {
PermissionedDomain::Index(id) => {
if id.len() != 64
|| !id.chars().all(|c| c.is_ascii_hexdigit())
|| id.chars().all(|c| c == '0')
{
return Err(XRPLModelException::InvalidValue {
field: "permissioned_domain.index".into(),
expected: "non-zero 64-character hex string".into(),
found: id.as_ref().into(),
});
}
}
PermissionedDomain::Object(obj) => {
if !crate::core::addresscodec::is_valid_classic_address(&obj.account) {
return Err(XRPLModelException::InvalidValue {
field: "permissioned_domain.account".into(),
expected: "valid classic XRPL address".into(),
found: obj.account.as_ref().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.

This check is not necessary. The values returned from a rippled server are from the XRPL blockchain state data. If there are errors in that data, it must be caught and validated by the rippled node implementation. There is little that the client library can do to solve that problem.

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.

Removed in 904cfdd. No other ledger_entry selector validates fields client-side — the PD selector now matches that convention.

Comment thread src/models/requests/mod.rs Outdated
LedgerCurrent(ledger_current::LedgerCurrent<'a>),
LedgerData(ledger_data::LedgerData<'a>),
LedgerEntry(ledger_entry::LedgerEntry<'a>),
LedgerEntry(alloc::boxed::Box<ledger_entry::LedgerEntry<'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.

Why is the Box type necessary here? PD is similar to the existing ledger-objects.

I'm concerned that using Box will weaken some of the memory-safety guarantees of concrete Rust types

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.

Removed in bc2f1f8. The Box was only silencing the large_enum_variant lint. Replaced with #[allow(clippy::large_enum_variant)] on XRPLRequest — same pattern already on XRPLResult.

Comment thread tests/transactions/mod.rs
pub mod escrow_cancel;
pub mod escrow_create;
pub mod escrow_finish;
pub mod mptoken_issuance_create;

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.

Why is this transaction module removed?

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.

Accidental rebase deletion. pub mod mptoken_issuance_create; was dropped from tests/transactions/mod.rs but the file was still on disk — silently uncompiled. Restored in 5594406.

@@ -0,0 +1,139 @@
// xrpl.js reference: N/A (XLS-80 is a new feature)

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.

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.

Cross-referenced in db4eb3a. Two gaps closed: Flags == 0 assertion on new domains, and deep-equal between account_objects entry and ledger_entry node — both matching the xrpl.js lifecycle test.

Comment on lines +4 to +9
// Scenarios:
// - base: create a new PermissionedDomain, verify tesSUCCESS
// - account_objects_filter: filter by type=permissioned_domain; verify exactly 1 object
// - ledger_entry_by_index: query domain by its ledger hash
// - ledger_entry_by_account_seq: query domain by owner account + sequence
// - update: replace credentials on existing domain (KYC → AML), verify KYC absent

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.

Since there is so much in common between these two integration test files, merging them into a single file will make for more readable code

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.

Done in db4eb3a. Merged into tests/transactions/permissioned_domain.rs. Delete test now uses the shared kyc_credential/new_pd_set helpers. Single pub mod permissioned_domain in mod.rs.

Patel-Raj11
Patel-Raj11 previously approved these changes Jun 25, 2026
e-desouza added 13 commits June 25, 2026 18:17
Adds LedgerEntryType::PermissionedDomain (0x0082) with Owner,
AcceptedCredentials, Sequence, OwnerNode (SoeRequired), PreviousTxnID,
PreviousTxnLgrSeq, and Flags fields.

OwnerNode typed as non-optional String per rippled SoeRequired constraint.
Validates:
- AcceptedCredentials list must be 1–10 entries (temARRAY_EMPTY / temARRAY_TOO_LARGE)
- CredentialType must be even-length hex string, max 128 chars (64 bytes)
- Duplicate (Issuer, CredentialType) pairs rejected — comparison is case-normalised
  to uppercase to match rippled behaviour
- Issuer must be a valid classic XRPL address
- DomainID (when updating) must not be all zeros
Validates DomainID is present and not all zeros.
Adds PermissionedDomain enum with two lookup variants:
- Index(hash) — 64-char hex string validated
- Object(account, seq) — account validated as classic address, seq within u32 range

XRPLRequest::LedgerEntry variant is boxed to satisfy large_enum_variant lint.

Imports ToString and alloc::format for no_std compatibility.
Unit tests:
- Serde roundtrip for Set and Delete
- Validation: empty creds, duplicate creds, invalid issuer, bad hex CredentialType,
  zero DomainID, oversized list, ledger_entry invalid index/account

Integration tests (against xrpld standalone):
- Full lifecycle: Set → account_objects → ledger_entry by index → Delete
- ledger_entry lookup by account+seq
- Error codes: temARRAY_EMPTY, tecNO_ISSUER, temARRAY_TOO_LARGE
- PermissionedDomainSet update (change credential list)
…ealth status

Docker container health reflects the container state, not whether xrpld has
finished loading the ledger. Poll the RPC endpoint directly so integration
tests only start after xrpld is ready to serve requests.
Integration tests reference AccountObjectType::PermissionedDomain for
account_objects filtering in permissioned domain tests, but the variant
was missing from the enum.
- Add Model::get_errors() validation to PermissionedDomain ledger object
  (owner address, 1..=10 credentials, per-credential validation)
- Fix seq field type u64→u32 in PermissionedDomainObject (XRPL sequences
  are u32; removes unreachable runtime range check)
- Add all-zeros rejection to PermissionedDomain::Index validation
  (consistent with validate_domain_id in permissioned_domain_set)
- Fix dead _expected assertion in ledger_entry test; switch to typed assert_eq
- Swap misplaced doc comments between validate_domain_id and validate_credential
- Fix duplicate credential error to include issuer/credential_type pair
- Add bounds assert to with_credential() (XLS-80 cap is 10 entries)
- Fix test_default to assert empty AcceptedCredentials fails validation
- Replace positional CommonFields::new() calls with struct literals (CLAUDE.md)
- Add eprintln! to silent temDISABLED skips in integration tests
- Fix 66-char domain_id test fixture (trim to 64 hex chars)
- Remove unused ToString import from ledger_entry.rs
… tests

PermissionedDomains is always enabled in standalone mode; conditional
skips are unnecessary and mask test failures.
…ector

rippled validates XRPL state data server-side. No other ledger_entry selector
performs per-field validation on the values it passes through — the PD selector
was the sole exception. Remove the address and index checks to match the
established convention and avoid false confidence from client-only guards.
Box was added only to silence the large_enum_variant lint. Every other
XRPLRequest variant is unboxed; the indirection breaks idiomatic Rust
ergonomics at the API boundary with no memory-safety benefit. Replace the
Box with an allow attribute on the enum, matching the pattern already used
on XRPLResult.
The two files shared wallet setup, client init, and helper functions.
Merging into permissioned_domain.rs makes the full lifecycle readable
as a single file and eliminates duplicated boilerplate.

Changes:
- Move all tests from permissioned_domain_set.rs and
  permissioned_domain_delete.rs into tests/transactions/permissioned_domain.rs
- Route the delete test through the shared kyc_credential/new_pd_set helpers
  instead of its inlined PermissionedDomainSet struct literal
- Drop the temDISABLED early-return guard from the delete path (PermissionedDomains
  is always enabled in standalone mode — redundant guard masked test intent)
- Add Flags=0 assertion (xrpl.js parity: newly-created PD has Flags=0)
- Add ledger_entry deep-equal vs account_objects assertion (xrpl.js parity:
  assert.deepEqual(pd, ledgerEntryResult.result.node))
- Replace two mod entries in tests/transactions/mod.rs with single
  pub mod permissioned_domain
…dation

Follow-up to the multi-stream review of the XLS-80 PermissionedDomain models.

- Replace the with_credential builder's assert! (panic on the 11th credential)
  with unconditional push; the 1..=10 bound is already enforced non-fatally by
  get_errors, so a library builder no longer aborts on caller input.
- Extract validate_accepted_credentials (count bound + per-credential validity +
  case-insensitive duplicate detection) and call it from both PermissionedDomainSet
  and the PermissionedDomain ledger object so the two validate under identical rules
  (the ledger object previously skipped the duplicate check).
- Reuse validate_domain_id in PermissionedDomainDelete instead of duplicating the
  len/hex/non-zero check inline, keeping the rule in lockstep with PermissionedDomainSet.
- Add the missing unit coverage: ledger-object get_errors (valid, invalid owner,
  empty list, duplicate, over-limit), case-insensitive credential dedup
  (4b5943 vs 4B5943), and DomainID wrong-length / non-hex rejection on the Set path.
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