fix(wallet): use minimum value promise in VN reg output - #7460
Conversation
WalkthroughAdds a new minimum_value_promise: MicroMinotari field to pay-to-self and aggregate-UTXO flows; updates request enums, public handle/service method signatures, and propagates the value through service internals and call sites, defaulting to MicroMinotari::zero() where callers do not supply it. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TS as TransactionService
participant OMH as OutputManagerHandle
participant OMS as OutputManagerService
participant Builder as Output/Tx Builder
TS->>OMH: create_pay_to_self_transaction(..., payment_id, minimum_value_promise)
OMH->>OMS: create_pay_to_self_transaction(..., payment_id, minimum_value_promise)
OMS->>OMS: select inputs / encumber aggregate utxo (passes minimum_value_promise)
OMS->>Builder: output_to_self(output_features, amount, covenant, payment_id, fee, minimum_value_promise)
Builder-->>OMS: (DbWalletOutput, KeyId) [metadata includes minimum_value_promise]
OMS->>OMS: assemble Transaction
OMS-->>OMH: (fee, Transaction)
OMH-->>TS: (fee, Transaction)
note over OMS,Builder: minimum_value_promise is carried into output metadata and construction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (5)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
f50cd5a to
05a93a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
base_layer/wallet/src/output_manager_service/handle.rs (1)
95-104: CreatePayToSelfTransaction now includes minimum_value_promise — good propagationField added in the request enum aligns with service changes and PR intent.
You may want to include this value in Display logging for easier traceability.
- CreatePayToSelfTransaction { .. } => write!(f, "CreatePayToSelfTransaction",), + CreatePayToSelfTransaction { .. } => write!(f, "CreatePayToSelfTransaction"),base_layer/wallet/src/transaction_service/service.rs (1)
2091-2104: VN registration sets minimum_value_promise = amount — matches base node requirementThis should satisfy “promise ≥ deposit” while remaining ≤ actual output amount.
Consider a short comment clarifying why amount is used here.
- .create_pay_to_self_transaction( + .create_pay_to_self_transaction( tx_id, amount, selection_criteria, output_features, fee_per_gram, None, payment_id.clone(), - amount, + amount, // minimum_value_promise set to deposit amount for BN validation )base_layer/wallet/src/output_manager_service/service.rs (3)
366-387: Request handler threads minimum_value_promise — goodEnd-to-end propagation is correct.
Add a sanity check before forwarding (log or early error) to catch accidental promise > amount at call-time.
1690-1700: Pay-to-self uses caller-supplied promise — consider guarding invariantFunctional pass-through is correct. A defensive check that promise ≤ amount would prevent malformed outputs.
pub async fn create_pay_to_self_transaction( &mut self, @@ - minimum_value_promise: MicroMinotari, + minimum_value_promise: MicroMinotari, ) -> Result<(MicroMinotari, Transaction), OutputManagerError> { + if minimum_value_promise > amount { + return Err(OutputManagerError::InvalidArgument( + "minimum_value_promise cannot exceed output amount".to_string(), + )); + }
2401-2410: output_to_self now accepts minimum_value_promise — bind it with a guardGood inclusion and use in metadata preimage. Add a small invariant check here too since this is the central constructor.
async fn output_to_self( @@ - minimum_value_promise: MicroMinotari, + minimum_value_promise: MicroMinotari, ) -> Result<(DbWalletOutput, TariKeyId), OutputManagerError> { + if minimum_value_promise > amount { + return Err(OutputManagerError::InvalidArgument( + "minimum_value_promise cannot exceed output amount".to_string(), + )); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
base_layer/wallet/src/output_manager_service/handle.rs(3 hunks)base_layer/wallet/src/output_manager_service/service.rs(8 hunks)base_layer/wallet/src/transaction_service/service.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
base_layer/wallet/src/output_manager_service/handle.rs (1)
base_layer/transaction_components/src/transaction_components/covenants/fields.rs (1)
minimum_value_promise(262-264)
base_layer/wallet/src/output_manager_service/service.rs (1)
base_layer/transaction_components/src/transaction_components/covenants/fields.rs (1)
minimum_value_promise(262-264)
⏰ 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). (9)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ledger build tests
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ci
- GitHub Check: wasm build tests
- GitHub Check: cargo check with stable
🔇 Additional comments (7)
base_layer/wallet/src/output_manager_service/handle.rs (2)
924-933: Pass-through includes minimum_value_promise — LGTMCorrectly forwards the new parameter to the service.
910-921: Allcreate_pay_to_self_transactioninvocations updated Verified that every call site now passes the new eight parameters; no 7-argument calls remain.base_layer/wallet/src/transaction_service/service.rs (3)
2176-2189: Validator exit uses zero promise — consistent if no deposit check appliesLooks fine as long as BN doesn’t enforce a minimum for exits.
Please confirm that exit outputs have no minimum-value requirement in consensus.
2251-2264: Eviction proof uses zero promise — consistent with intentNo issues spotted.
2366-2379: Code template registration uses zero promise — appropriate for zero-amount outputThis keeps behavior unchanged for template registration.
base_layer/wallet/src/output_manager_service/service.rs (2)
2171-2175: Coin-split-even sets promise to zero — OKExplicit zero keeps prior semantics.
2332-2336: Coin-split sets promise to zero — OKConsistent with existing flows.
3 participants
Description
fix(wallet): use minimum value promise in VN reg output
Motivation and Context
Base node validation for VN reg requires the minimum value promise to be >= deposit amount
Ref tari-project/tari-ootle#1560
How Has This Been Tested?
In PR tari-project/tari-ootle#1560
Summary by CodeRabbit