feat: replace by fee and user pay for fee commands#7284
feat: replace by fee and user pay for fee commands#7284SWvheerden merged 31 commits intotari-project:developmentfrom
Conversation
WalkthroughThis update introduces Replace-By-Fee (RBF) and UserPayForFee functionality throughout the wallet system. It adds new gRPC endpoints, CLI commands, transaction service methods, UTXO selection filters, error handling, and integration tests to support replacing pending transactions with higher fees and spending outputs from pending transactions. Supporting logic and tests are included. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/User
participant WalletGRPC
participant TransactionService
participant OutputManager
participant Database
CLI/User->>WalletGRPC: ReplaceByFee(tx_id, fee_increase)
WalletGRPC->>TransactionService: replace_by_fee(tx_id, fee_increase)
TransactionService->>Database: fetch original transaction
TransactionService->>OutputManager: select original inputs (MustInclude)
TransactionService->>TransactionService: calculate new fee per gram
TransactionService->>TransactionService: create replacement transaction
TransactionService->>Database: store new transaction
TransactionService-->>WalletGRPC: new_tx_id
WalletGRPC-->>CLI/User: new_tx_id
sequenceDiagram
participant CLI/User
participant WalletGRPC
participant TransactionService
participant OutputManager
participant Database
CLI/User->>WalletGRPC: UserPayForFee(tx_id, destination, fee)
WalletGRPC->>TransactionService: user_pay_for_fee(tx_id, destination, fee)
TransactionService->>Database: fetch original transaction
TransactionService->>OutputManager: select outputs from original tx
TransactionService->>TransactionService: calculate fee per gram
TransactionService->>TransactionService: create new transaction spending outputs
TransactionService->>Database: store new transaction
TransactionService-->>WalletGRPC: transfer result(s)
WalletGRPC-->>CLI/User: transfer result(s)
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
Test Results (Integration tests)1 tests 1 ✅ 0s ⏱️ For more details on these parsing errors, see this check. Results for commit 55ff020. ♻️ This comment has been updated with latest results. |
Test Results (CI) 3 files 135 suites 1h 12m 18s ⏱️ Results for commit 8d72882. ♻️ This comment has been updated with latest results. |
…ing stuck in the mempool
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Hi there,
Some comments to consider.
Thanks.
base_layer/wallet/src/output_manager_service/input_selection.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs
Outdated
Show resolved
Hide resolved
…ons for replace_by_fee
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (5)
applications/minotari_console_wallet/src/cli.rs (1)
223-229: Clarify the fee parameter semantics.The
new_fee_per_gramparameter name suggests this is an absolute new fee rate, but there's a previous comment indicating potential confusion between absolute fee and fee increase. Please confirm whether this parameter represents:
- The new absolute fee per gram for the replacement transaction
- An increase amount to be added to the existing fee per gram
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
2172-2176: Verify the use of one-sided address for this operationBased on past review feedback, single addresses cannot receive one-sided payments. Consider using the interactive address instead.
Consider using
get_wallet_interactive_address()instead:let wallet_address = self .wallet - .get_wallet_one_sided_address() + .get_wallet_interactive_address() .await .map_err(|e| Status::internal(format!("{:?}", e)))?;base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (3)
325-330: Simplify commitment filtering for maintenance.The conditional logic for single vs. multiple commitments adds complexity without significant performance benefit in a SQLite context.
- must_include_query = match commitments.len() { - 1 => must_include_query.filter(outputs::commitment.eq(commitments[0].to_vec())), - _ => must_include_query.filter( - outputs::commitment.eq_any::<Vec<Vec<u8>>>(commitments.iter().map(|c| c.to_vec()).collect()), - ), - }; + must_include_query = must_include_query.filter( + outputs::commitment.eq_any::<Vec<Vec<u8>>>(commitments.iter().map(|c| c.to_vec()).collect()) + );
247-276: Address duplicate code as flagged in previous reviews.This code duplicates the standard output type filtering and onesided exclusion logic that exists in the
Standardfilter case. As mentioned in previous reviews, this should be integrated to improve maintenance and reduce duplication.Extract the common filtering logic into a helper method:
+ fn apply_standard_filters( + mut query: diesel::query_builder::BoxedSelectStatement<'_, diesel::sql_types::BigInt, outputs::table, diesel::sqlite::Sqlite>, + selection_criteria: &UtxoSelectionCriteria, + ) -> diesel::query_builder::BoxedSelectStatement<'_, diesel::sql_types::BigInt, outputs::table, diesel::sqlite::Sqlite> { + query = query.filter( + outputs::output_type + .eq(i32::from(OutputType::Standard.as_byte())) + .or(outputs::output_type.eq(i32::from(OutputType::Coinbase.as_byte()))), + ); + + if selection_criteria.excluding_onesided { + query = query.filter(outputs::source.ne(OutputSource::OneSided as i32)); + } + + query + }Then use this helper in both the
StandardandMustIncludecases.
307-360: Integrate MustInclude handling into main query structure.As suggested in previous reviews, this special handling code should be integrated into the main match statements above to improve maintenance and reduce code duplication. The current approach creates a separate code path that duplicates query construction logic.
Consider restructuring the method to handle
MustIncludeas part of the main query flow rather than as a separate special case. This would involve:
- Modifying the main query construction to handle must-include outputs
- Using a single query execution path with conditional logic
- Avoiding the separate query construction for must-include outputs
This would align with the maintenance concerns raised in previous reviews and reduce the overall complexity of the method.
🧹 Nitpick comments (7)
applications/minotari_console_wallet/src/cli.rs (1)
231-237: Clarify the UserPayForFee command purpose.The
UserPayForFeecommand structure includes adestinationandamountfield, but it's unclear from the CLI definition alone what this command is intended to do. Based on the PR objectives, this seems to create a new transaction that sends outputs to a specified destination, but the relationship betweenamountand the original transaction's outputs needs clarification.Consider adding documentation comments to clarify:
- What happens to the original transaction's outputs
- How the
amountparameter relates to the original transaction- Whether this creates a completely new transaction or modifies the existing one
+/// Creates a new transaction that sends the outputs of an unmined transaction to a specified destination. +/// This effectively allows a user to "pay for" someone else's transaction fees. #[derive(Debug, Args, Clone)] pub struct UserPayForFeeArgs { #[clap(short, long)] pub tx_id: u64, + /// Destination address to send the transaction outputs to pub destination: TariAddress, + /// Amount to send (must match or be less than the original transaction's output amount) pub amount: MicroMinotari, }applications/minotari_app_grpc/proto/wallet.proto (1)
1708-1715: Clarify the ignored fields documentation.The comment about ignoring
amountandfee_per_gramin thePaymentRecipientis helpful, but it would be clearer to specify which fields are actually used.message TxOutputsToSpendTransfer { // Id of the transaction to spend outputs from. uint64 tx_id = 1; // Fee to pay for the transaction in microTari. uint64 fee = 2; - // Amount and fee_per_gram are ignored. These values will be calculated automatically. + // Only the address field from PaymentRecipient is used. Amount and fee_per_gram are ignored and calculated automatically. PaymentRecipient recipient = 3; }base_layer/wallet/src/transaction_service/handle.rs (1)
560-578: Add documentation for consistencyThe implementation is correct and follows the established pattern, but lacks documentation unlike the
replace_by_feemethod above.Add documentation similar to the
replace_by_feemethod:+ /// Use the outputs of a pending outbound transaction to create a new transaction + /// that sends those outputs to the specified destination address + /// + /// # Arguments + /// * `tx_id` - The transaction ID of the pending outbound transaction + /// * `destination` - The destination address to send the outputs to + /// * `fee` - The fee for the new transaction + /// + /// # Returns + /// The new transaction ID or an error pub async fn user_pay_for_fee(applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
2177-2195: Consider the timeout value consistencyThe timeout is set to 1000ms here, while the similar
transfermethod uses 100ms. Consider aligning these values or documenting why they differ.If the longer timeout is intentional due to the nature of fee replacement transactions, consider adding a comment explaining the rationale.
2156-2161: Use more idiomatic cloning approachSince these values implement Clone/Copy, using
to_owned()is unnecessary. Consider using*for Copy types or.clone()for consistency.self.get_transaction_service() .user_pay_for_fee( - TxId::from(tx_id.to_owned()), - address.to_owned(), - MicroMinotari::from(fee.to_owned()), + TxId::from(*tx_id), + address.clone(), + MicroMinotari::from(*fee), )integration_tests/tests/steps/wallet_steps.rs (1)
3240-3289: Consider removing or documenting the hardcoded sleepThe function implementation looks good with proper error handling. However, there's a hardcoded 1-second sleep at line 3270 that doesn't seem necessary. If it's required for some timing reason, please add a comment explaining why. Otherwise, consider removing it to speed up test execution.
let response = client.user_pay_for_fee(user_pay_for_fee_req).await; - tokio::time::sleep(Duration::from_millis(1000)).await;base_layer/wallet/src/transaction_service/service.rs (1)
4290-4382: Fee calculation methodology is comprehensive but complex.The
calculate_fee_per_gram_from_total_feemethod correctly:
- Analyzes actual output sizes from the original transaction
- Calculates transaction weight using the consensus fee calculator
- Handles edge cases like empty outputs
- Uses floating point division to avoid truncation
However, consider extracting some of the size calculation logic into smaller helper methods for better readability.
// Consider extracting this into a helper method +fn calculate_average_output_size(&self, outputs: &[TransactionOutput]) -> Result<usize, TransactionServiceError> { + if outputs.is_empty() { + return Ok(self.get_default_output_size()?); + } + + let total_size: Result<usize, std::io::Error> = outputs + .iter() + .map(|output| output.get_features_and_scripts_size()) + .sum(); + + Ok(total_size.map_err(|e| + TransactionServiceError::InvalidMessageError(format!("Failed to calculate size: {}", e)) + )? / outputs.len()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/cli.rs(3 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(5 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(2 hunks)base_layer/common_types/src/transaction.rs(1 hunks)base_layer/wallet/src/output_manager_service/input_selection.rs(3 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs(3 hunks)base_layer/wallet/src/transaction_service/error.rs(1 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs(4 hunks)base_layer/wallet/src/transaction_service/service.rs(6 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(1 hunks)base_layer/wallet/tests/output_manager_service_tests/storage.rs(2 hunks)integration_tests/tests/features/WalletTransactions.feature(3 hunks)integration_tests/tests/steps/wallet_steps.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
base_layer/common_types/src/transaction.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
applications/minotari_console_wallet/src/wallet_modes.rs (2)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
base_layer/wallet/src/transaction_service/error.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/wallet/src/output_manager_service/input_selection.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
applications/minotari_console_wallet/src/automation/commands.rs (1)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
applications/minotari_console_wallet/src/cli.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/wallet/tests/output_manager_service_tests/storage.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
base_layer/wallet/src/transaction_service/handle.rs (2)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
applications/minotari_app_grpc/proto/wallet.proto (2)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
integration_tests/tests/steps/wallet_steps.rs (5)
Learnt from: hansieodendaal
PR: tari-project/tari#7189
File: integration_tests/tests/steps/wallet_cli_steps.rs:464-467
Timestamp: 2025-06-07T06:49:32.174Z
Learning: In integration test code, using panic! for setup validation and prerequisite checking is acceptable and often preferred, as it provides immediate feedback when test scenarios are incorrectly configured rather than allowing tests to continue in invalid states.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
base_layer/wallet/src/transaction_service/service.rs (2)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>
🧬 Code Graph Analysis (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)
base_layer/wallet/src/transaction_service/handle.rs (4)
user_pay_for_fee(1560-1578)from(715-719)new(744-752)replace_by_fee(1545-1558)applications/minotari_console_wallet/src/cli.rs (2)
from_str(267-302)from(480-482)base_layer/common_types/src/tari_address/mod.rs (1)
from_str(409-419)applications/minotari_app_grpc/src/conversions/transaction.rs (1)
not_found(114-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (31)
base_layer/common_types/src/transaction.rs (1)
76-81: Well-implemented transaction status utility method.The
is_minedmethod correctly identifies mined transactions by checking for bothMinedUnconfirmedandMinedConfirmedstatuses. The implementation follows the established pattern used by other similar methods in this impl block and will be useful for the new replace-by-fee and user-pay-for-fee features.applications/minotari_console_wallet/src/wallet_modes.rs (2)
470-470: Good housekeeping: whitespace cleanup.The removal of trailing whitespace improves code cleanliness.
562-563: Correct addition of new command variants to the test.The new
ReplaceByFeeandUserPayForFeecommand variants are properly added to the exhaustive match statement, ensuring the test continues to compile and function correctly with the new CLI commands.base_layer/wallet/src/transaction_service/error.rs (1)
211-216: Well-structured error variants for RBF functionality.The three new error variants are appropriately named and provide clear error messages for the Replace-By-Fee and UserPayForFee operations. The error messages follow the existing pattern and will help with debugging transaction service issues.
base_layer/wallet/src/output_manager_service/input_selection.rs (3)
75-82: Well-implemented constructor for MustInclude filter.The
must_includeconstructor properly initializes the criteria with the newMustIncludefilter. Settingmin_dust: 0is appropriate for this use case since the specified UTXOs must be included regardless of their dust value.
123-125: Appropriate new filter variant for RBF functionality.The
MustIncludefilter variant is well-documented and provides the necessary flexibility for Replace-By-Fee operations where specific UTXOs must be included in the selection.
142-144: Clear Display implementation for the new filter.The Display implementation correctly shows the count of commitments in the
MustIncludefilter, maintaining consistency with the existingSpecificOutputsvariant.integration_tests/tests/features/WalletTransactions.feature (2)
425-439: Comprehensive test scenario for Replace-By-Fee functionality.The test scenario properly verifies the RBF flow: sends initial transaction, replaces it with higher fee, and confirms only one unconfirmed transaction remains after mining. This provides good coverage for the new functionality.
441-455: Well-designed test for UserPayForFee functionality.The test scenario effectively validates the user pay for fee mechanism by sending a transaction, creating a fee payment transaction, and verifying that both transactions remain unconfirmed after mining. This covers the expected behavior of the new feature.
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (4)
103-103: LGTM: Struct field addition is well-integrated.The addition of
utxo_selection_criteriaas an optional field to theTransactionSendProtocolstruct correctly addresses the architectural concern raised in previous reviews about making this part of the struct rather than just a parameter.
128-128: LGTM: Constructor parameter addition is consistent.The constructor properly accepts the
utxo_selection_criteriaparameter and follows the existing pattern for optional parameters in the protocol.
143-143: LGTM: Field initialization is correct.The struct field is properly initialized in the constructor, maintaining consistency with the other fields.
226-226: LGTM: Proper usage of UTXO selection criteria.The implementation correctly uses
clone().unwrap_or_default()to handle the optional nature of the criteria, providing a safe fallback to default selection when no specific criteria are provided. This aligns with the learning that suggests using accurate estimates rather than generic defaults where possible.applications/minotari_console_wallet/src/cli.rs (2)
36-36: LGTM: Import statement follows Rust conventions.Adding
selfto the import statement is a standard practice in Rust for accessing the module's own items alongside imported types.
159-160: LGTM: CLI command additions are well-structured.Both
ReplaceByFeeandUserPayForFeecommands are properly added to the enum and follow the existing pattern for CLI commands with associated argument structs.applications/minotari_app_grpc/proto/wallet.proto (1)
1155-1157: New RPC methods look well-defined.The
ReplaceByFeeandUserPayForFeeRPC methods are properly added to the Wallet service and follow the established naming conventions.base_layer/wallet/tests/output_manager_service_tests/storage.rs (2)
35-35: Import addition looks correct.The
UtxoSelectionCriteriaimport is properly added to support the new test functionality.
382-458: Comprehensive test coverage for the must_include filter.The test function provides excellent coverage of the
UtxoSelectionCriteria::must_includefunctionality with four well-designed scenarios:
- Sufficient amount: Tests that only the must-include output is returned when it meets the requirement
- Insufficient amount: Tests that additional outputs are selected when the must-include output is insufficient
- Multiple outputs: Tests that multiple must-include outputs are correctly selected
- Empty commitments: Tests fallback to standard selection logic
The test assertions are appropriate and validate both the selection logic and total value requirements.
base_layer/wallet/src/transaction_service/handle.rs (4)
245-253: LGTM: Request enum variants are well-definedThe new enum variants follow the established patterns with appropriate parameter types and naming conventions.
476-489: LGTM: Display formatting is consistentThe display implementations follow the established pattern and provide clear, readable output for the new request variants.
569-569: LGTM: Response variant is appropriately definedThe
TransactionReplaced(TxId)response variant follows the established pattern and is correctly placed for the replace by fee functionality.
545-558: LGTM: Replace by fee implementation is correctThe method follows the established pattern with proper async handling, error management, and comprehensive documentation.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
92-93: LGTM!The imports are correctly added and necessary for the new RPC methods.
Also applies to: 113-114, 123-123
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (2)
349-358: Logic looks correct for combining outputs.The implementation correctly:
- Calculates remaining input slots after must-include outputs
- Fetches additional outputs within the limit
- Combines must-include and additional outputs
The transaction input limit enforcement and output combination logic appears sound.
344-346: By design: must-include outputs bypass the input limitThe early return in
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (lines 344–346)
occurs before applyingTRANSACTION_INPUTS_LIMIT. All other paths uselimit(TRANSACTION_INPUTS_LIMIT)when querying additional outputs, and the service layer enforces the limit when adding beyond those must-include UTXOs. This behavior is intentional—no change needed.integration_tests/tests/steps/wallet_steps.rs (1)
38-50: LGTM!The new imports for
ReplaceByFeeRequest,UserPayForFeeRequest, andTxOutputsToSpendTransfercorrectly support the new RBF and UserPayForFee functionality.base_layer/wallet/src/transaction_service/service.rs (5)
61-61: New imports look appropriate for the functionality.The added imports for
SerializedSize,Fee, andEncryptedDataare properly used in the new methods for fee calculation and output decryption.Also applies to: 67-67, 73-73
1114-1128: Request handler implementation looks correct.The new request handlers for
ReplaceByFeeandUserPayForFeeproperly delegate to the respective methods and handle the reply channel appropriately. TheUserPayForFeehandler correctly takes the reply channel for async response handling.
3967-3976: Helper method implementation is clean and focused.The
get_broadcast_transactionsmethod correctly filters transactions withBroadcaststatus. The implementation is straightforward and follows the existing pattern.
3978-3999: Input commitment extraction handles compact inputs correctly.The method properly filters out compact inputs that don't have full commitment data available. The error handling for commitment extraction is appropriate.
4196-4288: Replace-by-fee implementation addresses previous review concerns.The method correctly:
- Validates that fee_increase is not zero
- Checks if the original transaction is mined using
is_mined()- Calculates fee_per_gram from total fee and weight
- Uses
UtxoSelectionCriteria::must_include()to ensure same inputs are usedThis addresses the concerns raised in past reviews about mining status checks and fee calculations.
| UserPayForFee(args) => { | ||
| transaction_service | ||
| .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount) | ||
| .await | ||
| .unwrap(); | ||
| }, |
There was a problem hiding this comment.
Fix error handling and add user feedback.
The current implementation uses .unwrap() which will panic on errors, inconsistent with the error handling pattern used throughout this file. Other commands use proper error handling with match statements and provide user feedback.
Apply this diff to fix the error handling and add user feedback:
- UserPayForFee(args) => {
- transaction_service
- .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount)
- .await
- .unwrap();
- },
+ UserPayForFee(args) => {
+ match transaction_service
+ .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount)
+ .await
+ {
+ Ok(tx_id) => {
+ debug!(target: LOG_TARGET, "user-pay-for-fee concluded with tx_id {}", tx_id);
+ println!("Fee payment transaction created with tx_id: {}", tx_id);
+ tx_ids.push(tx_id);
+ },
+ Err(e) => eprintln!("UserPayForFee error! {}", e),
+ }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UserPayForFee(args) => { | |
| transaction_service | |
| .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount) | |
| .await | |
| .unwrap(); | |
| }, | |
| UserPayForFee(args) => { | |
| match transaction_service | |
| .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount) | |
| .await | |
| { | |
| Ok(tx_id) => { | |
| debug!(target: LOG_TARGET, "user-pay-for-fee concluded with tx_id {}", tx_id); | |
| println!("Fee payment transaction created with tx_id: {}", tx_id); | |
| tx_ids.push(tx_id); | |
| }, | |
| Err(e) => eprintln!("UserPayForFee error! {}", e), | |
| } | |
| }, |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
1384 to 1389, replace the use of .unwrap() on the async call with proper error
handling using a match statement or if let to handle the Result. On success,
proceed normally; on error, provide user feedback by printing or logging an
appropriate error message instead of panicking. This aligns with the existing
error handling pattern in the file.
There was a problem hiding this comment.
Agree, unwrap should be handled.
There was a problem hiding this comment.
@hansieodendaal Thank you for confirming! I appreciate your agreement on implementing proper error handling. The consistent pattern throughout the file of using match statements with debug logging, user feedback, and tx_id tracking will make the code more robust and maintainable.
🐰✨
✏️ Learnings added
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
| pub fn get_transaction_to_be_broadcast( | ||
| &self, | ||
| tx_id: TxId, | ||
| ) -> Result<CompletedTransaction, TransactionStorageError> { | ||
| let key = DbKey::CompletedTransaction(tx_id); | ||
| let t = match self.db.fetch(&DbKey::CompletedTransaction(tx_id)) { | ||
| Ok(None) => Err(TransactionStorageError::ValueNotFound(key)), | ||
| Ok(Some(DbValue::CompletedTransaction(pt))) => Ok(pt), | ||
| Ok(Some(other)) => unexpected_result(key, other), | ||
| Err(e) => log_error(key, e), | ||
| }?; | ||
| Ok(*t) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate method - identical to existing functionality.
The get_transaction_to_be_broadcast method is essentially identical to the existing get_completed_transaction_cancelled_or_not method (lines 549-561). Both methods perform the same operations: fetch a completed transaction by ID without any filtering logic.
Consider either:
- Removing this method and using the existing
get_completed_transaction_cancelled_or_notmethod instead - Consolidating both methods into a single, well-named method like
get_completed_transaction_by_id
- pub fn get_transaction_to_be_broadcast(
- &self,
- tx_id: TxId,
- ) -> Result<CompletedTransaction, TransactionStorageError> {
- let key = DbKey::CompletedTransaction(tx_id);
- let t = match self.db.fetch(&DbKey::CompletedTransaction(tx_id)) {
- Ok(None) => Err(TransactionStorageError::ValueNotFound(key)),
- Ok(Some(DbValue::CompletedTransaction(pt))) => Ok(pt),
- Ok(Some(other)) => unexpected_result(key, other),
- Err(e) => log_error(key, e),
- }?;
- Ok(*t)
- }If this method has a specific semantic meaning for the RBF functionality, consider adding documentation to clarify the distinction from the existing method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_transaction_to_be_broadcast( | |
| &self, | |
| tx_id: TxId, | |
| ) -> Result<CompletedTransaction, TransactionStorageError> { | |
| let key = DbKey::CompletedTransaction(tx_id); | |
| let t = match self.db.fetch(&DbKey::CompletedTransaction(tx_id)) { | |
| Ok(None) => Err(TransactionStorageError::ValueNotFound(key)), | |
| Ok(Some(DbValue::CompletedTransaction(pt))) => Ok(pt), | |
| Ok(Some(other)) => unexpected_result(key, other), | |
| Err(e) => log_error(key, e), | |
| }?; | |
| Ok(*t) | |
| } |
🤖 Prompt for AI Agents
In base_layer/wallet/src/transaction_service/storage/database.rs around lines
535 to 547, the method get_transaction_to_be_broadcast duplicates the
functionality of get_completed_transaction_cancelled_or_not (lines 549-561). To
fix this, remove get_transaction_to_be_broadcast and replace its usage with
get_completed_transaction_cancelled_or_not, or consolidate both methods into a
single method with a clear, descriptive name like
get_completed_transaction_by_id. If the method is kept for semantic clarity
related to RBF, add documentation explaining its specific purpose.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
Show resolved
Hide resolved
| #[then(expr = "wallet {word} detects only {int} transaction as unconfirmed")] | ||
| async fn wallet_detects_only_transactions_as_unconfirmed( | ||
| world: &mut TariWorld, | ||
| wallet_name: String, | ||
| expected_count: u64, | ||
| ) { | ||
| let mut client = create_wallet_client(world, wallet_name.clone()).await.unwrap(); | ||
| let mut completed_tx_res = client | ||
| .get_completed_transactions(GetCompletedTransactionsRequest { | ||
| payment_id: None, | ||
| block_hash: None, | ||
| block_height: None, | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .into_inner(); | ||
|
|
||
| let num_retries = 40; | ||
| let mut total_unconfirmed_transactions = 0u64; | ||
| let mut all_transactions_status = Vec::new(); | ||
|
|
||
| 'outer: for _ in 0..num_retries { | ||
| cucumber_steps_log(format!("{}, Detecting unconfirmed transactions", wallet_name)); | ||
| total_unconfirmed_transactions = 0; | ||
| all_transactions_status.clear(); | ||
|
|
||
| 'inner: while let Some(tx_info) = completed_tx_res.next().await { | ||
| let tx_id = tx_info.unwrap().transaction.unwrap().tx_id; | ||
| let request = GetTransactionInfoRequest { | ||
| transaction_ids: vec![tx_id], | ||
| }; | ||
| let tx_info = client.get_transaction_info(request).await.unwrap().into_inner(); | ||
| let tx_info = tx_info.transactions.first().unwrap(); | ||
| all_transactions_status.push((tx_id, tx_info.status())); | ||
| match tx_info.status() { | ||
| grpc::TransactionStatus::MinedUnconfirmed => { | ||
| total_unconfirmed_transactions += 1; | ||
| }, | ||
| _ => continue 'inner, | ||
| } | ||
| } | ||
|
|
||
| // Debug: Print all transaction statuses | ||
| cucumber_steps_log(format!( | ||
| "{}, Found {} completed transactions with statuses: {:?}", | ||
| wallet_name, | ||
| all_transactions_status.len(), | ||
| all_transactions_status | ||
| )); | ||
|
|
||
| if total_unconfirmed_transactions == expected_count { | ||
| break 'outer; | ||
| } | ||
|
|
||
| tokio::time::sleep(tokio::time::Duration::from_secs(3)).await; | ||
| completed_tx_res = client | ||
| .get_completed_transactions(GetCompletedTransactionsRequest { | ||
| payment_id: None, | ||
| block_hash: None, | ||
| block_height: None, | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .into_inner(); | ||
| } | ||
|
|
||
| assert_eq!( | ||
| total_unconfirmed_transactions, expected_count, | ||
| "Expected {} unconfirmed transactions, but found {}. All transaction statuses: {:?}", | ||
| expected_count, total_unconfirmed_transactions, all_transactions_status | ||
| ); | ||
| } |
There was a problem hiding this comment.
Function only checks for MinedUnconfirmed status, not all unconfirmed statuses
The function name wallet_detects_only_transactions_as_unconfirmed suggests it checks for all unconfirmed transactions, but it only counts MinedUnconfirmed status (lines 1377-1379). Other unconfirmed statuses like Broadcast, Pending, and OneSidedUnconfirmed are ignored.
Consider either:
- Renaming the function to be more specific (e.g.,
wallet_detects_only_transactions_as_mined_unconfirmed) - Or expanding the check to include all unconfirmed statuses
🤖 Prompt for AI Agents
In integration_tests/tests/steps/wallet_steps.rs around lines 1342 to 1413, the
function wallet_detects_only_transactions_as_unconfirmed currently counts only
transactions with the MinedUnconfirmed status as unconfirmed, ignoring other
unconfirmed statuses like Broadcast, Pending, and OneSidedUnconfirmed. To fix
this, either rename the function to reflect it only checks for MinedUnconfirmed
transactions or modify the status check logic to include all relevant
unconfirmed statuses by expanding the match condition to cover Broadcast,
Pending, OneSidedUnconfirmed, and any other applicable unconfirmed statuses.
| #[when( | ||
| expr = "I send a replace by fee of {int} uT from wallet {word} to wallet {word} at fee higher by {int} then before" | ||
| )] | ||
| async fn send_replace_by_fee_transaction( | ||
| world: &mut TariWorld, | ||
| _amount: u64, | ||
| sender: String, | ||
| _receiver: String, | ||
| fee_increase: u64, | ||
| ) { | ||
| let mut client = create_wallet_client(world, sender.clone()).await.unwrap(); | ||
| let sender_wallet_address = world.get_wallet_address(&sender).await.unwrap(); | ||
|
|
||
| let wallet_tx_ids = world.wallet_tx_ids.get(&sender_wallet_address).unwrap(); | ||
| let tx_id = *wallet_tx_ids.last().unwrap(); | ||
|
|
||
| let replace_by_fee_req = ReplaceByFeeRequest { | ||
| transaction_id: tx_id, | ||
| fee_increase, | ||
| }; | ||
|
|
||
| let replace_by_fee_res = client.replace_by_fee(replace_by_fee_req).await.unwrap().into_inner(); | ||
| let new_tx_id = replace_by_fee_res.transaction_id; | ||
|
|
||
| let wallet_tx_ids = world.wallet_tx_ids.get_mut(&sender_wallet_address).unwrap(); | ||
| wallet_tx_ids.push(new_tx_id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple issues with RBF transaction handling
-
The parameters
_amountand_receiverare unused. If they're not needed, consider removing them from the function signature. -
Missing error handling for the RBF request. The function should handle potential errors from
client.replace_by_fee(). -
No verification that the RBF transaction was broadcast successfully. Other similar functions in this file wait for broadcast status.
-
Potential panic if
wallet_tx_idsis empty when calling.last().unwrap().
Apply this diff to address these issues:
async fn send_replace_by_fee_transaction(
world: &mut TariWorld,
- _amount: u64,
sender: String,
- _receiver: String,
fee_increase: u64,
) {
let mut client = create_wallet_client(world, sender.clone()).await.unwrap();
let sender_wallet_address = world.get_wallet_address(&sender).await.unwrap();
- let wallet_tx_ids = world.wallet_tx_ids.get(&sender_wallet_address).unwrap();
- let tx_id = *wallet_tx_ids.last().unwrap();
+ let wallet_tx_ids = world.wallet_tx_ids.get(&sender_wallet_address)
+ .expect("Sender wallet should have transaction history");
+ let tx_id = *wallet_tx_ids.last()
+ .expect("Sender wallet should have at least one transaction to replace");
let replace_by_fee_req = ReplaceByFeeRequest {
transaction_id: tx_id,
fee_increase,
};
- let replace_by_fee_res = client.replace_by_fee(replace_by_fee_req).await.unwrap().into_inner();
+ let replace_by_fee_res = client.replace_by_fee(replace_by_fee_req)
+ .await
+ .expect("Replace by fee request should succeed")
+ .into_inner();
let new_tx_id = replace_by_fee_res.transaction_id;
+ // Wait for the new transaction to be broadcast
+ let num_retries = 100;
+ let tx_info_req = GetTransactionInfoRequest {
+ transaction_ids: vec![new_tx_id],
+ };
+
+ for i in 0..num_retries {
+ let tx_info_res = client
+ .get_transaction_info(tx_info_req.clone())
+ .await
+ .unwrap()
+ .into_inner();
+ let tx_info = tx_info_res.transactions.first().unwrap();
+
+ // TransactionStatus::TRANSACTION_STATUS_BROADCAST == 1_i32
+ if tx_info.status == 1_i32 {
+ cucumber_steps_log(format!(
+ "RBF transaction {} has been broadcast successfully",
+ new_tx_id
+ ));
+ break;
+ }
+
+ if i == num_retries - 1 {
+ panic!("RBF transaction {} failed to be broadcast", new_tx_id);
+ }
+
+ tokio::time::sleep(Duration::from_secs(2)).await;
+ }
+
let wallet_tx_ids = world.wallet_tx_ids.get_mut(&sender_wallet_address).unwrap();
wallet_tx_ids.push(new_tx_id);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In integration_tests/tests/steps/wallet_steps.rs around lines 3212 to 3238,
remove the unused parameters _amount and _receiver from the function signature.
Add proper error handling for the client.replace_by_fee() call to handle
potential failures gracefully. Include a check or wait to verify that the RBF
transaction was broadcast successfully, similar to other functions in this file.
Before accessing the last element of wallet_tx_ids, add a check to ensure the
list is not empty to avoid panics.
| #[allow(clippy::too_many_lines)] | ||
| async fn user_pay_for_fee( | ||
| &mut self, | ||
| tx_id: TxId, | ||
| destination: TariAddress, | ||
| fee: MicroMinotari, | ||
| send_transaction_join_handles: &mut FuturesUnordered< | ||
| JoinHandle<Result<TransactionSendResult, TransactionServiceProtocolError<TxId>>>, | ||
| >, | ||
| reply_channel: oneshot::Sender<Result<TransactionServiceResponse, TransactionServiceError>>, | ||
| ) -> Result<TxId, TransactionServiceError> { | ||
| let original_transaction = self.resources.db.get_transaction_to_be_broadcast(tx_id).map_err(|_| { | ||
| TransactionServiceError::TransactionStorageError(TransactionStorageError::ValueNotFound( | ||
| DbKey::CompletedTransaction(tx_id), | ||
| )) | ||
| })?; | ||
| if original_transaction.status.is_mined() { | ||
| return Err(TransactionServiceError::TransactionAlreadyMined(tx_id.to_string())); | ||
| } | ||
|
|
||
| // Filter outputs based on transaction direction | ||
| let filtered_output_hashes: Vec<FixedHash> = match original_transaction.direction { | ||
| TransactionDirection::Inbound => { | ||
| // If we're receiving, include outputs that are sent to us | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Inbound transaction - filtering {} received outputs", | ||
| original_transaction.received_output_hashes.len() | ||
| ); | ||
| original_transaction.received_output_hashes.clone() | ||
| }, | ||
| TransactionDirection::Outbound => { | ||
| // If we're sending, include change outputs | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Outbound transaction - filtering {} change outputs", | ||
| original_transaction.change_output_hashes.len() | ||
| ); | ||
| original_transaction.change_output_hashes.clone() | ||
| }, | ||
| TransactionDirection::Unknown => { | ||
| // Fallback to all outputs if direction is unknown | ||
| let all_outputs: Vec<FixedHash> = original_transaction | ||
| .transaction | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .map(|output| output.hash()) | ||
| .collect(); | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Unknown transaction direction - using all {} outputs", | ||
| all_outputs.len() | ||
| ); | ||
| all_outputs | ||
| }, | ||
| }; | ||
|
|
||
| // Convert filtered hashes to CompressedCommitments by finding matching outputs | ||
| let original_outputs: Vec<CompressedCommitment> = original_transaction | ||
| .transaction | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .filter(|output| filtered_output_hashes.contains(&output.hash())) | ||
| .map(|output| output.commitment().to_owned()) | ||
| .collect(); | ||
|
|
||
| // Calculate total amount from original outputs by decrypting their encrypted data | ||
| let mut total_amount = MicroMinotari::zero(); | ||
| let view_key = match self | ||
| .resources | ||
| .transaction_key_manager_service | ||
| .get_private_view_key() | ||
| .await | ||
| { | ||
| Ok(key) => key, | ||
| Err(e) => { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Could not get private view key: {}", | ||
| e | ||
| ); | ||
| return Err(TransactionServiceError::KeyManagerServiceError(e)); | ||
| }, | ||
| }; | ||
|
|
||
| // Find the actual outputs from the transaction and decrypt their amounts | ||
| for commitment in &original_outputs { | ||
| if let Some(output) = original_transaction | ||
| .transaction | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .find(|output| output.commitment() == commitment) | ||
| { | ||
| match EncryptedData::decrypt_data(&view_key, output.commitment(), output.encrypted_data()) { | ||
| Ok((amount, _, _)) => { | ||
| total_amount += amount; | ||
| }, | ||
| Err(e) => { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Could not decrypt amount for commitment {:?}: {}", | ||
| commitment, | ||
| e | ||
| ); | ||
| }, | ||
| } | ||
| } else { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Could not find transaction output for commitment {:?}", | ||
| commitment | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let calculated_amount = total_amount; | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Filtered {} outputs from {} total outputs for transaction {}, total amount: {}", | ||
| original_outputs.len(), | ||
| original_transaction.transaction.body.outputs().len(), | ||
| tx_id, | ||
| calculated_amount | ||
| ); | ||
|
|
||
| let new_tx_id = TxId::new_random(); | ||
| let (tx_reply_sender, tx_reply_receiver) = mpsc::channel(100); | ||
| let (cancellation_sender, cancellation_receiver) = oneshot::channel(); | ||
|
|
||
| // Store the senders for the new transaction | ||
| self.pending_transaction_reply_senders | ||
| .insert(new_tx_id, tx_reply_sender); | ||
| self.send_transaction_cancellation_senders | ||
| .insert(new_tx_id, cancellation_sender); | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee Amount: {:?} target fee: {:?}", | ||
| calculated_amount, fee | ||
| ); | ||
|
|
||
| let num_inputs = original_outputs.len(); | ||
| let num_outputs = 2; // destination + change | ||
| let (weight_in_grams, fee_per_gram) = | ||
| self.calculate_fee_per_gram_from_total_fee(fee, &original_transaction, num_inputs, num_outputs)?; | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Transaction weight calculated: {} grams (inputs: {}, outputs: {})", | ||
| weight_in_grams, | ||
| num_inputs, | ||
| num_outputs | ||
| ); | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Fee calculation - target_fee: {}, weight: {} grams, fee_per_gram: {} (calculated as {:.3} rounded)", | ||
| fee, | ||
| weight_in_grams, | ||
| fee_per_gram, | ||
| if weight_in_grams > 0 { fee.0 as f64 / weight_in_grams as f64 } else { 0.0 } | ||
| ); | ||
|
|
||
| let protocol = TransactionSendProtocol::new( | ||
| new_tx_id, | ||
| self.resources.clone(), | ||
| tx_reply_receiver, | ||
| cancellation_receiver, | ||
| destination.clone(), | ||
| calculated_amount, | ||
| fee_per_gram, | ||
| original_transaction.payment_id, | ||
| TransactionMetadata::default(), | ||
| Some(reply_channel), | ||
| TransactionSendProtocolStage::Initial, | ||
| None, | ||
| Some(UtxoSelectionCriteria::must_include(original_outputs)), | ||
| ); | ||
|
|
||
| let join_handle = tokio::spawn(protocol.execute()); | ||
| send_transaction_join_handles.push(join_handle); | ||
|
|
||
| info!( | ||
| target: LOG_TARGET, | ||
| "user-pay-for-fee: Created new transaction {} to spend outputs transaction with id: {}, weight: {} grams", | ||
| new_tx_id, | ||
| tx_id, | ||
| weight_in_grams, | ||
| ); | ||
|
|
||
| Ok(new_tx_id) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
User-pay-for-fee implementation is thorough but has performance implications.
The method correctly:
- Validates mining status
- Filters outputs based on transaction direction
- Decrypts all relevant outputs to calculate amounts
- Creates a new transaction with proper UTXO selection
Performance concern: Decrypting all outputs individually could be expensive for transactions with many outputs. Consider if there's a more efficient way to calculate the total amount.
// Consider batching the decryption or using a more efficient approach
-for commitment in &original_outputs {
- if let Some(output) = original_transaction
- .transaction
- .body
- .outputs()
- .iter()
- .find(|output| output.commitment() == commitment)
- {
- match EncryptedData::decrypt_data(&view_key, output.commitment(), output.encrypted_data()) {
- Ok((amount, _, _)) => {
- total_amount += amount;
- },
- Err(e) => {
- warn!(/* ... */);
- },
- }
- }
-}
+// Consider using a more efficient bulk decryption approach
+let outputs_to_decrypt: Vec<_> = original_transaction
+ .transaction
+ .body
+ .outputs()
+ .iter()
+ .filter(|output| original_outputs.contains(&output.commitment()))
+ .collect();
+
+for output in outputs_to_decrypt {
+ match EncryptedData::decrypt_data(&view_key, output.commitment(), output.encrypted_data()) {
+ Ok((amount, _, _)) => total_amount += amount,
+ Err(e) => warn!(/* ... */),
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In base_layer/wallet/src/transaction_service/service.rs between lines 4384 and
4578, the user_pay_for_fee function decrypts each output individually to
calculate the total amount, which can be costly for transactions with many
outputs. To improve performance, refactor the amount calculation to avoid
decrypting each output separately, possibly by caching decrypted amounts if
available or by using metadata or stored values that represent output amounts
without full decryption. Ensure the new approach still accurately sums the total
amount while reducing the number of decryption operations.
| Some(reply_channel), | ||
| TransactionSendProtocolStage::Initial, | ||
| None, | ||
| None, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new parameter usage in TransactionSendProtocol::new.
The addition of None parameters in these calls suggests a new optional parameter (likely for UTXO selection criteria). Ensure this parameter is properly documented and used consistently throughout the codebase.
Also applies to: 2292-2292
🏁 Script executed:
#!/bin/bash
# Check the TransactionSendProtocol::new method signature to understand the new parameter
ast-grep --pattern 'impl TransactionSendProtocol {
$$$
pub fn new($$$) -> $_
$$$
}'Length of output: 91
I’m not seeing the new signature or parameter list—let’s locate both the implementation and its call sites to confirm the added parameter:
🏁 Script executed:
#!/bin/bash
set -eo pipefail
# Find the TransactionSendProtocol implementation block
rg "impl TransactionSendProtocol" -n base_layer/wallet/src/transaction_service/service.rs
# Find the new() signature itself
rg "pub fn new" -n base_layer/wallet/src/transaction_service/service.rs
# Locate all call sites to TransactionSendProtocol::new
rg "TransactionSendProtocol::new" -n base_layer/wallet/src/transaction_service/service.rsLength of output: 110
Let’s expand the search to the entire repo to find both the definition and all call sites:
🏁 Script executed:
#!/bin/bash
set -eo pipefail
# 1. Locate the file(s) defining or mentioning TransactionSendProtocol
rg -n "TransactionSendProtocol" -C 2
# 2. Find the impl block and new() signature for TransactionSendProtocol
rg -n "impl TransactionSendProtocol" -C 3
rg -n "TransactionSendProtocol::new" -C 3
# 3. List all calls to TransactionSendProtocol::new across the repo
rg -n "TransactionSendProtocol::new" Length of output: 10346
Let’s inspect the new method’s signature and its docs in the protocol implementation:
🏁 Script executed:
#!/bin/bash
set -eo pipefail
# Show the pub fn new signature and any doc comments around it
rg -n "pub fn new" -C 10 base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rsLength of output: 1072
Let’s pull the complete new signature and the surrounding call sites for full context:
🏁 Script executed:
#!/bin/bash
set -eo pipefail
# 1. Show the full `pub fn new` signature (with docs) in the protocol impl
rg -n "pub fn new" -C 50 base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs
# 2. Show each call to TransactionSendProtocol::new with surrounding lines in service.rs
rg -n "TransactionSendProtocol::new" -C 10 base_layer/wallet/src/transaction_service/service.rsLength of output: 8263
Add documentation for the new utxo_selection_criteria parameter
I’ve confirmed that the new signature in
transaction_send_protocol.rs (lines 113–129) now ends with
sender_protocol: Option<SenderTransactionProtocol>,
utxo_selection_criteria: Option<UtxoSelectionCriteria>,and that every call site in service.rs passes the two optional parameters in the correct order—defaulting to None or supplying
Some(UtxoSelectionCriteria::must_include(...)) where needed.
• No parameter mismatches were found.
• All existing call sites are updated consistently.
Recommendations:
- Add a doc comment on the
utxo_selection_criteriafield in theTransactionSendProtocolstruct and/or on thepub fn newsignature to explain when and how to use it.
Affected files:
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs(aroundpub fn new)
🤖 Prompt for AI Agents
In
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs
around lines 113 to 129, add a doc comment to the `utxo_selection_criteria`
parameter in the `pub fn new` function signature and/or on the corresponding
field in the `TransactionSendProtocol` struct. The comment should explain the
purpose of this parameter, when it should be used, and how it influences UTXO
selection during transaction creation. This will improve code clarity and help
future maintainers understand the usage of this new optional parameter.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Hi there, looking good. Some comments to consider.
applications/minotari_console_wallet/src/automation/commands.rs
Outdated
Show resolved
Hide resolved
| UserPayForFee(args) => { | ||
| transaction_service | ||
| .user_pay_for_fee(args.tx_id.into(), args.destination, args.amount) | ||
| .await | ||
| .unwrap(); | ||
| }, |
There was a problem hiding this comment.
Agree, unwrap should be handled.
| MicroMinotari::zero() | ||
| }; | ||
|
|
||
| Ok((weight_in_grams, fee_per_gram)) |
There was a problem hiding this comment.
Consider adding a sanity check to ensure the new fee request is met, for example.
| Ok((weight_in_grams, fee_per_gram)) | |
| let mut fee_per_gram = MicroMinotari::from(fee_per_gram); | |
| loop { | |
| let calculated_fee = fee_calculator.calculate( | |
| fee_per_gram, | |
| 1, // num_kernels = 1 | |
| num_inputs, | |
| num_outputs, | |
| features_and_scripts_size, | |
| ); | |
| if calculated_fee >= total_fee { | |
| break; | |
| } | |
| fee_per_gram += MicroMinotari::from(1); | |
| } | |
| Ok((weight_in_grams, fee_per_gram)) |
There was a problem hiding this comment.
I am bit reluctant to implement this - if calculation where indeed invalid than we would get an error and be aware that there is something off with a fee calculations. But with this sanity check we would cover up internal calculation bugs.
There was a problem hiding this comment.
Added loop as final validation of whole calculation to ensure the calculated fee is higher or equal to that given as function argument.
There was a problem hiding this comment.
I think remove the loop, its unnessary complex and will cause a bit of loops.
Even if we have the perfect fee per gram here, we are not going to end up with the perfect fee on the other end, keep this best effort
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Looking good.
utACK
| let all_outputs = original_transaction | ||
| .transaction | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
what happens if you dont have all outputs in your db? Does the selection graph handle that?
If you are the recipient, it could happen that you only have the change in your db, but you have the full output here, If you are the sender, you will only have the change.
There was a problem hiding this comment.
all_outputs are later on filtered to contain only those belonging to the user, this is done by attempting to decrypt every output and those that succeeded are later on used as outputs.
If somehow this ended up with no outputs or the total amount of outputs was zero then fee calculation would return error and propagate it back to the user.
I am no sure if given case is possible, if there is transaction id in a wallet then it should contain outputs that can be decrypted by the user.
| )); | ||
| } | ||
|
|
||
| let consensus_constants = consensus_manager.consensus_constants(0); |
There was a problem hiding this comment.
pass in the correct tip height here, don't use 0
There was a problem hiding this comment.
or easier, change the function to take in a consensus contact and not the manager
There was a problem hiding this comment.
changed it as suggested.
| MicroMinotari::zero() | ||
| }; | ||
|
|
||
| Ok((weight_in_grams, fee_per_gram)) |
There was a problem hiding this comment.
I think remove the loop, its unnessary complex and will cause a bit of loops.
Even if we have the perfect fee per gram here, we are not going to end up with the perfect fee on the other end, keep this best effort
| MicroMinotari::from(1) | ||
| }; | ||
|
|
||
| loop { |
| # Then all nodes are at height 16 | ||
|
|
||
| @missing-steps | ||
| @missing-steps @missing-steps |
There was a problem hiding this comment.
| @missing-steps @missing-steps | |
| @missing-steps |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
base_layer/wallet/src/transaction_service/service.rs (2)
1364-1364: Parameter usage is correct.The addition of
Nonefor theutxo_selection_criteriaparameter is consistent with the new optional parameter inTransactionSendProtocol::new. This correctly defaults to no specific UTXO selection criteria for regular transactions.
2289-2289: Parameter usage is correct.The addition of
Nonefor theutxo_selection_criteriaparameter is consistent with the new optional parameter inTransactionSendProtocol::new. This correctly defaults to no specific UTXO selection criteria for regular transactions.
🧹 Nitpick comments (1)
base_layer/wallet/src/transaction_service/storage/models.rs (1)
668-717: Consider enhancing test data realism.The test helper creates transactions with default/dummy values for features and scripts. While functional for testing, this might not fully validate the fee calculation logic with realistic transaction sizes.
Consider adding more realistic test data:
fn create_test_completed_transaction(num_outputs: usize) -> CompletedTransaction { let mut outputs = Vec::new(); for _i in 0..num_outputs { + // Create more realistic output features for better fee calculation testing + let features = OutputFeatures { + flags: Default::default(), + maturity: 0, + recovery_byte: 0, + metadata: vec![0; 32], // More realistic metadata size + }; let output = TransactionOutput::new_current_version( - OutputFeatures::default(), + features, Default::default(), Some(RangeProof::default()), - TariScript::default(), + TariScript::new(vec![0; 64]), // More realistic script size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base_layer/wallet/src/transaction_service/service.rs(5 hunks)base_layer/wallet/src/transaction_service/storage/models.rs(3 hunks)integration_tests/tests/features/WalletTransactions.feature(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/tests/features/WalletTransactions.feature
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
base_layer/wallet/src/transaction_service/storage/models.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/wallet/src/transaction_service/service.rs (9)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of tokio::task::spawn(async move { with tokio::task::spawn_blocking().await in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
</retrieved_learning>
<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
</retrieved_learning>
<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the hard_delete_all_stale_peers method in comms/core/src/peer_manager/storage/database.rs, the SQL query intentionally uses exact equality (peers.features = ?) rather than bitwise operations (peers.features & ? != 0) when matching COMMUNICATION_NODE features. This is the intended behavior to match only peers with exactly the COMMUNICATION_NODE feature, excluding those with additional feature flags.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
</retrieved_learning>
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
🧬 Code Graph Analysis (2)
base_layer/wallet/src/transaction_service/storage/models.rs (2)
base_layer/wallet/src/transaction_service/error.rs (6)
new(328-330)from(226-228)from(232-234)from(238-240)from(314-316)from(334-336)base_layer/core/src/transactions/transaction_components/transaction_output.rs (2)
commitment(165-167)new_current_version(139-162)
base_layer/wallet/src/transaction_service/service.rs (6)
base_layer/wallet/src/transaction_service/handle.rs (3)
replace_by_fee(1545-1558)user_pay_for_fee(1569-1587)new(744-752)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
replace_by_fee(2227-2240)user_pay_for_fee(2134-2225)new(197-216)base_layer/wallet/src/output_manager_service/input_selection.rs (1)
must_include(74-80)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (2)
default(225-230)decrypt_data(122-155)base_layer/wallet/src/transaction_service/storage/models.rs (3)
new(67-90)new(112-138)new(198-242)base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (1)
new(113-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (8)
base_layer/wallet/src/transaction_service/storage/models.rs (4)
34-45: LGTM: Import changes support new functionality.The new imports are necessary for the added methods -
CompressedCommitmentfor input extraction,ConsensusConstantsandFeefor fee calculation logic.
270-345: Well-implemented fee calculation with proper validation and ceiling division.The method correctly:
- Validates all zero-value inputs that would cause division errors
- Calculates average features/scripts size from original transaction outputs
- Uses ceiling division to prevent underpayment
- Ensures minimum fee per gram of 1
- Provides comprehensive error messages
The implementation aligns with the retrieved learning about using accurate fee estimates in the Tari wallet system.
350-367: Appropriate input commitment extraction with proper error handling.The method correctly filters out inputs that don't have commitment data (e.g., compact inputs), which is the expected behavior for RBF functionality where only full commitment data is needed.
719-992: Comprehensive test coverage validates fee calculation correctness.The test suite excellently covers:
- Basic fee calculations with exact division
- Rounding up behavior for fractional results
- Edge cases with small and large amounts
- Error conditions for zero inputs
- Consistency across multiple calls
- Overpayment bounds verification
The specific test for the 134/200 user example demonstrates proper ceiling division behavior.
base_layer/wallet/src/transaction_service/service.rs (4)
71-71: Import addition looks good.The
EncryptedDataimport is necessary for the newuser_pay_for_feemethod to decrypt transaction outputs and determine spendable amounts.
1112-1125: Request handlers implemented correctly.The new request handlers for
ReplaceByFeeandUserPayForFeefollow the established pattern in the codebase. They properly call the respective methods, handle responses, and return appropriate transaction service responses.
4159-4239: Well-implemented replace-by-fee functionality.The
replace_by_feemethod is properly implemented with:
- ✅ Zero fee increase validation (line 4176-4178)
- ✅ Transaction mining status check (line 4186-4188)
- ✅ Proper fee calculation using original transaction weight (lines 4197-4206)
- ✅ Correct use of
UtxoSelectionCriteria::must_includeto force original inputs (line 4221)- ✅ One-sided transaction creation as specified in past reviews
- ✅ Appropriate logging and error handling
The method follows established patterns and addresses all the requirements from previous review feedback.
4241-4351: Well-implemented user-pay-for-fee functionality.The
user_pay_for_feemethod is properly implemented with:
- ✅ Transaction mining status validation (lines 4271-4273)
- ✅ Efficient output decryption to identify spendable outputs (lines 4290-4300)
- ✅ Proper total amount calculation from spendable outputs (lines 4292-4293)
- ✅ Correct fee per gram calculation using estimated transaction weight (lines 4314-4319)
- ✅ One-sided transaction creation with
UtxoSelectionCriteria::must_include(lines 4330-4340)- ✅ Comprehensive logging for debugging and monitoring
The method follows the simplified approach suggested in past reviews, correctly identifying spendable outputs through decryption and using them as mandatory inputs for the new transaction.
3 participants
Description
Implements : #7279
Add 2 new methods:
replace_by_feewhich takestransaction_idandfee_increaseand bids the fee of the unmined transaction.user_pay_for_feewhich also takestransaction_idbut alsodestinationas an adress. This methods takes outputs of unmined transaction and creates new transaction which will sends those outputs to given destination address.Motivation and Context
These methods will be helpful in cases where transaction has been stuck in the mempool and also gives guarantees that the new transaction won't perform double spending.
How Has This Been Tested?
Run 2 new integration tests for both methods describes in
WalletTransactions.featureby running commands:cargo +nightly test --test cucumber -- --name "Adds new transaction to spend pending transaction outputs"and
cargo +nightly test --test cucumber -- --name "Replace transaction with higher fee"What process can a PR reviewer use to test or verify this change?
Run 2 commands above for integration tests and do code review for potential oversights.
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests