Es add credential create #864
Conversation
* add AMMInstanceCreate * update definitions to get AMMInstanceCreate working * fix lint errors * add unit test for Instance Create * fix lint errors * fix definitions and change AmmCreate to AMMInstanceCreate * fix lint error * add AMMInfo * update AMMInfo docstring * add docstring to AMMInfo params * update definitions.json * remove AMMAccount from AMMInstanceCreate model * add AMMDeposit * fix lint errors * add AMMWithdraw * add AMMVote * add AMMBid * fix typo * update AMMBid test * add MaxSlotPrice param to AMMBid * refactor test * update definitions and replace AMMHash with AMMID * update lptokens type to IssuedCurrencyAmount * assert with error message * assert with error messages for AMMInstanceCreate and amm_info * move to_xrpl tests to test_base_model * rename lptokens to lp_tokens * update amm_info request params to be in snake_case * update docstrings * reorder SPECIAL_CAMELCASE_STRINGS to alphabetical order * refactor ABBREVIATIONS to be set in one place * rename LPTokens to LPToken * update CHANGELOG.md * fix typo * fix lint error * refactor max trading fee to be in one place * update amm_bid error message * add AuthAccount base model for AMMBid * update definitions to fix AMM in LEDGER_ENTRY_TYPES * update CHANGELOG.md to specify XLS-30 * update wording on AMMDeposit & AMMWithdraw validation errors * add negative FeeVal check * add negative TradingFee check * fix lint error * export AuthAccount model * add AuthAccount and refactor special case models check * revert Path and _value_to_tx_json() changes * fix AuthAccount capitalization issues (XRPLF#432) Co-authored-by: Omar Khan <[email protected]> * add AMMBid codec-fixture * add AMMInstanceCreate codec-fixture * update definitions.json with different AuthAccounts number * remove AMM codec-fixtures * Change amm_info asset parameters to Currency type * API name changes with updated definitions.json * rename amm_info param asset1 -> asset * change AMM_MAX_TRADING_FEE to 1% and rename fee_val to trading_fee * rename MinBidPrice -> BidMin and MaxBidPrice -> BidMax * update definitions to change Asset & Asset2 nth values to 3 & 4 * Use asset/asset2 instead of amm_id for Deposit/Withdraw/Bid/Vote * update definitions * add Issue type * add flags to AMM deposit & withdraw * add Issue model * add Issue type to models with asset & asset2; remove amm_id * resolve lint errors * rename LPToken in amm deposit & withdraw * update docstrings * add AMM codec-fixtures * add one asset withdraw & withdraw all tests * update definitions.json with refactored error codes * add Owner Reserve Fee for AMMCreate transaction * refactor asset pair to be Currency type * update amm_info asset pair to be Currency type and remove Issue model * update definitions and codec-fixtures * update DiscountedFee definition * update definitions and codec-fixtures * update definitions * remove sidechain method * small refactor * update docstrings * refactor _value_to_tx_json to remove special case for auth_account * refactor AuthAccount to be a NestedModel * remove test_base_model tests for amm * add test_to_xrpl_auth_accounts * fix indentation with tests * update definitions * add AMMDelete * add AMMDelete docstring --------- Co-authored-by: Mayukha Vadari <[email protected]>
[cherry-pick] add AMM support (XRPLF#422)
Update remote repo
Update type
Pulling set_fee tx updates
Add default value of NULL for optional fields
Codeowners file
Add nft token metadata
WalkthroughAdds AMM support across models, requests, flags, and binary codec definitions; introduces new transaction types (AMM, XChain, Oracle, DID, Credential, Clawback, NFTokenModify); updates Transaction and BaseModel behavior; adds Issue binary codec type; adjusts fee logic for AMM_CREATE; expands tests and fixtures; updates SetFee pseudo-transaction; minor docs and repo metadata edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Tx as Transaction(AMMCreate)
participant Autofill as asyncio.transaction.autofill
participant Client as XRPL Client
participant Server as rippled
Dev->>Tx: Construct AMMCreate(...)
Dev->>Autofill: autofill(Tx, client)
note right of Autofill: Check tx type for AMM_CREATE/ACCOUNT_DELETE
alt client provided
Autofill->>Client: _request_impl(ServerState())
Client->>Server: server_state
Server-->>Client: reserve_inc
Client-->>Autofill: reserve_inc (owner reserve)
Autofill->>Autofill: base_fee = reserve_inc
else no client
Autofill->>Autofill: base_fee = _OWNER_RESERVE_FEE
end
Autofill-->>Dev: Tx with Fee set
sequenceDiagram
autonumber
actor Dev
participant T as Transaction
participant Codec as binarycodec
participant Parser as from_xrpl
Dev->>Parser: Transaction.from_xrpl(json_or_str)
note right of Parser: Key/value transform using ABBREVIATIONS
alt Payment with deliver_max present
Parser->>Parser: If amount missing, set amount = deliver_max
Parser->>Parser: If amount != deliver_max, raise
Parser->>Parser: Remove deliver_max
end
Parser-->>Dev: Transaction instance
Dev->>T: blob()
T->>Codec: encode(to_xrpl())
Codec-->>T: hex blob
T-->>Dev: hex blob
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xrpl/asyncio/transaction/main.py (1)
341-345: Bug: calling non-existent/private client method and no error handling.
Clientexposesrequest_impl, not_request_impl. Also, handle unsuccessful responses.-async def _fetch_owner_reserve_fee(client: Client) -> int: - server_state = await client._request_impl(ServerState()) - fee = server_state.result["state"]["validated_ledger"]["reserve_inc"] - return int(fee) +async def _fetch_owner_reserve_fee(client: Client) -> int: + resp = await client.request_impl(ServerState()) + if not resp.is_successful(): + raise XRPLRequestFailureException(resp.result) + fee = resp.result["state"]["validated_ledger"]["reserve_inc"] + return int(fee)xrpl/models/transactions/transaction.py (1)
416-423: Unsigned transactions can slip through get_hash when signers == []. Use is_signed().Empty Signers ([]) bypass the current None-check and incorrectly allow hashing. Leverage the new is_signed() helper.
- if self.txn_signature is None and self.signers is None: + if not self.is_signed(): raise XRPLModelException( "Cannot get the hash from an unsigned Transaction." )
🧹 Nitpick comments (78)
.github/CODEOWNERS (1)
1-2: CODEOWNERS team must belong to the XRPLF orgGitHub only recognizes users and teams that have access within the repository’s org.
@trmlabs/ci-blockchain-datais an external org team and will be ignored. Replace with XRPLF team(s) or specific maintainers.Example (adjust to the correct XRPLF team slug or user handles):
-* @trmlabs/ci-blockchain-data +* @XRPLF/maintainers +# or explicit users: +# * @alice @bobxrpl/models/auth_account.py (1)
11-21: LGTM: minimal, idiomatic NestedModel for AMMBid auth accountsFrozen dataclass, keyword-only init, and required
accountfield align with existing model patterns.Small doc polish for consistency:
- “transactions” plural, and mention the XRPL key “Account”.
-"""Model used in AMMBid transaction.""" +"""Model used in AMMBid transactions.""" @@ - """Represents one entry in a list of AuthAccounts used in AMMBid transaction.""" + """One entry in `AuthAccounts` used by AMMBid (XRPL field: `Account`)."""xrpl/models/flags.py (1)
24-39: AMM flags added — verify constants and TransactionType keysThe new groups look correct. Please double-check bit values against XLS-30/rippled and ensure
TransactionTypenames are exactly"AMMDeposit"/"AMMWithdraw"soTX_FLAGSkey lookup works.Consider adding unit tests for:
interface_to_flag_list(TransactionType.AMMDeposit, {...})check_false_flag_definitionwith AMM flags.xrpl/models/utils.py (1)
39-46: Typos and clarity in comments.
- “higer” → “higher”
- “enbles” → “enables”
- “data_class” → “dataclass”
- Clarify that we’re checking for the
kw_onlyparameter support, not “KW_ONLY attribute.”Also applies to: 99-103
xrpl/models/base_model.py (3)
170-176: Remove unused exception variable (F841).Keep the comment, drop the unused binding.
- except XRPLModelException as e: + except XRPLModelException: # Uncomment this if you are getting parsing errors with a Union or other collection type # and want to uncover the parsing errors with each param type option # print(f"{param_type_option} failed to parse: {e}")
223-246: Deduplicate JSON processing via the new helper.Reuse
_process_xrpl_jsoninfrom_xrplto avoid drift.@classmethod def from_xrpl(cls: Type[BM], value: Union[str, Dict[str, Any]]) -> BM: @@ - if isinstance(value, str): - value = json.loads(value) - - formatted_dict = { - _key_to_json(k): _value_to_json(v) - for (k, v) in cast(Dict[str, XRPL_VALUE_TYPE], value).items() - } - - return cls.from_dict(formatted_dict) + return cls.from_dict(cls._process_xrpl_json(value))Also applies to: 266-274
355-361: Consider filtering out None to match to_dict semantics.
to_dict_no_nestingcurrently includes None-valued fields;to_dictdoesn’t. For consistency, filter them out unless downstream requires explicit nulls.- return { - key: self._to_dict_elem_no_nesting(getattr(self, key)) - for key in dataclass_fields - } + return { + key: self._to_dict_elem_no_nesting(getattr(self, key)) + for key in dataclass_fields + if getattr(self, key) is not None + }tests/unit/models/transactions/test_amm_delete.py (2)
7-7: Rename test class to match subject (AMMDelete).Current name says Deposit; rename for clarity.
-class TestAMMDeposit(TestCase): +class TestAMMDelete(TestCase):
4-4: Also assert the transaction_type for stronger coverage.This ensures the model binds to TransactionType.AMM_DELETE.
-from xrpl.models.transactions import AMMDelete +from xrpl.models.transactions import AMMDelete, TransactionType @@ - self.assertTrue(tx.is_valid()) + self.assertTrue(tx.is_valid()) + self.assertEqual(tx.transaction_type, TransactionType.AMM_DELETE)Also applies to: 17-17
xrpl/models/__init__.py (1)
4-4: Adding AuthAccount to top-level exports — note API surface change.Public API expands; consider adding a CHANGELOG entry.
I can draft the CHANGELOG blurb if helpful.
Also applies to: 17-17
tests/unit/models/transactions/test_amm_vote.py (2)
24-35: Fix unreachable assertion inside assertRaises.The equality assertion never runs because it’s inside the assertRaises block. Move it below the with block.
Apply:
def test_trading_fee_too_high(self): - with self.assertRaises(XRPLModelException) as error: - AMMVote( + with self.assertRaises(XRPLModelException) as error: + AMMVote( account=_ACCOUNT, asset=_ASSET, asset2=_ASSET2, trading_fee=maxsize, ) - self.assertEqual( - error.exception.args[0], - "{'trading_fee': 'Must be between 0 and 1000'}", - ) + self.assertEqual( + error.exception.args[0], + "{'trading_fee': 'Must be between 0 and 1000'}", + )
37-48: Same unreachable assertion pattern here.Repeat the fix for the negative number case.
def test_trading_fee_negative_number(self): - with self.assertRaises(XRPLModelException) as error: - AMMVote( + with self.assertRaises(XRPLModelException) as error: + AMMVote( account=_ACCOUNT, asset=_ASSET, asset2=_ASSET2, trading_fee=-1, ) - self.assertEqual( - error.exception.args[0], - "{'trading_fee': 'Must be between 0 and 1000'}", - ) + self.assertEqual( + error.exception.args[0], + "{'trading_fee': 'Must be between 0 and 1000'}", + )tests/unit/models/test_base_model.py (1)
604-671: AMMBid AuthAccounts to_xrpl round-trip coverage is solid.
- Verifies nested AuthAccount wrapper structure and standard fields. Nice addition.
- Optional: add a negative test (len>4) only if not already covered elsewhere.
tests/unit/models/requests/test_amm_info.py (1)
12-16: Add a couple of negative cases.Consider tests for missing asset/asset2 and an invalid IssuedCurrency (e.g., currency="XRP") to lock in validation behavior.
tests/unit/models/transactions/test_amm_bid.py (3)
25-26: Secret scanner false positive — add inline allowlist.These are test constants (an LP token code and an address), but gitleaks flags them. Annotate to avoid CI noise.
Apply:
-_LPTOKEN_CURRENCY = "5475B6C930B7BDD81CDA8FBA5CED962B11218E5A" -_LPTOKEN_ISSUER = "r3628pXjRqfw5zfwGfhSusjZTvE3BoxEBw" +_LPTOKEN_CURRENCY = "5475B6C930B7BDD81CDA8FBA5CED962B11218E5A" # gitleaks:allow test fixture +_LPTOKEN_ISSUER = "r3628pXjRqfw5zfwGfhSusjZTvE3BoxEBw" # gitleaks:allow test fixture
49-66: Use context manager pattern for asserting on the exception.Slightly clearer and less brittle to assert after the with-block and to use assertIn for the message.
- with self.assertRaises(XRPLModelException) as error: + with self.assertRaises(XRPLModelException) as cm: AMMBid( account=_ACCOUNT, asset=_ASSET, asset2=_ASSET2, auth_accounts=auth_accounts, ) - self.assertEqual( - error.exception.args[0], - "{'auth_accounts': 'Length must not be greater than 4'}", - ) + self.assertIn("Length must not be greater than 4", str(cm.exception))
11-24: Optional: add a spec-driven test.If the model forbids including the sender in auth_accounts (per docstring), add a failure test for that case.
xrpl/models/transactions/did_delete.py (1)
12-20: Add unit test and export check.Please add a basic construction/serialization test and ensure DIDDelete is exported in xrpl.models.transactions.init.
tests/unit/models/transactions/test_amm_create.py (3)
24-37: Refine exception assertion pattern.- with self.assertRaises(XRPLModelException) as error: + with self.assertRaises(XRPLModelException) as cm: AMMCreate( account=_ACCOUNT, amount="1000", amount2=IssuedCurrencyAmount( currency="USD", issuer=_IOU_ISSUER, value="1000" ), trading_fee=maxsize, ) - self.assertEqual( - error.exception.args[0], - "{'trading_fee': 'Must be between 0 and 1000'}", - ) + self.assertIn("Must be between 0 and 1000", str(cm.exception))
39-52: Same refinement here for consistency.- with self.assertRaises(XRPLModelException) as error: + with self.assertRaises(XRPLModelException) as cm: AMMCreate( account=_ACCOUNT, amount="1000", amount2=IssuedCurrencyAmount( currency="USD", issuer=_IOU_ISSUER, value="1000" ), trading_fee=-1, ) - self.assertEqual( - error.exception.args[0], - "{'trading_fee': 'Must be between 0 and 1000'}", - ) + self.assertIn("Must be between 0 and 1000", str(cm.exception))
12-22: Optional: add boundary tests.Add tests for trading_fee=0 and trading_fee=1000 to pin exact limits.
xrpl/models/transactions/clawback.py (3)
19-25: Confirm semantics for IssuedCurrencyAmount.issuer.Docstring states issuer carries the holder’s address for Clawback. Please verify this matches the canonical Clawback spec and binary codec expectations.
32-43: Clarify validation message and consider amount positivity check.
- “Holder's address is wrong.” is vague; suggest “Holder's address must not equal the sending account.”
- Optionally validate amount.value > 0 to prevent zero/negative clawbacks.
- if is_issued_currency(self.amount): - if self.account == self.amount.issuer: - errors["amount"] = "Holder's address is wrong." + if is_issued_currency(self.amount): + if self.account == self.amount.issuer: + errors["amount"] = "Holder's address must not equal the sending account." + # Optional: require positive amount value + try: + if float(self.amount.value) <= 0: + errors["amount"] = "Amount must be greater than 0." + except (TypeError, ValueError): + errors["amount"] = "Amount value must be numeric."
14-16: Align with newer models: make kwargs-only for consistency.DIDDelete uses KW_ONLY_DATACLASS; mirror that here for consistency.
-from xrpl.models.utils import require_kwargs_on_init +from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init @@ -@require_kwargs_on_init -@dataclass(frozen=True) +@require_kwargs_on_init +@dataclass(frozen=True, **KW_ONLY_DATACLASS) class Clawback(Transaction):tests/unit/models/transactions/test_amm_withdraw.py (2)
107-121: Use assertRaisesRegex to avoid brittle exact-message assertionsComparing the entire dict string is fragile. Match just the relevant text.
Apply:
- with self.assertRaises(XRPLModelException) as error: - AMMWithdraw( + with self.assertRaisesRegex( + XRPLModelException, r"Must set `amount` with `amount2`" + ): + AMMWithdraw( account=_ACCOUNT, sequence=1337, asset=_ASSET, asset2=_ASSET2, amount2=IssuedCurrencyAmount( currency=_ASSET2.currency, issuer=_ASSET2.issuer, value="500" ), ) - self.assertEqual( - error.exception.args[0], - "{'AMMWithdraw': 'Must set `amount` with `amount2`'}", - )
123-135: Same here: prefer assertRaisesRegex for message matchKeeps the test resilient to formatting changes.
Apply:
- with self.assertRaises(XRPLModelException) as error: - AMMWithdraw( + with self.assertRaisesRegex( + XRPLModelException, r"Must set `amount` with `e_price`" + ): + AMMWithdraw( account=_ACCOUNT, sequence=1337, asset=_ASSET, asset2=_ASSET2, e_price="25", ) - self.assertEqual( - error.exception.args[0], - "{'AMMWithdraw': 'Must set `amount` with `e_price`'}", - )xrpl/models/transactions/xchain_add_claim_attestation.py (2)
1-1: Grammar nit: “An XChain…”Use “An” before XChain for correctness.
-"""Model for a XChainAddClaimAttestation transaction type.""" +"""Model for an XChainAddClaimAttestation transaction type."""
21-25: Minor doc nit: “An XChain…”Same grammar nit in class docstring.
- Represents a XChainAddClaimAttestation transaction. + Represents an XChainAddClaimAttestation transaction.tests/unit/models/transactions/test_amm_deposit.py (4)
86-97: Prefer assertRaisesRegex over exact string equalityReduces brittleness of error-message checks.
- with self.assertRaises(XRPLModelException) as error: + with self.assertRaisesRegex(XRPLModelException, r"Must set at least `lp_token_out` or `amount`"): AMMDeposit( account=_ACCOUNT, sequence=1337, asset=_ASSET, asset2=_ASSET2, ) - self.assertEqual( - error.exception.args[0], - "{'AMMDeposit': 'Must set at least `lp_token_out` or `amount`'}", - )
99-114: Same: use assertRaisesRegex for amount2 without amount- with self.assertRaises(XRPLModelException) as error: + with self.assertRaisesRegex(XRPLModelException, r"Must set `amount` with `amount2`"): AMMDeposit( account=_ACCOUNT, sequence=1337, asset=_ASSET, asset2=_ASSET2, amount2=IssuedCurrencyAmount( currency=_ASSET2.currency, issuer=_ASSET2.issuer, value="500" ), ) - self.assertEqual( - error.exception.args[0], - "{'AMMDeposit': 'Must set `amount` with `amount2`'}", - )
115-127: And here for e_price without amount- with self.assertRaises(XRPLModelException) as error: + with self.assertRaisesRegex(XRPLModelException, r"Must set `amount` with `e_price`"): AMMDeposit( account=_ACCOUNT, sequence=1337, asset=_ASSET, asset2=_ASSET2, e_price="25", ) - self.assertEqual( - error.exception.args[0], - "{'AMMDeposit': 'Must set `amount` with `e_price`'}", - )
18-31: Tiny naming nitConsider renaming test_tx_valid_xrpl_lptokenout -> test_tx_valid_xrp_lptokenout for accuracy (XRP vs XRPL).
xrpl/models/transactions/oracle_delete.py (2)
14-27: Align with existing transaction class patternsTwo small consistency tweaks:
- Avoid redeclaring
account; it already exists on Transaction and redeclaration can cause dataclass field shadowing.- Use KW_ONLY_DATACLASS to match newer models’ keyword-only init behavior on 3.10+.
-from xrpl.models.utils import require_kwargs_on_init +from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init @@ -@require_kwargs_on_init -@dataclass(frozen=True) +@require_kwargs_on_init +@dataclass(frozen=True, **KW_ONLY_DATACLASS) class OracleDelete(Transaction): @@ - account: str = REQUIRED # type: ignore - """This account must match the account in the Owner field of the Oracle object.""" - oracle_document_id: int = REQUIRED # type: ignore """A unique identifier of the price oracle for the Account."""
1-1: Docstring nit“Model for an OracleDelete transaction type.” reads slightly better.
-"""Model for OracleDelete transaction type.""" +"""Model for an OracleDelete transaction type."""xrpl/models/transactions/xchain_create_bridge.py (2)
86-96: Standardize error message formatting for field names.Other models (e.g., XChainModifyBridge) wrap field names in backticks; mirror that here for consistency.
Apply:
- errors["signature_reward"] = "signature_reward must be numeric." + errors["signature_reward"] = "`signature_reward` must be numeric." @@ - ] = "min_account_create_amount must be numeric." + ] = "`min_account_create_amount` must be numeric."
1-1: Grammar: use “an XChain…”Tiny doc nit.
-"""Model for a XChainCreateBridge transaction type.""" +"""Model for an XChainCreateBridge transaction type.""" @@ - Represents a XChainCreateBridge transaction. + Represents an XChainCreateBridge transaction.Also applies to: 21-27
xrpl/models/transactions/amm_delete.py (2)
30-44: Add local validation to prevent identical asset pair.AMMs require two distinct assets. Catch early at model validation.
from dataclasses import dataclass, field +from typing import Dict @@ transaction_type: TransactionType = field( default=TransactionType.AMM_DELETE, init=False, ) + + def _get_errors(self: AMMDelete) -> Dict[str, str]: + errors = super()._get_errors() + if self.asset == self.asset2: + errors["asset_pair"] = "asset and asset2 must be different." + return errors
13-15: Optional: use KW_ONLY_DATACLASS for cross-version kw-only consistency.Other new models sometimes pass KW_ONLY_DATACLASS; consider aligning.
-from dataclasses import dataclass, field +from dataclasses import dataclass, field +from xrpl.models.utils import KW_ONLY_DATACLASS @@ -@dataclass(frozen=True) +@dataclass(frozen=True, **KW_ONLY_DATACLASS) class AMMDelete(Transaction):xrpl/models/transactions/amm_vote.py (2)
49-57: Validate asset pair differs (match AMMCreate/Withdraw expectations).Prevents malformed pools at the model layer.
def _get_errors(self: AMMVote) -> Dict[str, str]: return { key: value for key, value in { **super()._get_errors(), "trading_fee": self._get_trading_fee_error(), + "asset_pair": "asset and asset2 must be different." + if self.asset == self.asset2 + else None, }.items() if value is not None }
37-42: Avoid hard-coding the max value in the docstring.If AMM_MAX_TRADING_FEE changes, the text drifts. Suggest wording “The maximum value is AMM_MAX_TRADING_FEE (currently 1000).”
xrpl/models/transactions/amm_create.py (1)
40-50: Consider validating positive amountsDocs say both amounts “must be a positive amount,” but there’s no check. Consider adding amount positivity validation (guard for REQUIRED first) similar to how
trading_feeis validated.xrpl/models/transactions/nftoken_modify.py (3)
26-31: Fix inaccurate docstring (“offer references”)This transaction modifies a token; it doesn’t reference an offer.
Apply:
- Identifies the TokenID of the NFToken object that the - offer references. This field is required. + Identifies the TokenID of the NFToken object that this + transaction modifies. This field is required.
64-67: Clarify empty-URI message and handle None explicitlyMessage says “empty string” but
Nonealso hits this branch.Apply:
- if not self.uri: - return "URI must not be empty string" + if self.uri in (None, ""): + return "URI must be provided and non-empty"
67-72: Clarify URI length error message to match spec semantics
The 512-character limit corresponds to the spec’s 256-byte maximum for hex-encoded URIs. Update the error text in NFTokenModify._get_uri_error to mention both bytes and hex characters:- elif len(self.uri) > _MAX_URI_LENGTH: - return f"URI must not be longer than {_MAX_URI_LENGTH} characters" + elif len(self.uri) > _MAX_URI_LENGTH: + return f"URI must not exceed 256 bytes (up to {_MAX_URI_LENGTH} hex characters)"xrpl/models/transactions/pseudo_transactions/set_fee.py (2)
25-26: Fix RST link spacingAdd a space before the angle-bracketed URL to render correctly.
Apply:
- `XRPFees Amendment<https://xrpl.org/known-amendments.html#xrpfees>`_ + `XRPFees Amendment <https://xrpl.org/known-amendments.html#xrpfees>`_
71-91: Refine SetFee pseudo-transaction _drops field docstrings
- Retain “as hex” on
base_fee_drops(tests use"000000000000000A"); reserve fields use decimal and need no “hex” mention.- Replace each “This field is required.” with “Required after the XRPFees Amendment; otherwise unset.”
xrpl/models/transactions/xchain_create_claim_id.py (2)
21-26: Grammar nit: “An XChain…”Use “An XChainCreateClaimID transaction” for correctness and consistency with other docs.
Apply this diff:
- Represents a XChainCreateClaimID transaction. + Represents an XChainCreateClaimID transaction.
35-41: Clarify denomination (“drops of XRP”).Elsewhere we typically mean string drops. Minor doc precision.
Apply this diff:
- The amount, in XRP, to reward the witness servers for providing signatures. + The amount, in drops of XRP (string), to reward the witness servers for providing signatures.xrpl/models/transactions/xchain_modify_bridge.py (1)
41-45: Grammar nit: “An XChain…”Use “An XChainModifyBridge transaction.”
Apply this diff:
- Represents a XChainModifyBridge transaction. + Represents an XChainModifyBridge transaction.xrpl/models/transactions/oracle_set.py (3)
22-27: Make EPOCH_OFFSET an int to avoid float comparisons/formatting.-# epoch offset must equal 946684800 seconds. It represents the diff between the -# genesis of Unix time and Ripple-Epoch time -EPOCH_OFFSET = ( - datetime.datetime(2000, 1, 1) - datetime.datetime(1970, 1, 1) -).total_seconds() +# Diff between Unix epoch and Ripple epoch (2000-01-01T00:00:00Z), in seconds. +EPOCH_OFFSET = 946684800
84-87: Cap size aligned with owner reserves rule.Consider also validating that count > 5 implies the creator can afford two owner reserves (if that’s enforceable here or at least note it in the error message).
170-178: Tighten typing for asset_price.Current type is Optional[Any]. Prefer Optional[Union[int, str]] and validate numeric. Keeps API safer and self-documenting.
- asset_price: Optional[Any] = None + asset_price: Optional[Union[int, str]] = Nonexrpl/models/transactions/amm_deposit.py (1)
95-103: Add basic cross-field validation (distinct assets; minimal flag sanity).
- Enforce that asset != asset2.
- Optionally validate flag combinations (e.g., SINGLE_ASSET vs TWO_ASSET vs ONE_ASSET_LP_TOKEN are mutually exclusive; LP_TOKEN-only flows).
def _get_errors(self: AMMDeposit) -> Dict[str, str]: errors = super()._get_errors() + if self.asset == self.asset2: + errors["AMMDeposit"] = "asset and asset2 must be distinct" if self.amount2 is not None and self.amount is None: errors["AMMDeposit"] = "Must set `amount` with `amount2`" elif self.e_price is not None and self.amount is None: errors["AMMDeposit"] = "Must set `amount` with `e_price`" elif self.lp_token_out is None and self.amount is None: errors["AMMDeposit"] = "Must set at least `lp_token_out` or `amount`" return errorsIf you confirm the spec details, I can add precise flag validations.
xrpl/models/transactions/transaction.py (2)
459-461: Config.smart_union likely no-op here.If BaseModel doesn’t consume pydantic-style Config, consider removing to avoid confusion.
459-474: Add unit tests fordeliver_maxaliasing inTransaction.from_xrpl
- Payment with only
deliver_max: verifyamountis set todeliver_maxanddeliver_maxis removed.- Payment with both
amountanddeliver_maxequal: no exception is thrown and output is correct.- Payment with both
amountanddeliver_maxunequal: ensureXRPLModelExceptionis raised.- Non-Payment transaction with a
deliver_maxfield: confirm it remains unchanged (or is handled according to spec).xrpl/core/binarycodec/definitions/definitions.json (4)
1506-1535: New SetFee “Drops” Amount fields — clarify coexistence with legacy fieldsBaseFeeDrops/ReserveBaseDrops/ReserveIncrementDrops coexist with legacy BaseFee/ReserveBase/ReserveIncrement. Add a brief comment in CHANGELOG or docs clarifying when each is used (e.g., SetFee v2) to avoid downstream confusion.
1536-1585: AMM LP/price fields — add round-trip testsLPTokenIn/Out/Balance, EPrice, Price: please add codec round-trip tests covering AMM{Create,Deposit,Withdraw,Bid,Delete} that include these fields, including edge cases (zero, max, issued currencies).
I can draft fixtures if helpful.
2206-2235: Voting and auth fields — confirm cardinality constraints are enforcedVoteEntry/VoteSlots/AuctionSlot and AuthAccount(s): if protocol constrains counts (e.g., <= 8 vote slots, <= 4 auth accounts), ensure model-level or request-level validation exists.
Also applies to: 2326-2335, 2386-2395
2450-2450: New error/result codes — propagate to exceptions and testsMake sure these codes surface through client exceptions and have tests asserting correct mapping/messages for AMM and Clawback flows.
Also applies to: 2486-2486, 2535-2544
xrpl/models/transactions/account_set.py (4)
18-18: Rename typo: _SPECIAL_CASE_TRANFER_RATE → _SPECIAL_CASE_TRANSFER_RATESpelling nit; keeps consistency and avoids future slips.
-_SPECIAL_CASE_TRANFER_RATE: Final[int] = 0 +_SPECIAL_CASE_TRANSFER_RATE: Final[int] = 0 @@ - and self.transfer_rate != _SPECIAL_CASE_TRANFER_RATE + and self.transfer_rate != _SPECIAL_CASE_TRANSFER_RATEAlso applies to: 258-260
264-270: Domain validation: optionally enforce hex-onlySpec requires hex-encoded domain. Consider validating characters to preempt server-side rejection.
def _get_domain_error(self: Self) -> Optional[str]: - if self.domain is not None and self.domain.lower() != self.domain: - return f"Domain {self.domain} is not lowercase" - if self.domain is not None and len(self.domain) > _MAX_DOMAIN_LENGTH: + if self.domain is not None and self.domain.lower() != self.domain: + return f"Domain {self.domain} is not lowercase" + if self.domain is not None: + # hex-only (lowercase) per spec + import re + if not re.fullmatch(r"[0-9a-f]*", self.domain): + return "Domain must be hex-encoded (lowercase 0-9a-f)." + if self.domain is not None and len(self.domain) > _MAX_DOMAIN_LENGTH: return f"Must not be longer than {_MAX_DOMAIN_LENGTH} characters" return None
276-303: nftoken_minter validation logic LGTM; add negative testThe three cases cover presence/absence well. Please add a test asserting each branch.
27-40: Enum doc link — consider pinning to section anchorThe link is correct today. If docs move, tests may still rely on it. Optional: add a brief note with the commit/anchor used at time of change.
xrpl/models/transactions/xchain_commit.py (2)
1-2: Nit: article usage in docstring.Use “an XChainCommit” instead of “a XChainCommit”.
-"""Model for a XChainCommit transaction type.""" +"""Model for an XChainCommit transaction type."""
32-38: Optionally validatexchain_claim_idwhen provided as string.Guard against non-numeric strings to fail fast and mirror validation style used elsewhere.
@@ class XChainCommit(Transaction): @@ xchain_claim_id: Union[int, str] = REQUIRED # type: ignore @@ """ @@ """ @@ + def _get_errors(self) -> Dict[str, str]: + errors = super()._get_errors() + if isinstance(self.xchain_claim_id, str) and not self.xchain_claim_id.isnumeric(): + errors["xchain_claim_id"] = "`xchain_claim_id` must be numeric." + return errorsxrpl/models/transactions/did_set.py (2)
15-16: Make the hex intent explicit (readability).Anchors are implied via
fullmatch, but using an anchored raw string improves clarity.-HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*") +HEX_REGEX: Final[Pattern[str]] = re.compile(r"^[A-Fa-f0-9]*$")
53-61: Minor copy edit in error message.Use “or” for the list and tighten phrasing.
- errors["did_set"] = "Must have one of `did_document`, `data`, and `uri`." + errors["did_set"] = "Must provide at least one of `did_document`, `data`, or `uri`."xrpl/models/transactions/xchain_account_create_commit.py (2)
1-2: Nit: article usage in docstring.Use “an XChainAccountCreateCommit”.
-"""Model for a XChainAccountCreateCommit transaction type.""" +"""Model for an XChainAccountCreateCommit transaction type."""
64-73: Also validatexchain_claim_idwhen it’s a string.Adds parity with existing numeric checks for
signature_rewardandamount.def _get_errors(self: Self) -> Dict[str, str]: errors = super()._get_errors() if self.signature_reward != REQUIRED and not self.signature_reward.isnumeric(): errors["signature_reward"] = "`signature_reward` must be numeric." if self.amount != REQUIRED and not self.amount.isnumeric(): errors["amount"] = "`amount` must be numeric." + if isinstance(getattr(self, "xchain_claim_id", None), str) and not self.xchain_claim_id.isnumeric(): + errors["xchain_claim_id"] = "`xchain_claim_id` must be numeric." + return errorsxrpl/models/transactions/amm_bid.py (2)
20-22: Align with KW-only dataclass pattern used by other transactions.For consistency with DIDSet and XChain* models, enable keyword-only init via
KW_ONLY_DATACLASS.-from xrpl.models.utils import require_kwargs_on_init +from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init @@ -@dataclass(frozen=True) +@dataclass(frozen=True, **KW_ONLY_DATACLASS) class AMMBid(Transaction):
58-63: Optional: enforce “no sender in auth_accounts”.Spec text says the sender cannot be included. Add a check to prevent this.
def _get_errors(self: AMMBid) -> Dict[str, str]: return { key: value for key, value in { **super()._get_errors(), - "auth_accounts": self._get_auth_accounts_error(), + "auth_accounts": self._get_auth_accounts_error(), + "auth_accounts_sender": ( + "auth_accounts cannot include the transaction sender" + if self.auth_accounts + and any(a.account == getattr(self, "account", None) for a in self.auth_accounts) + else None + ), }.items() if value is not None }xrpl/core/binarycodec/types/issue.py (3)
32-37: Align docs with behavior (dict-only input).Docstring promises “string or dictionary” but the implementation is dict-only. Tighten wording.
Apply:
- Construct an Issue object from a string or dictionary representation - of an issued currency. + Construct an Issue object from a dictionary representation + of a currency (XRP or issued).Also applies to: 41-43
58-64: Use or validate length_hint (silence ARG003, catch malformed buffers).Either acknowledge the param or validate it (20 or 40 bytes).
Apply one of:
def from_parser( cls: Type[Issue], parser: BinaryParser, length_hint: Optional[int] = None, ) -> Issue: - """ + """ Construct an Issue object from an existing BinaryParser. @@ - currency = Currency.from_parser(parser) + # Optional sanity check for variable-length encoded fields + if length_hint is not None and length_hint not in (Currency.LENGTH, Currency.LENGTH + AccountID.LENGTH): + raise XRPLBinaryCodecException( + f"Invalid Issue length: expected 20 or 40, got {length_hint}." + ) + currency = Currency.from_parser(parser)or minimally:
) -> Issue: - """ + """ Construct an Issue object from an existing BinaryParser. @@ - currency = Currency.from_parser(parser) + _ = length_hint # avoid ARG003 + currency = Currency.from_parser(parser)Also applies to: 71-79
81-94: Narrow return type of to_json().This always returns a dict; remove the stray Union[str, …].
Apply:
- def to_json(self: Issue) -> Union[str, Dict[Any, Any]]: + def to_json(self: Issue) -> Dict[Any, Any]:xrpl/models/transactions/xchain_add_account_create_attestation.py (2)
1-1: Grammar nits in user-facing docs (“An XChain…”)Docs are published; small polish.
Apply:
-"""Model for a XChainAddAccountCreateAttestation transaction type.""" +"""Model for an XChainAddAccountCreateAttestation transaction type.""" @@ - The XChainAddAccountCreateAttestation transaction provides an attestation - from a witness server that a XChainAccountCreateCommit transaction occurred + The XChainAddAccountCreateAttestation transaction provides an attestation + from a witness server that an XChainAccountCreateCommit transaction occurredAlso applies to: 22-26
28-115: Consider basic value validation via _get_errors (addresses, hex).Add lightweight checks to fail fast on obvious mistakes: base58/X-address format for accounts; hex for public_key/signature; non-negative integer/str for xchain_account_create_count.
I can draft an _get_errors implementation aligned with existing validators (e.g., xrpl.core.addresscodec, regex for hex). Want me to add it?
xrpl/models/transactions/amm_withdraw.py (2)
14-15: Make dataclass KW-only like other transaction models.Keep consistency with newer models and IDE hints.
Apply:
-from xrpl.models.utils import require_kwargs_on_init +from xrpl.models.utils import require_kwargs_on_init, KW_ONLY_DATACLASS @@ -@dataclass(frozen=True) +@dataclass(frozen=True, **KW_ONLY_DATACLASS) class AMMWithdraw(Transaction):Also applies to: 47-49
17-45: Optional: enforce flag-field consistency.Examples:
- TF_TWO_ASSET should generally require amount and amount2.
- TF_SINGLE_ASSET/TF_ONE_ASSET_LP_TOKEN are mutually exclusive with TF_TWO_ASSET.
- If TF_LIMIT_LP_TOKEN set, lp_token_in should be present.
I can add cross-flag validations to _get_errors mirroring AMM spec — proceed?
Also applies to: 65-88
| - Function to parse the final account balances from a transaction's metadata | ||
| - Function to parse order book changes from a transaction's metadata | ||
| - Support for Ed25519 seeds that don't use the `sEd` prefix | ||
| - Support for Automated Market Maker (AMM) transactions and requests as defined in XLS-30. |
There was a problem hiding this comment.
Do not rewrite historical release notes; move AMM entry to Unreleased
Adding AMM support under 1.7.0 (dated 2022-10-12) misrepresents history. Put the AMM bullet under Unreleased (or the next version) and preserve the Ed25519 bullet unchanged.
Apply:
## [[Unreleased]]
+### Added:
+- Support for Automated Market Maker (AMM) transactions and requests as defined in XLS-30.
+
## [1.7.0] - 2022-10-12
### Added:
@@
-- Support for Automated Market Maker (AMM) transactions and requests as defined in XLS-30.📝 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.
| - Support for Automated Market Maker (AMM) transactions and requests as defined in XLS-30. | |
| ## [[Unreleased]] | |
| ### Added: | |
| - Support for Automated Market Maker (AMM) transactions and requests as defined in XLS-30. | |
| ## [1.7.0] - 2022-10-12 | |
| ### Added: |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 16, the AMM support bullet was incorrectly added
under the historical 1.7.0 release; move the "Support for Automated Market Maker
(AMM) transactions and requests as defined in XLS-30." line out of the 1.7.0
section and place it under the Unreleased section (or the next upcoming version)
instead, ensuring the 1.7.0 entry remains exactly as originally published
(including the Ed25519 bullet) and update only the placement of the AMM bullet
without altering historical text.
| "UInt192": 21, | ||
| "UInt384": 22, | ||
| "UInt512": 23, | ||
| "Issue": 24, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add Issue type — ensure codec support and exports wired up
Good addition. Please confirm the new binary codec type is implemented and exported so encoding/decoding works end-to-end.
Run:
🏁 Script executed:
#!/bin/bash
# Check Issue type exists and is exported
fd -a --max-depth 3 'issue.py' xrpl | sed -n '1,120p'
rg -nP '^class\s+Issue\b' xrpl
rg -nP 'from\s+\.issue\s+import\s+Issue' xrpl/core/binarycodec/types/__init__.py
# Light sanity: search for usages of Asset/Asset2 in models/tests
rg -n 'Asset2|Asset\b' xrpl | sed -n '1,200p'Length of output: 626
Export Issue in types initializer
Add the following line to xrpl/core/binarycodec/types/__init__.py to wire up the new type:
-from .accountroot import AccountRoot
+from .accountroot import AccountRoot
...
+from .issue import IssueThis ensures Issue is exported for encoding/decoding.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In xrpl/core/binarycodec/types/__init__.py (add near other type exports), the
new Issue type is not exported from the package, so encoding/decoding won't find
it; update the types initializer to import and re-export the Issue type (i.e.,
add the corresponding import and include Issue in the module's exports/__all__)
so the Issue definition in definitions.json is wired into the package.
| "NegativeUNL": 78, | ||
| "NFTokenPage": 80, | ||
| "NFTokenOffer": 55, | ||
| "AMM": 121, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New ledger entry type AMM (121) — confirm canonical code
Looks right, but please verify this numeric code matches rippled’s canonical definitions to avoid wire incompatibility.
🌐 Web query:
What is the canonical numeric code for the AMM ledger entry type in XRPL (rippled)?
💡 Result:
The AMM ledger-entry space key is 0x0041 (decimal 65). [1][2]
Sources:
[1] XRPL AMM ledger-entry docs — AMM ID Format (shows space key 0x0041).
[2] XRPL dev docs / XLS-30d AMM reference (AMM object description including the space key).
Update AMM ledger entry type code to canonical value (65)
Change the AMM entry in xrpl/core/binarycodec/definitions/definitions.json from 121 to 65 to align with rippled’s ledger-entry space key (0x0041, decimal 65).
- "AMM": 121,
+ "AMM": 65,🤖 Prompt for AI Agents
In xrpl/core/binarycodec/definitions/definitions.json around line 48, the AMM
ledger entry type is set to 121 but should be the canonical rippled value 65;
update the "AMM" entry value from 121 to 65 (decimal) so it matches rippled’s
ledger-entry space key (0x0041), and run any JSON linting/format checks to
ensure the file remains valid.
| if XRPModel.is_dict_of_model(value): | ||
| currency_bytes = bytes(Currency.from_value(value["currency"])) | ||
| return cls(currency_bytes) | ||
|
|
||
| if IssuedCurrencyModel.is_dict_of_model(value): | ||
| currency_bytes = bytes(Currency.from_value(value["currency"])) | ||
| issuer_bytes = bytes(AccountID.from_value(value["issuer"])) | ||
| return cls(currency_bytes + issuer_bytes) | ||
|
|
||
| raise XRPLBinaryCodecException( | ||
| "Invalid type to construct an Issue: expected str or dict," | ||
| f" received {value.__class__.__name__}." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix XRP-vs-IOU detection to avoid invalid serializations (missing issuer).
A dict like {"currency":"USD"} currently matches XRPModel.is_dict_of_model and gets encoded without issuer, which later breaks to_json(). Gate XRP strictly on currency == "XRP" and require issuer for non-XRP.
Apply:
- if XRPModel.is_dict_of_model(value):
- currency_bytes = bytes(Currency.from_value(value["currency"]))
- return cls(currency_bytes)
-
- if IssuedCurrencyModel.is_dict_of_model(value):
- currency_bytes = bytes(Currency.from_value(value["currency"]))
- issuer_bytes = bytes(AccountID.from_value(value["issuer"]))
- return cls(currency_bytes + issuer_bytes)
-
- raise XRPLBinaryCodecException(
- "Invalid type to construct an Issue: expected str or dict,"
- f" received {value.__class__.__name__}."
- )
+ if not isinstance(value, dict):
+ raise XRPLBinaryCodecException("Invalid type to construct an Issue: expected dict.")
+
+ # XRP: exactly {"currency": "XRP"}
+ if value.get("currency") == "XRP" and len(value) == 1:
+ return cls(bytes(Currency.from_value("XRP")))
+
+ # IOU: require issuer
+ if "currency" in value and "issuer" in value and IssuedCurrencyModel.is_dict_of_model(value):
+ currency_bytes = bytes(Currency.from_value(value["currency"]))
+ issuer_bytes = bytes(AccountID.from_value(value["issuer"]))
+ return cls(currency_bytes + issuer_bytes)
+
+ raise XRPLBinaryCodecException(
+ "Invalid dict to construct an Issue: expected {'currency': 'XRP'} "
+ "or {'currency': <code>, 'issuer': <address>}."
+ )📝 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.
| if XRPModel.is_dict_of_model(value): | |
| currency_bytes = bytes(Currency.from_value(value["currency"])) | |
| return cls(currency_bytes) | |
| if IssuedCurrencyModel.is_dict_of_model(value): | |
| currency_bytes = bytes(Currency.from_value(value["currency"])) | |
| issuer_bytes = bytes(AccountID.from_value(value["issuer"])) | |
| return cls(currency_bytes + issuer_bytes) | |
| raise XRPLBinaryCodecException( | |
| "Invalid type to construct an Issue: expected str or dict," | |
| f" received {value.__class__.__name__}." | |
| ) | |
| if not isinstance(value, dict): | |
| raise XRPLBinaryCodecException("Invalid type to construct an Issue: expected dict.") | |
| # XRP: exactly {"currency": "XRP"} | |
| if value.get("currency") == "XRP" and len(value) == 1: | |
| return cls(bytes(Currency.from_value("XRP"))) | |
| # IOU: require issuer | |
| if "currency" in value and "issuer" in value and IssuedCurrencyModel.is_dict_of_model(value): | |
| currency_bytes = bytes(Currency.from_value(value["currency"])) | |
| issuer_bytes = bytes(AccountID.from_value(value["issuer"])) | |
| return cls(currency_bytes + issuer_bytes) | |
| raise XRPLBinaryCodecException( | |
| "Invalid dict to construct an Issue: expected {'currency': 'XRP'} " | |
| "or {'currency': <code>, 'issuer': <address>}." | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
53-56: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In xrpl/core/binarycodec/types/issue.py around lines 44 to 56, the current dict
detection treats any dict with a "currency" as XRP and omits the issuer; change
the logic so that if the value is a dict and value["currency"] == "XRP" you
encode as XRP (currency_bytes only), otherwise for dicts require an "issuer"
field and encode as issued currency (currency_bytes + issuer_bytes); if a
non-XRP dict lacks "issuer", raise XRPLBinaryCodecException indicating missing
issuer for non-XRP currency.
| # Do not fail parsing if we encounter an unknown arg | ||
| logging.debug( | ||
| f"{param} not a valid parameter for {cls.__name__}" | ||
| ) | ||
| continue | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Silently ignoring unknown keys may hide bugs.
Switching from raising to ignoring can mask typos and schema drift. Consider a strict flag (default True) to retain current safety for writers, while allowing leniency for readers.
Want a patch to introduce a strict: bool = True parameter to from_dict and plumb it through?
| The parameters are different depending on if this is before or after the | ||
| `XRPFees Amendment<https://xrpl.org/known-amendments.html#xrpfees>`_ | ||
| Before the XRPFees Amendment which was proposed in rippled 1.10.0 | ||
| base_fee, reference_fee_units, reserve_base, and reserve_increment | ||
| were required fields. | ||
| After the XRPFees Amendment, base_fee_drops, reserve_base_drops, | ||
| and reserve_increment_drops are required fields. | ||
| No SetFee Pseudo Transaction should contain fields from BOTH before | ||
| and after the XRPFees amendment. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce “don’t mix pre/post XRPFees fields” and completeness
Model allows invalid combos (mixing sets, partial sets, or none). Add validation to enforce exactly one complete set.
Apply (also add the missing typing import):
- from typing import Optional
+ from typing import Optional, Dict
@@
class SetFee(PseudoTransaction):
@@
No SetFee Pseudo Transaction should contain fields from BOTH before
and after the XRPFees amendment.
"""
@@
transaction_type: PseudoTransactionType = field(
default=PseudoTransactionType.SET_FEE,
init=False,
)
+
+ def _get_errors(self) -> Dict[str, str]:
+ errors: Dict[str, str] = super()._get_errors()
+ pre = [self.base_fee, self.reference_fee_units, self.reserve_base, self.reserve_increment]
+ post = [self.base_fee_drops, self.reserve_base_drops, self.reserve_increment_drops]
+ pre_present = any(v is not None for v in pre)
+ post_present = any(v is not None for v in post)
+ if pre_present and post_present:
+ errors["SetFee"] = "Do not mix pre- and post-amendment fields."
+ return errors
+ if not pre_present and not post_present:
+ errors["SetFee"] = "Provide either all pre-amendment or all post-amendment fields."
+ return errors
+ if pre_present and not all(v is not None for v in pre):
+ errors["SetFee"] = "Pre-amendment fields must either all be set or none."
+ if post_present and not all(v is not None for v in post):
+ errors["SetFee"] = "Post-amendment fields must either all be set or none."
+ return errors📝 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.
| The parameters are different depending on if this is before or after the | |
| `XRPFees Amendment<https://xrpl.org/known-amendments.html#xrpfees>`_ | |
| Before the XRPFees Amendment which was proposed in rippled 1.10.0 | |
| base_fee, reference_fee_units, reserve_base, and reserve_increment | |
| were required fields. | |
| After the XRPFees Amendment, base_fee_drops, reserve_base_drops, | |
| and reserve_increment_drops are required fields. | |
| No SetFee Pseudo Transaction should contain fields from BOTH before | |
| and after the XRPFees amendment. | |
| """ | |
| ++ b/xrpl/models/transactions/pseudo_transactions/set_fee.py | |
| @@ | |
| from typing import Optional, Dict | |
| @@ class SetFee(PseudoTransaction): | |
| transaction_type: PseudoTransactionType = field( | |
| default=PseudoTransactionType.SET_FEE, | |
| init=False, | |
| ) | |
| def _get_errors(self) -> Dict[str, str]: | |
| errors: Dict[str, str] = super()._get_errors() | |
| pre = [ | |
| self.base_fee, | |
| self.reference_fee_units, | |
| self.reserve_base, | |
| self.reserve_increment, | |
| ] | |
| post = [ | |
| self.base_fee_drops, | |
| self.reserve_base_drops, | |
| self.reserve_increment_drops, | |
| ] | |
| pre_present = any(v is not None for v in pre) | |
| post_present = any(v is not None for v in post) | |
| if pre_present and post_present: | |
| errors["SetFee"] = "Do not mix pre- and post-amendment fields." | |
| return errors | |
| if not pre_present and not post_present: | |
| errors["SetFee"] = "Provide either all pre-amendment or all post-amendment fields." | |
| return errors | |
| if pre_present and not all(v is not None for v in pre): | |
| errors["SetFee"] = "Pre-amendment fields must either all be set or none." | |
| if post_present and not all(v is not None for v in post): | |
| errors["SetFee"] = "Post-amendment fields must either all be set or none." | |
| return errors |
| base_fee: Optional[str] = None | ||
| """ | ||
| The charge, in drops of XRP, for the reference transaction, as hex. (This is the | ||
| transaction cost before scaling for load.) This field is required. | ||
| :meta hide-value: | ||
| """ | ||
|
|
||
| reference_fee_units: int = REQUIRED # type: ignore | ||
| reference_fee_units: Optional[int] = None | ||
| """ | ||
| The cost, in fee units, of the reference transaction. This field is required. | ||
| :meta hide-value: | ||
| """ | ||
|
|
||
| reserve_base: int = REQUIRED # type: ignore | ||
| reserve_base: Optional[int] = None | ||
| """ | ||
| The base reserve, in drops. This field is required. | ||
| :meta hide-value: | ||
| """ | ||
|
|
||
| reserve_increment: int = REQUIRED # type: ignore | ||
| reserve_increment: Optional[int] = None | ||
| """ | ||
| The incremental reserve, in drops. This field is required. | ||
| :meta hide-value: | ||
| """ |
There was a problem hiding this comment.
Docstrings still say “required” though fields are Optional
These fields are conditionally required (pre-amendment). Update wording to prevent confusion for users.
Apply (example for one field; mirror for the rest in this block):
- The charge, in drops of XRP, for the reference transaction, as hex. (This is the
- transaction cost before scaling for load.) This field is required.
+ The charge, in drops of XRP, for the reference transaction, as hex. (This is the
+ transaction cost before scaling for load.) Required for pre‑XRPFees amendment; otherwise unset.📝 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.
| base_fee: Optional[str] = None | |
| """ | |
| The charge, in drops of XRP, for the reference transaction, as hex. (This is the | |
| transaction cost before scaling for load.) This field is required. | |
| :meta hide-value: | |
| """ | |
| reference_fee_units: int = REQUIRED # type: ignore | |
| reference_fee_units: Optional[int] = None | |
| """ | |
| The cost, in fee units, of the reference transaction. This field is required. | |
| :meta hide-value: | |
| """ | |
| reserve_base: int = REQUIRED # type: ignore | |
| reserve_base: Optional[int] = None | |
| """ | |
| The base reserve, in drops. This field is required. | |
| :meta hide-value: | |
| """ | |
| reserve_increment: int = REQUIRED # type: ignore | |
| reserve_increment: Optional[int] = None | |
| """ | |
| The incremental reserve, in drops. This field is required. | |
| :meta hide-value: | |
| """ | |
| base_fee: Optional[str] = None | |
| """ | |
| The charge, in drops of XRP, for the reference transaction, as hex. (This is the | |
| transaction cost before scaling for load.) Required for pre-XRPFees amendment; otherwise unset. | |
| :meta hide-value: | |
| """ | |
| reference_fee_units: Optional[int] = None | |
| """ | |
| The cost, in fee units, of the reference transaction. This field is required. | |
| :meta hide-value: | |
| """ | |
| reserve_base: Optional[int] = None | |
| """ | |
| The base reserve, in drops. This field is required. | |
| :meta hide-value: | |
| """ | |
| reserve_increment: Optional[int] = None | |
| """ | |
| The incremental reserve, in drops. This field is required. | |
| :meta hide-value: | |
| """ |
🤖 Prompt for AI Agents
In xrpl/models/transactions/pseudo_transactions/set_fee.py around lines 40 to
67, the docstrings incorrectly state these fields are "required" while the type
is Optional; change the wording to indicate they are conditionally required
pre-amendment (or similar phrasing) to avoid confusion. For each field
(base_fee, reference_fee_units, reserve_base, reserve_increment) update the
docstring text to say something like "This field is required before [specific
amendment] is enabled" or "This field is required on pre-amendment ledgers",
ensuring consistent phrasing across all four entries and keeping the meta
hide-value note intact.
| network_id: Optional[int] = None | ||
| """The network id of the transaction.""" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm NetworkID end-to-end.
Ensure NetworkID is supported in:
- binary codec definitions (field present, correct type)
- signing/encoding/decoding round-trips
🏁 Script executed:
#!/bin/bash
# Verify presence across codebase and fixtures.
rg -nC2 -e '\bNetworkID\b' -e '\bnetwork_id\b' --type=py --type=jsonLength of output: 382
Implement end-to-end support for network_id
network_id is only declared in the model; it must be added to the binary codec definitions and covered by encoding/decoding/signing round-trip tests.
🤖 Prompt for AI Agents
In xrpl/models/transactions/transaction.py around lines 252 to 254, the
network_id field is only declared but not included in binary codec handling; add
network_id to the transaction binary codec definitions (in the correct TLV/field
order and with proper type/length), handle presence/absence (Optional) in both
encode and decode paths, ensure it is included when computing signing/hashing
and when serializing for wire format, and add unit tests that perform
encode→decode and sign→verify round-trips covering cases with and without
network_id to validate encoding, decoding, and signing behavior.
| HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
HEX regex should be anchored and non-empty.
Current pattern matches empty strings and substrings. Anchor and require at least one hex char.
-HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*")
+HEX_REGEX: Final[Pattern[str]] = re.compile(r"^[A-Fa-f0-9]+$")If empty string should be allowed, use r"^[A-Fa-f0-9]*$" instead.
📝 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.
| HEX_REGEX: Final[Pattern[str]] = re.compile("[a-fA-F0-9]*") | |
| # xrpl/models/utils.py | |
| HEX_REGEX: Final[Pattern[str]] = re.compile(r"^[A-Fa-f0-9]+$") |
🤖 Prompt for AI Agents
In xrpl/models/utils.py around lines 10-11, the HEX_REGEX pattern currently uses
"[a-fA-F0-9]*" which allows empty strings and matches substrings; change it to
an anchored, non-empty pattern by using "^" and "$" and a "+" quantifier (i.e.
require at least one hex char) so the regex becomes anchored and rejects empty
strings; if empty strings must be allowed instead, use the anchored "*" variant
with "^" and "$".
| def _is_kw_only_attr_defined_in_dataclass() -> bool: | ||
| """ | ||
| Returns: | ||
| Utility function to determine if the Python interpreter's version is older | ||
| than 3.10. This information is used to check the presence of KW_ONLY attribute | ||
| in the dataclass | ||
| For ease of understanding, the output of this function should be equivalent to the | ||
| below code, unless the `kw_only` attribute is backported to older versions of | ||
| Python interpreter | ||
| Returns: | ||
| if sys.version_info.major < 3: | ||
| return True | ||
| return sys.version_info.minor < 10 | ||
| """ | ||
| return "kw_only" in dataclass.__kwdefaults__ |
There was a problem hiding this comment.
Guard against None kwdefaults and fix misleading docstring.
dataclass.__kwdefaults__ can be None on older Pythons, causing a TypeError. Also the docstring describes version checks that aren’t implemented.
-def _is_kw_only_attr_defined_in_dataclass() -> bool:
- """
- Returns:
- Utility function to determine if the Python interpreter's version is older
- than 3.10. This information is used to check the presence of KW_ONLY attribute
- in the dataclass
-
- For ease of understanding, the output of this function should be equivalent to the
- below code, unless the `kw_only` attribute is backported to older versions of
- Python interpreter
-
- Returns:
- if sys.version_info.major < 3:
- return True
- return sys.version_info.minor < 10
- """
- return "kw_only" in dataclass.__kwdefaults__
+def _is_kw_only_attr_defined_in_dataclass() -> bool:
+ """Return True if `dataclasses.dataclass` supports the `kw_only` parameter (Py ≥ 3.10)."""
+ kwdefaults = (getattr(dataclass, "__kwdefaults__", {}) or {})
+ return "kw_only" in kwdefaults📝 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.
| def _is_kw_only_attr_defined_in_dataclass() -> bool: | |
| """ | |
| Returns: | |
| Utility function to determine if the Python interpreter's version is older | |
| than 3.10. This information is used to check the presence of KW_ONLY attribute | |
| in the dataclass | |
| For ease of understanding, the output of this function should be equivalent to the | |
| below code, unless the `kw_only` attribute is backported to older versions of | |
| Python interpreter | |
| Returns: | |
| if sys.version_info.major < 3: | |
| return True | |
| return sys.version_info.minor < 10 | |
| """ | |
| return "kw_only" in dataclass.__kwdefaults__ | |
| def _is_kw_only_attr_defined_in_dataclass() -> bool: | |
| """Return True if `dataclasses.dataclass` supports the `kw_only` parameter (Py ≥ 3.10).""" | |
| kwdefaults = (getattr(dataclass, "__kwdefaults__", {}) or {}) | |
| return "kw_only" in kwdefaults |
🤖 Prompt for AI Agents
In xrpl/models/utils.py around lines 20 to 36, the function
_is_kw_only_attr_defined_in_dataclass unconditionally accesses
dataclass.__kwdefaults__ which can be None on some Python versions and the
docstring incorrectly describes a version check not present; update the
implementation to safely handle None (e.g., retrieve __kwdefaults__ with getattr
or check if dataclass.__kwdefaults__ is truthy before using "in") and simplify
the docstring to accurately state that the function returns whether the
dataclass defines a "kw_only" default by safely checking __kwdefaults__
(guarding against None) rather than describing Python version logic.
7 participants
High Level Overview of Change
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Summary by CodeRabbit