feat: add 25 new XRPL transaction types, 7 ledger objects, and docs#6
Merged
Conversation
…ation tests Add models, converters, TxFormat, validation, and integration tests for: - XChain Bridge (XLS-38d): 8 transaction types + 3 ledger objects - Vault (XLS-65d): 6 transaction types + 1 ledger object - Loan/LoanBroker (XLS-66d): 9 transaction types + 2 ledger objects - DelegateSet (XLS-74d) + LedgerStateFix: 2 transaction types + 1 ledger object
Currency properties in XChain and Loan transaction models were missing [JsonConverter(typeof(CurrencyConverter))] attributes, causing XRP amounts to serialize as objects instead of strings — leading to invalidTransaction errors. Also fix VaultClawback test to set asfAllowTrustLineClawback flag.
XChainBridge compound type was writing raw 20-byte AccountID fields without VL length prefix, causing rippled to reject blobs with "Invalid STAccount size". Now matches rippled's STXChainBridge::add() serialization format. Mark integration tests [Ignore] for amendments not available on rippled 3.0.0 standalone: Loan, DelegateKeys, LedgerStateFix, XChainBridge (requires sidechain), VaultCreate_WithAsset2 (Asset2 field unsupported), VaultClawback (XRP vaults not clawback-eligible).
…entation - Fix error messages to include error_exception from rippled responses (RequestManager.cs, XrplErrorClassifier.cs) - Fix nullable request fields (AccountObjects.Limit, DeletionBlockersOnly, AccountOffers.Strict) to prevent invalidParams errors on rippled 3.1.x - Fix XChainBridge XRP tests: use genesis account as IssuingChainDoor - Add IOU-IOU bridge tests with DefaultRipple, TrustLine setup, token issuance - Fix XChainBridgeType binary size assertions after VL-encoding fix - Update CI rippled config: unveto XChainBridge/SingleAssetVault/LendingProtocol, comment out removed Batch amendment - Add [Ignore] on tests for removed/unavailable amendments (Batch, DelegateSet, Loan, LedgerStateFix) with explanatory messages - Add Cross-Chain Bridge Guide documentation (EN + RU) to DocFx
…fication - Fix DelegateSet model: field "Delegate" → "Authorize" to match rippled protocol - Update validation to check for "Authorize" field - Add LODelegate ledger object verification in integration tests via account_objects - Verify Account, Delegate, Permissions fields and PermissionValue correctness
LedgerStateFix requires a special transaction cost >= owner reserve (0.2 XRP), similar to AccountDelete and AMMCreate. Added it to IsReserveFeeTxNeed() so Autofill handles the fee automatically. Updated test to use fail_hard and rely on Autofill for fee.
NumberType was writing 8 bytes (raw ulong) but rippled expects 12 bytes (8 bytes int64 mantissa + 4 bytes int32 exponent). This caused `invalid SerialIter geti32` errors on LoanSet submission. Key changes: - NumberType: 12-byte serialization with mantissa normalized to [10^18, MaxValue] - LoanSet: CounterpartySignature co-signing with fee tripling - LoanBrokerSet/LoanSet/LoanManage/LoanPay: field corrections per XLS-66 spec - TxFormat: corrected field requirements for Loan transaction types - Integration tests: all 8 Loan tests passing (broker, loan, pay, delete)
Add comprehensive documentation for the XRPL Lending Protocol (XLS-66d): - LendingProtocol-Guide.md (English) and LendingProtocol-Guide.ru.md (Russian) - Covers: broker setup, vault funding, loan creation, CounterpartySignature co-signing flow, Number type encoding, common errors, best practices - Update CHANGES.md with all new transaction types, ledger objects, and fixes - Add Lending Protocol Guide to DocFx table of contents
Implement three signing patterns for LoanSet CounterpartySignature analogous to Batch signing (XLS-66d): - V1 (automatic): both keys available locally - V2 (parallel): independent signing + CombineLoanSignatures merge - V3 (sequential): borrower signs first via SignAsLoanCounterparty, broker completes via LoanSigningHelper.BrokerSign Add SignAsLoanCounterparty() to XrplWallet for borrower partial signing. Add V2/V3 integration tests (10 total Loan tests passing). Update documentation with all three signing patterns.
…fill Fee for CounterpartySignature overhead is now computed in CalculateBaseFeeForType (baseFee * 2) alongside EscrowFinish and Batch fee logic. Removed adjustFee parameter from PrepareForSigning.
- Replace TestVaultCreate_WithAsset2 with TestVaultCreate_WithIOU (Asset2 not supported in VaultCreate on rippled 3.1.3) - Remove Asset2 from VaultCreate TxFormat - Rewrite TestVaultClawback_Basic to use IOU vault with proper setup: DefaultRipple, AllowTrustLineClawback, TrustLine, Payment, then VaultDeposit before clawback - All 8 Vault tests now pass
…docs - Audit all 25 Validate methods against xrpl.org documentation - VaultClawback: Holder field changed from Optional to Required in TxFormat - XChainAddAccountCreateAttestation: add missing OtherChainSource field - XChainAddClaimAttestation: add WasLockingChainSend validation - WasLockingChainSend changed from int to byte (UInt8, 0/1 values) - DelegateSet: rename Authorize to Delegate (PascalCase, no keyword conflict) - LOVault: add VaultDataFormat, VaultCreateFlags, VaultLedgerFlags, DataParsed - VaultCreate/VaultSet/VaultWithdraw: add Scale, WithdrawalPolicy, Destination fields - LOLoan/LOLoanBroker: relax STNumber null-field assertions (rippled omits zero defaults) - Add Vault Guide documentation (EN + RU)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cross-chain bridge, vault, lending protocol, and related support end‑to‑end: codec wire/JSON changes (Number, Issue, XChainBridge), many new ledger/transaction models and validators, client API/stream events, loan co‑signing helpers and wallet partial signing, autofill fee updates, extensive unit/integration tests, documentation (EN/RU), CI rippled feature toggles, and a .gitignore entry. ChangesEnd-to-end feature surface (codec → models → client → wallet → tests → docs)
Sequence Diagram(s)sequenceDiagram
participant Borrower as Borrower (Wallet)
participant Broker as Broker (Wallet)
participant Client as SDK Client
participant Server as XRPL Network
Borrower->>Client: PrepareForSigning(LoanSet)
Borrower->>Client: Sign counterparty preimage (SignAsLoanCounterparty)
Client-->>Broker: deliver partially-signed blob (contains CounterpartySignature)
Broker->>Client: BrokerSign(partially-signed blob)
Client->>Server: submit fully-signed tx blob
Server-->>Client: tx accepted / metadata
Client->>Broker: return TransactionSummary
Estimated code review effort Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
Base/Xrpl.BinaryCodec/Types/XChainBridgeType.cs-65-69 (1)
65-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate AccountID VL lengths before constructing
AccountId.
FromParserreadslockingDoorLen/issuingDoorLenbut never enforces the expected 20-byte AccountID length. A malformed buffer can be accepted and misparsed. Please reject non-20 lengths at parse time (Line 65 and Line 68).🔧 Proposed fix
public static XChainBridgeType FromParser(BinaryParser parser, int? hint = null) { int lockingDoorLen = parser.ReadVlLength(); + if (lockingDoorLen != 20) + throw new FormatException($"Invalid LockingChainDoor length: {lockingDoorLen}. Expected 20."); AccountId lockingDoor = new AccountId(parser.Read(lockingDoorLen)); Issue lockingIssue = Issue.FromParser(parser); int issuingDoorLen = parser.ReadVlLength(); + if (issuingDoorLen != 20) + throw new FormatException($"Invalid IssuingChainDoor length: {issuingDoorLen}. Expected 20."); AccountId issuingDoor = new AccountId(parser.Read(issuingDoorLen)); Issue issuingIssue = Issue.FromParser(parser); return new XChainBridgeType(lockingDoor, lockingIssue, issuingDoor, issuingIssue); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Base/Xrpl.BinaryCodec/Types/XChainBridgeType.cs` around lines 65 - 69, The code reads variable-length AccountID fields into lockingDoorLen and issuingDoorLen and constructs new AccountId without validating their length, so update the parsing logic in the XChainBridgeType parsing block (where lockingDoorLen, lockingDoor, Issue.FromParser, issuingDoorLen, issuingDoor are handled) to check parser.ReadVlLength() returns exactly 20 bytes before calling new AccountId(parser.Read(...)); if the length is not 20, throw a parsing/format exception (or return an error) to reject the malformed buffer rather than constructing an AccountId from an incorrect-length byte array.Xrpl/Client/RequestManager.cs-352-357 (1)
352-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
ErrorExceptionin the no-status error gate.On Line 352, responses with only
ErrorExceptioncurrently bypass this branch and get downgraded to generic"Response has no status".💡 Suggested fix
- if (response.Error is not null || response.ErrorMessage is not null) + if (response.Error is not null || response.ErrorMessage is not null || response.ErrorException is not null) { - string detail = response.ErrorMessage ?? response.ErrorException; + string detail = response.ErrorMessage ?? response.ErrorException ?? "Unknown error"; var errMessage = response.Error is null ? detail : $"{response.Error} - {detail}";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Client/RequestManager.cs` around lines 352 - 357, The branch that builds an error message currently only checks response.Error and response.ErrorMessage, causing responses with only response.ErrorException to fall through to the "no status" gate; update the conditional in RequestManager (where response is evaluated) to include response.ErrorException (i.e., if response.Error is not null || response.ErrorMessage is not null || response.ErrorException is not null) and keep constructing detail with response.ErrorMessage ?? response.ErrorException and errMessage using response.Error when present so that ErrorException-only failures are handled and logged like other errors.Xrpl/Models/Transactions/LoanSet.cs-292-296 (1)
292-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ValidateLoanSetis too permissive for required string fields.At Lines 292–296,
PrincipalRequestedonly checks null (not string/non-empty), andLoanBrokerIDallows empty whitespace strings. This lets invalid tx payloads pass validation.Suggested fix
- if (!tx.TryGetValue("LoanBrokerID", out var brokerId) || brokerId is not string) - throw new ValidationException("LoanSet: missing field LoanBrokerID"); + if (!tx.TryGetValue("LoanBrokerID", out var brokerId) + || brokerId is not string brokerIdText + || string.IsNullOrWhiteSpace(brokerIdText)) + throw new ValidationException("LoanSet: missing or invalid field LoanBrokerID"); - if (!tx.TryGetValue("PrincipalRequested", out var principal) || principal is null) - throw new ValidationException("LoanSet: missing field PrincipalRequested"); + if (!tx.TryGetValue("PrincipalRequested", out var principal) + || principal is not string principalText + || string.IsNullOrWhiteSpace(principalText)) + throw new ValidationException("LoanSet: missing or invalid field PrincipalRequested");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanSet.cs` around lines 292 - 296, The ValidateLoanSet validator is too permissive: update the checks for "LoanBrokerID" and "PrincipalRequested" so they require non-empty strings (not just presence or non-null). Specifically, in the ValidateLoanSet method, change the LoanBrokerID check to ensure tx.TryGetValue("LoanBrokerID", out var brokerId) && brokerId is string s && !string.IsNullOrWhiteSpace(s) (trim/validate non-empty) and change the PrincipalRequested check to ensure tx.TryGetValue("PrincipalRequested", out var principal) && principal is string p && !string.IsNullOrWhiteSpace(p); if either fails, throw the existing ValidationException with the same field-specific message.Tests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.cs-28-42 (1)
28-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd assertions; current test is observational-only.
Lines 28-42 only print values, so the test passes even when encoding/decoding regresses.
Suggested fix
string encoded = XrplBinaryCodec.Encode(node); - Console.WriteLine("Encoded hex: " + encoded); - Console.WriteLine("Encoded length: " + encoded.Length / 2 + " bytes"); + Assert.IsFalse(string.IsNullOrWhiteSpace(encoded), "Encoded hex should not be empty."); JsonNode decodedNode = XrplBinaryCodec.Decode(encoded); - string decoded = decodedNode.ToJsonString(new JsonSerializerOptions { WriteIndented = true }); - Console.WriteLine("Decoded JSON: " + decoded); + Assert.IsNotNull(decodedNode, "Decoded node should not be null."); + Assert.AreEqual("LoanSet", decodedNode["TransactionType"]?.ToString()); + Assert.AreEqual("10000000000000", decodedNode["PrincipalRequested"]?.ToString()); string principalDecoded = decodedNode["PrincipalRequested"]?.ToString(); - Console.WriteLine("PrincipalRequested decoded: " + principalDecoded); + Assert.AreEqual("10000000000000", principalDecoded); - // Also encode for signing string forSigning = XrplBinaryCodec.EncodeForSigning(node); - Console.WriteLine("ForSigning hex: " + forSigning); + Assert.IsFalse(string.IsNullOrWhiteSpace(forSigning), "Signing payload should not be empty.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.cs` around lines 28 - 42, The test currently only prints values; make it assert encoding/decoding correctness by adding assertions: use XrplBinaryCodec.Encode(node) and Assert.NotNull/NotEmpty on the resulting string, Assert.Equal on the decoded JSON or on specific fields to verify round-trip (e.g., compare decodedNode["PrincipalRequested"] to the original node's PrincipalRequested), and Assert.Equal for the byte length if you have an expected length; also assert that XrplBinaryCodec.EncodeForSigning(node) produces a non-empty string and, if available, matches an expected signing hex or at least differs from the regular Encode output. Use the test framework's Assert methods (e.g., Assert.Equal, Assert.NotNull/NotEmpty) to replace the Console.WriteLine checks around XrplBinaryCodec.Encode, Decode, decodedNode and EncodeForSigning.Base/Xrpl.BinaryCodec/Types/NumberType.cs-57-69 (1)
57-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ToJson()will throwOverflowExceptionfor large protocol-valid exponents.Lines 57-69 apply repeated decimal multiplication/division based on
Exponent(which permits magnitudes up to ±32768), butSystem.Decimalhas a maximum magnitude of ~7.92 × 10^28. A mantissa nearlong.MaxValue(≈9.22 × 10^18) multiplied by 10 even 11 times exceeds this limit and throwsOverflowExceptionat runtime. The protocol allows exponent magnitudes far beyond whatdecimalcan handle, and there is no overflow handling.A safer approach is string-based formatting from
(Mantissa, Exponent)instead of decimal scaling, or useBigIntegerfor arbitrary precision arithmetic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Base/Xrpl.BinaryCodec/Types/NumberType.cs` around lines 57 - 69, ToJson currently scales a System.Decimal by looping on Mantissa and Exponent which can overflow for protocol-allowed exponents; replace that decimal-scaling logic in ToJson with a string-based conversion (or BigInteger-backed) that builds the numeric string from Mantissa and Exponent without performing repeated decimal multiplication/division. Specifically, in ToJson use Mantissa and Exponent to compute sign, absMantissa (as BigInteger or string), then shift the decimal point by Exponent to produce a correctly placed integer/decimal or scientific-style string (handling large positive/negative exponents and trimming trailing zeros) and return that string instead of using System.Decimal; update any references to Mantissa and Exponent in NumberType.ToJson accordingly.Xrpl/Models/Transactions/TxFormat.cs-553-571 (1)
553-571:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
LoanSetformat is missingCounterpartySignature.Given LoanSet co-signing support, omitting this field from
TxFormatcan cause validation failures for otherwise valid signed LoanSet transactions.💡 Proposed fix
[BinaryCodec.Types.TransactionType.LoanSet] = new TxFormat { [Field.LoanBrokerID] = Requirement.Required, [Field.PrincipalRequested] = Requirement.Required, [Field.Counterparty] = Requirement.Optional, + [Field.CounterpartySignature] = Requirement.Optional, [Field.InterestRate] = Requirement.Optional, [Field.LateInterestRate] = Requirement.Optional, [Field.CloseInterestRate] = Requirement.Optional,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/TxFormat.cs` around lines 553 - 571, The LoanSet TxFormat (BinaryCodec.Types.TransactionType.LoanSet) is missing the CounterpartySignature field which breaks validation for co-signed LoanSet transactions; update the TxFormat initializer for LoanSet (the object assigned to BinaryCodec.Types.TransactionType.LoanSet) to include Field.CounterpartySignature with an appropriate requirement (e.g., Requirement.Optional) alongside the other fields so co-signed LoanSet transactions validate correctly.Xrpl/Sugar/Autofill.cs-324-327 (1)
324-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve-fee transactions can be incorrectly capped after adding
LedgerStateFix.Line 324 extends reserve-fee detection, but cap bypass still only checks
AccountDelete(Line 209). With a lowmaxFeeXRP,LedgerStateFixcan be autofilled with insufficient fee and fail submission.💡 Proposed fix
- totalFee = transactionType == "AccountDelete" + totalFee = IsReserveFeeTxNeed(tx) ? calculatedFee : BigInteger.Min(calculatedFee, maxFeeBI);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Sugar/Autofill.cs` around lines 324 - 327, The reserve-fee detection was expanded to include LedgerStateFix but the fee-cap bypass logic still only treats AccountDelete as exempt; update the cap-bypass check (the conditional that currently tests for nameof(TransactionType.AccountDelete)) to also allow nameof(TransactionType.LedgerStateFix) so LedgerStateFix transactions bypass the maxFeeXRP cap the same way; locate the comparison that checks for AccountDelete and extend it to include LedgerStateFix (or refactor both places to call the same IsReserveFeeTransaction helper) to keep behavior consistent.DocFx/LendingProtocol-Guide.md-435-448 (1)
435-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve contradictory LoanSet fee guidance.
Line 275 says Autofill already includes
CounterpartySignatureoverhead, but Lines 444-448 instruct users to triple the fee after autofill. These instructions conflict and can lead to systematic overpayment.Suggested fix
-| `telINSUF_FEE_P` | Fee too low after adding CounterpartySignature | Triple the fee after autofill to account for extra signature bytes | +| `telINSUF_FEE_P` | Fee too low after adding CounterpartySignature | Re-run Autofill on the final pre-signed payload and verify the computed fee before submit | ... -2. **Triple the fee for LoanSet** — after autofilling, multiply the fee by 3 to account for the `CounterpartySignature` overhead: - ```csharp - ulong feeDrops = ulong.Parse(loanTx.Fee.Value); - loanTx.Fee = new Currency { Value = (feeDrops * 3).ToString(), CurrencyCode = "XRP" }; - ``` +2. **Use Autofill for final fee calculation** — ensure the transaction includes the signing fields expected by your flow, then call Autofill and submit with the returned fee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DocFx/LendingProtocol-Guide.md` around lines 435 - 448, The docs conflict: one place states Autofill includes CounterpartySignature overhead while another instructs developers to manually triple loanTx.Fee after Autofill; remove the "triple the fee" snippet and replace it with guidance to rely on Autofill's returned fee (i.e., call Autofill after you have added any expected signing fields such as CounterpartySignature or other signing fields, then submit using the fee value returned in loanTx.Fee), and update the LoanSet guidance to reference using Autofill for final fee calculation instead of multiplying loanTx.Fee by 3.Xrpl/Wallet/XrplWallet.cs-1023-1033 (1)
1023-1033:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire broker
SigningPubKeybefore generatingCounterpartySignature.
SignAsLoanCounterpartysigns whatever preimage is present, but it doesn't enforce thatSigningPubKeyis already set (broker key). If missing, you produce a counterparty signature over the wrong preimage and the combinedLoanSetbecomes invalid.Suggested fix
string txType = tx["TransactionType"]?.GetValue<string>(); if (!string.Equals(txType, "LoanSet", StringComparison.OrdinalIgnoreCase)) throw new ValidationException($"SignAsLoanCounterparty requires TransactionType=LoanSet, got: {txType}"); + + var brokerSigningPubKey = tx["SigningPubKey"]?.GetValue<string>(); + if (string.IsNullOrWhiteSpace(brokerSigningPubKey)) + throw new ValidationException("LoanSet must include broker SigningPubKey before counterparty signing.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Wallet/XrplWallet.cs` around lines 1023 - 1033, The SignAsLoanCounterparty flow currently computes a signing preimage without ensuring the broker's SigningPubKey is present, causing invalid CounterpartySignature; in SignAsLoanCounterparty (XrplWallet.cs) check that tx["SigningPubKey"] (the broker's public key) exists and is non-empty before removing existing signatures and before calling LoanSigningHelper.GetSigningPreimage(tx), and if it's missing throw a ValidationException with a clear message (e.g., "SignAsLoanCounterparty requires broker SigningPubKey to be set"); this ensures you sign the correct preimage.Xrpl/Models/Transactions/VaultWithdraw.cs-53-55 (1)
53-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
CurrencyConvertertoAmountfor VaultWithdraw.
Amountis aCurrencyfield but lacksJsonConverter(typeof(CurrencyConverter))here, unlike other transaction models withCurrencypayloads.Suggested patch
+using Xrpl.Client.Json.Converters; using Xrpl.Models.Common; @@ /// <inheritdoc /> [JsonPropertyName("Amount")] + [JsonConverter(typeof(CurrencyConverter))] public Currency Amount { get; set; } @@ /// <inheritdoc /> [JsonPropertyName("Amount")] + [JsonConverter(typeof(CurrencyConverter))] public Currency Amount { get; set; }Also applies to: 73-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/VaultWithdraw.cs` around lines 53 - 55, The VaultWithdraw model's Amount property(s) are missing the JSON converter attribute; add [JsonConverter(typeof(CurrencyConverter))] to each Currency-typed Amount property in the VaultWithdraw class so they serialize/deserialize like other transaction models that use CurrencyConverter (apply this to the primary Amount property and the second Amount declaration later in the VaultWithdraw class).Xrpl/Models/Transactions/LedgerStateFix.cs-57-63 (1)
57-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required
Ownervalidation in LedgerStateFix validator.Line 61 only validates
LedgerFixType.Owneris part ofILedgerStateFixand currently passes through unchecked.Suggested patch
public static async Task ValidateLedgerStateFix(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); if (!tx.TryGetValue("LedgerFixType", out var fixType) || fixType is null) throw new ValidationException("LedgerStateFix: missing field LedgerFixType"); + + if (!tx.TryGetValue("Owner", out var owner) || owner is not string s || string.IsNullOrWhiteSpace(s)) + throw new ValidationException("LedgerStateFix: missing field Owner"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LedgerStateFix.cs` around lines 57 - 63, The LedgerStateFix validator (ValidateLedgerStateFix) currently only checks LedgerFixType but must also validate the required Owner field from ILedgerStateFix: add a check that tx.TryGetValue("Owner", out var owner) succeeds and owner is a non-null, non-empty string, and then validate the owner value using the project account/address validator (e.g. call Common.ValidateAccount(owner) or the equivalent helper in Common); if missing or invalid, throw a ValidationException with a clear message (e.g. "LedgerStateFix: missing or invalid field Owner").Xrpl/Models/Transactions/LoanManage.cs-93-99 (1)
93-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce LoanManage action-flag semantics in validation.
Line 97 validates only
LoanID; it doesn’t enforce that exactly one oftfLoanDefault/tfLoanImpair/tfLoanUnimpairis selected.Suggested patch
public static async Task ValidateLoanManage(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); if (!tx.TryGetValue("LoanID", out var id) || id is not string) throw new ValidationException("LoanManage: missing field LoanID"); + + if (!tx.TryGetValue("Flags", out var rawFlags) || rawFlags is null) + throw new ValidationException("LoanManage: missing field Flags"); + + uint flagsValue = rawFlags switch + { + uint u => u, + int i when i >= 0 => (uint)i, + long l when l is >= 0 and <= uint.MaxValue => (uint)l, + _ => throw new ValidationException("LoanManage: invalid field Flags") + }; + + LoanManageFlags flags = (LoanManageFlags)flagsValue; + int actionCount = + ((flags & LoanManageFlags.tfLoanDefault) != 0 ? 1 : 0) + + ((flags & LoanManageFlags.tfLoanImpair) != 0 ? 1 : 0) + + ((flags & LoanManageFlags.tfLoanUnimpair) != 0 ? 1 : 0); + + if (actionCount != 1) + throw new ValidationException("LoanManage: exactly one action flag must be set"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanManage.cs` around lines 93 - 99, ValidateLoanManage currently only checks LoanID; update it to also enforce that exactly one action flag is set among tfLoanDefault, tfLoanImpair, tfLoanUnimpair. In ValidateLoanManage, after verifying LoanID, read the three keys from tx (e.g., using tx.TryGetValue for "tfLoanDefault", "tfLoanImpair", "tfLoanUnimpair"), count how many evaluate as set/true (handle boolean or numeric truthy values), and if the count != 1 throw a ValidationException with a clear message like "LoanManage: must set exactly one of tfLoanDefault, tfLoanImpair, tfLoanUnimpair"; keep the existing LoanID check and Common.ValidateBaseTransaction call.Xrpl/Models/Transactions/VaultClawback.cs-47-48 (1)
47-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Amountis missingCurrencyConverter, which can break XRPL amount JSON encoding.Please align with other transaction models and explicitly apply
CurrencyConverterto both request and responseAmountfields.Proposed fix
using Xrpl.Client.Exceptions; +using Xrpl.Client.Json.Converters; using Xrpl.Models.Common; @@ [JsonPropertyName("Amount")] + [JsonConverter(typeof(CurrencyConverter))] public Currency Amount { get; set; } @@ [JsonPropertyName("Amount")] + [JsonConverter(typeof(CurrencyConverter))] public Currency Amount { get; set; }Also applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/VaultClawback.cs` around lines 47 - 48, The Amount property in the VaultClawback model lacks the CurrencyConverter attribute and must mirror other transaction models; add the JsonConverter attribute referencing CurrencyConverter to the Amount properties in VaultClawback (both occurrences — the request and response Amount fields) so JSON (de)serialization uses CurrencyConverter for XRPL amount encoding/decoding; update the Amount property declarations in the VaultClawback class to include [JsonConverter(typeof(CurrencyConverter))].Xrpl/Models/Transactions/LoanPay.cs-14-17 (1)
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMutually exclusive
LoanPayflags are documented but never enforced.
ValidateLoanPayshould reject combinations oftfLoanOverpayment,tfLoanFullPayment, andtfLoanLatePayment.Proposed fix
using System; using System.Collections.Generic; +using System.Text.Json; using System.Text.Json.Serialization; using System.Threading.Tasks; @@ public static async Task ValidateLoanPay(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); @@ if (!tx.TryGetValue("Amount", out var amount) || amount is null) throw new ValidationException("LoanPay: missing field Amount"); + + if (tx.TryGetValue("Flags", out var flagsObj) && flagsObj is not null) + { + uint rawFlags = flagsObj switch + { + uint u => u, + int i => checked((uint)i), + long l => checked((uint)l), + JsonElement { ValueKind: JsonValueKind.Number } je when je.TryGetUInt32(out var u) => u, + _ => throw new ValidationException("LoanPay: invalid field Flags"), + }; + + uint exclusive = (uint)(LoanPayFlags.tfLoanOverpayment | LoanPayFlags.tfLoanFullPayment | LoanPayFlags.tfLoanLatePayment); + uint selected = rawFlags & exclusive; + if (selected != 0 && (selected & (selected - 1)) != 0) + throw new ValidationException("LoanPay: mutually exclusive flags cannot be combined"); + } }Also applies to: 110-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanPay.cs` around lines 14 - 17, The enum LoanPayFlags documents mutually exclusive flags but ValidateLoanPay currently does not enforce that only one of tfLoanOverpayment, tfLoanFullPayment, and tfLoanLatePayment may be set; update ValidateLoanPay to read the LoanPayFlags bitmask (use the tfLoanOverpayment, tfLoanFullPayment, tfLoanLatePayment symbols) and fail validation if more than one of those three bits is present (e.g., count set bits or test pairwise combinations) so the method rejects invalid combinations and returns/throws the existing validation error type.Xrpl/Models/Transactions/XChainAddClaimAttestation.cs-45-48 (1)
45-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Destinationis defined as required but never validated.
ValidateXChainAddClaimAttestationshould reject payloads missingDestination.Proposed fix
if (!tx.TryGetValue("AttestationSignerAccount", out var signerAccount) || signerAccount is not string) throw new ValidationException("XChainAddClaimAttestation: missing field AttestationSignerAccount"); + if (!tx.TryGetValue("Destination", out var destination) || destination is not string) + throw new ValidationException("XChainAddClaimAttestation: missing field Destination"); + if (!tx.TryGetValue("OtherChainSource", out var source) || source is not string) throw new ValidationException("XChainAddClaimAttestation: missing field OtherChainSource");Also applies to: 188-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/XChainAddClaimAttestation.cs` around lines 45 - 48, ValidateXChainAddClaimAttestation currently doesn't check that the XChainAddClaimAttestation.Destination property is set; update the ValidateXChainAddClaimAttestation method to explicitly reject payloads where Destination is null, empty, or whitespace (throw or return the same validation error mechanism used elsewhere). Also add the same presence check for the other validator/validation block that handles the XChainAddClaimAttestation payload at the second location referenced (lines 188-190) so both validation paths enforce Destination is provided.Xrpl/Models/Transactions/LoanBrokerCoverClawback.cs-12-14 (1)
12-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ValidateLoanBrokerCoverClawbackcurrently allows empty transaction-specific payloads.The method should enforce “at least one of
LoanBrokerIDorAmount”.Proposed fix
public static async Task ValidateLoanBrokerCoverClawback(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); + + bool hasLoanBrokerId = tx.TryGetValue("LoanBrokerID", out var loanBrokerId) && loanBrokerId is string; + bool hasAmount = tx.TryGetValue("Amount", out var amount) && amount is not null; + + if (!hasLoanBrokerId && !hasAmount) + throw new ValidationException("LoanBrokerCoverClawback: at least one of LoanBrokerID or Amount is required"); }Also applies to: 62-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanBrokerCoverClawback.cs` around lines 12 - 14, The ValidateLoanBrokerCoverClawback method currently permits empty transaction payloads; update it (and the analogous check around lines 62-65) to require that at least one of LoanBrokerID or Amount is provided: inside ValidateLoanBrokerCoverClawback (and the duplicate validation block), check if both LoanBrokerID is null/empty/zero and Amount is null/zero and, if so, return/throw the existing validation error type with a clear message (e.g., "either LoanBrokerID or Amount must be specified") so the transaction is rejected when both fields are absent.Xrpl/Wallet/LoanSigningHelper.cs-163-166 (1)
163-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompatibility check drops
SigningPubKey, allowing invalid signature combinations.In V2, two blobs with different
SigningPubKeyvalues can be treated as compatible even though signatures are over different preimages.Proposed fix
// Verify bodies match (excluding signatures) + string brokerSigningPubKey = brokerTx["SigningPubKey"]?.GetValue<string>(); + string counterpartySigningPubKey = counterpartyTx["SigningPubKey"]?.GetValue<string>(); + if (!string.Equals(brokerSigningPubKey, counterpartySigningPubKey, StringComparison.Ordinal)) + throw new ValidationException("Incompatible LoanSet SigningPubKey values."); + JsonObject brokerCanon = Canonicalize(brokerTx); JsonObject counterpartyCanon = Canonicalize(counterpartyTx); @@ JsonObject canon = tx.DeepClone().AsObject(); canon.Remove("TxnSignature"); - canon.Remove("SigningPubKey"); canon.Remove("CounterpartySignature"); return canon;Also applies to: 238-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Wallet/LoanSigningHelper.cs` around lines 163 - 166, The compatibility check using Canonicalize(brokerTx) and Canonicalize(counterpartyTx) currently removes SigningPubKey, allowing different SigningPubKey values to pass; update the comparison logic in the method that does JsonObject brokerCanon = Canonicalize(brokerTx) / counterpartyCanon = Canonicalize(counterpartyTx) (and the analogous block at 238-243) to also require identical SigningPubKey values (or include SigningPubKey in the canonicalization result) before accepting the two transactions as compatible, and throw the same ValidationException if SigningPubKey differs.Xrpl/Wallet/LoanSigningHelper.cs-210-211 (1)
210-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
BrokerSignsilently overwritesSigningPubKey, which can invalidate an existingCounterpartySignature.If the partial blob was prepared with a different broker key, this should fail fast instead of returning an invalid signed blob.
Proposed fix
- tx["SigningPubKey"] = brokerWallet.PublicKey; + string existingSigningPubKey = tx["SigningPubKey"]?.GetValue<string>(); + if (!string.IsNullOrEmpty(existingSigningPubKey) && + !string.Equals(existingSigningPubKey, brokerWallet.PublicKey, StringComparison.Ordinal)) + { + throw new ValidationException("Partially signed blob SigningPubKey does not match broker wallet."); + } + tx["SigningPubKey"] = brokerWallet.PublicKey;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Wallet/LoanSigningHelper.cs` around lines 210 - 211, BrokerSign currently unconditionally sets tx["SigningPubKey"] = brokerWallet.PublicKey which can silently invalidate an existing CounterpartySignature; change BrokerSign in LoanSigningHelper so that if tx already contains "SigningPubKey" and its value does not equal brokerWallet.PublicKey you fail fast (throw an InvalidOperationException or return an error) instead of overwriting, otherwise (if absent or equal) set tx["SigningPubKey"] to brokerWallet.PublicKey; reference the BrokerSign method and the tx["SigningPubKey"] and CounterpartySignature fields when making this change.Xrpl/Models/Transactions/DelegateSet.cs-65-66 (1)
65-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Permissionsvalidation should enforce array shape, not just non-null.Right now scalar/object values pass validation even though this field is defined as a list.
Proposed fix
+using System.Text.Json; @@ if (!tx.TryGetValue("Permissions", out var perms) || perms is null) throw new ValidationException("DelegateSet: missing field Permissions"); + if (perms is not List<object> && perms is not JsonElement { ValueKind: JsonValueKind.Array }) + throw new ValidationException("DelegateSet: invalid field Permissions");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/DelegateSet.cs` around lines 65 - 66, The current check in DelegateSet (the tx.TryGetValue("Permissions", out var perms) branch) only ensures Permissions is non-null; change it to require that perms is an array (e.g., JArray or IEnumerable) and validate the element shape/type (reject scalar/object values) before proceeding; if perms is not an array or contains invalid items, throw a ValidationException with a clear message. Update the Permissions validation logic in the DelegateSet class where tx.TryGetValue("Permissions", ...) is used and ensure each element is validated for the expected type/structure.Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs-35-38 (1)
35-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinal-result validator should not treat
terQUEUEDas successFor
TransactionSummary(post-submit result), acceptingterQUEUEDcan hide non-final or failed outcomes.Proposed fix
- if (res is not { Meta: { TransactionResult: "tesSUCCESS" or "terQUEUED" } }) + if (res is not { Meta: { TransactionResult: "tesSUCCESS" } }) throw new RippleException($"Final tx result is not success: {res.Meta?.TransactionResult}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs` around lines 35 - 38, The validator ValidateResult currently treats both "tesSUCCESS" and "terQUEUED" as success; change it to accept only the final success result "tesSUCCESS". In ValidateResult, update the pattern check on TransactionSummary.Meta.TransactionResult to only match "tesSUCCESS" and throw RippleException with the actual TransactionResult when it is not "tesSUCCESS" to surface non-final/queued outcomes; reference the ValidateResult method, TransactionSummary.Meta.TransactionResult and RippleException when making the change.Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs-177-184 (1)
177-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winV2 signing is not isolated per signer
Both signatures are derived from the same mutable dictionary instance. If
Sign(...)mutates input, the second signature is no longer independent.Proposed fix
- Dictionary<string, object> brokerDict = JsonSerializer.Deserialize<Dictionary<string, object>>( - prepared.ToJsonString(), XrplJsonOptions.Default); + string preparedJson = prepared.ToJsonString(); + Dictionary<string, object> brokerDict = JsonSerializer.Deserialize<Dictionary<string, object>>( + preparedJson, XrplJsonOptions.Default); + Dictionary<string, object> borrowerDict = JsonSerializer.Deserialize<Dictionary<string, object>>( + preparedJson, XrplJsonOptions.Default); SignatureResult brokerSig = brokerWallet.Sign(brokerDict); // Device B (borrower): signs as counterparty (adds CounterpartySignature) - SignatureResult counterpartySig = borrowerWallet.SignAsLoanCounterparty(brokerDict); + SignatureResult counterpartySig = borrowerWallet.SignAsLoanCounterparty(borrowerDict);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs` around lines 177 - 184, The test currently reuses the same mutable Dictionary<string, object> (created via JsonSerializer.Deserialize(prepared.ToJsonString(), XrplJsonOptions.Default)) for both brokerWallet.Sign and borrowerWallet.SignAsLoanCounterparty which can cause the second sign to operate on a mutated object; fix by ensuring V2 signing is isolated: create a fresh independent dictionary for each signer (e.g. deserialize prepared.ToJsonString() twice or deep-clone the brokerDict) so that brokerWallet.Sign and borrowerWallet.SignAsLoanCounterparty each receive their own dictionary instance and cannot interfere with one another.Xrpl/Models/Transactions/LoanBrokerSet.cs-137-138 (1)
137-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidation blocks valid LoanBrokerSet modify flow
ValidateLoanBrokerSetcurrently requiresVaultIDfor all cases. Per this model’s own contract, updates can be done viaLoanBrokerIDwithout creating a new broker. This can reject valid transactions.Proposed fix
- if (!tx.TryGetValue("VaultID", out var vaultId) || vaultId is not string) - throw new ValidationException("LoanBrokerSet: missing field VaultID"); + var hasVaultId = tx.TryGetValue("VaultID", out var vaultId); + var hasLoanBrokerId = tx.TryGetValue("LoanBrokerID", out var loanBrokerId); + + if (!hasVaultId && !hasLoanBrokerId) + throw new ValidationException("LoanBrokerSet: missing required field VaultID or LoanBrokerID"); + + if (hasVaultId && vaultId is not string) + throw new ValidationException("LoanBrokerSet: invalid field VaultID"); + + if (hasLoanBrokerId && loanBrokerId is not string) + throw new ValidationException("LoanBrokerSet: invalid field LoanBrokerID");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanBrokerSet.cs` around lines 137 - 138, ValidateLoanBrokerSet currently unconditionally requires VaultID which blocks valid update transactions that reference an existing broker by LoanBrokerID; change the validation in ValidateLoanBrokerSet to accept either a VaultID (string) for new broker creation or a LoanBrokerID (string/appropriate type) for updates: use tx.TryGetValue checks for both "VaultID" and "LoanBrokerID", validate the present one(s) for correct type, and only throw a ValidationException if neither field is present or if the provided field has the wrong type, updating the exception message to indicate that either VaultID or LoanBrokerID is required.Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs-232-237 (1)
232-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSingle fixed delay before
txlookup is flakyA hardcoded 5-second wait plus one lookup can intermittently fail under slower ledger close or queue timing.
Proposed fix
- // Wait for ledger to close and get the result via tx lookup + // Poll tx lookup until metadata is available (bounded retries) string txHash = global::Xrpl.Utils.Hashes.HashLedger.HashSignedTx(txBlob); - await Task.Delay(5000); - - TxRequest txReq = new TxRequest(txHash); - TransactionResponse txResponse = await client.Tx(txReq); + TxRequest txReq = new TxRequest(txHash); + TransactionResponse txResponse = null; + for (int i = 0; i < 15; i++) + { + txResponse = await client.Tx(txReq); + if (txResponse?.Meta != null) break; + await Task.Delay(1000); + } + if (txResponse?.Meta == null) + throw new RippleException($"LoanSet tx not validated in time: {txHash}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs` around lines 232 - 237, The single fixed Task.Delay(5000) before doing the tx lookup is flaky; replace it with a polling loop that repeatedly calls client.Tx(new TxRequest(txHash)) (use txHash from global::Xrpl.Utils.Hashes.HashLedger.HashSignedTx(txBlob)) with a short await Task.Delay(pollInterval) between attempts until a valid TransactionResponse is returned (e.g., response != null and contains the expected tx/validated fields) or until a configurable timeout is reached; ensure you surface a clear failure when the timeout elapses and avoid a single one-shot lookup.Xrpl/Models/Transactions/XChainAddAccountCreateAttestation.cs-192-220 (1)
192-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing type/range checks for critical attestation fields
XChainAccountCreateCountandWasLockingChainSendare only null-checked. Invalid values (non-numeric count, chain flag other than 0/1) currently pass validation.Proposed fix
- if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || count is null) - throw new ValidationException("XChainAddAccountCreateAttestation: missing field XChainAccountCreateCount"); + if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || + count is not string countStr || + !ulong.TryParse(countStr, out _)) + throw new ValidationException("XChainAddAccountCreateAttestation: invalid field XChainAccountCreateCount"); ... - if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || wasLocking is null) - throw new ValidationException("XChainAddAccountCreateAttestation: missing field WasLockingChainSend"); + if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || + !byte.TryParse(wasLocking?.ToString(), out var wasLockingByte) || + (wasLockingByte != 0 && wasLockingByte != 1)) + throw new ValidationException("XChainAddAccountCreateAttestation: invalid field WasLockingChainSend");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/XChainAddAccountCreateAttestation.cs` around lines 192 - 220, The current validation in XChainAddAccountCreateAttestation only null-checks XChainAccountCreateCount and WasLockingChainSend allowing non-numeric or out-of-range values; update the validation in the parsing block to (1) ensure XChainAccountCreateCount is present and can be parsed to an integer (use int.TryParse or convert from JSON number) and enforce a sensible range (e.g., >= 0), throwing ValidationException with the same context if it fails, and (2) ensure WasLockingChainSend is present and is either a boolean or a numeric 0/1 (accept boolean true/false or parse int 0/1) and normalize it to a bool, throwing ValidationException if it cannot be interpreted; adjust any downstream uses of the parsed values in the XChainAddAccountCreateAttestation processing to use the validated/converted int and bool variables.
🧹 Nitpick comments (1)
Tests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.cs (1)
49-55: ⚡ Quick winGuard amendment-disabled environments to reduce integration flakiness.
This assertion is strict for environments where
LedgerStateFixis unavailable. Consider treating amendment-disabled outcomes asAssert.Inconclusive(...)so the test signals environment mismatch instead of functional failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.cs` around lines 49 - 55, The test's strict assertion on result.EngineResult should be relaxed to treat amendment-disabled environments as inconclusive: before the Assert.IsTrue on txResult, detect amendment-disabled outcomes (either by checking for the node-specific engine result code that indicates the LedgerStateFix amendment is disabled or by calling an existing helper like IsAmendmentDisabledOutcome(txResult) / an env flag) and call Assert.Inconclusive("LedgerStateFix amendment disabled in this environment") when detected; otherwise proceed with the existing check in TestILedgerStateFix using the txResult variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f904a137-bead-428c-b4ae-7a3cda192082
📒 Files selected for processing (73)
.ci-config/rippled.cfg.gitignoreBase/Xrpl.BinaryCodec/Types/NumberType.csBase/Xrpl.BinaryCodec/Types/XChainBridgeType.csCHANGES.mdDocFx/LendingProtocol-Guide.mdDocFx/LendingProtocol-Guide.ru.mdDocFx/Vault-Guide.mdDocFx/Vault-Guide.ru.mdDocFx/XChainBridge-Guide.mdDocFx/XChainBridge-Guide.ru.mdDocFx/toc.ymlTests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.csTests/Xrpl.BinaryCodec.Test/Types/TestNumberType.csTests/Xrpl.BinaryCodec.Test/Types/TestXChainBridgeType.csTests/Xrpl.Tests/Integration/transactions/TestIBatch.csTests/Xrpl.Tests/Integration/transactions/TestIDelegateSet.csTests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.csTests/Xrpl.Tests/Integration/transactions/TestILoan.csTests/Xrpl.Tests/Integration/transactions/TestILoanBase.csTests/Xrpl.Tests/Integration/transactions/TestIVault.csTests/Xrpl.Tests/Integration/transactions/TestIVaultBase.csTests/Xrpl.Tests/Integration/transactions/TestIXChainBridge.csTests/Xrpl.Tests/Integration/transactions/TestIXChainBridgeBase.csXrpl/Client/Exceptions/XrplErrorClassifier.csXrpl/Client/Json/Converters/LedgerObjectConverter.csXrpl/Client/Json/Converters/TransactionRequestConverter.csXrpl/Client/Json/Converters/TransactionResponseConverter.csXrpl/Client/RequestManager.csXrpl/Models/Common/PermissionModel.csXrpl/Models/Common/XChainAttestations.csXrpl/Models/Common/XChainBridgeModel.csXrpl/Models/Enums/LedgerEntryType.csXrpl/Models/Enums/TransactionType.csXrpl/Models/Ledger/LOBridge.csXrpl/Models/Ledger/LODelegate.csXrpl/Models/Ledger/LOLoan.csXrpl/Models/Ledger/LOLoanBroker.csXrpl/Models/Ledger/LOVault.csXrpl/Models/Ledger/LOXChainOwnedClaimID.csXrpl/Models/Ledger/LOXChainOwnedCreateAccountClaimID.csXrpl/Models/Methods/AccountObjects.csXrpl/Models/Methods/AccountOffers.csXrpl/Models/Transactions/DelegateSet.csXrpl/Models/Transactions/LedgerStateFix.csXrpl/Models/Transactions/LoanBrokerCoverClawback.csXrpl/Models/Transactions/LoanBrokerCoverDeposit.csXrpl/Models/Transactions/LoanBrokerCoverWithdraw.csXrpl/Models/Transactions/LoanBrokerDelete.csXrpl/Models/Transactions/LoanBrokerSet.csXrpl/Models/Transactions/LoanDelete.csXrpl/Models/Transactions/LoanManage.csXrpl/Models/Transactions/LoanPay.csXrpl/Models/Transactions/LoanSet.csXrpl/Models/Transactions/TxFormat.csXrpl/Models/Transactions/Validation.csXrpl/Models/Transactions/VaultClawback.csXrpl/Models/Transactions/VaultCreate.csXrpl/Models/Transactions/VaultDelete.csXrpl/Models/Transactions/VaultDeposit.csXrpl/Models/Transactions/VaultSet.csXrpl/Models/Transactions/VaultWithdraw.csXrpl/Models/Transactions/XChainAccountCreateCommit.csXrpl/Models/Transactions/XChainAddAccountCreateAttestation.csXrpl/Models/Transactions/XChainAddClaimAttestation.csXrpl/Models/Transactions/XChainClaim.csXrpl/Models/Transactions/XChainCommit.csXrpl/Models/Transactions/XChainCreateBridge.csXrpl/Models/Transactions/XChainCreateClaimID.csXrpl/Models/Transactions/XChainModifyBridge.csXrpl/Sugar/Autofill.csXrpl/Wallet/LoanSigningHelper.csXrpl/Wallet/XrplWallet.cs
Collaborator
Author
ValidateSubmitResult (строка 31) — проверяет результат submit, где terQUEUED валиден (транзакция поставлена в очередь)
|
- XChainBridgeType: add 20-byte AccountID length validation in FromParser - NumberType: replace decimal with BigInteger to prevent OverflowException - RequestManager: include ErrorException in error detection gate - TestLoanSetEncode: replace Console.WriteLine with proper assertions - VaultWithdraw/VaultClawback: add missing CurrencyConverter attributes - XrplWallet: validate SigningPubKey before counterparty signing - LoanSigningHelper: verify SigningPubKey match and guard against overwrite - TestILoanBase: isolate mutable dicts in V2 signing, replace delay with polling - Autofill: use IsReserveFeeTxNeed helper for fee cap bypass - LendingProtocol docs: remove contradictory fee advice - TxFormat: add CounterpartySignature as Optional for LoanSet - LoanManage: add tfLoanImpair/tfLoanUnimpair mutual exclusion validation - LoanPay: add tfLoanOverpayment/tfLoanFullPayment/tfLoanLatePayment exclusion - DelegateSet: full Permissions validation (shape, max 10, non-delegable, uniqueness) - LoanBrokerCoverClawback: at-least-one of LoanBrokerID/Amount validation
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Base/Xrpl.BinaryCodec/Types/NumberType.cs`:
- Around line 15-16: The XML comment on NumberType.cs describing mantissa
normalization does not match the implemented constants (the large literal
1000000000000000000 and long.MaxValue) — update the doc comments in the
NumberType class (both the header comment and the later doc block around the
other occurrence) to state that for non-zero values the mantissa is normalized
to [10^18, long.MaxValue] and that zero is represented as mantissa=0,
exponent=Int32.MinValue, so the prose matches the numeric constants in the
implementation.
- Around line 31-35: Validate mantissa/exponent invariants early in NumberType
decoding/creation: in the NumberType constructor (or FromParser) enforce a sane
exponent bound (use a constant like MAX_EXPONENT_ABS and throw on |exponent| >
MAX_EXPONENT_ABS to prevent unbounded BigInteger.Pow(10, exponent) work) and
enforce the zero-representation invariant (if Mantissa == 0 then Exponent must
be 0; if Mantissa != 0 then Exponent must not be 0), throwing a clear exception
on violation so ToJson and other code (e.g., BigInteger.Pow usage) never see
malformed or expensive states.
- Around line 124-127: NumberType.FromString currently uses decimal.Parse which
overflows for XRPL-extents (exponents ±32768); replace the decimal-based parse
with manual parsing of the string (handle sign, optional decimal point, and
scientific "e/E" exponent), convert the integer mantissa into a BigInteger and
compute the combined exponent as an int, then apply the existing
mantissa/exponent normalization logic (matching the approach used by ToJson and
BigInteger in this class) so values like "1e-32000" are accepted; ensure you
reference and reuse the same normalization code paths in NumberType
(mantissa/exponent normalization) and keep the exponent bounds consistent with
the XRPL comment in ToJson.
In `@DocFx/LendingProtocol-Guide.md`:
- Around line 26-34: The fenced diagram block in LendingProtocol-Guide.md is
missing a language tag and triggers markdownlint MD040; update the
triple-backtick that opens the ASCII diagram (the LoanSet/LoanBroker/Vault
diagram) to include a language tag such as "text" (e.g., change ``` to ```text)
so the block is explicitly marked as plain text while leaving the diagram
content unchanged.
- Around line 295-301: The current code creates one mutable dictionary
(brokerDict) and uses it for both brokerWallet.Sign(...) and
borrowerWallet.SignAsLoanCounterparty(...), risking the second signature being
derived from a mutated payload; to fix, produce isolated JSON dictionaries per
signer (e.g., call JsonSerializer.Deserialize(...) into two separate
Dictionary<string, object> instances or deep-clone brokerDict) and pass the
independent dictionary to brokerWallet.Sign(...) and
borrowerWallet.SignAsLoanCounterparty(...) respectively so each signer operates
on an unmodified payload.
In `@DocFx/LendingProtocol-Guide.ru.md`:
- Around line 332-333: В документе есть противоречие по комиссии после
использования Autofill: строка с утверждением "Комиссию необходимо увеличить
после автозаполнения" конфликтует со строкой "`Autofill` автоматически
рассчитывает корректную комиссию для LoanSet (включая overhead
CounterpartySignature)". Уберите одно из утверждений и оставьте согласованный
вариант: либо удалите строку про необходимость вручную увеличивать комиссию,
либо замените вторую строку на пояснение, что Autofill не учитывает overhead
CounterpartySignature и поэтому требуется дополнительная корректировка; при
правке сориентируйтесь по терминам "Autofill", "CounterpartySignature" и
"LoanSet" чтобы сохранить точную техническую формулировку.
- Around line 26-34: The fenced code block containing the ASCII diagram in
LendingProtocol-Guide.ru.md is missing a language tag (triggering markdownlint
MD040); add a language identifier such as "text" to the opening backticks of
that diagram block so it becomes ```text ... ```, preserving the exact diagram
content and spacing to avoid formatting changes.
- Around line 295-301: The code currently deserializes prepared.ToJsonString()
into a single Dictionary<string, object> named brokerDict and reuses it for both
brokerWallet.Sign(brokerDict) and
borrowerWallet.SignAsLoanCounterparty(brokerDict); if either signing method
mutates the dictionary the second signature will be computed against a mutated
payload. Fix by creating separate dictionaries for each signer (e.g.,
deserialize prepared.ToJsonString() twice or deep-clone brokerDict into a
distinct counterpartyDict) and pass brokerDict to brokerWallet.Sign and the
separate counterpartyDict to borrowerWallet.SignAsLoanCounterparty so each
signer operates on an independent payload.
In `@Xrpl/Models/Transactions/DelegateSet.cs`:
- Around line 116-122: The IList<object> branch in DelegateSet currently only
checks counts; update it to mirror the JsonElement branch by iterating the list
entries in the IList<object> path inside the DelegateSet validation/constructor,
verifying each entry is a Permission (or has the expected shape), ensuring
PermissionValue exists and is non-empty, rejecting any PermissionValue that
represents a non-delegable transaction type, and detecting/throwing on duplicate
PermissionValue entries; use the same ValidationException messages and the same
MaxPermissions/duplicate logic as the JsonElement handling to keep validation
consistent.
In `@Xrpl/Models/Transactions/LoanManage.cs`:
- Around line 101-109: The current check in LoanManage.cs only forbids
tfLoanImpair and tfLoanUnimpair together but does not require exactly one action
flag; update the validation after ParseFlags to build an action mask using the
LoanManageFlags values (e.g., tfLoanImpair, tfLoanUnimpair, tfLoanDefault —
include all action-related enum members), compute how many of those bits are set
on rawFlags, and throw a ValidationException from the same block if the count is
not exactly 1 (message like "LoanManage: exactly one action flag required").
Make the change in the same flags-parsing block where ParseFlags and rawFlags
are used so the validation runs for missing or multiple action flags.
- Around line 112-121: The ParseFlags method currently swallows invalid input
and returns 0, which masks malformed Flags payloads; update ParseFlags to
validate inputs and throw a descriptive exception (e.g., FormatException or
JsonException) when the input cannot be parsed instead of returning 0. Locate
the ParseFlags method and replace the final fallback case with throwing an
exception that includes the original flagsObj (or its type/value) and a clear
message; ensure callers of ParseFlags (e.g., any deserialization or constructor
logic that uses ParseFlags) handle or propagate this exception appropriately so
malformed payloads are detected rather than silently coerced.
- Around line 98-99: The current LoanManage parsing checks for presence and type
of "LoanID" but allows empty or whitespace strings; update the validation in the
LoanManage parsing logic (where tx.TryGetValue("LoanID", out var id) is used) to
also reject empty or whitespace values by verifying id is a non-empty string
(e.g., cast to string and use string.IsNullOrWhiteSpace) and throw the same
ValidationException("LoanManage: missing field LoanID") when the value is
null/empty/whitespace.
In `@Xrpl/Models/Transactions/VaultWithdraw.cs`:
- Around line 94-95: VaultID is only checked for being a CLR string in the
VaultWithdraw validation, but malformed non-Hash256 values slip through; after
the existing tx.TryGetValue("VaultID", out var vaultId) and string check in
VaultWithdraw.cs, validate that vaultId is a valid Hash256 (use the project's
Hash256 parsing/validation helper such as Hash256.TryParse/Parse or a regex that
enforces 64 hex chars), and if it fails throw a ValidationException with a clear
message (e.g., "VaultWithdraw: VaultID must be a valid Hash256"); update any
tests that assume the stricter validation if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1adeb0b7-f1a3-4cef-8e2b-d9e08cd40142
📒 Files selected for processing (17)
Base/Xrpl.BinaryCodec/Types/NumberType.csBase/Xrpl.BinaryCodec/Types/XChainBridgeType.csDocFx/LendingProtocol-Guide.mdDocFx/LendingProtocol-Guide.ru.mdTests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.csTests/Xrpl.Tests/Integration/transactions/TestILoanBase.csXrpl/Client/RequestManager.csXrpl/Models/Transactions/DelegateSet.csXrpl/Models/Transactions/LoanBrokerCoverClawback.csXrpl/Models/Transactions/LoanManage.csXrpl/Models/Transactions/LoanPay.csXrpl/Models/Transactions/TxFormat.csXrpl/Models/Transactions/VaultClawback.csXrpl/Models/Transactions/VaultWithdraw.csXrpl/Sugar/Autofill.csXrpl/Wallet/LoanSigningHelper.csXrpl/Wallet/XrplWallet.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- Xrpl/Client/RequestManager.cs
- Xrpl/Models/Transactions/LoanBrokerCoverClawback.cs
- Xrpl/Sugar/Autofill.cs
- Xrpl/Models/Transactions/VaultClawback.cs
- Xrpl/Wallet/XrplWallet.cs
- Xrpl/Wallet/LoanSigningHelper.cs
- Xrpl/Models/Transactions/LoanPay.cs
- Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs
- Xrpl/Models/Transactions/TxFormat.cs
Rewrite Issue.cs to support MPT binary format matching rippled's STIssue serialization: account(20) + noAccount(20) + sequence(4). Add Hash192 type for 24-byte MPTokenIssuanceID. Add MptIssuanceId to IssuedCurrency model and IssuedCurrencyConverter. Integration tests: - Vault: TestVaultClawback_MPT (MPT-backed vault lifecycle) - Loan: TestLoanBrokerSet_MPT, TestLoanSet_MPT, TestLoanPay_MPT - AMM: TestAMMCreate_MptXrpPool_RejectedByProtocol (XLS-62 pending) Unit tests: TestIssueMpt (encode/decode roundtrip, binary match)
NumberType.cs: - Fix doc comments: mantissa range is [10^18, long.MaxValue], not [10^15, 10^16-1] - Add exponent/mantissa invariant validation in FromParser - Replace decimal.Parse with BigInteger-based manual parsing to handle full XRPL exponent range (±32768) without overflow LendingProtocol-Guide.md / .ru.md: - Add ```text language tag to ASCII diagram blocks (MD040) - Fix V2 signing example: use separate dicts per signer to avoid mutation of shared payload - Remove contradictory fee statement in RU guide (Autofill handles it) DelegateSet.cs: - Add full validation in IList<object> branch matching JsonElement path (entry shape, PermissionValue, non-delegable types, duplicates) LoanManage.cs: - Require exactly one action flag (tfLoanDefault|tfLoanImpair|tfLoanUnimpair) - ParseFlags now throws FormatException instead of silently returning 0 - Reject empty/whitespace LoanID strings VaultWithdraw.cs: - Validate VaultID as Hash256 (64 hex chars)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Xrpl/Models/Transactions/LoanManage.cs (1)
109-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Flagsis still optional in validation despite “exactly one required”.Line 110 only checks action bits when
Flagsis present. A missingFlagsfield currently passes, which violates the stated rule.Proposed fix
-// Exactly one action flag is required; they are mutually exclusive -if (tx.TryGetValue("Flags", out var flagsObj) && flagsObj is not null) -{ - uint rawFlags = ParseFlags(flagsObj); - uint actionBits = rawFlags & LoanManageActionMask; - - // Count set action bits — must be exactly 1 - int actionCount = System.Numerics.BitOperations.PopCount(actionBits); - if (actionCount != 1) - throw new ValidationException("LoanManage: exactly one action flag required (tfLoanDefault, tfLoanImpair, or tfLoanUnimpair)"); -} +if (!tx.TryGetValue("Flags", out var flagsObj) || flagsObj is null) + throw new ValidationException("LoanManage: exactly one action flag required (tfLoanDefault, tfLoanImpair, or tfLoanUnimpair)"); + +uint rawFlags = ParseFlags(flagsObj); +uint actionBits = rawFlags & LoanManageActionMask; +int actionCount = System.Numerics.BitOperations.PopCount(actionBits); +if (actionCount != 1) + throw new ValidationException("LoanManage: exactly one action flag required (tfLoanDefault, tfLoanImpair, or tfLoanUnimpair)");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Transactions/LoanManage.cs` around lines 109 - 119, The validation currently only enforces the "exactly one action flag" rule when Flags exists, allowing missing Flags to pass; update the LoanManage validation to require Flags and enforce exactly one action bit by treating absent Flags as invalid: in the method performing this check (look for LoanManageActionMask, ParseFlags, and the ValidationException thrown), if tx does not contain "Flags" or Flags is null, throw a ValidationException with the same message, otherwise ParseFlags(tx["Flags"]), compute actionBits = rawFlags & LoanManageActionMask, and keep the PopCount check to ensure exactly one flag is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Base/Xrpl.BinaryCodec/Types/Issue.cs`:
- Around line 193-200: The early-return when curr.IsNative returns an Issue
after reading only 20 bytes can leave extra payload bytes unread and desync the
parser; in the method that decodes Issue (the code around curr.IsNative and the
Issue(...) constructor), check the provided hint or the remaining field length
before returning for native XRP: verify the field payload length equals 20 (or
that hint indicates native) and if not, either consume the remaining bytes for
this field or throw a descriptive parse error; ensure parser.Read/Peek APIs are
used to validate length prior to the early return so subsequent decoding remains
aligned.
In `@Xrpl/Models/Common/Common.cs`:
- Around line 64-71: The IsMpt() and ToString() logic treats an empty string
MptIssuanceId as a valid MPT; update both checks to ensure MptIssuanceId is not
null or empty/whitespace (e.g., use string.IsNullOrWhiteSpace or equivalent)
before considering it MPT. Modify IsMpt() to return true only when MptIssuanceId
contains non-whitespace content, and change the ToString() conditional to use
the same non-empty check so it emits "MPT:{MptIssuanceId}" only for valid values
and falls back to Currency.CurrencyReadableName() otherwise.
---
Duplicate comments:
In `@Xrpl/Models/Transactions/LoanManage.cs`:
- Around line 109-119: The validation currently only enforces the "exactly one
action flag" rule when Flags exists, allowing missing Flags to pass; update the
LoanManage validation to require Flags and enforce exactly one action bit by
treating absent Flags as invalid: in the method performing this check (look for
LoanManageActionMask, ParseFlags, and the ValidationException thrown), if tx
does not contain "Flags" or Flags is null, throw a ValidationException with the
same message, otherwise ParseFlags(tx["Flags"]), compute actionBits = rawFlags &
LoanManageActionMask, and keep the PopCount check to ensure exactly one flag is
set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e5f7efb-fb1c-490c-a124-1eed29583668
📒 Files selected for processing (15)
.ci-config/rippled.cfgBase/Xrpl.BinaryCodec/Types/Issue.csBase/Xrpl.BinaryCodec/Types/NumberType.csDocFx/LendingProtocol-Guide.mdDocFx/LendingProtocol-Guide.ru.mdTests/Xrpl.BinaryCodec.Test/Types/TestIssueMpt.csTests/Xrpl.Tests/Integration/transactions/TestIAMMCreateMpt.csTests/Xrpl.Tests/Integration/transactions/TestILoan.csTests/Xrpl.Tests/Integration/transactions/TestILoanBase.csTests/Xrpl.Tests/Integration/transactions/TestIVault.csXrpl/Client/Json/Converters/CurrencyConverter.csXrpl/Models/Common/Common.csXrpl/Models/Transactions/DelegateSet.csXrpl/Models/Transactions/LoanManage.csXrpl/Models/Transactions/VaultWithdraw.cs
✅ Files skipped from review due to trivial changes (2)
- .ci-config/rippled.cfg
- DocFx/LendingProtocol-Guide.ru.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Xrpl/Models/Transactions/DelegateSet.cs
- Tests/Xrpl.Tests/Integration/transactions/TestIVault.cs
Comment on lines
193
to
+200
| if (curr.IsNative) | ||
| { | ||
| // XRP case | ||
| // XRP: just 20 zero bytes | ||
| return new Issue(curr, null); | ||
| } | ||
| // IOU case: read 20-byte issuer | ||
| var issuer = AccountId.FromParser(parser); | ||
|
|
||
| // Non-XRP: read 20 more bytes (issuer for IOU, or noAccount marker for MPT) | ||
| byte[] secondBytes = parser.Read(20); |
There was a problem hiding this comment.
Guard parser consumption with hint (or strict length checks) to prevent stream desync.
Line 193 can return as XRP after 20 bytes even when the field payload is longer, leaving unread bytes and shifting subsequent decode state on malformed inputs. Enforce expected length before early-return.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Base/Xrpl.BinaryCodec/Types/Issue.cs` around lines 193 - 200, The
early-return when curr.IsNative returns an Issue after reading only 20 bytes can
leave extra payload bytes unread and desync the parser; in the method that
decodes Issue (the code around curr.IsNative and the Issue(...) constructor),
check the provided hint or the remaining field length before returning for
native XRP: verify the field payload length equals 20 (or that hint indicates
native) and if not, either consume the remaining bytes for this field or throw a
descriptive parse error; ensure parser.Read/Peek APIs are used to validate
length prior to the early return so subsequent decoding remains aligned.
Collaborator
Author
There was a problem hiding this comment.
STIssue — самоописывающийся формат: XRP = 20 нулевых байт, IOU = 40, MPT = 44. Парсер читает ровно столько байт, сколько нужно для каждого варианта. hint для Issue не передаётся — это стандартный подход (аналогично Amount). rippled никогда не отправит XRP Issue с лишними байтами.
There was a problem hiding this comment.
@Platonenkov, thank you for the clear explanation. You're right — STIssue is a self-describing format where the byte count itself determines the variant (XRP = 20, IOU = 40, MPT = 44), so the parser can never desync on well-formed data. The early return after 20 bytes for the native XRP case is entirely correct and consistent with how rippled serializes STIssue. My concern about unread bytes was not applicable here. I'll retract this comment.
✏️ Learnings added
Learnt from: Platonenkov
Repo: StaticBit-io/XrplCSharp PR: 6
File: Base/Xrpl.BinaryCodec/Types/Issue.cs:193-200
Timestamp: 2026-05-13T14:04:05.706Z
Learning: In `Base/Xrpl.BinaryCodec/Types/Issue.cs`, `STIssue` (the `Issue` type) uses a self-describing binary format: XRP = 20 bytes (all zeros), IOU = 40 bytes (20 currency + 20 issuer), MPT = 44 bytes (20 account + 20 noAccount marker + 4 sequence). The parser reads exactly as many bytes as required for each variant. The `hint` parameter is not passed for `Issue`, consistent with the same approach used for `Amount` in rippled. There is no parser desync risk from the early XRP return — rippled never sends an XRP Issue with extra bytes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- IssuedCurrency.IsMpt()/ToString(): use IsNullOrWhiteSpace instead of null check to reject empty/whitespace MptIssuanceId - LoanManage validation: Flags is now mandatory (exactly one action flag required), not just validated when present
Fix validationReceived handler bug (was calling OnManifestReceived). Add 3 missing streams: manifestReceived, bookChanges, serverStatus. Add 5 RPC methods: vault_info, channel_authorize, channel_verify, server_definitions, transaction_entry. Remove dead code ValidationsStream.cs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Xrpl/Models/Subscriptions/BookChangesStream.cs (1)
47-48: ⚡ Quick winInitialize
Changesto an empty list by default.This avoids null-handling in downstream event consumers when a payload omits
changes.♻️ Proposed fix
[JsonPropertyName("changes")] -public List<BookChange> Changes { get; set; } +public List<BookChange> Changes { get; set; } = new();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Xrpl/Models/Subscriptions/BookChangesStream.cs` around lines 47 - 48, The Changes property on the BookChangesStream class may be null if the payload omits it; initialize it to an empty list by default to avoid downstream null checks — update the property declaration for BookChangesStream (Changes) to instantiate a new List<BookChange> as its default value so consumers always receive an empty collection instead of null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/TestsClients/Test.ClonsoleApp/Program.cs`:
- Around line 420-423: InitTestData calls TryFillAccounts before the static
client field is assigned in Main, so TryFillAccounts (and methods it calls like
client.GetXrpFreeBalance) may dereference a null client; fix by ensuring client
is initialized before any call to TryFillAccounts (for example move the client
assignment in Main to occur before InitTestData is invoked or add a guard that
initializes/throws with a clear message inside TryFillAccounts), and/or change
TryFillAccounts to accept an IXrplClient parameter instead of using the static
client to avoid relying on uninitialized static state.
---
Nitpick comments:
In `@Xrpl/Models/Subscriptions/BookChangesStream.cs`:
- Around line 47-48: The Changes property on the BookChangesStream class may be
null if the payload omits it; initialize it to an empty list by default to avoid
downstream null checks — update the property declaration for BookChangesStream
(Changes) to instantiate a new List<BookChange> as its default value so
consumers always receive an empty collection instead of null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d825d214-acef-41d9-8871-84d2d176b314
📒 Files selected for processing (17)
Tests/TestsClients/Test.ClonsoleApp/Program.csTests/Xrpl.Tests/Client/TestSubscribe.csTests/Xrpl.Tests/Integration/transactions/TestIVault.csTests/Xrpl.Tests/Sugar/TestUAutofillFees.csXrpl/Client/IXrplClient.csXrpl/Client/connection.csXrpl/Models/Common/Common.csXrpl/Models/Methods/ChannelAuthorize.csXrpl/Models/Methods/ChannelVerify.csXrpl/Models/Methods/ServerDefinitions.csXrpl/Models/Methods/TransactionEntry.csXrpl/Models/Methods/VaultInfo.csXrpl/Models/Subscriptions/BookChangesStream.csXrpl/Models/Subscriptions/ManifestStream.csXrpl/Models/Subscriptions/ResponseStreamType.csXrpl/Models/Subscriptions/ServerStatusStream.csXrpl/Models/Subscriptions/ValidationsStream.cs
💤 Files with no reviewable changes (1)
- Xrpl/Models/Subscriptions/ValidationsStream.cs
✅ Files skipped from review due to trivial changes (2)
- Tests/Xrpl.Tests/Sugar/TestUAutofillFees.cs
- Xrpl/Models/Subscriptions/ServerStatusStream.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Xrpl/Models/Common/Common.cs
TryFillAccounts was using static client field before it was assigned. Initialize BookChangesStream.Changes to empty list to avoid null.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Xrpl/Models/Subscriptions/BookChangesStream.cs`:
- Around line 47-48: The Changes auto-property on BookChangesStream can be set
to null by System.Text.Json when payloads contain "changes": null; replace the
auto-property with a private backing field (e.g., _changes) and implement the
public property Changes with get => _changes; set => _changes = value ?? new
List<BookChange>(); so deserialization will coalesce null into an empty
List<BookChange> and avoid NREs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b0d6381-2b41-45b8-a0d7-841e97480ffc
📒 Files selected for processing (2)
Tests/TestsClients/Test.ClonsoleApp/Program.csXrpl/Models/Subscriptions/BookChangesStream.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/TestsClients/Test.ClonsoleApp/Program.cs
System.Text.Json sets null on "changes": null, overriding initializer.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
Xrpl/Models/Transactions/Xrpl/Models/Ledger/LO*.csXChainBridgeModel,XChainAttestations,PermissionModelTransactionRequest/Response/LOConverterTransactionType(+25),LedgerEntryType(+2)NumberType,XChainBridgeTypeLoanSigningHelper,XrplWalletTestI*.csDocFx/*.mdTest plan
dotnet buildpasses without errorsdotnet test --filter "TestU")dotnet test --filter "TestI")🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores