Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add persistence of transaction cancellation reason to wallet db #3842

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion applications/tari_app_grpc/proto/wallet.proto
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ message TransactionInfo {
bytes excess_sig = 9;
google.protobuf.Timestamp timestamp = 10;
string message = 11;
bool valid = 12;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor but maybe worth commenting this out instead to show that 12 is still reserved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that is true. Def how it should be done in the future though I don't think its a major issue at the moment as this is just used by the cucumber at this point.

}

enum TransactionDirection {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ impl wallet_server::Wallet for WalletGrpcServer {
dest_pk: txn.destination_public_key.to_vec(),
status: TransactionStatus::from(txn.status) as i32,
amount: txn.amount.into(),
is_cancelled: txn.cancelled,
is_cancelled: txn.cancelled.is_some(),
direction: TransactionDirection::from(txn.direction) as i32,
fee: txn.fee.into(),
timestamp: Some(naive_datetime_to_timestamp(txn.timestamp)),
Expand All @@ -550,7 +550,6 @@ impl wallet_server::Wallet for WalletGrpcServer {
.get_signature()
.to_vec(),
message: txn.message,
valid: txn.valid,
}),
};
match sender.send(Ok(response)).await {
Expand Down Expand Up @@ -931,7 +930,6 @@ fn convert_wallet_transaction_into_transaction_info(
excess_sig: Default::default(),
timestamp: Some(naive_datetime_to_timestamp(tx.timestamp)),
message: tx.message,
valid: true,
},
PendingOutbound(tx) => TransactionInfo {
tx_id: tx.tx_id.into(),
Expand All @@ -945,15 +943,14 @@ fn convert_wallet_transaction_into_transaction_info(
excess_sig: Default::default(),
timestamp: Some(naive_datetime_to_timestamp(tx.timestamp)),
message: tx.message,
valid: true,
},
Completed(tx) => TransactionInfo {
tx_id: tx.tx_id.into(),
source_pk: tx.source_public_key.to_vec(),
dest_pk: tx.destination_public_key.to_vec(),
status: TransactionStatus::from(tx.status) as i32,
amount: tx.amount.into(),
is_cancelled: tx.cancelled,
is_cancelled: tx.cancelled.is_some(),
direction: TransactionDirection::from(tx.direction) as i32,
fee: tx.fee.into(),
timestamp: Some(naive_datetime_to_timestamp(tx.timestamp)),
Expand All @@ -963,7 +960,6 @@ fn convert_wallet_transaction_into_transaction_info(
.map(|s| s.get_signature().to_vec())
.unwrap_or_default(),
message: tx.message,
valid: tx.valid,
},
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::collections::HashMap;
use chrono::{DateTime, Local};
use log::*;
use tari_common_types::transaction::{TransactionDirection, TransactionStatus};
use tari_wallet::transaction_service::storage::models::TxCancellationReason;
use tokio::runtime::Handle;
use tui::{
backend::Backend,
Expand Down Expand Up @@ -96,13 +97,16 @@ impl TransactionsTab {
let mut column2_items = Vec::new();
let mut column3_items = Vec::new();
for t in windowed_view.iter() {
let text_color = text_colors.get(&t.cancelled).unwrap_or(&Color::Reset).to_owned();
let text_color = text_colors
.get(&t.cancelled.is_some())
.unwrap_or(&Color::Reset)
.to_owned();
if t.direction == TransactionDirection::Outbound {
column0_items.push(ListItem::new(Span::styled(
app_state.get_alias(&t.destination_public_key),
Style::default().fg(text_color),
)));
let amount_style = if t.cancelled {
let amount_style = if t.cancelled.is_some() {
Style::default().fg(Color::Red).add_modifier(Modifier::DIM)
} else {
Style::default().fg(Color::Red)
Expand All @@ -118,7 +122,7 @@ impl TransactionsTab {
app_state.get_alias(&t.source_public_key),
Style::default().fg(text_color),
)));
let amount_style = if t.cancelled {
let amount_style = if t.cancelled.is_some() {
Style::default().fg(Color::Green).add_modifier(Modifier::DIM)
} else {
Style::default().fg(Color::Green)
Expand Down Expand Up @@ -191,14 +195,14 @@ impl TransactionsTab {
let mut column3_items = Vec::new();

for t in windowed_view.iter() {
let cancelled = t.cancelled || !t.valid;
let cancelled = t.cancelled.is_some();
let text_color = text_colors.get(&cancelled).unwrap_or(&Color::Reset).to_owned();
if t.direction == TransactionDirection::Outbound {
column0_items.push(ListItem::new(Span::styled(
app_state.get_alias(&t.destination_public_key),
Style::default().fg(text_color),
)));
let amount_style = if t.cancelled {
let amount_style = if t.cancelled.is_some() {
Style::default().fg(Color::Red).add_modifier(Modifier::DIM)
} else {
Style::default().fg(Color::Red)
Expand All @@ -214,7 +218,7 @@ impl TransactionsTab {
app_state.get_alias(&t.source_public_key),
Style::default().fg(text_color),
)));
let color = match (t.cancelled, chain_height) {
let color = match (t.cancelled.is_some(), chain_height) {
// cancelled
(true, _) => Color::DarkGray,
// not mature yet
Expand All @@ -235,14 +239,12 @@ impl TransactionsTab {
format!("{}", local_time.format("%Y-%m-%d %H:%M:%S")),
Style::default().fg(text_color),
)));
let status = if (t.cancelled || !t.valid) && t.status == TransactionStatus::Coinbase {
let status = if matches!(t.cancelled, Some(TxCancellationReason::AbandonedCoinbase)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do something akin to this:

let status_msg = if let Some(reason) = tx.cancelled {
                format!("Cancelled: {}", reason)
            } else {
                tx.status.to_string()
            };

"Abandoned".to_string()
} else if t.cancelled && t.status == TransactionStatus::Rejected {
"Rejected".to_string()
} else if t.cancelled {
} else if matches!(t.cancelled, Some(TxCancellationReason::UserCancelled)) {
"Cancelled".to_string()
} else if !t.valid {
"Invalid".to_string()
} else if t.cancelled.is_some() {
"Rejected".to_string()
} else {
t.status.to_string()
};
Expand Down Expand Up @@ -378,15 +380,12 @@ impl TransactionsTab {
Span::styled(format!("{}", tx.fee), Style::default().fg(Color::White)),
fee_details,
]);
let status_msg = if tx.cancelled && tx.status == TransactionStatus::Rejected {
"Rejected".to_string()
} else if tx.cancelled {
"Cancelled".to_string()
} else if !tx.valid {
"Invalid".to_string()
let status_msg = if let Some(reason) = tx.cancelled {
format!("Cancelled: {}", reason)
} else {
tx.status.to_string()
};

let status = Span::styled(status_msg, Style::default().fg(Color::White));
let message = Span::styled(tx.message.as_str(), Style::default().fg(Color::White));
let local_time = DateTime::<Local>::from_utc(tx.timestamp, Local::now().offset().to_owned());
Expand All @@ -396,9 +395,9 @@ impl TransactionsTab {
);
let excess = Span::styled(tx.excess_signature.as_str(), Style::default().fg(Color::White));
let confirmation_count = app_state.get_confirmations(&tx.tx_id);
let confirmations_msg = if tx.status == TransactionStatus::MinedConfirmed && !tx.cancelled {
let confirmations_msg = if tx.status == TransactionStatus::MinedConfirmed && tx.cancelled.is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a match here would read easier than a nested if.
Something like:

|match (tx.status, confirmation_count, tx.cancelled){
(TransactionStatus::MinedConfirmed, _,None) => ...
...
}

format!("{} required confirmations met", required_confirmations)
} else if tx.status == TransactionStatus::MinedUnconfirmed && !tx.cancelled {
} else if tx.status == TransactionStatus::MinedUnconfirmed && tx.cancelled.is_none() {
if let Some(count) = confirmation_count {
format!("{} of {} required confirmations met", count, required_confirmations)
} else {
Expand Down
15 changes: 8 additions & 7 deletions applications/tari_console_wallet/src/ui/state/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ use tari_wallet::{
contacts_service::storage::database::Contact,
output_manager_service::{handle::OutputManagerEventReceiver, service::Balance},
tokens::Token,
transaction_service::{handle::TransactionEventReceiver, storage::models::CompletedTransaction},
transaction_service::{
handle::TransactionEventReceiver,
storage::models::{CompletedTransaction, TxCancellationReason},
},
WalletSqlite,
};
use tokio::{
Expand Down Expand Up @@ -415,7 +418,7 @@ impl AppState {
self.cached_data
.completed_txs
.iter()
.filter(|tx| !((tx.cancelled || !tx.valid) && tx.status == TransactionStatus::Coinbase))
.filter(|tx| !matches!(tx.cancelled, Some(TxCancellationReason::AbandonedCoinbase)))
.collect()
} else {
self.cached_data.completed_txs.iter().collect()
Expand Down Expand Up @@ -697,14 +700,14 @@ impl AppStateInner {
let tx =
CompletedTransactionInfo::from_completed_transaction(tx.into(), &self.get_transaction_weight());
if let Some(index) = self.data.pending_txs.iter().position(|i| i.tx_id == tx_id) {
if tx.status == TransactionStatus::Pending && !tx.cancelled {
if tx.status == TransactionStatus::Pending && tx.cancelled.is_none() {
self.data.pending_txs[index] = tx;
self.updated = true;
return Ok(());
} else {
let _ = self.data.pending_txs.remove(index);
}
} else if tx.status == TransactionStatus::Pending && !tx.cancelled {
} else if tx.status == TransactionStatus::Pending && tx.cancelled.is_none() {
self.data.pending_txs.push(tx);
self.data.pending_txs.sort_by(|a, b| {
b.timestamp
Expand Down Expand Up @@ -971,9 +974,8 @@ pub struct CompletedTransactionInfo {
pub status: TransactionStatus,
pub message: String,
pub timestamp: NaiveDateTime,
pub cancelled: bool,
pub cancelled: Option<TxCancellationReason>,
pub direction: TransactionDirection,
pub valid: bool,
pub mined_height: Option<u64>,
pub is_coinbase: bool,
pub weight: u64,
Expand Down Expand Up @@ -1032,7 +1034,6 @@ impl CompletedTransactionInfo {
timestamp: tx.timestamp,
cancelled: tx.cancelled,
direction: tx.direction,
valid: tx.valid,
mined_height: tx.mined_height,
is_coinbase,
weight,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-- This file should undo anything in `up.sql`
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
PRAGMA foreign_keys=OFF;

ALTER TABLE completed_transactions
RENAME TO completed_transactions_old;

CREATE TABLE completed_transactions
(
tx_id BIGINT NOT NULL PRIMARY KEY,
source_public_key BLOB NOT NULL,
destination_public_key BLOB NOT NULL,
amount BIGINT NOT NULL,
fee BIGINT NOT NULL,
transaction_protocol TEXT NOT NULL,
status INTEGER NOT NULL,
message TEXT NOT NULL,
timestamp DATETIME NOT NULL,
cancelled INTEGER NULL,
direction INTEGER,
coinbase_block_height BIGINT,
send_count INTEGER default 0 NOT NULL,
last_send_timestamp DATETIME,
confirmations BIGINT default NULL,
mined_height BIGINT,
mined_in_block BLOB,
transaction_signature_nonce BLOB default 0 NOT NULL,
transaction_signature_key BLOB default 0 NOT NULL
);

INSERT INTO completed_transactions (tx_id, source_public_key, destination_public_key, amount, fee, transaction_protocol,
status, message, timestamp, cancelled, direction, coinbase_block_height, send_count,
last_send_timestamp, confirmations, mined_height, mined_in_block, transaction_signature_nonce,
transaction_signature_key)
SELECT tx_id,
source_public_key,
destination_public_key,
amount,
fee,
transaction_protocol,
status,
message,
timestamp,
CASE completed_transactions_old.valid --This flag was only ever used to signify an abandoned coinbase, we will do that in the cancelled reason enum now
WHEN 0
THEN 7 -- This is the value for AbandonedCoinbase
ELSE
NULLIF(cancelled, 0)
END,
direction,
coinbase_block_height,
send_count,
last_send_timestamp,
confirmations,
mined_height,
mined_in_block,
transaction_signature_nonce,
transaction_signature_key
FROM completed_transactions_old;

DROP TABLE completed_transactions_old;
PRAGMA foreign_keys=ON;
19 changes: 10 additions & 9 deletions base_layer/wallet/src/output_manager_service/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,21 +954,22 @@ where
)?;

// Clear any existing pending coinbase transactions for this blockheight if they exist
if let Err(e) = self
match self
.resources
.db
.clear_pending_coinbase_transaction_at_block_height(block_height)
.await
{
match e {
OutputManagerStorageError::DieselError(DieselError::NotFound) => {
debug!(
target: LOG_TARGET,
"An existing pending coinbase was cleared for block height {}", block_height
)
},
Ok(_) => {
debug!(
target: LOG_TARGET,
"An existing pending coinbase was cleared for block height {}", block_height
)
},
Err(e) => match e {
OutputManagerStorageError::DieselError(DieselError::NotFound) => {},
_ => return Err(OutputManagerError::from(e)),
}
},
};

// Clear any matching outputs for this commitment. Even if the older output is valid
Expand Down
3 changes: 1 addition & 2 deletions base_layer/wallet/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@ table! {
status -> Integer,
message -> Text,
timestamp -> Timestamp,
cancelled -> Integer,
cancelled -> Nullable<Integer>,
direction -> Nullable<Integer>,
coinbase_block_height -> Nullable<BigInt>,
send_count -> Integer,
last_send_timestamp -> Nullable<Timestamp>,
valid -> Integer,
confirmations -> Nullable<BigInt>,
mined_height -> Nullable<BigInt>,
mined_in_block -> Nullable<Binary>,
Expand Down
2 changes: 2 additions & 0 deletions base_layer/wallet/src/transaction_service/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ pub enum TransactionStorageError {
TransactionNotMined(TxId),
#[error("Conversion error: `{0}`")]
ByteArrayError(#[from] ByteArrayError),
#[error("Not a coinbase transaction so cannot be abandoned")]
NotCoinbase,
}

/// This error type is used to return TransactionServiceErrors from inside a Transaction Service protocol but also
Expand Down
11 changes: 8 additions & 3 deletions base_layer/wallet/src/transaction_service/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,13 @@ use tower::Service;
use crate::{
transaction_service::{
error::TransactionServiceError,
protocols::TxRejection,
storage::models::{CompletedTransaction, InboundTransaction, OutboundTransaction, WalletTransaction},
storage::models::{
CompletedTransaction,
InboundTransaction,
OutboundTransaction,
TxCancellationReason,
WalletTransaction,
},
},
OperationId,
};
Expand Down Expand Up @@ -208,7 +213,7 @@ pub enum TransactionEvent {
TransactionDirectSendResult(TxId, bool),
TransactionCompletedImmediately(TxId),
TransactionStoreForwardSendResult(TxId, bool),
TransactionCancelled(TxId, TxRejection),
TransactionCancelled(TxId, TxCancellationReason),
TransactionBroadcast(TxId),
TransactionImported(TxId),
FauxTransactionUnconfirmed {
Expand Down
11 changes: 0 additions & 11 deletions base_layer/wallet/src/transaction_service/protocols/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,6 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
pub enum TxRejection {
Unknown, // 0
UserCancelled, // 1
Timeout, // 2
DoubleSpend, // 3
Orphan, // 4
TimeLocked, // 5
InvalidTransaction, // 6
}

pub mod transaction_broadcast_protocol;
pub mod transaction_receive_protocol;
pub mod transaction_send_protocol;
Expand Down
Loading