Release v10.4 to main#7
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.
📝 WalkthroughWalkthroughRefactors and auto-generates XRPL binary codec enums/fields, adds new serialized types and stricter Amount handling, and updates definitions. Introduces models, validation, converters, client APIs/events, and signing helpers for XChain Bridge, Vault, Lending, DelegateSet, and LedgerStateFix. Expands unit/integration tests, adds documentation, CI tweaks, and version bumps. ChangesBinary Codec Refactor and New Wire Types
XChain Bridge, Vault, Lending, DelegateSet, and Admin Transactions
Tests Expansion (Unit and Integration)
Docs, CI, and Config
Sequence Diagram(s)sequenceDiagram
actor Borrower as Borrower Wallet
actor Broker as Broker Wallet
participant SDK as XrplCSharp (LoanSigningHelper)
participant Node as rippled
Note over SDK,Node: LoanSet V3 sequential signing
Borrower->>SDK: SignAsLoanCounterparty(LoanSet JSON)
SDK->>SDK: EncodeForSigning(preimage)
SDK-->>Borrower: Partial blob (CounterpartySignature)
Broker->>SDK: BrokerSign(partial blob)
SDK->>SDK: Verify SigningPubKey matches broker
SDK->>SDK: EncodeForSigning(preimage)
SDK-->>Broker: Fully signed blob (TxnSignature + CounterpartySignature)
Broker->>Node: submit(tx_blob)
Node-->>Broker: submit result (hash)
Broker->>Node: tx(hash) wait for meta
Node-->>Broker: meta with Created Loan ID
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: 1
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 (2)
Tests/TestsClients/Test.ClonsoleApp/Program.cs (1)
444-449:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not silently swallow exceptions in account funding flow.
The empty catch masks why balance fetch failed and makes funding behavior hard to troubleshoot. At minimum, log the exception before fallback funding.
🤖 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 444 - 449, The empty catch in Program.Main around the account funding flow swallows exceptions; update the catch to log the exception before proceeding to the fallback funding call: inside the catch block that currently catches Exception e, write a descriptive error using the available logger or Console.Error (e.g., include xrplWallet.ClassicAddress, e.Message and e.StackTrace), then continue to call xrplClient.FundWallet(xrplWallet) as before; reference the catch around the balance fetch and the xrplClient.FundWallet/xrplWallet/funded variables to locate where to add the logging.Base/Xrpl.BinaryCodec/Types/Issue.cs (1)
51-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-native currency without issuer in constructor.
Issue(Currency, AccountId)can create an invalid IOU issue (issuer == null) that later crashes inToBytes(Line 105-107).Suggested fix
public Issue(Currency currency, AccountId issuer) { Currency = currency ?? Currency.Xrp; - Issuer = issuer; + if (!Currency.IsNative && issuer is null) + throw new ArgumentNullException(nameof(issuer), "Issuer is required for non-native currency issues."); + Issuer = issuer; MptIssuanceId = 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 `@Base/Xrpl.BinaryCodec/Types/Issue.cs` around lines 51 - 56, The Issue constructor allows creating a non-native currency with a null issuer which later causes ToBytes to crash; update the Issue(Currency currency, AccountId issuer) constructor to validate inputs and throw a meaningful exception when a non-XRP currency is provided without an issuer (e.g., if (currency != Currency.Xrp && issuer == null) throw new ArgumentNullException(nameof(issuer))), keep existing behavior for Currency.Xrp (allow null issuer or set to null), and ensure MptIssuanceId initialization remains unchanged; reference the Issue constructor and the ToBytes method to locate the related logic.
🟠 Major comments (23)
Tests/Xrpl.BinaryCodec.Test/Types/TestUInt.cs-22-23 (1)
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComparison assertions are reversed and will fail deterministically.
Line 22, Line 23, Line 41, and Line 42 invert operand order relative to the expected values (
124,123,125), so these checks are always false.Proposed fix
- Assert.IsLessThan(v1, v2); - Assert.IsGreaterThan(v2, v1); + Assert.IsGreaterThan(v1, v2); + Assert.IsLessThan(v2, v1); - Assert.IsLessThan(125u, v); - Assert.IsGreaterThan(123u, v); + Assert.IsGreaterThan(125u, v); + Assert.IsLessThan(123u, v);Also applies to: 41-42
🤖 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/TestUInt.cs` around lines 22 - 23, The assertions in TestUInt.cs use the wrong operand order: swap the arguments to Assert.IsLessThan and Assert.IsGreaterThan so they compare the expected smaller value first and the larger value second (e.g., change Assert.IsLessThan(v1, v2) to Assert.IsLessThan(v2, v1) and Assert.IsGreaterThan(v2, v1) to Assert.IsGreaterThan(v1, v2)); apply the same swap for the two assertions around lines 41–42 that also use Assert.IsLessThan/IsGreaterThan involving v1 and v2.Base/Xrpl.BinaryCodec/Types/AmountValue.cs-271-279 (1)
271-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCast to
ulongcan throw unhandledOverflowExceptionbefore validation.Line 277 casts
Math.Abs(parsed)toulongbefore the bounds check on line 278. Inputs larger thanulong.MaxValue(whichdecimalcan represent) will throwOverflowExceptioninstead of the intendedBinaryCodecException. Move theMaxNativeDropscheck before the cast, or usedecimal.ToUInt64()with proper overflow handling.🤖 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/AmountValue.cs` around lines 271 - 279, The cast of Math.Abs(parsed) to ulong in AmountValue (setting Mantissa) can throw OverflowException for decimals > ulong.MaxValue; reorder or change the logic so you validate parsed against MaxNativeDrops and ulong limits before casting: check Math.Abs(parsed) <= MaxNativeDrops and <= ulong.MaxValue (or use decimal.ToUInt64 inside a try/catch) and only then assign Mantissa = (ulong)...; keep setting IsNegative = parsed < 0 and throw BinaryCodecException on out-of-range values instead of allowing OverflowException.Base/Xrpl.BinaryCodec/Types/Int64Type.cs-24-39 (1)
24-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd culture-invariant parsing and null-safety guard to Int64Type JSON handling.
The
FromJsonmethod lacks a null guard and uses culture-sensitiveToString()andlong.Parse()calls. When running in locales with different number formatting (e.g., using comma as decimal separator), these operations produce non-deterministic JSON output and parsing failures. Additionally, a nulltokenparameter throws an unhelpfulNullReferenceExceptioninstead of a clearArgumentNullException.Suggested patch
using System; +using System.Globalization; using System.Text.Json; using System.Text.Json.Nodes; @@ - public JsonNode ToJson() => JsonValue.Create(Value.ToString()); + public JsonNode ToJson() => JsonValue.Create(Value.ToString(CultureInfo.InvariantCulture)); - public override string ToString() => Value.ToString(); + public override string ToString() => Value.ToString(CultureInfo.InvariantCulture); @@ public static Int64Type FromJson(JsonNode token) { + if (token is null) throw new ArgumentNullException(nameof(token)); JsonValueKind kind = token.GetValueKind(); @@ if (kind == JsonValueKind.String) { - return new Int64Type(long.Parse(token.GetValue<string>())); + string raw = token.GetValue<string>(); + if (!long.TryParse(raw, NumberStyles.Integer, CultureInfo.InvariantCulture, out long parsed)) + { + throw new FormatException($"Cannot parse Int64 from JSON string '{raw}'."); + } + return new Int64Type(parsed); }Similar culture-sensitive patterns exist in Uint64.cs, Int32Type.cs, AmountValue.cs, and other numeric types and should be addressed consistently.
🤖 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 24 - 39, Add a null-check and culture-invariant formatting/parsing to Int64Type: in FromJson add a guard that throws ArgumentNullException if token is null; when handling JsonValueKind.Number use token.GetValue<long>() as before, and when handling JsonValueKind.String parse using long.Parse(..., NumberStyles.Integer, CultureInfo.InvariantCulture) (or TryParse with the same culture) and ensure ToJson() and ToString() use Value.ToString(CultureInfo.InvariantCulture) instead of the culture-sensitive overload; apply the same pattern to the analogous numeric types (Uint64, Int32Type, AmountValue, etc.) so JSON output and parsing are deterministic and null-safe.Xrpl/Models/Transactions/LoanBrokerSet.cs-133-139 (1)
133-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidation incorrectly requires
VaultIDfor all LoanBrokerSet flows.The validator contradicts this model’s own contract: modify flow can be identified by
LoanBrokerID, but current logic rejects it unlessVaultIDis present.Proposed fix
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"); + bool hasVaultId = tx.TryGetValue("VaultID", out var vaultId) && + vaultId is string vaultIdStr && + !string.IsNullOrWhiteSpace(vaultIdStr); + + bool hasLoanBrokerId = tx.TryGetValue("LoanBrokerID", out var loanBrokerId) && + loanBrokerId is string loanBrokerIdStr && + !string.IsNullOrWhiteSpace(loanBrokerIdStr); + + if (!hasVaultId && !hasLoanBrokerId) + throw new ValidationException("LoanBrokerSet: missing field VaultID or 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 133 - 139, The ValidateLoanBrokerSet validator incorrectly requires VaultID for all LoanBrokerSet transactions; update ValidateLoanBrokerSet to allow either a create flow (require VaultID) or a modify flow (require LoanBrokerID) by checking tx for LoanBrokerID when VaultID is absent and only throwing ValidationException if neither is present; reference the ValidateLoanBrokerSet method and the VaultID and LoanBrokerID keys in the tx dictionary when implementing the conditional validation.Tools/GenerateEnums/Program.cs-448-452 (1)
448-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unknown field types instead of silently skipping.
Skipping unknown types can generate an incomplete codec surface without failing CI, which is risky for release integrity.
Proposed 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."); }🤖 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 silently skips unknown field types by checking TypeToFieldClass.ContainsKey(typeName) and writing to Console.Error then continuing; instead fail fast so CI catches missing mappings: replace the Console.Error.WriteLine + continue with a hard failure (throw a descriptive exception or call Environment.Exit(1)) that includes typeName and field name, so the failure occurs immediately when TypeToFieldClass.ContainsKey(typeName) is false (reference the TypeToFieldClass.ContainsKey check, the typeName and name variables, and the existing Console.Error.WriteLine/continue location).Tools/GenerateEnums/Program.cs-301-316 (1)
301-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat duplicate
nthconflicts as hard errors.The generator currently reports duplicate
nthvalues but still proceeds, allowing invalid field tables to be emitted.Proposed fix
private static void ValidateFields(Dictionary<string, List<FieldEntry>> grouped) { + bool hasConflicts = 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) { string names = string.Join(", ", dup.Select(f => f.Name)); Console.Error.WriteLine($" ! CONFLICT: Duplicate nth={dup.Key} in {typeName}: {names}"); + hasConflicts = true; } } + + if (hasConflicts) + throw new InvalidOperationException("definitions.json contains duplicate field nth values."); }🤖 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 logs duplicate nth conflicts via Console.Error.WriteLine but continues; change it to treat duplicates as hard errors by throwing an exception (e.g., InvalidOperationException) or terminating process after logging so generation stops. In the ValidateFields method, after detecting duplicateNths (and after constructing the names/message for each dup), either aggregate messages and throw a single InvalidOperationException including typeName/dup.Key/names (so the generator halts) or call Environment.Exit(1) after writing errors; ensure you reference ValidateFields, the duplicateNths/dup variable and FieldEntry.Nth/Name when building the error text so the exception contains the same conflict details that were previously logged.Tests/Xrpl.BinaryCodec.Test/TestEnumParity.cs-12-149 (1)
12-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename test methods to follow required
TestU/TestIprefixes.Test methods in this file currently use
Test...naming, but not the requiredTestU...(unit) orTestI...(integration) prefixes, which can break test filtering conventions.As per coding guidelines: "
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering".🤖 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 12 - 149, The test methods in TestEnumParity.cs use the "Test..." prefix but must follow the repo convention of "TestU..." for unit tests; rename each test method (e.g., TestTransactionType_AllNamesResolvable, TestTransactionType_OrdinalLookup, TestLedgerEntryType_AllNamesResolvable, TestLedgerEntryType_OrdinalLookup, TestEngineResult_SuccessAndFailureCodes, TestEngineResult_ShouldClaimFee, TestEngineResult_NewXChainCodes, TestEngineResult_NewAmmCodes) to start with "TestU" (for example TestUTransactionType_AllNamesResolvable) while keeping the [TestMethod] attributes and method bodies unchanged so test discovery and behavior remain the same.Xrpl/Wallet/XrplWallet.cs-1032-1034 (1)
1032-1034:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
Signersfor counterparty single-sign flow.
SignAsLoanCounterpartyremovesTxnSignaturebut currently allowsSignersto remain. That can produce a mismatched preimage/signature flow for LoanSet co-signing.Suggested fix
// Remove existing signatures but keep SigningPubKey (broker's) +if (tx["Signers"] != null) + throw new ValidationException("LoanSet for counterparty signing must not include Signers."); + tx.Remove("CounterpartySignature"); tx.Remove("TxnSignature");🤖 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 1032 - 1034, In SignAsLoanCounterparty (XrplWallet.cs) the method currently removes "CounterpartySignature" and "TxnSignature" but leaves "Signers", which allows a Signers array to produce mismatched preimage/signature flows; update the method to also reject/remove any existing "Signers" entry from the transaction (call tx.Remove("Signers") or equivalent) so the counterparty single-sign flow cannot include co-signatures.Tests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.cs-15-55 (1)
15-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard this integration test on amendment availability to avoid flaky failures.
This test only accepts
tesSUCCESS/tecFAILED_PROCESSING, but it can legitimately fail for environments where the LedgerStateFix amendment is not enabled. Add a pre-check and mark inconclusive/skip when unsupported.🤖 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 15 - 55, Add a runtime guard in TestLedgerStateFix_Basic that queries the test XRPL node/client for enabled amendments and skips the test if the LedgerStateFix amendment is not present: use the existing static client (set up in ClassInitializeAsync) to retrieve server/amendment info, check for the "LedgerStateFix" amendment (or whatever exact amendment identifier your node exposes), and call Assert.Inconclusive (or the MSTest skip mechanism) to mark the test as skipped when the amendment is unavailable before attempting to fund the wallet or submit the LedgerStateFix transaction.Tests/Xrpl.Tests/Integration/transactions/TestIAMMCreateMpt.cs-115-120 (1)
115-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid asserting on exception message text for protocol rejection checks.
ex.Message.Contains("MPT")is fragile; prefer asserting structured error/result identifiers when available so tests stay stable across message wording changes.🤖 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/TestIAMMCreateMpt.cs` around lines 115 - 120, The test currently asserts on the free-form exception message inside the catch (RippledException ex) block in TestIAMMCreateMpt.cs which is brittle; change the assertion to check a structured error identifier exposed by RippledException (for example use ex.ErrorCode or ex.Error / ex.Result["error_code"] if available) or, if those properties are not present, add a small accessor on RippledException that returns the protocol error code and assert against that (e.g., Assert.AreEqual("temMPT", ex.ErrorCode)). Update the catch block to use the structured field instead of ex.Message.Contains("MPT") so the test validates the protocol error identifier rather than message text.Tests/TestsClients/Test.ClonsoleApp/Program.cs-718-729 (1)
718-729:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout/cancellation path to websocket wait loops.
Both loops can block forever when no event arrives, which can hang this client workflow indefinitely. Add a bounded timeout and fail/exit explicitly.
Also applies to: 772-783
🤖 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 - 729, The waiting loops that spin on the shared isFinished flag inside the Task.Run lambda (and the similar loop at the other occurrence) need a bounded timeout/cancellation to avoid hanging; introduce a CancellationTokenSource with a sensible timeout or accept a CancellationToken, use Task.WhenAny or pass the token into Task.Delay(token) inside the while(!isFinished) loop so the delay can be cancelled, and when the timeout triggers set isFinished (or throw/return) and log/propagate an explicit timeout error so the client workflow fails fast instead of blocking forever; update both the Task.Run async lambda that references isFinished and the loop at the other occurrence (lines ~772-783) to use the same cancellation/timeout pattern.Xrpl/Models/Transactions/VaultDelete.cs-50-51 (1)
50-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty
VaultIDvalues inValidateVaultDelete.The current check accepts empty/whitespace strings, which should fail fast as invalid transaction input.
🔧 Suggested fix
- if (!tx.TryGetValue("VaultID", out var vaultId) || vaultId is not string) + if (!tx.TryGetValue("VaultID", out var vaultId) || + vaultId is not string vaultIdStr || + string.IsNullOrWhiteSpace(vaultIdStr)) throw new ValidationException("VaultDelete: missing field VaultID");🤖 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/VaultDelete.cs` around lines 50 - 51, The VaultDelete validation currently only checks presence/type of "VaultID" but allows empty/whitespace values; update the ValidateVaultDelete logic in class VaultDelete (the block that reads var vaultId) to cast vaultId to string and reject empty or whitespace-only values using string.IsNullOrWhiteSpace (or trim + length check), throwing the same ValidationException("VaultDelete: missing field VaultID") when the value is invalid so empty/whitespace VaultID is treated as missing.Xrpl/Models/Transactions/VaultSet.cs-90-92 (1)
90-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten
VaultIDvalidation to reject empty values and invalid optional field types.Current check allows
""/whitespaceVaultID, and optional fields can be non-string without early validation.🔧 Suggested validation hardening
- if (!tx.TryGetValue("VaultID", out var vaultId) || vaultId is not string) + if (!tx.TryGetValue("VaultID", out var vaultId) || + vaultId is not string vaultIdStr || + string.IsNullOrWhiteSpace(vaultIdStr)) throw new ValidationException("VaultSet: missing field VaultID"); + + if (tx.TryGetValue("Data", out var data) && data is not string) + throw new ValidationException("VaultSet: field Data must be a string"); + if (tx.TryGetValue("AssetsMaximum", out var assetsMaximum) && assetsMaximum is not string) + throw new ValidationException("VaultSet: field AssetsMaximum must be a string"); + if (tx.TryGetValue("DomainID", out var domainId) && domainId is not string) + throw new ValidationException("VaultSet: field DomainID must be a string");🤖 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 90 - 92, The VaultSet validation currently accepts empty/whitespace VaultID and allows optional fields with wrong types; update the check in the VaultSet validation (the block using tx.TryGetValue and throwing ValidationException) to require VaultID be a non-empty, non-whitespace string (e.g., use string.IsNullOrWhiteSpace on the cast string) and—where optional fields are read from tx—add early type checks that throw a ValidationException if an optional field exists but is not a string (use tx.TryGetValue(...) and validate the value is string before using it). Ensure you reference the existing tx.TryGetValue/VaultID logic and throw ValidationException with a clear message when validation fails.Xrpl/Models/Transactions/LedgerStateFix.cs-57-63 (1)
57-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
OwnerinValidateLedgerStateFix.Line 61 validates
LedgerFixType, butOwneris never validated. Incomplete transactions can pass local validation unexpectedly.💡 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) + 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, ValidateLedgerStateFix currently checks LedgerFixType but never ensures the Owner field exists; update ValidateLedgerStateFix to also require "Owner" by attempting tx.TryGetValue("Owner", out var owner) and throw a ValidationException like "LedgerStateFix: missing field Owner" if missing or null/empty, and where available call an existing address/account validator (e.g., Common.ValidateAccount or other project validation helper) to validate the Owner format; keep the existing Common.ValidateBaseTransaction call and add this Owner presence/format validation after the LedgerFixType check.Xrpl/Models/Transactions/XChainAddClaimAttestation.cs-173-199 (1)
173-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete attestation validation for
DestinationandWasLockingChainSend.
Destinationis required by the model but not validated, andWasLockingChainSendshould be validated as a byte value limited to0or1.Suggested 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) || wasLocking is not byte flag || (flag != 0 && flag != 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, The validation block in XChainAddClaimAttestation is missing a check for the required Destination field and does not enforce WasLockingChainSend to be a byte 0/1; update the validation inside the XChainAddClaimAttestation parsing/validation routine to (1) verify tx contains "Destination" and that it is a string (or the expected type) and throw a ValidationException with the same naming pattern if absent/invalid, and (2) ensure "WasLockingChainSend" exists and its value is a numeric/byte type and equals 0 or 1 (throw a ValidationException if out of range or wrong type). Ensure you reference the same exception messages style used elsewhere in this method.Xrpl/Models/Transactions/VaultCreate.cs-96-98 (1)
96-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
CurrencyConverterto theAmountproperty in both request and response models.The
Currencytype is polymorphic and can represent XRP (as a string), MPT (as an object withmpt_issuance_id), or issued tokens (as an object withcurrency,issuer,value). Without the converter, serialization/deserialization will fail for non-XRP amounts. Related vault transaction models (VaultClawback,VaultWithdraw) correctly use this converter on theirAmountfields.Suggested fix
[JsonPropertyName("Amount")] + [JsonConverter(typeof(CurrencyConverter))] public Currency Amount { get; set; }Apply to both VaultCreate (lines 96-97) and VaultCreateResponse (lines 133-134).
🤖 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/VaultCreate.cs` around lines 96 - 98, Add the JsonConverter attribute to the Amount property in both VaultCreate and VaultCreateResponse so the polymorphic Currency type is properly (de)serialized: on the Amount property in class VaultCreate and the Amount property in class VaultCreateResponse, add [JsonConverter(typeof(CurrencyConverter))] (or the project's equivalent CurrencyConverter type) so XRP, MPT and issued-token shapes are handled correctly.Base/Xrpl.BinaryCodec/Types/NumberType.cs-206-213 (1)
206-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
expValuetolongto prevent integer overflow in exponent parsing.The line
expValue = expValue * 10 + (str[pos] - '0')usingintcan overflow in unchecked arithmetic before the> 100000cap check evaluates. A malformed exponent string with enough digits will cause the value to wrap, bypassing validation. Uselonginstead and cast the result tointwhen assigning tosciExponent: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++; } @@ - 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 exponent parsing uses an int variable expValue which can overflow during expValue = expValue * 10 + (str[pos] - '0') before the > 100000 check; change expValue's type to long in the loop that accumulates the exponent (where expValue and hasExpDigits are declared and updated) to prevent wrapping, keep the existing sanity cap check against 100000, and when assigning the final value to sciExponent cast or clamp the long to int (e.g., (int)expValue) so sciExponent remains an int.Tests/Xrpl.Tests/Integration/transactions/TestIVault.cs-35-625 (1)
35-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename integration test methods to use the
TestIprefix.Current method names don’t follow the required integration-test naming convention for filtering.
As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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/TestIVault.cs` around lines 35 - 625, The test methods in this file use the TestU/Test... naming instead of the required integration-test prefix; rename each public async Task method that starts with TestVault... (e.g., TestVaultCreate_Basic, TestVaultCreate_WithIOU, TestVaultCreate_WithData, TestVaultCreate_WithOptionalFields, TestVaultDeposit_Basic, TestVaultWithdraw_Basic, TestVaultWithdraw_WithDestination, TestVaultSet_UpdateData, TestVaultSet_UpdateAssetsMaximum, TestVaultDelete_Empty, TestVaultClawback_Basic, TestVaultLedgerEntry_VerifyFields, TestVaultClawback_MPT, TestVaultInfo_Basic) to start with TestI (e.g., TestIVaultCreate_Basic) so they follow the integration-test naming convention; update each method identifier consistently (keeping signatures and attributes unchanged) and run tests to ensure no external references break.Tests/Xrpl.Tests/Integration/transactions/TestIXChainBridge.cs-31-377 (1)
31-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename integration test methods to
TestI*for filterability.Current method names (
TestXChain...) don’t follow the required integration prefix, which will break naming-based test filters.As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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/TestIXChainBridge.cs` around lines 31 - 377, The test methods in this file (e.g., TestXChainCreateBridge_Basic, TestXChainCreateClaimID_Basic, TestXChainModifyBridge_UpdateSignatureReward, TestXChainCommit_Basic, TestXChainAccountCreateCommit_Basic, TestXChainCreateBridge_Iou, TestXChainCreateClaimID_Iou, TestXChainCommit_Iou, TestXChainModifyBridge_Iou) must be renamed to use the TestI prefix so they match integration-test filters; rename each method identifier from TestXChain... to TestI... (preserving the rest of the name and [TestMethod] attributes) and update any references/calls to those method names if present so the test discovery/filtering picks them up.Xrpl/Models/Transactions/XChainAddAccountCreateAttestation.cs-192-193 (1)
192-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate numeric/range constraints for
XChainAccountCreateCountandWasLockingChainSend.Line 192 and Line 219 only enforce non-null. Non-numeric values (or
WasLockingChainSendoutside0/1) currently pass local validation.Suggested fix
+using System; +using System.Text.Json;- if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || count is null) + if (!tx.TryGetValue("XChainAccountCreateCount", out var count) || !TryGetUInt64(count, out _)) throw new ValidationException("XChainAddAccountCreateAttestation: missing field XChainAccountCreateCount"); ... - if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || wasLocking is null) + if (!tx.TryGetValue("WasLockingChainSend", out var wasLocking) || !TryGetByte(wasLocking, out var chainFlag) || (chainFlag != 0 && chainFlag != 1)) throw new ValidationException("XChainAddAccountCreateAttestation: missing field WasLockingChainSend");private static bool TryGetUInt64(object value, out ulong parsed) { switch (value) { case ulong u: parsed = u; return true; case long l when l >= 0: parsed = (ulong)l; return true; case int i when i >= 0: parsed = (ulong)i; return true; case string s when ulong.TryParse(s, out var us): parsed = us; return true; case JsonElement je when je.ValueKind == JsonValueKind.Number && je.TryGetUInt64(out var j): parsed = j; return true; default: parsed = 0; return false; } } private static bool TryGetByte(object value, out byte parsed) { switch (value) { case byte b: parsed = b; return true; case int i when i is >= byte.MinValue and <= byte.MaxValue: parsed = (byte)i; return true; case JsonElement je when je.ValueKind == JsonValueKind.Number && je.TryGetByte(out var j): parsed = j; return true; default: parsed = 0; return false; } }Also applies to: 219-220
🤖 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 - 193, The checks in XChainAddAccountCreateAttestation currently only ensure XChainAccountCreateCount and WasLockingChainSend are present but do not validate numeric type or ranges; add robust parsing helpers (e.g., TryGetUInt64 and TryGetByte as suggested) and use them when reading tx["XChainAccountCreateCount"] and tx["WasLockingChainSend"] to reject non-numeric values, handle JsonElement/string/int/long/ulong cases, and enforce WasLockingChainSend is either 0 or 1 and XChainAccountCreateCount is a non-negative ulong before proceeding; throw ValidationException with the same field-specific message if parsing or range checks fail.Xrpl/Models/Transactions/DelegateSet.cs-102-107 (1)
102-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
PermissionValuestring type before normalization.Line 105 and Line 136 use
ToString(), which accepts non-string JSON/token types and can incorrectly pass validation.Suggested fix
- if (!permObj.TryGetProperty("PermissionValue", out JsonElement permVal)) + if (!permObj.TryGetProperty("PermissionValue", out JsonElement permVal) || permVal.ValueKind != JsonValueKind.String) throw new ValidationException("DelegateSet: each Permission must contain PermissionValue"); - string permValueStr = permVal.ToString(); + string permValueStr = permVal.GetString(); if (string.IsNullOrWhiteSpace(permValueStr)) throw new ValidationException("DelegateSet: PermissionValue must not be empty");- if (!perm.TryGetValue("PermissionValue", out object permVal)) + if (!perm.TryGetValue("PermissionValue", out object permVal) || permVal is not string permValueStr) throw new ValidationException("DelegateSet: each Permission must contain PermissionValue"); - string permValueStr = permVal?.ToString(); if (string.IsNullOrWhiteSpace(permValueStr)) throw new ValidationException("DelegateSet: PermissionValue must not be empty");Also applies to: 133-138
🤖 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 102 - 107, The code currently calls JsonElement.ToString() for "PermissionValue" (and the other occurrence at the second block) which accepts non-string JSON values; change the checks in DelegateSet deserialization to first assert permVal.ValueKind == JsonValueKind.String, then call permVal.GetString() and validate that the returned string is not null/whitespace, throwing the same ValidationException if the kind is not String or the value is empty; update both places where PermissionValue is read so only actual JSON strings are accepted before normalization.Xrpl/Models/Transactions/LoanManage.cs-114-115 (1)
114-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize flag parse failures to
ValidationException.
ParseFlagsthrowsFormatException, soValidateLoanManagecan emit a different exception type than the rest of transaction validators.Suggested fix
- uint rawFlags = ParseFlags(flagsObj); + uint rawFlags; + try + { + rawFlags = ParseFlags(flagsObj); + } + catch (FormatException ex) + { + throw new ValidationException($"LoanManage: invalid field Flags ({ex.Message})"); + }Also applies to: 124-133
🤖 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 114 - 115, ValidateLoanManage calls ParseFlags (which throws FormatException) and then uses LoanManageActionMask; to normalize errors, wrap the ParseFlags calls inside a try/catch that catches FormatException and rethrows a ValidationException (preserving the original exception as InnerException and a clear message). Update both occurrences where ParseFlags is used in ValidateLoanManage so all flag parse failures produce ValidationException instead of FormatException.Xrpl/Models/Transactions/XChainModifyBridge.cs-102-108 (1)
102-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject no-op
XChainModifyBridgetransactions.Current validation allows requests with only
XChainBridgeand no actual modifications (SignatureReward,MinAccountCreateAmount, or clear flag).Suggested fix
public static async Task ValidateXChainModifyBridge(Dictionary<string, object> tx) { await Common.ValidateBaseTransaction(tx); if (!tx.TryGetValue("XChainBridge", out var bridge) || bridge is null) throw new ValidationException("XChainModifyBridge: missing field XChainBridge"); + + bool hasSignatureReward = tx.TryGetValue("SignatureReward", out var reward) && reward is not null; + bool hasMinAccountCreateAmount = tx.TryGetValue("MinAccountCreateAmount", out var minAmount) && minAmount is not null; + bool hasClearFlag = tx.TryGetValue("Flags", out var flagsObj) && flagsObj is uint flags && + (flags & (uint)XChainModifyBridgeFlags.tfClearAccountCreateAmount) != 0; + + if (!hasSignatureReward && !hasMinAccountCreateAmount && !hasClearFlag) + throw new ValidationException("XChainModifyBridge: at least one bridge parameter must be modified"); }🤖 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/XChainModifyBridge.cs` around lines 102 - 108, ValidateXChainModifyBridge currently accepts a transaction that only contains XChainBridge with no actual changes; update ValidateXChainModifyBridge to cast the 'bridge' object to a Dictionary<string, object> and check for at least one modifying field (e.g., "SignatureReward", "MinAccountCreateAmount") or the clear flag (name used in the diff for clearing the bridge), and if none are present throw a ValidationException like "XChainModifyBridge: no modifications specified". Use the existing method name ValidateXChainModifyBridge and the local variable 'bridge' to locate where to add the extra check.
🧹 Nitpick comments (5)
Tests/Xrpl.BinaryCodec.Test/TestDefinitions.cs (1)
16-107: ⚡ Quick winAlign unit test method names with
TestUprefix for filterability.Consider renaming these test methods to
TestU...to keep filtering conventions uniform.As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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/TestDefinitions.cs` around lines 16 - 107, Rename the unit test methods in TestDefinitions.cs to use the TestU prefix so they follow the repository filtering convention; specifically rename TestFieldExists, TestGetFieldTypeName, TestGetFieldTypeCode, TestGetFieldCode, TestGetFieldHeader, TestGetFieldNameFromHeader, TestInverseTransactionTypeMap, TestInverseTransactionResultMap, TestFieldTypes_KnownMappings, TestFieldIsSigningField, and TestFieldIsVlEncoded to TestUFieldExists, TestUGetFieldTypeName, TestUGetFieldTypeCode, TestUGetFieldCode, TestUGetFieldHeader, TestUGetFieldNameFromHeader, TestUInverseTransactionTypeMap, TestUInverseTransactionResultMap, TestUFieldTypes_KnownMappings, TestUFieldIsSigningField, and TestUFieldIsVlEncoded respectively so the test runner/filter can consistently identify unit tests.Tests/Xrpl.Tests/Integration/transactions/TestILedgerStateFix.cs (1)
32-32: ⚡ Quick winUse the integration test prefix for method naming consistency.
Rename this method to start with
TestIto preserve filtering conventions.As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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 32, The test method TestLedgerStateFix_Basic is misnamed for integration tests; rename the method to start with TestI (e.g., TestILedgerStateFix_Basic) to follow the TestI prefix convention and update any references or usages of TestLedgerStateFix_Basic (such as test runners, callers, or reflection-based filters) so the test continues to run under the integration test filter.Tests/Xrpl.Tests/Integration/transactions/TestIAMMCreateMpt.cs (1)
50-50: ⚡ Quick winUse
TestIprefix for integration test method naming.Rename this test method to start with
TestIfor consistent test filtering.As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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/TestIAMMCreateMpt.cs` at line 50, The integration test method currently named TestAMMCreate_MptXrpPool_RejectedByProtocol should be renamed to start with the TestI prefix to satisfy test-filtering conventions; update the method declaration for TestAMMCreate_MptXrpPool_RejectedByProtocol to TestI_AMMCreate_MptXrpPool_RejectedByProtocol (or TestIAMMCreate_MptXrpPool_RejectedByProtocol per your naming pattern) and adjust any references or attributes (e.g., in TestAMMCreate_MptXrpPool_RejectedByProtocol usages or test runners) to use the new method name so integration tests are correctly discovered.Tests/Xrpl.Tests/Integration/transactions/TestIXChainBridge.cs (1)
132-137: ⚡ Quick winAvoid hardcoded
XChainClaimID = "1"in commit tests.Use the claim ID produced by
XChainCreateClaimIDresult instead of a fixed literal to reduce flakiness when ledger state or sequencing differs.Also applies to: 324-325
🤖 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/TestIXChainBridge.cs` around lines 132 - 137, The test currently hardcodes XChainClaimID = "1" in the XChainCommit construction which can flake; instead obtain the claim ID from the earlier XChainCreateClaimID result and assign that value to XChainCommit.XChainClaimID. Locate the code that calls XChainCreateClaimID (the result object from that call) and replace the literal "1" in the XChainCommit initializer with the claim ID property from that result; apply the same change at the other occurrence around lines 324-325 to use the generated claim ID rather than a hardcoded string.Tests/Xrpl.Tests/Integration/transactions/TestILoan.cs (1)
35-508: 🏗️ Heavy liftRename integration test methods to
TestI*for filter compliance.Methods like
TestLoanBrokerSet_BasicandTestLoanPay_Basicshould use theTestIprefix to match the repository’s integration-test naming convention.As per coding guidelines
**/Tests/**/*.cs: Name unit tests with TestU prefix and integration tests with TestI prefix for filtering.🤖 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/TestILoan.cs` around lines 35 - 508, Rename the integration test methods that start with "Test" to use the "TestI" prefix for integration-test filter compliance (e.g., rename TestLoanBrokerSet_Basic → TestILoanBrokerSet_Basic, TestLoanBrokerSet_WithRates → TestILoanBrokerSet_WithRates, TestLoanBrokerCoverDeposit_Basic → TestILoanBrokerCoverDeposit_Basic, TestLoanSet_Basic → TestILoanSet_Basic, TestLoanDelete_Basic → TestILoanDelete_Basic, TestLoanBrokerDelete_Basic → TestILoanBrokerDelete_Basic, TestLoanPay_Basic → TestILoanPay_Basic, TestLoanSet_V2_ParallelSigning → TestILoanSet_V2_ParallelSigning, TestLoanSet_V3_SequentialSigning → TestILoanSet_V3_SequentialSigning, TestLoanBrokerLedgerEntry_VerifyFields → TestILoanBrokerLedgerEntry_VerifyFields, TestLoanLedgerEntry_VerifyFields → TestILoanLedgerEntry_VerifyFields, TestLoanBrokerSet_MPT → TestILoanBrokerSet_MPT, TestLoanSet_MPT → TestILoanSet_MPT, TestLoanPay_MPT → TestILoanPay_MPT, TestLoanPay_WithOverpaymentFlag_Rejected → TestILoanPay_WithOverpaymentFlag_Rejected), updating each method declaration name (e.g., the public async Task TestLoanBrokerSet_Basic() definitions) while leaving their [TestMethod] attributes and signatures intact; search for any external references to these method names (rare for tests) and update them accordingly so the MSTest runner and repository filters pick them up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b73eaf8-8512-44a2-bb3f-13369b80a617
📒 Files selected for processing (153)
.ci-config/rippled.cfg.github/workflows/nuget.release.yml.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
+57
to
+60
| if (mantissa != 0 && exponent != ZeroExponent && | ||
| (exponent < MinExponent || exponent > MaxExponent)) | ||
| throw new FormatException( | ||
| $"NumberType: exponent {exponent} out of range [{MinExponent}, {MaxExponent}]"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "NumberType.cs" | head -20Repository: StaticBit-io/XrplCSharp
Length of output: 111
🏁 Script executed:
cat -n Base/Xrpl.BinaryCodec/Types/NumberType.cs | head -150Repository: StaticBit-io/XrplCSharp
Length of output: 7219
🏁 Script executed:
grep -n "ZeroExponent\|MinExponent\|MaxExponent" Base/Xrpl.BinaryCodec/Types/NumberType.csRepository: StaticBit-io/XrplCSharp
Length of output: 902
🏁 Script executed:
sed -n '50,80p' Base/Xrpl.BinaryCodec/Types/NumberType.cs | cat -nRepository: StaticBit-io/XrplCSharp
Length of output: 1679
🏁 Script executed:
# Search for the FromParser method
grep -n "FromParser\|private static\|public static" Base/Xrpl.BinaryCodec/Types/NumberType.cs | head -30Repository: StaticBit-io/XrplCSharp
Length of output: 275
🏁 Script executed:
sed -n '140,260p' Base/Xrpl.BinaryCodec/Types/NumberType.cs | cat -nRepository: StaticBit-io/XrplCSharp
Length of output: 5135
🏁 Script executed:
grep -n "exponent" Base/Xrpl.BinaryCodec/Types/NumberType.cs | grep -A2 -B2 "FromString\|int.*exponent"Repository: StaticBit-io/XrplCSharp
Length of output: 919
Reject ZeroExponent for non-zero mantissas in FromParser.
mantissa != 0 with exponent == int.MinValue currently passes validation, but the documented invariant explicitly requires ZeroExponent only for zero mantissas. This invalid state creates undefined behavior in ToJson() when negating int.MinValue at line 86.
Suggested fix
- if (mantissa != 0 && exponent != ZeroExponent &&
- (exponent < MinExponent || exponent > MaxExponent))
+ if (mantissa != 0 &&
+ (exponent == ZeroExponent || exponent < MinExponent || exponent > MaxExponent))
throw new FormatException(
- $"NumberType: exponent {exponent} out of range [{MinExponent}, {MaxExponent}]");
+ $"NumberType: exponent {exponent} is invalid for non-zero mantissa; expected range [{MinExponent}, {MaxExponent}]");📝 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
| if (mantissa != 0 && exponent != ZeroExponent && | |
| (exponent < MinExponent || exponent > MaxExponent)) | |
| throw new FormatException( | |
| $"NumberType: exponent {exponent} out of range [{MinExponent}, {MaxExponent}]"); | |
| if (mantissa != 0 && | |
| (exponent == ZeroExponent || exponent < MinExponent || exponent > MaxExponent)) | |
| throw new FormatException( | |
| $"NumberType: exponent {exponent} is invalid for non-zero mantissa; expected range [{MinExponent}, {MaxExponent}]"); |
🤖 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 - 60, FromParser
currently allows mantissa != 0 with exponent == ZeroExponent (int.MinValue),
which later causes overflow in ToJson when negating int.MinValue; update the
validation in FromParser to explicitly reject exponent == ZeroExponent for
non-zero mantissas (i.e., if mantissa != 0 and (exponent == ZeroExponent ||
exponent < MinExponent || exponent > MaxExponent) then throw a FormatException),
referencing the ZeroExponent, MinExponent, MaxExponent checks and the
mantissa/exponent variables so the invalid state cannot reach ToJson.
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)Summary by CodeRabbit
Release Notes: Version 10.4.0.0
New Features
Documentation