[TA-4979] EncodeForSigningBatch support#145
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughSupport for encoding batch transactions into a canonical binary format for signing was added. New error variables and a batch prefix constant were introduced. The function Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Codec
Caller->>Codec: EncodeForSigningBatch(json)
Codec->>Codec: Validate 'flags' and 'txIDs' fields
Codec->>Codec: Check txIDs is array of strings
Codec->>Codec: Encode flags and txIDs length as UInt32
Codec->>Codec: Convert each txID to 256-bit hash
Codec->>Codec: Concatenate batch prefix, flags, length, txIDs
Codec-->>Caller: Return encoded string or error
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. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
binary-codec/codec_test.go (1)
652-738: Excellent comprehensive test coverage for batch encoding.The test suite thoroughly covers all scenarios:
Success Cases:
- Multiple transaction IDs with non-zero flags
- Single transaction ID with zero flags
Error Cases:
- Missing
flagsfield →ErrBatchFlagsFieldNotFound- Missing
txIDsfield →ErrBatchTxIDsFieldNotFound- Invalid
txIDstype →ErrBatchTxIDsNotArray- Non-string elements in
txIDs→ErrBatchTxIDNotStringThe expected output strings are correctly formatted with proper hex encoding, and all error assertions use the appropriate error types defined in the main implementation.
Minor Enhancement Suggestion:
Consider adding a test case for the integer overflow scenario to ensure the fix mentioned in the main implementation works correctly:{ description: "fail - txIDs array too large", input: map[string]any{ "flags": uint32(1), "txIDs": make([]any, math.MaxUint32+1), // This would trigger overflow }, output: "", expectedErr: errors.New("txIDs array too large"), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
binary-codec/codec.go(2 hunks)binary-codec/codec_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: florent-uzio
PR: Peersyst/xrpl-go#113
File: xrpl/transaction/credential_create.go:30-34
Timestamp: 2025-03-11T11:50:58.330Z
Learning: In PR #113 for xrpl-go, the `CredentialCreate` struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
🧬 Code Graph Analysis (2)
binary-codec/codec_test.go (1)
binary-codec/codec.go (5)
ErrBatchFlagsFieldNotFound(21-21)ErrBatchTxIDsFieldNotFound(23-23)ErrBatchTxIDsNotArray(25-25)ErrBatchTxIDNotString(27-27)EncodeForSigningBatch(129-175)
binary-codec/codec.go (3)
xrpl/wallet/wallet.go (1)
New(38-44)binary-codec/types/uint32.go (1)
UInt32(11-11)binary-codec/types/hash256.go (1)
NewHash256(9-13)
🪛 golangci-lint (1.64.8)
binary-codec/codec.go
[high] 152-152: G115: integer overflow conversion int -> uint32
(gosec)
🪛 GitHub Actions: XRPL-GO Lint and Test
binary-codec/codec.go
[error] 152-152: gosec G115: integer overflow conversion int -> uint32 detected. Code: 'txIDsLengthBytes, err := txIDsLengthType.FromJSON(uint32(len(txIDsInterface)))'.
🔇 Additional comments (2)
binary-codec/codec.go (2)
20-27: LGTM! Well-defined error messages for batch validation.The error variables follow the established pattern and provide clear, descriptive messages for different batch validation failures.
34-34: LGTM! Batch prefix constant follows the established pattern.The
batchPrefixconstant is consistent with other prefixes defined in the same block and uses the correct hex format.
2 participants
[TA-4979] EncodeForSigningBatch support
Description
This PR aims to add encoding support for batch transactions.
Type of change
Checklist:
Changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests