Skip to content

fix(models): distinguish DepositPreauth validation errors in ledger_entry request - #358

Open
soloking1412 wants to merge 1 commit into
XRPLF:mainfrom
soloking1412:fix/deposit-preauth-error-variants
Open

fix(models): distinguish DepositPreauth validation errors in ledger_entry request#358
soloking1412 wants to merge 1 commit into
XRPLF:mainfrom
soloking1412:fix/deposit-preauth-error-variants

Conversation

@soloking1412

Copy link
Copy Markdown

High Level Overview of Change

Fixes two related validation bugs in DepositPreauth::get_errors() (the ledger_entry request selector, src/models/requests/ledger_entry.rs):

  1. The catch-all match arm returned the same ExpectedOneOf error for both "neither authorized nor authorized_credentials set" and "both set" — two states that need opposite remediation (add a field vs. remove one). These are now split into distinct arms.
  2. Duplicate entries in authorized_credentials raised InvalidValue, while every other site in the codebase that detects duplicate credentials (validate_credential_ids, the DepositPreauth transaction model, the DepositPreauth ledger object model) raises ValueEqualsValue. Callers that match on ValueEqualsValue to categorize "duplicate credential" errors silently fell through to their default arm for this one request type.

Context of Change

Both inconsistencies were found during review of #154 (XLS-70 Credentials support); neither was introduced by that PR. Filed as #333 and #334.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Tests

Before / After

Input Before After
authorized: None, authorized_credentials: None ExpectedOneOf(["authorized", "authorized_credentials"]) unchanged
authorized: Some, authorized_credentials: Some ExpectedOneOf(["authorized", "authorized_credentials"]) InvalidFieldCombination { field: "authorized", other_fields: ["authorized_credentials"] }
duplicate (issuer, credential_type) pair in authorized_credentials InvalidValue { field: "authorized_credentials", expected: "unique issuer and credential_type pairs", .. } ValueEqualsValue { field1: "authorized_credentials", field2: "authorized_credentials (duplicate entry)" }, matching every other duplicate-credential check in the codebase

InvalidFieldCombination was chosen over ValueEqualsValue for the mutually-exclusive-fields case because ValueEqualsValue is used elsewhere in this codebase strictly for "these two fields hold the same value" (e.g. account == authorize), not "these two fields are both present" — InvalidFieldCombination already carries that second meaning at the sibling ledger-object and transaction validators.

Test Plan

cargo test --release (1314 passed, up from 1313). Updated test_deposit_preauth_rejects_duplicate_authorized_credentials, test_deposit_preauth_rejects_case_variant_duplicate_credentials, and test_deposit_preauth_rejects_authorized_and_credentials to assert on the specific error variant instead of only is_err(). Added test_deposit_preauth_rejects_neither_authorized_nor_credentials, which previously had no coverage.

cargo clippy --all-targets and cargo fmt --check are clean on the changed file.

Closes #333
Closes #334

…ntry request

Split the (Some,Some)/(None,None) catch-all into distinct
InvalidFieldCombination/ExpectedOneOf arms, and align the duplicate-credential
error with ValueEqualsValue to match every other duplicate-credential check
in the codebase.

Closes XRPLF#333
Closes XRPLF#334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant