Release v10.4 to NuGet#8
Merged
Merged
Conversation
- Improved null checks and error handling in Amount.FromJson and Uint64.FromJson methods. - Strengthened validation for XRP Amount objects and Uint64 JSON tokens. - Updated Oracle currency converters for better handling of non-standard currency codes. - Enhanced unit tests for currency encoding and decoding to ensure robust exception handling.
…ol (#5) * update definition.json file * Enhance EngineResult, Field, FieldType, LedgerEntryType, and TransactionType enums with new values and improved organization - Added multiple new EngineResult values for better error handling. - Introduced new Uint8Field and Uint16Field values in Field.cs for enhanced functionality. - Expanded FieldType with additional types including Number, Int32, and Int64. - Updated LedgerEntryType with new entries and deprecated legacy types. - Added new TransactionType values for XChain and Vault operations, along with loan-related transactions. * Update Visual Studio solution to version 18 and add GenerateEnums tool - Updated the solution file to target Visual Studio version 18. - Added a new project for GenerateEnums tool to generate C# enums from definitions.json. - Implemented the main functionality for reading definitions and generating enum source files for EngineResult, TransactionType, and LedgerEntryType. * Refactor enum generation for EngineResult, LedgerEntryType, and TransactionType - Converted EngineResult, LedgerEntryType, and TransactionType to partial classes for better organization. - Generated new files for EngineResult.Generated.cs, LedgerEntryType.Generated.cs, and TransactionType.Generated.cs containing only static readonly field declarations. - Updated the GenerateEnums tool to reflect changes in enum structure and generation process. * update changes * Add generated enum files for various field types in Xrpl.BinaryCodec - Introduced new generated files for AccountIdField, AmountField, BlobField, CurrencyField, Hash128Field, Hash160Field, Hash192Field, Hash256Field, Int32Field, IssueField, NumberField, PathSetField, StArrayField, StObjectField, Uint8Field, Uint16Field, Uint32Field, and Uint64Field. - Each file contains static readonly field declarations for respective field types, enhancing the enum structure and organization. - These changes were made to support the updated definitions from definitions.json and improve the overall codebase structure. * Remove unused folder declaration from Xrpl.BinaryCodec.Test project file * Refactor and enhance unit tests for binary codec functionality - Updated TestBinaryParser to improve method assertions and added new tests for reading variable-length data. - Introduced TestBinarySerializer with methods for writing length-encoded data and validating serialized output. - Expanded TestUDefinitions with comprehensive tests for field properties and mappings. - Enhanced TestFieldIDCodec to validate field ID encoding and decoding. - Improved TestAmount with additional tests for XRP and IOU handling, ensuring robust serialization and deserialization processes. - Cleaned up TestMain by removing commented-out code and improving test structure. * Refactor Field and AmountValue classes for improved validation and dispatch handling - Updated Field class to use private backing fields for FromJson and FromParser properties, enhancing encapsulation and ensuring proper dispatch. - Introduced a maximum limit for native XRP amounts in AmountValue, with validation to prevent exceeding this limit. - Enhanced Uint32's FromJson method to handle various JSON value types more robustly. - Improved unit tests to replace deprecated assertions with more descriptive methods, ensuring clarity in test outcomes. * Update auto-generated file comments to clarify usage instructions - Modified comments in multiple auto-generated files to specify that they should not be edited manually and to provide the command for regenerating them using the GenerateEnums tool. - This change enhances clarity for developers working with generated files, ensuring proper usage and maintenance. * Refactor Field class and enhance error handling in StObject - Updated the Field class to use a private backing field for Values, improving encapsulation and lazy initialization. - Enhanced error handling in StObject by adding an exception for unregistered FromJson/FromParser dispatches. - Improved Uint32's FromJson method to include range validation for long values and ensure proper parsing with invariant culture. - Expanded unit tests to cover additional cases and improve assertions for field definitions and transaction types. * Enhance Field class documentation and improve error handling in GenerateEnums tool - Added comments in the Field class to clarify the thread-safety and idempotency of the FromJson and FromParser properties. - Improved error handling in the GenerateEnums tool by adding checks for missing properties in the definitions.json file, ensuring a more robust file structure validation.
* feat: add 25 new XRPL transaction types, 7 ledger objects, and integration 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 * fix: add missing CurrencyConverter attributes to transaction models 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. * fix: VL-encode AccountID in XChainBridgeType and skip unavailable tests 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). * fix: improve error handling, XChainBridge tests, and add bridge documentation - 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 * chore: add .claude/settings.local.json to .gitignore * fix: rename DelegateSet.Delegate to Authorize and add LODelegate verification - 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 * feat: add LedgerStateFix to Autofill reserve fee logic 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. * fix: rewrite NumberType to 12-byte format and fix Loan integration tests 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) * docs: add Lending Protocol guide and update CHANGES.md 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 * feat: add LoanSigningHelper with V1/V2/V3 multi-party signing patterns 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. * refactor: move LoanSet fee calculation from LoanSigningHelper to Autofill Fee for CounterpartySignature overhead is now computed in CalculateBaseFeeForType (baseFee * 2) alongside EscrowFinish and Batch fee logic. Removed adjustFee parameter from PrepareForSigning. * fix: enable VaultCreate_WithIOU and VaultClawback tests - 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 * fix: audit Validate methods, fix WasLockingChainSend type, add Vault 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) * fix: address CodeRabbitAI review findings on PR #6 - 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 * feat: add MPT support in BinaryCodec STIssue and integration tests 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) * fix: address code review findings from CodeRabbitAI 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) * fix: IsMpt empty string check, LoanManage require Flags - 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 * feat: add missing stream subscriptions and RPC methods 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. * fix: pass client parameter to TryFillAccounts, init Changes list TryFillAccounts was using static client field before it was assigned. Initialize BookChangesStream.Changes to empty list to avoid null. * fix: null-coalesce Changes setter to prevent NRE from deserialization System.Text.Json sets null on "changes": null, overriding initializer.
📝 WalkthroughWalkthroughRegenerates binary codec enums/fields and refactors dispatch; adds new XRPL types (Number, Int32, Int64, XChainBridge) and MPT Issue support. Introduces models, validation, and client wiring for XChain Bridge, Vault, Lending Protocol, DelegateSet, and LedgerStateFix. Expands websocket streams and requests. Adds enum generator tool, extensive tests, docs, and config updates. ChangesBinary Codec regeneration and core type extensions
Protocol features: XChain, Vault, Lending, DelegateSet, LedgerStateFix (+ client and converters)
Tests
Enum generator tool
Docs and configuration
Sequence Diagram(s)sequenceDiagram
autonumber
actor Borrower
participant Broker as Broker Wallet
participant Helper as LoanSigningHelper
participant Codec as BinaryCodec
participant Node as XRPL Node
Borrower->>Broker: Share unsigned LoanSet JSON
Broker->>Helper: PrepareForSigning(tx, broker)
Helper-->>Broker: Prepared tx JSON
par Parallel signing (V2)
Broker->>Helper: Broker TxnSignature(preimage)
Borrower->>Codec: EncodeForSigning(prepared tx)
Borrower->>Borrower: Sign CounterpartySignature
end
Helper->>Helper: CombineLoanSignatures(brokerBlob, borrowerBlob)
Helper-->>Broker: Combined signed blob + hash
Broker->>Node: Submit(signed blob)
Node-->>Broker: Result (tes/tec)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ 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.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Xrpl/Client/connection.cs (1)
2528-2531:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard failed stream-type parsing before dispatch.
At Line 2528,
Enum.TryParseresult is ignored. On parse failure, dispatch can fall through using a default enum value and hit the wrong handler.Proposed fix
- Enum.TryParse(value: data.Type.ToString(), result: out ResponseStreamType type); - switch (type) + if (!Enum.TryParse(value: data.Type.ToString(), result: out ResponseStreamType type)) + { + return; + } + switch (type)🤖 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/connection.cs` around lines 2528 - 2531, Enum.TryParse currently ignores its boolean result and may dispatch using a default ResponseStreamType; change the parsing to capture the return value (e.g., bool parsed = Enum.TryParse(value: data.Type.ToString(), out ResponseStreamType type)) and guard on parsed before the switch on ResponseStreamType; if parsed is false, log or handle the unknown stream-type and return/skip dispatch (or throw a clear exception) instead of falling through to handlers like ResponseStreamType.ledgerClosed.
🟠 Major comments (19)
Base/Xrpl.BinaryCodec/Types/NumberType.cs-206-213 (1)
206-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent overflow while parsing scientific exponent digits.
expValue = expValue * 10 + ...onintcan overflow before your cap check executes.💡 Suggested fix
- int expValue = 0; + long expValue = 0; bool hasExpDigits = false; while (pos < str.Length && char.IsDigit(str[pos])) { expValue = expValue * 10 + (str[pos] - '0'); if (expValue > 100000) // sanity cap throw new FormatException($"NumberType: exponent too large in '{str}'"); hasExpDigits = true; pos++; } if (!hasExpDigits) throw new FormatException($"NumberType: no digits in exponent of '{str}'"); - sciExponent = expNegative ? -expValue : expValue; + sciExponent = (int)(expNegative ? -expValue : expValue);🤖 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 206 - 213, The loop in NumberType.Parse (the expValue accumulation using expValue = expValue * 10 + (str[pos] - '0') with the expValue cap) can overflow before the cap check; change the accumulation to guard against overflow by either using a wider type (e.g., long) for the temporary accumulator or checking before multiplication/addition (e.g., if (expValue > (cap - digit) / 10) throw FormatException("exponent too large...")). Update the loop that references expValue and hasExpDigits so the overflow guard runs before performing the multiply/add and still enforces the existing sanity cap and throws the same FormatException when exceeded.Tests/Xrpl.Tests/Integration/transactions/TestIDelegateSet.cs-37-41 (1)
37-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreating
terQUEUEDas success makes the ledger assertions race-prone.At Line 39,
terQUEUEDmeans not yet validated; the test then immediately reads ledger state and can fail nondeterministically. Either requiretesSUCCESShere or poll until final validation before asserting ledger objects.🤖 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/TestIDelegateSet.cs` around lines 37 - 41, The current ValidateResult treats "terQUEUED" as success which is race-prone; change ValidateResult to only accept "tesSUCCESS" (i.e., require res is { Meta: { TransactionResult: "tesSUCCESS" } }) and remove "terQUEUED" from the condition, and/or implement a polling helper (e.g., PollUntilValidated/GetTransaction loop) in the test that retries fetching the transaction until its Meta shows final validation (TransactionResult == "tesSUCCESS" and validated flag present) or a timeout before making ledger state assertions; update calls to ValidateResult or replace its usage so tests wait for final validation rather than treating queued results as success.Tests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.cs-14-14 (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the integration-test classification prefix for filtering.
Line 14 uses
LedgerStateFixwithout theTestIprefix, which can break category-based integration test filtering.Suggested fix
-[TestCategory("LedgerStateFix")] +[TestCategory("TestI_LedgerStateFix")]As per coding guidelines
**/Tests/**/*.cs: Use TestU filter prefix for unit tests and TestI for integration tests in test classification.🤖 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` at line 14, The test category attribute in TestILedgerStateFix.cs uses "LedgerStateFix" without the required integration prefix; update the TestCategory attribute on the test (the [TestCategory("LedgerStateFix")] instance) to include the "TestI" prefix (e.g., "TestI LedgerStateFix") so it matches the integration-test classification filter.Xrpl/Models/Common/Common.cs-44-49 (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
CurrencyinCurrencyName/ToString()to avoid NREs.
Currencycan be null, and bothCurrencyNameand the non-MPT branch ofToString()dereference it directly. This can crash during logging/debug formatting or model inspection paths.Proposed fix
[JsonIgnore] -public string CurrencyName => Currency.CurrencyReadableName(); +public string CurrencyName => Currency?.CurrencyReadableName() ?? string.Empty; public override string ToString() => !string.IsNullOrWhiteSpace(MptIssuanceId) ? $"MPT:{MptIssuanceId}" - : $"{Currency.CurrencyReadableName()}"; + : CurrencyName;Also applies to: 75-77
🤖 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/Common/Common.cs` around lines 44 - 49, The CurrencyName getter and the non-MPT branch of ToString() dereference Currency without null checks and can throw NREs; update CurrencyName (property CurrencyName => Currency?.CurrencyReadableName() ?? string.Empty or similar) and modify ToString() to guard Currency (use null-coalescing or conditional access and fallback values) so all dereferences use Currency? or fallback strings, ensuring logging/inspection won't crash; locate these changes in the Common class members CurrencyName and ToString().Xrpl/Models/Transactions/VaultWithdraw.cs-98-99 (1)
98-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
Amountstructure, not just nullability.Lines 98-99 allow any non-null runtime type for
Amount, so malformed payloads can pass this validator and fail deeper in the request path.🤖 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 98 - 99, The current check only ensures Amount is non-null; instead validate its shape: in VaultWithdraw.cs where tx.TryGetValue("Amount", out var amount) is used, verify amount is the expected JSON type(s) (e.g., a primitive numeric/string for drops or a JObject for issued currencies) and, when it's a JObject, confirm required fields like "currency" and "value" (and "issuer" if applicable) exist and have correct types/format before proceeding; if validation fails, throw the same ValidationException("VaultWithdraw: missing field Amount") or a more specific ValidationException to fail fast and avoid downstream errors.Xrpl/Models/Transactions/XChainCreateClaimID.cs-82-89 (1)
82-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen validation for
SignatureRewardandOtherChainSource.Lines 85-89 only enforce presence/basic type, so invalid payloads (for example, empty
OtherChainSourceor non-amountSignatureReward) can pass local validation and fail later in signing/submission.🤖 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/XChainCreateClaimID.cs` around lines 82 - 89, The current validation in XChainCreateClaimID only checks presence/type for SignatureReward and OtherChainSource, so strengthen it inside the XChainCreateClaimID validation routine: for OtherChainSource (variable source) ensure it is a non-empty, non-whitespace string (throw ValidationException("XChainCreateClaimID: OtherChainSource must be a non-empty string") if invalid), and for SignatureReward (variable reward) validate it represents a proper XRP/amount format (e.g., non-empty string or numeric amount and, if expected, matches the ledger amount pattern or parses via the same Amount parsing routine used elsewhere; throw ValidationException("XChainCreateClaimID: SignatureReward must be a valid amount") on failure). Keep the existing checks for XChainBridge but add these stricter checks to fail early.Base/Xrpl.BinaryCodec/Types/Int64Type.cs-28-31 (1)
28-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against null JSON in
FromJson.Line 30 dereferences
tokenwithout a null check, so malformed/null inputs throwNullReferenceExceptioninstead of a controlled parse exception.Suggested fix
public static Int64Type FromJson(JsonNode token) { + if (token is null) + throw new FormatException("Cannot parse Int64 from null JSON token."); + JsonValueKind kind = token.GetValueKind();🤖 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/Int64Type.cs` around lines 28 - 31, The FromJson(JsonNode token) method currently dereferences token to call token.GetValueKind() and can throw NullReferenceException for null/malformed input; add a null-check at the start of Int64Type.FromJson to validate token is not null (and optionally token.GetValueKind() is expected), and if null throw or return a controlled parse exception (e.g., ArgumentNullException or a custom parse error) with a clear message; update any callers/tests to expect that controlled exception instead of a NullReferenceException.Tests/Xrpl.Tests/Integration/transactions/TestILoanBase.cs-247-250 (1)
247-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid swallowing all exceptions in tx polling.
Lines 247-250 catch everything and discard context, which can hide deterministic failures and convert them into generic timeout errors.
Suggested fix
+Exception lastError = null; for (int i = 0; i < 15; i++) { await Task.Delay(1000); try { txResponse = await client.Tx(txReq); if (txResponse?.Meta != null) break; } - catch + catch (Exception ex) { - // tx may not be found yet — retry + lastError = ex; // keep context for final failure message } } if (txResponse?.Meta == null) - throw new RippleException($"LoanSet tx not validated in time: {txHash}"); + throw new RippleException($"LoanSet tx not validated in time: {txHash}. Last error: {lastError?.Message}");🤖 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 247 - 250, In TestILoanBase, replace the empty catch in the tx polling block with explicit exception handling: catch only the expected "not found/temporary" exception(s) (e.g., the rippled client not-found or transient response type) and allow other exceptions to surface—capture the exception into a variable (e.g., catch (Exception ex)), detect and swallow only when it matches the transient/not-found condition, otherwise rethrow or log ex with context (including transaction id and attempt info); update the polling loop so the retry path checks that condition rather than swallowing all exceptions silently.Xrpl/Models/Methods/ServerDefinitions.cs-45-64 (1)
45-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModel optional
server_definitionsmaps as nullable (or default-initialize them).Your own request comment notes hash-matched responses can be empty, but the response maps are non-nullable. That creates an avoidable null-deref risk for valid API responses.
🛠️ Proposed fix
[JsonPropertyName("LEDGER_ENTRY_TYPES")] -public Dictionary<string, int> LedgerEntryTypes { get; set; } +public Dictionary<string, int>? LedgerEntryTypes { get; set; } [JsonPropertyName("TRANSACTION_RESULTS")] -public Dictionary<string, int> TransactionResults { get; set; } +public Dictionary<string, int>? TransactionResults { get; set; } [JsonPropertyName("TRANSACTION_TYPES")] -public Dictionary<string, int> TransactionTypes { get; set; } +public Dictionary<string, int>? TransactionTypes { get; set; } [JsonPropertyName("TYPES")] -public Dictionary<string, int> Types { get; set; } +public Dictionary<string, int>? Types { get; 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/Methods/ServerDefinitions.cs` around lines 45 - 64, The four mapping properties in ServerDefinitions (LedgerEntryTypes, TransactionResults, TransactionTypes, Types) are currently non-nullable and can be null when the API returns empty/omitted maps; change each property to be nullable (Dictionary<string,int>?) or default-initialize them to an empty Dictionary to avoid null dereferences. Locate the properties named LedgerEntryTypes, TransactionResults, TransactionTypes and Types in ServerDefinitions.cs and update their declarations to either add the nullable annotation or assign new Dictionary<string,int>() as a default value so callers never get a null reference.Xrpl/Models/Transactions/VaultSet.cs-86-92 (1)
86-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen
VaultSetfield validation beyond type-checkingVaultID.
ValidateVaultSetcurrently accepts any string forVaultIDand skips format checks for optional fields (Data,DomainID). This can pass invalid transactions through local validation and fail only at submit-time.🛠️ Proposed fix
public static async Task ValidateVaultSet(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); - if (!tx.TryGetValue("VaultID", out var vaultId) || vaultId is not string) + if (!tx.TryGetValue("VaultID", out var vaultIdObj) || vaultIdObj is not string vaultId) throw new ValidationException("VaultSet: missing field VaultID"); + + if (!IsHex(vaultId, 64)) + throw new ValidationException("VaultSet: VaultID must be a 64-char hex string"); + + if (tx.TryGetValue("DomainID", out var domainIdObj)) + { + if (domainIdObj is not string domainId || !IsHex(domainId, 64)) + throw new ValidationException("VaultSet: DomainID must be a 64-char hex string"); + } + + if (tx.TryGetValue("Data", out var dataObj)) + { + if (dataObj is not string data || data.Length > 512 || (data.Length % 2) != 0 || !IsHex(data)) + throw new ValidationException("VaultSet: Data must be hex and <= 256 bytes"); + } } + +private static bool IsHex(string value, int? exactLength = null) +{ + if (string.IsNullOrWhiteSpace(value)) return false; + if (exactLength.HasValue && value.Length != exactLength.Value) return false; + foreach (char c in value) + { + bool isHex = (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); + if (!isHex) return false; + } + return true; +}🤖 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/VaultSet.cs` around lines 86 - 92, ValidateVaultSet currently only checks that "VaultID" is a string; strengthen it by verifying "VaultID" matches the expected VaultID format (non-empty, correct charset and length per VaultID spec) and throw a ValidationException if it doesn't; additionally, if optional fields "Data" or "DomainID" are present in tx, validate "Data" is in the expected encoded format (e.g., hex/base64 as per protocol) and that "DomainID" meets allowed character/length rules, throwing ValidationException with clear messages on failure; keep Common.ValidateBaseTransaction(tx) and add these checks in ValidateVaultSet.Tools/GenerateEnums/Program.cs-448-452 (1)
448-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently skip unknown field types.
Skipping unknown types risks drift between
definitions.jsonand generated codec files. Prefer failing fast so new protocol types cannot be missed.Suggested fix
if (!TypeToFieldClass.ContainsKey(typeName)) { - Console.Error.WriteLine($" ! Unknown field type '{typeName}' for field '{name}', skipping."); - continue; + throw new InvalidOperationException( + $"Unknown field type '{typeName}' for field '{name}'. " + + "Update TypeToFieldClass/TypeToFileName mappings before generating."); }Based on learnings: Generate enums from rippled binary codec definitions using the GenerateEnums tool to keep protocol definitions in sync.
🤖 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 `@Tools/GenerateEnums/Program.cs` around lines 448 - 452, The code currently logs unknown field types and continues, which can hide missing protocol types; update the logic in Program.cs where TypeToFieldClass is checked (the block referencing TypeToFieldClass, typeName and name) to fail fast instead of skipping: throw a descriptive exception (e.g., InvalidOperationException) or call Environment.Exit(1) with a clear message that includes the unknown typeName and field name so generation stops and the mismatch between definitions.json and generated codec files is immediately visible.Tools/GenerateEnums/Program.cs-301-316 (1)
301-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail generation when duplicate
nthvalues are detected.Logging conflicts but continuing can still produce invalid generated field definitions. This should terminate generation with a non-zero exit.
Suggested fix
private static void ValidateFields(Dictionary<string, List<FieldEntry>> grouped) { + bool hasConflict = false; foreach (var (typeName, fields) in grouped) { var duplicateNths = fields .GroupBy(f => f.Nth) .Where(g => g.Count() > 1) .ToList(); foreach (var dup in duplicateNths) { + hasConflict = true; string names = string.Join(", ", dup.Select(f => f.Name)); Console.Error.WriteLine($" ! CONFLICT: Duplicate nth={dup.Key} in {typeName}: {names}"); } } + + if (hasConflict) + throw new InvalidOperationException("Duplicate field nth values detected. Aborting generation."); }🤖 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 `@Tools/GenerateEnums/Program.cs` around lines 301 - 316, ValidateFields currently only logs duplicate nth conflicts and continues; change it to fail generation with a non-zero exit when any duplicates are found. Inside ValidateFields (the method iterating grouped Dictionary<string, List<FieldEntry>>), record if any duplicate groups are detected and after writing Console.Error messages either throw an InvalidOperationException with a consolidated message or call Environment.Exit(1) so the process terminates non-zero; ensure the error output includes the same conflict details (typeName, dup.Key, names) before exiting so callers get the failure and context.Xrpl/Models/Transactions/LedgerStateFix.cs-61-63 (1)
61-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
OwnerinValidateLedgerStateFixas well.
ILedgerStateFix/models exposeOwner, but the validator currently allows requests without it, which can pass client validation and fail downstream.Suggested patch
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 ownerText || string.IsNullOrWhiteSpace(ownerText)) + throw new ValidationException("LedgerStateFix: missing or invalid 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 61 - 63, The ValidateLedgerStateFix validator currently only checks for "LedgerFixType"; update the ValidateLedgerStateFix method to also require and validate the "Owner" field from the ILedgerStateFix model: check tx.TryGetValue("Owner", out var owner) and ensure owner is not null/empty (and of the expected type/string), throwing a ValidationException with a clear message like "LedgerStateFix: missing field Owner" when absent or invalid; update any existing error text/logging in LedgerStateFix.cs accordingly.Xrpl/Models/Transactions/LoanBrokerSet.cs-133-139 (1)
133-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidator is missing the documented range/format checks for optional fields.
Line 137 only validates
VaultID, but the model documents bounds forCoverRateMinimum,CoverRateLiquidation, andManagementFeeRate, plus constraints forDebtMaximum/Data. Add checks when those fields are present.Proposed fix (range checks)
@@ public static async Task ValidateLoanBrokerSet(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); if (!tx.TryGetValue("VaultID", out var vaultId) || vaultId is not string) throw new ValidationException("LoanBrokerSet: missing field VaultID"); + + if (tx.TryGetValue("CoverRateMinimum", out var coverMinObj) && coverMinObj is not null && + (!uint.TryParse(coverMinObj.ToString(), out var coverMin) || coverMin > 100000)) + throw new ValidationException("LoanBrokerSet: invalid field CoverRateMinimum"); + + if (tx.TryGetValue("CoverRateLiquidation", out var coverLiqObj) && coverLiqObj is not null && + (!uint.TryParse(coverLiqObj.ToString(), out var coverLiq) || coverLiq > 100000)) + throw new ValidationException("LoanBrokerSet: invalid field CoverRateLiquidation"); + + if (tx.TryGetValue("ManagementFeeRate", out var feeObj) && feeObj is not null && + (!ushort.TryParse(feeObj.ToString(), out var fee) || fee > 10000)) + throw new ValidationException("LoanBrokerSet: invalid field ManagementFeeRate"); }🤖 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 133 - 139, The ValidateLoanBrokerSet validator currently only checks VaultID; extend it to validate optional fields when present: for CoverRateMinimum and CoverRateLiquidation ensure they are numeric (int/long/double/decimal) and within documented bounds (e.g., 0–100 or the model's specified min/max), for ManagementFeeRate ensure numeric and within its allowed range, for DebtMaximum validate numeric type and non-negative/within documented cap, and for Data validate type/format (e.g., string or JSON object) per the model; implement these checks inside ValidateLoanBrokerSet (use tx.TryGetValue for each key and type checks + range comparisons) and throw ValidationException with clear messages referencing the field name when a check fails.Xrpl/Models/Transactions/XChainAddClaimAttestation.cs-173-199 (1)
173-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
Destinationvalidation and enforceWasLockingChainSendas 0/1.Line 173 onward validates many required fields, but
Destinationis never validated, and Line 197 only null-checksWasLockingChainSend. This lets invalid payloads pass local validation.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"); @@ - if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || wasLocking is null) + if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || + !byte.TryParse(wasLocking?.ToString(), out var wasLockingByte) || + (wasLockingByte != 0 && wasLockingByte != 1)) throw new ValidationException("XChainAddClaimAttestation: missing 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/XChainAddClaimAttestation.cs` around lines 173 - 199, Add a validation check for the "Destination" field and strengthen the "WasLockingChainSend" check inside XChainAddClaimAttestation validation: ensure you call tx.TryGetValue("Destination", out var destination) and throw a ValidationException if missing or not a string, and replace the current null-check on wasLocking with a type-and-value check (e.g. ensure wasLocking is an integer/long/number and its numeric value is exactly 0 or 1) so invalid values fail validation; update the ValidationException messages to reference "Destination" and "WasLockingChainSend" accordingly.Xrpl/Models/Transactions/XChainAddAccountCreateAttestation.cs-192-220 (1)
192-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten validation for
XChainAccountCreateCountandWasLockingChainSend.Line 192 only checks null for
XChainAccountCreateCount, and Line 219 only checks null forWasLockingChainSend. Add type/range checks so invalid values are rejected before submission.Proposed fix
@@ - if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || count is null) + if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || count is not string) throw new ValidationException("XChainAddAccountCreateAttestation: missing field XChainAccountCreateCount"); @@ - if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || wasLocking is null) + if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || + !byte.TryParse(wasLocking?.ToString(), out var wasLockingByte) || + (wasLockingByte != 0 && wasLockingByte != 1)) throw new ValidationException("XChainAddAccountCreateAttestation: missing 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 checks in XChainAddAccountCreateAttestation currently only ensure presence for XChainAccountCreateCount and WasLockingChainSend; update the tx.TryGetValue validations to assert correct types and ranges: for XChainAccountCreateCount (from tx.TryGetValue("XChainAccountCreateCount", out var count)) ensure count is an integer type (int/long) and convert to an int then validate >= 0 (and optionally within a sensible max) before accepting, otherwise throw ValidationException with a clear message; for WasLockingChainSend (from tx.TryGetValue("WasLockingChainSend", out var wasLocking)) ensure the value is a boolean (bool) and reject non-bool values with ValidationException; keep using the same tx.TryGetValue pattern and error style so the checks live next to the existing validations in XChainAddAccountCreateAttestation.Tests/Xrpl.BinaryCodec.Test/Types/TestNumberType.cs-11-12 (1)
11-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winApply test naming convention.
The class should be named
TestUNumberTypeto follow the project's test classification convention. As per coding guidelines, useTestUprefix for unit tests andTestIfor integration tests.♻️ Proposed fix
[TestClass] -public class TestNumberType +public class TestUNumberType🤖 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/TestNumberType.cs` around lines 11 - 12, Rename the test class to follow the project's convention by changing the class declaration name from TestNumberType to TestUNumberType (i.e., public class TestUNumberType) and update any references to this class (test attributes, test runner references, or usages in other tests) to use the new name; ensure the [TestClass] attribute remains and run the tests to verify the rename didn't break any references.Tests/Xrpl.BinaryCodec.Test/TestFieldDispatch.cs-11-12 (1)
11-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winApply test naming convention.
The class should be named
TestUFieldDispatchto follow the project's test classification convention. As per coding guidelines, useTestUprefix for unit tests.♻️ Proposed fix
[TestClass] -public class TestFieldDispatch +public class TestUFieldDispatch🤖 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/TestFieldDispatch.cs` around lines 11 - 12, Rename the test class TestFieldDispatch to follow the unit-test naming convention by changing its declaration to TestUFieldDispatch; update the class identifier in the file (TestFieldDispatch -> TestUFieldDispatch) so references (attributes like [TestClass] and any usages) continue to point to the renamed class.Xrpl/Wallet/LoanSigningHelper.cs-217-225 (1)
217-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
SigningPubKeyto already exist and match inBrokerSign.For a partially signed blob, setting
SigningPubKeyhere can invalidate the existingCounterpartySignaturepreimage. This should be treated as invalid input instead of being auto-filled.Suggested patch
string existingSigningPubKey = tx["SigningPubKey"]?.GetValue<string>(); -if (!string.IsNullOrEmpty(existingSigningPubKey) && - !string.Equals(existingSigningPubKey, brokerWallet.PublicKey, StringComparison.Ordinal)) +if (string.IsNullOrEmpty(existingSigningPubKey)) +{ + throw new ValidationException("Partially signed blob is missing SigningPubKey."); +} +if (!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 217 - 225, In BrokerSign (LoanSigningHelper.cs) do not auto-set tx["SigningPubKey"]; instead require the SigningPubKey field to already exist and exactly match brokerWallet.PublicKey (use StringComparison.Ordinal) — if tx["SigningPubKey"] is null/empty or does not equal brokerWallet.PublicKey, throw a ValidationException indicating the partially signed blob has an invalid/missing SigningPubKey (do not assign brokerWallet.PublicKey to tx["SigningPubKey"] as that can invalidate CounterpartySignature).
🧹 Nitpick comments (7)
Tests/Xrpl.Tests/Client/Json/RequestSerializationTests.cs (1)
46-65: ⚡ Quick winAdd explicit null-omission checks for newly nullable request fields.
These tests validate happy-path serialization but not the nullable behavior introduced in this PR (
strict,limit,deletion_blockers_only). Add assertions that null fields are omitted, and present when set.Example test tightening
public void Serialize_AccountObjectsRequest_IncludesAccount() { BaseRequest request = new AccountObjectsRequest("rTestAddress"); string json = JsonSerializer.Serialize(request, request.GetType(), Options); Assert.IsTrue(json.Contains("\"account\""), "Missing 'account' field"); Assert.IsTrue(json.Contains("rTestAddress"), "Missing account value"); Assert.IsTrue(json.Contains("\"account_objects\""), "Missing command value"); + Assert.IsFalse(json.Contains("\"limit\""), "Nullable 'limit' should be omitted when null"); + Assert.IsFalse(json.Contains("\"deletion_blockers_only\""), "Nullable 'deletion_blockers_only' should be omitted when null"); } public void Serialize_AccountOffersRequest_IncludesAccount() { - BaseRequest request = new AccountOffersRequest("rTestAddress") { Limit = 50 }; + BaseRequest request = new AccountOffersRequest("rTestAddress") { Limit = 50 }; string json = JsonSerializer.Serialize(request, request.GetType(), Options); Assert.IsTrue(json.Contains("\"account\""), "Missing 'account' field"); Assert.IsTrue(json.Contains("rTestAddress"), "Missing account value"); Assert.IsTrue(json.Contains("\"limit\""), "Missing 'limit' field"); + Assert.IsFalse(json.Contains("\"strict\""), "Nullable 'strict' should be omitted when null"); }🤖 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/Client/Json/RequestSerializationTests.cs` around lines 46 - 65, Update the two tests Serialize_AccountObjectsRequest_IncludesAccount and Serialize_AccountOffersRequest_IncludesAccount to assert nullable fields are omitted when null and present when set: for AccountObjectsRequest verify that "strict" and "deletion_blockers_only" do not appear in JSON by default (null) and do appear when you set request.Strict = true and request.DeletionBlockersOnly = true; for AccountOffersRequest verify that "limit" is omitted when null, present when set (request.Limit = 50) and similarly check any new nullable fields like "strict" if applicable; use JsonSerializer.Serialize(request, request.GetType(), Options) and Assert.IsFalse/IsTrue on json.Contains(...) to validate both omission and presence.Tests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.cs (1)
10-13: ⚡ Quick winUse the
TestUprefix for this unit test classification.Please rename the class/test (or apply your project’s classification mechanism) so this unit test is discoverable by the
TestUfilter convention.As per coding guidelines: "
**/Tests/**/*.cs: Use TestU filter prefix for unit tests and TestI for integration tests in test classification".🤖 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 10 - 13, Rename the unit test to use the TestU prefix so it matches the project test classification; specifically update the class name TestLoanSetEncode (or the test method TestEncodeDecode_LoanSet) to start with "TestU" (for example TestULoanSetEncode or TestU_EncodeDecode_LoanSet) so the test is discoverable by the TestU filter convention referenced in the guidelines.Xrpl/Models/Subscriptions/ManifestStream.cs (1)
44-48: ⚡ Quick winModel the optional
domainfield as nullable.At Line 48,
Domainis optional by contract; usestring?to reflect deserialization reality and avoid nullability mismatches in consumers.🤖 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/ManifestStream.cs` around lines 44 - 48, The Domain property in ManifestStream is declared as a non-nullable string but is optional in the payload; change its type to a nullable reference (string?) on the Domain property in ManifestStream.cs (property name: Domain) so deserialization can produce null safely and update any callers that assume non-null or add null checks where Domain is consumed.Tests/Xrpl.BinaryCodec.Test/TestEnumParity.cs (1)
8-150: ⚡ Quick winRename to
TestU...for unit-test classification compliance.Lines 8-150 use
Test...naming in a unit-test file; switch toTestU...prefix so CI/test filters can classify these tests reliably.As per coding guidelines
**/Tests/**/*.cs: Use TestU filter prefix for unit tests and TestI for integration tests in test classification.🤖 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/TestEnumParity.cs` around lines 8 - 150, The test class and its test methods use the Test... prefix which fails unit-test classification; rename the class TestEnumParity to TestUEnumParity and update each test method name that begins with "Test" (e.g., TestTransactionType_AllNamesResolvable, TestTransactionType_OrdinalLookup, TestLedgerEntryType_AllNamesResolvable, TestLedgerEntryType_OrdinalLookup, TestEngineResult_SuccessAndFailureCodes, TestEngineResult_ShouldClaimFee, TestEngineResult_NewXChainCodes, TestEngineResult_NewAmmCodes) to start with "TestU" (e.g., TestUTransactionType_AllNamesResolvable) so they match the TestU filter for unit tests; ensure all references to the renamed class/methods (attributes like [TestMethod] usages and any test runner entries) are updated accordingly and run the test discovery to confirm CI recognizes them as unit tests.Tests/Xrpl.BinaryCodec.Test/Types/TestAmount.cs (1)
13-155: 🏗️ Heavy liftApply
TestUprefix for unit-test filtering.Lines 13-155 define unit tests without the required
TestUprefix; please rename class/methods accordingly to keep filter-based test selection consistent.As per coding guidelines
**/Tests/**/*.cs: Use TestU filter prefix for unit tests and TestI for integration tests in test classification.🤖 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/TestAmount.cs` around lines 13 - 155, Rename the test class and its test methods to include the required TestU prefix for unit-test filtering; specifically, change class TestAmount to TestUAmount and rename each [TestMethod] method (TestXrpFromJsonString, TestXrpRoundtrip, TestXrpKnownEncoding, TestIouFromJson, TestIouRoundtrip, TestZeroIou, TestNegativeIou, TestFromJson_NullThrows, TestFromJson_NumericDrops) to start with "TestU" (e.g., TestU_XrpFromJsonString or TestUXrpFromJsonString) so the test runner's filter (TestU) will discover them. Ensure all references to the class or methods in the file are updated accordingly and run the tests to verify discovery.Tests/Xrpl.BinaryCodec.Test/Types/TestXChainBridgeType.cs (1)
13-160: ⚡ Quick winUse
TestUprefix for unit-test classification.Lines 13-160 use
Test...names in a unit-test file; rename class/methods toTestU...so test filters classify them correctly.As per coding guidelines
**/Tests/**/*.cs: Use TestU filter prefix for unit tests and TestI for integration tests in test classification.🤖 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/TestXChainBridgeType.cs` around lines 13 - 160, Rename the test class and its test methods to use the TestU prefix so the test runner classifies them as unit tests: change class TestXChainBridgeType to TestUXChainBridgeType and rename each method starting with "Test" to start with "TestU" (e.g. TestFromJson_XrpBridge -> TestUFromJson_XrpBridge, TestFromJson_IouBridge -> TestUFromJson_IouBridge, TestRoundtrip_XrpBridge -> TestURoundtrip_XrpBridge, TestRoundtrip_IouBridge -> TestURoundtrip_IouBridge, TestToJson_Structure -> TestUToJson_Structure, TestBinarySize_XrpBridge -> TestUBinarySize_XrpBridge, TestBinarySize_IouBridge -> TestUBinarySize_IouBridge, TestFromJson_NotObject_Throws -> TestUFromJson_NotObject_Throws); update any internal references to the class or methods in this file (e.g. reflection-based lookups or test lists) to the new names.Tests/TestsClients/Test.ClonsoleApp/Program.cs (1)
718-731: ⚡ Quick winAdd a timeout to websocket wait loops to prevent indefinite hangs.
Both wait loops depend on receiving stream events; if events don’t arrive, the process blocks forever.
Suggested patch pattern
- var task = Task.Run( - async () => - { - while (!isFinished) - { - Debug.WriteLine($"WAITING: {DateTime.Now}"); - await Task.Delay(1000); - } - }); - - await task; + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); + while (!isFinished && !cts.Token.IsCancellationRequested) + { + Debug.WriteLine($"WAITING: {DateTime.Now}"); + await Task.Delay(1000, cts.Token); + } + + if (!isFinished) + throw new TimeoutException("Websocket test timed out waiting for stream data.");Also applies to: 772-785
🤖 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/TestsClients/Test.ClonsoleApp/Program.cs` around lines 718 - 731, The wait loop that spins on the isFinished flag (the Task.Run async lambda that logs WAITING and awaits Task.Delay(1000)) must include a timeout to avoid hanging; implement a timeout using a CancellationTokenSource or Task.WhenAny pattern (e.g., record start time or create a CTS with a TimeSpan, check elapsed/CTS.IsCancellationRequested inside the while loop and break or throw a TimeoutException when exceeded) and apply the same change to the second wait loop around lines 772-785; update cleanup to ensure client.Disconnect() still runs when timeout occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eda6d2c9-8452-473b-88db-45a682871119
📒 Files selected for processing (152)
.ci-config/rippled.cfg.gitignoreBase/Xrpl.BinaryCodec/Enums/EngineResult.Generated.csBase/Xrpl.BinaryCodec/Enums/EngineResult.csBase/Xrpl.BinaryCodec/Enums/Field.AccountId.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Amount.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Blob.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Currency.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Hash128.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Hash160.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Hash192.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Hash256.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Int32.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Issue.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Number.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.PathSet.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.StArray.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.StObject.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Uint16.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Uint32.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Uint64.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Uint8.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.Vector256.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.XChainBridge.Generated.csBase/Xrpl.BinaryCodec/Enums/Field.csBase/Xrpl.BinaryCodec/Enums/FieldType.csBase/Xrpl.BinaryCodec/Enums/Int32Field.csBase/Xrpl.BinaryCodec/Enums/Int64Field.csBase/Xrpl.BinaryCodec/Enums/LedgerEntryType.Generated.csBase/Xrpl.BinaryCodec/Enums/LedgerEntryType.csBase/Xrpl.BinaryCodec/Enums/NumberField.csBase/Xrpl.BinaryCodec/Enums/TransactionType.Generated.csBase/Xrpl.BinaryCodec/Enums/TransactionType.csBase/Xrpl.BinaryCodec/Enums/XChainBridgeField.csBase/Xrpl.BinaryCodec/Enums/definitions.jsonBase/Xrpl.BinaryCodec/Types/AmountValue.csBase/Xrpl.BinaryCodec/Types/Int32Type.csBase/Xrpl.BinaryCodec/Types/Int64Type.csBase/Xrpl.BinaryCodec/Types/Issue.csBase/Xrpl.BinaryCodec/Types/NumberType.csBase/Xrpl.BinaryCodec/Types/StObject.csBase/Xrpl.BinaryCodec/Types/Uint32.csBase/Xrpl.BinaryCodec/Types/XChainBridgeType.csBase/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csprojCHANGES.mdCLAUDE.mdDocFx/LendingProtocol-Guide.mdDocFx/LendingProtocol-Guide.ru.mdDocFx/Vault-Guide.mdDocFx/Vault-Guide.ru.mdDocFx/XChainBridge-Guide.mdDocFx/XChainBridge-Guide.ru.mdDocFx/toc.ymlTests/TestsClients/Test.ClonsoleApp/Program.csTests/Xrpl.BinaryCodec.Test/TestBinaryParser.csTests/Xrpl.BinaryCodec.Test/TestBinarySerializer.csTests/Xrpl.BinaryCodec.Test/TestDefinitions.csTests/Xrpl.BinaryCodec.Test/TestEnumParity.csTests/Xrpl.BinaryCodec.Test/TestFieldDispatch.csTests/Xrpl.BinaryCodec.Test/TestFieldIDCodec.csTests/Xrpl.BinaryCodec.Test/TestMain.csTests/Xrpl.BinaryCodec.Test/Types/TestAmount.csTests/Xrpl.BinaryCodec.Test/Types/TestInt32Type.csTests/Xrpl.BinaryCodec.Test/Types/TestInt64Type.csTests/Xrpl.BinaryCodec.Test/Types/TestIssueMpt.csTests/Xrpl.BinaryCodec.Test/Types/TestLoanSetEncode.csTests/Xrpl.BinaryCodec.Test/Types/TestNumberType.csTests/Xrpl.BinaryCodec.Test/Types/TestUInt.csTests/Xrpl.BinaryCodec.Test/Types/TestXChainBridgeType.csTests/Xrpl.BinaryCodec.Test/Xrpl.BinaryCodec.Test.csprojTests/Xrpl.Tests/Client/Json/Converters/TransactionRequestRoundTripTests.csTests/Xrpl.Tests/Client/Json/RequestSerializationTests.csTests/Xrpl.Tests/Client/TestSubscribe.csTests/Xrpl.Tests/Integration/transactions/TestIAMMCreateMpt.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.csTests/Xrpl.Tests/Sugar/TestUAutofillFees.csTools/GenerateEnums/GenerateEnums.csprojTools/GenerateEnums/Program.csTools/GenerateEnums/README.mdXrpl/Client/Exceptions/XrplErrorClassifier.csXrpl/Client/IXrplClient.csXrpl/Client/Json/Converters/CurrencyConverter.csXrpl/Client/Json/Converters/LedgerObjectConverter.csXrpl/Client/Json/Converters/TransactionRequestConverter.csXrpl/Client/Json/Converters/TransactionResponseConverter.csXrpl/Client/RequestManager.csXrpl/Client/connection.csXrpl/Models/Common/Common.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/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.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.csXrpl/Xrpl.csprojXrplCSharp.sln
💤 Files with no reviewable changes (2)
- Tests/Xrpl.BinaryCodec.Test/Xrpl.BinaryCodec.Test.csproj
- Xrpl/Models/Subscriptions/ValidationsStream.cs
Comment on lines
+51
to
+54
| public static XChainBridgeType FromJson(JsonNode token) | ||
| { | ||
| if (token is not JsonObject obj) | ||
| throw new InvalidJsonException("XChainBridge must be a JSON object."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for InvalidJsonException definition
rg -nP --type=cs 'class\s+InvalidJsonException\s*:'
# Verify usage in XChainBridgeType compiles correctly
rg -nP --type=cs -A2 -B2 'InvalidJsonException' Base/Xrpl.BinaryCodec/Types/XChainBridgeType.csRepository: StaticBit-io/XrplCSharp
Length of output: 395
🏁 Script executed:
# Show the first 20 lines of XChainBridgeType.cs to see all using statements
head -n 20 Base/Xrpl.BinaryCodec/Types/XChainBridgeType.csRepository: StaticBit-io/XrplCSharp
Length of output: 838
🏁 Script executed:
# Check the namespace and class definition in Exceptions.cs
head -n 20 Base/Xrpl.BinaryCodec/Exceptions.csRepository: StaticBit-io/XrplCSharp
Length of output: 506
🏁 Script executed:
# Get all using statements from XChainBridgeType.cs before the namespace declaration
sed -n '1,/^namespace/p' Base/Xrpl.BinaryCodec/Types/XChainBridgeType.cs | grep -E '^using|^namespace'Repository: StaticBit-io/XrplCSharp
Length of output: 175
🏁 Script executed:
# Search for how InvalidJsonException is used elsewhere in the codebase
rg -n 'throw new InvalidJsonException|catch.*InvalidJsonException' --type=cs -B1Repository: StaticBit-io/XrplCSharp
Length of output: 6622
🏁 Script executed:
# Check using statements in other Type files that use InvalidJsonException
for file in Base/Xrpl.BinaryCodec/Types/StObject.cs Base/Xrpl.BinaryCodec/Types/Issue.cs Base/Xrpl.BinaryCodec/Types/Amount.cs; do
echo "=== $file ==="
sed -n '1,/^namespace/p' "$file" | grep -E '^using|^namespace'
doneRepository: StaticBit-io/XrplCSharp
Length of output: 777
🏁 Script executed:
# Check if there's a GlobalUsings.cs file or global using statements
find Base/Xrpl.BinaryCodec -name "GlobalUsings.cs" -o -name "*.csproj" | head -5
# Also check the content if GlobalUsings exists
find Base/Xrpl.BinaryCodec -name "GlobalUsings.cs" -exec cat {} \;Repository: StaticBit-io/XrplCSharp
Length of output: 113
🏁 Script executed:
# Check if InvalidJsonException might be in the Types namespace
rg -n 'namespace Xrpl.BinaryCodec' Base/Xrpl.BinaryCodec/Exceptions.cs -A 20 | head -30
# Also check if there are any partial class definitions
rg -n 'partial class InvalidJsonException' --type=csRepository: StaticBit-io/XrplCSharp
Length of output: 651
🏁 Script executed:
# Search for all definitions of InvalidJsonException in the codebase
rg -n 'class InvalidJsonException' --type=csRepository: StaticBit-io/XrplCSharp
Length of output: 156
🏁 Script executed:
# Check if there's any indication this code compiles - look for CI configuration
find . -name ".github" -o -name "*.yml" -o -name "*.yaml" | head -5
# Also check the .csproj file for any global usings
cat Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj | head -50Repository: StaticBit-io/XrplCSharp
Length of output: 1551
Add missing using statement for InvalidJsonException.
Line 54 references InvalidJsonException, which is defined in the Xrpl.BinaryCodec namespace. Add using Xrpl.BinaryCodec; to the file's using statements, or use the fully qualified name Xrpl.BinaryCodec.InvalidJsonException.
🤖 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 51 - 54, The
FromJson method in XChainBridgeType uses InvalidJsonException but the file lacks
the proper using; update the file to either add "using Xrpl.BinaryCodec;" at the
top or change the thrown type to the fully qualified
Xrpl.BinaryCodec.InvalidJsonException in XChainBridgeType.FromJson so the symbol
resolves correctly.
Comment on lines
+91
to
+96
| public void TestFieldTotalCount() | ||
| { | ||
| int count = Field.Values.Count(); | ||
| Assert.IsGreaterThan(200, count, | ||
| $"Expected more than 200 registered fields, got {count}."); | ||
| } |
There was a problem hiding this comment.
Fix inverted assertion logic.
Line 94 uses Assert.IsGreaterThan(200, count) which checks if 200 > count, meaning the test expects fewer than 200 fields. The comment indicates the intent is to verify more than 200 fields are registered. The assertion is backwards.
🐛 Proposed fix
int count = Field.Values.Count();
- Assert.IsGreaterThan(200, count,
+ Assert.IsTrue(count > 200,
$"Expected more than 200 registered fields, got {count}.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void TestFieldTotalCount() | |
| { | |
| int count = Field.Values.Count(); | |
| Assert.IsGreaterThan(200, count, | |
| $"Expected more than 200 registered fields, got {count}."); | |
| } | |
| public void TestFieldTotalCount() | |
| { | |
| int count = Field.Values.Count(); | |
| Assert.IsTrue(count > 200, | |
| $"Expected more than 200 registered fields, got {count}."); | |
| } |
🤖 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/TestFieldDispatch.cs` around lines 91 - 96, The
assertion in TestFieldTotalCount is inverted: it currently calls
Assert.IsGreaterThan(200, count) which checks 200 > count; change it to assert
that count is greater than 200 by swapping the arguments or using an appropriate
assertion (e.g., Assert.IsGreaterThan(count, 200) or Assert.Greater(count, 200)
/ Assert.IsTrue(count > 200)) so the test verifies Field.Values.Count() > 200.
Comment on lines
+111
to
+131
| public static async Task ValidateLoanPay(Dictionary<string, object> tx) | ||
| { | ||
| await Common.ValidateBaseTransaction(tx); | ||
|
|
||
| if (!tx.TryGetValue("LoanID", out var id) || id is not string) | ||
| throw new ValidationException("LoanPay: missing field LoanID"); | ||
|
|
||
| if (!tx.TryGetValue("Amount", out var amount) || amount is null) | ||
| throw new ValidationException("LoanPay: missing field Amount"); | ||
|
|
||
| // Only one of tfLoanOverpayment, tfLoanFullPayment, tfLoanLatePayment can be set (per xrpl.js) | ||
| if (tx.TryGetValue("Flags", out var flagsObj) && flagsObj is not null) | ||
| { | ||
| uint rawFlags = ParseFlags(flagsObj); | ||
| uint exclusive = (uint)(LoanPayFlags.tfLoanOverpayment | LoanPayFlags.tfLoanFullPayment | LoanPayFlags.tfLoanLatePayment); | ||
| uint selected = rawFlags & exclusive; | ||
| // Check if more than one bit is set among the exclusive flags | ||
| if (selected != 0 && (selected & (selected - 1)) != 0) | ||
| throw new ValidationException("LoanPay: only one of tfLoanOverpayment, tfLoanFullPayment, or tfLoanLatePayment can be set"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ParseFlags method definition in Validation partial classes
rg -nP --type=cs 'ParseFlags\s*\(' Xrpl/Models/Transactions/
# Check the method signature and implementation
ast-grep --pattern $'ParseFlags($$$) {
$$$
}'Repository: StaticBit-io/XrplCSharp
Length of output: 357
🏁 Script executed:
# Check the beginning of LoanPay.cs to confirm it's a partial class
head -20 Xrpl/Models/Transactions/LoanPay.cs
# Search for all ParseFlags definitions in the entire project
rg -nP --type=cs 'private\s+static\s+uint\s+ParseFlags'
# Check if there are multiple parts of LoanPay partial class
rg -l --type=cs 'partial class LoanPay' Xrpl/Repository: StaticBit-io/XrplCSharp
Length of output: 637
🏁 Script executed:
# Read more of LoanPay.cs to see the class declaration
sed -n '20,140p' Xrpl/Models/Transactions/LoanPay.cs
# Check if there are other files for LoanPay partial class
find Xrpl/ -name "*LoanPay*" -type fRepository: StaticBit-io/XrplCSharp
Length of output: 3933
🏁 Script executed:
# Check if there's a Validation.cs file
find Xrpl/Models/Transactions/ -name "Validation.cs" -type f
# Search for all ParseFlags definitions in the entire codebase
rg -nP --type=cs 'static\s+uint\s+ParseFlags' Xrpl/
# Check if ParseFlags is defined in the same LoanPay.cs file after line 131
wc -l Xrpl/Models/Transactions/LoanPay.cs
tail -20 Xrpl/Models/Transactions/LoanPay.csRepository: StaticBit-io/XrplCSharp
Length of output: 1339
🏁 Script executed:
# Check the Validation.cs file for ParseFlags definition
rg -nP --type=cs 'ParseFlags' Xrpl/Models/Transactions/Validation.cs -A 5
# Get the full content or relevant portions of Validation.cs
wc -l Xrpl/Models/Transactions/Validation.csRepository: StaticBit-io/XrplCSharp
Length of output: 110
🏁 Script executed:
# Search for ParseFlags across all C# files in the entire repository
rg -nP --type=cs 'ParseFlags' --no-filename
# Get the content of Validation.cs to understand its structure
cat Xrpl/Models/Transactions/Validation.cs | head -50
# Search specifically for method definitions that might contain ParseFlags
rg -B 2 -A 10 'ParseFlags' Xrpl/Models/Transactions/Validation.csRepository: StaticBit-io/XrplCSharp
Length of output: 1975
🏁 Script executed:
# Look for all ParseFlags definitions across the codebase
rg -B 2 -A 15 'private static uint ParseFlags' Xrpl/Models/Transactions/
# Check if ParseFlags is defined in Validation.cs
rg -n 'ParseFlags' Xrpl/Models/Transactions/Validation.cs
# Look at the entire Validation.cs to see the structure
cat Xrpl/Models/Transactions/Validation.csRepository: StaticBit-io/XrplCSharp
Length of output: 11144
🏁 Script executed:
# Confirm the exact structure of the Validation class in LoanPay.cs
grep -n "class Validation" Xrpl/Models/Transactions/LoanPay.cs
# Confirm the structure in LoanManage.cs
grep -n "class Validation" Xrpl/Models/Transactions/LoanManage.cs
# Check if ParseFlags exists anywhere else in the codebase
rg 'static.*ParseFlags' Xrpl/ --type=csRepository: StaticBit-io/XrplCSharp
Length of output: 241
🏁 Script executed:
# Let's verify the exact class structure - check if they're truly partial classes
sed -n '92,95p' Xrpl/Models/Transactions/LoanManage.cs
sed -n '109,112p' Xrpl/Models/Transactions/LoanPay.cs
# Check if there are any other files with partial Validation class
rg -l 'partial class Validation' Xrpl/Models/Transactions/Repository: StaticBit-io/XrplCSharp
Length of output: 3452
ParseFlags method is missing and will cause a compilation error.
Line 124 calls ParseFlags(flagsObj), but this method is only defined in LoanManage.cs as private static. Since each transaction validation class is a separate partial class in its own file, the LoanPay validation cannot access the private ParseFlags method from LoanManage.cs. Define ParseFlags in LoanPay.cs to handle the same input types (uint, int, long, JsonElement).
🤖 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 111 - 131, The
ValidateLoanPay method calls ParseFlags but parses is missing in this file (it
exists as a private static in LoanManage.cs and isn't accessible); add a local
static ParseFlags method to LoanPay.cs that accepts the same input types (uint,
int, long, JsonElement or object) and returns a uint, mirroring the behavior
used elsewhere (e.g., the private static ParseFlags used by LoanManage), then
update ValidateLoanPay to call this new ParseFlags; ensure the new method
handles nulls and the same numeric/JSON element cases so ParseFlags(flagsObj)
compiles and behaves identically.
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
Packages updated
Xrpl10.3.0.0 → 10.4.0.0Xrpl.BinaryCodec10.3.0.0 → 10.4.0.0Xrpl.AddressCodecandXrpl.Keypairsunchanged (no changes since v10.3)NuGet release
Merging to
releasetriggers automatic NuGet publish vianuget.release.ymlworkflow.Summary by CodeRabbit
New Features
Documentation
Tests
Chores