Skip to content

Conversation

EssamTrmlabs
Copy link

@EssamTrmlabs EssamTrmlabs commented Sep 4, 2025

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update CHANGELOG.md?

  • Yes
  • No, this change does not impact library users

Test Plan

Summary by CodeRabbit

  • New Features
    • Added full support for Automated Market Maker (AMM) transactions and AMM info requests.
    • Introduced new transaction types: Clawback, CredentialCreate, DID Set/Delete, NFTokenModify, Oracle Set/Delete, and multiple XChain operations.
    • Enhanced transaction utilities: create from/encode to blobs, signature detection, and optional network ID.
  • Improvements
    • Updated fees to handle AMM creation.
    • Expanded binary codec and flags to support new assets and AMM behaviors.
  • Documentation
    • CHANGELOG updated to announce AMM transaction support.
  • Tests
    • Extensive unit tests added for AMM, SetFee formats, and new transaction models.
  • Chores
    • Added default CODEOWNERS.

pbjc and others added 26 commits October 26, 2022 16:23
* 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]>
Add default value of NULL for optional fields
Copy link
Contributor

coderabbitai bot commented Sep 4, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds AMM feature support (transactions, requests, flags, codec schema/type Issue), expands XChain, Oracle, DID, CredentialCreate models, updates transaction parsing/encoding (from_blob/from_xrpl, network_id, signing), adjusts SetFee (pre/post amendment), refactors AccountSet flags, updates fee calc to use owner-reserve for AMMCreate, and adds extensive tests/fixtures and metadata.

Changes

Cohort / File(s) Summary
Repo metadata
.github/CODEOWNERS, CHANGELOG.md
Add default CODEOWNERS; update changelog to note AMM support.
Binary codec schema & types
xrpl/core/binarycodec/definitions/definitions.json, xrpl/core/binarycodec/types/__init__.py, xrpl/core/binarycodec/types/issue.py, tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json
Add Issue type, AMM ledger/fields, AMM/NFT/XChain tx types/results; export Issue; implement Issue codec; expand fixtures with AMM transactions.
Async fee calculation
xrpl/asyncio/transaction/main.py
Rename account-delete fee helpers to owner-reserve; apply to AMMCreate; adjust client request method.
Core modeling/parsing utilities
xrpl/models/base_model.py, xrpl/models/utils.py, xrpl/models/transactions/transaction.py, xrpl/models/currencies/currency.py, xrpl/models/flags.py
Make BaseModel frozen; add abbreviations, literal handling, non-nesting export; add HEX_REGEX, KW_ONLY_DATACLASS, kw-only init behavior; add Transaction.from_blob/from_xrpl, network_id, is_signed/blob; adjust currency union order; add AMMDeposit/Withdraw flags.
Requests: AMM info
xrpl/models/requests/request.py, xrpl/models/requests/amm_info.py, xrpl/models/requests/__init__.py, tests/unit/models/requests/test_amm_info.py
Add RequestMethod.AMM_INFO; implement/export AMMInfo request; add unit test.
AMM transaction models
xrpl/models/transactions/amm_create.py, .../amm_deposit.py, .../amm_withdraw.py, .../amm_vote.py, .../amm_bid.py, .../amm_delete.py, xrpl/models/transactions/__init__.py, xrpl/models/auth_account.py, xrpl/models/__init__.py, tests/unit/models/transactions/test_amm_*, tests/unit/models/test_base_model.py
Add AMM tx models, flags, validation, AuthAccount model; export symbols; add comprehensive AMM tests and serialization test.
AccountSet flags refactor
xrpl/models/transactions/account_set.py
Split ASF vs TF flags; add new ASF/TF entries; update validation/messages; keyword-only, frozen dataclass.
Pseudo SetFee updates + tests
xrpl/models/transactions/pseudo_transactions/set_fee.py, tests/unit/models/transactions/test_pseudo_transactions.py
Support pre-/post-amendment SetFee fields; remove ledger_sequence; add tests for both formats.
XChain suite
xrpl/models/xchain_bridge.py, xrpl/models/transactions/xchain_* (AccountCreateCommit, AddAccountCreateAttestation, AddClaimAttestation, Claim, Commit, CreateBridge, CreateClaimID, ModifyBridge), xrpl/models/transactions/types/transaction_type.py
Add XChainBridge model; implement and export multiple XChain transaction models with validations; extend TransactionType enum.
Oracle models
xrpl/models/transactions/oracle_set.py, .../oracle_delete.py, xrpl/models/transactions/__init__.py
Add OracleSet (with PriceData and validations) and OracleDelete; export.
DID models
xrpl/models/transactions/did_set.py, .../did_delete.py, xrpl/models/transactions/__init__.py
Add DIDSet (validations) and DIDDelete; export.
CredentialCreate
xrpl/models/transactions/credential_create.py, xrpl/models/transactions/__init__.py
Add CredentialCreate transaction model; export.
Payment cosmetic
xrpl/models/transactions/payment.py
Remove a comment; no behavior change.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App
  participant TxModel as Transaction
  participant Codec as Binary Codec
  participant Net as Network/Client

  App->>TxModel: from_xrpl(value)
  activate TxModel
  TxModel->>TxModel: _process_xrpl_json + key/value normalization
  TxModel-->>App: Transaction instance
  App->>TxModel: blob()
  TxModel->>Codec: encode(to_xrpl())
  Codec-->>TxModel: hex blob
  TxModel-->>App: hex blob
  App->>TxModel: is_signed()
  TxModel-->>App: bool (single or multi-sig)

  rect rgba(200,230,255,0.3)
    note right of TxModel: New flow: from_blob
    App->>TxModel: from_blob(tx_blob)
    TxModel->>Codec: decode(tx_blob)
    Codec-->>TxModel: XRPL JSON
    TxModel-->>App: Transaction instance
  end
Loading
sequenceDiagram
  autonumber
  actor App
  participant Fee as asyncio.transaction.main
  participant Cl as Client
  participant Server as rippled (server_state)

  App->>Fee: calculate_base_fee(tx=AMMCreate, client=?)
  alt client provided
    Fee->>Cl: _request_impl(ServerState)
    Cl-->>Fee: server_state(reserve_inc)
    Fee->>Fee: _fetch_owner_reserve_fee(reserve_inc)
    Fee-->>App: owner-reserve-based fee
  else no client
    Fee-->>App: _OWNER_RESERVE_FEE
  end
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested reviewers

  • ckeshava
  • mvadari
  • pdp2121

Poem

I wiggle my whiskers at AMM dawn,
New flags unfurl, byte-fields yawn.
Bridges span, oracles sing,
DIDs tiptoe on ledger string.
From blobs to bids, I hop with glee—
Code carrots minted, LP for me! 🥕✨


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8b0b410 and 3483b7e.

📒 Files selected for processing (53)
  • .github/CODEOWNERS (1 hunks)
  • CHANGELOG.md (1 hunks)
  • tests/unit/core/binarycodec/fixtures/data/codec-fixtures.json (1 hunks)
  • tests/unit/models/requests/test_amm_info.py (1 hunks)
  • tests/unit/models/test_base_model.py (3 hunks)
  • tests/unit/models/transactions/test_amm_bid.py (1 hunks)
  • tests/unit/models/transactions/test_amm_create.py (1 hunks)
  • tests/unit/models/transactions/test_amm_delete.py (1 hunks)
  • tests/unit/models/transactions/test_amm_deposit.py (1 hunks)
  • tests/unit/models/transactions/test_amm_vote.py (1 hunks)
  • tests/unit/models/transactions/test_amm_withdraw.py (1 hunks)
  • tests/unit/models/transactions/test_pseudo_transactions.py (2 hunks)
  • xrpl/asyncio/transaction/main.py (4 hunks)
  • xrpl/core/binarycodec/definitions/definitions.json (16 hunks)
  • xrpl/core/binarycodec/types/__init__.py (2 hunks)
  • xrpl/core/binarycodec/types/issue.py (1 hunks)
  • xrpl/models/__init__.py (2 hunks)
  • xrpl/models/auth_account.py (1 hunks)
  • xrpl/models/base_model.py (10 hunks)
  • xrpl/models/currencies/currency.py (1 hunks)
  • xrpl/models/flags.py (1 hunks)
  • xrpl/models/requests/__init__.py (3 hunks)
  • xrpl/models/requests/amm_info.py (1 hunks)
  • xrpl/models/requests/request.py (2 hunks)
  • xrpl/models/transactions/__init__.py (5 hunks)
  • xrpl/models/transactions/account_set.py (8 hunks)
  • xrpl/models/transactions/amm_bid.py (1 hunks)
  • xrpl/models/transactions/amm_create.py (1 hunks)
  • xrpl/models/transactions/amm_delete.py (1 hunks)
  • xrpl/models/transactions/amm_deposit.py (1 hunks)
  • xrpl/models/transactions/amm_vote.py (1 hunks)
  • xrpl/models/transactions/amm_withdraw.py (1 hunks)
  • xrpl/models/transactions/clawback.py (1 hunks)
  • xrpl/models/transactions/credential_create.py (1 hunks)
  • xrpl/models/transactions/did_delete.py (1 hunks)
  • xrpl/models/transactions/did_set.py (1 hunks)
  • xrpl/models/transactions/nftoken_modify.py (1 hunks)
  • xrpl/models/transactions/oracle_delete.py (1 hunks)
  • xrpl/models/transactions/oracle_set.py (1 hunks)
  • xrpl/models/transactions/payment.py (0 hunks)
  • xrpl/models/transactions/pseudo_transactions/set_fee.py (2 hunks)
  • xrpl/models/transactions/transaction.py (9 hunks)
  • xrpl/models/transactions/types/transaction_type.py (2 hunks)
  • xrpl/models/transactions/xchain_account_create_commit.py (1 hunks)
  • xrpl/models/transactions/xchain_add_account_create_attestation.py (1 hunks)
  • xrpl/models/transactions/xchain_add_claim_attestation.py (1 hunks)
  • xrpl/models/transactions/xchain_claim.py (1 hunks)
  • xrpl/models/transactions/xchain_commit.py (1 hunks)
  • xrpl/models/transactions/xchain_create_bridge.py (1 hunks)
  • xrpl/models/transactions/xchain_create_claim_id.py (1 hunks)
  • xrpl/models/transactions/xchain_modify_bridge.py (1 hunks)
  • xrpl/models/utils.py (2 hunks)
  • xrpl/models/xchain_bridge.py (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@EssamTrmlabs EssamTrmlabs deleted the add-credential-create-transaction branch September 4, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants