xrpl-py icon indicating copy to clipboard operation
xrpl-py copied to clipboard

feat: add support for Batch amendment

Open mvadari opened this issue 1 year ago • 2 comments

High Level Overview of Change

This PR:

  • Adds models for the Batch transactions
  • Updates definitions.json to handle the new field types
  • Adds support to the binary codec to properly sign multi-account Batch transactions
  • Adds helper functions for signing and combining multi-account Batch transactions
  • Adds support to autofill for filling in the BatchTxn and TxIDs fields
  • Adds support for Batchnet to the faucets
  • Fixes a side issue where multisigned transactions weren't properly autofilled

Context of Change

https://github.com/XRPLF/rippled/pull/5060

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] 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)
  • [x] 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?

  • [x] Yes
  • [ ] No, this change does not impact library users

Test Plan

Added tests for the new features.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced support for batch transactions, including new transaction types and models.
    • Added functionality for ledger state fixes.
    • Enhanced spell-checking capabilities by recognizing the term "multisigned."
  • Bug Fixes

    • Improved transaction validation logic for batch transactions.
  • Tests

    • Expanded test coverage for batch transaction functionality and multisigned transactions.
    • Standardized naming conventions for test methods.
  • Documentation

    • Updated transaction type definitions to include new batch processing and ledger state fix types.

mvadari avatar Oct 10 '24 01:10 mvadari

Walkthrough

This update introduces comprehensive support for batch transactions (XLS-56d) in the XRPL Python library. It adds new transaction models, batch signing and combination utilities, batch encoding for signing, and test coverage. Numerous transaction flag interfaces are unified, and new type annotations are applied throughout transaction autofill and signing workflows.

Changes

Files / Groups Change Summary
.vscode/settings.json Added batch-related terms ("autofilling", "multiaccount", "multisigned", "multisigning") to spell checker words.
.ci-config/rippled.cfg Updated amendments: removed some, added Batch and others under new version headings.
CHANGELOG.md Documented batch transaction support, validation improvements, and other enhancements.
tools/generate_tx_models.py Added usage message for missing command-line arguments.
xrpl/core/binarycodec/main.py, xrpl/core/binarycodec/__init__.py Added encode_for_signing_batch for batch transaction signing; exported it.
xrpl/models/transactions/batch.py Introduced Batch transaction model, flags, and batch signer classes.
xrpl/models/transactions/types/transaction_type.py Added BATCH member to TransactionType enum.
xrpl/models/transactions/transaction.py Added TransactionFlag enum and interface; adjusted get_hash for batch flag.
xrpl/models/transactions/__init__.py Exported Batch-related and transaction flag classes.
xrpl/models/transactions/* (various transaction types) Unified flag interface base classes to use TransactionFlagInterface.
xrpl/models/transactions/delegate_set.py, tests/unit/models/transactions/test_delegate_set.py Updated non-delegable transaction logic and error messages for Batch.
xrpl/asyncio/transaction/main.py, xrpl/transaction/main.py Added batch transaction autofill/signing support; improved type annotations.
xrpl/transaction/batch_signers.py, xrpl/transaction/__init__.py Added batch multi-account signing and signer combination utilities; exported them.
xrpl/core/binarycodec/definitions/definitions.py Formatting and whitespace clean-up.
tests/unit/models/transactions/test_batch.py Added unit test for Batch model and flag propagation.
tests/unit/transaction/test_batch_signers.py Added unit tests for batch multi-account signing and signer combination.
tests/unit/core/binarycodec/test_main.py Added test for batch transaction signing encoding.
tests/integration/transactions/test_batch.py Added integration tests for batch transaction submission and multi-signing.
tests/integration/sugar/test_transaction.py Added async test for batch autofill; updated imports.
tests/unit/models/transactions/test_transaction.py Added test for multisigned transaction with x-addresses; refactored test names.
tests/integration/it_utils.py Removed explicit fee check argument in wallet funding helpers.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant BatchTx
    participant Wallet
    participant Client
    participant BatchSigners

    User->>BatchTx: Create Batch with raw_transactions
    User->>Client: autofill(BatchTx)
    Client->>BatchTx: Assign sequence, set flags, validate inner txns
    User->>Wallet: sign_multiaccount_batch(BatchTx, Wallet)
    Wallet->>BatchSigners: Sign Batch, add to BatchSigners
    User->>Client: submit(BatchTx with BatchSigners)
    Client-->>User: Submission result

Possibly related PRs

  • XRPLF/xrpl-py#708: Enhances model validation by adding type checks for parameters in base models; related to transaction validation improvements.
  • XRPLF/xrpl-py#840: Implements features related to account permission delegation and DelegateSet transaction type, overlapping with DelegateSet changes here.
  • XRPLF/xrpl-py#732: Adds Multi-Purpose Tokens (MPT) support including new transaction models and tests, overlapping with transaction flag interface unifications and enhancements.

Suggested reviewers

  • ckeshava

Poem

🐇 In fields of code where batchers leap,
New flags and signers now run deep.
Transactions hop in groups, not one—
With multi-sign, the work gets done!
Tests abound, and models grow,
As rabbits cheer, “Batch, away we go!”
✨🐰

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings

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.
    • Explain this complex logic.
    • 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. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • 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 src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai 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.

Documentation and Community

  • 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.

coderabbitai[bot] avatar Oct 10 '24 01:10 coderabbitai[bot]

cc @dangell7 @tequdev

mvadari avatar Oct 10 '24 02:10 mvadari

I could only get this to work by signing the interior transactions in addition to signing the full batch, otherwise I get an exception about being unable to retrieve the hash from an unsigned transaction. At least to my ignorance, that's surprising given [if I recall correctly] the spec specifically states that the transaction atoms are unsigned.

 def toy(url: str):
     xrpl_client = JsonRpcClient(url=url)
     alice = Wallet.from_seed(seed='OMITTED', algorithm=CryptoAlgorithm.ED25519)
     bob = Wallet.from_seed(seed='OMITTED', algorithm=CryptoAlgorithm.ED25519)
     alice_account_info = xrpl_client.request(AccountInfo(account=alice.address)).result['account_data']
     bob_account_info = xrpl_client.request(AccountInfo(account=bob.address)).result['account_data']
     alice_sequence_start = alice_account_info['Sequence']
     bob_sequence_start = bob_account_info['Sequence']
-    payment_alice_to_bob = Payment(account=alice.address, amount=str(1), destination=bob.address, fee=str(0), sequence=alice_sequence_start + 1)
-    payment_bob_to_alice = Payment(account=bob.address, amount=str(1), destination=alice.address, fee=str(0), sequence=bob_sequence_start)
+    payment_alice_to_bob = transaction.sign(Payment(account=alice.address, amount=str(1), destination=bob.address, fee=str(0), sequence=alice_sequence_start + 1), alice)
+    payment_bob_to_alice = transaction.sign(Payment(account=bob.address, amount=str(1), destination=alice.address, fee=str(0), sequence=bob_sequence_start), bob)
     base_fee = 10
     number_of_signers = 2
     number_of_transactions = 2
     fee = (number_of_signers + 2 + number_of_transactions) * base_fee
     batch = Batch(account=alice.address, raw_transactions=[payment_alice_to_bob, payment_bob_to_alice], sequence=alice_sequence_start, fee=str(fee))
     bob_consent = transaction.sign_multiaccount_batch(alice, batch)
     alice_submission = transaction.sign(bob_consent, alice)
     batch_response = transaction.submit(transaction=alice_submission, client=xrpl_client)
     print('Payment ' + ('successful' if batch_response.is_successful() else 'failed'))

Else:

Traceback (most recent call last):
  File "/Users/lmaisons/src/rippled_sandbox/main.py", line 155, in <module>
    sys.exit(main())
  File "/Users/lmaisons/src/rippled_sandbox/main.py", line 151, in main
    junk(url)
  File "/Users/lmaisons/src/rippled_sandbox/main.py", line 28, in toy
    bob_consent = transaction.sign_multiaccount_batch(alice, batch)
  File "/Users/lmaisons/src/xrpl-py/xrpl/transaction/batch_signers.py", line 38, in sign_multiaccount_batch
    "transaction_ids": [tx.get_hash() for tx in transaction.raw_transactions],
  File "/Users/lmaisons/src/xrpl-py/xrpl/transaction/batch_signers.py", line 38, in <listcomp>
    "transaction_ids": [tx.get_hash() for tx in transaction.raw_transactions],
  File "/Users/lmaisons/src/xrpl-py/xrpl/models/transactions/transaction.py", line 444, in get_hash
    raise XRPLModelException(
xrpl.models.exceptions.XRPLModelException: Cannot get the hash from an unsigned Transaction.

lmaisons avatar Apr 10 '25 15:04 lmaisons

@lmaisons the immediate cause to that issue is that the inner transactions don't have the tfInnerBatchTxn flag. It's added via autofill, which it looks like you're not using.

Let me see if I can come up with a better solution for that, where you don't need to use autofill. But in the meanwhile, adding that flag should fix your issue.

mvadari avatar Apr 10 '25 16:04 mvadari

Confirming this worked as described. Thank you.

-    payment_alice_to_bob = Payment(account=alice.address, amount=str(1), destination=bob.address, fee=str(0), sequence=alice_sequence_start + 1)
-    payment_bob_to_alice = Payment(account=bob.address, amount=str(1), destination=alice.address, fee=str(0), sequence=bob_sequence_start)
+    payment_alice_to_bob = Payment(account=alice.address, amount=str(1), destination=bob.address, fee=str(0), sequence=alice_sequence_start + 1, flags=TransactionFlag.TF_INNER_BATCH_TXN)
+    payment_bob_to_alice = Payment(account=bob.address, amount=str(1), destination=alice.address, fee=str(0), sequence=bob_sequence_start, flags=TransactionFlag.TF_INNER_BATCH_TXN)

lmaisons avatar Apr 10 '25 17:04 lmaisons

@lmaisons the latest commit should add the flag for you automatically now.

mvadari avatar Apr 10 '25 18:04 mvadari

Confirming the new patch makes things work without explicitly setting the flag - Thank you

lmaisons avatar Apr 10 '25 19:04 lmaisons

I didn't see a mention of batchnet in this PR. Did you intend to add that faucet-host in this python library? Here's the relevant file from JS library PR:

This appears to be a suitable place for such an addition.

ckeshava avatar Apr 15 '25 22:04 ckeshava

I didn't see a mention of batchnet in this PR. Did you intend to add that faucet-host in this python library? Here's the relevant file from JS library PR:

This appears to be a suitable place for such an addition.

I published a beta for xrpl.js, so the faucet was more necessary to include there. It's not super necessary for Python given that I haven't been asked to create a beta (given that Batchnet will no longer be necessary by the time this PR is merged).

mvadari avatar Apr 15 '25 22:04 mvadari

The develop Docker container needs to be updated before integration tests will pass. They do pass locally.

mvadari avatar May 27 '25 15:05 mvadari

@mvadari I only have one concern left that needs to be addressed. Apart from that, this PR looks good.

ckeshava avatar Jun 06 '25 17:06 ckeshava