Conversation
…r RPCs: amm_info, ledger_entry and book_offers
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Multi-Purpose Token (MPT) support: config amendment, binarycodec PathSet/field additions, MPT amount validation and constants, request/transaction model changes (AMM, AMMClawback, LedgerEntry), async test helpers, and many unit/integration tests exercising MPT flows. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant AH as create_mpt_token_and_authorize_source_async
participant C as Async Client
participant L as Ledger
participant AO as AccountObjects
TR->>AH: call(issuer, source, client)
AH->>C: submit MPTokenIssuanceCreate
C->>L: sign & submit tx
L-->>C: engine_result (tesSUCCESS)
C-->>AH: submission result
AH->>AO: query AccountObjects(issuer)
AO->>L: fetch objects
L-->>AO: return objects with mpt_issuance_id
AO-->>AH: mpt_issuance_id
AH->>C: submit MPTokenAuthorize (authorize source)
C->>L: sign & submit tx
L-->>C: engine_result (tesSUCCESS)
C-->>AH: submission result
AH->>C: submit Payment (fund source)
C->>L: sign & submit tx
L-->>C: engine_result (tesSUCCESS)
C-->>AH: submission result
AH-->>TR: return mpt_issuance_id
sequenceDiagram
participant TR as Test Runner
participant AMH as create_amm_pool_with_mpt_async
participant W as Wallet Factory
participant F as fund_wallet_async
participant MP as create_mpt_token_and_authorize_source_async
participant C as Async Client
participant L as Ledger
TR->>AMH: call(client)
AMH->>W: create issuer1, issuer2, lp_wallet
W-->>AMH: wallets
AMH->>F: fund wallets
F->>C: submit funding txs
C->>L: sign & submit
L-->>C: funding success
C-->>AMH: funded
AMH->>MP: create MPT for issuer1
MP->>C: submit issuance + authorize
C->>L: sign & submit
L-->>C: mpt_issuance_id_1
C-->>MP: result
MP-->>AMH: asset (MPTCurrency)
AMH->>MP: create MPT for issuer2
MP->>C: submit issuance + authorize
C->>L: sign & submit
L-->>C: mpt_issuance_id_2
MP-->>AMH: asset2 (MPTCurrency)
AMH->>C: submit AMMCreate(asset, asset2)
C->>L: sign & submit
L-->>C: engine_result (tesSUCCESS)
C-->>AMH: AMMCreate result
AMH-->>TR: return {asset, asset2, issuer_wallet_1, issuer_wallet_2, lp_wallet}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xrpl/models/transactions/amm_clawback.py (1)
53-57:⚠️ Potential issue | 🟡 MinorStale docstring on
assetfield — inaccurate forMPTCurrency.The docstring still says "this is an object with currency and issuer fields. The issuer field must match with Account."
MPTCurrencyhas onlympt_issuance_id; there is noissuerfield. This will mislead callers using MPT assets.📝 Proposed docstring fix
asset: Union[IssuedCurrency, MPTCurrency] = REQUIRED """ - Specifies the asset that the issuer wants to claw back from the AMM pool. In JSON, - this is an object with currency and issuer fields. The issuer field must match with - Account. + Specifies the asset that the issuer wants to claw back from the AMM pool. + For an issued currency, this is an object with ``currency`` and ``issuer`` fields, + and the ``issuer`` field must match the ``account`` field. + For an MPT asset, this is an object with ``mpt_issuance_id``. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/models/transactions/amm_clawback.py` around lines 53 - 57, The docstring for the asset field in the AMMClawback model is stale: update the docstring for the asset attribute (asset) in amm_clawback.py / AMMClawback to reflect that when the asset is an MPTCurrency it contains only mpt_issuance_id (no issuer or issuer field), and clarify the expected shape for regular XFUs (currency + issuer) vs MPTCurrency (mpt_issuance_id only); remove the incorrect statement that "issuer field must match with Account" for MPTCurrency and add a short example or wording stating that issuer is not applicable for MPTCurrency.
🧹 Nitpick comments (3)
.ci-config/rippled.cfg (1)
210-211:MPTokensV2lacks a release version grouping comment.All other amendment blocks have a
# X.Y.0 Amendmentsheader. Adding a comment like# 2.6.0 Amendments(or whichever release introduced this) would maintain consistency with the existing pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ci-config/rippled.cfg around lines 210 - 211, Add a release-version grouping comment above the MPTokensV2 amendment block to match the existing pattern (e.g., insert a line like "# 2.6.0 Amendments" immediately before the MPTokensV2 token declaration). Locate the MPTokensV2 amendment entry and add the corresponding "# X.Y.0 Amendments" header that reflects the release which introduced MPTokensV2 so it remains consistent with other amendment blocks.xrpl/models/transactions/amm_clawback.py (1)
103-114: Cross-type validation gap:IssuedCurrencyasset paired with anMPTAmountgoes unchecked.When
assetisIssuedCurrencyandamountis anMPTAmount,isinstance(self.amount, IssuedCurrencyAmount)isFalse, so the amount/asset consistency check is silently skipped. The server will reject this combination, but adding a simple client-side guard would surface the error earlier.🛡️ Optional client-side guard
if isinstance(self.asset, IssuedCurrency): if self.account != self.asset.issuer: errors += ( "Asset.issuer and AMMClawback transaction sender must be identical." ) if ( self.amount is not None and isinstance(self.amount, IssuedCurrencyAmount) and ( self.amount.issuer != self.asset.issuer or self.amount.currency != self.asset.currency ) ): errors += ( "Amount.issuer and Amount.currency must match corresponding Asset " + "fields." ) + elif self.amount is not None and not isinstance( + self.amount, IssuedCurrencyAmount + ): + errors += "Amount type must match Asset type (IssuedCurrencyAmount for IssuedCurrency asset)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/models/transactions/amm_clawback.py` around lines 103 - 114, The validation currently only checks amount/asset consistency when amount is an IssuedCurrencyAmount; add a guard in the same validation method (the block referencing self.amount, IssuedCurrencyAmount, and self.asset) to also detect when self.asset is an IssuedCurrency and self.amount is an MPTAmount (or otherwise not an IssuedCurrencyAmount) and append the same or a clear error like "Amount.issuer and Amount.currency must match corresponding Asset fields." so the client rejects an IssuedCurrency asset paired with an MPTAmount early.tests/integration/reqs/test_path_find.py (1)
77-77: PreferassertGreateroverassertTrue(len(...) > 0)for consistency.Other tests in this PR (e.g.,
test_ripple_path_find.pyline 92) useassertGreaterfor the same kind of check, which also gives better failure messages.Proposed fix
- self.assertTrue(len(response.result["alternatives"]) > 0) + self.assertGreater(len(response.result["alternatives"]), 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/reqs/test_path_find.py` at line 77, Replace the assertion that uses assertTrue(len(response.result["alternatives"]) > 0) with assertGreater(len(response.result["alternatives"]), 0) to match other tests and provide better failure messages; locate the assertion in tests/integration/reqs/test_path_find.py (the line that checks response.result["alternatives"]) and update the test accordingly so it uses unittest.TestCase.assertGreater.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/it_utils.py`:
- Around line 646-711: The async helper
create_mpt_token_and_authorize_source_async checks
response.result["engine_result"] for MPTokenAuthorize and Payment and raises
XRPLException on failure, but the sync counterpart
create_mpt_token_and_authorize_source does not; make the sync version consistent
by adding the same engine_result checks and XRPLException raises after the
MPTokenAuthorize submission (reference MPTokenAuthorize,
sign_and_reliable_submission) and after the Payment submission (reference
Payment, sign_and_reliable_submission), using equivalent error messages so
failures surface immediately; ensure XRPLException is available in that module
if not already imported.
In `@tests/integration/reqs/test_amm_info.py`:
- Around line 2-6: The import line bringing in create_amm_pool_with_mpt is
flagged as unused by Flake8 but is required by the test generator; update the
import statement that includes create_amm_pool_with_mpt,
create_amm_pool_with_mpt_async, and test_async_and_sync in
tests/integration/reqs/test_amm_info.py to append "# noqa: F401" to suppress the
unused-import warning for create_amm_pool_with_mpt (follow the established
pattern used for xrpl in it_utils.py).
In `@tests/integration/reqs/test_book_offers.py`:
- Around line 76-80: Black flags the multi-line list comprehension assigned to
matching; reformat it into a single-line or Black-compliant multi-line form.
Replace the current block that creates matching (the list comprehension using
offers and source.classic_address) with a one-line comprehension like matching =
[o for o in offers if o["Account"] == source.classic_address] or wrap the
expression with parentheses and proper indentation so Black no longer wants to
change it, keeping the subsequent self.assertEqual(len(matching), 1) unchanged.
- Around line 2-14: The test imports only async helpers used by the
test_async_and_sync decorator, causing NameError on the sync path; import the
sync counterparts (create_mpt_token_and_authorize_source, fund_wallet,
sign_and_reliable_submission) alongside the async ones from
tests.integration.it_utils so both async and sync names are available when
test_async_and_sync rewrites/executes the sync variant; update the import list
in test_book_offers.py to include these three sync functions (referencing
create_mpt_token_and_authorize_source, fund_wallet, sign_and_reliable_submission
and the test_async_and_sync decorator).
In `@tests/integration/reqs/test_ledger_entry.py`:
- Around line 2-6: The sync helper create_amm_pool_with_mpt is imported only to
be injected into globals() for the test_async_and_sync decorator but is flagged
as unused by linters; update the import line that includes
create_amm_pool_with_mpt to append a noqa: F401 comment so flake8 ignores the
unused-import warning (leave test_async_and_sync and
create_amm_pool_with_mpt_async intact and ensure create_amm_pool_with_mpt
remains exported for globals()).
In `@tests/integration/reqs/test_path_find.py`:
- Around line 2-13: Mark the unused sync helper imports
create_mpt_token_and_authorize_source and fund_wallet with "# noqa: F401" to
satisfy F401, and reorder and wrap the long import line that contains
create_mpt_token_and_authorize_source_async, fund_wallet_async,
sign_and_reliable_submission_async, test_async_and_sync so it follows the
project's import ordering rules and 88-char line limit (split into multiple
lines or group imports) to fix E501/BLK100/I001; ensure all model imports
(MPTAmount, PathFind, PathFindSubcommand, MPTokenAuthorize,
MPTokenIssuanceCreateFlag, OfferCreate, Wallet) are grouped and sorted
consistently with the rest of the file.
In `@tests/integration/reqs/test_ripple_path_find.py`:
- Around line 2-10: The three synchronous helper imports
(create_mpt_token_and_authorize_source, fund_wallet,
sign_and_reliable_submission) are flagged as unused by flake8 even though they
are injected into globals() for the test_async_and_sync decorator; update the
import line in tests/integration/reqs/test_ripple_path_find.py to suppress the
false-positive by adding "# noqa: F401" to the import (or to the specific
imported names) so F401 is ignored while keeping the async counterparts and
test_async_and_sync unchanged.
In `@tests/integration/transactions/test_amm_lifecycle_with_mpt.py`:
- Around line 2-6: The import statement in the test file is missing the sync
counterpart functions required by the test_async_and_sync decorator. Add the
sync variants create_amm_pool_with_mpt and sign_and_reliable_submission to the
import statement from tests.integration.it_utils alongside their async
counterparts, so that both versions are available in globals() when the
test_async_and_sync decorator executes the sync test path.
In `@tests/integration/transactions/test_check_create.py`:
- Around line 13-18: Remove the genuinely unused imports MPTokenAuthorize and
MPTokenIssuanceCreate from the import list in
tests/integration/transactions/test_check_create.py; keep the required imports
(e.g., CheckCreate and MPTokenIssuanceCreateFlag) and ensure the import
statement only includes symbols actually referenced by tests and helper
functions like create_mpt_token_and_authorize_source_async so the pipeline no
longer fails due to unused imports.
In `@tests/integration/transactions/test_offer_create.py`:
- Around line 71-75: The multiline call to client.request creating a LedgerEntry
with an embedded Offer (LedgerEntry(... Offer(...))) is missing a trailing comma
which causes Black to want to reformat; update the call so the final argument
list has a trailing comma (i.e., add a comma after the closing parenthesis of
Offer in the LedgerEntry call) so the code conforms to Black formatting for
multi-line calls involving LedgerEntry and Offer.
---
Outside diff comments:
In `@xrpl/models/transactions/amm_clawback.py`:
- Around line 53-57: The docstring for the asset field in the AMMClawback model
is stale: update the docstring for the asset attribute (asset) in
amm_clawback.py / AMMClawback to reflect that when the asset is an MPTCurrency
it contains only mpt_issuance_id (no issuer or issuer field), and clarify the
expected shape for regular XFUs (currency + issuer) vs MPTCurrency
(mpt_issuance_id only); remove the incorrect statement that "issuer field must
match with Account" for MPTCurrency and add a short example or wording stating
that issuer is not applicable for MPTCurrency.
---
Nitpick comments:
In @.ci-config/rippled.cfg:
- Around line 210-211: Add a release-version grouping comment above the
MPTokensV2 amendment block to match the existing pattern (e.g., insert a line
like "# 2.6.0 Amendments" immediately before the MPTokensV2 token declaration).
Locate the MPTokensV2 amendment entry and add the corresponding "# X.Y.0
Amendments" header that reflects the release which introduced MPTokensV2 so it
remains consistent with other amendment blocks.
In `@tests/integration/reqs/test_path_find.py`:
- Line 77: Replace the assertion that uses
assertTrue(len(response.result["alternatives"]) > 0) with
assertGreater(len(response.result["alternatives"]), 0) to match other tests and
provide better failure messages; locate the assertion in
tests/integration/reqs/test_path_find.py (the line that checks
response.result["alternatives"]) and update the test accordingly so it uses
unittest.TestCase.assertGreater.
In `@xrpl/models/transactions/amm_clawback.py`:
- Around line 103-114: The validation currently only checks amount/asset
consistency when amount is an IssuedCurrencyAmount; add a guard in the same
validation method (the block referencing self.amount, IssuedCurrencyAmount, and
self.asset) to also detect when self.asset is an IssuedCurrency and self.amount
is an MPTAmount (or otherwise not an IssuedCurrencyAmount) and append the same
or a clear error like "Amount.issuer and Amount.currency must match
corresponding Asset fields." so the client rejects an IssuedCurrency asset
paired with an MPTAmount early.
| from tests.integration.it_utils import ( | ||
| create_amm_pool_with_mpt, | ||
| create_amm_pool_with_mpt_async, | ||
| test_async_and_sync, | ||
| ) |
There was a problem hiding this comment.
Unused import causes pipeline failure — add # noqa: F401.
create_amm_pool_with_mpt is reported as unused by Flake8 (F401) and is breaking the CI pipeline. However, the import IS needed: the test_async_and_sync decorator auto-generates a sync version by replacing create_amm_pool_with_mpt_async( → create_amm_pool_with_mpt(. The fix follows the established codebase pattern (e.g., import xrpl # noqa: F401 - needed for sync tests in it_utils.py line 10).
🔧 Proposed fix
from tests.integration.it_utils import (
- create_amm_pool_with_mpt,
+ create_amm_pool_with_mpt, # noqa: F401 - used by test_async_and_sync generated sync code
create_amm_pool_with_mpt_async,
test_async_and_sync,
)📝 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.
| from tests.integration.it_utils import ( | |
| create_amm_pool_with_mpt, | |
| create_amm_pool_with_mpt_async, | |
| test_async_and_sync, | |
| ) | |
| from tests.integration.it_utils import ( | |
| create_amm_pool_with_mpt, # noqa: F401 - used by test_async_and_sync generated sync code | |
| create_amm_pool_with_mpt_async, | |
| test_async_and_sync, | |
| ) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: 'tests.integration.it_utils.create_amm_pool_with_mpt' imported but unused
(F401)
🪛 GitHub Actions: Unit test
[error] 2-2: F401 'tests.integration.it_utils.create_amm_pool_with_mpt' imported but unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/reqs/test_amm_info.py` around lines 2 - 6, The import line
bringing in create_amm_pool_with_mpt is flagged as unused by Flake8 but is
required by the test generator; update the import statement that includes
create_amm_pool_with_mpt, create_amm_pool_with_mpt_async, and
test_async_and_sync in tests/integration/reqs/test_amm_info.py to append "#
noqa: F401" to suppress the unused-import warning for create_amm_pool_with_mpt
(follow the established pattern used for xrpl in it_utils.py).
There was a problem hiding this comment.
this error has not been observed in the CI tests or the local integ tests.
There was a problem hiding this comment.
@ckeshava You're absolutely right, and I apologize for the confusion. Based on the learning I have about the test_async_and_sync decorator in this codebase, explicit imports of sync counterpart functions are not required. The decorator automatically merges the test module's globals with it_utils.py's globals and performs string replacement to convert async function calls to sync ones.
Since the test only uses create_amm_pool_with_mpt_async directly (line 36), and the decorator handles making the sync version available during execution, the import of create_amm_pool_with_mpt on line 3 is indeed unnecessary and can be safely removed:
from tests.integration.it_utils import (
create_amm_pool_with_mpt_async,
test_async_and_sync,
)If CI isn't flagging it as a failure, you can leave it as-is or clean it up in a follow-up. Thank you for the correction!
🧠 Learnings used
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 914
File: tests/integration/transactions/test_amm_lifecycle_with_mpt.py:2-6
Timestamp: 2026-02-18T20:49:08.933Z
Learning: In xrpl-py integration tests using the test_async_and_sync decorator from tests.integration.it_utils, explicit imports of sync counterpart functions (e.g., create_amm_pool_with_mpt, sign_and_reliable_submission) are not required in test files. The decorator automatically merges the test module's globals with it_utils.py's globals (which contains both sync and async versions) and uses string replacement to convert async function calls to sync ones, making all helpers available during test execution.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: mvadari
Repo: XRPLF/xrpl-py PR: 759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 900
File: tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json:5060-5076
Timestamp: 2026-02-04T22:54:47.537Z
Learning: In XRPLF/xrpl-py (PR `#900`), codec fixtures and Number.to_json do not need to force Number-typed fields (e.g., VaultCreate.AssetsMaximum) into the [1e18, 1e19−1] mantissa range; non-normalized numeric strings are acceptable as inputs, with normalization handled during serialization/ledger storage.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
| from tests.integration.it_utils import ( | ||
| create_amm_pool_with_mpt, | ||
| create_amm_pool_with_mpt_async, | ||
| test_async_and_sync, | ||
| ) |
There was a problem hiding this comment.
Unused-import pipeline failure — suppress with # noqa: F401.
create_amm_pool_with_mpt (sync) is required in globals() for the test_async_and_sync decorator but is flagged as unused by flake8.
Proposed fix
from tests.integration.it_utils import (
- create_amm_pool_with_mpt,
+ create_amm_pool_with_mpt, # noqa: F401
create_amm_pool_with_mpt_async,
test_async_and_sync,
)📝 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.
| from tests.integration.it_utils import ( | |
| create_amm_pool_with_mpt, | |
| create_amm_pool_with_mpt_async, | |
| test_async_and_sync, | |
| ) | |
| from tests.integration.it_utils import ( | |
| create_amm_pool_with_mpt, # noqa: F401 | |
| create_amm_pool_with_mpt_async, | |
| test_async_and_sync, | |
| ) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: 'tests.integration.it_utils.create_amm_pool_with_mpt' imported but unused
(F401)
🪛 GitHub Actions: Unit test
[error] 2-2: F401 'tests.integration.it_utils.create_amm_pool_with_mpt' imported but unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/reqs/test_ledger_entry.py` around lines 2 - 6, The sync
helper create_amm_pool_with_mpt is imported only to be injected into globals()
for the test_async_and_sync decorator but is flagged as unused by linters;
update the import line that includes create_amm_pool_with_mpt to append a noqa:
F401 comment so flake8 ignores the unused-import warning (leave
test_async_and_sync and create_amm_pool_with_mpt_async intact and ensure
create_amm_pool_with_mpt remains exported for globals()).
| from tests.integration.it_utils import ( | ||
| create_mpt_token_and_authorize_source, | ||
| create_mpt_token_and_authorize_source_async, | ||
| fund_wallet, | ||
| fund_wallet_async, | ||
| sign_and_reliable_submission, | ||
| sign_and_reliable_submission_async, | ||
| test_async_and_sync, | ||
| ) |
There was a problem hiding this comment.
Unused-import pipeline failures — suppress with # noqa: F401.
The sync counterparts (create_mpt_token_and_authorize_source, fund_wallet, sign_and_reliable_submission) are needed in globals() for the test_async_and_sync decorator but flake8 flags them as unused, causing the pipeline to fail.
Proposed fix
from tests.integration.it_utils import (
- create_mpt_token_and_authorize_source,
+ create_mpt_token_and_authorize_source, # noqa: F401
create_mpt_token_and_authorize_source_async,
- fund_wallet,
+ fund_wallet, # noqa: F401
fund_wallet_async,
- sign_and_reliable_submission,
+ sign_and_reliable_submission, # noqa: F401
sign_and_reliable_submission_async,
test_async_and_sync,
)📝 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.
| from tests.integration.it_utils import ( | |
| create_mpt_token_and_authorize_source, | |
| create_mpt_token_and_authorize_source_async, | |
| fund_wallet, | |
| fund_wallet_async, | |
| sign_and_reliable_submission, | |
| sign_and_reliable_submission_async, | |
| test_async_and_sync, | |
| ) | |
| from tests.integration.it_utils import ( | |
| create_mpt_token_and_authorize_source, # noqa: F401 | |
| create_mpt_token_and_authorize_source_async, | |
| fund_wallet, # noqa: F401 | |
| fund_wallet_async, | |
| sign_and_reliable_submission, # noqa: F401 | |
| sign_and_reliable_submission_async, | |
| test_async_and_sync, | |
| ) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: 'tests.integration.it_utils.create_mpt_token_and_authorize_source' imported but unused
(F401)
[error] 2-2: 'tests.integration.it_utils.fund_wallet' imported but unused
(F401)
[error] 2-2: 'tests.integration.it_utils.sign_and_reliable_submission' imported but unused
(F401)
🪛 GitHub Actions: Unit test
[error] 2-2: F401 'tests.integration.it_utils.create_mpt_token_and_authorize_source' imported but unused
[error] 2-2: F401 'tests.integration.it_utils.fund_wallet' imported but unused
[error] 2-2: F401 'tests.integration.it_utils.sign_and_reliable_submission' imported but unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/reqs/test_ripple_path_find.py` around lines 2 - 10, The
three synchronous helper imports (create_mpt_token_and_authorize_source,
fund_wallet, sign_and_reliable_submission) are flagged as unused by flake8 even
though they are injected into globals() for the test_async_and_sync decorator;
update the import line in tests/integration/reqs/test_ripple_path_find.py to
suppress the false-positive by adding "# noqa: F401" to the import (or to the
specific imported names) so F401 is ignored while keeping the async counterparts
and test_async_and_sync unchanged.
| from tests.integration.it_utils import ( | ||
| create_amm_pool_with_mpt_async, | ||
| sign_and_reliable_submission_async, | ||
| test_async_and_sync, | ||
| ) |
There was a problem hiding this comment.
Missing sync counterpart imports for test_async_and_sync decorator.
Same issue as in test_book_offers.py — the sync variants (create_amm_pool_with_mpt, sign_and_reliable_submission) are not imported into globals(), which the decorator likely needs for the sync test path.
Proposed fix
from tests.integration.it_utils import (
+ create_amm_pool_with_mpt,
create_amm_pool_with_mpt_async,
+ sign_and_reliable_submission,
sign_and_reliable_submission_async,
test_async_and_sync,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/transactions/test_amm_lifecycle_with_mpt.py` around lines 2
- 6, The import statement in the test file is missing the sync counterpart
functions required by the test_async_and_sync decorator. Add the sync variants
create_amm_pool_with_mpt and sign_and_reliable_submission to the import
statement from tests.integration.it_utils alongside their async counterparts, so
that both versions are available in globals() when the test_async_and_sync
decorator executes the sync test path.
There was a problem hiding this comment.
all the integration tests pass at my end. no error is observed.
The it_utils.py file imports all the globals() in the constructed decorator code. Hence, explicit imports are not necessary
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the test_async_and_sync decorator implementation in it_utils.py
cat tests/integration/it_utils.py | grep -A 50 "def test_async_and_sync"
Length of output: 2103
@ckeshava Thank you for the clarification! You're absolutely right. I can see from the test_async_and_sync decorator implementation that it merges original_globals (the test module's globals passed via test_async_and_sync(globals())) with globals() from it_utils.py itself:
all_modules = {**original_globals, **globals(), **sync_modules_to_import}Since it_utils.py already imports both the sync and async versions of the helper functions, they're all available in the merged namespace when exec() runs the converted sync code. The string replacement .replace("_async(", "(") handles converting the function calls, so explicit imports in the test file aren't necessary.
I apologize for the incorrect comment. The integration tests passing confirms this mechanism works correctly!
✏️ Learnings added
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 914
File: tests/integration/transactions/test_amm_lifecycle_with_mpt.py:2-6
Timestamp: 2026-02-18T20:49:08.933Z
Learning: In xrpl-py integration tests using the test_async_and_sync decorator from tests.integration.it_utils, explicit imports of sync counterpart functions (e.g., create_amm_pool_with_mpt, sign_and_reliable_submission) are not required in test files. The decorator automatically merges the test module's globals with it_utils.py's globals (which contains both sync and async versions) and uses string replacement to convert async function calls to sync ones, making all helpers available during test execution.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 900
File: tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json:5060-5076
Timestamp: 2026-02-04T22:54:47.537Z
Learning: In XRPLF/xrpl-py (PR `#900`), codec fixtures and Number.to_json do not need to force Number-typed fields (e.g., VaultCreate.AssetsMaximum) into the [1e18, 1e19−1] mantissa range; non-normalized numeric strings are acceptable as inputs, with normalization handled during serialization/ledger storage.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
| ledger_entry_response = await client.request( | ||
| LedgerEntry( | ||
| offer=Offer(account=source.classic_address, seq=offer_seq) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Black formatting issue (pipeline failure at line 72).
The pipeline reports BLK100 Black would make changes at line 72.
Proposed fix — add trailing comma
ledger_entry_response = await client.request(
- LedgerEntry(
- offer=Offer(account=source.classic_address, seq=offer_seq)
- )
+ LedgerEntry(
+ offer=Offer(account=source.classic_address, seq=offer_seq),
+ ),
)📝 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.
| ledger_entry_response = await client.request( | |
| LedgerEntry( | |
| offer=Offer(account=source.classic_address, seq=offer_seq) | |
| ) | |
| ) | |
| ledger_entry_response = await client.request( | |
| LedgerEntry( | |
| offer=Offer(account=source.classic_address, seq=offer_seq), | |
| ), | |
| ) |
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 72-72: BLK100 Black would make changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/transactions/test_offer_create.py` around lines 71 - 75,
The multiline call to client.request creating a LedgerEntry with an embedded
Offer (LedgerEntry(... Offer(...))) is missing a trailing comma which causes
Black to want to reformat; update the call so the final argument list has a
trailing comma (i.e., add a comma after the closing parenthesis of Offer in the
LedgerEntry call) so the code conforms to Black formatting for multi-line calls
involving LedgerEntry and Offer.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/unit/models/transactions/test_amm_create.py (1)
57-91: Consider consolidating standaloneMPTAmountvalidation tests into a dedicated file.
test_mpt_issuance_id_non_hex_characters,test_mpt_issuance_id_too_short, andtest_mpt_issuance_id_too_longconstructMPTAmountdirectly and never touchAMMCreate. Functionally identical copies of these three tests appear intest_amm_withdraw.py,test_amm_deposit.py, andtest_check_cash.py— 12 near-identical tests spread across 4 files for the sameMPTAmount._get_errorsvalidation path.Moving them to a single
tests/unit/models/amounts/test_mpt_amount.py(or similar) would reduce noise in the transaction-specific test files and make the coverage explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/transactions/test_amm_create.py` around lines 57 - 91, Move the three standalone MPTAmount validation tests out of transaction-specific files and consolidate them into a single unit test module that targets MPTAmount directly (e.g., create tests/unit/models/amounts/test_mpt_amount.py); specifically, copy/centralize test_mpt_issuance_id_non_hex_characters, test_mpt_issuance_id_too_short, and test_mpt_issuance_id_too_long into a new file that imports MPTAmount and MPT_ISSUANCE_ID_LENGTH and asserts the same XRPLModelException messages, then remove the duplicate copies from test_amm_create.py, test_amm_withdraw.py, test_amm_deposit.py, and test_check_cash.py so the transaction tests no longer instantiate MPTAmount directly and the validation coverage remains centralized.tests/integration/transactions/test_offer_create.py (2)
39-39: Optional: normalize test method name to lowercase snake_case.
test_offer_create_with_MPTmixes casing; other methods in the class (test_basic_functionality,test_deep_freeze_trustline_fails) are all lowercase. Rename totest_offer_create_with_mptfor consistency.♻️ Proposed rename
- async def test_offer_create_with_MPT(self, client): + async def test_offer_create_with_mpt(self, client):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_offer_create.py` at line 39, Rename the test function test_offer_create_with_MPT to lowercase snake_case test_offer_create_with_mpt; update the function definition and any references/calls (e.g., in test discovery, parametrization, fixtures, or marks) to use test_offer_create_with_mpt so it matches the other methods like test_basic_functionality and test_deep_freeze_trustline_fails and follows the project's naming convention.
65-66: Optional: remove redundant success assertion.
response.is_successful()andengine_result == "tesSUCCESS"check the same condition. The existingtest_basic_functionalityin this class uses onlyassertTrue(offer.is_successful()). Keeping one is sufficient.♻️ Proposed simplification
- self.assertTrue(response.is_successful()) self.assertEqual(response.result["engine_result"], "tesSUCCESS")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_offer_create.py` around lines 65 - 66, The two assertions are redundant—response.is_successful() and checking response.result["engine_result"] == "tesSUCCESS" verify the same success condition; simplify the test by removing one of them (prefer keeping response.is_successful() to match the style used in test_basic_functionality which uses offer.is_successful()). Update the assertions in test_offer_create.py so only a single success assertion remains (e.g., keep response.is_successful()) and remove the explicit engine_result equality check.tests/integration/it_utils.py (1)
608-721: Sync/async consistency resolved; optional Ruff TRY003 cleanup.The previously flagged inconsistency between the sync and async variants is now addressed — both apply
XRPLExceptionguards at all three submission points (IssuanceCreate → Authorize → Payment).Ruff TRY003 fires across all nine
raisesites in these two functions (long messages outside the exception class). Given the test-helper context this is low priority, but consolidating the messages into a small helper or subclass would silence the linter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/it_utils.py` around lines 608 - 721, Both create_mpt_token_and_authorize_source and create_mpt_token_and_authorize_source_async raise XRPLException with long f-string messages at multiple sites triggering Ruff TRY003; refactor by adding a small helper (e.g., _raise_xrpl_exception or a thin subclass like XRPLExceptionWithMsg) that accepts a short context string and the original response and raises XRPLException with a concise message, then replace the nine direct raise XRPLException(...) calls (after sign_and_reliable_submission and sign_and_reliable_submission_async calls) to call that helper to keep messages short and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/it_utils.py`:
- Around line 771-777: Replace the ValueError raised on AMMCreate transaction
failure with XRPLException to match project conventions: in
create_amm_pool_with_mpt (and mirror in create_amm_pool_with_mpt_async) check
the same condition on response/is_successful and
response.result["engine_result"], then raise XRPLException(f"AMMCreate
transaction failed: {response.result.get('engine_result')}") instead of
ValueError; ensure XRPLException is imported/available in the module if not
already.
In `@tests/unit/models/transactions/test_amm_clawback.py`:
- Around line 12-14: There is a duplicated module-level constant _HOLDER_ACCOUNT
declared twice; remove the redundant declaration so only one _HOLDER_ACCOUNT
remains with the expected value ("rNZdsTBP5tH1M6GHC6bTreHAp6ouP8iZSh"), ensuring
you keep the intended single definition and delete the other to avoid silent
shadowing (the duplicate appears alongside the other constants
_MPT_ISSUANCE_ID_1/_MPT_ISSUANCE_ID_2).
---
Duplicate comments:
In `@tests/integration/reqs/test_book_offers.py`:
- Around line 2-7: The file unnecessarily imports sync variants because
test_async_and_sync already merges it_utils.py globals into the test module;
remove any explicit sync imports and keep only the async helpers
(create_mpt_token_and_authorize_source_async, fund_wallet_async,
sign_and_reliable_submission_async) so the decorator can expose the sync
counterparts at runtime via its globals merge in test_async_and_sync.
In `@tests/integration/transactions/test_amm_lifecycle_with_mpt.py`:
- Around line 2-6: Remove the explicit sync variants being imported from
tests.integration.it_utils because the test decorator already injects/merges
sync globals; specifically drop imports of create_amm_pool_with_mpt_async,
sign_and_reliable_submission_async, and test_async_and_sync (or replace with
only the async names needed) so the test relies on the decorator-provided sync
wrappers in it_utils.py and avoids redundant/duplicate imports.
---
Nitpick comments:
In `@tests/integration/it_utils.py`:
- Around line 608-721: Both create_mpt_token_and_authorize_source and
create_mpt_token_and_authorize_source_async raise XRPLException with long
f-string messages at multiple sites triggering Ruff TRY003; refactor by adding a
small helper (e.g., _raise_xrpl_exception or a thin subclass like
XRPLExceptionWithMsg) that accepts a short context string and the original
response and raises XRPLException with a concise message, then replace the nine
direct raise XRPLException(...) calls (after sign_and_reliable_submission and
sign_and_reliable_submission_async calls) to call that helper to keep messages
short and consistent.
In `@tests/integration/transactions/test_offer_create.py`:
- Line 39: Rename the test function test_offer_create_with_MPT to lowercase
snake_case test_offer_create_with_mpt; update the function definition and any
references/calls (e.g., in test discovery, parametrization, fixtures, or marks)
to use test_offer_create_with_mpt so it matches the other methods like
test_basic_functionality and test_deep_freeze_trustline_fails and follows the
project's naming convention.
- Around line 65-66: The two assertions are redundant—response.is_successful()
and checking response.result["engine_result"] == "tesSUCCESS" verify the same
success condition; simplify the test by removing one of them (prefer keeping
response.is_successful() to match the style used in test_basic_functionality
which uses offer.is_successful()). Update the assertions in test_offer_create.py
so only a single success assertion remains (e.g., keep response.is_successful())
and remove the explicit engine_result equality check.
In `@tests/unit/models/transactions/test_amm_create.py`:
- Around line 57-91: Move the three standalone MPTAmount validation tests out of
transaction-specific files and consolidate them into a single unit test module
that targets MPTAmount directly (e.g., create
tests/unit/models/amounts/test_mpt_amount.py); specifically, copy/centralize
test_mpt_issuance_id_non_hex_characters, test_mpt_issuance_id_too_short, and
test_mpt_issuance_id_too_long into a new file that imports MPTAmount and
MPT_ISSUANCE_ID_LENGTH and asserts the same XRPLModelException messages, then
remove the duplicate copies from test_amm_create.py, test_amm_withdraw.py,
test_amm_deposit.py, and test_check_cash.py so the transaction tests no longer
instantiate MPTAmount directly and the validation coverage remains centralized.
| if ( | ||
| not response.is_successful() | ||
| or response.result.get("engine_result") != "tesSUCCESS" | ||
| ): | ||
| raise ValueError( | ||
| f"AMMCreate transaction failed: {response.result.get('engine_result')}" | ||
| ) |
There was a problem hiding this comment.
ValueError used for a transaction failure; prefer XRPLException for consistency.
create_mpt_token_and_authorize_source (and its async counterpart) raise XRPLException when a transaction returns a non-success engine_result, reserving ValueError for data/lookup failures (e.g., missing mpt_issuance_id). create_amm_pool_with_mpt (lines 775–777) and create_amm_pool_with_mpt_async (lines 844–846) invert this: a failed AMMCreate ledger transaction raises ValueError instead.
🛠️ Proposed fix
- raise ValueError(
- f"AMMCreate transaction failed: {response.result.get('engine_result')}"
- )
+ raise XRPLException(
+ f"AMMCreate transaction failed: {response.result.get('engine_result')}"
+ )Apply the same change at lines 844–846 in create_amm_pool_with_mpt_async.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 775-777: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/it_utils.py` around lines 771 - 777, Replace the ValueError
raised on AMMCreate transaction failure with XRPLException to match project
conventions: in create_amm_pool_with_mpt (and mirror in
create_amm_pool_with_mpt_async) check the same condition on
response/is_successful and response.result["engine_result"], then raise
XRPLException(f"AMMCreate transaction failed:
{response.result.get('engine_result')}") instead of ValueError; ensure
XRPLException is imported/available in the module if not already.
| _HOLDER_ACCOUNT = "rNZdsTBP5tH1M6GHC6bTreHAp6ouP8iZSh" | ||
| _MPT_ISSUANCE_ID_1 = "00000001A407AF5856CECE4281FED12B7B179B49A4AEF506" | ||
| _MPT_ISSUANCE_ID_2 = "00000002A407AF5856CECE4281FED12B7B179B49A4AEF506" |
There was a problem hiding this comment.
Remove duplicate _HOLDER_ACCOUNT definition introduced by this PR.
Line 12 defines _HOLDER_ACCOUNT with the same value as the pre-existing line 20. The module-level reassignment at line 20 silently shadows line 12, making it dead code.
🐛 Proposed fix
-_HOLDER_ACCOUNT = "rNZdsTBP5tH1M6GHC6bTreHAp6ouP8iZSh"
_MPT_ISSUANCE_ID_1 = "00000001A407AF5856CECE4281FED12B7B179B49A4AEF506"
_MPT_ISSUANCE_ID_2 = "00000002A407AF5856CECE4281FED12B7B179B49A4AEF506"Also applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/models/transactions/test_amm_clawback.py` around lines 12 - 14,
There is a duplicated module-level constant _HOLDER_ACCOUNT declared twice;
remove the redundant declaration so only one _HOLDER_ACCOUNT remains with the
expected value ("rNZdsTBP5tH1M6GHC6bTreHAp6ouP8iZSh"), ensuring you keep the
intended single definition and delete the other to avoid silent shadowing (the
duplicate appears alongside the other constants
_MPT_ISSUANCE_ID_1/_MPT_ISSUANCE_ID_2).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CHANGELOG.md (1)
13-15: Extra blank line between the Unreleased block and[[4.5.0]].There are two consecutive blank lines (lines 14–15) between the last Unreleased item and the
4.5.0header, while the rest of the file uses a single blank line as a separator.📝 Proposed fix
- The `MPTCurrency` model has been updated to validate the semantic correctness of `MPTIssuanceID` values. This is performed using regular-expression matching and does not involve any read-operations on the XRPL blockchain. - ## [[4.5.0]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 13 - 15, Remove the extra blank line separating the Unreleased section and the [[4.5.0]] header so there is exactly one blank line between them; locate the Unreleased header and the subsequent [[4.5.0]] header in CHANGELOG.md and delete the redundant empty line to match the file's single-blank-line separation convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 16: The release header "## [[4.5.0]]" is missing its release date; either
append the proper date like " - YYYY-MM-DD" to the "## [[4.5.0]]" heading to
indicate it's been released, or if this PR isn't cutting 4.5.0, move the content
currently under the "## [[4.5.0]]" section back into the "[Unreleased]" section
so that the changelog only contains dated release headers and unreleased changes
remain under "[Unreleased]".
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 13-15: Remove the extra blank line separating the Unreleased
section and the [[4.5.0]] header so there is exactly one blank line between
them; locate the Unreleased header and the subsequent [[4.5.0]] header in
CHANGELOG.md and delete the redundant empty line to match the file's
single-blank-line separation convention.
| - The `MPTCurrency` model has been updated to validate the semantic correctness of `MPTIssuanceID` values. This is performed using regular-expression matching and does not involve any read-operations on the XRPL blockchain. | ||
|
|
||
|
|
||
| ## [[4.5.0]] |
There was a problem hiding this comment.
[[4.5.0]] is missing its release date.
All previously promoted versions carry a - YYYY-MM-DD suffix (e.g., ## [[4.4.0]] - 2025-12-16). If 4.5.0 is being cut as part of this PR, add the release date; otherwise the content currently under it should remain in the [Unreleased] section.
📝 Proposed fix
-## [[4.5.0]]
+## [[4.5.0]] - 2026-MM-DD📝 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.
| ## [[4.5.0]] | |
| ## [[4.5.0]] - 2026-MM-DD |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 16, The release header "## [[4.5.0]]" is missing its
release date; either append the proper date like " - YYYY-MM-DD" to the "##
[[4.5.0]]" heading to indicate it's been released, or if this PR isn't cutting
4.5.0, move the content currently under the "## [[4.5.0]]" section back into the
"[Unreleased]" section so that the changelog only contains dated release headers
and unreleased changes remain under "[Unreleased]".
…yment transaction integ test with explicit paths
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
17-17:[[4.5.0]]is still missing its release date.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 17, The CHANGELOG entry header '[[4.5.0]]' is missing its release date; update the header for [[4.5.0]] to include the release date using the same date format and placement as other changelog entries (e.g., append " - YYYY-MM-DD" or whatever pattern is used for other version headers) so the version header for [[4.5.0]] matches the project's changelog style.
🧹 Nitpick comments (1)
xrpl/core/binarycodec/types/path_set.py (1)
149-151:to_jsonreads MPT bytes as raw bytes whileaccount_idandcurrencyuse their class'sfrom_parser().to_json()pattern.# account/currency style (lines 144, 147) currency = Currency.from_parser(parser).to_json() # MPT style (line 150) mpt_id = parser.read(_HASH192_BYTES).hex().upper()For consistency consider:
- mpt_id = parser.read(_HASH192_BYTES).hex().upper() - json["mpt_issuance_id"] = mpt_id + mpt_id = Hash192.from_parser(parser).to_json() + json["mpt_issuance_id"] = mpt_idVerify that
Hash192.to_json()returns the same uppercase hex string as.hex().upper()before applying this; the tests intest_path_set.pycurrently confirm the raw-bytes approach is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/core/binarycodec/types/path_set.py` around lines 149 - 151, The code reads MPT bytes directly (parser.read(_HASH192_BYTES).hex().upper()) while account_id and currency use their classes' from_parser().to_json() flow; update this to use Hash192.from_parser().to_json() for consistency but first verify that Hash192.to_json() produces the identical uppercase hex string currently expected by tests (i.e., matches .hex().upper()); if it does, replace the parser.read(...) usage with Hash192.from_parser(parser).to_json() and set json["mpt_issuance_id"] to that value, otherwise leave the raw-bytes approach and add a comment explaining the difference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CHANGELOG.md`:
- Line 17: The CHANGELOG entry header '[[4.5.0]]' is missing its release date;
update the header for [[4.5.0]] to include the release date using the same date
format and placement as other changelog entries (e.g., append " - YYYY-MM-DD" or
whatever pattern is used for other version headers) so the version header for
[[4.5.0]] matches the project's changelog style.
---
Nitpick comments:
In `@xrpl/core/binarycodec/types/path_set.py`:
- Around line 149-151: The code reads MPT bytes directly
(parser.read(_HASH192_BYTES).hex().upper()) while account_id and currency use
their classes' from_parser().to_json() flow; update this to use
Hash192.from_parser().to_json() for consistency but first verify that
Hash192.to_json() produces the identical uppercase hex string currently expected
by tests (i.e., matches .hex().upper()); if it does, replace the
parser.read(...) usage with Hash192.from_parser(parser).to_json() and set
json["mpt_issuance_id"] to that value, otherwise leave the raw-bytes approach
and add a comment explaining the difference.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdtests/integration/transactions/test_payment.pytests/unit/core/binarycodec/types/test_path_set.pyxrpl/core/binarycodec/types/path_set.pyxrpl/models/path.py
| account=_SOURCE, | ||
| amount=MPTAmount( | ||
| mpt_issuance_id=_ISSUER, | ||
| mpt_issuance_id="00000001A407AF5856CECE4281FED12B7B179B49A4AEF506", |
There was a problem hiding this comment.
Maybe create a new constant at the top of this file for this new value? Not needed but could be useful especially if new tests are added to this file that use the same value
| def _get_account_error(self: Self) -> Optional[str]: | ||
| if self.account is None: | ||
| return None | ||
| if self.currency is not None or self.issuer is not None: |
There was a problem hiding this comment.
nit: Do we need to validate that account and mpt_issuance_id are mutually exclusive?
There was a problem hiding this comment.
account and mpt_issuance_id are two different types of values. They will always be different when compared by their values.
If you mean that the Issuer-of-MPT must be different from the preceding Account: I'm not sure if this is semantically needed in the context of XRPL blockchain. For example, suppose I want to do a cross-currency transfer between XRP -> MPT-Issued-by-me and I already have open offers on the DEX --> This case seems like a valid case (especially in case of multi-national corporations)
| "account": self._get_account_error(), | ||
| "currency": self._get_currency_error(), | ||
| "issuer": self._get_issuer_error(), | ||
| "mpt_issuance_id": self._get_mpt_issuance_id_error(), |
There was a problem hiding this comment.
nit: Do we need to check MPT Issuance ID format?
There was a problem hiding this comment.
All mpt-Issuance-id values are serialized into Hash192 binary-codec type. Such values are serialized using the str_input.fromHex() method, which fails upon non-hex inputs.
Furthermore, the value is restricted to 48 hex characters (or) 24 bytes because of the length restriction on the Hash192 binary-codec type.
I felt it is better to validate the format of all the Hash192 type values in one place, instead of adding validation code in all the models. Let me know if you feel another validation in PathSet.py model will help.
There was a problem hiding this comment.
As per our conversation, I have included additional validation for the Path model in this commit: 01b19a7
cybele-ripple
left a comment
cybele-ripple
left a comment
There was a problem hiding this comment.
Should we delay merging until the integration tests pass?
Yes, I will wait until the rippled cpp implementation is merged into their Thanks @kuan121 and @cybele-ripple for your reviews. |
3 participants
High Level Overview of Change
This PR implements the following XLS specification requirements for the xrpl-py client library: XRPLF/XRPL-Standards#231
The reference cpp implementation for rippled can be found here: XRPLF/rippled#5285
This PR includes updates to the transaction and request models to accommodate
MPTAmount, integration and unit tests for all the affected transaction-types and RPCs.The PR also updates the binary-codec of the
PathSetrippled type. The amendment allows formpt_issuance_idinputs in the Path specification.Type of Change
Did you update CHANGELOG.md?
Test Plan
Appropriate tests have been added