Enhance documentation and fix IOU amount validation issues#12
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.
📝 WalkthroughWalkthroughThis PR introduces three independent changes: an update to IOU numeric parsing to accept trailing decimal points with refactored regex sharing (version 10.4.1.0), comprehensive Russian documentation and bilingual navigation restructuring for guides, and new integration tests exercising path-finding RPC methods with negative-one destination amounts. ChangesIOU Trailing-Dot Parsing
Documentation and Navigation Updates
Path-Finding Integration Tests
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Base/Xrpl.BinaryCodec/Xrpl.BinaryCodec.csproj`:
- Line 16: Two projects have inconsistent PackageVersion values; update the
<PackageVersion> element in Base/Xrpl.AddressCodec/Xrpl.AddressCodec.csproj and
Base/Xrpl.Keypairs/Xrpl.Keypairs.csproj to match 10.4.1.0 (same as Xrpl and
Xrpl.BinaryCodec). Locate the PackageVersion XML element in each of those
.csproj files and change its value to 10.4.1.0, then save and verify all four
projects now share the identical PackageVersion.
In `@Tests/Xrpl.BinaryCodec.Test/Types/TestIouValueTrailingDot.cs`:
- Around line 6-7: The TestIouValueTrailingDot test class is missing the
required TestU filter category; add the [TestCategory("TestU")] attribute above
the TestIouValueTrailingDot class (alongside the existing [TestClass] attribute)
so the class is tagged for unit-test organization during test discovery and
filtering.
In `@Tests/Xrpl.Tests/Integration/requests/pathFind.cs`:
- Around line 216-218: Add the integration test category attribute to the new
tests: add [TestCategory("TestI")] above the TestPathFindCreateNegativeOneXrp
test method and likewise add [TestCategory("TestI")] to the other new
integration test in this file (the second new test added in the same PR); ensure
the attribute sits alongside the existing [TestMethod] and [Timeout(...)]
attributes so the tests are discoverable by the TestI filter.
- Around line 339-342: The test currently only asserts
response.Alternatives[0].DestinationAmount is non-null; extend this to also
assert that the DestinationAmount represents XRP and that the delivered/amount
value is positive: locate PathAlternative best = response.Alternatives[0] and
add assertions that best.DestinationAmount.Currency (or the equivalent currency
property) equals "XRP" and that the numeric delivered amount/value (e.g.,
best.DestinationAmount.Value or DeliveredAmount) is greater than 0 to ensure the
-1 XRP case yields a positive XRP amount.
- Around line 483-489: SubmitTx currently only logs res.Meta?.TransactionResult
so failed setup transactions can slip by; update SubmitTx to fail fast by
asserting the transaction succeeded: after obtaining TransactionSummary res (in
SubmitTx) check that res?.Meta?.TransactionResult is non-null and equals
"tesSUCCESS" and if not throw an exception (include label and the returned
result in the message) so the test run stops immediately on setup failures
rather than continuing to subsequent path-find assertions.
In `@Tests/Xrpl.Tests/Integration/requests/TestIPathPayment.cs`:
- Around line 375-377: Add the missing TestCategory attribute "TestI" to the
integration test methods so CI/test filtering works; specifically, annotate the
TestRipplePathFindNegativeOneXrp method (and other integration test methods in
this file such as the ones around lines referenced by the reviewer) with
[TestCategory("TestI")] so they are marked as integration tests and picked up by
the TestI filter.
- Around line 498-500: Add assertions after retrieving PathAlternative best =
pathResponse.Alternatives[0] to not only assert best.DestinationAmount is not
null but also validate its semantics: assert the destination currency is XRP
(check the Currency/IsNative property on best.DestinationAmount as used in this
codebase) and assert the numeric amount is > 0 (parse/inspect the
Amount/Value/AsInteger field used by the test). Update TestIPathPayment.cs near
the PathAlternative best / best.DestinationAmount checks to include these two
assertions so malformed-but-present values fail the test.
🪄 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: 0bd44224-07c9-4746-b1ef-3f4a5444ab1c
📒 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
| <PackageProjectUrl>https://github.com/StaticBit-io/XrplCSharp</PackageProjectUrl> | ||
| <Title>XrplCSharp</Title> | ||
| <PackageVersion>10.4.0.0</PackageVersion> | ||
| <PackageVersion>10.4.1.0</PackageVersion> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PackageVersion in all .csproj files
fd -e csproj -x grep -H 'PackageVersion' {} \; | grep -E '(Xrpl|AddressCodec|BinaryCodec|Keypairs)\.csproj'Repository: StaticBit-io/XrplCSharp
Length of output: 410
Update PackageVersion across all release packages (inconsistent versioning)
Per the release guideline, all four projects should be bumped together: Xrpl and Xrpl.BinaryCodec are at 10.4.1.0, but Base/Xrpl.AddressCodec/Xrpl.AddressCodec.csproj and Base/Xrpl.Keypairs/Xrpl.Keypairs.csproj are still 10.3.0.0. Update those two to 10.4.1.0 for consistency.
🤖 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/Xrpl.BinaryCodec.csproj` at line 16, Two projects have
inconsistent PackageVersion values; update the <PackageVersion> element in
Base/Xrpl.AddressCodec/Xrpl.AddressCodec.csproj and
Base/Xrpl.Keypairs/Xrpl.Keypairs.csproj to match 10.4.1.0 (same as Xrpl and
Xrpl.BinaryCodec). Locate the PackageVersion XML element in each of those
.csproj files and change its value to 10.4.1.0, then save and verify all four
projects now share the identical PackageVersion.
Comment on lines
+6
to
+7
| [TestClass] | ||
| public class TestIouValueTrailingDot |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test category attribute for test organization.
The coding guidelines require tagging unit tests with the TestU filter category. Add [TestCategory("TestU")] to this test class.
🏷️ Proposed fix to add test category
[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 | |
| [TestClass] | |
| [TestCategory("TestU")] | |
| public class TestIouValueTrailingDot |
🤖 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
- 7, The TestIouValueTrailingDot test class is missing the required TestU filter
category; add the [TestCategory("TestU")] attribute above the
TestIouValueTrailingDot class (alongside the existing [TestClass] attribute) so
the class is tagged for unit-test organization during test discovery and
filtering.
Comment on lines
+216
to
+218
| [TestMethod] | ||
| [Timeout(120000)] | ||
| public async Task TestPathFindCreateNegativeOneXrp() |
There was a problem hiding this comment.
Add integration test category attributes.
Both new integration tests are missing TestCategory("TestI"), which breaks the test organization convention used for filtering.
Suggested patch
[TestMethod]
[Timeout(120000)]
+[TestCategory("TestI")]
public async Task TestPathFindCreateNegativeOneXrp()
@@
[TestMethod]
[Timeout(120000)]
+[TestCategory("TestI")]
public async Task TestPathFindCreateNegativeOneToken()As per coding guidelines, "Tag unit tests with TestU filter category and integration tests with TestI filter category for test organization".
Also applies to: 350-352
🤖 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,
Add the integration test category attribute to the new tests: add
[TestCategory("TestI")] above the TestPathFindCreateNegativeOneXrp test method
and likewise add [TestCategory("TestI")] to the other new integration test in
this file (the second new test added in the same PR); ensure the attribute sits
alongside the existing [TestMethod] and [Timeout(...)] attributes so the tests
are discoverable by the TestI filter.
Comment on lines
+339
to
+342
| PathAlternative best = response.Alternatives[0]; | ||
| Assert.IsNotNull(best.DestinationAmount, | ||
| "destination_amount should be populated when request uses -1"); | ||
| } |
There was a problem hiding this comment.
Strengthen the XRP -1 assertion to verify type and positivity.
At Line 339-342, the test only checks non-null DestinationAmount. Add assertions for XRP currency and positive delivered amount to protect the contract this test is targeting.
Suggested patch
PathAlternative best = response.Alternatives[0];
Assert.IsNotNull(best.DestinationAmount,
"destination_amount should be populated when request uses -1");
+Assert.AreEqual("XRP", best.DestinationAmount.CurrencyCode ?? "XRP",
+ "Expected XRP destination_amount for XRP -1 request");
+decimal delivered = best.DestinationAmount.ValueAsXrp ?? best.DestinationAmount.ValueAsNumber;
+Assert.IsTrue(delivered > 0, $"Expected positive destination_amount, got {delivered}");📝 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
| PathAlternative best = response.Alternatives[0]; | |
| Assert.IsNotNull(best.DestinationAmount, | |
| "destination_amount should be populated when request uses -1"); | |
| } | |
| PathAlternative best = response.Alternatives[0]; | |
| Assert.IsNotNull(best.DestinationAmount, | |
| "destination_amount should be populated when request uses -1"); | |
| Assert.AreEqual("XRP", best.DestinationAmount.CurrencyCode ?? "XRP", | |
| "Expected XRP destination_amount for XRP -1 request"); | |
| decimal delivered = best.DestinationAmount.ValueAsXrp ?? best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(delivered > 0, $"Expected positive destination_amount, got {delivered}"); | |
| } |
🤖 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 339 - 342,
The test currently only asserts response.Alternatives[0].DestinationAmount is
non-null; extend this to also assert that the DestinationAmount represents XRP
and that the delivered/amount value is positive: locate PathAlternative best =
response.Alternatives[0] and add assertions that best.DestinationAmount.Currency
(or the equivalent currency property) equals "XRP" and that the numeric
delivered amount/value (e.g., best.DestinationAmount.Value or DeliveredAmount)
is greater than 0 to ensure the -1 XRP case yields a positive XRP amount.
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 are unsuccessful.
SubmitTx logs TransactionResult but never asserts success. A failed setup transaction can silently cascade into misleading path-find failures.
Suggested patch
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}");📝 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
| 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}"); | |
| } | |
| 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,
SubmitTx currently only logs res.Meta?.TransactionResult so failed setup
transactions can slip by; update SubmitTx to fail fast by asserting the
transaction succeeded: after obtaining TransactionSummary res (in SubmitTx)
check that res?.Meta?.TransactionResult is non-null and equals "tesSUCCESS" and
if not throw an exception (include label and the returned result in the message)
so the test run stops immediately on setup failures rather than continuing to
subsequent path-find assertions.
Comment on lines
+375
to
+377
| [TestMethod] | ||
| [Timeout(120000)] | ||
| public async Task TestRipplePathFindNegativeOneXrp() |
There was a problem hiding this comment.
Tag these integration tests with TestI.
The newly added integration tests are missing TestCategory("TestI"), which makes CI/test filtering inconsistent.
Suggested patch
[TestMethod]
[Timeout(120000)]
+[TestCategory("TestI")]
public async Task TestRipplePathFindNegativeOneXrp()
@@
[TestMethod]
[Timeout(120000)]
+[TestCategory("TestI")]
public async Task TestRipplePathFindNegativeOneToken()As per coding guidelines, "Tag unit tests with TestU filter category and integration tests with TestI filter category for test organization".
Also applies to: 508-510
🤖 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, Add the missing TestCategory attribute "TestI" to the integration test
methods so CI/test filtering works; specifically, annotate the
TestRipplePathFindNegativeOneXrp method (and other integration test methods in
this file such as the ones around lines referenced by the reviewer) with
[TestCategory("TestI")] so they are marked as integration tests and picked up by
the TestI filter.
Comment on lines
+498
to
+500
| PathAlternative best = pathResponse.Alternatives[0]; | ||
| Assert.IsNotNull(best.DestinationAmount, | ||
| "destination_amount should be populated in alternatives when request uses -1"); |
There was a problem hiding this comment.
Validate XRP destination_amount semantics in the XRP -1 test.
At Line 498-500, add checks for expected XRP currency and positive amount so the test catches malformed-but-present values.
Suggested patch
PathAlternative best = pathResponse.Alternatives[0];
Assert.IsNotNull(best.DestinationAmount,
"destination_amount should be populated in alternatives when request uses -1");
+Assert.AreEqual("XRP", best.DestinationAmount.CurrencyCode ?? "XRP",
+ "Expected XRP destination_amount for XRP -1 request");
+decimal delivered = best.DestinationAmount.ValueAsXrp ?? best.DestinationAmount.ValueAsNumber;
+Assert.IsTrue(delivered > 0, $"Expected positive destination_amount, got {delivered}");📝 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
| PathAlternative best = pathResponse.Alternatives[0]; | |
| Assert.IsNotNull(best.DestinationAmount, | |
| "destination_amount should be populated in alternatives when request uses -1"); | |
| PathAlternative best = pathResponse.Alternatives[0]; | |
| Assert.IsNotNull(best.DestinationAmount, | |
| "destination_amount should be populated in alternatives when request uses -1"); | |
| Assert.AreEqual("XRP", best.DestinationAmount.CurrencyCode ?? "XRP", | |
| "Expected XRP destination_amount for XRP -1 request"); | |
| decimal delivered = best.DestinationAmount.ValueAsXrp ?? best.DestinationAmount.ValueAsNumber; | |
| Assert.IsTrue(delivered > 0, $"Expected positive destination_amount, got {delivered}"); |
🤖 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 498 -
500, Add assertions after retrieving PathAlternative best =
pathResponse.Alternatives[0] to not only assert best.DestinationAmount is not
null but also validate its semantics: assert the destination currency is XRP
(check the Currency/IsNative property on best.DestinationAmount as used in this
codebase) and assert the numeric amount is > 0 (parse/inspect the
Amount/Value/AsInteger field used by the test). Update TestIPathPayment.cs near
the PathAlternative best / best.DestinationAmount checks to include these two
assertions so malformed-but-present values fail the test.
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
Release Notes
Bug Fixes
Documentation