feat: add Credentials support (XLS-70)#154
Conversation
e-desouza
left a comment
e-desouza
left a comment
There was a problem hiding this comment.
Took a close look at this. The structure follows existing patterns well and the scope is right for an initial XLS-70 pass, but there are a couple of data-breaking issues that need fixing before merge.
Blockers:
-
credential_idsserializes asCredentialIdsinstead ofCredentialIDs. The struct-levelrename_all = "PascalCase"gets the casing wrong. Needs explicit#[serde(rename = "CredentialIDs")]on all four transaction types that gained this field. This would silently break serialization against rippled. -
The
Credentialledger object is missing thelsfAcceptedflag (0x00010000). Without it the SDK can't tell whether a credential has been accepted by the subject, which is the whole point of the accept/unaccept lifecycle in XLS-70. Should use aCredentialFlagenum instead ofNoFlags. -
Clippy fails with
large_enum_variantonXRPLRequestafter theLedgerEntrystruct grew. Needs boxing. -
cargo fmtfails on two files.
Should fix:
- No
credential_typelength validation (spec: non-empty, max 64 bytes / 128 hex chars). The codebase already does this fordomainanduri. - No
urilength validation inCredentialCreate(spec: max 256 bytes). CredentialAuthorizationFields/CredentialAuthorizationare duplicated identically in bothtransactions/deposit_preauth.rsandledger/objects/deposit_preauth.rs. Define once, re-export.- No validation on
AuthorizeCredentialsarray size (spec: 1-8 items). - No tests exercise
credential_idson the four modified transactions. A serde roundtrip would have caught the rename bug.
Implemented all blocker items and the listed should-fix items in commit 85430fe (with main fixes in dede078). This includes |
e-desouza
left a comment
e-desouza
left a comment
There was a problem hiding this comment.
Good progress. 9 of 10 original issues are fixed. But the fix commits introduced three new problems.
1. Test failure: credential_delete::test_valid_with_subject
The test sets account = "rSubmitter..." and subject = "rSubject..." (different addresses). The new validation correctly enforces account == subject || account == issuer, but this test wasn't updated to match. Fix: use the same address for both account and subject.
2. no_std build is broken
Three missing imports:
vec!macro not in scope intransactions/deposit_preauth.rs(lines 378, 400) andledger/objects/deposit_preauth.rs(line 177). Needsuse alloc::vec;in the test modules.Boxnot in scope inrequests/mod.rs(lines 150, 325). TheBox<LedgerEntry>fix forlarge_enum_variantneedsuse alloc::boxed::Box;for no_std builds.
Reproduce with: cargo test --release --no-default-features --features embassy-rt,core,utils,wallet,models,helpers,websocket,json-rpc
3. cargo fmt still fails
Import ordering in src/models/ledger/objects/deposit_preauth.rs. Just needs a cargo fmt --all pass.
Fixed in ba48c88. I updated the failing |
e-desouza
left a comment
e-desouza
left a comment
There was a problem hiding this comment.
Reviewed across three rounds. All issues from both review passes are fixed: serde CredentialIDs rename, typed lsfAccepted flag, credential_type validation, shared authorization types, CredentialDelete submitter checks, array bounds, no_std imports, and test fixture alignment. Ran the full test suite locally -- fmt, clippy, 614 default tests, 485 no_std tests, and 41 integration tests against a standalone rippled node. All green.
One thing for next time: the agent's sandbox needs firewall rules allowing outbound access to testnet.xrpl-labs.com, faucet.altnet.rippletest.net, and xrpl.org so it can run integration tests against the testnet faucet directly.
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several correctness issues flagged inline: a logic bug in CredentialDelete authorization that incorrectly rejects valid implicit-issuer cases (line 120), an unreachable else branch (line 140), doc/code contradiction on both-None (line 118), missing length validation for authorize_credentials in the ledger object, unvalidated credential_ids on Payment/AccountDelete/EscrowFinish/PaymentChannelClaim, and Cow<[T]> serde reliability concerns across multiple types. The authorize field change in DepositPreauth is also a semver-breaking API change that needs changelog/version attention.
Review by Claude Opus 4.6 · Prompt: V12
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| pub previous_txn_id: Cow<'a, str>, | ||
| /// The index of the ledger containing the transaction that most recently modified this object. | ||
| pub previous_txn_lgr_seq: u32, |
There was a problem hiding this comment.
Given that the rust library is yet to be equipped with many ledger-objects in the future, this is a good time to refactor common fields such as these to prevent code duplication.
previous_txn_lgr_seq and previous_txn_id is found in a huge majority of the ledger objects. These fields should be refactored.
There was a problem hiding this comment.
I left this unresolved because moving PreviousTxnID/PreviousTxnLgrSeq into ledger-object common fields would be a broader public-model refactor across many existing ledger objects, not just the XLS-70 Credential object. I addressed the concrete XLS-70 validation/test feedback in af71a30 and can handle this as a separate refactor if you want it in this PR.
There was a problem hiding this comment.
Tracked separately in #325.
This is broader than PR #154's XLS-70 credential changes because it involves cross-model public API/request-shape refactors:
- ledger-object common metadata fields like
PreviousTxnID/PreviousTxnLgrSeq - request ledger lookup fields via
LookupByLedgerRequest
Leaving this PR focused on the XLS-70 compatibility fixes and handling the CommonFields refactor in the follow-up issue.
| pub credentials: Option<Vec<Cow<'a, str>>>, | ||
| /// The unique identifier of a ledger. | ||
| #[serde(flatten)] | ||
| pub ledger_lookup: Option<LookupByLedgerRequest<'a>>, |
There was a problem hiding this comment.
This model can be further simplified by refactoring LookupByLedgerRequest inside CommonFields.
There was a problem hiding this comment.
Tracked separately in #325.
This is broader than PR #154's XLS-70 credential changes because it involves cross-model public API/request-shape refactors:
- ledger-object common metadata fields like
PreviousTxnID/PreviousTxnLgrSeq - request ledger lookup fields via
LookupByLedgerRequest
Leaving this PR focused on the XLS-70 compatibility fixes and handling the CommonFields refactor in the follow-up issue.
…egration tests Each credential_ids integration test now verifies both sides of the authorization gate: - Submit WITHOUT credential_ids → assert tecNO_PERMISSION (gate is enforced) - Submit WITH credential_ids → assert tesSUCCESS (gate is satisfied) Previously tests only checked the happy path, which passed regardless of whether the DepositAuth gate was actually enforced on the ledger.
Move hardcoded "4B5943" (hex for "KYC") into a single exported constant in tests/common/constants.rs and reference it from all credential integration tests. Eliminates 7 independent copies of the same magic string.
Distribute proptest blocks from the integration crate (proptest_credentials.rs) into #[cfg(test)] mod tests in each model file. Unit tests belong with the code under test, not in a separate integration binary that requires a running node and adds noise to cargo test --release. Removes tests/proptest_credentials.rs entirely; no behavioral change.
…dential_ids test Replace raw sign_and_submit + ledger_accept with the test_transaction helper, which asserts tesSUCCESS and advances the ledger atomically. Consistent with how every other transaction submission is written in the integration suite.
…d and ledger_entry tests Replace inline CredentialCreate + CredentialAccept + account_objects sequences with the shared provision_credential helper. Removes ~50 lines of duplicated setup code and makes the test intent clearer.
… serde roundtrips Using ? with map_err gives proptest proper failure messages on serialize/deserialize errors instead of a panic that can mask shrinking.
…xrpl-labs.com xrpl-labs testnet is rate-limiting / timing out. Switch to the canonical altnet endpoints: - JSON-RPC: https://s.altnet.rippletest.net:51234 - WebSocket: wss://s.altnet.rippletest.net:51233 Faucet URL derivation in get_faucet_url already matches on "altnet", so faucet routing works automatically with the new host.
…ests `amount` (without `signature`) does not deliver XRP — it is the cumulative claim amount for signature-validated off-chain claims. To deliver XRP directly from the source account, `balance` is the correct field. Negative gate test was getting tesSUCCESS (no delivery → DepositAuth not triggered) instead of tecNO_PERMISSION. Using `balance` causes actual fund delivery and the DepositAuth gate fires correctly.
Tests the full DepositAuth gate for direct XRP payments: negative assertion (tecNO_PERMISSION without credentials) then positive (tesSUCCESS with credential_ids). This is the highest-value gap vs rippled's DepositAuth_test.cpp.
…on tests Scenarios added (from rippled Credentials_test.cpp): 6. duplicate CredentialCreate → tecDUPLICATE 7. double CredentialAccept → tecDUPLICATE 8. CredentialCreate with uri field — uri stored on ledger object 9. expired credential → tecEXPIRED on create; CredentialAccept after expiry → tecEXPIRED
- payment.rs: add missing ledger_accept() after negative submit (prevented positive test from seeing committed sequence number → tefPAST_SEQ risk) - payment.rs: replace Payment::new() positional soup with struct literal (CLAUDE.md: prefer ..Default::default() over positional None args) - credential.rs: saturating_sub(2) instead of - 2 to avoid u64 underflow on early-genesis standalone nodes - credential.rs: use provision_credential() in test_credential_accept_duplicate instead of inlining create+accept manually - credential.rs: remove redundant inline use imports (already at file scope) - credential_create.rs / credential_accept.rs proptests: use "AB".repeat(len) with len in 1..=64 instead of single-char repeat — ensures even-length hex (valid byte-encodable strings) rather than odd-length strings that are technically undecodable as complete bytes
…tion
validate_credential_type: reject odd-length hex strings (cannot represent
whole bytes; rippled binary codec rejects them at serialization time).
validate_credential_ids: enforce exactly 64 hex chars per ID (credential IDs
are SHA-512Half ledger-object hashes: 256-bit = 32 bytes = 64 hex chars).
Update tests to use even-length non-hex strings ("NOTHEX") for non-hex error
assertions. Fix credential_delete proptest regex to produce even-length strings
only: ([0-9A-F]{2}){1,64} instead of [0-9A-F]{2,64}.
Add test_credential_type_odd_length_error test to explicitly cover the new
parity guard.
e-desouza
force-pushed
the
copilot/xls-70-support-credentials
branch
from
a7c2952 to
f08feb5
Compare
…redential_ids tests Convert EscrowCreate/EscrowFinish, AccountDelete, PaymentChannelCreate/ PaymentChannelClaim positional constructors to struct-literal form with ..Default::default() in the new *_with_credential_ids test functions.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Odd-length hex URIs cannot represent whole bytes and would be rejected by rippled's binary codec. Added parity guard matching validate_credential_type. Updated test fixture to use even-length non-hex string for the non-hex path.
| # Seeds for failure cases proptest has generated in the past. It is | ||
| # automatically read and these particular cases re-run before any | ||
| # novel cases are generated. | ||
| # | ||
| # It is recommended to check this file in to source control so that | ||
| # everyone who runs the test benefits from these saved cases. | ||
| cc fbc4b34a079b2bde9fb6af79a2d16814ab42060ff7d2987f8c150f8ddc132cde # shrinks to ct = "AAAA000A00A0A00A0A0A000", has_subject = false, has_issuer = false |
There was a problem hiding this comment.
Did you intend to commit this file into the git worktree? Where is this test-file used?
As per my understanding, the rust SDK only has unit, integration and CLI tests
…redential conflicts
High Level Overview of Change
This PR adds initial XLS-70 Credentials support to the model layer and includes follow-up fixes from review to ensure protocol compatibility, validation correctness, and no_std build compatibility.
Credential transaction models
CredentialCreate,CredentialAccept, andCredentialDelete.credential_type(non-empty, max 128 hex chars) in all three credential transactions.urimax length inCredentialCreate.CredentialDeletevalidation requiring the submitterAccountto match eithersubjectorissuer.CredentialDeletetests to align with the new submitter validation logic (account == subject || account == issuer).Credential ledger object
Credentialledger object model.CredentialFlagenum withLsfAccepted = 0x00010000.NoFlags.DepositPreauth XLS-70 extensions
AuthorizeCredentialsUnauthorizeCredentialsvec!macro usage.Shared credential authorization types
CredentialAuthorizationFields/CredentialAuthorizationinto a shared model:src/models/credential_authorization.rsDepositPreauthmodels.Related request/model surface
account_objectsrequest supportstype=credential.ledger_entryrequest supports credential selector fields.deposit_authorizedrequest supportscredentials.credential_idson affected tx models:Payment,EscrowFinish,PaymentChannelClaim,AccountDelete.#[serde(rename = "CredentialIDs")]on all four to match rippled casing.Clippy / formatting / compatibility fixes
XRPLRequest::LedgerEntryto resolvelarge_enum_variant.use alloc::boxed::Box;in request models for no_std compatibility.cargo fmtfixes, including import ordering.CredentialIDsfield naming.Compatibility + docs
DepositPreauth::new(...)constructor semantics and added credential-based constructor path for ledger objects.Context of Change
XLS-70 requires first-class support for
Credentialobjects and credential-aware authorization flows. The initial pass added missing model coverage, but review identified protocol-facing gaps (notablyCredentialIDscasing, missinglsfAccepted, and missing validations) that could cause silent incompatibility with rippled or allow invalid model states. A later review also identified no_std regressions introduced during follow-up fixes (missingBox/vec!imports and a mismatched test fixture). This update closes those gaps while preserving the original architecture and scope.Type of Change
Before / After
Before:
credential_idsserialized incorrectly asCredentialIdsinstead ofCredentialIDs.Credentialobject had no typedlsfAcceptedflag support.DepositPreauthcredential arrays were not size-validated.XRPLRequestlarge enum variant.After:
CredentialIDsserializes correctly on all four affected transaction types.Credentialobject exposes typedCredentialFlag::LsfAccepted.DepositPreauthcredential arrays are validated at 1..=8.XRPLRequest::LedgerEntryis boxed and no_std-compatible (alloc::boxed::Boxin scope).alloc::vecimport forvec!.CredentialDeletetest fixtures now match the enforced submitter relationship rules.Test Plan
models::transactions::payment::tests::test_credential_ids_serde_namemodels::transactions::account_delete::tests::test_credential_ids_serde_namemodels::transactions::escrow_finish::tests::test_credential_ids_serde_namemodels::transactions::payment_channel_claim::tests::test_credential_ids_serde_namemodels::transactions::credential_create::tests::test_credential_type_length_validationmodels::transactions::credential_accept::tests::test_credential_type_length_validationmodels::transactions::credential_delete::tests::test_account_must_match_subject_or_issuermodels::transactions::credential_delete::tests::test_valid_with_subjectmodels::transactions::deposit_preauth::tests::test_authorize_credentials_array_size_validationmodels::transactions::deposit_preauth::tests::test_valid_with_authorize_credentialsmodels::ledger::objects::deposit_preauth::tests::test_serde_with_authorize_credentialscargo test --release --no-default-features --features embassy-rt,core,utils,wallet,models,helpers,websocket,json-rpccargo fmt --allcargo clippy --lib -- -D warnings