Refactor/cleanup tx success method

[fdefelici]

Description

This change makes a cleanup of TransactionResult::success and TransactionResult::success_with_soft_limit, removing the 'fee' argument, while it is possible to retrieve the same data using StacksTransaction::get_tx_fee() internally.

Applicable issues

• fixes [Cleanup] Remove fee argument from TransactionResult::success method #5900

Additional info (benefits, drawbacks, caveats)

The refactoring has been kept localized to the direct client of TransactionResult::success and TransactionResult::success_with_soft_limit.

Maybe could be also cleaned the TransactionSuccess struct (instantiated by the two methods above) that as a copy of the StacksTransaction and related fee. (it could also be a non-problem considering that is now computed internally, and works like a shortcut over tx.get_txt_fee(), but potentially could be or become an unused field)

stacks-core/stackslib/src/chainstate/stacks/miner.rs

Lines 303 to 312 in 3123ccb

	/// Represents a successful transaction. This transaction should be added to the block.
	#[derive(Debug, Clone, PartialEq)]
	pub struct TransactionSuccess {
	pub tx: StacksTransaction,
	/// The fee that was charged to the user for doing this transaction.
	pub fee: u64,
	pub receipt: StacksTransactionReceipt,
	/// Whether the soft limit was reached after this transaction was processed.
	pub soft_limit_reached: bool,
	}

Furthermore, the refactor (around the fee topic) could be pushed a little further involving StacksChainState::process_transaction.

stacks-core/stackslib/src/chainstate/stacks/db/transactions.rs

Lines 1468 to 1474 in 3123ccb

	/// Process a transaction. Return the fee and the transaction receipt
	pub fn process_transaction(
	clarity_block: &mut ClarityTx,
	tx: &StacksTransaction,
	quiet: bool,
	ast_rules: ASTRules,
	) -> Result<(u64, StacksTransactionReceipt), Error> {

Basically, this method receive a StacksTransaction and return a fee as part of the result (the u64 in the tuple). This fee value is retrieved internally using StacksTransaction::get_tx_fee() (and not manipulated in any way if I got it right).

Then the fee result is used from the caller (the is the one that also pass the StacksTrasaction) to feed some TansactionResult::success and TansactionResult::success_with_soft_limit (and infact in the proposed change the fee result is marked as ignored variable), but also for doing other operation (like accumulating fee amount)

Probably, (just my speculation) the signature of StacksChainState::process_transaction has been designed this way to be match keyword friendly.

Anyhow if it's worth it, I'm opened to do further investigation on this (maybe in a separted PR?) or just leave as is if it is the correct design.

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[jcnelson]

Can you help us to understand why the change is necessary?

I have no problem approving it; just want to know what the rationale is.

EDIT: It's my understanding that TransactionResult is used to construct transaction receipts to the event observer. Is this PR necessary because the transaction receipt currently has the fee in two or more other places?

[fdefelici]

Can you help us to understand why the change is necessary?

Yes, sure.

Mainly to make the code clear, about the fact that the fee passed as argument to TransactionResult::success() is actually the same of StacksTransaction::get_tx_fee() (this evidence emerged from here #5601)

So, removing the fee argument and letting the TransactionResult::success() "constructor" method to retrieve it from the StacksTransaction should enforce this concept, and eventually prevent potential wrong usage in the future (e.g. passing a fee different from the one of the related transaction)

Does it make sense?