Enhance documentation and fix IOU amount validation issues#11
Merged
Conversation
* fix: accept trailing decimal point in IOU amount value
The IOU amount value validation regex required at least one digit after
the decimal point, rejecting values like "128700." that xrpl.js,
ripple-binary-codec and rippled accept. This broke signing of
transactions (e.g. AMMDeposit via WalletConnect) carrying such amounts.
Relax the fractional group from (\.(\d+))? to (\.(\d*))? in both
AmountValue.cs and ExtenstionHelpers.cs, and deduplicate the regex by
reusing the single IouValue.ValueRegex constant. Native XRP and MPT
parsing are untouched; mantissa/exponent math, ToString() output and
ToBytes() round-trip are preserved bit-for-bit for already-valid values.
Bump Xrpl and Xrpl.BinaryCodec to 10.4.1.0 and update CHANGES.md.
* fix: reject bare-dot IOU amount values like "." and ".e10"
The relaxed fractional group (\.(\d*))? also matched a lone dot with no
digits anywhere in the mantissa, so "." and ".e10" parsed as 0 — which
BigNumber (xrpl.js / ripple-binary-codec) rejects. Add a non-capturing
(?=\.?\d) lookahead after the optional sign requiring at least one
mantissa digit. Capture-group indices are unchanged, so the index-based
parsing in FromString and ExtenstionHelpers is unaffected; trailing-dot
("128700.") and leading-dot (".5") values remain valid.
📝 WalkthroughWalkthroughThe PR introduces IOU numeric value parsing relaxation to accept trailing decimal points (e.g., ChangesIOU Value Parsing and Regex Deduplication
Path Finding Integration Tests for Negative-One Destination
Documentation Restructuring and Localization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DocFx/ErrorClassifier.ru.md (1)
1-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Russian grammar issues flagged by LanguageTool.
The document is comprehensive and well-structured, but contains two grammar issues:
Line 42: The phrase "устанавливается в
internal error" uses a preposition before the target value incorrectly. It should be "устанавливается какinternal error" (set as) or restructured to avoid the preposition issue.Lines 41-42: The bullet list has two verb forms in sequence that may need restructuring for proper Russian grammar flow.
📝 Suggested grammar corrections
-- `Category` и `Subject` остаются `Unknown` -- `Title` устанавливается в `internal error` -+ `Category` и `Subject` остаются `Unknown` -+ `Title` устанавливается как `internal error` +- `Category` и `Subject` имеют значение `Unknown` +- `Title` имеет значение `internal error` - `UserMessage` содержит оригинальное сообщение исключенияAlternative simpler fix:
- `Category` и `Subject` остаются `Unknown` -- `Title` устанавливается в `internal error` +- `Title` устанавливается на `internal error` - `UserMessage` содержит оригинальное сообщение исключения🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DocFx/ErrorClassifier.ru.md` around lines 1 - 162, В разделе описания полей XrplErrorInfo исправьте грамматику: заменить фразу "устанавливается в `internal error`" на "устанавливается как `internal error`" (или альтернативно перестроить на "Значение Title: `internal error`"), и привести соседние буллеты (в частности про Title и UserMessage) к единому синтаксическому виду — либо все начать с существительного/фразы ("Title — краткий..."; "UserMessage — читаемое объяснение..."), либо все в инфинитиве/пассиве, чтобы устранить последовательность двух разных глагольных форм; отредактируйте упоминания полей Title и UserMessage соответственно.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGES.md`:
- Line 3: The markdown heading level jumps from H1 to H3 for the release line
"### 10.4.1.0 05/28/2026" which triggers MD001; change that heading to H2
(replace the leading "###" with "##") so the document increments headings
correctly (or insert an appropriate H2 section above it if a subsection is
intended) and ensure the heading text "10.4.1.0 05/28/2026" remains unchanged.
In `@Tests/Xrpl.BinaryCodec.Test/Types/TestIouValueTrailingDot.cs`:
- Around line 6-90: Add the MSTest TestCategory "TestU" to this unit-test suite
so it can be filtered; specifically, annotate the TestIouValueTrailingDot class
with [TestCategory("TestU")] (or apply [TestCategory("TestU")] to each test
method) to tag all tests in this class as unit tests, keeping existing test
attributes and behavior unchanged.
In `@Tests/Xrpl.Tests/Integration/requests/pathFind.cs`:
- Around line 483-489: The helper SubmitTx currently logs the submission result
but does not assert success, so failed setup transactions can go unnoticed;
update SubmitTx to check the transaction result (e.g., inspect
TransactionSummary res and res.Meta?.TransactionResult) and throw or assert when
the result is not a success code (e.g., not "tesSUCCESS"), including the label
in the error message so failing setup transactions fail fast and stop the test
cascade.
- Around line 466-472: The test currently asserts only destination currency and
positive value for best.DestinationAmount; add a check that when the destination
is an issued currency (i.e., destCurrency != "XRP" or when
best.DestinationAmount.Issuer is present) the issuer matches the expected
destination account—e.g., Assert.AreEqual(DestinationAccount,
best.DestinationAmount.Issuer, $"Expected destination issuer
{DestinationAccount}, got {best.DestinationAmount.Issuer}"); place this check
after computing destCurrency and destValue so wrong-issuer token `-1` path-find
results are rejected.
- Around line 216-218: Two integration tests, TestPathFindCreateNegativeOneXrp
and TestPathFindCreateNegativeOneToken, are missing the required TestCategory
attribute; add [TestCategory("TestI")] immediately above each method declaration
(the methods named TestPathFindCreateNegativeOneXrp and
TestPathFindCreateNegativeOneToken in the pathFind.cs test class) so they are
marked as integration tests; ensure the attribute is placed together with the
existing [TestMethod] and [Timeout(...)] attributes.
In `@Tests/Xrpl.Tests/Integration/requests/TestIPathPayment.cs`:
- Around line 624-630: Add an assertion that the destination issuer matches the
expected issuer in the token `-1` scenario: after the existing currency and
positive value checks, assert that best.DestinationAmount.Issuer equals your
expected issuer constant (e.g., ExpectedIssuer or Issuer) using Assert.AreEqual
and include a clear message like $"Expected destination issuer {ExpectedIssuer},
got {best.DestinationAmount.Issuer}"; place this check next to the existing
checks around best.DestinationAmount and CurrencyCode.
- Around line 375-377: Both integration tests TestRipplePathFindNegativeOneXrp
and TestRipplePathFindNegativeOneToken are missing the required
[TestCategory("TestI")] attribute and TestRipplePathFindNegativeOneToken lacks
an assertion for the token destination Issuer; add [TestCategory("TestI")] above
both test methods and, inside TestRipplePathFindNegativeOneToken (the method
that currently asserts CurrencyCode and ValueAsNumber), add an assertion that
the returned destination Issuer equals the expected issuer value (use the same
expected issuer variable or literal used in the test setup) to pin the contract.
---
Outside diff comments:
In `@DocFx/ErrorClassifier.ru.md`:
- Around line 1-162: В разделе описания полей XrplErrorInfo исправьте
грамматику: заменить фразу "устанавливается в `internal error`" на
"устанавливается как `internal error`" (или альтернативно перестроить на
"Значение Title: `internal error`"), и привести соседние буллеты (в частности
про Title и UserMessage) к единому синтаксическому виду — либо все начать с
существительного/фразы ("Title — краткий..."; "UserMessage — читаемое
объяснение..."), либо все в инфинитиве/пассиве, чтобы устранить
последовательность двух разных глагольных форм; отредактируйте упоминания полей
Title и UserMessage соответственно.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3369bbea-ae94-4077-a6f6-00a5b3880698
📒 Files selected for processing (11)
Base/Xrpl.BinaryCodec/ExtenstionHelpers.csBase/Xrpl.BinaryCodec/Types/AmountValue.csBase/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csprojCHANGES.mdDocFx/ErrorClassifier.ru.mdDocFx/index.mdDocFx/toc.ymlTests/Xrpl.BinaryCodec.Test/Types/TestIouValueTrailingDot.csTests/Xrpl.Tests/Integration/requests/TestIPathPayment.csTests/Xrpl.Tests/Integration/requests/pathFind.csXrpl/Xrpl.csproj
| # Changes | ||
|
|
||
| ### 10.4.0.0 (unreleased) | ||
| ### 10.4.1.0 05/28/2026 |
There was a problem hiding this comment.
Fix heading-level jump flagged by markdownlint (MD001).
Line 3 uses ### immediately after the H1 title, which violates heading increment rules.
Suggested fix
-### 10.4.1.0 05/28/2026
+## 10.4.1.0 05/28/2026📝 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
| ### 10.4.1.0 05/28/2026 | |
| ## 10.4.1.0 05/28/2026 |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@CHANGES.md` at line 3, The markdown heading level jumps from H1 to H3 for the
release line "### 10.4.1.0 05/28/2026" which triggers MD001; change that heading
to H2 (replace the leading "###" with "##") so the document increments headings
correctly (or insert an appropriate H2 section above it if a subsection is
intended) and ensure the heading text "10.4.1.0 05/28/2026" remains unchanged.
Comment on lines
+6
to
+90
| [TestClass] | ||
| public class TestIouValueTrailingDot | ||
| { | ||
| private static void AssertEquivalent(string withDot, string canonical) | ||
| { | ||
| IouValue a = IouValue.FromString(withDot); | ||
| IouValue b = IouValue.FromString(canonical); | ||
|
|
||
| Assert.AreEqual(b.Mantissa, a.Mantissa, "mantissa"); | ||
| Assert.AreEqual(b.Exponent, a.Exponent, "exponent"); | ||
| Assert.AreEqual(b.Precision, a.Precision, "precision"); | ||
| Assert.AreEqual(b.IsNegative, a.IsNegative, "sign"); | ||
| CollectionAssert.AreEqual(b.ToBytes(), a.ToBytes(), "ToBytes blob"); | ||
| Assert.AreEqual(b.ToString(), a.ToString(), "ToString"); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void TrailingDot_LargeValue_EquivalentToNoDot() | ||
| { | ||
| // Repro of the XPmarket AMMDeposit case ("128700."). | ||
| AssertEquivalent("128700.", "128700"); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void TrailingDot_SingleDigit_EquivalentToNoDot() | ||
| { | ||
| AssertEquivalent("1.", "1"); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void TrailingDot_NegativeValue_EquivalentToNoDot() | ||
| { | ||
| AssertEquivalent("-42.", "-42"); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void TrailingDot_ToBytesMatchesCanonical() | ||
| { | ||
| byte[] withDot = IouValue.FromString("128700.").ToBytes(); | ||
| byte[] canonical = IouValue.FromString("128700").ToBytes(); | ||
| CollectionAssert.AreEqual(canonical, withDot); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [DataRow("5")] | ||
| [DataRow(".5")] | ||
| [DataRow("-.5")] | ||
| [DataRow("0.5")] | ||
| public void LeadingDot_StillValid(string value) | ||
| { | ||
| // A leading dot with fractional digits stays valid (BigNumber accepts ".5"). | ||
| IouValue parsed = IouValue.FromString(value); | ||
| Assert.IsNotNull(parsed); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [DataRow(".")] | ||
| [DataRow("-.")] | ||
| [DataRow(".e10")] | ||
| [DataRow("+.e-3")] | ||
| public void BareDot_Rejected(string value) | ||
| { | ||
| // A dot with no digit anywhere in the mantissa must not parse (matches BigNumber). | ||
| Assert.ThrowsExactly<InvalidAmountValueException>(() => IouValue.FromString(value)); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [DataRow("100")] | ||
| [DataRow("100.50")] | ||
| [DataRow("0")] | ||
| [DataRow("1.5e10")] | ||
| [DataRow("-0.001")] | ||
| public void ExistingValues_RoundTripUnchanged(string value) | ||
| { | ||
| // Regression guard: previously valid values must parse and round-trip to themselves. | ||
| IouValue parsed = IouValue.FromString(value); | ||
| string serialized = parsed.ToString(); | ||
| IouValue reparsed = IouValue.FromString(serialized); | ||
|
|
||
| Assert.AreEqual(parsed.Mantissa, reparsed.Mantissa, "mantissa"); | ||
| Assert.AreEqual(parsed.Exponent, reparsed.Exponent, "exponent"); | ||
| Assert.AreEqual(parsed.Precision, reparsed.Precision, "precision"); | ||
| CollectionAssert.AreEqual(parsed.ToBytes(), reparsed.ToBytes(), "ToBytes blob"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add required MSTest category tagging for unit-test filtering.
Line 6 onward defines unit tests but no TestU category is applied, so test filtering conventions won’t work as required.
Suggested fix
[TestClass]
+[TestCategory("TestU")]
public class TestIouValueTrailingDotAs per coding guidelines, "Tag unit tests with TestU filter category and integration tests with TestI filter category for test organization".
📝 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
| [TestClass] | |
| public class TestIouValueTrailingDot | |
| { | |
| private static void AssertEquivalent(string withDot, string canonical) | |
| { | |
| IouValue a = IouValue.FromString(withDot); | |
| IouValue b = IouValue.FromString(canonical); | |
| Assert.AreEqual(b.Mantissa, a.Mantissa, "mantissa"); | |
| Assert.AreEqual(b.Exponent, a.Exponent, "exponent"); | |
| Assert.AreEqual(b.Precision, a.Precision, "precision"); | |
| Assert.AreEqual(b.IsNegative, a.IsNegative, "sign"); | |
| CollectionAssert.AreEqual(b.ToBytes(), a.ToBytes(), "ToBytes blob"); | |
| Assert.AreEqual(b.ToString(), a.ToString(), "ToString"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_LargeValue_EquivalentToNoDot() | |
| { | |
| // Repro of the XPmarket AMMDeposit case ("128700."). | |
| AssertEquivalent("128700.", "128700"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_SingleDigit_EquivalentToNoDot() | |
| { | |
| AssertEquivalent("1.", "1"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_NegativeValue_EquivalentToNoDot() | |
| { | |
| AssertEquivalent("-42.", "-42"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_ToBytesMatchesCanonical() | |
| { | |
| byte[] withDot = IouValue.FromString("128700.").ToBytes(); | |
| byte[] canonical = IouValue.FromString("128700").ToBytes(); | |
| CollectionAssert.AreEqual(canonical, withDot); | |
| } | |
| [TestMethod] | |
| [DataRow("5")] | |
| [DataRow(".5")] | |
| [DataRow("-.5")] | |
| [DataRow("0.5")] | |
| public void LeadingDot_StillValid(string value) | |
| { | |
| // A leading dot with fractional digits stays valid (BigNumber accepts ".5"). | |
| IouValue parsed = IouValue.FromString(value); | |
| Assert.IsNotNull(parsed); | |
| } | |
| [TestMethod] | |
| [DataRow(".")] | |
| [DataRow("-.")] | |
| [DataRow(".e10")] | |
| [DataRow("+.e-3")] | |
| public void BareDot_Rejected(string value) | |
| { | |
| // A dot with no digit anywhere in the mantissa must not parse (matches BigNumber). | |
| Assert.ThrowsExactly<InvalidAmountValueException>(() => IouValue.FromString(value)); | |
| } | |
| [TestMethod] | |
| [DataRow("100")] | |
| [DataRow("100.50")] | |
| [DataRow("0")] | |
| [DataRow("1.5e10")] | |
| [DataRow("-0.001")] | |
| public void ExistingValues_RoundTripUnchanged(string value) | |
| { | |
| // Regression guard: previously valid values must parse and round-trip to themselves. | |
| IouValue parsed = IouValue.FromString(value); | |
| string serialized = parsed.ToString(); | |
| IouValue reparsed = IouValue.FromString(serialized); | |
| Assert.AreEqual(parsed.Mantissa, reparsed.Mantissa, "mantissa"); | |
| Assert.AreEqual(parsed.Exponent, reparsed.Exponent, "exponent"); | |
| Assert.AreEqual(parsed.Precision, reparsed.Precision, "precision"); | |
| CollectionAssert.AreEqual(parsed.ToBytes(), reparsed.ToBytes(), "ToBytes blob"); | |
| } | |
| } | |
| [TestClass] | |
| [TestCategory("TestU")] | |
| public class TestIouValueTrailingDot | |
| { | |
| private static void AssertEquivalent(string withDot, string canonical) | |
| { | |
| IouValue a = IouValue.FromString(withDot); | |
| IouValue b = IouValue.FromString(canonical); | |
| Assert.AreEqual(b.Mantissa, a.Mantissa, "mantissa"); | |
| Assert.AreEqual(b.Exponent, a.Exponent, "exponent"); | |
| Assert.AreEqual(b.Precision, a.Precision, "precision"); | |
| Assert.AreEqual(b.IsNegative, a.IsNegative, "sign"); | |
| CollectionAssert.AreEqual(b.ToBytes(), a.ToBytes(), "ToBytes blob"); | |
| Assert.AreEqual(b.ToString(), a.ToString(), "ToString"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_LargeValue_EquivalentToNoDot() | |
| { | |
| // Repro of the XPmarket AMMDeposit case ("128700."). | |
| AssertEquivalent("128700.", "128700"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_SingleDigit_EquivalentToNoDot() | |
| { | |
| AssertEquivalent("1.", "1"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_NegativeValue_EquivalentToNoDot() | |
| { | |
| AssertEquivalent("-42.", "-42"); | |
| } | |
| [TestMethod] | |
| public void TrailingDot_ToBytesMatchesCanonical() | |
| { | |
| byte[] withDot = IouValue.FromString("128700.").ToBytes(); | |
| byte[] canonical = IouValue.FromString("128700").ToBytes(); | |
| CollectionAssert.AreEqual(canonical, withDot); | |
| } | |
| [TestMethod] | |
| [DataRow("5")] | |
| [DataRow(".5")] | |
| [DataRow("-.5")] | |
| [DataRow("0.5")] | |
| public void LeadingDot_StillValid(string value) | |
| { | |
| // A leading dot with fractional digits stays valid (BigNumber accepts ".5"). | |
| IouValue parsed = IouValue.FromString(value); | |
| Assert.IsNotNull(parsed); | |
| } | |
| [TestMethod] | |
| [DataRow(".")] | |
| [DataRow("-.")] | |
| [DataRow(".e10")] | |
| [DataRow("+.e-3")] | |
| public void BareDot_Rejected(string value) | |
| { | |
| // A dot with no digit anywhere in the mantissa must not parse (matches BigNumber). | |
| Assert.ThrowsExactly<InvalidAmountValueException>(() => IouValue.FromString(value)); | |
| } | |
| [TestMethod] | |
| [DataRow("100")] | |
| [DataRow("100.50")] | |
| [DataRow("0")] | |
| [DataRow("1.5e10")] | |
| [DataRow("-0.001")] | |
| public void ExistingValues_RoundTripUnchanged(string value) | |
| { | |
| // Regression guard: previously valid values must parse and round-trip to themselves. | |
| IouValue parsed = IouValue.FromString(value); | |
| string serialized = parsed.ToString(); | |
| IouValue reparsed = IouValue.FromString(serialized); | |
| Assert.AreEqual(parsed.Mantissa, reparsed.Mantissa, "mantissa"); | |
| Assert.AreEqual(parsed.Exponent, reparsed.Exponent, "exponent"); | |
| Assert.AreEqual(parsed.Precision, reparsed.Precision, "precision"); | |
| CollectionAssert.AreEqual(parsed.ToBytes(), reparsed.ToBytes(), "ToBytes blob"); | |
| } | |
| } |
🤖 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/TestIouValueTrailingDot.cs` around lines 6
- 90, Add the MSTest TestCategory "TestU" to this unit-test suite so it can be
filtered; specifically, annotate the TestIouValueTrailingDot class with
[TestCategory("TestU")] (or apply [TestCategory("TestU")] to each test method)
to tag all tests in this class as unit tests, keeping existing test attributes
and behavior unchanged.
Comment on lines
+216
to
+218
| [TestMethod] | ||
| [Timeout(120000)] | ||
| public async Task TestPathFindCreateNegativeOneXrp() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\[TestMethod\]|\[TestCategory\("TestI"\)\]|TestPathFindCreateNegativeOne(Xrp|Token)\(' Tests/Xrpl.Tests/Integration/requests/pathFind.cs -C1Repository: StaticBit-io/XrplCSharp
Length of output: 666
Add [TestCategory("TestI")] to the new integration tests.
TestPathFindCreateNegativeOneXrp and TestPathFindCreateNegativeOneToken are missing the required [TestCategory("TestI")] attribute in Tests/Xrpl.Tests/Integration/requests/pathFind.cs (near lines 216-218 and 350-352).
Suggested patch
[TestMethod]
+ [TestCategory("TestI")]
[Timeout(120000)]
public async Task TestPathFindCreateNegativeOneXrp()
@@
[TestMethod]
+ [TestCategory("TestI")]
[Timeout(120000)]
public async Task TestPathFindCreateNegativeOneToken()📝 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
| [TestMethod] | |
| [Timeout(120000)] | |
| public async Task TestPathFindCreateNegativeOneXrp() | |
| [TestMethod] | |
| [TestCategory("TestI")] | |
| [Timeout(120000)] | |
| public async Task TestPathFindCreateNegativeOneXrp() |
🤖 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/requests/pathFind.cs` around lines 216 - 218,
Two integration tests, TestPathFindCreateNegativeOneXrp and
TestPathFindCreateNegativeOneToken, are missing the required TestCategory
attribute; add [TestCategory("TestI")] immediately above each method declaration
(the methods named TestPathFindCreateNegativeOneXrp and
TestPathFindCreateNegativeOneToken in the pathFind.cs test class) so they are
marked as integration tests; ensure the attribute is placed together with the
existing [TestMethod] and [Timeout(...)] attributes.
Comment on lines
+466
to
+472
| string destCurrency = best.DestinationAmount.CurrencyCode; | ||
| Assert.AreEqual(CurrencyCode, destCurrency, | ||
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | ||
|
|
||
| decimal destValue = best.DestinationAmount.ValueAsNumber; | ||
| Assert.IsTrue(destValue > 0, | ||
| $"Expected positive destination_amount value, got {destValue}"); |
There was a problem hiding this comment.
Also validate destination issuer for token -1 path-find results.
For issued currency, asserting issuer alongside currency/value prevents false positives from a wrong-issuer destination amount.
Suggested patch
string destCurrency = best.DestinationAmount.CurrencyCode;
Assert.AreEqual(CurrencyCode, destCurrency,
$"Expected destination currency {CurrencyCode}, got {destCurrency}");
+ Assert.AreEqual(walletIssuer.ClassicAddress, best.DestinationAmount.Issuer,
+ $"Expected destination issuer {walletIssuer.ClassicAddress}, got {best.DestinationAmount.Issuer}");
decimal destValue = best.DestinationAmount.ValueAsNumber;
Assert.IsTrue(destValue > 0,
$"Expected positive destination_amount value, got {destValue}");📝 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
| string destCurrency = best.DestinationAmount.CurrencyCode; | |
| Assert.AreEqual(CurrencyCode, destCurrency, | |
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | |
| decimal destValue = best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(destValue > 0, | |
| $"Expected positive destination_amount value, got {destValue}"); | |
| string destCurrency = best.DestinationAmount.CurrencyCode; | |
| Assert.AreEqual(CurrencyCode, destCurrency, | |
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | |
| Assert.AreEqual(walletIssuer.ClassicAddress, best.DestinationAmount.Issuer, | |
| $"Expected destination issuer {walletIssuer.ClassicAddress}, got {best.DestinationAmount.Issuer}"); | |
| decimal destValue = best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(destValue > 0, | |
| $"Expected positive destination_amount value, got {destValue}"); |
🤖 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/requests/pathFind.cs` around lines 466 - 472,
The test currently asserts only destination currency and positive value for
best.DestinationAmount; add a check that when the destination is an issued
currency (i.e., destCurrency != "XRP" or when best.DestinationAmount.Issuer is
present) the issuer matches the expected destination account—e.g.,
Assert.AreEqual(DestinationAccount, best.DestinationAmount.Issuer, $"Expected
destination issuer {DestinationAccount}, got {best.DestinationAmount.Issuer}");
place this check after computing destCurrency and destValue so wrong-issuer
token `-1` path-find results are rejected.
Comment on lines
+483
to
+489
| private static async Task SubmitTx(IXrplClient client, ITransactionRequest tx, XrplWallet wallet, string label) | ||
| { | ||
| var autofilled = await client.Autofill(tx); | ||
| TransactionSummary res = await client.SubmitAndWait(autofilled, wallet, true); | ||
| string result = res.Meta?.TransactionResult; | ||
| Console.WriteLine($"[PathFind] {label}: {result}"); | ||
| } |
There was a problem hiding this comment.
Fail fast when setup transactions do not succeed.
Line 486-488 logs the tx result but never asserts it. A failed setup tx can cascade into misleading path-find failures later in the test.
Suggested patch
private static async Task SubmitTx(IXrplClient client, ITransactionRequest tx, XrplWallet wallet, string label)
{
var autofilled = await client.Autofill(tx);
TransactionSummary res = await client.SubmitAndWait(autofilled, wallet, true);
string result = res.Meta?.TransactionResult;
Console.WriteLine($"[PathFind] {label}: {result}");
+ Assert.IsTrue(
+ result == "tesSUCCESS" || result == "terQUEUED",
+ $"Setup transaction '{label}' failed: {result}");
}🤖 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/requests/pathFind.cs` around lines 483 - 489,
The helper SubmitTx currently logs the submission result but does not assert
success, so failed setup transactions can go unnoticed; update SubmitTx to check
the transaction result (e.g., inspect TransactionSummary res and
res.Meta?.TransactionResult) and throw or assert when the result is not a
success code (e.g., not "tesSUCCESS"), including the label in the error message
so failing setup transactions fail fast and stop the test cascade.
Comment on lines
+375
to
+377
| [TestMethod] | ||
| [Timeout(120000)] | ||
| public async Task TestRipplePathFindNegativeOneXrp() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\[TestMethod\]|\[TestCategory\("TestI"\)\]|TestRipplePathFindNegativeOne(Xrp|Token)\(' Tests/Xrpl.Tests/Integration/requests/TestIPathPayment.cs -C1Repository: StaticBit-io/XrplCSharp
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Tests/Xrpl.Tests/Integration/requests/TestIPathPayment.cs"
# Show surrounding code for the two targeted tests
sed -n '350,420p' "$FILE"
echo "----"
sed -n '480,550p' "$FILE"
# See whether the file contains any TestCategory("TestI") anywhere
echo "----"
rg -n 'TestCategory\("TestI"\)' "$FILE" || trueRepository: StaticBit-io/XrplCSharp
Length of output: 6220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Tests/Xrpl.Tests/Integration/requests/TestIPathPayment.cs"
# 1) confirm whether TestCategory("TestI") exists anywhere in the file
rg -n 'TestCategory\("TestI"\)' "$FILE" || true
# 2) show the end of each relevant test for assertion content
sed -n '420,520p' "$FILE" # end part of Xrp test
echo "----"
sed -n '520,700p' "$FILE" # end part of Token testRepository: StaticBit-io/XrplCSharp
Length of output: 10374
Add [TestCategory("TestI")] to the integration tests and assert destination issuer for the token case
TestRipplePathFindNegativeOneXrpandTestRipplePathFindNegativeOneTokenare missing the required[TestCategory("TestI")]attribute, so they won’t be selectable via the integration test filter.TestRipplePathFindNegativeOneTokenonly assertsCurrencyCodeand positiveValueAsNumber; it doesn’t pin the expectedIssuerfor stronger contract coverage.
Suggested patch
[TestMethod]
+ [TestCategory("TestI")]
[Timeout(120000)]
public async Task TestRipplePathFindNegativeOneXrp()
@@
[TestMethod]
+ [TestCategory("TestI")]
[Timeout(120000)]
public async Task TestRipplePathFindNegativeOneToken()🤖 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/requests/TestIPathPayment.cs` around lines 375 -
377, Both integration tests TestRipplePathFindNegativeOneXrp and
TestRipplePathFindNegativeOneToken are missing the required
[TestCategory("TestI")] attribute and TestRipplePathFindNegativeOneToken lacks
an assertion for the token destination Issuer; add [TestCategory("TestI")] above
both test methods and, inside TestRipplePathFindNegativeOneToken (the method
that currently asserts CurrencyCode and ValueAsNumber), add an assertion that
the returned destination Issuer equals the expected issuer value (use the same
expected issuer variable or literal used in the test setup) to pin the contract.
Comment on lines
+624
to
+630
| string destCurrency = best.DestinationAmount.CurrencyCode; | ||
| Assert.AreEqual(CurrencyCode, destCurrency, | ||
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | ||
|
|
||
| decimal destValue = best.DestinationAmount.ValueAsNumber; | ||
| Assert.IsTrue(destValue > 0, | ||
| $"Expected positive destination_amount value, got {destValue}"); |
There was a problem hiding this comment.
Assert destination issuer in the token -1 scenario as well.
The test currently validates destination currency and positivity, but not issuer. Adding issuer assertion tightens the contract for issued-currency results.
Suggested patch
string destCurrency = best.DestinationAmount.CurrencyCode;
Assert.AreEqual(CurrencyCode, destCurrency,
$"Expected destination currency {CurrencyCode}, got {destCurrency}");
+ Assert.AreEqual(walletIssuer.ClassicAddress, best.DestinationAmount.Issuer,
+ $"Expected destination issuer {walletIssuer.ClassicAddress}, got {best.DestinationAmount.Issuer}");
decimal destValue = best.DestinationAmount.ValueAsNumber;
Assert.IsTrue(destValue > 0,
$"Expected positive destination_amount value, got {destValue}");📝 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
| string destCurrency = best.DestinationAmount.CurrencyCode; | |
| Assert.AreEqual(CurrencyCode, destCurrency, | |
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | |
| decimal destValue = best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(destValue > 0, | |
| $"Expected positive destination_amount value, got {destValue}"); | |
| string destCurrency = best.DestinationAmount.CurrencyCode; | |
| Assert.AreEqual(CurrencyCode, destCurrency, | |
| $"Expected destination currency {CurrencyCode}, got {destCurrency}"); | |
| Assert.AreEqual(walletIssuer.ClassicAddress, best.DestinationAmount.Issuer, | |
| $"Expected destination issuer {walletIssuer.ClassicAddress}, got {best.DestinationAmount.Issuer}"); | |
| decimal destValue = best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(destValue > 0, | |
| $"Expected positive destination_amount value, got {destValue}"); |
🤖 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/requests/TestIPathPayment.cs` around lines 624 -
630, Add an assertion that the destination issuer matches the expected issuer in
the token `-1` scenario: after the existing currency and positive value checks,
assert that best.DestinationAmount.Issuer equals your expected issuer constant
(e.g., ExpectedIssuer or Issuer) using Assert.AreEqual and include a clear
message like $"Expected destination issuer {ExpectedIssuer}, got
{best.DestinationAmount.Issuer}"; place this check next to the existing checks
around best.DestinationAmount and CurrencyCode.
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 by CodeRabbit
New Features
Bug Fixes
"128700.") for improved compatibility.Tests
Chores