feat: Support BatchV1_1#3371
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtends the XLS-56 Batch signing protocol to V1_1 by adding ChangesXLS-56 Batch V1_1 Signing Payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
50-82: 💤 Low valueConsider adding a test case for
TicketSequence.The test transaction in
beforeEachhas noSequencefield, which results ingetBatchSeqValuereturning 0. WhilecombineBatchSignerstests useSequence: 215, there's no test coverage for theTicketSequencefallback path whereSequenceis explicitly 0 andTicketSequenceprovides the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xrpl/test/wallet/batchSigner.test.ts` around lines 50 - 82, The test transaction in the beforeEach hook lacks coverage for the TicketSequence fallback scenario in getBatchSeqValue. Currently, the RawTransaction objects have Sequence values set (215 and 470) but no TicketSequence field, so the fallback path where Sequence is 0 and TicketSequence should be used is not being tested. Add a new test case or modify one of the existing RawTransaction objects to set Sequence to 0 and include a TicketSequence field to ensure the getBatchSeqValue function correctly returns the TicketSequence value when Sequence is explicitly 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ripple-binary-codec/src/binary.ts`:
- Around line 216-218: The error message in the batch.flags null check uses
inconsistent quote styling with a backtick before flags and a single quote after
it. Fix this by updating the error message to use consistent backticks on both
sides of the field name, changing "No field `flags'" to "No field `flags`" to
match the quote style used in other error messages throughout the file.
---
Nitpick comments:
In `@packages/xrpl/test/wallet/batchSigner.test.ts`:
- Around line 50-82: The test transaction in the beforeEach hook lacks coverage
for the TicketSequence fallback scenario in getBatchSeqValue. Currently, the
RawTransaction objects have Sequence values set (215 and 470) but no
TicketSequence field, so the fallback path where Sequence is 0 and
TicketSequence should be used is not being tested. Add a new test case or modify
one of the existing RawTransaction objects to set Sequence to 0 and include a
TicketSequence field to ensure the getBatchSeqValue function correctly returns
the TicketSequence value when Sequence is explicitly 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 606b7a89-73e5-4777-bfdc-0e37c27a9a35
📒 Files selected for processing (6)
packages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/binary.tspackages/ripple-binary-codec/test/signing-data-encoding.test.tspackages/xrpl/HISTORY.mdpackages/xrpl/src/Wallet/batchSigner.tspackages/xrpl/test/wallet/batchSigner.test.ts
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| ## Unreleased | ||
|
|
||
| ### Changed | ||
| * Add XLS-56 Batch V1_1 support to `signMultiBatch` and `combineBatchSigners` ([XRPLF/rippled#6446](https://github.com/XRPLF/rippled/pull/6446)). |
There was a problem hiding this comment.
nit: A WIP tag will be helpful to identify that this PR has not been finalized yet.
| { | ||
| "name": "ripple-binary-codec", | ||
| "version": "2.8.0", | ||
| "version": "2.9.0-batch.1", |
There was a problem hiding this comment.
My understanding was that beta releases have a suffix of the form: 2.9.0-beta-batch-1.
Are we sure this released is not interpreted as a "General Availability" release by the community? @pdp2121
There was a problem hiding this comment.
Other feature beta/experimental releases are following the same pattern, for example:
https://github.com/XRPLF/xrpl.js/releases/tag/xrpl%404.7.0-smartcontract.0
ckeshava
left a comment
ckeshava
left a comment
There was a problem hiding this comment.
I suggest that we add BatchV1_1 feature into the xrpld.cfg file. This ensures that the CI-docker container enables this amendment + runs the tests with complete integrity.
Since this is a beta release anyway, we can refine the changes for GA release in the future. What do you think? @pdp2121
Yes this will not work atmm (I had to test using standalone), will do once the rippled PR is merged |
ckeshava
left a comment
ckeshava
left a comment
There was a problem hiding this comment.
How can we make a beta release with this PR? I don't expect the integration tests to pass.
(1) Disable the integration tests check for this specific PR
(2) Create a docker image with the custom rippled PR to complete the integ tests.
We will need to pick one of the two options.
| // `Counterparty` of one. | ||
| const involvedAccounts = new Set<string>() | ||
| transaction.RawTransactions.forEach((raw) => { | ||
| involvedAccounts.add(raw.RawTransaction.Account) |
There was a problem hiding this comment.
This format does not account for potential Delegate of the inner transaction. Please refer to line 427 in this file: https://github.com/XRPLF/rippled/pull/6446/changes#diff-a9cdff3331a64b75a44d24094b4699fd27c8e4fdc52537eb97f881ce8b28c2db
raw.RawTransaction.Delegate ?? raw.RawTransaction.Account would be more appropriate here.
2 participants
High Level Overview of Change
ripppled PR: XRPLF/rippled#6446
Related issues:
#3370
#3324
Type of Change
Did you update HISTORY.md?
Test Plan
Unit tests passed. Integration tests passed on standalone with BatchV1_1 enabled