[TA-5091]: batch transaction integration tests#150
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 📝 WalkthroughWalkthroughThis set of changes introduces and tests support for batch transactions and multisigned batch transactions in the XRPL integration suite. It refactors fee calculation and signing logic for batch transactions, updates type assertions for transaction fields, and modifies the integration runner and test cases to support new options and signatures. New integration tests for batch transactions are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Runner as Runner
participant Client as Websocket Client
participant Wallet as Wallet
Test->>Runner: TestTransaction(flatTx, signer, expectedResult, opts)
Runner->>Runner: processTransaction(flatTx, signer, opts)
alt opts.SkipAutofill == false
Runner->>Client: Autofill(flatTx)
end
Runner->>Wallet: Sign(flatTx)
Runner->>Client: Submit(flatTx)
Client-->>Runner: SubmitResponse
Runner-->>Test: SubmitResponse
sequenceDiagram
participant Test as Batch Test
participant Runner as Runner
participant Wallet as Wallet
Test->>Runner: TestTransaction(batchTx, signer, "tesSUCCESS", opts)
Runner->>Wallet: SignMultiBatch(wallet, batchTx, opts)
Wallet->>Wallet: FromFlatBatchTransaction(batchTx)
Wallet->>Wallet: Sign each inner transaction
Wallet-->>Runner: Signed batchTx
Runner->>Client: Submit(signed batchTx)
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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: 3
🧹 Nitpick comments (2)
xrpl/transaction/integration/batch_test.go (1)
49-83: Consider expanding test coverage.The test only covers the happy path. Consider adding test cases for:
- Invalid batch transactions (e.g., exceeding the 8-transaction limit per batch)
- Batch transactions with failing inner transactions
- Batch transactions without the "all or nothing" flag
Would you like me to generate additional test cases to improve coverage?
xrpl/websocket/client.go (1)
140-159: Type assertion can be simplified using comma-ok pattern.The type assertion and comparison can be combined for cleaner code.
-if txType, ok := (*tx)["TransactionType"].(string); ok { - if acc, ok := (*tx)["Account"].(types.Address); txType == transaction.AccountDeleteTx.String() && ok { +if txType, ok := (*tx)["TransactionType"].(string); ok { + if txType == transaction.AccountDeleteTx.String() { + if acc, ok := (*tx)["Account"].(types.Address); ok { err := c.checkAccountDeleteBlockers(acc) if err != nil { return err } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
xrpl/rpc/helpers.go(1 hunks)xrpl/testutil/integration/faucet.go(1 hunks)xrpl/testutil/integration/runner.go(3 hunks)xrpl/transaction/integration/batch_test.go(1 hunks)xrpl/transaction/integration/credential/credential_create_test.go(1 hunks)xrpl/transaction/integration/delegate_set_test.go(6 hunks)xrpl/transaction/integration/nft/nftoken_mint_test.go(1 hunks)xrpl/transaction/integration/payment_test.go(2 hunks)xrpl/wallet/batch.go(5 hunks)xrpl/wallet/batch_test.go(9 hunks)xrpl/wallet/types/batch_signable.go(2 hunks)xrpl/wallet/wallet.go(1 hunks)xrpl/websocket/client.go(2 hunks)xrpl/websocket/client_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 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.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
Learnt from: banasa44
PR: Peersyst/xrpl-go#145
File: binary-codec/codec.go:0-0
Timestamp: 2025-07-07T11:40:34.738Z
Learning: XRPL batch transactions have a maximum limit of 8 transactions per batch, as defined by the XRPL protocol. This constraint eliminates practical concerns about integer overflow when converting batch length to uint32.
Learnt from: vriveraPeersyst
PR: Peersyst/xrpl-snap#129
File: packages/site/src/domain/transaction/controller/TransactionController.ts:98-111
Timestamp: 2025-05-17T14:29:55.132Z
Learning: In the TransactionController class of the XRPL Snap project, the `...rest` parameter in the `sendIOUTransaction` method does not contain flags from callers, so directly setting `flags: PARTIAL_PAYMENT_FLAG` without merging with existing flags is appropriate.
xrpl/transaction/integration/credential/credential_create_test.go (3)
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
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.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
xrpl/transaction/integration/payment_test.go (4)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
Learnt from: vriveraPeersyst
PR: Peersyst/xrpl-snap#129
File: packages/site/src/domain/transaction/controller/TransactionController.ts:98-111
Timestamp: 2025-05-17T14:29:55.132Z
Learning: In the TransactionController class of the XRPL Snap project, the `...rest` parameter in the `sendIOUTransaction` method does not contain flags from callers, so directly setting `flags: PARTIAL_PAYMENT_FLAG` without merging with existing flags is appropriate.
xrpl/testutil/integration/faucet.go (2)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
xrpl/wallet/wallet.go (4)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
Learnt from: GuillemGarciaDev
PR: Peersyst/xrpl-go#131
File: xrpl/rpc/client.go:265-265
Timestamp: 2025-03-31T17:29:49.615Z
Learning: In the XRPL Go codebase, type assertions from maps should always use the two-return pattern (value, ok := map[key].(type)) with proper error handling to avoid potential panics.
xrpl/transaction/integration/nft/nftoken_mint_test.go (4)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
Learnt from: vriveraPeersyst
PR: Peersyst/xrpl-snap#129
File: packages/site/src/domain/transaction/controller/TransactionController.ts:98-111
Timestamp: 2025-05-17T14:29:55.132Z
Learning: In the TransactionController class of the XRPL Snap project, the `...rest` parameter in the `sendIOUTransaction` method does not contain flags from callers, so directly setting `flags: PARTIAL_PAYMENT_FLAG` without merging with existing flags is appropriate.
xrpl/transaction/integration/delegate_set_test.go (4)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
Learnt from: vriveraPeersyst
PR: Peersyst/xrpl-snap#129
File: packages/site/src/domain/transaction/controller/TransactionController.ts:98-111
Timestamp: 2025-05-17T14:29:55.132Z
Learning: In the TransactionController class of the XRPL Snap project, the `...rest` parameter in the `sendIOUTransaction` method does not contain flags from callers, so directly setting `flags: PARTIAL_PAYMENT_FLAG` without merging with existing flags is appropriate.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
xrpl/websocket/client_test.go (5)
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.
Learnt from: GuillemGarciaDev
PR: Peersyst/xrpl-go#131
File: xrpl/rpc/client.go:265-265
Timestamp: 2025-03-31T17:29:49.615Z
Learning: In the XRPL Go codebase, type assertions from maps should always use the two-return pattern (value, ok := map[key].(type)) with proper error handling to avoid potential panics.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
Learnt from: banasa44
PR: Peersyst/xrpl-go#145
File: binary-codec/codec.go:0-0
Timestamp: 2025-07-07T11:40:34.738Z
Learning: XRPL batch transactions have a maximum limit of 8 transactions per batch, as defined by the XRPL protocol. This constraint eliminates practical concerns about integer overflow when converting batch length to uint32.
Learnt from: AgustinMJ
PR: Peersyst/near-mobile#58
File: packages/shared/blockchain/core/src/signers/signer.ts:26-27
Timestamp: 2025-02-27T10:24:14.276Z
Learning: The Signer interface uses `Record<string, any>` for the transaction parameter in `signTransaction` method to allow implementing classes to add their own specific types extending this base type, providing flexibility for different blockchain implementations.
xrpl/wallet/batch_test.go (1)
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.
xrpl/testutil/integration/runner.go (4)
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.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
Learnt from: vriveraPeersyst
PR: Peersyst/xrpl-snap#129
File: packages/site/src/domain/transaction/controller/TransactionController.ts:98-111
Timestamp: 2025-05-17T14:29:55.132Z
Learning: In the TransactionController class of the XRPL Snap project, the `...rest` parameter in the `sendIOUTransaction` method does not contain flags from callers, so directly setting `flags: PARTIAL_PAYMENT_FLAG` without merging with existing flags is appropriate.
xrpl/wallet/types/batch_signable.go (2)
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
xrpl/websocket/client.go (4)
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.
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 the PR #113 for xrpl-go, the initial flagging of duplicated fields (`CredentialType` and `Expiration`) in the `xrpl/transaction/credential_create.go` file was determined to be irrelevant and not an actual issue.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
Learnt from: GuillemGarciaDev
PR: Peersyst/xrpl-go#131
File: xrpl/rpc/client.go:265-265
Timestamp: 2025-03-31T17:29:49.615Z
Learning: In the XRPL Go codebase, type assertions from maps should always use the two-return pattern (value, ok := map[key].(type)) with proper error handling to avoid potential panics.
xrpl/wallet/batch.go (1)
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.
xrpl/transaction/integration/batch_test.go (3)
Learnt from: banasa44
PR: Peersyst/xrpl-go#145
File: binary-codec/codec.go:0-0
Timestamp: 2025-07-07T11:40:34.738Z
Learning: XRPL batch transactions have a maximum limit of 8 transactions per batch, as defined by the XRPL protocol. This constraint eliminates practical concerns about integer overflow when converting batch length to uint32.
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.
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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.
xrpl/rpc/helpers.go (4)
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.
Learnt from: GuillemGarciaDev
PR: Peersyst/xrpl-go#131
File: xrpl/rpc/client.go:265-265
Timestamp: 2025-03-31T17:29:49.615Z
Learning: In the XRPL Go codebase, type assertions from maps should always use the two-return pattern (value, ok := map[key].(type)) with proper error handling to avoid potential panics.
Learnt from: banasa44
PR: Peersyst/xrpl-go#145
File: binary-codec/codec.go:0-0
Timestamp: 2025-07-07T11:40:34.738Z
Learning: XRPL batch transactions have a maximum limit of 8 transactions per batch, as defined by the XRPL protocol. This constraint eliminates practical concerns about integer overflow when converting batch length to uint32.
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: 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 (4)
xrpl/wallet/wallet.go (1)
binary-codec/codec.go (1)
EncodeForSigning(94-103)
xrpl/testutil/integration/runner.go (1)
xrpl/wallet/wallet.go (1)
Wallet(28-33)
xrpl/websocket/client.go (1)
xrpl/transaction/tx_type.go (3)
AccountDeleteTx(8-8)PaymentTx(48-48)BatchTx(16-16)
xrpl/wallet/batch.go (3)
xrpl/wallet/wallet.go (1)
Wallet(28-33)xrpl/wallet/types/batch_signable.go (2)
ErrRawTransactionsFieldIsNotAnArray(18-18)FromFlatBatchTransaction(32-61)keypairs/keypairs.go (1)
Sign(88-94)
🔇 Additional comments (35)
xrpl/transaction/integration/nft/nftoken_mint_test.go (1)
52-52: LGTM! Signature update handled correctly.The addition of
nilas the fourth parameter correctly adapts to the updatedTestTransactionsignature while maintaining the existing test behavior.xrpl/transaction/integration/credential/credential_create_test.go (1)
121-121: LGTM! Signature update handled correctly.The addition of
nilas the fourth parameter correctly adapts to the updatedTestTransactionsignature while maintaining the existing test behavior.xrpl/testutil/integration/faucet.go (1)
60-60: LGTM! Signature update handled correctly.The addition of
nilas the fourth parameter correctly adapts to the updatedTestTransactionsignature while maintaining the existing wallet funding behavior.xrpl/transaction/integration/payment_test.go (1)
61-61: LGTM! Signature updates handled correctly.The addition of
nilas the fourth parameter in both test functions correctly adapts to the updatedTestTransactionsignature while maintaining the existing test behavior for both websocket and RPC client variants.Also applies to: 115-115
xrpl/wallet/wallet.go (1)
143-149: Excellent defensive programming practice!Creating a shallow copy of the transaction map before passing it to
EncodeForSigningprevents mutation of the original transaction. This is important sinceEncodeForSigning(as shown inbinary-codec/codec.go) callsremoveNonSigningFieldswhich likely modifies the input map. The copy ensures transaction immutability during the signing process.xrpl/websocket/client_test.go (2)
699-726: LGTM: Type safety improvements in batch transaction test dataThe changes from
[]interface{}to[]map[string]anyforRawTransactionsimprove type safety and make the test data structure more explicit. This aligns with the codebase's approach to using specific types rather than generic interfaces.
773-800: LGTM: Consistent type assertions in multisign batch testThe type updates are consistent with the previous batch transaction test case, maintaining uniformity in test data structures.
xrpl/transaction/integration/delegate_set_test.go (6)
136-136: LGTM: Method signature update handled correctlyThe addition of
nilas the fourth parameter toTestTransactioncorrectly adapts to the new method signature. This indicates no special test options are needed for this transaction.
234-234: LGTM: Consistent signature updateThe method signature update is consistently applied across all test functions.
282-282: LGTM: Consistent signature update in usage testsThe signature update maintains consistency across all
TestTransactioncalls in the delegate set usage tests.
296-296: LGTM: Final signature update maintains consistencyAll
TestTransactioncalls now use the updated four-parameter signature consistently.
339-339: LGTM: RPC client tests updated consistentlyThe RPC client integration tests also maintain the same signature consistency.
353-353: LGTM: Complete signature consistency across all testsAll integration tests now use the updated method signature consistently.
xrpl/rpc/helpers.go (5)
606-609: LGTM: Improved type safety with direct assertionThe direct assertion of
RawTransactionsas[]map[string]anyeliminates intermediate type conversions and makes the expected data structure more explicit. This improves type safety and aligns with the codebase's preference for specific type assertions.
612-617: LGTM: Simplified iteration with better error handlingThe direct iteration over the typed slice is cleaner and the error message is more descriptive. The type assertion for
RawTransactionis also more explicit.
620-624: LGTM: Cleaner fee calculation logicThe direct conversion to
FlatTransactionand immediate fee calculation is more straightforward than the previous approach.
627-632: LGTM: Better error handling and fee resetThe fee extraction with proper error handling and the reset to "0" for inner transactions is correct for batch transaction processing.
635-638: LGTM: Improved error message formattingThe error message now includes the actual fee value that failed to parse, which will be helpful for debugging.
xrpl/wallet/types/batch_signable.go (6)
12-21: LGTM: Well-defined error variablesThe new error variables provide clear, descriptive error messages for different validation failures in batch signable creation.
30-41: LGTM: Proper type assertion with validationThe function signature is clear and the type assertion for
Flagsuses the two-return pattern with proper error handling, following the established pattern in the XRPL Go codebase.
38-46: LGTM: Structured validation approachThe validation of
RawTransactionsas[]map[string]anyis correct and the initialization of theBatchSignablewith pre-allocated slice is efficient.
48-58: LGTM: Comprehensive inner transaction validationThe inner loop properly validates each raw transaction structure and generates transaction IDs using the hash function. The error wrapping provides good context for debugging.
66-71: LGTM: Refactored for cleaner iterationThe refactored
FromBatchTransactionis cleaner with direct assignment ofrawTxsand pre-allocated slice for better performance.
63-63: Fix: Duplicate function commentThe comment on line 63 is identical to the one on line 30-31 and appears to be a duplicate.
-// FromFlatBatchTransaction creates a BatchSignable from a Batch transaction. +// FromBatchTransaction creates a BatchSignable from a Batch transaction.⛔ Skipped due to 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: The CredentialCreate struct in xrpl/transaction/credential_create.go does not have any duplicated fields as was incorrectly flagged in a previous review comment.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.xrpl/wallet/batch_test.go (9)
15-71: LGTM: Comprehensive helper function for batch signer parsingThe
parseBatchSignersFromFlatfunction correctly handles the complex nested structure of batch signers, including proper parsing of multisign signers. The error handling is appropriate with graceful handling of missing fields.
240-250: LGTM: Proper adaptation to flattened transaction signingThe test correctly adapts to the new signing approach by flattening the transaction before signing and parsing the batch signers back to the original structure. The
SetAllOrNothingFlag()call is appropriate for batch transactions.
352-362: LGTM: Consistent test pattern implementationThe SECP256K1 test follows the same pattern as the ED25519 test, maintaining consistency in the test suite.
470-487: LGTM: Complex test case handlingThe combine batch signers test correctly handles the more complex scenario with multiple transactions, maintaining the same flattened transaction approach.
507-524: LGTM: Test case maintains verification logicThe sorting test case is properly adapted while maintaining the original verification logic for signer ordering.
567-592: LGTM: Comprehensive test coverage maintainedThe submitter removal test case properly handles the complex scenario with multiple wallets and maintains comprehensive verification of the batch signing behavior.
625-635: LGTM: Error case testing preservedThe error case testing is properly maintained with the new flattened transaction approach, ensuring edge cases are still covered.
651-668: LGTM: Complex error scenario testingThe signed inner transaction error case is properly handled, demonstrating that the test suite maintains comprehensive error coverage.
689-706: LGTM: Flag validation testing preservedThe different flags error testing is correctly adapted to the new approach while maintaining the original validation logic.
xrpl/testutil/integration/runner.go (1)
21-24: Well-structured test options implementation.The addition of
TestTransactionOptionsprovides clean control over transaction processing behavior in tests, with proper nil-safety in the conditional check.xrpl/websocket/client.go (1)
861-887: Cleaner batch fee calculation implementation.The simplified type assertions and direct handling of RawTransactions as
[]map[string]anyimprove code clarity while maintaining the same functionality.
1 participant
[TA-5091]: batch transaction integration tests
Description
This PR aims to add integration tests for
Batchtransactions.Type of change
Checklist:
Changes
batch_testintegration tests for websocketNotes (optional)
Describe any additional notes here.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests