From 15347614df1cd2ade1fa8fe22f200fd79ee8de23 Mon Sep 17 00:00:00 2001 From: Utkarsh Gupta <32920299+utkarshg6@users.noreply.github.com> Date: Fri, 4 Apr 2025 17:11:46 +0530 Subject: [PATCH 01/46] GH-601: Introduce SentPayable Table (#611) * GH-601: introduce sent_payable table * GH-601: test that table with certain columns were created during migration * GH-601: resuse code to test fields of table * GH-601: migrate utils code * GH-601: trigger actions * GH-601: fix test constants_have_correct_values * GH-601: review changes * GH-601: move constant to database/test_utils.rs --- masq_lib/src/constants.rs | 2 +- node/src/database/db_initializer.rs | 57 ++++++++++++- .../src/database/db_migrations/db_migrator.rs | 2 + .../migrations/migration_10_to_11.rs | 85 +++++++++++++++++++ .../migrations/migration_8_to_9.rs | 27 +++--- .../migrations/migration_9_to_10.rs | 27 +++--- .../database/db_migrations/migrations/mod.rs | 2 + node/src/database/test_utils/mod.rs | 37 ++++++++ node/src/test_utils/database_utils.rs | 5 ++ 9 files changed, 216 insertions(+), 28 deletions(-) create mode 100644 node/src/database/db_migrations/migrations/migration_10_to_11.rs diff --git a/masq_lib/src/constants.rs b/masq_lib/src/constants.rs index 155bff5cc..bcd1b94c7 100644 --- a/masq_lib/src/constants.rs +++ b/masq_lib/src/constants.rs @@ -5,7 +5,7 @@ use crate::data_version::DataVersion; use const_format::concatcp; pub const DEFAULT_CHAIN: Chain = Chain::PolyMainnet; -pub const CURRENT_SCHEMA_VERSION: usize = 10; +pub const CURRENT_SCHEMA_VERSION: usize = 11; pub const HIGHEST_RANDOM_CLANDESTINE_PORT: u16 = 9999; pub const HTTP_PORT: u16 = 80; diff --git a/node/src/database/db_initializer.rs b/node/src/database/db_initializer.rs index be5547576..0f9685c2b 100644 --- a/node/src/database/db_initializer.rs +++ b/node/src/database/db_initializer.rs @@ -135,6 +135,7 @@ impl DbInitializerReal { Self::create_config_table(conn); Self::initialize_config(conn, external_params); Self::create_payable_table(conn); + Self::create_sent_payable_table(conn); Self::create_pending_payable_table(conn); Self::create_receivable_table(conn); Self::create_banned_table(conn); @@ -258,6 +259,31 @@ impl DbInitializerReal { Self::set_config_value(conn, "max_block_count", None, false, "maximum block count"); } + pub fn create_sent_payable_table(conn: &Connection) { + conn.execute( + "create table if not exists sent_payable ( + rowid integer primary key, + tx_hash text not null, + receiver_address text not null, + amount_high_b integer not null, + amount_low_b integer not null, + timestamp integer not null, + gas_price_wei integer not null, + nonce integer not null, + status text not null, + retried integer not null + )", + [], + ) + .expect("Can't create sent_payable table"); + + conn.execute( + "CREATE UNIQUE INDEX sent_payable_tx_hash_idx ON sent_payable (tx_hash)", + [], + ) + .expect("Can't create transaction hash index in sent payments"); + } + pub fn create_pending_payable_table(conn: &Connection) { conn.execute( "create table if not exists pending_payable ( @@ -621,6 +647,7 @@ impl Debug for DbInitializationConfig { mod tests { use super::*; use crate::database::db_initializer::InitializationError::SqliteError; + use crate::database::test_utils::SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE; use crate::db_config::config_dao::{ConfigDao, ConfigDaoReal}; use crate::test_utils::database_utils::{ assert_create_table_stm_contains_all_parts, @@ -652,7 +679,7 @@ mod tests { #[test] fn constants_have_correct_values() { assert_eq!(DATABASE_FILE, "node-data.db"); - assert_eq!(CURRENT_SCHEMA_VERSION, 10); + assert_eq!(CURRENT_SCHEMA_VERSION, 11); } #[test] @@ -713,6 +740,34 @@ mod tests { ) } + #[test] + fn db_initialize_creates_sent_payable_table() { + let home_dir = ensure_node_home_directory_does_not_exist( + "db_initializer", + "db_initialize_creates_sent_payable_table", + ); + let subject = DbInitializerReal::default(); + + let conn = subject + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + + let mut stmt = conn.prepare("select rowid, tx_hash, receiver_address, amount_high_b, amount_low_b, timestamp, gas_price_wei, nonce, status, retried from sent_payable").unwrap(); + let mut sent_payable_contents = stmt.query_map([], |_| Ok(42)).unwrap(); + assert!(sent_payable_contents.next().is_none()); + assert_create_table_stm_contains_all_parts( + &*conn, + "sent_payable", + SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE, + ); + let expected_key_words: &[&[&str]] = &[&["tx_hash"]]; + assert_index_stm_is_coupled_with_right_parameter( + conn.as_ref(), + "sent_payable_tx_hash_idx", + expected_key_words, + ) + } + #[test] fn db_initialize_creates_payable_table() { let home_dir = ensure_node_home_directory_does_not_exist( diff --git a/node/src/database/db_migrations/db_migrator.rs b/node/src/database/db_migrations/db_migrator.rs index 7d1ec4f8c..369a78788 100644 --- a/node/src/database/db_migrations/db_migrator.rs +++ b/node/src/database/db_migrations/db_migrator.rs @@ -2,6 +2,7 @@ use crate::database::db_initializer::ExternalData; use crate::database::db_migrations::migrations::migration_0_to_1::Migrate_0_to_1; +use crate::database::db_migrations::migrations::migration_10_to_11::Migrate_10_to_11; use crate::database::db_migrations::migrations::migration_1_to_2::Migrate_1_to_2; use crate::database::db_migrations::migrations::migration_2_to_3::Migrate_2_to_3; use crate::database::db_migrations::migrations::migration_3_to_4::Migrate_3_to_4; @@ -80,6 +81,7 @@ impl DbMigratorReal { &Migrate_7_to_8, &Migrate_8_to_9, &Migrate_9_to_10, + &Migrate_10_to_11, ] } diff --git a/node/src/database/db_migrations/migrations/migration_10_to_11.rs b/node/src/database/db_migrations/migrations/migration_10_to_11.rs new file mode 100644 index 000000000..4b683208d --- /dev/null +++ b/node/src/database/db_migrations/migrations/migration_10_to_11.rs @@ -0,0 +1,85 @@ +use crate::database::db_migrations::db_migrator::DatabaseMigration; +use crate::database::db_migrations::migrator_utils::DBMigDeclarator; + +#[allow(non_camel_case_types)] +pub struct Migrate_10_to_11; + +impl DatabaseMigration for Migrate_10_to_11 { + fn migrate<'a>( + &self, + declaration_utils: Box, + ) -> rusqlite::Result<()> { + let sql_statement = "create table if not exists sent_payable ( + rowid integer primary key, + tx_hash text not null, + receiver_address text not null, + amount_high_b integer not null, + amount_low_b integer not null, + timestamp integer not null, + gas_price_wei integer not null, + nonce integer not null, + status text not null, + retried integer not null + )"; + + declaration_utils.execute_upon_transaction(&[&sql_statement]) + } + + fn old_version(&self) -> usize { + 10 + } +} + +#[cfg(test)] +mod tests { + use crate::database::db_initializer::{ + DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, + }; + use crate::database::test_utils::SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE; + use crate::test_utils::database_utils::{ + assert_create_table_stm_contains_all_parts, assert_table_exists, + bring_db_0_back_to_life_and_return_connection, make_external_data, + }; + use masq_lib::test_utils::logging::{init_test_logging, TestLogHandler}; + use masq_lib::test_utils::utils::ensure_node_home_directory_exists; + use std::fs::create_dir_all; + + #[test] + fn migration_from_10_to_11_is_applied_correctly() { + init_test_logging(); + let dir_path = ensure_node_home_directory_exists( + "db_migrations", + "migration_from_10_to_11_is_properly_set", + ); + create_dir_all(&dir_path).unwrap(); + let db_path = dir_path.join(DATABASE_FILE); + let _ = bring_db_0_back_to_life_and_return_connection(&db_path); + let subject = DbInitializerReal::default(); + + let _prev_connection = subject + .initialize_to_version( + &dir_path, + 10, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); + + let connection = subject + .initialize_to_version( + &dir_path, + 11, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); + + assert_table_exists(connection.as_ref(), "sent_payable"); + assert_create_table_stm_contains_all_parts( + &*connection, + "sent_payable", + SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE, + ); + TestLogHandler::new().assert_logs_contain_in_order(vec![ + "DbMigrator: Database successfully migrated from version 10 to 11", + ]); + } +} diff --git a/node/src/database/db_migrations/migrations/migration_8_to_9.rs b/node/src/database/db_migrations/migrations/migration_8_to_9.rs index 4bf95e955..eb89ac002 100644 --- a/node/src/database/db_migrations/migrations/migration_8_to_9.rs +++ b/node/src/database/db_migrations/migrations/migration_8_to_9.rs @@ -43,21 +43,22 @@ mod tests { let _ = bring_db_0_back_to_life_and_return_connection(&db_path); let subject = DbInitializerReal::default(); - let result = subject.initialize_to_version( - &dir_path, - 8, - DbInitializationConfig::create_or_migrate(make_external_data()), - ); - - assert!(result.is_ok()); + let _prev_connection = subject + .initialize_to_version( + &dir_path, + 8, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); - let result = subject.initialize_to_version( - &dir_path, - 9, - DbInitializationConfig::create_or_migrate(make_external_data()), - ); + let connection = subject + .initialize_to_version( + &dir_path, + 9, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); - let connection = result.unwrap(); let (mp_value, mp_encrypted) = retrieve_config_row(connection.as_ref(), "max_block_count"); let (cs_value, cs_encrypted) = retrieve_config_row(connection.as_ref(), "schema_version"); assert_eq!(mp_value, None); diff --git a/node/src/database/db_migrations/migrations/migration_9_to_10.rs b/node/src/database/db_migrations/migrations/migration_9_to_10.rs index 7622ef01f..be240429a 100644 --- a/node/src/database/db_migrations/migrations/migration_9_to_10.rs +++ b/node/src/database/db_migrations/migrations/migration_9_to_10.rs @@ -43,21 +43,22 @@ mod tests { let _ = bring_db_0_back_to_life_and_return_connection(&db_path); let subject = DbInitializerReal::default(); - let result = subject.initialize_to_version( - &dir_path, - 9, - DbInitializationConfig::create_or_migrate(make_external_data()), - ); - - assert!(result.is_ok()); + let _prev_connection = subject + .initialize_to_version( + &dir_path, + 9, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); - let result = subject.initialize_to_version( - &dir_path, - 10, - DbInitializationConfig::create_or_migrate(make_external_data()), - ); + let connection = subject + .initialize_to_version( + &dir_path, + 10, + DbInitializationConfig::create_or_migrate(make_external_data()), + ) + .unwrap(); - let connection = result.unwrap(); let (mp_value, mp_encrypted) = retrieve_config_row(connection.as_ref(), "max_block_count"); let (cs_value, cs_encrypted) = retrieve_config_row(connection.as_ref(), "schema_version"); assert_eq!(mp_value, Some(100_000u64.to_string())); diff --git a/node/src/database/db_migrations/migrations/mod.rs b/node/src/database/db_migrations/migrations/mod.rs index bcdb14176..e093df006 100644 --- a/node/src/database/db_migrations/migrations/mod.rs +++ b/node/src/database/db_migrations/migrations/mod.rs @@ -10,3 +10,5 @@ pub mod migration_6_to_7; pub mod migration_7_to_8; pub mod migration_8_to_9; pub mod migration_9_to_10; +#[rustfmt::skip] +pub mod migration_10_to_11; diff --git a/node/src/database/test_utils/mod.rs b/node/src/database/test_utils/mod.rs index e8b9060f1..a9c5fac77 100644 --- a/node/src/database/test_utils/mod.rs +++ b/node/src/database/test_utils/mod.rs @@ -12,6 +12,19 @@ use std::fmt::Debug; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; +pub const SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE: &[&[&str]] = &[ + &["rowid", "integer", "primary", "key"], + &["tx_hash", "text", "not", "null"], + &["receiver_address", "text", "not", "null"], + &["amount_high_b", "integer", "not", "null"], + &["amount_low_b", "integer", "not", "null"], + &["timestamp", "integer", "not", "null"], + &["gas_price_wei", "integer", "not", "null"], + &["nonce", "integer", "not", "null"], + &["status", "text", "not", "null"], + &["retried", "integer", "not", "null"], +]; + #[derive(Debug, Default)] pub struct ConnectionWrapperMock<'conn> { prepare_params: Arc>>, @@ -114,3 +127,27 @@ impl DbInitializerMock { self } } + +#[cfg(test)] +mod tests { + use crate::database::test_utils::SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE; + + #[test] + fn constants_have_correct_values() { + assert_eq!( + SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE, + &[ + &["rowid", "integer", "primary", "key"], + &["tx_hash", "text", "not", "null"], + &["receiver_address", "text", "not", "null"], + &["amount_high_b", "integer", "not", "null"], + &["amount_low_b", "integer", "not", "null"], + &["timestamp", "integer", "not", "null"], + &["gas_price_wei", "integer", "not", "null"], + &["nonce", "integer", "not", "null"], + &["status", "text", "not", "null"], + &["retried", "integer", "not", "null"] + ] + ); + } +} diff --git a/node/src/test_utils/database_utils.rs b/node/src/test_utils/database_utils.rs index 02ba441a4..fb8ba3a83 100644 --- a/node/src/test_utils/database_utils.rs +++ b/node/src/test_utils/database_utils.rs @@ -103,6 +103,11 @@ pub fn retrieve_config_row(conn: &dyn ConnectionWrapper, name: &str) -> (Option< }) } +pub fn assert_table_exists(conn: &dyn ConnectionWrapper, table_name: &str) { + let result = conn.prepare(&format!("select * from {}", table_name)); + assert!(result.is_ok(), "Table {} should exist", table_name); +} + pub fn assert_table_does_not_exist(conn: &dyn ConnectionWrapper, table_name: &str) { let error_stm = conn .prepare(&format!("select * from {}", table_name)) From df415c0bfd0448db7b7caebd0ed4ea77058bd305 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 10 Apr 2025 13:04:48 +0530 Subject: [PATCH 02/46] GH-608: change the trait object to support SentPayables --- .../db_access_objects/pending_payable_dao.rs | 125 ++++++++++-------- 1 file changed, 71 insertions(+), 54 deletions(-) diff --git a/node/src/accountant/db_access_objects/pending_payable_dao.rs b/node/src/accountant/db_access_objects/pending_payable_dao.rs index 67c779ce0..91dae29f0 100644 --- a/node/src/accountant/db_access_objects/pending_payable_dao.rs +++ b/node/src/accountant/db_access_objects/pending_payable_dao.rs @@ -15,10 +15,11 @@ use std::collections::HashSet; use std::fmt::Debug; use std::str::FromStr; use std::time::SystemTime; -use web3::types::H256; +use web3::types::{Address, H256}; +use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; #[derive(Debug, PartialEq, Eq)] -pub enum PendingPayableDaoError { +pub enum SentPayableDaoError { InsertionFailed(String), UpdateFailed(String), SignConversionError(u64), @@ -28,27 +29,43 @@ pub enum PendingPayableDaoError { } #[derive(Debug)] -pub struct TransactionHashes { +pub struct TxIdentifiers { pub rowid_results: Vec<(u64, H256)>, pub no_rowid_results: Vec, } -pub trait PendingPayableDao { +struct TransactionRecord { + tx: Tx, + status: TxStatus, +} + +struct Tx { + // GH-608: Perhaps TxReceipt could be a similar structure to be used + receiver_address: Address, + amount: u128, + tx_hash: String, + timestamp: SystemTime, + gas_price_wei: u64, + nonce: u32, +} + +struct StatusChange { + new_status: TxStatus, +} + +pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed - fn fingerprints_rowids(&self, hashes: &[H256]) -> TransactionHashes; - fn return_all_errorless_fingerprints(&self) -> Vec; - fn insert_new_fingerprints( - &self, - hashes_and_amounts: &[HashAndAmount], - batch_wide_timestamp: SystemTime, - ) -> Result<(), PendingPayableDaoError>; - fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; - fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; - fn mark_failures(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; + fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; + fn retrieve_pending_txs(&self) -> Vec; + + fn retrieve_txs_to_retry(&self) -> Vec; + fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; + fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; } -impl PendingPayableDao for PendingPayableDaoReal<'_> { - fn fingerprints_rowids(&self, hashes: &[H256]) -> TransactionHashes { +impl SentPayableDao for SentPayableDaoReal<'_> { + fn transaction_rowids(&self, hashes: &[H256]) -> TxIdentifiers { //Vec<(Option, H256)> { fn hash_and_rowid_in_single_row(row: &Row) -> rusqlite::Result<(u64, H256)> { let hash_str: String = row.get(0).expectv("hash"); @@ -81,7 +98,7 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { .cloned() .collect(); - TransactionHashes { + TxIdentifiers { rowid_results: all_found_records, no_rowid_results: hashes_of_missing_rowids, } @@ -128,7 +145,7 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { &self, hashes_and_amounts: &[HashAndAmount], batch_wide_timestamp: SystemTime, - ) -> Result<(), PendingPayableDaoError> { + ) -> Result<(), SentPayableDaoError> { fn values_clause_for_fingerprints_to_insert( hashes_and_amounts: &[HashAndAmount], batch_wide_timestamp: SystemTime, @@ -162,11 +179,11 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { hashes_and_amounts.len(), x ), - Err(e) => Err(PendingPayableDaoError::InsertionFailed(e.to_string())), + Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), } } - fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { + fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { let sql = format!( "delete from pending_payable where rowid in ({})", Self::serialize_ids(ids) @@ -183,11 +200,11 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { ids.len(), num ), - Err(e) => Err(PendingPayableDaoError::RecordDeletion(e.to_string())), + Err(e) => Err(SentPayableDaoError::RecordDeletion(e.to_string())), } } - fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { + fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { let sql = format!( "update pending_payable set attempt = attempt + 1 where rowid in ({})", Self::serialize_ids(ids) @@ -199,11 +216,11 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { ids.len(), num ), - Err(e) => Err(PendingPayableDaoError::UpdateFailed(e.to_string())), + Err(e) => Err(SentPayableDaoError::UpdateFailed(e.to_string())), } } - fn mark_failures(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { + fn mark_failures(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { let sql = format!( "update pending_payable set process_error = 'ERROR' where rowid in ({})", Self::serialize_ids(ids) @@ -220,7 +237,7 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { ids.len(), num ) , - Err(e) => Err(PendingPayableDaoError::ErrorMarkFailed(e.to_string())), + Err(e) => Err(SentPayableDaoError::ErrorMarkFailed(e.to_string())), } } } @@ -241,11 +258,11 @@ impl PendingPayable { } #[derive(Debug)] -pub struct PendingPayableDaoReal<'a> { +pub struct SentPayableDaoReal<'a> { conn: Box, } -impl<'a> PendingPayableDaoReal<'a> { +impl<'a> SentPayableDaoReal<'a> { pub fn new(conn: Box) -> Self { Self { conn } } @@ -260,12 +277,12 @@ impl<'a> PendingPayableDaoReal<'a> { } pub trait PendingPayableDaoFactory { - fn make(&self) -> Box; + fn make(&self) -> Box; } impl PendingPayableDaoFactory for DaoFactoryReal { - fn make(&self) -> Box { - Box::new(PendingPayableDaoReal::new(self.make_connection())) + fn make(&self) -> Box { + Box::new(SentPayableDaoReal::new(self.make_connection())) } } @@ -273,7 +290,7 @@ impl PendingPayableDaoFactory for DaoFactoryReal { mod tests { use crate::accountant::checked_conversion; use crate::accountant::db_access_objects::pending_payable_dao::{ - PendingPayableDao, PendingPayableDaoError, PendingPayableDaoReal, + SentPayableDao, SentPayableDaoError, SentPayableDaoReal, }; use crate::accountant::db_access_objects::utils::from_time_t; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; @@ -305,7 +322,7 @@ mod tests { let hash_2 = make_tx_hash(6789); let amount_2 = 44445; let batch_wide_timestamp = from_time_t(200_000_000); - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let hash_and_amount_1 = HashAndAmount { hash: hash_1, amount: amount_1, @@ -366,14 +383,14 @@ mod tests { let hash = make_tx_hash(45466); let amount = 55556; let timestamp = from_time_t(200_000_000); - let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash, amount }; let result = subject.insert_new_fingerprints(&[hash_and_amount], timestamp); assert_eq!( result, - Err(PendingPayableDaoError::InsertionFailed( + Err(SentPayableDaoError::InsertionFailed( "attempt to write a readonly database".to_string() )) ) @@ -395,7 +412,7 @@ mod tests { let hash_1 = make_tx_hash(4546); let amount_1 = 55556; let batch_wide_timestamp = from_time_t(200_000_000); - let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash: hash_1, amount: amount_1, @@ -413,7 +430,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let timestamp = from_time_t(195_000_000); // use full range tx hashes because SqLite has tendencies to see the value as a hex and convert it to an integer, // then complain about its excessive size if supplied in unquoted strings @@ -438,7 +455,7 @@ mod tests { .unwrap(); } - let result = subject.fingerprints_rowids(&[hash_1, hash_2]); + let result = subject.transaction_rowids(&[hash_1, hash_2]); let first_expected_pair = &(1, hash_1); assert!( @@ -466,7 +483,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let hash_1 = make_tx_hash(11119); let hash_2 = make_tx_hash(22229); let hash_3 = make_tx_hash(33339); @@ -494,7 +511,7 @@ mod tests { .unwrap(); subject.delete_fingerprints(&[1]).unwrap(); - let result = subject.fingerprints_rowids(&[hash_1, hash_2, hash_3, hash_4]); + let result = subject.transaction_rowids(&[hash_1, hash_2, hash_3, hash_4]); assert_eq!(result.rowid_results, vec![(2, hash_3),]); assert_eq!(result.no_rowid_results, vec![hash_1, hash_2, hash_4]); @@ -509,7 +526,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let batch_wide_timestamp = from_time_t(195_000_000); let hash_1 = make_tx_hash(11119); let amount_1 = 787; @@ -567,7 +584,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let timestamp = from_time_t(198_000_000); let hash = make_tx_hash(10000); let amount = 333; @@ -619,7 +636,7 @@ mod tests { .execute([]) .unwrap(); } - let subject = PendingPayableDaoReal::new(wrapped_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); let _ = subject.return_all_errorless_fingerprints(); } @@ -633,7 +650,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -684,13 +701,13 @@ mod tests { .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); let rowid = 45; - let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.delete_fingerprints(&[rowid]); assert_eq!( result, - Err(PendingPayableDaoError::RecordDeletion( + Err(SentPayableDaoError::RecordDeletion( "attempt to write a readonly database".to_string() )) ) @@ -710,7 +727,7 @@ mod tests { .unwrap(); let rowid_1 = 1; let rowid_2 = 2; - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -751,7 +768,7 @@ mod tests { amount: 3344, }; let timestamp = from_time_t(190_000_000); - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -794,13 +811,13 @@ mod tests { ) .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); - let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.increment_scan_attempts(&[1]); assert_eq!( result, - Err(PendingPayableDaoError::UpdateFailed( + Err(SentPayableDaoError::UpdateFailed( "attempt to write a readonly database".to_string() )) ) @@ -818,7 +835,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); let _ = subject.increment_scan_attempts(&[1, 2]); } @@ -843,7 +860,7 @@ mod tests { amount: amount_2, }; let timestamp = from_time_t(190_000_000); - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); { subject .insert_new_fingerprints(&[hash_and_amount_1, hash_and_amount_2], timestamp) @@ -919,13 +936,13 @@ mod tests { ) .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); - let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.mark_failures(&[1]); assert_eq!( result, - Err(PendingPayableDaoError::ErrorMarkFailed( + Err(SentPayableDaoError::ErrorMarkFailed( "attempt to write a readonly database".to_string() )) ) @@ -943,7 +960,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = PendingPayableDaoReal::new(conn); + let subject = SentPayableDaoReal::new(conn); let _ = subject.mark_failures(&[10, 20]); } From f9f43be49f9d06e0ce1cd79cd440560acadd5626 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 11 Apr 2025 11:14:51 +0530 Subject: [PATCH 03/46] Revert "GH-608: change the trait object to support SentPayables" This reverts commit df415c0bfd0448db7b7caebd0ed4ea77058bd305. --- .../db_access_objects/pending_payable_dao.rs | 125 ++++++++---------- 1 file changed, 54 insertions(+), 71 deletions(-) diff --git a/node/src/accountant/db_access_objects/pending_payable_dao.rs b/node/src/accountant/db_access_objects/pending_payable_dao.rs index 91dae29f0..67c779ce0 100644 --- a/node/src/accountant/db_access_objects/pending_payable_dao.rs +++ b/node/src/accountant/db_access_objects/pending_payable_dao.rs @@ -15,11 +15,10 @@ use std::collections::HashSet; use std::fmt::Debug; use std::str::FromStr; use std::time::SystemTime; -use web3::types::{Address, H256}; -use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; +use web3::types::H256; #[derive(Debug, PartialEq, Eq)] -pub enum SentPayableDaoError { +pub enum PendingPayableDaoError { InsertionFailed(String), UpdateFailed(String), SignConversionError(u64), @@ -29,43 +28,27 @@ pub enum SentPayableDaoError { } #[derive(Debug)] -pub struct TxIdentifiers { +pub struct TransactionHashes { pub rowid_results: Vec<(u64, H256)>, pub no_rowid_results: Vec, } -struct TransactionRecord { - tx: Tx, - status: TxStatus, -} - -struct Tx { - // GH-608: Perhaps TxReceipt could be a similar structure to be used - receiver_address: Address, - amount: u128, - tx_hash: String, - timestamp: SystemTime, - gas_price_wei: u64, - nonce: u32, -} - -struct StatusChange { - new_status: TxStatus, -} - -pub trait SentPayableDao { +pub trait PendingPayableDao { // Note that the order of the returned results is not guaranteed - fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; - fn retrieve_pending_txs(&self) -> Vec; - - fn retrieve_txs_to_retry(&self) -> Vec; - fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; - fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; - fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; + fn fingerprints_rowids(&self, hashes: &[H256]) -> TransactionHashes; + fn return_all_errorless_fingerprints(&self) -> Vec; + fn insert_new_fingerprints( + &self, + hashes_and_amounts: &[HashAndAmount], + batch_wide_timestamp: SystemTime, + ) -> Result<(), PendingPayableDaoError>; + fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; + fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; + fn mark_failures(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError>; } -impl SentPayableDao for SentPayableDaoReal<'_> { - fn transaction_rowids(&self, hashes: &[H256]) -> TxIdentifiers { +impl PendingPayableDao for PendingPayableDaoReal<'_> { + fn fingerprints_rowids(&self, hashes: &[H256]) -> TransactionHashes { //Vec<(Option, H256)> { fn hash_and_rowid_in_single_row(row: &Row) -> rusqlite::Result<(u64, H256)> { let hash_str: String = row.get(0).expectv("hash"); @@ -98,7 +81,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .cloned() .collect(); - TxIdentifiers { + TransactionHashes { rowid_results: all_found_records, no_rowid_results: hashes_of_missing_rowids, } @@ -145,7 +128,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { &self, hashes_and_amounts: &[HashAndAmount], batch_wide_timestamp: SystemTime, - ) -> Result<(), SentPayableDaoError> { + ) -> Result<(), PendingPayableDaoError> { fn values_clause_for_fingerprints_to_insert( hashes_and_amounts: &[HashAndAmount], batch_wide_timestamp: SystemTime, @@ -179,11 +162,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { hashes_and_amounts.len(), x ), - Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), + Err(e) => Err(PendingPayableDaoError::InsertionFailed(e.to_string())), } } - fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { + fn delete_fingerprints(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { let sql = format!( "delete from pending_payable where rowid in ({})", Self::serialize_ids(ids) @@ -200,11 +183,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { ids.len(), num ), - Err(e) => Err(SentPayableDaoError::RecordDeletion(e.to_string())), + Err(e) => Err(PendingPayableDaoError::RecordDeletion(e.to_string())), } } - fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { + fn increment_scan_attempts(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { let sql = format!( "update pending_payable set attempt = attempt + 1 where rowid in ({})", Self::serialize_ids(ids) @@ -216,11 +199,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { ids.len(), num ), - Err(e) => Err(SentPayableDaoError::UpdateFailed(e.to_string())), + Err(e) => Err(PendingPayableDaoError::UpdateFailed(e.to_string())), } } - fn mark_failures(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { + fn mark_failures(&self, ids: &[u64]) -> Result<(), PendingPayableDaoError> { let sql = format!( "update pending_payable set process_error = 'ERROR' where rowid in ({})", Self::serialize_ids(ids) @@ -237,7 +220,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { ids.len(), num ) , - Err(e) => Err(SentPayableDaoError::ErrorMarkFailed(e.to_string())), + Err(e) => Err(PendingPayableDaoError::ErrorMarkFailed(e.to_string())), } } } @@ -258,11 +241,11 @@ impl PendingPayable { } #[derive(Debug)] -pub struct SentPayableDaoReal<'a> { +pub struct PendingPayableDaoReal<'a> { conn: Box, } -impl<'a> SentPayableDaoReal<'a> { +impl<'a> PendingPayableDaoReal<'a> { pub fn new(conn: Box) -> Self { Self { conn } } @@ -277,12 +260,12 @@ impl<'a> SentPayableDaoReal<'a> { } pub trait PendingPayableDaoFactory { - fn make(&self) -> Box; + fn make(&self) -> Box; } impl PendingPayableDaoFactory for DaoFactoryReal { - fn make(&self) -> Box { - Box::new(SentPayableDaoReal::new(self.make_connection())) + fn make(&self) -> Box { + Box::new(PendingPayableDaoReal::new(self.make_connection())) } } @@ -290,7 +273,7 @@ impl PendingPayableDaoFactory for DaoFactoryReal { mod tests { use crate::accountant::checked_conversion; use crate::accountant::db_access_objects::pending_payable_dao::{ - SentPayableDao, SentPayableDaoError, SentPayableDaoReal, + PendingPayableDao, PendingPayableDaoError, PendingPayableDaoReal, }; use crate::accountant::db_access_objects::utils::from_time_t; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; @@ -322,7 +305,7 @@ mod tests { let hash_2 = make_tx_hash(6789); let amount_2 = 44445; let batch_wide_timestamp = from_time_t(200_000_000); - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let hash_and_amount_1 = HashAndAmount { hash: hash_1, amount: amount_1, @@ -383,14 +366,14 @@ mod tests { let hash = make_tx_hash(45466); let amount = 55556; let timestamp = from_time_t(200_000_000); - let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash, amount }; let result = subject.insert_new_fingerprints(&[hash_and_amount], timestamp); assert_eq!( result, - Err(SentPayableDaoError::InsertionFailed( + Err(PendingPayableDaoError::InsertionFailed( "attempt to write a readonly database".to_string() )) ) @@ -412,7 +395,7 @@ mod tests { let hash_1 = make_tx_hash(4546); let amount_1 = 55556; let batch_wide_timestamp = from_time_t(200_000_000); - let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash: hash_1, amount: amount_1, @@ -430,7 +413,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let timestamp = from_time_t(195_000_000); // use full range tx hashes because SqLite has tendencies to see the value as a hex and convert it to an integer, // then complain about its excessive size if supplied in unquoted strings @@ -455,7 +438,7 @@ mod tests { .unwrap(); } - let result = subject.transaction_rowids(&[hash_1, hash_2]); + let result = subject.fingerprints_rowids(&[hash_1, hash_2]); let first_expected_pair = &(1, hash_1); assert!( @@ -483,7 +466,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let hash_1 = make_tx_hash(11119); let hash_2 = make_tx_hash(22229); let hash_3 = make_tx_hash(33339); @@ -511,7 +494,7 @@ mod tests { .unwrap(); subject.delete_fingerprints(&[1]).unwrap(); - let result = subject.transaction_rowids(&[hash_1, hash_2, hash_3, hash_4]); + let result = subject.fingerprints_rowids(&[hash_1, hash_2, hash_3, hash_4]); assert_eq!(result.rowid_results, vec![(2, hash_3),]); assert_eq!(result.no_rowid_results, vec![hash_1, hash_2, hash_4]); @@ -526,7 +509,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let batch_wide_timestamp = from_time_t(195_000_000); let hash_1 = make_tx_hash(11119); let amount_1 = 787; @@ -584,7 +567,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let timestamp = from_time_t(198_000_000); let hash = make_tx_hash(10000); let amount = 333; @@ -636,7 +619,7 @@ mod tests { .execute([]) .unwrap(); } - let subject = SentPayableDaoReal::new(wrapped_conn); + let subject = PendingPayableDaoReal::new(wrapped_conn); let _ = subject.return_all_errorless_fingerprints(); } @@ -650,7 +633,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -701,13 +684,13 @@ mod tests { .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); let rowid = 45; - let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.delete_fingerprints(&[rowid]); assert_eq!( result, - Err(SentPayableDaoError::RecordDeletion( + Err(PendingPayableDaoError::RecordDeletion( "attempt to write a readonly database".to_string() )) ) @@ -727,7 +710,7 @@ mod tests { .unwrap(); let rowid_1 = 1; let rowid_2 = 2; - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -768,7 +751,7 @@ mod tests { amount: 3344, }; let timestamp = from_time_t(190_000_000); - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); { subject .insert_new_fingerprints( @@ -811,13 +794,13 @@ mod tests { ) .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); - let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.increment_scan_attempts(&[1]); assert_eq!( result, - Err(SentPayableDaoError::UpdateFailed( + Err(PendingPayableDaoError::UpdateFailed( "attempt to write a readonly database".to_string() )) ) @@ -835,7 +818,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); let _ = subject.increment_scan_attempts(&[1, 2]); } @@ -860,7 +843,7 @@ mod tests { amount: amount_2, }; let timestamp = from_time_t(190_000_000); - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); { subject .insert_new_fingerprints(&[hash_and_amount_1, hash_and_amount_2], timestamp) @@ -936,13 +919,13 @@ mod tests { ) .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); - let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.mark_failures(&[1]); assert_eq!( result, - Err(SentPayableDaoError::ErrorMarkFailed( + Err(PendingPayableDaoError::ErrorMarkFailed( "attempt to write a readonly database".to_string() )) ) @@ -960,7 +943,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let subject = SentPayableDaoReal::new(conn); + let subject = PendingPayableDaoReal::new(conn); let _ = subject.mark_failures(&[10, 20]); } From 17360c5d6f1b4529b3614b5d2f916caf91c2b63f Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 11 Apr 2025 11:23:12 +0530 Subject: [PATCH 04/46] GH-608: introduce sent payable dao --- node/src/accountant/db_access_objects/mod.rs | 1 + .../db_access_objects/sent_payable_dao.rs | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 node/src/accountant/db_access_objects/sent_payable_dao.rs diff --git a/node/src/accountant/db_access_objects/mod.rs b/node/src/accountant/db_access_objects/mod.rs index a350148ab..ce727bea8 100644 --- a/node/src/accountant/db_access_objects/mod.rs +++ b/node/src/accountant/db_access_objects/mod.rs @@ -4,4 +4,5 @@ pub mod banned_dao; pub mod payable_dao; pub mod pending_payable_dao; pub mod receivable_dao; +pub mod sent_payable_dao; pub mod utils; diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs new file mode 100644 index 000000000..701e2bb52 --- /dev/null +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -0,0 +1,44 @@ +use std::time::SystemTime; +use ethereum_types::H256; +use web3::types::Address; +use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; + +#[derive(Debug, PartialEq, Eq)] +pub enum SentPayableDaoError { + // InsertionFailed(String), + // UpdateFailed(String), + // SignConversionError(u64), + // RecordCannotBeRead, + // RecordDeletion(String), + // ErrorMarkFailed(String), +} + +#[derive(Debug)] +pub struct TxIdentifiers { + // pub rowid_results: Vec<(u64, H256)>, + // pub no_rowid_results: Vec, +} + +pub struct Tx { + // GH-608: Perhaps TxReceipt could be a similar structure to be used + receiver_address: Address, + amount: u128, + tx_hash: String, + timestamp: SystemTime, + gas_price_wei: u64, + nonce: u32, +} + +pub struct StatusChange { + new_status: TxStatus, +} + +pub trait SentPayableDao { + // Note that the order of the returned results is not guaranteed + fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; + fn retrieve_pending_txs(&self) -> Vec; + fn retrieve_txs_to_retry(&self) -> Vec; + fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; + fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; +} From b9c56ce87523d890250789b03e0de770f830ca2b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 11 Apr 2025 15:59:02 +0530 Subject: [PATCH 05/46] GH-608: add boilerplate code --- .../db_access_objects/sent_payable_dao.rs | 95 +++++++++++++++++-- 1 file changed, 89 insertions(+), 6 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 701e2bb52..7724c9d29 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,7 +1,9 @@ +use std::collections::HashMap; use std::time::SystemTime; use ethereum_types::H256; use web3::types::Address; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; +use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { @@ -13,17 +15,13 @@ pub enum SentPayableDaoError { // ErrorMarkFailed(String), } -#[derive(Debug)] -pub struct TxIdentifiers { - // pub rowid_results: Vec<(u64, H256)>, - // pub no_rowid_results: Vec, -} +type TxIdentifiers = HashMap>; pub struct Tx { // GH-608: Perhaps TxReceipt could be a similar structure to be used + tx_hash: String, receiver_address: Address, amount: u128, - tx_hash: String, timestamp: SystemTime, gas_price_wei: u64, nonce: u32, @@ -42,3 +40,88 @@ pub trait SentPayableDao { fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; } + +#[derive(Debug)] +pub struct SentPayableDaoReal<'a> { + conn: Box, +} + +impl<'a> SentPayableDaoReal<'a> { + pub fn new(conn: Box) -> Self { + // TODO: GH-608: Figure out how to test this guy + Self { conn } + } +} + +impl SentPayableDao for SentPayableDaoReal<'_> { + fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers { + todo!() + } + + fn retrieve_pending_txs(&self) -> Vec { + todo!() + } + + fn retrieve_txs_to_retry(&self) -> Vec { + todo!() + } + + fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { + todo!() + } + + fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { + todo!() + } + + fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError> { + todo!() + } +} + +#[cfg(test)] +mod tests { + use crate::accountant::db_access_objects::pending_payable_dao::{ + PendingPayableDao, PendingPayableDaoReal, + }; + use crate::accountant::db_access_objects::sent_payable_dao::{ + SentPayableDao, SentPayableDaoReal, Tx, + }; + use crate::database::db_initializer::{ + DbInitializationConfig, DbInitializer, DbInitializerReal, + }; + use masq_lib::test_utils::utils::ensure_node_home_directory_exists; + + #[test] + fn insert_new_records_works() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "insert_new_records_works"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + // let tx = Tx { + // tx_hash: "".to_string(), + // receiver_address: Default::default(), + // amount: 0, + // timestamp: (), + // gas_price_wei: 0, + // nonce: 0, + // }; + let subject = SentPayableDaoReal::new(wrapped_conn); + + // let _ = subject.insert_new_records() + todo!("finish me first"); + } + + #[test] + fn get_tx_identifiers_works() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "get_tx_identifiers_works"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + + todo!("first you'll have to write tests for the insertion method"); + } +} From 00dba1777e181734d705c2b187446cfd3410764b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Mon, 14 Apr 2025 14:41:03 +0530 Subject: [PATCH 06/46] GH-608: implement insert_new_records --- .../db_access_objects/sent_payable_dao.rs | 74 ++++++++++++++----- 1 file changed, 57 insertions(+), 17 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 7724c9d29..0e1f3cea4 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -2,12 +2,16 @@ use std::collections::HashMap; use std::time::SystemTime; use ethereum_types::H256; use web3::types::Address; +use crate::accountant::{checked_conversion, comma_joined_stringifiable}; +use crate::accountant::db_access_objects::pending_payable_dao::PendingPayableDaoError; +use crate::accountant::db_access_objects::utils::to_time_t; +use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { - // InsertionFailed(String), + InsertionFailed(String), // UpdateFailed(String), // SignConversionError(u64), // RecordCannotBeRead, @@ -19,7 +23,7 @@ type TxIdentifiers = HashMap>; pub struct Tx { // GH-608: Perhaps TxReceipt could be a similar structure to be used - tx_hash: String, + hash: H256, receiver_address: Address, amount: u128, timestamp: SystemTime, @@ -48,7 +52,7 @@ pub struct SentPayableDaoReal<'a> { impl<'a> SentPayableDaoReal<'a> { pub fn new(conn: Box) -> Self { - // TODO: GH-608: Figure out how to test this guy + // TODO: GH-608: You'll need to write a mock to test this, do that wisely Self { conn } } } @@ -67,7 +71,36 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { - todo!() + fn generate_values(txs: &[Tx]) -> String { + comma_joined_stringifiable(txs, |tx| { + let amount_checked = checked_conversion::(tx.amount); + let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); + format!( + "('{:?}', '{:?}', {}, {}, {}, {}, {}, 'Pending', 0)", + tx.hash, + tx.receiver_address, + high_bytes, + low_bytes, + to_time_t(tx.timestamp), + tx.gas_price_wei, + tx.nonce, + ) + }) + } + + let sql = format!( + "insert into sent_payable (\ + tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status, retried\ + ) values {}", + generate_values(&txs) + ); + + match self.conn.prepare(&sql).expect("Internal error").execute([]) { + Ok(x) if x == txs.len() => Ok(()), + Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), + Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), + } } fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { @@ -81,9 +114,6 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::pending_payable_dao::{ - PendingPayableDao, PendingPayableDaoReal, - }; use crate::accountant::db_access_objects::sent_payable_dao::{ SentPayableDao, SentPayableDaoReal, Tx, }; @@ -91,6 +121,7 @@ mod tests { DbInitializationConfig, DbInitializer, DbInitializerReal, }; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; + use std::time::SystemTime; #[test] fn insert_new_records_works() { @@ -99,18 +130,27 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - // let tx = Tx { - // tx_hash: "".to_string(), - // receiver_address: Default::default(), - // amount: 0, - // timestamp: (), - // gas_price_wei: 0, - // nonce: 0, - // }; + let tx = Tx { + hash: Default::default(), + receiver_address: Default::default(), + amount: 0, + timestamp: SystemTime::now(), + gas_price_wei: 0, + nonce: 0, + }; let subject = SentPayableDaoReal::new(wrapped_conn); - // let _ = subject.insert_new_records() - todo!("finish me first"); + let result = subject.insert_new_records(vec![tx]); + + let row_count: i64 = { + let mut stmt = subject + .conn + .prepare("SELECT COUNT(*) FROM sent_payable") + .unwrap(); + stmt.query_row([], |row| row.get(0)).unwrap() + }; + assert_eq!(result, Ok(())); + assert_eq!(row_count, 1); } #[test] From 077906649298a40e4cbbfb212f3000fcc1928ecc Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 15 Apr 2025 16:12:54 +0530 Subject: [PATCH 07/46] GH-608: add test cases for the panic and error clause for the insert method --- .../db_access_objects/sent_payable_dao.rs | 112 +++++++++++++----- 1 file changed, 84 insertions(+), 28 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 0e1f3cea4..502db77e4 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -55,6 +55,23 @@ impl<'a> SentPayableDaoReal<'a> { // TODO: GH-608: You'll need to write a mock to test this, do that wisely Self { conn } } + + fn generate_sql_values_for_insertion(txs: &[Tx]) -> String { + comma_joined_stringifiable(txs, |tx| { + let amount_checked = checked_conversion::(tx.amount); + let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); + format!( + "('{:?}', '{:?}', {}, {}, {}, {}, {}, 'Pending', 0)", + tx.hash, + tx.receiver_address, + high_bytes, + low_bytes, + to_time_t(tx.timestamp), + tx.gas_price_wei, + tx.nonce, + ) + }) + } } impl SentPayableDao for SentPayableDaoReal<'_> { @@ -71,29 +88,12 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { - fn generate_values(txs: &[Tx]) -> String { - comma_joined_stringifiable(txs, |tx| { - let amount_checked = checked_conversion::(tx.amount); - let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); - format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, 'Pending', 0)", - tx.hash, - tx.receiver_address, - high_bytes, - low_bytes, - to_time_t(tx.timestamp), - tx.gas_price_wei, - tx.nonce, - ) - }) - } - let sql = format!( "insert into sent_payable (\ tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status, retried\ ) values {}", - generate_values(&txs) + Self::generate_sql_values_for_insertion(&txs) ); match self.conn.prepare(&sql).expect("Internal error").execute([]) { @@ -115,14 +115,28 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { use crate::accountant::db_access_objects::sent_payable_dao::{ - SentPayableDao, SentPayableDaoReal, Tx, + SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, }; use crate::database::db_initializer::{ - DbInitializationConfig, DbInitializer, DbInitializerReal, + DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, }; + use crate::database::rusqlite_wrappers::ConnectionWrapperReal; + use crate::database::test_utils::ConnectionWrapperMock; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; + use rusqlite::{Connection, OpenFlags}; use std::time::SystemTime; + fn make_tx() -> Tx { + Tx { + hash: Default::default(), + receiver_address: Default::default(), + amount: 0, + timestamp: SystemTime::now(), + gas_price_wei: 0, + nonce: 0, + } + } + #[test] fn insert_new_records_works() { let home_dir = @@ -130,14 +144,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let tx = Tx { - hash: Default::default(), - receiver_address: Default::default(), - amount: 0, - timestamp: SystemTime::now(), - gas_price_wei: 0, - nonce: 0, - }; + let tx = make_tx(); let subject = SentPayableDaoReal::new(wrapped_conn); let result = subject.insert_new_records(vec![tx]); @@ -151,6 +158,55 @@ mod tests { }; assert_eq!(result, Ok(())); assert_eq!(row_count, 1); + // TODO: GH-608: Add more assertions to verify the inserted data after retrieve functions are implemented + } + + #[test] + #[should_panic(expected = "expected 1 changed rows but got 0")] + fn insert_new_records_can_panic() { + let setup_conn = Connection::open_in_memory().unwrap(); + // Inject a deliberately failing statement into the mocked connection. + let failing_stmt = { + setup_conn + .execute("create table example (id integer)", []) + .unwrap(); + setup_conn.prepare("select id from example").unwrap() + }; + let wrapped_conn = ConnectionWrapperMock::default().prepare_result(Ok(failing_stmt)); + let tx = make_tx(); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + + let _ = subject.insert_new_records(vec![tx]); + } + + #[test] + fn insert_new_records_can_throw_error() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "insert_new_records_can_throw_error", + ); + { + DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + } + let read_only_conn = Connection::open_with_flags( + home_dir.join(DATABASE_FILE), + OpenFlags::SQLITE_OPEN_READ_ONLY, + ) + .unwrap(); + let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); + let tx = make_tx(); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + + let result = subject.insert_new_records(vec![tx]); + + assert_eq!( + result, + Err(SentPayableDaoError::InsertionFailed( + "attempt to write a readonly database".to_string() + )) + ) } #[test] From 954299a7356a2e2128626b7715e7af16367a303e Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 15 Apr 2025 16:38:57 +0530 Subject: [PATCH 08/46] GH-608: introduce builder pattern for the Tx --- .../db_access_objects/sent_payable_dao.rs | 86 ++++++++++++++++--- 1 file changed, 73 insertions(+), 13 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 502db77e4..4c03294a2 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -19,7 +19,12 @@ pub enum SentPayableDaoError { // ErrorMarkFailed(String), } -type TxIdentifiers = HashMap>; +pub enum TxIdentifier { + Id(u64), + NotFound, +} + +type TxIdentifiers = HashMap; pub struct Tx { // GH-608: Perhaps TxReceipt could be a similar structure to be used @@ -122,18 +127,65 @@ mod tests { }; use crate::database::rusqlite_wrappers::ConnectionWrapperReal; use crate::database::test_utils::ConnectionWrapperMock; + use ethereum_types::{Address, H256}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; use std::time::SystemTime; - fn make_tx() -> Tx { - Tx { - hash: Default::default(), - receiver_address: Default::default(), - amount: 0, - timestamp: SystemTime::now(), - gas_price_wei: 0, - nonce: 0, + #[derive(Default)] + pub struct TxBuilder { + hash_opt: Option, + receiver_address_opt: Option
, + amount_opt: Option, + timestamp_opt: Option, + gas_price_wei_opt: Option, + nonce_opt: Option, + } + + impl TxBuilder { + pub fn default() -> Self { + Default::default() + } + + pub fn hash(mut self, hash: H256) -> Self { + self.hash_opt = Some(hash); + self + } + + pub fn receiver_address(mut self, receiver_address: Address) -> Self { + self.receiver_address_opt = Some(receiver_address); + self + } + + pub fn amount(mut self, amount: u128) -> Self { + self.amount_opt = Some(amount); + self + } + + pub fn timestamp(mut self, timestamp: SystemTime) -> Self { + self.timestamp_opt = Some(timestamp); + self + } + + pub fn gas_price_wei(mut self, gas_price_wei: u64) -> Self { + self.gas_price_wei_opt = Some(gas_price_wei); + self + } + + pub fn nonce(mut self, nonce: u32) -> Self { + self.nonce_opt = Some(nonce); + self + } + + pub fn build(self) -> Tx { + Tx { + hash: self.hash_opt.unwrap_or_default(), + receiver_address: self.receiver_address_opt.unwrap_or_default(), + amount: self.amount_opt.unwrap_or_default(), + timestamp: self.timestamp_opt.unwrap_or_else(SystemTime::now), + gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), + nonce: self.nonce_opt.unwrap_or_default(), + } } } @@ -144,7 +196,7 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let tx = make_tx(); + let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(wrapped_conn); let result = subject.insert_new_records(vec![tx]); @@ -173,7 +225,7 @@ mod tests { setup_conn.prepare("select id from example").unwrap() }; let wrapped_conn = ConnectionWrapperMock::default().prepare_result(Ok(failing_stmt)); - let tx = make_tx(); + let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let _ = subject.insert_new_records(vec![tx]); @@ -196,7 +248,7 @@ mod tests { ) .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); - let tx = make_tx(); + let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let result = subject.insert_new_records(vec![tx]); @@ -217,7 +269,15 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); + let hash1 = H256::from_low_u64_le(1); + let hash2 = H256::from_low_u64_le(2); + let hash3 = H256::from_low_u64_le(3); // not present in the database + let tx1 = TxBuilder::default().hash(hash1).build(); + let tx2 = TxBuilder::default().hash(hash2).build(); + subject.insert_new_records(vec![tx1, tx2]).unwrap(); + + let result = subject.get_tx_identifiers(&vec![hash1, hash2, hash3]); - todo!("first you'll have to write tests for the insertion method"); + todo!("write assertions for the returned TxIdentifiers"); } } From 46ab105b388bbc7b77e38c069ce90b1c9f435cdf Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 16 Apr 2025 15:56:58 +0530 Subject: [PATCH 09/46] GH-608: implement get_tx_identifiers --- .../db_access_objects/sent_payable_dao.rs | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 4c03294a2..3423d9a9d 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; +use std::str::FromStr; use std::time::SystemTime; use ethereum_types::H256; use web3::types::Address; +use masq_lib::utils::ExpectValue; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; use crate::accountant::db_access_objects::pending_payable_dao::PendingPayableDaoError; use crate::accountant::db_access_objects::utils::to_time_t; @@ -19,12 +21,7 @@ pub enum SentPayableDaoError { // ErrorMarkFailed(String), } -pub enum TxIdentifier { - Id(u64), - NotFound, -} - -type TxIdentifiers = HashMap; +type TxIdentifiers = HashMap; pub struct Tx { // GH-608: Perhaps TxReceipt could be a similar structure to be used @@ -46,6 +43,7 @@ pub trait SentPayableDao { fn retrieve_pending_txs(&self) -> Vec; fn retrieve_txs_to_retry(&self) -> Vec; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; + // TODO: GH-608: We might not need this fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; } @@ -81,7 +79,26 @@ impl<'a> SentPayableDaoReal<'a> { impl SentPayableDao for SentPayableDaoReal<'_> { fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers { - todo!() + let sql = format!( + "select tx_hash, rowid from sent_payable where tx_hash in ({})", + comma_joined_stringifiable(hashes, |hash| format!("'{:?}'", hash)) + ); + + let mut stmt = self + .conn + .prepare(&sql) + .expect("Failed to prepare SQL statement"); + + stmt.query_map([], |row| { + let tx_hash_str: String = row.get(0).expectv("Failed to get tx_hash"); + let tx_hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); + let row_id: u64 = row.get(1).expectv("Failed to get row_id"); + + Ok((tx_hash, row_id)) + }) + .expect("Failed to execute query") + .filter_map(Result::ok) + .collect() } fn retrieve_pending_txs(&self) -> Vec { @@ -278,6 +295,8 @@ mod tests { let result = subject.get_tx_identifiers(&vec![hash1, hash2, hash3]); - todo!("write assertions for the returned TxIdentifiers"); + assert_eq!(result.get(&hash1), Some(&1u64)); + assert_eq!(result.get(&hash2), Some(&2u64)); + assert_eq!(result.get(&hash3), None); } } From b5e11b5c8bb100d5847138e34c4711b1e81357e7 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 17 Apr 2025 13:17:33 +0530 Subject: [PATCH 10/46] GH-608: implement retrieve_pending_txs() --- .../db_access_objects/sent_payable_dao.rs | 78 ++++++++++++++++--- .../src/accountant/db_access_objects/utils.rs | 2 + 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 3423d9a9d..3e0e6f9a7 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,12 +1,9 @@ use std::collections::HashMap; use std::str::FromStr; -use std::time::SystemTime; use ethereum_types::H256; use web3::types::Address; use masq_lib::utils::ExpectValue; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; -use crate::accountant::db_access_objects::pending_payable_dao::PendingPayableDaoError; -use crate::accountant::db_access_objects::utils::to_time_t; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; use crate::database::rusqlite_wrappers::ConnectionWrapper; @@ -23,14 +20,16 @@ pub enum SentPayableDaoError { type TxIdentifiers = HashMap; +#[derive(Clone, Debug, PartialEq, Eq)] pub struct Tx { - // GH-608: Perhaps TxReceipt could be a similar structure to be used + // TODO: GH-608: Perhaps TxReceipt could be a similar structure to be used hash: H256, receiver_address: Address, amount: u128, - timestamp: SystemTime, + timestamp: i64, gas_price_wei: u64, nonce: u32, + // status: TxStatus, } pub struct StatusChange { @@ -63,13 +62,14 @@ impl<'a> SentPayableDaoReal<'a> { comma_joined_stringifiable(txs, |tx| { let amount_checked = checked_conversion::(tx.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); + // TODO: GH-608: Perhaps you should pick status from the Tx itself format!( "('{:?}', '{:?}', {}, {}, {}, {}, {}, 'Pending', 0)", tx.hash, tx.receiver_address, high_bytes, low_bytes, - to_time_t(tx.timestamp), + tx.timestamp, tx.gas_price_wei, tx.nonce, ) @@ -90,9 +90,9 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .expect("Failed to prepare SQL statement"); stmt.query_map([], |row| { - let tx_hash_str: String = row.get(0).expectv("Failed to get tx_hash"); + let tx_hash_str: String = row.get(0).expectv("tx_hash"); let tx_hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); - let row_id: u64 = row.get(1).expectv("Failed to get row_id"); + let row_id: u64 = row.get(1).expectv("rowid"); Ok((tx_hash, row_id)) }) @@ -102,7 +102,40 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn retrieve_pending_txs(&self) -> Vec { - todo!() + let sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status from sent_payable where status in ('Pending')" + .to_string(); + + let mut stmt = self + .conn + .prepare(&sql) + .expect("Failed to prepare SQL statement"); + + stmt.query_map([], |row| { + let tx_hash_str: String = row.get(0).expectv("tx_hash"); + let hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); + let receiver_address_str: String = row.get(1).expectv("row_id"); + let receiver_address = + Address::from_str(&receiver_address_str[2..]).expect("Failed to parse H160"); + let amount_high_b = row.get(2).expectv("amount_high_b"); + let amount_low_b = row.get(3).expectv("amount_low_b"); + let amount = BigIntDivider::reconstitute(amount_high_b, amount_low_b) as u128; + let timestamp = row.get(4).expectv("timestamp"); + let gas_price_wei = row.get(5).expectv("gas_price_wei"); + let nonce = row.get(6).expectv("nonce"); + + Ok(Tx { + hash, + receiver_address, + amount, + timestamp, + gas_price_wei, + nonce, + }) + }) + .expect("Failed to execute query") + .filter_map(Result::ok) + .collect() } fn retrieve_txs_to_retry(&self) -> Vec { @@ -139,6 +172,7 @@ mod tests { use crate::accountant::db_access_objects::sent_payable_dao::{ SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, }; + use crate::accountant::db_access_objects::utils::now_time_t; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, }; @@ -154,7 +188,7 @@ mod tests { hash_opt: Option, receiver_address_opt: Option
, amount_opt: Option, - timestamp_opt: Option, + timestamp_opt: Option, gas_price_wei_opt: Option, nonce_opt: Option, } @@ -179,7 +213,7 @@ mod tests { self } - pub fn timestamp(mut self, timestamp: SystemTime) -> Self { + pub fn timestamp(mut self, timestamp: i64) -> Self { self.timestamp_opt = Some(timestamp); self } @@ -199,7 +233,7 @@ mod tests { hash: self.hash_opt.unwrap_or_default(), receiver_address: self.receiver_address_opt.unwrap_or_default(), amount: self.amount_opt.unwrap_or_default(), - timestamp: self.timestamp_opt.unwrap_or_else(SystemTime::now), + timestamp: self.timestamp_opt.unwrap_or_else(now_time_t), gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), nonce: self.nonce_opt.unwrap_or_default(), } @@ -299,4 +333,24 @@ mod tests { assert_eq!(result.get(&hash2), Some(&2u64)); assert_eq!(result.get(&hash3), None); } + + #[test] + fn retrieve_pending_txs_works() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "retrieve_pending_txs_works"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let hash1 = H256::from_low_u64_le(1); + let hash2 = H256::from_low_u64_le(2); + let tx1 = TxBuilder::default().hash(hash1).build(); + let tx2 = TxBuilder::default().hash(hash2).build(); + let txs = vec![tx1, tx2]; + subject.insert_new_records(txs.clone()).unwrap(); + + let result = subject.retrieve_pending_txs(); + + assert_eq!(result, txs); + } } diff --git a/node/src/accountant/db_access_objects/utils.rs b/node/src/accountant/db_access_objects/utils.rs index 8b78bb5f4..13fd7e8cd 100644 --- a/node/src/accountant/db_access_objects/utils.rs +++ b/node/src/accountant/db_access_objects/utils.rs @@ -21,6 +21,7 @@ use std::string::ToString; use std::time::Duration; use std::time::SystemTime; +// TODO: GH-608: This should be renamed to to_unix_timestamp and it should be u64 pub fn to_time_t(system_time: SystemTime) -> i64 { match system_time.duration_since(SystemTime::UNIX_EPOCH) { Ok(d) => sign_conversion::(d.as_secs()).expect("MASQNode has expired"), @@ -35,6 +36,7 @@ pub fn now_time_t() -> i64 { to_time_t(SystemTime::now()) } +// TODO: GH-608: This should be renamed to from_unix_timestamp and it should be u64 pub fn from_time_t(time_t: i64) -> SystemTime { let interval = Duration::from_secs(time_t as u64); SystemTime::UNIX_EPOCH + interval From df8e82aab5af56ebedf1407c0603a578b30ed297 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 17 Apr 2025 15:36:25 +0530 Subject: [PATCH 11/46] GH-608: add more assertions for the retrieve_pending_txs() and implement Display for TxStatus --- .../db_access_objects/sent_payable_dao.rs | 57 +++++++++++++++---- .../lower_level_interface_web3.rs | 13 +++++ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 3e0e6f9a7..0f187805e 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -29,7 +29,7 @@ pub struct Tx { timestamp: i64, gas_price_wei: u64, nonce: u32, - // status: TxStatus, + status: TxStatus, } pub struct StatusChange { @@ -64,7 +64,7 @@ impl<'a> SentPayableDaoReal<'a> { let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); // TODO: GH-608: Perhaps you should pick status from the Tx itself format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, 'Pending', 0)", + "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", tx.hash, tx.receiver_address, high_bytes, @@ -72,6 +72,7 @@ impl<'a> SentPayableDaoReal<'a> { tx.timestamp, tx.gas_price_wei, tx.nonce, + tx.status ) }) } @@ -131,6 +132,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { timestamp, gas_price_wei, nonce, + status: TxStatus::Pending, }) }) .expect("Failed to execute query") @@ -181,7 +183,7 @@ mod tests { use ethereum_types::{Address, H256}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; - use std::time::SystemTime; + use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; #[derive(Default)] pub struct TxBuilder { @@ -191,6 +193,7 @@ mod tests { timestamp_opt: Option, gas_price_wei_opt: Option, nonce_opt: Option, + status_opt: Option, } impl TxBuilder { @@ -228,6 +231,11 @@ mod tests { self } + pub fn status(mut self, status: TxStatus) -> Self { + self.status_opt = Some(status); + self + } + pub fn build(self) -> Tx { Tx { hash: self.hash_opt.unwrap_or_default(), @@ -236,6 +244,7 @@ mod tests { timestamp: self.timestamp_opt.unwrap_or_else(now_time_t), gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), nonce: self.nonce_opt.unwrap_or_default(), + status: self.status_opt.unwrap_or(TxStatus::Pending), } } } @@ -261,7 +270,17 @@ mod tests { }; assert_eq!(result, Ok(())); assert_eq!(row_count, 1); - // TODO: GH-608: Add more assertions to verify the inserted data after retrieve functions are implemented + // TODO: GH-608: Add more assertions to verify transactions of all statuses are retreived properly + } + + #[test] + fn insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted() { + todo!("write me"); + } + + #[test] + fn insert_new_records_throws_error_when_txs_with_an_already_present_hash_is_inserted() { + todo!("write me"); } #[test] @@ -342,15 +361,31 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); - let hash1 = H256::from_low_u64_le(1); - let hash2 = H256::from_low_u64_le(2); - let tx1 = TxBuilder::default().hash(hash1).build(); - let tx2 = TxBuilder::default().hash(hash2).build(); - let txs = vec![tx1, tx2]; - subject.insert_new_records(txs.clone()).unwrap(); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Pending) + .build(); + let tx3 = TxBuilder::default() + .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Failed) + .build(); + let tx4 = TxBuilder::default() + .hash(H256::from_low_u64_le(4)) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); + subject + .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3, tx4]) + .unwrap(); let result = subject.retrieve_pending_txs(); - assert_eq!(result, txs); + assert_eq!(result, vec![tx1, tx2]); } } diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs index 5879a47a3..7186779bc 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs @@ -7,6 +7,7 @@ use crate::blockchain::blockchain_interface::lower_level_interface::LowBlockchai use ethereum_types::{H256, U256, U64}; use futures::Future; use serde_json::Value; +use std::fmt::Display; use web3::contract::{Contract, Options}; use web3::transports::{Batch, Http}; use web3::types::{Address, BlockNumber, Filter, Log, TransactionReceipt}; @@ -25,6 +26,18 @@ pub enum TxStatus { Succeeded(TransactionBlock), } +impl Display for TxStatus { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TxStatus::Failed => write!(f, "Failed"), + TxStatus::Pending => write!(f, "Pending"), + TxStatus::Succeeded(block) => { + write!(f, "Succeeded({},{})", block.block_number, block.block_hash) + } + } + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct TxReceipt { pub transaction_hash: H256, From df0ea02605ff09b9327534878ce9f97df227b7f0 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 17 Apr 2025 15:45:34 +0530 Subject: [PATCH 12/46] GH-608: rename time_t in fn names to unix_timestamp --- .../db_access_objects/payable_dao.rs | 77 +++++----- .../db_access_objects/pending_payable_dao.rs | 26 ++-- .../db_access_objects/receivable_dao.rs | 141 +++++++++--------- .../db_access_objects/sent_payable_dao.rs | 4 +- .../src/accountant/db_access_objects/utils.rs | 30 ++-- node/src/accountant/mod.rs | 31 ++-- node/src/accountant/scanners/mod.rs | 30 ++-- .../src/accountant/scanners/scanners_utils.rs | 18 +-- node/src/accountant/test_utils.rs | 26 ++-- node/src/blockchain/blockchain_bridge.rs | 8 +- .../blockchain_interface_web3/utils.rs | 6 +- .../migrations/migration_4_to_5.rs | 8 +- node/tests/financials_test.rs | 8 +- 13 files changed, 210 insertions(+), 203 deletions(-) diff --git a/node/src/accountant/db_access_objects/payable_dao.rs b/node/src/accountant/db_access_objects/payable_dao.rs index 88897281b..c7d438a41 100644 --- a/node/src/accountant/db_access_objects/payable_dao.rs +++ b/node/src/accountant/db_access_objects/payable_dao.rs @@ -7,7 +7,7 @@ use crate::accountant::db_big_integer::big_int_db_processor::{BigIntDbProcessor, use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::accountant::db_access_objects::utils; use crate::accountant::db_access_objects::utils::{ - sum_i128_values_from_table, to_time_t, AssemblerFeeder, CustomQuery, DaoFactoryReal, + sum_i128_values_from_table, to_unix_timestamp, AssemblerFeeder, CustomQuery, DaoFactoryReal, RangeStmConfig, TopStmConfig, VigilantRusqliteFlatten, }; use crate::accountant::db_access_objects::payable_dao::mark_pending_payable_associated_functions::{ @@ -100,7 +100,7 @@ impl PayableDao for PayableDaoReal { let update_clause_with_compensated_overflow = "update payable set \ balance_high_b = :balance_high_b, balance_low_b = :balance_low_b where wallet_address = :wallet"; - let last_paid_timestamp = to_time_t(timestamp); + let last_paid_timestamp = to_unix_timestamp(timestamp); let params = SQLParamsBuilder::default() .key(WalletAddress(wallet)) .wei_change(WeiChange::new( @@ -158,7 +158,7 @@ impl PayableDao for PayableDaoReal { pending_payable_rowid = null where pending_payable_rowid = :rowid"; let i64_rowid = checked_conversion::(pending_payable_fingerprint.rowid); - let last_paid = to_time_t(pending_payable_fingerprint.timestamp); + let last_paid = to_unix_timestamp(pending_payable_fingerprint.timestamp); let params = SQLParamsBuilder::default() .key( PendingPayableRowid(&i64_rowid)) .wei_change(WeiChange::new( "balance", pending_payable_fingerprint.amount, WeiChangeDirection::Subtraction)) @@ -196,7 +196,7 @@ impl PayableDao for PayableDaoReal { balance_wei: checked_conversion::(BigIntDivider::reconstitute( high_b, low_b, )), - last_paid_timestamp: utils::from_time_t(last_paid_timestamp), + last_paid_timestamp: utils::from_unix_timestamp(last_paid_timestamp), pending_payable_opt: None, }) } @@ -282,7 +282,7 @@ impl PayableDao for PayableDaoReal { balance_wei: checked_conversion::(BigIntDivider::reconstitute( high_bytes, low_bytes, )), - last_paid_timestamp: utils::from_time_t(last_paid_timestamp), + last_paid_timestamp: utils::from_unix_timestamp(last_paid_timestamp), pending_payable_opt: match rowid { Some(rowid) => Some(PendingPayableId::new( u64::try_from(rowid).unwrap(), @@ -338,7 +338,7 @@ impl PayableDaoReal { balance_wei: checked_conversion::(BigIntDivider::reconstitute( high_bytes, low_bytes, )), - last_paid_timestamp: utils::from_time_t(last_paid_timestamp), + last_paid_timestamp: utils::from_unix_timestamp(last_paid_timestamp), pending_payable_opt: rowid_opt.map(|rowid| { let hash_str = hash_opt.expect("database corrupt; missing hash but existing rowid"); @@ -541,7 +541,7 @@ mod mark_pending_payable_associated_functions { #[cfg(test)] mod tests { use super::*; - use crate::accountant::db_access_objects::utils::{from_time_t, now_time_t, to_time_t}; + use crate::accountant::db_access_objects::utils::{from_unix_timestamp, current_unix_timestamp, to_unix_timestamp}; use crate::accountant::gwei_to_wei; use crate::accountant::db_access_objects::payable_dao::mark_pending_payable_associated_functions::explanatory_extension; use crate::accountant::test_utils::{assert_account_creation_fn_fails_on_finding_wrong_columns_and_value_types, make_pending_payable_fingerprint, trick_rusqlite_with_read_only_conn}; @@ -577,7 +577,10 @@ mod tests { let status = subject.account_status(&wallet).unwrap(); assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, 1234); - assert_eq!(to_time_t(status.last_paid_timestamp), to_time_t(now)); + assert_eq!( + to_unix_timestamp(status.last_paid_timestamp), + to_unix_timestamp(now) + ); } #[test] @@ -616,8 +619,8 @@ mod tests { assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, expected_balance); assert_eq!( - to_time_t(status.last_paid_timestamp), - to_time_t(SystemTime::UNIX_EPOCH) + to_unix_timestamp(status.last_paid_timestamp), + to_unix_timestamp(SystemTime::UNIX_EPOCH) ); }; assert_account(wallet, initial_value + balance_change); @@ -653,8 +656,8 @@ mod tests { assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, initial_value + balance_change); assert_eq!( - to_time_t(status.last_paid_timestamp), - to_time_t(SystemTime::UNIX_EPOCH) + to_unix_timestamp(status.last_paid_timestamp), + to_unix_timestamp(SystemTime::UNIX_EPOCH) ); } @@ -746,13 +749,13 @@ mod tests { PayableAccount { wallet: wallet_0, balance_wei: u128::try_from(BigIntDivider::reconstitute(12345, 1)).unwrap(), - last_paid_timestamp: from_time_t(45678), + last_paid_timestamp: from_unix_timestamp(45678), pending_payable_opt: None, }, PayableAccount { wallet: wallet_1, balance_wei: u128::try_from(BigIntDivider::reconstitute(0, i64::MAX)).unwrap(), - last_paid_timestamp: from_time_t(150_000_000), + last_paid_timestamp: from_unix_timestamp(150_000_000), pending_payable_opt: Some(PendingPayableId::new( pending_payable_rowid_1, make_tx_hash(0) @@ -762,7 +765,7 @@ mod tests { PayableAccount { wallet: wallet_2, balance_wei: u128::try_from(BigIntDivider::reconstitute(3, 0)).unwrap(), - last_paid_timestamp: from_time_t(151_000_000), + last_paid_timestamp: from_unix_timestamp(151_000_000), pending_payable_opt: Some(PendingPayableId::new( pending_payable_rowid_2, make_tx_hash(0) @@ -907,12 +910,12 @@ mod tests { let hash_1 = make_tx_hash(12345); let rowid_1 = 789; let previous_timestamp_1_s = 190_000_000; - let new_payable_timestamp_1 = from_time_t(199_000_000); + let new_payable_timestamp_1 = from_unix_timestamp(199_000_000); let wallet_1 = make_wallet("bobble"); let hash_2 = make_tx_hash(54321); let rowid_2 = 792; let previous_timestamp_2_s = 187_100_000; - let new_payable_timestamp_2 = from_time_t(191_333_000); + let new_payable_timestamp_2 = from_unix_timestamp(191_333_000); let wallet_2 = make_wallet("booble bobble"); { insert_payable_record_fn( @@ -946,8 +949,8 @@ mod tests { amount: balance_change_2, process_error: None, }; - let previous_timestamp_1 = from_time_t(previous_timestamp_1_s); - let previous_timestamp_2 = from_time_t(previous_timestamp_2_s); + let previous_timestamp_1 = from_unix_timestamp(previous_timestamp_1_s); + let previous_timestamp_2 = from_unix_timestamp(previous_timestamp_2_s); TestSetupValuesHolder { fingerprint_1, fingerprint_2, @@ -1203,13 +1206,13 @@ mod tests { PayableAccount { wallet: make_wallet("foobar"), balance_wei: 1234567890123456 as u128, - last_paid_timestamp: from_time_t(111_111_111), + last_paid_timestamp: from_unix_timestamp(111_111_111), pending_payable_opt: None }, PayableAccount { wallet: make_wallet("barfoo"), balance_wei: 1234567890123456 as u128, - last_paid_timestamp: from_time_t(111_111_111), + last_paid_timestamp: from_unix_timestamp(111_111_111), pending_payable_opt: None }, ] @@ -1304,7 +1307,7 @@ mod tests { //Accounts of balances smaller than one gwei don't qualify. //Two accounts differ only in debt's age but not balance which allows to check doubled ordering, //here by balance and then by age. - let now = now_time_t(); + let now = current_unix_timestamp(); let main_test_setup = accounts_for_tests_of_top_records(now); let subject = custom_query_test_body_for_payable( "custom_query_in_top_records_mode_with_default_ordering", @@ -1324,13 +1327,13 @@ mod tests { PayableAccount { wallet: Wallet::new("0x2222222222222222222222222222222222222222"), balance_wei: 7_562_000_300_000, - last_paid_timestamp: from_time_t(now - 86_001), + last_paid_timestamp: from_unix_timestamp(now - 86_001), pending_payable_opt: None }, PayableAccount { wallet: Wallet::new("0x5555555555555555555555555555555555555555"), balance_wei: 10_000_000_100, - last_paid_timestamp: from_time_t(now - 86_401), + last_paid_timestamp: from_unix_timestamp(now - 86_401), pending_payable_opt: Some(PendingPayableId::new( 1, H256::from_str( @@ -1342,7 +1345,7 @@ mod tests { PayableAccount { wallet: Wallet::new("0x4444444444444444444444444444444444444444"), balance_wei: 10_000_000_100, - last_paid_timestamp: from_time_t(now - 86_300), + last_paid_timestamp: from_unix_timestamp(now - 86_300), pending_payable_opt: None }, ] @@ -1354,7 +1357,7 @@ mod tests { //Accounts of balances smaller than one gwei don't qualify. //Two accounts differ only in balance but not in the debt's age which allows to check doubled ordering, //here by age and then by balance. - let now = now_time_t(); + let now = current_unix_timestamp(); let main_test_setup = accounts_for_tests_of_top_records(now); let subject = custom_query_test_body_for_payable( "custom_query_in_top_records_mode_ordered_by_age", @@ -1374,7 +1377,7 @@ mod tests { PayableAccount { wallet: Wallet::new("0x5555555555555555555555555555555555555555"), balance_wei: 10_000_000_100, - last_paid_timestamp: from_time_t(now - 86_401), + last_paid_timestamp: from_unix_timestamp(now - 86_401), pending_payable_opt: Some(PendingPayableId::new( 1, H256::from_str( @@ -1386,13 +1389,13 @@ mod tests { PayableAccount { wallet: Wallet::new("0x1111111111111111111111111111111111111111"), balance_wei: 1_000_000_002, - last_paid_timestamp: from_time_t(now - 86_401), + last_paid_timestamp: from_unix_timestamp(now - 86_401), pending_payable_opt: None }, PayableAccount { wallet: Wallet::new("0x4444444444444444444444444444444444444444"), balance_wei: 10_000_000_100, - last_paid_timestamp: from_time_t(now - 86_300), + last_paid_timestamp: from_unix_timestamp(now - 86_300), pending_payable_opt: None }, ] @@ -1422,7 +1425,7 @@ mod tests { fn custom_query_in_range_mode() { //Two accounts differ only in debt's age but not balance which allows to check doubled ordering, //by balance and then by age. - let now = now_time_t(); + let now = current_unix_timestamp(); let main_setup = |conn: &dyn ConnectionWrapper, insert: InsertPayableHelperFn| { insert( conn, @@ -1482,7 +1485,7 @@ mod tests { max_age_s: 200000, min_amount_gwei: 500_000_000, max_amount_gwei: 35_000_000_000, - timestamp: from_time_t(now), + timestamp: from_unix_timestamp(now), }) .unwrap(); @@ -1492,19 +1495,19 @@ mod tests { PayableAccount { wallet: Wallet::new("0x7777777777777777777777777777777777777777"), balance_wei: gwei_to_wei(2_500_647_000_u32), - last_paid_timestamp: from_time_t(now - 80_333), + last_paid_timestamp: from_unix_timestamp(now - 80_333), pending_payable_opt: None }, PayableAccount { wallet: Wallet::new("0x6666666666666666666666666666666666666666"), balance_wei: gwei_to_wei(1_800_456_000_u32), - last_paid_timestamp: from_time_t(now - 100_401), + last_paid_timestamp: from_unix_timestamp(now - 100_401), pending_payable_opt: None }, PayableAccount { wallet: Wallet::new("0x2222222222222222222222222222222222222222"), balance_wei: gwei_to_wei(1_800_456_000_u32), - last_paid_timestamp: from_time_t(now - 55_120), + last_paid_timestamp: from_unix_timestamp(now - 55_120), pending_payable_opt: Some(PendingPayableId::new( 1, H256::from_str( @@ -1519,7 +1522,7 @@ mod tests { #[test] fn range_query_does_not_display_values_from_below_1_gwei() { - let now = now_time_t(); + let now = current_unix_timestamp(); let timestamp_1 = now - 11_001; let timestamp_2 = now - 5000; let main_setup = |conn: &dyn ConnectionWrapper, insert: InsertPayableHelperFn| { @@ -1558,7 +1561,7 @@ mod tests { vec![PayableAccount { wallet: Wallet::new("0x2222222222222222222222222222222222222222"), balance_wei: 30_000_300_000, - last_paid_timestamp: from_time_t(timestamp_2), + last_paid_timestamp: from_unix_timestamp(timestamp_2), pending_payable_opt: None },] ) @@ -1570,7 +1573,7 @@ mod tests { let conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let timestamp = utils::now_time_t(); + let timestamp = utils::current_unix_timestamp(); insert_payable_record_fn( &*conn, "0x1111111111111111111111111111111111111111", diff --git a/node/src/accountant/db_access_objects/pending_payable_dao.rs b/node/src/accountant/db_access_objects/pending_payable_dao.rs index 67c779ce0..e555fcc9a 100644 --- a/node/src/accountant/db_access_objects/pending_payable_dao.rs +++ b/node/src/accountant/db_access_objects/pending_payable_dao.rs @@ -1,7 +1,7 @@ // Copyright (c) 2019, MASQ (https://masq.ai) and/or its affiliates. All rights reserved. use crate::accountant::db_access_objects::utils::{ - from_time_t, to_time_t, DaoFactoryReal, VigilantRusqliteFlatten, + from_unix_timestamp, to_unix_timestamp, DaoFactoryReal, VigilantRusqliteFlatten, }; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; @@ -104,7 +104,7 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { let attempt: u16 = Self::get_with_expect(row, 5); Ok(PendingPayableFingerprint { rowid, - timestamp: from_time_t(timestamp), + timestamp: from_unix_timestamp(timestamp), hash: H256::from_str(&transaction_hash[2..]).unwrap_or_else(|e| { panic!( "Invalid hash format (\"{}\": {:?}) - database corrupt", @@ -133,7 +133,7 @@ impl PendingPayableDao for PendingPayableDaoReal<'_> { hashes_and_amounts: &[HashAndAmount], batch_wide_timestamp: SystemTime, ) -> String { - let time_t = to_time_t(batch_wide_timestamp); + let time_t = to_unix_timestamp(batch_wide_timestamp); comma_joined_stringifiable(hashes_and_amounts, |hash_and_amount| { let amount_checked = checked_conversion::(hash_and_amount.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); @@ -275,7 +275,7 @@ mod tests { use crate::accountant::db_access_objects::pending_payable_dao::{ PendingPayableDao, PendingPayableDaoError, PendingPayableDaoReal, }; - use crate::accountant::db_access_objects::utils::from_time_t; + use crate::accountant::db_access_objects::utils::from_unix_timestamp; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_bridge::PendingPayableFingerprint; use crate::blockchain::blockchain_interface::blockchain_interface_web3::HashAndAmount; @@ -304,7 +304,7 @@ mod tests { let amount_1 = 55556; let hash_2 = make_tx_hash(6789); let amount_2 = 44445; - let batch_wide_timestamp = from_time_t(200_000_000); + let batch_wide_timestamp = from_unix_timestamp(200_000_000); let subject = PendingPayableDaoReal::new(wrapped_conn); let hash_and_amount_1 = HashAndAmount { hash: hash_1, @@ -365,7 +365,7 @@ mod tests { let wrapped_conn = ConnectionWrapperReal::new(conn_read_only); let hash = make_tx_hash(45466); let amount = 55556; - let timestamp = from_time_t(200_000_000); + let timestamp = from_unix_timestamp(200_000_000); let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash, amount }; @@ -394,7 +394,7 @@ mod tests { let wrapped_conn = ConnectionWrapperMock::default().prepare_result(Ok(statement)); let hash_1 = make_tx_hash(4546); let amount_1 = 55556; - let batch_wide_timestamp = from_time_t(200_000_000); + let batch_wide_timestamp = from_unix_timestamp(200_000_000); let subject = PendingPayableDaoReal::new(Box::new(wrapped_conn)); let hash_and_amount = HashAndAmount { hash: hash_1, @@ -414,7 +414,7 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = PendingPayableDaoReal::new(wrapped_conn); - let timestamp = from_time_t(195_000_000); + let timestamp = from_unix_timestamp(195_000_000); // use full range tx hashes because SqLite has tendencies to see the value as a hex and convert it to an integer, // then complain about its excessive size if supplied in unquoted strings let hash_1 = @@ -510,7 +510,7 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = PendingPayableDaoReal::new(wrapped_conn); - let batch_wide_timestamp = from_time_t(195_000_000); + let batch_wide_timestamp = from_unix_timestamp(195_000_000); let hash_1 = make_tx_hash(11119); let amount_1 = 787; let hash_2 = make_tx_hash(10000); @@ -568,7 +568,7 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = PendingPayableDaoReal::new(wrapped_conn); - let timestamp = from_time_t(198_000_000); + let timestamp = from_unix_timestamp(198_000_000); let hash = make_tx_hash(10000); let amount = 333; let hash_and_amount_1 = HashAndAmount { @@ -750,7 +750,7 @@ mod tests { hash: hash_3, amount: 3344, }; - let timestamp = from_time_t(190_000_000); + let timestamp = from_unix_timestamp(190_000_000); let subject = PendingPayableDaoReal::new(conn); { subject @@ -842,7 +842,7 @@ mod tests { hash: hash_2, amount: amount_2, }; - let timestamp = from_time_t(190_000_000); + let timestamp = from_unix_timestamp(190_000_000); let subject = PendingPayableDaoReal::new(conn); { subject @@ -868,7 +868,7 @@ mod tests { let process_error: Option = row.get(6).unwrap(); Ok(PendingPayableFingerprint { rowid, - timestamp: from_time_t(timestamp), + timestamp: from_unix_timestamp(timestamp), hash: H256::from_str(&transaction_hash[2..]).unwrap(), attempt, amount: checked_conversion::(BigIntDivider::reconstitute( diff --git a/node/src/accountant/db_access_objects/receivable_dao.rs b/node/src/accountant/db_access_objects/receivable_dao.rs index 9b71a3939..ad8f52462 100644 --- a/node/src/accountant/db_access_objects/receivable_dao.rs +++ b/node/src/accountant/db_access_objects/receivable_dao.rs @@ -4,7 +4,7 @@ use crate::accountant::checked_conversion; use crate::accountant::db_access_objects::receivable_dao::ReceivableDaoError::RusqliteError; use crate::accountant::db_access_objects::utils; use crate::accountant::db_access_objects::utils::{ - sum_i128_values_from_table, to_time_t, AssemblerFeeder, CustomQuery, DaoFactoryReal, + sum_i128_values_from_table, to_unix_timestamp, AssemblerFeeder, CustomQuery, DaoFactoryReal, RangeStmConfig, ThresholdUtils, TopStmConfig, VigilantRusqliteFlatten, }; use crate::accountant::db_big_integer::big_int_db_processor::KeyVariants::WalletAddress; @@ -120,7 +120,7 @@ impl ReceivableDao for ReceivableDaoReal { let update_clause_with_compensated_overflow = "update receivable set balance_high_b = :balance_high_b, balance_low_b = :balance_low_b \ where wallet_address = :wallet"; - let last_received_timestamp = to_time_t(timestamp); + let last_received_timestamp = to_unix_timestamp(timestamp); let params = SQLParamsBuilder::default() .key(WalletAddress(wallet)) .wei_change(WeiChange::new( @@ -216,7 +216,7 @@ impl ReceivableDao for ReceivableDaoReal { named_params! { ":debt_threshold": checked_conversion::(payment_thresholds.debt_threshold_gwei), ":slope": slope, - ":sugg_and_grace": payment_thresholds.sugg_and_grace(to_time_t(now)), + ":sugg_and_grace": payment_thresholds.sugg_and_grace(to_unix_timestamp(now)), ":permanent_debt_allowed_high_b": permanent_debt_allowed_high_b, ":permanent_debt_allowed_low_b": permanent_debt_allowed_low_b }, @@ -337,7 +337,7 @@ impl ReceivableDaoReal { where wallet_address = :wallet"; match received_payments.iter().try_for_each(|received_payment| { - let last_received_timestamp = to_time_t(timestamp); + let last_received_timestamp = to_unix_timestamp(timestamp); let params = SQLParamsBuilder::default() .key(WalletAddress(&received_payment.from)) .wei_change(WeiChange::new( @@ -414,7 +414,7 @@ impl ReceivableDaoReal { Ok(ReceivableAccount { wallet, balance_wei: BigIntDivider::reconstitute(high_bytes, low_bytes), - last_received_timestamp: utils::from_time_t(last_received_timestamp), + last_received_timestamp: utils::from_unix_timestamp(last_received_timestamp), }) } e => panic!( @@ -493,7 +493,7 @@ impl TableNameDAO for ReceivableDaoReal { mod tests { use super::*; use crate::accountant::db_access_objects::utils::{ - from_time_t, now_time_t, to_time_t, CustomQuery, + current_unix_timestamp, from_unix_timestamp, to_unix_timestamp, CustomQuery, }; use crate::accountant::gwei_to_wei; use crate::accountant::test_utils::{ @@ -609,8 +609,8 @@ mod tests { "receivable_dao", "more_money_receivable_works_for_new_address", ); - let payment_time_t = to_time_t(SystemTime::now()) - 1111; - let payment_time = from_time_t(payment_time_t); + let payment_time_t = to_unix_timestamp(SystemTime::now()) - 1111; + let payment_time = from_unix_timestamp(payment_time_t); let wallet = make_wallet("booga"); let subject = ReceivableDaoReal::new( DbInitializerReal::default() @@ -625,7 +625,10 @@ mod tests { let status = subject.account_status(&wallet).unwrap(); assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, 1234); - assert_eq!(to_time_t(status.last_received_timestamp), payment_time_t); + assert_eq!( + to_unix_timestamp(status.last_received_timestamp), + payment_time_t + ); } #[test] @@ -661,8 +664,8 @@ mod tests { assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, expected_balance); assert_eq!( - to_time_t(status.last_received_timestamp), - to_time_t(SystemTime::UNIX_EPOCH) + to_unix_timestamp(status.last_received_timestamp), + to_unix_timestamp(SystemTime::UNIX_EPOCH) ); }; assert_account(wallet, 1234 + 2345); @@ -695,8 +698,8 @@ mod tests { assert_eq!(status.wallet, wallet); assert_eq!(status.balance_wei, 1234 + i64::MAX as i128); assert_eq!( - to_time_t(status.last_received_timestamp), - to_time_t(SystemTime::UNIX_EPOCH) + to_unix_timestamp(status.last_received_timestamp), + to_unix_timestamp(SystemTime::UNIX_EPOCH) ); } @@ -812,15 +815,15 @@ mod tests { assert_eq!(status1.wallet, debtor1); assert_eq!(status1.balance_wei, first_expected_result); assert_eq!( - to_time_t(status1.last_received_timestamp), - to_time_t(payment_time) + to_unix_timestamp(status1.last_received_timestamp), + to_unix_timestamp(payment_time) ); let status2 = subject.account_status(&debtor2).unwrap(); assert_eq!(status2.wallet, debtor2); assert_eq!(status2.balance_wei, second_expected_result); assert_eq!( - to_time_t(status2.last_received_timestamp), - to_time_t(payment_time) + to_unix_timestamp(status2.last_received_timestamp), + to_unix_timestamp(payment_time) ); } @@ -887,8 +890,8 @@ mod tests { first_initial_balance as i128 - 1111 ); assert_eq!( - to_time_t(actual_record_1.last_received_timestamp), - to_time_t(time_of_change) + to_unix_timestamp(actual_record_1.last_received_timestamp), + to_unix_timestamp(time_of_change) ); let actual_record_2 = subject.account_status(&unknown_wallet); assert!(actual_record_2.is_none()); @@ -899,8 +902,8 @@ mod tests { second_initial_balance as i128 - 9999 ); assert_eq!( - to_time_t(actual_record_3.last_received_timestamp), - to_time_t(time_of_change) + to_unix_timestamp(actual_record_3.last_received_timestamp), + to_unix_timestamp(time_of_change) ); let log_handler = TestLogHandler::new(); log_handler.exists_log_containing(&format!( @@ -1202,37 +1205,37 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = now_time_t(); + let now = current_unix_timestamp(); let mut not_delinquent_inside_grace_period = make_receivable_account(1234, false); not_delinquent_inside_grace_period.balance_wei = gwei_to_wei(payment_thresholds.debt_threshold_gwei + 1); not_delinquent_inside_grace_period.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) + 2); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) + 2); let mut not_delinquent_after_grace_below_slope = make_receivable_account(2345, false); not_delinquent_after_grace_below_slope.balance_wei = gwei_to_wei(payment_thresholds.debt_threshold_gwei - 2); not_delinquent_after_grace_below_slope.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 1); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 1); let mut delinquent_above_slope_after_grace = make_receivable_account(3456, true); delinquent_above_slope_after_grace.balance_wei = gwei_to_wei(payment_thresholds.debt_threshold_gwei - 1); delinquent_above_slope_after_grace.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 2); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 2); let mut not_delinquent_below_slope_before_stop = make_receivable_account(4567, false); not_delinquent_below_slope_before_stop.balance_wei = gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 1); not_delinquent_below_slope_before_stop.last_received_timestamp = - from_time_t(payment_thresholds.sugg_thru_decreasing(now) + 2); + from_unix_timestamp(payment_thresholds.sugg_thru_decreasing(now) + 2); let mut delinquent_above_slope_before_stop = make_receivable_account(5678, true); delinquent_above_slope_before_stop.balance_wei = gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 2); delinquent_above_slope_before_stop.last_received_timestamp = - from_time_t(payment_thresholds.sugg_thru_decreasing(now) + 1); + from_unix_timestamp(payment_thresholds.sugg_thru_decreasing(now) + 1); let mut not_delinquent_above_slope_after_stop = make_receivable_account(6789, false); not_delinquent_above_slope_after_stop.balance_wei = gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei - 1); not_delinquent_above_slope_after_stop.last_received_timestamp = - from_time_t(payment_thresholds.sugg_thru_decreasing(now) - 2); + from_unix_timestamp(payment_thresholds.sugg_thru_decreasing(now) - 2); let home_dir = ensure_node_home_directory_exists("accountant", "new_delinquencies"); let conn = make_connection_with_our_defined_sqlite_functions(&home_dir); add_receivable_account(&conn, ¬_delinquent_inside_grace_period); @@ -1243,7 +1246,7 @@ mod tests { add_receivable_account(&conn, ¬_delinquent_above_slope_after_stop); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert_contains(&result, &delinquent_above_slope_after_grace); assert_contains(&result, &delinquent_above_slope_before_stop); @@ -1260,15 +1263,15 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = now_time_t(); + let now = current_unix_timestamp(); let mut not_delinquent = make_receivable_account(1234, false); not_delinquent.balance_wei = gwei_to_wei(105); not_delinquent.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 25); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 25); let mut delinquent = make_receivable_account(2345, true); delinquent.balance_wei = gwei_to_wei(105); delinquent.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 75); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 75); let home_dir = ensure_node_home_directory_exists("accountant", "new_delinquencies_shallow_slope"); let conn = make_connection_with_our_defined_sqlite_functions(&home_dir); @@ -1276,7 +1279,7 @@ mod tests { add_receivable_account(&conn, &delinquent); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert_contains(&result, &delinquent); assert_eq!(result.len(), 1); @@ -1292,15 +1295,15 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = now_time_t(); + let now = current_unix_timestamp(); let mut not_delinquent = make_receivable_account(1234, false); not_delinquent.balance_wei = gwei_to_wei(600); not_delinquent.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 25); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 25); let mut delinquent = make_receivable_account(2345, true); delinquent.balance_wei = gwei_to_wei(600); delinquent.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 75); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 75); let home_dir = ensure_node_home_directory_exists("accountant", "new_delinquencies_steep_slope"); let conn = make_connection_with_our_defined_sqlite_functions(&home_dir); @@ -1308,7 +1311,7 @@ mod tests { add_receivable_account(&conn, &delinquent); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert_contains(&result, &delinquent); assert_eq!(result.len(), 1); @@ -1324,15 +1327,15 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = now_time_t(); + let now = current_unix_timestamp(); let mut existing_delinquency = make_receivable_account(1234, true); existing_delinquency.balance_wei = gwei_to_wei(250); existing_delinquency.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 1); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 1); let mut new_delinquency = make_receivable_account(2345, true); new_delinquency.balance_wei = gwei_to_wei(250); new_delinquency.last_received_timestamp = - from_time_t(payment_thresholds.sugg_and_grace(now) - 1); + from_unix_timestamp(payment_thresholds.sugg_and_grace(now) - 1); let home_dir = ensure_node_home_directory_exists( "receivable_dao", "new_delinquencies_does_not_find_existing_delinquencies", @@ -1343,7 +1346,7 @@ mod tests { add_banned_account(&conn, &existing_delinquency); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert_contains(&result, &new_delinquency); assert_eq!(result.len(), 1); @@ -1359,7 +1362,7 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = now_time_t(); + let now = current_unix_timestamp(); let home_dir = ensure_node_home_directory_exists( "receivable_dao", "new_delinquencies_work_for_still_empty_tables", @@ -1367,7 +1370,7 @@ mod tests { let conn = make_connection_with_our_defined_sqlite_functions(&home_dir); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert!(result.is_empty()) } @@ -1387,24 +1390,24 @@ mod tests { threshold_interval_sec: 100, unban_below_gwei: 0, }; - let now = to_time_t(SystemTime::now()); + let now = to_unix_timestamp(SystemTime::now()); let sugg_and_grace = payment_thresholds.sugg_and_grace(now); let too_young_new_delinquency = ReceivableAccount { wallet: make_wallet("abc123"), balance_wei: 123_456_789_101_112, - last_received_timestamp: from_time_t(sugg_and_grace + 1), + last_received_timestamp: from_unix_timestamp(sugg_and_grace + 1), }; let ok_new_delinquency = ReceivableAccount { wallet: make_wallet("aaa999"), balance_wei: 123_456_789_101_112, - last_received_timestamp: from_time_t(sugg_and_grace - 1), + last_received_timestamp: from_unix_timestamp(sugg_and_grace - 1), }; let conn = make_connection_with_our_defined_sqlite_functions(&home_dir); add_receivable_account(&conn, &too_young_new_delinquency); add_receivable_account(&conn, &ok_new_delinquency.clone()); let subject = ReceivableDaoReal::new(conn); - let result = subject.new_delinquencies(from_time_t(now), &payment_thresholds); + let result = subject.new_delinquencies(from_unix_timestamp(now), &payment_thresholds); assert_eq!(result, vec![ok_new_delinquency]) } @@ -1535,7 +1538,7 @@ mod tests { #[test] fn custom_query_in_top_records_mode_default_ordering() { - let now = now_time_t(); + let now = current_unix_timestamp(); let main_test_setup = common_setup_of_accounts_for_tests_of_top_records(now); let subject = custom_query_test_body_for_receivable( "custom_query_in_top_records_mode_default_ordering", @@ -1555,17 +1558,17 @@ mod tests { ReceivableAccount { wallet: Wallet::new("0x5555555555555555555555555555555555555555"), balance_wei: 32_000_000_200, - last_received_timestamp: from_time_t(now - 86_480), + last_received_timestamp: from_unix_timestamp(now - 86_480), }, ReceivableAccount { wallet: Wallet::new("0x2222222222222222222222222222222222222222"), balance_wei: 1_000_000_001, - last_received_timestamp: from_time_t(now - 222_000), + last_received_timestamp: from_unix_timestamp(now - 222_000), }, ReceivableAccount { wallet: Wallet::new("0x1111111111111111111111111111111111111111"), balance_wei: 1_000_000_001, - last_received_timestamp: from_time_t(now - 86_480), + last_received_timestamp: from_unix_timestamp(now - 86_480), }, ] ); @@ -1573,7 +1576,7 @@ mod tests { #[test] fn custom_query_in_top_records_mode_ordered_by_age() { - let now = now_time_t(); + let now = current_unix_timestamp(); let main_test_setup = common_setup_of_accounts_for_tests_of_top_records(now); let subject = custom_query_test_body_for_receivable( "custom_query_in_top_records_mode_ordered_by_age", @@ -1593,17 +1596,17 @@ mod tests { ReceivableAccount { wallet: Wallet::new("0x2222222222222222222222222222222222222222"), balance_wei: 1_000_000_001, - last_received_timestamp: from_time_t(now - 222_000), + last_received_timestamp: from_unix_timestamp(now - 222_000), }, ReceivableAccount { wallet: Wallet::new("0x5555555555555555555555555555555555555555"), balance_wei: 32_000_000_200, - last_received_timestamp: from_time_t(now - 86_480), + last_received_timestamp: from_unix_timestamp(now - 86_480), }, ReceivableAccount { wallet: Wallet::new("0x1111111111111111111111111111111111111111"), balance_wei: 1_000_000_001, - last_received_timestamp: from_time_t(now - 86_480), + last_received_timestamp: from_unix_timestamp(now - 86_480), }, ] ); @@ -1632,7 +1635,7 @@ mod tests { fn custom_query_in_range_mode() { //Two accounts differ only in debt's age but not balance which allows to check doubled ordering, //by balance and then by age. - let now = now_time_t(); + let now = current_unix_timestamp(); let main_test_setup = |conn: &dyn ConnectionWrapper, insert: InsertReceivableHelperFn| { insert( conn, @@ -1692,7 +1695,7 @@ mod tests { max_age_s: 99000, min_amount_gwei: -560000, max_amount_gwei: 1_100_000_000, - timestamp: from_time_t(now), + timestamp: from_unix_timestamp(now), }) .unwrap(); @@ -1702,22 +1705,22 @@ mod tests { ReceivableAccount { wallet: Wallet::new("0x6666666666666666666666666666666666666666"), balance_wei: gwei_to_wei(1_050_444_230), - last_received_timestamp: from_time_t(now - 66_244), + last_received_timestamp: from_unix_timestamp(now - 66_244), }, ReceivableAccount { wallet: Wallet::new("0x5555555555555555555555555555555555555555"), balance_wei: gwei_to_wei(1_000_000_230), - last_received_timestamp: from_time_t(now - 86_000), + last_received_timestamp: from_unix_timestamp(now - 86_000), }, ReceivableAccount { wallet: Wallet::new("0x3333333333333333333333333333333333333333"), balance_wei: gwei_to_wei(1_000_000_230), - last_received_timestamp: from_time_t(now - 70_000), + last_received_timestamp: from_unix_timestamp(now - 70_000), }, ReceivableAccount { wallet: Wallet::new("0x8888888888888888888888888888888888888888"), balance_wei: gwei_to_wei(-90), - last_received_timestamp: from_time_t(now - 66_000), + last_received_timestamp: from_unix_timestamp(now - 66_000), } ] ); @@ -1725,20 +1728,20 @@ mod tests { #[test] fn range_query_does_not_display_values_from_below_1_gwei() { - let timestamp1 = now_time_t() - 5000; - let timestamp2 = now_time_t() - 3232; + let timestamp1 = current_unix_timestamp() - 5000; + let timestamp2 = current_unix_timestamp() - 3232; let main_setup = |conn: &dyn ConnectionWrapper, insert: InsertReceivableHelperFn| { insert( conn, "0x1111111111111111111111111111111111111111", 999_999_999, //smaller than 1 gwei - now_time_t() - 11_001, + current_unix_timestamp() - 11_001, ); insert( conn, "0x2222222222222222222222222222222222222222", -999_999_999, //smaller than -1 gwei - now_time_t() - 5_606, + current_unix_timestamp() - 5_606, ); insert( conn, @@ -1774,12 +1777,12 @@ mod tests { ReceivableAccount { wallet: Wallet::new("0x3333333333333333333333333333333333333333"), balance_wei: 30_000_300_000, - last_received_timestamp: from_time_t(timestamp1), + last_received_timestamp: from_unix_timestamp(timestamp1), }, ReceivableAccount { wallet: Wallet::new("0x4444444444444444444444444444444444444444"), balance_wei: -2_000_300_000, - last_received_timestamp: from_time_t(timestamp2), + last_received_timestamp: from_unix_timestamp(timestamp2), } ] ) @@ -1793,7 +1796,7 @@ mod tests { .unwrap(); let insert = insert_account_by_separate_values; - let timestamp = utils::now_time_t(); + let timestamp = utils::current_unix_timestamp(); insert( &*conn, "0x1111111111111111111111111111111111111111", @@ -1855,7 +1858,7 @@ mod tests { &account.wallet, &high_bytes, &low_bytes, - &to_time_t(account.last_received_timestamp), + &to_unix_timestamp(account.last_received_timestamp), ]; stmt.execute(params).unwrap(); } diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 0f187805e..687ed5264 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -174,7 +174,7 @@ mod tests { use crate::accountant::db_access_objects::sent_payable_dao::{ SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, }; - use crate::accountant::db_access_objects::utils::now_time_t; + use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, }; @@ -241,7 +241,7 @@ mod tests { hash: self.hash_opt.unwrap_or_default(), receiver_address: self.receiver_address_opt.unwrap_or_default(), amount: self.amount_opt.unwrap_or_default(), - timestamp: self.timestamp_opt.unwrap_or_else(now_time_t), + timestamp: self.timestamp_opt.unwrap_or_else(current_unix_timestamp), gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), nonce: self.nonce_opt.unwrap_or_default(), status: self.status_opt.unwrap_or(TxStatus::Pending), diff --git a/node/src/accountant/db_access_objects/utils.rs b/node/src/accountant/db_access_objects/utils.rs index 13fd7e8cd..fe84712d7 100644 --- a/node/src/accountant/db_access_objects/utils.rs +++ b/node/src/accountant/db_access_objects/utils.rs @@ -21,8 +21,7 @@ use std::string::ToString; use std::time::Duration; use std::time::SystemTime; -// TODO: GH-608: This should be renamed to to_unix_timestamp and it should be u64 -pub fn to_time_t(system_time: SystemTime) -> i64 { +pub fn to_unix_timestamp(system_time: SystemTime) -> i64 { match system_time.duration_since(SystemTime::UNIX_EPOCH) { Ok(d) => sign_conversion::(d.as_secs()).expect("MASQNode has expired"), Err(e) => panic!( @@ -32,13 +31,12 @@ pub fn to_time_t(system_time: SystemTime) -> i64 { } } -pub fn now_time_t() -> i64 { - to_time_t(SystemTime::now()) +pub fn current_unix_timestamp() -> i64 { + to_unix_timestamp(SystemTime::now()) } -// TODO: GH-608: This should be renamed to from_unix_timestamp and it should be u64 -pub fn from_time_t(time_t: i64) -> SystemTime { - let interval = Duration::from_secs(time_t as u64); +pub fn from_unix_timestamp(unix_timestamp: i64) -> SystemTime { + let interval = Duration::from_secs(unix_timestamp as u64); SystemTime::UNIX_EPOCH + interval } @@ -195,11 +193,11 @@ impl CustomQuery { max_age: u64, timestamp: SystemTime, ) -> RusqliteParamsWithOwnedToSql { - let now = to_time_t(timestamp); - let age_to_time_t = |age_limit| now - checked_conversion::(age_limit); + let now = to_unix_timestamp(timestamp); + let age_to_unix_timestamp = |age_limit| now - checked_conversion::(age_limit); vec![ - (":min_timestamp", Box::new(age_to_time_t(max_age))), - (":max_timestamp", Box::new(age_to_time_t(min_age))), + (":min_timestamp", Box::new(age_to_unix_timestamp(max_age))), + (":max_timestamp", Box::new(age_to_unix_timestamp(min_age))), ] } @@ -301,7 +299,7 @@ pub fn remap_receivable_accounts(accounts: Vec) -> Vec u64 { - (to_time_t(SystemTime::now()) - to_time_t(timestamp)) as u64 + (to_unix_timestamp(SystemTime::now()) - to_unix_timestamp(timestamp)) as u64 } #[allow(clippy::type_complexity)] @@ -468,8 +466,8 @@ mod tests { }; let assigned_value_1 = get_assigned_value(param_pair_1.1.to_sql().unwrap()); let assigned_value_2 = get_assigned_value(param_pair_2.1.to_sql().unwrap()); - assert_eq!(assigned_value_1, to_time_t(now) - 10000); - assert_eq!(assigned_value_2, to_time_t(now) - 5555) + assert_eq!(assigned_value_1, to_unix_timestamp(now) - 10000); + assert_eq!(assigned_value_2, to_unix_timestamp(now) - 5555) } #[test] @@ -610,10 +608,10 @@ mod tests { #[test] #[should_panic(expected = "Must be wrong, moment way far in the past")] - fn to_time_t_does_not_like_time_traveling() { + fn to_unix_timestamp_does_not_like_time_traveling() { let far_far_before = UNIX_EPOCH.checked_sub(Duration::from_secs(1)).unwrap(); - let _ = to_time_t(far_far_before); + let _ = to_unix_timestamp(far_far_before); } #[test] diff --git a/node/src/accountant/mod.rs b/node/src/accountant/mod.rs index 0a74d076c..750c4c1a7 100644 --- a/node/src/accountant/mod.rs +++ b/node/src/accountant/mod.rs @@ -1043,7 +1043,7 @@ mod tests { PendingPayable, PendingPayableDaoError, TransactionHashes, }; use crate::accountant::db_access_objects::receivable_dao::ReceivableAccount; - use crate::accountant::db_access_objects::utils::{from_time_t, to_time_t, CustomQuery}; + use crate::accountant::db_access_objects::utils::{from_unix_timestamp, to_unix_timestamp, CustomQuery}; use crate::accountant::payment_adjuster::Adjustment; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::test_utils::BlockchainAgentMock; use crate::accountant::scanners::test_utils::protect_payables_in_test; @@ -2126,7 +2126,8 @@ mod tests { assert!(new_delinquencies_params.is_empty()); assert!( captured_timestamp < SystemTime::now() - && captured_timestamp >= from_time_t(to_time_t(SystemTime::now()) - 5) + && captured_timestamp + >= from_unix_timestamp(to_unix_timestamp(SystemTime::now()) - 5) ); assert_eq!(captured_curves, PaymentThresholds::default()); assert_eq!(paid_delinquencies_params.len(), 1); @@ -2517,13 +2518,13 @@ mod tests { unban_below_gwei: 10_000_000, }; let config = bc_from_earning_wallet(make_wallet("mine")); - let now = to_time_t(SystemTime::now()); + let now = to_unix_timestamp(SystemTime::now()); let accounts = vec![ // below minimum balance, to the right of time intersection (inside buffer zone) PayableAccount { wallet: make_wallet("wallet0"), balance_wei: gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei - 1), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( payment_thresholds.threshold_interval_sec + 10, ), @@ -2534,7 +2535,7 @@ mod tests { PayableAccount { wallet: make_wallet("wallet1"), balance_wei: gwei_to_wei(payment_thresholds.debt_threshold_gwei + 1), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( payment_thresholds.maturity_threshold_sec - 10, ), @@ -2545,7 +2546,7 @@ mod tests { PayableAccount { wallet: make_wallet("wallet2"), balance_wei: gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 55), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( payment_thresholds.maturity_threshold_sec + 15, ), @@ -2592,7 +2593,7 @@ mod tests { payable_scan_interval: Duration::from_secs(50_000), receivable_scan_interval: Duration::from_secs(50_000), }); - let now = to_time_t(SystemTime::now()); + let now = to_unix_timestamp(SystemTime::now()); let qualified_payables = vec![ // slightly above minimum balance, to the right of the curve (time intersection) PayableAccount { @@ -2600,7 +2601,7 @@ mod tests { balance_wei: gwei_to_wei( DEFAULT_PAYMENT_THRESHOLDS.permanent_debt_allowed_gwei + 1, ), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( DEFAULT_PAYMENT_THRESHOLDS.threshold_interval_sec + DEFAULT_PAYMENT_THRESHOLDS.maturity_threshold_sec @@ -2613,7 +2614,7 @@ mod tests { PayableAccount { wallet: make_wallet("wallet1"), balance_wei: gwei_to_wei(DEFAULT_PAYMENT_THRESHOLDS.debt_threshold_gwei + 1), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( DEFAULT_PAYMENT_THRESHOLDS.maturity_threshold_sec + 10, ), @@ -2670,13 +2671,13 @@ mod tests { )) .start(); let pps_for_blockchain_bridge_sub = blockchain_bridge_addr.clone().recipient(); - let last_paid_timestamp = to_time_t(SystemTime::now()) + let last_paid_timestamp = to_unix_timestamp(SystemTime::now()) - DEFAULT_PAYMENT_THRESHOLDS.maturity_threshold_sec as i64 - 1; let payable_account = PayableAccount { wallet: make_wallet("scan_for_payables"), balance_wei: gwei_to_wei(DEFAULT_PAYMENT_THRESHOLDS.debt_threshold_gwei + 1), - last_paid_timestamp: from_time_t(last_paid_timestamp), + last_paid_timestamp: from_unix_timestamp(last_paid_timestamp), pending_payable_opt: None, }; let payable_dao = payable_dao @@ -2753,7 +2754,7 @@ mod tests { .start(); let payable_fingerprint_1 = PendingPayableFingerprint { rowid: 555, - timestamp: from_time_t(210_000_000), + timestamp: from_unix_timestamp(210_000_000), hash: make_tx_hash(45678), attempt: 1, amount: 4444, @@ -2761,7 +2762,7 @@ mod tests { }; let payable_fingerprint_2 = PendingPayableFingerprint { rowid: 550, - timestamp: from_time_t(210_000_100), + timestamp: from_unix_timestamp(210_000_100), hash: make_tx_hash(112233), attempt: 2, amount: 7999, @@ -3806,7 +3807,7 @@ mod tests { }; let fingerprint_1 = PendingPayableFingerprint { rowid: 5, - timestamp: from_time_t(200_000_000), + timestamp: from_unix_timestamp(200_000_000), hash: transaction_hash_1, attempt: 2, amount: 444, @@ -3822,7 +3823,7 @@ mod tests { }; let fingerprint_2 = PendingPayableFingerprint { rowid: 10, - timestamp: from_time_t(199_780_000), + timestamp: from_unix_timestamp(199_780_000), hash: Default::default(), attempt: 15, amount: 1212, diff --git a/node/src/accountant/scanners/mod.rs b/node/src/accountant/scanners/mod.rs index 1307cb006..e8ce95812 100644 --- a/node/src/accountant/scanners/mod.rs +++ b/node/src/accountant/scanners/mod.rs @@ -1068,7 +1068,7 @@ mod tests { use crate::accountant::db_access_objects::pending_payable_dao::{ PendingPayable, PendingPayableDaoError, TransactionHashes, }; - use crate::accountant::db_access_objects::utils::{from_time_t, to_time_t}; + use crate::accountant::db_access_objects::utils::{from_unix_timestamp, to_unix_timestamp}; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::msgs::QualifiedPayablesMessage; use crate::accountant::scanners::scanners_utils::payable_scanner_utils::PendingPayableMetadata; use crate::accountant::scanners::scanners_utils::pending_payable_scanner_utils::{handle_none_status, handle_status_with_failure, PendingPayableScanReport}; @@ -2103,11 +2103,11 @@ mod tests { let now = SystemTime::now(); let payment_thresholds = PaymentThresholds::default(); let debt = gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 1); - let time = to_time_t(now) - payment_thresholds.maturity_threshold_sec as i64 - 1; + let time = to_unix_timestamp(now) - payment_thresholds.maturity_threshold_sec as i64 - 1; let unqualified_payable_account = vec![PayableAccount { wallet: make_wallet("wallet0"), balance_wei: debt, - last_paid_timestamp: from_time_t(time), + last_paid_timestamp: from_unix_timestamp(time), pending_payable_opt: None, }]; let subject = PayableScannerBuilder::new() @@ -2136,7 +2136,7 @@ mod tests { let qualified_payable = PayableAccount { wallet: make_wallet("wallet0"), balance_wei: debt, - last_paid_timestamp: from_time_t(time), + last_paid_timestamp: from_unix_timestamp(time), pending_payable_opt: None, }; let subject = PayableScannerBuilder::new() @@ -2168,8 +2168,8 @@ mod tests { let unqualified_payable_account = vec![PayableAccount { wallet: make_wallet("wallet1"), balance_wei: gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 1), - last_paid_timestamp: from_time_t( - to_time_t(now) - payment_thresholds.maturity_threshold_sec as i64 + 1, + last_paid_timestamp: from_unix_timestamp( + to_unix_timestamp(now) - payment_thresholds.maturity_threshold_sec as i64 + 1, ), pending_payable_opt: None, }]; @@ -2194,7 +2194,7 @@ mod tests { let now = SystemTime::now(); let payable_fingerprint_1 = PendingPayableFingerprint { rowid: 555, - timestamp: from_time_t(210_000_000), + timestamp: from_unix_timestamp(210_000_000), hash: make_tx_hash(45678), attempt: 1, amount: 4444, @@ -2202,7 +2202,7 @@ mod tests { }; let payable_fingerprint_2 = PendingPayableFingerprint { rowid: 550, - timestamp: from_time_t(210_000_100), + timestamp: from_unix_timestamp(210_000_100), hash: make_tx_hash(112233), attempt: 1, amount: 7999, @@ -2682,7 +2682,7 @@ mod tests { let rowid_2 = 5; let pending_payable_fingerprint_1 = PendingPayableFingerprint { rowid: rowid_1, - timestamp: from_time_t(199_000_000), + timestamp: from_unix_timestamp(199_000_000), hash: make_tx_hash(0x123), attempt: 1, amount: 4567, @@ -2690,7 +2690,7 @@ mod tests { }; let pending_payable_fingerprint_2 = PendingPayableFingerprint { rowid: rowid_2, - timestamp: from_time_t(200_000_000), + timestamp: from_unix_timestamp(200_000_000), hash: make_tx_hash(0x567), attempt: 1, amount: 5555, @@ -2759,7 +2759,7 @@ mod tests { let test_name = "total_paid_payable_rises_with_each_bill_paid"; let fingerprint_1 = PendingPayableFingerprint { rowid: 5, - timestamp: from_time_t(189_999_888), + timestamp: from_unix_timestamp(189_999_888), hash: make_tx_hash(56789), attempt: 1, amount: 5478, @@ -2767,7 +2767,7 @@ mod tests { }; let fingerprint_2 = PendingPayableFingerprint { rowid: 6, - timestamp: from_time_t(200_000_011), + timestamp: from_unix_timestamp(200_000_011), hash: make_tx_hash(33333), attempt: 1, amount: 6543, @@ -2816,7 +2816,7 @@ mod tests { }; let fingerprint_1 = PendingPayableFingerprint { rowid: 5, - timestamp: from_time_t(200_000_000), + timestamp: from_unix_timestamp(200_000_000), hash: transaction_hash_1, attempt: 2, amount: 444, @@ -2832,7 +2832,7 @@ mod tests { }; let fingerprint_2 = PendingPayableFingerprint { rowid: 10, - timestamp: from_time_t(199_780_000), + timestamp: from_unix_timestamp(199_780_000), hash: transaction_hash_2, attempt: 15, amount: 1212, @@ -3277,7 +3277,7 @@ mod tests { let test_name = "signal_scanner_completion_and_log_if_timestamp_is_correct"; let logger = Logger::new(test_name); let mut subject = ScannerCommon::new(Rc::new(make_custom_payment_thresholds())); - let start = from_time_t(1_000_000_000); + let start = from_unix_timestamp(1_000_000_000); let end = start.checked_add(Duration::from_millis(145)).unwrap(); subject.initiated_at_opt = Some(start); diff --git a/node/src/accountant/scanners/scanners_utils.rs b/node/src/accountant/scanners/scanners_utils.rs index 30b3a3d2d..ce2851e28 100644 --- a/node/src/accountant/scanners/scanners_utils.rs +++ b/node/src/accountant/scanners/scanners_utils.rs @@ -440,7 +440,7 @@ pub mod receivable_scanner_utils { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::utils::{from_time_t, to_time_t}; + use crate::accountant::db_access_objects::utils::{from_unix_timestamp, to_unix_timestamp}; use crate::accountant::db_access_objects::payable_dao::{PayableAccount}; use crate::accountant::db_access_objects::receivable_dao::ReceivableAccount; use crate::accountant::scanners::scanners_utils::payable_scanner_utils::PayableTransactingErrorEnum::{ @@ -467,21 +467,21 @@ mod tests { #[test] fn investigate_debt_extremes_picks_the_most_relevant_records() { let now = SystemTime::now(); - let now_t = to_time_t(now); + let now_t = to_unix_timestamp(now); let same_amount_significance = 2_000_000; - let same_age_significance = from_time_t(now_t - 30000); + let same_age_significance = from_unix_timestamp(now_t - 30000); let payables = &[ PayableAccount { wallet: make_wallet("wallet0"), balance_wei: same_amount_significance, - last_paid_timestamp: from_time_t(now_t - 5000), + last_paid_timestamp: from_unix_timestamp(now_t - 5000), pending_payable_opt: None, }, //this debt is more significant because beside being high in amount it's also older, so should be prioritized and picked PayableAccount { wallet: make_wallet("wallet1"), balance_wei: same_amount_significance, - last_paid_timestamp: from_time_t(now_t - 10000), + last_paid_timestamp: from_unix_timestamp(now_t - 10000), pending_payable_opt: None, }, //similarly these two wallets have debts equally old but the second has a bigger balance and should be chosen @@ -511,7 +511,7 @@ mod tests { let receivable_account = ReceivableAccount { wallet: make_wallet("wallet0"), balance_wei: 10_000_000_000, - last_received_timestamp: from_time_t(to_time_t(now) - offset), + last_received_timestamp: from_unix_timestamp(to_unix_timestamp(now) - offset), }; let (balance, age) = balance_and_age(now, &receivable_account); @@ -614,7 +614,7 @@ mod tests { #[test] fn payables_debug_summary_prints_pretty_summary() { init_test_logging(); - let now = to_time_t(SystemTime::now()); + let now = to_unix_timestamp(SystemTime::now()); let payment_thresholds = PaymentThresholds { threshold_interval_sec: 2_592_000, debt_threshold_gwei: 1_000_000_000, @@ -628,7 +628,7 @@ mod tests { PayableAccount { wallet: make_wallet("wallet0"), balance_wei: gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 2000), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( payment_thresholds.maturity_threshold_sec + payment_thresholds.threshold_interval_sec, @@ -642,7 +642,7 @@ mod tests { PayableAccount { wallet: make_wallet("wallet1"), balance_wei: gwei_to_wei(payment_thresholds.debt_threshold_gwei - 1), - last_paid_timestamp: from_time_t( + last_paid_timestamp: from_unix_timestamp( now - checked_conversion::( payment_thresholds.maturity_threshold_sec + 55, ), diff --git a/node/src/accountant/test_utils.rs b/node/src/accountant/test_utils.rs index 5c9d6f14a..e7186d2cd 100644 --- a/node/src/accountant/test_utils.rs +++ b/node/src/accountant/test_utils.rs @@ -12,7 +12,9 @@ use crate::accountant::db_access_objects::pending_payable_dao::{ use crate::accountant::db_access_objects::receivable_dao::{ ReceivableAccount, ReceivableDao, ReceivableDaoError, ReceivableDaoFactory, }; -use crate::accountant::db_access_objects::utils::{from_time_t, to_time_t, CustomQuery}; +use crate::accountant::db_access_objects::utils::{ + from_unix_timestamp, to_unix_timestamp, CustomQuery, +}; use crate::accountant::payment_adjuster::{Adjustment, AnalysisError, PaymentAdjuster}; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::msgs::{ BlockchainAgentWithContextMessage, QualifiedPayablesMessage, @@ -60,7 +62,7 @@ use std::sync::{Arc, Mutex}; use std::time::{Duration, SystemTime}; pub fn make_receivable_account(n: u64, expected_delinquent: bool) -> ReceivableAccount { - let now = to_time_t(SystemTime::now()); + let now = to_unix_timestamp(SystemTime::now()); ReceivableAccount { wallet: make_wallet(&format!( "wallet{}{}", @@ -68,13 +70,13 @@ pub fn make_receivable_account(n: u64, expected_delinquent: bool) -> ReceivableA if expected_delinquent { "d" } else { "n" } )), balance_wei: gwei_to_wei(n), - last_received_timestamp: from_time_t(now - (n as i64)), + last_received_timestamp: from_unix_timestamp(now - (n as i64)), } } pub fn make_payable_account(n: u64) -> PayableAccount { - let now = to_time_t(SystemTime::now()); - let timestamp = from_time_t(now - (n as i64)); + let now = to_unix_timestamp(SystemTime::now()); + let timestamp = from_unix_timestamp(now - (n as i64)); make_payable_account_with_wallet_and_balance_and_timestamp_opt( make_wallet(&format!("wallet{}", n)), gwei_to_wei(n), @@ -1250,7 +1252,7 @@ pub fn make_custom_payment_thresholds() -> PaymentThresholds { pub fn make_pending_payable_fingerprint() -> PendingPayableFingerprint { PendingPayableFingerprint { rowid: 33, - timestamp: from_time_t(222_222_222), + timestamp: from_unix_timestamp(222_222_222), hash: make_tx_hash(456), attempt: 1, amount: 12345, @@ -1269,8 +1271,8 @@ pub fn make_payables( let unqualified_payable_accounts = vec![PayableAccount { wallet: make_wallet("wallet1"), balance_wei: gwei_to_wei(payment_thresholds.permanent_debt_allowed_gwei + 1), - last_paid_timestamp: from_time_t( - to_time_t(now) - payment_thresholds.maturity_threshold_sec as i64 + 1, + last_paid_timestamp: from_unix_timestamp( + to_unix_timestamp(now) - payment_thresholds.maturity_threshold_sec as i64 + 1, ), pending_payable_opt: None, }]; @@ -1280,8 +1282,8 @@ pub fn make_payables( balance_wei: gwei_to_wei( payment_thresholds.permanent_debt_allowed_gwei + 1_000_000_000, ), - last_paid_timestamp: from_time_t( - to_time_t(now) - payment_thresholds.maturity_threshold_sec as i64 - 1, + last_paid_timestamp: from_unix_timestamp( + to_unix_timestamp(now) - payment_thresholds.maturity_threshold_sec as i64 - 1, ), pending_payable_opt: None, }, @@ -1290,8 +1292,8 @@ pub fn make_payables( balance_wei: gwei_to_wei( payment_thresholds.permanent_debt_allowed_gwei + 1_200_000_000, ), - last_paid_timestamp: from_time_t( - to_time_t(now) - payment_thresholds.maturity_threshold_sec as i64 - 100, + last_paid_timestamp: from_unix_timestamp( + to_unix_timestamp(now) - payment_thresholds.maturity_threshold_sec as i64 - 100, ), pending_payable_opt: None, }, diff --git a/node/src/blockchain/blockchain_bridge.rs b/node/src/blockchain/blockchain_bridge.rs index e4275b036..1a01087e1 100644 --- a/node/src/blockchain/blockchain_bridge.rs +++ b/node/src/blockchain/blockchain_bridge.rs @@ -548,7 +548,7 @@ mod tests { use super::*; use crate::accountant::db_access_objects::payable_dao::PayableAccount; use crate::accountant::db_access_objects::pending_payable_dao::PendingPayable; - use crate::accountant::db_access_objects::utils::from_time_t; + use crate::accountant::db_access_objects::utils::from_unix_timestamp; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::agent_web3::WEB3_MAXIMAL_GAS_LIMIT_MARGIN; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::test_utils::BlockchainAgentMock; use crate::accountant::scanners::test_utils::protect_payables_in_test; @@ -876,7 +876,7 @@ mod tests { let accounts = vec![PayableAccount { wallet: wallet_account, balance_wei: 111_420_204, - last_paid_timestamp: from_time_t(150_000_000), + last_paid_timestamp: from_unix_timestamp(150_000_000), pending_payable_opt: None, }]; let agent_id_stamp = ArbitraryIdStamp::new(); @@ -966,7 +966,7 @@ mod tests { let accounts = vec![PayableAccount { wallet: wallet_account, balance_wei: 111_420_204, - last_paid_timestamp: from_time_t(150_000_000), + last_paid_timestamp: from_unix_timestamp(150_000_000), pending_payable_opt: None, }]; let consuming_wallet = make_paying_wallet(b"consuming_wallet"); @@ -1337,7 +1337,7 @@ mod tests { }; let fingerprint_4 = PendingPayableFingerprint { rowid: 450, - timestamp: from_time_t(230_000_000), + timestamp: from_unix_timestamp(230_000_000), hash: hash_4, attempt: 1, amount: 7879, diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/utils.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/utils.rs index 03ed4150b..ee2e49786 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/utils.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/utils.rs @@ -330,7 +330,7 @@ pub fn create_blockchain_agent_web3( #[cfg(test)] mod tests { use super::*; - use crate::accountant::db_access_objects::utils::from_time_t; + use crate::accountant::db_access_objects::utils::from_unix_timestamp; use crate::accountant::gwei_to_wei; use crate::accountant::scanners::mid_scan_msg_handling::payable_scanner::agent_web3::WEB3_MAXIMAL_GAS_LIMIT_MARGIN; use crate::accountant::test_utils::{ @@ -522,13 +522,13 @@ mod tests { PayableAccount { wallet: make_wallet("4567"), balance_wei: 2_345_678, - last_paid_timestamp: from_time_t(4500000), + last_paid_timestamp: from_unix_timestamp(4500000), pending_payable_opt: None, }, PayableAccount { wallet: make_wallet("5656"), balance_wei: 6_543_210, - last_paid_timestamp: from_time_t(333000), + last_paid_timestamp: from_unix_timestamp(333000), pending_payable_opt: None, }, ]; diff --git a/node/src/database/db_migrations/migrations/migration_4_to_5.rs b/node/src/database/db_migrations/migrations/migration_4_to_5.rs index 06deb809f..4b5bbb50a 100644 --- a/node/src/database/db_migrations/migrations/migration_4_to_5.rs +++ b/node/src/database/db_migrations/migrations/migration_4_to_5.rs @@ -76,7 +76,7 @@ impl DatabaseMigration for Migrate_4_to_5 { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::utils::{from_time_t, to_time_t}; + use crate::accountant::db_access_objects::utils::{from_unix_timestamp, to_unix_timestamp}; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, ExternalData, DATABASE_FILE, }; @@ -124,7 +124,7 @@ mod tests { None, &wallet_1, 113344, - from_time_t(250_000_000), + from_unix_timestamp(250_000_000), ); let config_table_before = fetch_all_from_config_table(conn.as_ref()); @@ -160,7 +160,7 @@ mod tests { let params: &[&dyn ToSql] = &[ &wallet, &amount, - &to_time_t(timestamp), + &to_unix_timestamp(timestamp), if !hash_str.is_empty() { &hash_str } else { @@ -208,7 +208,7 @@ mod tests { Some(transaction_hash_2), &wallet_2, 1111111, - from_time_t(200_000_000), + from_unix_timestamp(200_000_000), ); let config_table_before = fetch_all_from_config_table(&conn); diff --git a/node/tests/financials_test.rs b/node/tests/financials_test.rs index 9847efa38..7aff319d2 100644 --- a/node/tests/financials_test.rs +++ b/node/tests/financials_test.rs @@ -13,7 +13,7 @@ use masq_lib::test_utils::utils::{ensure_node_home_directory_exists, open_all_fi use masq_lib::utils::find_free_port; use node_lib::accountant::db_access_objects::payable_dao::{PayableDao, PayableDaoReal}; use node_lib::accountant::db_access_objects::receivable_dao::{ReceivableDao, ReceivableDaoReal}; -use node_lib::accountant::db_access_objects::utils::{from_time_t, to_time_t}; +use node_lib::accountant::db_access_objects::utils::{from_unix_timestamp, to_unix_timestamp}; use node_lib::accountant::gwei_to_wei; use node_lib::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, @@ -30,9 +30,9 @@ fn financials_command_retrieves_payable_and_receivable_records_integration() { let port = find_free_port(); let home_dir = ensure_node_home_directory_exists("integration", test_name); let now = SystemTime::now(); - let timestamp_payable = from_time_t(to_time_t(now) - 678); - let timestamp_receivable_1 = from_time_t(to_time_t(now) - 10000); - let timestamp_receivable_2 = from_time_t(to_time_t(now) - 1111); + let timestamp_payable = from_unix_timestamp(to_unix_timestamp(now) - 678); + let timestamp_receivable_1 = from_unix_timestamp(to_unix_timestamp(now) - 10000); + let timestamp_receivable_2 = from_unix_timestamp(to_unix_timestamp(now) - 1111); let wallet_payable = make_wallet("efef"); let wallet_receivable_1 = make_wallet("abcde"); let wallet_receivable_2 = make_wallet("ccccc"); From 5e198b2fb3746b3800a4a704968280dd52bbbe9f Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 17 Apr 2025 16:02:29 +0530 Subject: [PATCH 13/46] GH-608: refactor sent_payable_dao.rs --- .../db_access_objects/sent_payable_dao.rs | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 687ed5264..b84dbe073 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -22,7 +22,6 @@ type TxIdentifiers = HashMap; #[derive(Clone, Debug, PartialEq, Eq)] pub struct Tx { - // TODO: GH-608: Perhaps TxReceipt could be a similar structure to be used hash: H256, receiver_address: Address, amount: u128, @@ -57,25 +56,6 @@ impl<'a> SentPayableDaoReal<'a> { // TODO: GH-608: You'll need to write a mock to test this, do that wisely Self { conn } } - - fn generate_sql_values_for_insertion(txs: &[Tx]) -> String { - comma_joined_stringifiable(txs, |tx| { - let amount_checked = checked_conversion::(tx.amount); - let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); - // TODO: GH-608: Perhaps you should pick status from the Tx itself - format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", - tx.hash, - tx.receiver_address, - high_bytes, - low_bytes, - tx.timestamp, - tx.gas_price_wei, - tx.nonce, - tx.status - ) - }) - } } impl SentPayableDao for SentPayableDaoReal<'_> { @@ -150,7 +130,21 @@ impl SentPayableDao for SentPayableDaoReal<'_> { tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status, retried\ ) values {}", - Self::generate_sql_values_for_insertion(&txs) + comma_joined_stringifiable(&txs, |tx| { + let amount_checked = checked_conversion::(tx.amount); + let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); + format!( + "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", + tx.hash, + tx.receiver_address, + high_bytes, + low_bytes, + tx.timestamp, + tx.gas_price_wei, + tx.nonce, + tx.status + ) + }) ); match self.conn.prepare(&sql).expect("Internal error").execute([]) { From 8871b72e2b407111917de870a00bd2e8cd859868 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 18 Apr 2025 15:53:27 +0530 Subject: [PATCH 14/46] GH-608: add stronger tests for insert_new_records() --- .../db_access_objects/sent_payable_dao.rs | 128 ++++++++++++++++-- .../lower_level_interface_web3.rs | 37 ++++- 2 files changed, 151 insertions(+), 14 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index b84dbe073..174366625 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -38,6 +38,7 @@ pub struct StatusChange { pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; + fn retrieve_all_txs(&self) -> Vec; fn retrieve_pending_txs(&self) -> Vec; fn retrieve_txs_to_retry(&self) -> Vec; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; @@ -82,6 +83,46 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } + fn retrieve_all_txs(&self) -> Vec { + let sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status from sent_payable" + .to_string(); + + let mut stmt = self + .conn + .prepare(&sql) + .expect("Failed to prepare SQL statement"); + + stmt.query_map([], |row| { + let tx_hash_str: String = row.get(0).expectv("tx_hash"); + let hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); + let receiver_address_str: String = row.get(1).expectv("row_id"); + let receiver_address = + Address::from_str(&receiver_address_str[2..]).expect("Failed to parse H160"); + let amount_high_b = row.get(2).expectv("amount_high_b"); + let amount_low_b = row.get(3).expectv("amount_low_b"); + let amount = BigIntDivider::reconstitute(amount_high_b, amount_low_b) as u128; + let timestamp = row.get(4).expectv("timestamp"); + let gas_price_wei = row.get(5).expectv("gas_price_wei"); + let nonce = row.get(6).expectv("nonce"); + let status_str: String = row.get(7).expectv("status"); + let status = TxStatus::from_str(&status_str).expect("Failed to parse TxStatus"); + + Ok(Tx { + hash, + receiver_address, + amount, + timestamp, + gas_price_wei, + nonce, + status, + }) + }) + .expect("Failed to execute query") + .filter_map(Result::ok) + .collect() + } + fn retrieve_pending_txs(&self) -> Vec { let sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status from sent_payable where status in ('Pending')" @@ -250,31 +291,92 @@ mod tests { let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let tx = TxBuilder::default().build(); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Failed) + .build(); + let tx3 = TxBuilder::default() + .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); let subject = SentPayableDaoReal::new(wrapped_conn); + let txs = vec![tx1, tx2, tx3]; - let result = subject.insert_new_records(vec![tx]); + let result = subject.insert_new_records(txs.clone()); - let row_count: i64 = { - let mut stmt = subject - .conn - .prepare("SELECT COUNT(*) FROM sent_payable") - .unwrap(); - stmt.query_row([], |row| row.get(0)).unwrap() - }; + let retrieved_txs = subject.retrieve_all_txs(); assert_eq!(result, Ok(())); - assert_eq!(row_count, 1); - // TODO: GH-608: Add more assertions to verify transactions of all statuses are retreived properly + assert_eq!(retrieved_txs.len(), 3); + assert_eq!(retrieved_txs, txs); } #[test] fn insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted() { - todo!("write me"); + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let hash = H256::from_low_u64_be(1234567890); + let tx1 = TxBuilder::default() + .hash(hash) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(hash) + .status(TxStatus::Failed) + .build(); + let subject = SentPayableDaoReal::new(wrapped_conn); + + let result = subject.insert_new_records(vec![tx1, tx2]); + + assert_eq!( + result, + Err(SentPayableDaoError::InsertionFailed( + "UNIQUE constraint failed: sent_payable.tx_hash".to_string() + )) + ); } #[test] fn insert_new_records_throws_error_when_txs_with_an_already_present_hash_is_inserted() { - todo!("write me"); + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "insert_new_records_throws_error_when_txs_with_an_already_present_hash_is_inserted", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let hash = H256::from_low_u64_be(1234567890); + let tx1 = TxBuilder::default() + .hash(hash) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(hash) + .status(TxStatus::Failed) + .build(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let initial_insertion_result = subject.insert_new_records(vec![tx1]); + + let result = subject.insert_new_records(vec![tx2]); + + assert_eq!(initial_insertion_result, Ok(())); + assert_eq!( + result, + Err(SentPayableDaoError::InsertionFailed( + "UNIQUE constraint failed: sent_payable.tx_hash".to_string() + )) + ); } #[test] diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs index 7186779bc..eb56ba2ff 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs @@ -8,6 +8,7 @@ use ethereum_types::{H256, U256, U64}; use futures::Future; use serde_json::Value; use std::fmt::Display; +use std::str::FromStr; use web3::contract::{Contract, Options}; use web3::transports::{Batch, Http}; use web3::types::{Address, BlockNumber, Filter, Log, TransactionReceipt}; @@ -26,13 +27,47 @@ pub enum TxStatus { Succeeded(TransactionBlock), } +impl TxStatus { + pub fn from_str(s: &str) -> Result { + // TODO: GH-608: Test me + match s { + "Pending" => Ok(TxStatus::Pending), + "Failed" => Ok(TxStatus::Failed), + s if s.starts_with("Succeeded") => { + // The format is "Succeeded(block_number, block_hash)" + let parts: Vec<&str> = s[10..s.len() - 1].split(',').collect(); + if parts.len() != 2 { + return Err("Invalid Succeeded format".to_string()); + } + let block_number = match parts[0].parse() { + Ok(n) => n, + Err(_) => return Err("Invalid block number".to_string()), + }; + let block_hash = match H256::from_str(&parts[1][2..]) { + Ok(h) => h, + Err(_) => return Err("Invalid block hash".to_string()), + }; + Ok(TxStatus::Succeeded(TransactionBlock { + block_hash, + block_number, + })) + } + _ => Err(format!("Unknown status: {}", s)), + } + } +} + impl Display for TxStatus { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { TxStatus::Failed => write!(f, "Failed"), TxStatus::Pending => write!(f, "Pending"), TxStatus::Succeeded(block) => { - write!(f, "Succeeded({},{})", block.block_number, block.block_hash) + write!( + f, + "Succeeded({},{:?})", + block.block_number, block.block_hash + ) } } } From 5b99e07ce783a4cc81b9a1c6343abe1704f4c1fd Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 18 Apr 2025 16:09:49 +0530 Subject: [PATCH 15/46] GH-608: retrieve txs conditionally --- .../db_access_objects/sent_payable_dao.rs | 73 +++++++------------ 1 file changed, 26 insertions(+), 47 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 174366625..e32a0af55 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::fmt::{Display, Formatter}; use std::str::FromStr; use ethereum_types::H256; use web3::types::Address; @@ -35,11 +36,25 @@ pub struct StatusChange { new_status: TxStatus, } +pub enum RetrieveCondition { + IsPending, +} + +impl Display for RetrieveCondition { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // TODO: GH-608: Test me separately + match self { + RetrieveCondition::IsPending => { + write!(f, "where status in ('Pending')") + } + } + } +} + pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; - fn retrieve_all_txs(&self) -> Vec; - fn retrieve_pending_txs(&self) -> Vec; + fn retrieve_txs(&self, condition: Option) -> Vec; fn retrieve_txs_to_retry(&self) -> Vec; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; // TODO: GH-608: We might not need this @@ -83,10 +98,14 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn retrieve_all_txs(&self) -> Vec { - let sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ + fn retrieve_txs(&self, condition_opt: Option) -> Vec { + let raw_sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status from sent_payable" .to_string(); + let sql = match condition_opt { + None => raw_sql, + Some(condition) => format!("{} {}", raw_sql, condition), + }; let mut stmt = self .conn @@ -123,44 +142,6 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn retrieve_pending_txs(&self) -> Vec { - let sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ - timestamp, gas_price_wei, nonce, status from sent_payable where status in ('Pending')" - .to_string(); - - let mut stmt = self - .conn - .prepare(&sql) - .expect("Failed to prepare SQL statement"); - - stmt.query_map([], |row| { - let tx_hash_str: String = row.get(0).expectv("tx_hash"); - let hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); - let receiver_address_str: String = row.get(1).expectv("row_id"); - let receiver_address = - Address::from_str(&receiver_address_str[2..]).expect("Failed to parse H160"); - let amount_high_b = row.get(2).expectv("amount_high_b"); - let amount_low_b = row.get(3).expectv("amount_low_b"); - let amount = BigIntDivider::reconstitute(amount_high_b, amount_low_b) as u128; - let timestamp = row.get(4).expectv("timestamp"); - let gas_price_wei = row.get(5).expectv("gas_price_wei"); - let nonce = row.get(6).expectv("nonce"); - - Ok(Tx { - hash, - receiver_address, - amount, - timestamp, - gas_price_wei, - nonce, - status: TxStatus::Pending, - }) - }) - .expect("Failed to execute query") - .filter_map(Result::ok) - .collect() - } - fn retrieve_txs_to_retry(&self) -> Vec { todo!() } @@ -206,9 +187,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::sent_payable_dao::{ - SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, - }; + use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx}; use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, @@ -311,7 +290,7 @@ mod tests { let result = subject.insert_new_records(txs.clone()); - let retrieved_txs = subject.retrieve_all_txs(); + let retrieved_txs = subject.retrieve_txs(None); assert_eq!(result, Ok(())); assert_eq!(retrieved_txs.len(), 3); assert_eq!(retrieved_txs, txs); @@ -480,7 +459,7 @@ mod tests { .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3, tx4]) .unwrap(); - let result = subject.retrieve_pending_txs(); + let result = subject.retrieve_txs(Some(RetrieveCondition::IsPending)); assert_eq!(result, vec![tx1, tx2]); } From 64f14a07633c1b18b99425953ea3e9aa31ec2f54 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 18 Apr 2025 16:30:47 +0530 Subject: [PATCH 16/46] GH-608: change SQL to uppercase wherever necessary --- .../db_access_objects/sent_payable_dao.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index e32a0af55..8432d5d29 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -45,7 +45,7 @@ impl Display for RetrieveCondition { // TODO: GH-608: Test me separately match self { RetrieveCondition::IsPending => { - write!(f, "where status in ('Pending')") + write!(f, "WHERE status IN ('Pending')") } } } @@ -77,7 +77,7 @@ impl<'a> SentPayableDaoReal<'a> { impl SentPayableDao for SentPayableDaoReal<'_> { fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers { let sql = format!( - "select tx_hash, rowid from sent_payable where tx_hash in ({})", + "SELECT tx_hash, rowid FROM sent_payable WHERE tx_hash IN ({})", comma_joined_stringifiable(hashes, |hash| format!("'{:?}'", hash)) ); @@ -99,8 +99,8 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn retrieve_txs(&self, condition_opt: Option) -> Vec { - let raw_sql = "select tx_hash, receiver_address, amount_high_b, amount_low_b, \ - timestamp, gas_price_wei, nonce, status from sent_payable" + let raw_sql = "SELECT tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status FROM sent_payable" .to_string(); let sql = match condition_opt { None => raw_sql, @@ -148,10 +148,10 @@ impl SentPayableDao for SentPayableDaoReal<'_> { fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { let sql = format!( - "insert into sent_payable (\ + "INSERT INTO sent_payable (\ tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status, retried\ - ) values {}", + ) VALUES {}", comma_joined_stringifiable(&txs, |tx| { let amount_checked = checked_conversion::(tx.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); @@ -365,9 +365,9 @@ mod tests { // Inject a deliberately failing statement into the mocked connection. let failing_stmt = { setup_conn - .execute("create table example (id integer)", []) + .execute("CREATE TABLE example (id integer)", []) .unwrap(); - setup_conn.prepare("select id from example").unwrap() + setup_conn.prepare("SELECT id FROM example").unwrap() }; let wrapped_conn = ConnectionWrapperMock::default().prepare_result(Ok(failing_stmt)); let tx = TxBuilder::default().build(); From 41e6ea13bf4fc65d92f0bfbe5ccc6bb668dc9ad1 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 18 Apr 2025 17:39:11 +0530 Subject: [PATCH 17/46] GH-608: add RetrieveCondtion::ToRetry --- .../db_access_objects/sent_payable_dao.rs | 64 ++++++++++++++++++- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 8432d5d29..ed081fd57 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -5,6 +5,7 @@ use ethereum_types::H256; use web3::types::Address; use masq_lib::utils::ExpectValue; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; +use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; use crate::database::rusqlite_wrappers::ConnectionWrapper; @@ -38,6 +39,7 @@ pub struct StatusChange { pub enum RetrieveCondition { IsPending, + ToRetry, } impl Display for RetrieveCondition { @@ -45,7 +47,15 @@ impl Display for RetrieveCondition { // TODO: GH-608: Test me separately match self { RetrieveCondition::IsPending => { - write!(f, "WHERE status IN ('Pending')") + write!(f, "WHERE status = 'Pending'") + } + RetrieveCondition::ToRetry => { + let ten_minutes_ago = current_unix_timestamp() - 60 * 10; + write!( + f, + "WHERE status = 'Pending' AND timestamp <= {ten_minutes_ago}", + // "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp <= {ten_minutes_ago}", // TODO: GH-608: Failed Txs should also be included here. Think... + ) } } } @@ -429,9 +439,9 @@ mod tests { } #[test] - fn retrieve_pending_txs_works() { + fn can_retrieve_pending_txs() { let home_dir = - ensure_node_home_directory_exists("sent_payable_dao", "retrieve_pending_txs_works"); + ensure_node_home_directory_exists("sent_payable_dao", "can_retrieve_pending_txs"); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); @@ -463,4 +473,52 @@ mod tests { assert_eq!(result, vec![tx1, tx2]); } + + #[test] + fn can_retrieve_txs_to_retry() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "can_retrieve_pending_txs"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let current_unix_timestamp = current_unix_timestamp(); + let old_but_not_that_old = current_unix_timestamp - 60; // 1 minute old + let old_timestamp = current_unix_timestamp - 600; // 10 minutes old + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .timestamp(current_unix_timestamp) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .timestamp(old_but_not_that_old) + .status(TxStatus::Pending) + .build(); + let tx3 = TxBuilder::default() // this should be picked for retry + .hash(H256::from_low_u64_le(3)) + .timestamp(old_timestamp) + .status(TxStatus::Pending) + .build(); + let tx4 = TxBuilder::default() + .hash(H256::from_low_u64_le(4)) + .timestamp(old_timestamp) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); + let tx5 = TxBuilder::default() + .hash(H256::from_low_u64_le(5)) + .timestamp(old_timestamp) + .status(TxStatus::Failed) + .build(); + subject + .insert_new_records(vec![tx1, tx2, tx3.clone(), tx4, tx5]) + .unwrap(); + + let result = subject.retrieve_txs(Some(RetrieveCondition::ToRetry)); + + assert_eq!(result, vec![tx3]); + } } From fe1a66fb4a38870d3853822c50f0695882742e3e Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 18 Apr 2025 17:55:30 +0530 Subject: [PATCH 18/46] GH-608: add constant RETRY_THRESHOLD_SECS --- .../db_access_objects/sent_payable_dao.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index ed081fd57..015628a6f 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -10,6 +10,8 @@ use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; use crate::database::rusqlite_wrappers::ConnectionWrapper; +const RETRY_THRESHOLD_SECS: i64 = 10 * 60; // 10 minutes + #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { InsertionFailed(String), @@ -50,10 +52,10 @@ impl Display for RetrieveCondition { write!(f, "WHERE status = 'Pending'") } RetrieveCondition::ToRetry => { - let ten_minutes_ago = current_unix_timestamp() - 60 * 10; + let threshold_timestamp = current_unix_timestamp() - RETRY_THRESHOLD_SECS; write!( f, - "WHERE status = 'Pending' AND timestamp <= {ten_minutes_ago}", + "WHERE status = 'Pending' AND timestamp <= {threshold_timestamp}", // "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp <= {ten_minutes_ago}", // TODO: GH-608: Failed Txs should also be included here. Think... ) } @@ -197,7 +199,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx}; + use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, RETRY_THRESHOLD_SECS}; use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, @@ -273,6 +275,11 @@ mod tests { } } + #[test] + fn constants_have_correct_values() { + assert_eq!(RETRY_THRESHOLD_SECS, 10 * 60); + } + #[test] fn insert_new_records_works() { let home_dir = @@ -477,7 +484,7 @@ mod tests { #[test] fn can_retrieve_txs_to_retry() { let home_dir = - ensure_node_home_directory_exists("sent_payable_dao", "can_retrieve_pending_txs"); + ensure_node_home_directory_exists("sent_payable_dao", "can_retrieve_txs_to_retry"); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); From 2201030ee744b9ad86a335bcb8da3e269301b0f9 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Mon, 21 Apr 2025 14:22:21 +0530 Subject: [PATCH 19/46] GH-608: implement display and from_str() for TxStatus --- .../lower_level_interface_web3.rs | 92 ++++++++++++++++--- 1 file changed, 79 insertions(+), 13 deletions(-) diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs index eb56ba2ff..8d5be2b99 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs @@ -27,9 +27,10 @@ pub enum TxStatus { Succeeded(TransactionBlock), } -impl TxStatus { - pub fn from_str(s: &str) -> Result { - // TODO: GH-608: Test me +impl FromStr for TxStatus { + type Err = String; + + fn from_str(s: &str) -> Result { match s { "Pending" => Ok(TxStatus::Pending), "Failed" => Ok(TxStatus::Failed), @@ -39,17 +40,14 @@ impl TxStatus { if parts.len() != 2 { return Err("Invalid Succeeded format".to_string()); } - let block_number = match parts[0].parse() { - Ok(n) => n, - Err(_) => return Err("Invalid block number".to_string()), - }; - let block_hash = match H256::from_str(&parts[1][2..]) { - Ok(h) => h, - Err(_) => return Err("Invalid block hash".to_string()), - }; + let block_number: u64 = parts[0] + .parse() + .map_err(|_| "Invalid block number".to_string())?; + let block_hash = + H256::from_str(&parts[1][2..]).map_err(|_| "Invalid block hash".to_string())?; Ok(TxStatus::Succeeded(TransactionBlock { block_hash, - block_number, + block_number: U64::from(block_number), })) } _ => Err(format!("Unknown status: {}", s)), @@ -232,7 +230,7 @@ mod tests { use masq_lib::utils::find_free_port; use std::str::FromStr; use web3::types::{BlockNumber, Bytes, FilterBuilder, Log, TransactionReceipt, U256}; - use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TxReceipt, TxStatus}; + use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxReceipt, TxStatus}; #[test] fn get_transaction_fee_balance_works() { @@ -660,6 +658,74 @@ mod tests { assert_eq!(tx_receipt.status, TxStatus::Pending); } + #[test] + fn tx_status_display_works() { + // Test Failed + assert_eq!(TxStatus::Failed.to_string(), "Failed"); + + // Test Pending + assert_eq!(TxStatus::Pending.to_string(), "Pending"); + + // Test Succeeded + let block_number = U64::from(12345); + let block_hash = H256::from_low_u64_be(0xabcdef); + let succeeded = TxStatus::Succeeded(TransactionBlock { + block_hash, + block_number, + }); + assert_eq!( + succeeded.to_string(), + format!("Succeeded({},0x{:x})", block_number, block_hash) + ); + } + + #[test] + fn tx_status_from_str_works() { + // Test Pending + assert_eq!(TxStatus::from_str("Pending"), Ok(TxStatus::Pending)); + + // Test Failed + assert_eq!(TxStatus::from_str("Failed"), Ok(TxStatus::Failed)); + + // Test Succeeded with valid input + let block_number = 123456789; + let block_hash = H256::from_low_u64_be(0xabcdef); + let input = format!("Succeeded({},0x{:x})", block_number, block_hash); + assert_eq!( + TxStatus::from_str(&input), + Ok(TxStatus::Succeeded(TransactionBlock { + block_hash, + block_number: U64::from(block_number), + })) + ); + + // Test Succeeded with invalid format + assert_eq!( + TxStatus::from_str("Succeeded(123)"), + Err("Invalid Succeeded format".to_string()) + ); + + // Test Succeeded with invalid block number + assert_eq!( + TxStatus::from_str( + "Succeeded(abc,0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef)" + ), + Err("Invalid block number".to_string()) + ); + + // Test Succeeded with invalid block hash + assert_eq!( + TxStatus::from_str("Succeeded(123,0xinvalidhash)"), + Err("Invalid block hash".to_string()) + ); + + // Test unknown status + assert_eq!( + TxStatus::from_str("InProgress"), + Err("Unknown status: InProgress".to_string()) + ); + } + fn create_tx_receipt( status: Option, block_hash: Option, From 2352699784f19d3a554224a4a2e8148cb608c20c Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Mon, 21 Apr 2025 14:36:05 +0530 Subject: [PATCH 20/46] GH-608: implement retrieve_condition_display_works --- .../db_access_objects/sent_payable_dao.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 015628a6f..df3dfc038 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -46,7 +46,6 @@ pub enum RetrieveCondition { impl Display for RetrieveCondition { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - // TODO: GH-608: Test me separately match self { RetrieveCondition::IsPending => { write!(f, "WHERE status = 'Pending'") @@ -528,4 +527,22 @@ mod tests { assert_eq!(result, vec![tx3]); } + + #[test] + fn retrieve_condition_display_works() { + // Test IsPending + let is_pending = RetrieveCondition::IsPending; + assert_eq!(is_pending.to_string(), "WHERE status = 'Pending'"); + + // Test ToRetry + let to_retry = RetrieveCondition::ToRetry; + let expected_threshold = current_unix_timestamp() - RETRY_THRESHOLD_SECS; + + let result = to_retry.to_string(); + + let parts: Vec<&str> = result.split("<=").collect(); + let parsed_timestamp: i64 = parts[1].trim().parse().unwrap(); + assert_eq!(parts[0], "WHERE status = 'Pending' AND timestamp "); + assert!((parsed_timestamp - expected_threshold).abs() <= 1); // Allow a small tolerance for time differences during test execution + } } From 6d49640bd1533cd1ac057b0723b46e3312682fe2 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Mon, 21 Apr 2025 17:00:14 +0530 Subject: [PATCH 21/46] GH-608: update the query for txs_to_retry and reorder trait functions --- node/src/accountant/db_access_objects/mod.rs | 1 + .../db_access_objects/sent_payable_dao.rs | 163 ++++++------------ .../db_access_objects/test_utils.rs | 68 ++++++++ 3 files changed, 117 insertions(+), 115 deletions(-) create mode 100644 node/src/accountant/db_access_objects/test_utils.rs diff --git a/node/src/accountant/db_access_objects/mod.rs b/node/src/accountant/db_access_objects/mod.rs index ce727bea8..cf1ca4611 100644 --- a/node/src/accountant/db_access_objects/mod.rs +++ b/node/src/accountant/db_access_objects/mod.rs @@ -5,4 +5,5 @@ pub mod payable_dao; pub mod pending_payable_dao; pub mod receivable_dao; pub mod sent_payable_dao; +mod test_utils; pub mod utils; diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index df3dfc038..72e6649db 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -26,13 +26,13 @@ type TxIdentifiers = HashMap; #[derive(Clone, Debug, PartialEq, Eq)] pub struct Tx { - hash: H256, - receiver_address: Address, - amount: u128, - timestamp: i64, - gas_price_wei: u64, - nonce: u32, - status: TxStatus, + pub hash: H256, + pub receiver_address: Address, + pub amount: u128, + pub timestamp: i64, + pub gas_price_wei: u64, + pub nonce: u32, + pub status: TxStatus, } pub struct StatusChange { @@ -54,8 +54,7 @@ impl Display for RetrieveCondition { let threshold_timestamp = current_unix_timestamp() - RETRY_THRESHOLD_SECS; write!( f, - "WHERE status = 'Pending' AND timestamp <= {threshold_timestamp}", - // "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp <= {ten_minutes_ago}", // TODO: GH-608: Failed Txs should also be included here. Think... + "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp <= {threshold_timestamp}", ) } } @@ -65,12 +64,10 @@ impl Display for RetrieveCondition { pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; - fn retrieve_txs(&self, condition: Option) -> Vec; - fn retrieve_txs_to_retry(&self) -> Vec; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; - // TODO: GH-608: We might not need this - fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; + fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; } #[derive(Debug)] @@ -109,6 +106,36 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } + fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { + let sql = format!( + "INSERT INTO sent_payable (\ + tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status, retried\ + ) VALUES {}", + comma_joined_stringifiable(&txs, |tx| { + let amount_checked = checked_conversion::(tx.amount); + let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); + format!( + "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", + tx.hash, + tx.receiver_address, + high_bytes, + low_bytes, + tx.timestamp, + tx.gas_price_wei, + tx.nonce, + tx.status + ) + }) + ); + + match self.conn.prepare(&sql).expect("Internal error").execute([]) { + Ok(x) if x == txs.len() => Ok(()), + Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), + Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), + } + } + fn retrieve_txs(&self, condition_opt: Option) -> Vec { let raw_sql = "SELECT tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status FROM sent_payable" @@ -153,47 +180,13 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn retrieve_txs_to_retry(&self) -> Vec { + fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError> { todo!() } - fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { - let sql = format!( - "INSERT INTO sent_payable (\ - tx_hash, receiver_address, amount_high_b, amount_low_b, \ - timestamp, gas_price_wei, nonce, status, retried\ - ) VALUES {}", - comma_joined_stringifiable(&txs, |tx| { - let amount_checked = checked_conversion::(tx.amount); - let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); - format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", - tx.hash, - tx.receiver_address, - high_bytes, - low_bytes, - tx.timestamp, - tx.gas_price_wei, - tx.nonce, - tx.status - ) - }) - ); - - match self.conn.prepare(&sql).expect("Internal error").execute([]) { - Ok(x) if x == txs.len() => Ok(()), - Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), - Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), - } - } - fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { todo!() } - - fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError> { - todo!() - } } #[cfg(test)] @@ -208,72 +201,9 @@ mod tests { use ethereum_types::{Address, H256}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; + use crate::accountant::db_access_objects::test_utils::TxBuilder; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; - #[derive(Default)] - pub struct TxBuilder { - hash_opt: Option, - receiver_address_opt: Option
, - amount_opt: Option, - timestamp_opt: Option, - gas_price_wei_opt: Option, - nonce_opt: Option, - status_opt: Option, - } - - impl TxBuilder { - pub fn default() -> Self { - Default::default() - } - - pub fn hash(mut self, hash: H256) -> Self { - self.hash_opt = Some(hash); - self - } - - pub fn receiver_address(mut self, receiver_address: Address) -> Self { - self.receiver_address_opt = Some(receiver_address); - self - } - - pub fn amount(mut self, amount: u128) -> Self { - self.amount_opt = Some(amount); - self - } - - pub fn timestamp(mut self, timestamp: i64) -> Self { - self.timestamp_opt = Some(timestamp); - self - } - - pub fn gas_price_wei(mut self, gas_price_wei: u64) -> Self { - self.gas_price_wei_opt = Some(gas_price_wei); - self - } - - pub fn nonce(mut self, nonce: u32) -> Self { - self.nonce_opt = Some(nonce); - self - } - - pub fn status(mut self, status: TxStatus) -> Self { - self.status_opt = Some(status); - self - } - - pub fn build(self) -> Tx { - Tx { - hash: self.hash_opt.unwrap_or_default(), - receiver_address: self.receiver_address_opt.unwrap_or_default(), - amount: self.amount_opt.unwrap_or_default(), - timestamp: self.timestamp_opt.unwrap_or_else(current_unix_timestamp), - gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), - nonce: self.nonce_opt.unwrap_or_default(), - status: self.status_opt.unwrap_or(TxStatus::Pending), - } - } - } - #[test] fn constants_have_correct_values() { assert_eq!(RETRY_THRESHOLD_SECS, 10 * 60); @@ -520,12 +450,12 @@ mod tests { .status(TxStatus::Failed) .build(); subject - .insert_new_records(vec![tx1, tx2, tx3.clone(), tx4, tx5]) + .insert_new_records(vec![tx1, tx2, tx3.clone(), tx4, tx5.clone()]) .unwrap(); let result = subject.retrieve_txs(Some(RetrieveCondition::ToRetry)); - assert_eq!(result, vec![tx3]); + assert_eq!(result, vec![tx3, tx5]); } #[test] @@ -542,7 +472,10 @@ mod tests { let parts: Vec<&str> = result.split("<=").collect(); let parsed_timestamp: i64 = parts[1].trim().parse().unwrap(); - assert_eq!(parts[0], "WHERE status = 'Pending' AND timestamp "); + assert_eq!( + parts[0], + "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp " + ); assert!((parsed_timestamp - expected_threshold).abs() <= 1); // Allow a small tolerance for time differences during test execution } } diff --git a/node/src/accountant/db_access_objects/test_utils.rs b/node/src/accountant/db_access_objects/test_utils.rs new file mode 100644 index 000000000..9e078bd67 --- /dev/null +++ b/node/src/accountant/db_access_objects/test_utils.rs @@ -0,0 +1,68 @@ +use web3::types::{Address, H256}; +use crate::accountant::db_access_objects::sent_payable_dao::Tx; +use crate::accountant::db_access_objects::utils::current_unix_timestamp; +use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; + +#[derive(Default)] +pub struct TxBuilder { + hash_opt: Option, + receiver_address_opt: Option
, + amount_opt: Option, + timestamp_opt: Option, + gas_price_wei_opt: Option, + nonce_opt: Option, + status_opt: Option, +} + +impl TxBuilder { + pub fn default() -> Self { + Default::default() + } + + pub fn hash(mut self, hash: H256) -> Self { + self.hash_opt = Some(hash); + self + } + + pub fn receiver_address(mut self, receiver_address: Address) -> Self { + self.receiver_address_opt = Some(receiver_address); + self + } + + pub fn amount(mut self, amount: u128) -> Self { + self.amount_opt = Some(amount); + self + } + + pub fn timestamp(mut self, timestamp: i64) -> Self { + self.timestamp_opt = Some(timestamp); + self + } + + pub fn gas_price_wei(mut self, gas_price_wei: u64) -> Self { + self.gas_price_wei_opt = Some(gas_price_wei); + self + } + + pub fn nonce(mut self, nonce: u32) -> Self { + self.nonce_opt = Some(nonce); + self + } + + pub fn status(mut self, status: TxStatus) -> Self { + self.status_opt = Some(status); + self + } + + pub fn build(self) -> Tx { + Tx { + hash: self.hash_opt.unwrap_or_default(), + receiver_address: self.receiver_address_opt.unwrap_or_default(), + amount: self.amount_opt.unwrap_or_default(), + timestamp: self.timestamp_opt.unwrap_or_else(current_unix_timestamp), + gas_price_wei: self.gas_price_wei_opt.unwrap_or_default(), + nonce: self.nonce_opt.unwrap_or_default(), + status: self.status_opt.unwrap_or(TxStatus::Pending), + } + } +} From 7b1fefec892cbd05923c363c437ac50584a49918 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 24 Apr 2025 12:48:52 +0530 Subject: [PATCH 22/46] GH-608: change SQL query for txs to retry --- .../db_access_objects/sent_payable_dao.rs | 47 ++++--------------- 1 file changed, 9 insertions(+), 38 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 72e6649db..d98b0a5cd 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -52,10 +52,7 @@ impl Display for RetrieveCondition { } RetrieveCondition::ToRetry => { let threshold_timestamp = current_unix_timestamp() - RETRY_THRESHOLD_SECS; - write!( - f, - "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp <= {threshold_timestamp}", - ) + write!(f, "WHERE status = 'Failed'") } } } @@ -201,6 +198,7 @@ mod tests { use ethereum_types::{Address, H256}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; + use crate::accountant::db_access_objects::sent_payable_dao::RetrieveCondition::{IsPending, ToRetry}; use crate::accountant::db_access_objects::test_utils::TxBuilder; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; @@ -418,25 +416,13 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); - let current_unix_timestamp = current_unix_timestamp(); - let old_but_not_that_old = current_unix_timestamp - 60; // 1 minute old - let old_timestamp = current_unix_timestamp - 600; // 10 minutes old + let old_timestamp = current_unix_timestamp() - 60; // 1 minute old let tx1 = TxBuilder::default() - .hash(H256::from_low_u64_le(1)) - .timestamp(current_unix_timestamp) - .status(TxStatus::Pending) - .build(); - let tx2 = TxBuilder::default() - .hash(H256::from_low_u64_le(2)) - .timestamp(old_but_not_that_old) - .status(TxStatus::Pending) - .build(); - let tx3 = TxBuilder::default() // this should be picked for retry .hash(H256::from_low_u64_le(3)) .timestamp(old_timestamp) .status(TxStatus::Pending) .build(); - let tx4 = TxBuilder::default() + let tx2 = TxBuilder::default() .hash(H256::from_low_u64_le(4)) .timestamp(old_timestamp) .status(TxStatus::Succeeded(TransactionBlock { @@ -444,38 +430,23 @@ mod tests { block_number: Default::default(), })) .build(); - let tx5 = TxBuilder::default() + let tx3 = TxBuilder::default() // this should be picked for retry .hash(H256::from_low_u64_le(5)) .timestamp(old_timestamp) .status(TxStatus::Failed) .build(); subject - .insert_new_records(vec![tx1, tx2, tx3.clone(), tx4, tx5.clone()]) + .insert_new_records(vec![tx1, tx2, tx3.clone()]) .unwrap(); let result = subject.retrieve_txs(Some(RetrieveCondition::ToRetry)); - assert_eq!(result, vec![tx3, tx5]); + assert_eq!(result, vec![tx3]); } #[test] fn retrieve_condition_display_works() { - // Test IsPending - let is_pending = RetrieveCondition::IsPending; - assert_eq!(is_pending.to_string(), "WHERE status = 'Pending'"); - - // Test ToRetry - let to_retry = RetrieveCondition::ToRetry; - let expected_threshold = current_unix_timestamp() - RETRY_THRESHOLD_SECS; - - let result = to_retry.to_string(); - - let parts: Vec<&str> = result.split("<=").collect(); - let parsed_timestamp: i64 = parts[1].trim().parse().unwrap(); - assert_eq!( - parts[0], - "WHERE (status = 'Pending' OR status = 'Failed') AND timestamp " - ); - assert!((parsed_timestamp - expected_threshold).abs() <= 1); // Allow a small tolerance for time differences during test execution + assert_eq!(IsPending.to_string(), "WHERE status = 'Pending'"); + assert_eq!(ToRetry.to_string(), "WHERE status = 'Failed'"); } } From a953c37bb185efc42b5e1616f18c1d9275c8477d Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 24 Apr 2025 12:50:22 +0530 Subject: [PATCH 23/46] GH-608: remove RETRY_THRESHOLD_SECS --- .../accountant/db_access_objects/sent_payable_dao.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index d98b0a5cd..8fd52ae07 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -5,13 +5,10 @@ use ethereum_types::H256; use web3::types::Address; use masq_lib::utils::ExpectValue; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; -use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::TxStatus; use crate::database::rusqlite_wrappers::ConnectionWrapper; -const RETRY_THRESHOLD_SECS: i64 = 10 * 60; // 10 minutes - #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { InsertionFailed(String), @@ -51,7 +48,6 @@ impl Display for RetrieveCondition { write!(f, "WHERE status = 'Pending'") } RetrieveCondition::ToRetry => { - let threshold_timestamp = current_unix_timestamp() - RETRY_THRESHOLD_SECS; write!(f, "WHERE status = 'Failed'") } } @@ -188,7 +184,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal, Tx, RETRY_THRESHOLD_SECS}; + use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal}; use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, @@ -202,11 +198,6 @@ mod tests { use crate::accountant::db_access_objects::test_utils::TxBuilder; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; - #[test] - fn constants_have_correct_values() { - assert_eq!(RETRY_THRESHOLD_SECS, 10 * 60); - } - #[test] fn insert_new_records_works() { let home_dir = From 918fc8ff46c55e442ab782300f4a4ed0a1f48654 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 12:20:46 +0530 Subject: [PATCH 24/46] GH-608: remove retried column from the sent_payable table --- node/src/accountant/db_access_objects/sent_payable_dao.rs | 4 ++-- node/src/database/db_initializer.rs | 5 ++--- .../database/db_migrations/migrations/migration_10_to_11.rs | 3 +-- node/src/database/test_utils/mod.rs | 2 -- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 8fd52ae07..ba446bc17 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -103,13 +103,13 @@ impl SentPayableDao for SentPayableDaoReal<'_> { let sql = format!( "INSERT INTO sent_payable (\ tx_hash, receiver_address, amount_high_b, amount_low_b, \ - timestamp, gas_price_wei, nonce, status, retried\ + timestamp, gas_price_wei, nonce, status ) VALUES {}", comma_joined_stringifiable(&txs, |tx| { let amount_checked = checked_conversion::(tx.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}', 0)", + "('{:?}', '{:?}', {}, {}, {}, {}, {}, '{}')", tx.hash, tx.receiver_address, high_bytes, diff --git a/node/src/database/db_initializer.rs b/node/src/database/db_initializer.rs index 0f9685c2b..6e0090f0d 100644 --- a/node/src/database/db_initializer.rs +++ b/node/src/database/db_initializer.rs @@ -270,8 +270,7 @@ impl DbInitializerReal { timestamp integer not null, gas_price_wei integer not null, nonce integer not null, - status text not null, - retried integer not null + status text not null )", [], ) @@ -752,7 +751,7 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let mut stmt = conn.prepare("select rowid, tx_hash, receiver_address, amount_high_b, amount_low_b, timestamp, gas_price_wei, nonce, status, retried from sent_payable").unwrap(); + let mut stmt = conn.prepare("select rowid, tx_hash, receiver_address, amount_high_b, amount_low_b, timestamp, gas_price_wei, nonce, status from sent_payable").unwrap(); let mut sent_payable_contents = stmt.query_map([], |_| Ok(42)).unwrap(); assert!(sent_payable_contents.next().is_none()); assert_create_table_stm_contains_all_parts( diff --git a/node/src/database/db_migrations/migrations/migration_10_to_11.rs b/node/src/database/db_migrations/migrations/migration_10_to_11.rs index 4b683208d..8b7984673 100644 --- a/node/src/database/db_migrations/migrations/migration_10_to_11.rs +++ b/node/src/database/db_migrations/migrations/migration_10_to_11.rs @@ -18,8 +18,7 @@ impl DatabaseMigration for Migrate_10_to_11 { timestamp integer not null, gas_price_wei integer not null, nonce integer not null, - status text not null, - retried integer not null + status text not null )"; declaration_utils.execute_upon_transaction(&[&sql_statement]) diff --git a/node/src/database/test_utils/mod.rs b/node/src/database/test_utils/mod.rs index a9c5fac77..4251d1588 100644 --- a/node/src/database/test_utils/mod.rs +++ b/node/src/database/test_utils/mod.rs @@ -22,7 +22,6 @@ pub const SQL_ATTRIBUTES_FOR_CREATING_SENT_PAYABLE: &[&[&str]] = &[ &["gas_price_wei", "integer", "not", "null"], &["nonce", "integer", "not", "null"], &["status", "text", "not", "null"], - &["retried", "integer", "not", "null"], ]; #[derive(Debug, Default)] @@ -146,7 +145,6 @@ mod tests { &["gas_price_wei", "integer", "not", "null"], &["nonce", "integer", "not", "null"], &["status", "text", "not", "null"], - &["retried", "integer", "not", "null"] ] ); } From 1514096e8b7d25578cdc9ad75670580618763b0b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 13:29:42 +0530 Subject: [PATCH 25/46] GH-608: add ability to retrieve tx by hash --- .../db_access_objects/sent_payable_dao.rs | 88 ++++++++++++++++++- 1 file changed, 84 insertions(+), 4 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index ba446bc17..0926fc570 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -20,6 +20,7 @@ pub enum SentPayableDaoError { } type TxIdentifiers = HashMap; +type TxUpdates = HashMap; #[derive(Clone, Debug, PartialEq, Eq)] pub struct Tx { @@ -39,6 +40,7 @@ pub struct StatusChange { pub enum RetrieveCondition { IsPending, ToRetry, + ByHash(H256), } impl Display for RetrieveCondition { @@ -50,6 +52,9 @@ impl Display for RetrieveCondition { RetrieveCondition::ToRetry => { write!(f, "WHERE status = 'Failed'") } + RetrieveCondition::ByHash(tx_hash) => { + write!(f, "WHERE tx_hash = '{:?}'", tx_hash) + } } } } @@ -59,7 +64,7 @@ pub trait SentPayableDao { fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; - fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError>; + fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; } @@ -173,7 +178,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn change_statuses(&self, ids: &[StatusChange]) -> Result<(), SentPayableDaoError> { + fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError> { todo!() } @@ -184,6 +189,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { + use std::collections::HashMap; use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal}; use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ @@ -191,10 +197,11 @@ mod tests { }; use crate::database::rusqlite_wrappers::ConnectionWrapperReal; use crate::database::test_utils::ConnectionWrapperMock; - use ethereum_types::{Address, H256}; + use ethereum_types::{Address, H256, U64}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; - use crate::accountant::db_access_objects::sent_payable_dao::RetrieveCondition::{IsPending, ToRetry}; + use web3::types::U256; + use crate::accountant::db_access_objects::sent_payable_dao::RetrieveCondition::{ByHash, IsPending, ToRetry}; use crate::accountant::db_access_objects::test_utils::TxBuilder; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; @@ -439,5 +446,78 @@ mod tests { fn retrieve_condition_display_works() { assert_eq!(IsPending.to_string(), "WHERE status = 'Pending'"); assert_eq!(ToRetry.to_string(), "WHERE status = 'Failed'"); + assert_eq!( + ByHash(H256::default()).to_string(), + format!("WHERE tx_hash = '{:?}'", H256::default()) + ); + } + + #[test] + fn tx_can_be_retrieved_by_hash() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "tx_can_be_retrieved_by_hash"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Failed) + .build(); + subject + .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .unwrap(); + + let result = subject.retrieve_txs(Some(ByHash(tx1.hash))); + + assert_eq!(result, vec![tx1]); + } + + #[test] + fn change_statuses_works() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "change_statuses_works"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Pending) + .build(); + subject + .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .unwrap(); + let hash_map = HashMap::from([ + (tx1.hash, TxStatus::Failed), + ( + tx2.hash, + TxStatus::Succeeded(TransactionBlock { + block_hash: H256::from_low_u64_le(3), + block_number: U64::from(1), + }), + ), + ]); + + let result = subject.change_statuses(&hash_map); + + let tx1_updated = subject.retrieve_txs(Some(ByHash(tx1.hash))).pop().unwrap(); + let tx2_updated = subject.retrieve_txs(Some(ByHash(tx2.hash))).pop().unwrap(); + assert_eq!(tx1_updated.status, TxStatus::Failed); + assert_eq!( + tx2_updated.status, + TxStatus::Succeeded(TransactionBlock { + block_hash: H256::from_low_u64_le(3), + block_number: U64::from(1), + }) + ) } } From 9fade8047322673e7fecd757c9f26d95a9a45b69 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 15:47:54 +0530 Subject: [PATCH 26/46] GH-608: add the ability to update statuses --- .../db_access_objects/sent_payable_dao.rs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 0926fc570..eec81b581 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -12,7 +12,7 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { InsertionFailed(String), - // UpdateFailed(String), + UpdateFailed(String), // SignConversionError(u64), // RecordCannotBeRead, // RecordDeletion(String), @@ -179,7 +179,25 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError> { - todo!() + for (hash, status) in hash_map { + let sql = format!( + "UPDATE sent_payable SET status = '{}' WHERE tx_hash = '{:?}'", + status, hash + ); + + match self.conn.prepare(&sql).expect("Internal error").execute([]) { + Ok(updated) if updated == 1 => continue, + Ok(_) => { + return Err(SentPayableDaoError::UpdateFailed(format!( + "Failed to update status for hash {:?}", + hash + ))) + } + Err(e) => return Err(SentPayableDaoError::UpdateFailed(e.to_string())), + } + } + + Ok(()) } fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { From b27620acf4b3697d49d5dfd08b384c4442be48c1 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 15:55:03 +0530 Subject: [PATCH 27/46] GH-608: add types for TxHash and RowID --- .../db_access_objects/sent_payable_dao.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index eec81b581..579f0d639 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -19,12 +19,15 @@ pub enum SentPayableDaoError { // ErrorMarkFailed(String), } -type TxIdentifiers = HashMap; -type TxUpdates = HashMap; +type TxHash = H256; +type RowID = u64; + +type TxIdentifiers = HashMap; +type TxUpdates = HashMap; #[derive(Clone, Debug, PartialEq, Eq)] pub struct Tx { - pub hash: H256, + pub hash: TxHash, pub receiver_address: Address, pub amount: u128, pub timestamp: i64, @@ -33,14 +36,10 @@ pub struct Tx { pub status: TxStatus, } -pub struct StatusChange { - new_status: TxStatus, -} - pub enum RetrieveCondition { IsPending, ToRetry, - ByHash(H256), + ByHash(TxHash), } impl Display for RetrieveCondition { @@ -61,7 +60,7 @@ impl Display for RetrieveCondition { pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed - fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers; + fn get_tx_identifiers(&self, hashes: &[TxHash]) -> TxIdentifiers; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; @@ -81,7 +80,7 @@ impl<'a> SentPayableDaoReal<'a> { } impl SentPayableDao for SentPayableDaoReal<'_> { - fn get_tx_identifiers(&self, hashes: &[H256]) -> TxIdentifiers { + fn get_tx_identifiers(&self, hashes: &[TxHash]) -> TxIdentifiers { let sql = format!( "SELECT tx_hash, rowid FROM sent_payable WHERE tx_hash IN ({})", comma_joined_stringifiable(hashes, |hash| format!("'{:?}'", hash)) From c63c6f5ebc7f7e57d6f69d533ed211fc6b9606e8 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 16:02:14 +0530 Subject: [PATCH 28/46] GH-608: add test for deleting records --- .../db_access_objects/sent_payable_dao.rs | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 579f0d639..f6abfdec2 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -64,7 +64,7 @@ pub trait SentPayableDao { fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; - fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError>; } #[derive(Debug)] @@ -199,7 +199,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { Ok(()) } - fn delete_records(&self, ids: &[u64]) -> Result<(), SentPayableDaoError> { + fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError> { todo!() } } @@ -537,4 +537,36 @@ mod tests { }) ) } + + #[test] + fn txs_can_be_deleted() { + let home_dir = ensure_node_home_directory_exists("sent_payable_dao", "txs_can_be_deleted"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Failed) + .build(); + let tx3 = TxBuilder::default() + .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); + subject + .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .unwrap(); + + let result = subject.delete_records(&vec![tx1.hash, tx2.hash]); + + let remaining_records = subject.retrieve_txs(None); + assert_eq!(remaining_records, vec![tx3]); + } } From 9d42af157b84425738da8a9ee601f8fa2d35246c Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 16:15:27 +0530 Subject: [PATCH 29/46] GH-608: add fn for deleting records --- .../db_access_objects/sent_payable_dao.rs | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index f6abfdec2..7b3535c44 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -13,10 +13,7 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; pub enum SentPayableDaoError { InsertionFailed(String), UpdateFailed(String), - // SignConversionError(u64), - // RecordCannotBeRead, - // RecordDeletion(String), - // ErrorMarkFailed(String), + DeletionFailed(String), } type TxHash = H256; @@ -200,7 +197,21 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError> { - todo!() + if hashes.is_empty() { + return Ok(()); + } + + let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); + let hash_list = hash_strings.join(", "); + + let sql = format!("DELETE FROM sent_payable WHERE tx_hash IN ({})", hash_list); + + self.conn + .prepare(&sql) + .and_then(|mut stmt| stmt.execute([])) + .map_err(|e| SentPayableDaoError::DeletionFailed(e.to_string()))?; + + Ok(()) } } @@ -561,7 +572,7 @@ mod tests { })) .build(); subject - .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3.clone()]) .unwrap(); let result = subject.delete_records(&vec![tx1.hash, tx2.hash]); From f4e92dfa43e741839c18a85176917c4e868d4784 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 25 Apr 2025 17:09:38 +0530 Subject: [PATCH 30/46] GH-608: add more TODOs --- node/src/accountant/db_access_objects/sent_payable_dao.rs | 4 ++-- node/src/database/db_migrations/db_migrator.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 7b3535c44..ffad746ea 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -12,8 +12,8 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { InsertionFailed(String), - UpdateFailed(String), - DeletionFailed(String), + UpdateFailed(String), // TODO: GH-608: Test this error + DeletionFailed(String), // TODO: GH-608: Test this error } type TxHash = H256; diff --git a/node/src/database/db_migrations/db_migrator.rs b/node/src/database/db_migrations/db_migrator.rs index 369a78788..6cae9599a 100644 --- a/node/src/database/db_migrations/db_migrator.rs +++ b/node/src/database/db_migrations/db_migrator.rs @@ -81,7 +81,7 @@ impl DbMigratorReal { &Migrate_7_to_8, &Migrate_8_to_9, &Migrate_9_to_10, - &Migrate_10_to_11, + &Migrate_10_to_11, // TODO: GH-598: Make this one as null migration and yours as 12 ] } From 5eab47959dc5eaf0c94f86cde067632dfc3b0fd1 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 29 Apr 2025 17:38:41 +0530 Subject: [PATCH 31/46] GH-608: error testing --- .../db_access_objects/sent_payable_dao.rs | 99 ++++++++++++++++++- 1 file changed, 97 insertions(+), 2 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index ffad746ea..3b6e69aeb 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -187,9 +187,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { return Err(SentPayableDaoError::UpdateFailed(format!( "Failed to update status for hash {:?}", hash - ))) + ))); + } + Err(e) => { + return Err(SentPayableDaoError::UpdateFailed(e.to_string())); } - Err(e) => return Err(SentPayableDaoError::UpdateFailed(e.to_string())), } } @@ -580,4 +582,97 @@ mod tests { let remaining_records = subject.retrieve_txs(None); assert_eq!(remaining_records, vec![tx3]); } + + #[test] + fn change_statuses_returns_update_failed_error_when_update_fails() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "change_statuses_returns_update_failed_error_when_update_fails", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + subject.insert_new_records(vec![tx.clone()]).unwrap(); + + // Create a hash that doesn't exist in the database + let non_existent_hash = H256::from_low_u64_le(999); + let hash_map = HashMap::from([(non_existent_hash, TxStatus::Failed)]); + + let result = subject.change_statuses(&hash_map); + + assert_eq!( + result, + Err(SentPayableDaoError::UpdateFailed(format!( + "Failed to update status for hash {:?}", + non_existent_hash + ))) + ); + } + + #[test] + fn change_statuses_returns_update_failed_error_when_an_error_occurs_in_sql() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "change_statuses_can_throw_error", + ); + { + DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + } + let read_only_conn = Connection::open_with_flags( + home_dir.join(DATABASE_FILE), + OpenFlags::SQLITE_OPEN_READ_ONLY, + ) + .unwrap(); + let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + + let hash = H256::from_low_u64_le(1); + let hash_map = HashMap::from([(hash, TxStatus::Failed)]); + + let result = subject.change_statuses(&hash_map); + + assert_eq!( + result, + Err(SentPayableDaoError::UpdateFailed( + "attempt to write a readonly database".to_string() + )) + ) + } + + #[test] + fn delete_records_returns_deletion_failed_error_when_delete_fails() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "delete_records_returns_deletion_failed_error_when_delete_fails", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let mut subject = SentPayableDaoReal::new(wrapped_conn); + + // Replace the connection with a mock that always fails + let failing_conn = + ConnectionWrapperMock::default().prepare_result(Err(rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(1), + Some("Mock deletion error".to_string()), + ))); + subject.conn = Box::new(failing_conn); + + let hash = H256::from_low_u64_le(1); + let result = subject.delete_records(&[hash]); + + assert_eq!( + result, + Err(SentPayableDaoError::DeletionFailed( + "Mock deletion error".to_string() + )) + ); + } } From b7d35d88a1f43321c1a68628e849ef6c4c790e3a Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 30 Apr 2025 17:54:10 +0530 Subject: [PATCH 32/46] GH-608: write tests for error handling while deleting records --- .../db_access_objects/sent_payable_dao.rs | 141 +++++++++++------- 1 file changed, 84 insertions(+), 57 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 3b6e69aeb..61195b146 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -12,8 +12,9 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { InsertionFailed(String), - UpdateFailed(String), // TODO: GH-608: Test this error - DeletionFailed(String), // TODO: GH-608: Test this error + UpdateFailed(String), + DeletionFailed(String), + SqlExecutionError(String), } type TxHash = H256; @@ -125,8 +126,8 @@ impl SentPayableDao for SentPayableDaoReal<'_> { match self.conn.prepare(&sql).expect("Internal error").execute([]) { Ok(x) if x == txs.len() => Ok(()), - Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), - Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), + Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), // TODO: GH-608: This should be an error + Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), // TODO: THis should be a panic } } @@ -188,9 +189,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { "Failed to update status for hash {:?}", hash ))); + // TODO: GH-608: This should be inside if else } Err(e) => { return Err(SentPayableDaoError::UpdateFailed(e.to_string())); + // TODO: GH-608: This should be a panic } } } @@ -200,7 +203,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError> { if hashes.is_empty() { - return Ok(()); + return Ok(()); // TDOD: GH-608: Test this, it should be an error } let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); @@ -208,12 +211,20 @@ impl SentPayableDao for SentPayableDaoReal<'_> { let sql = format!("DELETE FROM sent_payable WHERE tx_hash IN ({})", hash_list); - self.conn - .prepare(&sql) - .and_then(|mut stmt| stmt.execute([])) - .map_err(|e| SentPayableDaoError::DeletionFailed(e.to_string()))?; - - Ok(()) + match self.conn.prepare(&sql).expect("Internal error").execute([]) { + Ok(deleted_rows) => { + if deleted_rows >= 1 { + Ok(()) + } else { + Err(SentPayableDaoError::DeletionFailed( + "No records were deleted for the given hashes".to_string(), + )) + } + } + Err(e) => { + panic!("SQL execution failed on record deletion: {}", e); + } + } } } @@ -551,38 +562,6 @@ mod tests { ) } - #[test] - fn txs_can_be_deleted() { - let home_dir = ensure_node_home_directory_exists("sent_payable_dao", "txs_can_be_deleted"); - let wrapped_conn = DbInitializerReal::default() - .initialize(&home_dir, DbInitializationConfig::test_default()) - .unwrap(); - let subject = SentPayableDaoReal::new(wrapped_conn); - let tx1 = TxBuilder::default() - .hash(H256::from_low_u64_le(1)) - .status(TxStatus::Pending) - .build(); - let tx2 = TxBuilder::default() - .hash(H256::from_low_u64_le(2)) - .status(TxStatus::Failed) - .build(); - let tx3 = TxBuilder::default() - .hash(H256::from_low_u64_le(3)) - .status(TxStatus::Succeeded(TransactionBlock { - block_hash: Default::default(), - block_number: Default::default(), - })) - .build(); - subject - .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3.clone()]) - .unwrap(); - - let result = subject.delete_records(&vec![tx1.hash, tx2.hash]); - - let remaining_records = subject.retrieve_txs(None); - assert_eq!(remaining_records, vec![tx3]); - } - #[test] fn change_statuses_returns_update_failed_error_when_update_fails() { let home_dir = ensure_node_home_directory_exists( @@ -647,32 +626,80 @@ mod tests { } #[test] - fn delete_records_returns_deletion_failed_error_when_delete_fails() { + fn txs_can_be_deleted() { + let home_dir = ensure_node_home_directory_exists("sent_payable_dao", "txs_can_be_deleted"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Failed) + .build(); + let tx3 = TxBuilder::default() + .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); + subject + .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3.clone()]) + .unwrap(); + + let result = subject.delete_records(&vec![tx1.hash, tx2.hash]); + + let remaining_records = subject.retrieve_txs(None); + assert_eq!(remaining_records, vec![tx3]); + } + + #[test] + fn delete_records_returns_error_when_no_records_are_deleted() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "delete_records_returns_deletion_failed_error_when_delete_fails", + "delete_records_returns_error_when_no_records_are_deleted", ); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); - let mut subject = SentPayableDaoReal::new(wrapped_conn); - - // Replace the connection with a mock that always fails - let failing_conn = - ConnectionWrapperMock::default().prepare_result(Err(rusqlite::Error::SqliteFailure( - rusqlite::ffi::Error::new(1), - Some("Mock deletion error".to_string()), - ))); - subject.conn = Box::new(failing_conn); + let subject = SentPayableDaoReal::new(wrapped_conn); + let non_existent_hash = H256::from_low_u64_le(999); - let hash = H256::from_low_u64_le(1); - let result = subject.delete_records(&[hash]); + let result = subject.delete_records(&[non_existent_hash]); assert_eq!( result, Err(SentPayableDaoError::DeletionFailed( - "Mock deletion error".to_string() + "No records were deleted for the given hashes".to_string() )) ); } + + #[test] + #[should_panic( + expected = "SQL execution failed on record deletion: attempt to write a readonly database" + )] + fn delete_records_returns_deletion_failed_error_when_an_error_occurs_in_sql() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "delete_records_can_throw_error"); + { + DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + } + let read_only_conn = Connection::open_with_flags( + home_dir.join(DATABASE_FILE), + OpenFlags::SQLITE_OPEN_READ_ONLY, + ) + .unwrap(); + let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); + let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); + let hashes = vec![H256::from_low_u64_le(1)]; + + let _ = subject.delete_records(&hashes); + } } From b164cf912ef8562a94cb91d74f30ab6fc168fcb5 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Wed, 30 Apr 2025 18:17:22 +0530 Subject: [PATCH 33/46] GH-608: add more TODOs --- .../db_access_objects/sent_payable_dao.rs | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 61195b146..c19c0e9eb 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -14,7 +14,10 @@ pub enum SentPayableDaoError { InsertionFailed(String), UpdateFailed(String), DeletionFailed(String), - SqlExecutionError(String), + SqlExecutionFailed(String), + // InvalidInput(String), + // PartialExecution(String), + // NoChange(String), // TODO: GH-608: Introduce the commented variants, and remove the first three } type TxHash = H256; @@ -75,6 +78,9 @@ impl<'a> SentPayableDaoReal<'a> { // TODO: GH-608: You'll need to write a mock to test this, do that wisely Self { conn } } + + // TODO: GH-608: There should be a function for executing SQL + // TODO: GH-608: There should be a function for handling database errors } impl SentPayableDao for SentPayableDaoReal<'_> { @@ -213,17 +219,22 @@ impl SentPayableDao for SentPayableDaoReal<'_> { match self.conn.prepare(&sql).expect("Internal error").execute([]) { Ok(deleted_rows) => { - if deleted_rows >= 1 { + if deleted_rows == hashes.len() { Ok(()) - } else { + } else if deleted_rows == 0 { Err(SentPayableDaoError::DeletionFailed( "No records were deleted for the given hashes".to_string(), )) + } else { + todo!("test me"); + // Err(SentPayableDaoError::PartialDeletion(format!( + // "Expected to delete {} records, but only {} were deleted", + // hashes.len(), + // deleted_rows + // ))) } } - Err(e) => { - panic!("SQL execution failed on record deletion: {}", e); - } + Err(e) => Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())), } } } @@ -680,9 +691,6 @@ mod tests { } #[test] - #[should_panic( - expected = "SQL execution failed on record deletion: attempt to write a readonly database" - )] fn delete_records_returns_deletion_failed_error_when_an_error_occurs_in_sql() { let home_dir = ensure_node_home_directory_exists("sent_payable_dao", "delete_records_can_throw_error"); @@ -700,6 +708,13 @@ mod tests { let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let hashes = vec![H256::from_low_u64_le(1)]; - let _ = subject.delete_records(&hashes); + let result = subject.delete_records(&hashes); + + assert_eq!( + result, + Err(SentPayableDaoError::SqlExecutionFailed( + "attempt to write a readonly database".to_string() + )) + ) } } From 8d5cc0a3849a6a6e93106797809f7d013fd8be9a Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 1 May 2025 13:36:15 +0530 Subject: [PATCH 34/46] GH-608: return SqlExecutionFailed in insert, change_status and delete operations --- .../db_access_objects/sent_payable_dao.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index c19c0e9eb..bf0c70c0d 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -133,7 +133,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { match self.conn.prepare(&sql).expect("Internal error").execute([]) { Ok(x) if x == txs.len() => Ok(()), Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), // TODO: GH-608: This should be an error - Err(e) => Err(SentPayableDaoError::InsertionFailed(e.to_string())), // TODO: THis should be a panic + Err(e) => Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())), } } @@ -198,8 +198,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { // TODO: GH-608: This should be inside if else } Err(e) => { - return Err(SentPayableDaoError::UpdateFailed(e.to_string())); - // TODO: GH-608: This should be a panic + return Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())); } } } @@ -314,7 +313,7 @@ mod tests { assert_eq!( result, - Err(SentPayableDaoError::InsertionFailed( + Err(SentPayableDaoError::SqlExecutionFailed( "UNIQUE constraint failed: sent_payable.tx_hash".to_string() )) ); @@ -346,7 +345,7 @@ mod tests { assert_eq!(initial_insertion_result, Ok(())); assert_eq!( result, - Err(SentPayableDaoError::InsertionFailed( + Err(SentPayableDaoError::SqlExecutionFailed( "UNIQUE constraint failed: sent_payable.tx_hash".to_string() )) ); @@ -394,7 +393,7 @@ mod tests { assert_eq!( result, - Err(SentPayableDaoError::InsertionFailed( + Err(SentPayableDaoError::SqlExecutionFailed( "attempt to write a readonly database".to_string() )) ) @@ -605,10 +604,10 @@ mod tests { } #[test] - fn change_statuses_returns_update_failed_error_when_an_error_occurs_in_sql() { + fn change_statuses_returns_error_when_an_error_occurs_while_executing_sql() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "change_statuses_can_throw_error", + "change_statuses_returns_error_when_an_error_occurs_while_executing_sql", ); { DbInitializerReal::default() @@ -630,7 +629,7 @@ mod tests { assert_eq!( result, - Err(SentPayableDaoError::UpdateFailed( + Err(SentPayableDaoError::SqlExecutionFailed( "attempt to write a readonly database".to_string() )) ) From 10ab1139dab9d31cda670050d103851f0952b594 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 1 May 2025 13:58:10 +0530 Subject: [PATCH 35/46] GH-608: add better error handling for delete records --- .../db_access_objects/sent_payable_dao.rs | 80 +++++++++++++++---- 1 file changed, 66 insertions(+), 14 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index bf0c70c0d..fbf0d22b4 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -11,13 +11,14 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { + // TODO: GH-608: Remove the first three InsertionFailed(String), UpdateFailed(String), DeletionFailed(String), SqlExecutionFailed(String), - // InvalidInput(String), - // PartialExecution(String), - // NoChange(String), // TODO: GH-608: Introduce the commented variants, and remove the first three + InvalidInput(String), + PartialExecution(String), + NoChange(String), } type TxHash = H256; @@ -208,8 +209,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError> { if hashes.is_empty() { - return Ok(()); // TDOD: GH-608: Test this, it should be an error + return Err(SentPayableDaoError::InvalidInput( + "No hashes were given.".to_string(), + )); } + // TODO: GH-608: Should we check for unique hashes before deletion, or should we use a hashset instead? I think prevention is better than cure let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); let hash_list = hash_strings.join(", "); @@ -221,16 +225,15 @@ impl SentPayableDao for SentPayableDaoReal<'_> { if deleted_rows == hashes.len() { Ok(()) } else if deleted_rows == 0 { - Err(SentPayableDaoError::DeletionFailed( - "No records were deleted for the given hashes".to_string(), + Err(SentPayableDaoError::NoChange( + "No records were deleted for the specified hashes.".to_string(), )) } else { - todo!("test me"); - // Err(SentPayableDaoError::PartialDeletion(format!( - // "Expected to delete {} records, but only {} were deleted", - // hashes.len(), - // deleted_rows - // ))) + Err(SentPayableDaoError::PartialExecution(format!( + "Only {} of the {} hashes has been deleted.", + deleted_rows, + hashes.len(), + ))) } } Err(e) => Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())), @@ -667,6 +670,27 @@ mod tests { assert_eq!(remaining_records, vec![tx3]); } + #[test] + fn delete_records_returns_error_when_input_records_are_invalid() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "delete_records_returns_error_when_input_records_are_invalid", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + + let result = subject.delete_records(&[]); + + assert_eq!( + result, + Err(SentPayableDaoError::InvalidInput( + "No hashes were given.".to_string() + )) + ); + } + #[test] fn delete_records_returns_error_when_no_records_are_deleted() { let home_dir = ensure_node_home_directory_exists( @@ -683,12 +707,40 @@ mod tests { assert_eq!( result, - Err(SentPayableDaoError::DeletionFailed( - "No records were deleted for the given hashes".to_string() + Err(SentPayableDaoError::NoChange( + "No records were deleted for the specified hashes.".to_string() )) ); } + #[test] + fn delete_records_returns_error_when_not_all_input_records_were_deleted() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "delete_records_returns_error_when_not_all_input_records_were_deleted", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let present_hash = H256::from_low_u64_le(1); + let absent_hash = H256::from_low_u64_le(1); + let tx = TxBuilder::default() + .hash(present_hash) + .status(TxStatus::Failed) + .build(); + subject.insert_new_records(vec![tx]); + + let result = subject.delete_records(&[present_hash, absent_hash]); + + assert_eq!( + result, + Err(SentPayableDaoError::PartialExecution(format!( + "Only 1 of the 2 hashes has been deleted." + ))) + ); + } + #[test] fn delete_records_returns_deletion_failed_error_when_an_error_occurs_in_sql() { let home_dir = From 875da42d0f30ec317fe83a593bad7e84e3739573 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 1 May 2025 16:26:26 +0530 Subject: [PATCH 36/46] GH-608: change the signature of delete_records() to accept Hashset --- .../db_access_objects/sent_payable_dao.rs | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index fbf0d22b4..bd6fbeeba 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt::{Display, Formatter}; use std::str::FromStr; use ethereum_types::H256; @@ -66,7 +66,7 @@ pub trait SentPayableDao { fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; - fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, hashes: HashSet) -> Result<(), SentPayableDaoError>; } #[derive(Debug)] @@ -207,13 +207,12 @@ impl SentPayableDao for SentPayableDaoReal<'_> { Ok(()) } - fn delete_records(&self, hashes: &[TxHash]) -> Result<(), SentPayableDaoError> { + fn delete_records(&self, hashes: HashSet) -> Result<(), SentPayableDaoError> { if hashes.is_empty() { return Err(SentPayableDaoError::InvalidInput( "No hashes were given.".to_string(), )); } - // TODO: GH-608: Should we check for unique hashes before deletion, or should we use a hashset instead? I think prevention is better than cure let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); let hash_list = hash_strings.join(", "); @@ -243,7 +242,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::collections::{HashMap, HashSet}; use crate::accountant::db_access_objects::sent_payable_dao::{RetrieveCondition, SentPayableDao, SentPayableDaoError, SentPayableDaoReal}; use crate::accountant::db_access_objects::utils::current_unix_timestamp; use crate::database::db_initializer::{ @@ -663,8 +662,9 @@ mod tests { subject .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3.clone()]) .unwrap(); + let hashset = HashSet::from([tx1.hash, tx2.hash]); - let result = subject.delete_records(&vec![tx1.hash, tx2.hash]); + let result = subject.delete_records(hashset); let remaining_records = subject.retrieve_txs(None); assert_eq!(remaining_records, vec![tx3]); @@ -681,7 +681,7 @@ mod tests { .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); - let result = subject.delete_records(&[]); + let result = subject.delete_records(HashSet::new()); assert_eq!( result, @@ -702,8 +702,9 @@ mod tests { .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); let non_existent_hash = H256::from_low_u64_le(999); + let hashset = HashSet::from([non_existent_hash]); - let result = subject.delete_records(&[non_existent_hash]); + let result = subject.delete_records(hashset); assert_eq!( result, @@ -724,14 +725,15 @@ mod tests { .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); let present_hash = H256::from_low_u64_le(1); - let absent_hash = H256::from_low_u64_le(1); + let absent_hash = H256::from_low_u64_le(2); let tx = TxBuilder::default() .hash(present_hash) .status(TxStatus::Failed) .build(); subject.insert_new_records(vec![tx]); + let hashset = HashSet::from([present_hash, absent_hash]); - let result = subject.delete_records(&[present_hash, absent_hash]); + let result = subject.delete_records(hashset); assert_eq!( result, @@ -757,9 +759,9 @@ mod tests { .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); - let hashes = vec![H256::from_low_u64_le(1)]; + let hashes = HashSet::from([H256::from_low_u64_le(1)]); - let result = subject.delete_records(&hashes); + let result = subject.delete_records(hashes); assert_eq!( result, From 4c4515e4d17c0e61c51bbf1f707c16c89c4cc2f1 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 1 May 2025 16:59:57 +0530 Subject: [PATCH 37/46] GH-608: remove unused SentPayableDaoError --- .../db_access_objects/sent_payable_dao.rs | 96 ++++++++++++++----- 1 file changed, 72 insertions(+), 24 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index bd6fbeeba..5a56bd98b 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -11,10 +11,6 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { - // TODO: GH-608: Remove the first three - InsertionFailed(String), - UpdateFailed(String), - DeletionFailed(String), SqlExecutionFailed(String), InvalidInput(String), PartialExecution(String), @@ -132,8 +128,17 @@ impl SentPayableDao for SentPayableDaoReal<'_> { ); match self.conn.prepare(&sql).expect("Internal error").execute([]) { - Ok(x) if x == txs.len() => Ok(()), - Ok(x) => panic!("expected {} changed rows but got {}", txs.len(), x), // TODO: GH-608: This should be an error + Ok(inserted_rows) => { + if inserted_rows == txs.len() { + Ok(()) + } else { + Err(SentPayableDaoError::PartialExecution(format!( + "Only {} out of {} records inserted", + inserted_rows, + txs.len() + ))) + } + } Err(e) => Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())), } } @@ -183,6 +188,12 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError> { + if hash_map.is_empty() { + return Err(SentPayableDaoError::InvalidInput( + "Input is empty".to_string(), + )); + } + for (hash, status) in hash_map { let sql = format!( "UPDATE sent_payable SET status = '{}' WHERE tx_hash = '{:?}'", @@ -190,13 +201,15 @@ impl SentPayableDao for SentPayableDaoReal<'_> { ); match self.conn.prepare(&sql).expect("Internal error").execute([]) { - Ok(updated) if updated == 1 => continue, - Ok(_) => { - return Err(SentPayableDaoError::UpdateFailed(format!( - "Failed to update status for hash {:?}", - hash - ))); - // TODO: GH-608: This should be inside if else + Ok(updated_rows) => { + if updated_rows == 1 { + continue; + } else { + return Err(SentPayableDaoError::PartialExecution(format!( + "Failed to update status for hash {:?}", + hash + ))); + } } Err(e) => { return Err(SentPayableDaoError::SqlExecutionFailed(e.to_string())); @@ -250,10 +263,9 @@ mod tests { }; use crate::database::rusqlite_wrappers::ConnectionWrapperReal; use crate::database::test_utils::ConnectionWrapperMock; - use ethereum_types::{Address, H256, U64}; + use ethereum_types::{ H256, U64}; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::{Connection, OpenFlags}; - use web3::types::U256; use crate::accountant::db_access_objects::sent_payable_dao::RetrieveCondition::{ByHash, IsPending, ToRetry}; use crate::accountant::db_access_objects::test_utils::TxBuilder; use crate::blockchain::blockchain_interface::blockchain_interface_web3::lower_level_interface_web3::{TransactionBlock, TxStatus}; @@ -354,8 +366,7 @@ mod tests { } #[test] - #[should_panic(expected = "expected 1 changed rows but got 0")] - fn insert_new_records_can_panic() { + fn insert_new_records_returns_err_if_partially_executed() { let setup_conn = Connection::open_in_memory().unwrap(); // Inject a deliberately failing statement into the mocked connection. let failing_stmt = { @@ -368,7 +379,14 @@ mod tests { let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); - let _ = subject.insert_new_records(vec![tx]); + let result = subject.insert_new_records(vec![tx]); + + assert_eq!( + result, + Err(SentPayableDaoError::PartialExecution( + "Only 0 out of 1 records inserted".to_string() + )) + ); } #[test] @@ -575,30 +593,60 @@ mod tests { } #[test] - fn change_statuses_returns_update_failed_error_when_update_fails() { + fn change_statuses_returns_error_when_input_is_empty() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "change_statuses_returns_update_failed_error_when_update_fails", + "change_statuses_returns_error_when_input_is_empty", ); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); + let existent_hash = H256::from_low_u64_le(1); let tx = TxBuilder::default() - .hash(H256::from_low_u64_le(1)) + .hash(existent_hash) .status(TxStatus::Pending) .build(); subject.insert_new_records(vec![tx.clone()]).unwrap(); + let hash_map = HashMap::new(); + + let result = subject.change_statuses(&hash_map); - // Create a hash that doesn't exist in the database + assert_eq!( + result, + Err(SentPayableDaoError::InvalidInput( + "Input is empty".to_string() + )) + ); + } + + #[test] + fn change_statuses_returns_error_during_partial_execution() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "change_statuses_returns_error_during_partial_execution", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let existent_hash = H256::from_low_u64_le(1); let non_existent_hash = H256::from_low_u64_le(999); - let hash_map = HashMap::from([(non_existent_hash, TxStatus::Failed)]); + let tx = TxBuilder::default() + .hash(existent_hash) + .status(TxStatus::Pending) + .build(); + subject.insert_new_records(vec![tx.clone()]).unwrap(); + let hash_map = HashMap::from([ + (existent_hash, TxStatus::Failed), + (non_existent_hash, TxStatus::Failed), + ]); let result = subject.change_statuses(&hash_map); assert_eq!( result, - Err(SentPayableDaoError::UpdateFailed(format!( + Err(SentPayableDaoError::PartialExecution(format!( "Failed to update status for hash {:?}", non_existent_hash ))) From 9783e3b1317edca4bb28658eb44be190de4f72a1 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 1 May 2025 17:52:43 +0530 Subject: [PATCH 38/46] GH-608: use the variant EmptyInput --- .../db_access_objects/sent_payable_dao.rs | 23 ++++--------------- 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 5a56bd98b..66617355f 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -11,6 +11,7 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { + EmptyInput, SqlExecutionFailed(String), InvalidInput(String), PartialExecution(String), @@ -189,9 +190,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError> { if hash_map.is_empty() { - return Err(SentPayableDaoError::InvalidInput( - "Input is empty".to_string(), - )); + return Err(SentPayableDaoError::EmptyInput); } for (hash, status) in hash_map { @@ -222,9 +221,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { fn delete_records(&self, hashes: HashSet) -> Result<(), SentPayableDaoError> { if hashes.is_empty() { - return Err(SentPayableDaoError::InvalidInput( - "No hashes were given.".to_string(), - )); + return Err(SentPayableDaoError::EmptyInput); } let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); @@ -612,12 +609,7 @@ mod tests { let result = subject.change_statuses(&hash_map); - assert_eq!( - result, - Err(SentPayableDaoError::InvalidInput( - "Input is empty".to_string() - )) - ); + assert_eq!(result, Err(SentPayableDaoError::EmptyInput)); } #[test] @@ -731,12 +723,7 @@ mod tests { let result = subject.delete_records(HashSet::new()); - assert_eq!( - result, - Err(SentPayableDaoError::InvalidInput( - "No hashes were given.".to_string() - )) - ); + assert_eq!(result, Err(SentPayableDaoError::EmptyInput)); } #[test] From c90b97b6d812df79e0d13e7b5e8aa27c2e9bef48 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 2 May 2025 12:38:55 +0530 Subject: [PATCH 39/46] GH-608: add more validations for insert_new_records --- .../db_access_objects/sent_payable_dao.rs | 49 ++++++++++++++----- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 66617355f..aebaf5045 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -12,10 +12,10 @@ use crate::database::rusqlite_wrappers::ConnectionWrapper; #[derive(Debug, PartialEq, Eq)] pub enum SentPayableDaoError { EmptyInput, - SqlExecutionFailed(String), + NoChange, InvalidInput(String), PartialExecution(String), - NoChange(String), + SqlExecutionFailed(String), } type TxHash = H256; @@ -106,6 +106,19 @@ impl SentPayableDao for SentPayableDaoReal<'_> { } fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { + if txs.is_empty() { + return Err(SentPayableDaoError::EmptyInput); + } + + let unique_hashes: HashSet<_> = txs.iter().map(|tx| tx.hash).collect(); + if unique_hashes.len() != txs.len() { + return Err(SentPayableDaoError::InvalidInput( + "Duplicate hashes found".to_string(), + )); + } + + // TODO: GH-608: Validate the inputs is not already in the database + let sql = format!( "INSERT INTO sent_payable (\ tx_hash, receiver_address, amount_high_b, amount_low_b, \ @@ -234,9 +247,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { if deleted_rows == hashes.len() { Ok(()) } else if deleted_rows == 0 { - Err(SentPayableDaoError::NoChange( - "No records were deleted for the specified hashes.".to_string(), - )) + Err(SentPayableDaoError::NoChange) } else { Err(SentPayableDaoError::PartialExecution(format!( "Only {} of the {} hashes has been deleted.", @@ -300,6 +311,23 @@ mod tests { assert_eq!(retrieved_txs, txs); } + #[test] + fn insert_new_records_throws_err_for_empty_input() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "insert_new_records_throws_err_for_empty_input", + ); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let empty_input = vec![]; + + let result = subject.insert_new_records(empty_input.clone()); + + assert_eq!(result, Err(SentPayableDaoError::EmptyInput)); + } + #[test] fn insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted() { let home_dir = ensure_node_home_directory_exists( @@ -324,8 +352,8 @@ mod tests { assert_eq!( result, - Err(SentPayableDaoError::SqlExecutionFailed( - "UNIQUE constraint failed: sent_payable.tx_hash".to_string() + Err(SentPayableDaoError::InvalidInput( + "Duplicate hashes found".to_string() )) ); } @@ -741,12 +769,7 @@ mod tests { let result = subject.delete_records(hashset); - assert_eq!( - result, - Err(SentPayableDaoError::NoChange( - "No records were deleted for the specified hashes.".to_string() - )) - ); + assert_eq!(result, Err(SentPayableDaoError::NoChange)); } #[test] From 5835ed292424748d1c51d732a5b0201694b6a002 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 2 May 2025 13:10:38 +0530 Subject: [PATCH 40/46] GH-608: test all errors --- .../db_access_objects/sent_payable_dao.rs | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index aebaf5045..f3371a9e6 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -59,7 +59,7 @@ impl Display for RetrieveCondition { pub trait SentPayableDao { // Note that the order of the returned results is not guaranteed - fn get_tx_identifiers(&self, hashes: &[TxHash]) -> TxIdentifiers; + fn get_tx_identifiers(&self, hashes: &HashSet) -> TxIdentifiers; fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; @@ -82,10 +82,11 @@ impl<'a> SentPayableDaoReal<'a> { } impl SentPayableDao for SentPayableDaoReal<'_> { - fn get_tx_identifiers(&self, hashes: &[TxHash]) -> TxIdentifiers { + fn get_tx_identifiers(&self, hashes: &HashSet) -> TxIdentifiers { + let hashes_vec: Vec = hashes.into_iter().copied().collect(); let sql = format!( "SELECT tx_hash, rowid FROM sent_payable WHERE tx_hash IN ({})", - comma_joined_stringifiable(hashes, |hash| format!("'{:?}'", hash)) + comma_joined_stringifiable(&hashes_vec, |hash| format!("'{:?}'", hash)) ); let mut stmt = self @@ -110,14 +111,18 @@ impl SentPayableDao for SentPayableDaoReal<'_> { return Err(SentPayableDaoError::EmptyInput); } - let unique_hashes: HashSet<_> = txs.iter().map(|tx| tx.hash).collect(); + let unique_hashes: HashSet = txs.iter().map(|tx| tx.hash).collect(); if unique_hashes.len() != txs.len() { return Err(SentPayableDaoError::InvalidInput( - "Duplicate hashes found".to_string(), + "Duplicate hashes found in the input".to_string(), )); } - // TODO: GH-608: Validate the inputs is not already in the database + if !self.get_tx_identifiers(&unique_hashes).is_empty() { + return Err(SentPayableDaoError::InvalidInput( + "Input hash is already present in the database".to_string(), + )); + } let sql = format!( "INSERT INTO sent_payable (\ @@ -329,10 +334,10 @@ mod tests { } #[test] - fn insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted() { + fn insert_new_records_throws_error_when_two_txs_with_same_hash_are_present_in_the_input() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "insert_new_records_throws_error_when_two_txs_with_same_hash_are_inserted", + "insert_new_records_throws_error_when_two_txs_with_same_hash_are_present_in_the_input", ); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) @@ -353,16 +358,16 @@ mod tests { assert_eq!( result, Err(SentPayableDaoError::InvalidInput( - "Duplicate hashes found".to_string() + "Duplicate hashes found in the input".to_string() )) ); } #[test] - fn insert_new_records_throws_error_when_txs_with_an_already_present_hash_is_inserted() { + fn insert_new_records_throws_error_when_input_tx_hash_is_already_present_in_the_db() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "insert_new_records_throws_error_when_txs_with_an_already_present_hash_is_inserted", + "insert_new_records_throws_error_when_input_tx_hash_is_already_present_in_the_db", ); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) @@ -384,8 +389,8 @@ mod tests { assert_eq!(initial_insertion_result, Ok(())); assert_eq!( result, - Err(SentPayableDaoError::SqlExecutionFailed( - "UNIQUE constraint failed: sent_payable.tx_hash".to_string() + Err(SentPayableDaoError::InvalidInput( + "Input hash is already present in the database".to_string() )) ); } @@ -400,7 +405,15 @@ mod tests { .unwrap(); setup_conn.prepare("SELECT id FROM example").unwrap() }; - let wrapped_conn = ConnectionWrapperMock::default().prepare_result(Ok(failing_stmt)); + let failing_stmt_2 = { + setup_conn + .execute("CREATE TABLE example2 (id integer)", []) + .unwrap(); + setup_conn.prepare("SELECT id FROM example2").unwrap() + }; + let wrapped_conn = ConnectionWrapperMock::default() + .prepare_result(Ok(failing_stmt)) + .prepare_result(Ok(failing_stmt_2)); let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); @@ -455,11 +468,12 @@ mod tests { let hash1 = H256::from_low_u64_le(1); let hash2 = H256::from_low_u64_le(2); let hash3 = H256::from_low_u64_le(3); // not present in the database + let hashset = HashSet::from([hash1, hash2, hash3]); let tx1 = TxBuilder::default().hash(hash1).build(); let tx2 = TxBuilder::default().hash(hash2).build(); subject.insert_new_records(vec![tx1, tx2]).unwrap(); - let result = subject.get_tx_identifiers(&vec![hash1, hash2, hash3]); + let result = subject.get_tx_identifiers(&hashset); assert_eq!(result.get(&hash1), Some(&1u64)); assert_eq!(result.get(&hash2), Some(&2u64)); From d44215e24f7789f2de8ca8646c6464df45dd523b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 2 May 2025 15:30:06 +0530 Subject: [PATCH 41/46] GH-608: remove unnecessary TODOs --- node/src/accountant/db_access_objects/sent_payable_dao.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index f3371a9e6..1647b1649 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -73,12 +73,8 @@ pub struct SentPayableDaoReal<'a> { impl<'a> SentPayableDaoReal<'a> { pub fn new(conn: Box) -> Self { - // TODO: GH-608: You'll need to write a mock to test this, do that wisely Self { conn } } - - // TODO: GH-608: There should be a function for executing SQL - // TODO: GH-608: There should be a function for handling database errors } impl SentPayableDao for SentPayableDaoReal<'_> { From e8049c435c33d4fc50f5d29aa163b7c53bd39db6 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 2 May 2025 16:23:48 +0530 Subject: [PATCH 42/46] GH-608: perform some cleanup --- .../db_access_objects/sent_payable_dao.rs | 59 ++++++++++--------- .../db_access_objects/test_utils.rs | 20 ------- 2 files changed, 30 insertions(+), 49 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 1647b1649..fa5980245 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -58,12 +58,11 @@ impl Display for RetrieveCondition { } pub trait SentPayableDao { - // Note that the order of the returned results is not guaranteed fn get_tx_identifiers(&self, hashes: &HashSet) -> TxIdentifiers; - fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError>; + fn insert_new_records(&self, txs: &Vec) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; - fn delete_records(&self, hashes: HashSet) -> Result<(), SentPayableDaoError>; + fn delete_records(&self, hashes: &HashSet) -> Result<(), SentPayableDaoError>; } #[derive(Debug)] @@ -102,7 +101,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn insert_new_records(&self, txs: Vec) -> Result<(), SentPayableDaoError> { + fn insert_new_records(&self, txs: &Vec) -> Result<(), SentPayableDaoError> { if txs.is_empty() { return Err(SentPayableDaoError::EmptyInput); } @@ -233,7 +232,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { Ok(()) } - fn delete_records(&self, hashes: HashSet) -> Result<(), SentPayableDaoError> { + fn delete_records(&self, hashes: &HashSet) -> Result<(), SentPayableDaoError> { if hashes.is_empty() { return Err(SentPayableDaoError::EmptyInput); } @@ -304,7 +303,7 @@ mod tests { let subject = SentPayableDaoReal::new(wrapped_conn); let txs = vec![tx1, tx2, tx3]; - let result = subject.insert_new_records(txs.clone()); + let result = subject.insert_new_records(&txs); let retrieved_txs = subject.retrieve_txs(None); assert_eq!(result, Ok(())); @@ -324,7 +323,7 @@ mod tests { let subject = SentPayableDaoReal::new(wrapped_conn); let empty_input = vec![]; - let result = subject.insert_new_records(empty_input.clone()); + let result = subject.insert_new_records(&empty_input); assert_eq!(result, Err(SentPayableDaoError::EmptyInput)); } @@ -349,7 +348,7 @@ mod tests { .build(); let subject = SentPayableDaoReal::new(wrapped_conn); - let result = subject.insert_new_records(vec![tx1, tx2]); + let result = subject.insert_new_records(&vec![tx1, tx2]); assert_eq!( result, @@ -378,9 +377,9 @@ mod tests { .status(TxStatus::Failed) .build(); let subject = SentPayableDaoReal::new(wrapped_conn); - let initial_insertion_result = subject.insert_new_records(vec![tx1]); + let initial_insertion_result = subject.insert_new_records(&vec![tx1]); - let result = subject.insert_new_records(vec![tx2]); + let result = subject.insert_new_records(&vec![tx2]); assert_eq!(initial_insertion_result, Ok(())); assert_eq!( @@ -413,7 +412,7 @@ mod tests { let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); - let result = subject.insert_new_records(vec![tx]); + let result = subject.insert_new_records(&vec![tx]); assert_eq!( result, @@ -443,7 +442,7 @@ mod tests { let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); - let result = subject.insert_new_records(vec![tx]); + let result = subject.insert_new_records(&vec![tx]); assert_eq!( result, @@ -467,7 +466,7 @@ mod tests { let hashset = HashSet::from([hash1, hash2, hash3]); let tx1 = TxBuilder::default().hash(hash1).build(); let tx2 = TxBuilder::default().hash(hash2).build(); - subject.insert_new_records(vec![tx1, tx2]).unwrap(); + subject.insert_new_records(&vec![tx1, tx2]).unwrap(); let result = subject.get_tx_identifiers(&hashset); @@ -504,7 +503,7 @@ mod tests { })) .build(); subject - .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3, tx4]) + .insert_new_records(&vec![tx1.clone(), tx2.clone(), tx3, tx4]) .unwrap(); let result = subject.retrieve_txs(Some(RetrieveCondition::IsPending)); @@ -540,7 +539,7 @@ mod tests { .status(TxStatus::Failed) .build(); subject - .insert_new_records(vec![tx1, tx2, tx3.clone()]) + .insert_new_records(&vec![tx1, tx2, tx3.clone()]) .unwrap(); let result = subject.retrieve_txs(Some(RetrieveCondition::ToRetry)); @@ -575,7 +574,7 @@ mod tests { .status(TxStatus::Failed) .build(); subject - .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .insert_new_records(&vec![tx1.clone(), tx2.clone()]) .unwrap(); let result = subject.retrieve_txs(Some(ByHash(tx1.hash))); @@ -600,7 +599,7 @@ mod tests { .status(TxStatus::Pending) .build(); subject - .insert_new_records(vec![tx1.clone(), tx2.clone()]) + .insert_new_records(&vec![tx1.clone(), tx2.clone()]) .unwrap(); let hash_map = HashMap::from([ (tx1.hash, TxStatus::Failed), @@ -617,6 +616,7 @@ mod tests { let tx1_updated = subject.retrieve_txs(Some(ByHash(tx1.hash))).pop().unwrap(); let tx2_updated = subject.retrieve_txs(Some(ByHash(tx2.hash))).pop().unwrap(); + assert_eq!(result, Ok(())); assert_eq!(tx1_updated.status, TxStatus::Failed); assert_eq!( tx2_updated.status, @@ -642,7 +642,7 @@ mod tests { .hash(existent_hash) .status(TxStatus::Pending) .build(); - subject.insert_new_records(vec![tx.clone()]).unwrap(); + subject.insert_new_records(&vec![tx.clone()]).unwrap(); let hash_map = HashMap::new(); let result = subject.change_statuses(&hash_map); @@ -666,7 +666,7 @@ mod tests { .hash(existent_hash) .status(TxStatus::Pending) .build(); - subject.insert_new_records(vec![tx.clone()]).unwrap(); + subject.insert_new_records(&vec![tx.clone()]).unwrap(); let hash_map = HashMap::from([ (existent_hash, TxStatus::Failed), (non_existent_hash, TxStatus::Failed), @@ -738,13 +738,14 @@ mod tests { })) .build(); subject - .insert_new_records(vec![tx1.clone(), tx2.clone(), tx3.clone()]) + .insert_new_records(&vec![tx1.clone(), tx2.clone(), tx3.clone()]) .unwrap(); let hashset = HashSet::from([tx1.hash, tx2.hash]); - let result = subject.delete_records(hashset); + let result = subject.delete_records(&hashset); let remaining_records = subject.retrieve_txs(None); + assert_eq!(result, Ok(())); assert_eq!(remaining_records, vec![tx3]); } @@ -759,7 +760,7 @@ mod tests { .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); - let result = subject.delete_records(HashSet::new()); + let result = subject.delete_records(&HashSet::new()); assert_eq!(result, Err(SentPayableDaoError::EmptyInput)); } @@ -777,7 +778,7 @@ mod tests { let non_existent_hash = H256::from_low_u64_le(999); let hashset = HashSet::from([non_existent_hash]); - let result = subject.delete_records(hashset); + let result = subject.delete_records(&hashset); assert_eq!(result, Err(SentPayableDaoError::NoChange)); } @@ -798,16 +799,16 @@ mod tests { .hash(present_hash) .status(TxStatus::Failed) .build(); - subject.insert_new_records(vec![tx]); + subject.insert_new_records(&vec![tx]).unwrap(); let hashset = HashSet::from([present_hash, absent_hash]); - let result = subject.delete_records(hashset); + let result = subject.delete_records(&hashset); assert_eq!( result, - Err(SentPayableDaoError::PartialExecution(format!( - "Only 1 of the 2 hashes has been deleted." - ))) + Err(SentPayableDaoError::PartialExecution( + "Only 1 of the 2 hashes has been deleted.".to_string() + )) ); } @@ -829,7 +830,7 @@ mod tests { let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); let hashes = HashSet::from([H256::from_low_u64_le(1)]); - let result = subject.delete_records(hashes); + let result = subject.delete_records(&hashes); assert_eq!( result, diff --git a/node/src/accountant/db_access_objects/test_utils.rs b/node/src/accountant/db_access_objects/test_utils.rs index 9e078bd67..23acf4ec5 100644 --- a/node/src/accountant/db_access_objects/test_utils.rs +++ b/node/src/accountant/db_access_objects/test_utils.rs @@ -24,31 +24,11 @@ impl TxBuilder { self } - pub fn receiver_address(mut self, receiver_address: Address) -> Self { - self.receiver_address_opt = Some(receiver_address); - self - } - - pub fn amount(mut self, amount: u128) -> Self { - self.amount_opt = Some(amount); - self - } - pub fn timestamp(mut self, timestamp: i64) -> Self { self.timestamp_opt = Some(timestamp); self } - pub fn gas_price_wei(mut self, gas_price_wei: u64) -> Self { - self.gas_price_wei_opt = Some(gas_price_wei); - self - } - - pub fn nonce(mut self, nonce: u32) -> Self { - self.nonce_opt = Some(nonce); - self - } - pub fn status(mut self, status: TxStatus) -> Self { self.status_opt = Some(status); self From 545bfeb9049401fb5a4b1d3c5a10f77449694d2d Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 2 May 2025 16:41:45 +0530 Subject: [PATCH 43/46] GH-608: eliminate clippy warnings --- .../accountant/db_access_objects/sent_payable_dao.rs | 10 ++++++---- node/src/accountant/db_access_objects/test_utils.rs | 3 +++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index fa5980245..556c181a2 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -1,3 +1,5 @@ +// Copyright (c) 2025, MASQ (https://masq.ai) and/or its affiliates. All rights reserved. + use std::collections::{HashMap, HashSet}; use std::fmt::{Display, Formatter}; use std::str::FromStr; @@ -59,7 +61,7 @@ impl Display for RetrieveCondition { pub trait SentPayableDao { fn get_tx_identifiers(&self, hashes: &HashSet) -> TxIdentifiers; - fn insert_new_records(&self, txs: &Vec) -> Result<(), SentPayableDaoError>; + fn insert_new_records(&self, txs: &[Tx]) -> Result<(), SentPayableDaoError>; fn retrieve_txs(&self, condition: Option) -> Vec; fn change_statuses(&self, hash_map: &TxUpdates) -> Result<(), SentPayableDaoError>; fn delete_records(&self, hashes: &HashSet) -> Result<(), SentPayableDaoError>; @@ -78,7 +80,7 @@ impl<'a> SentPayableDaoReal<'a> { impl SentPayableDao for SentPayableDaoReal<'_> { fn get_tx_identifiers(&self, hashes: &HashSet) -> TxIdentifiers { - let hashes_vec: Vec = hashes.into_iter().copied().collect(); + let hashes_vec: Vec = hashes.iter().copied().collect(); let sql = format!( "SELECT tx_hash, rowid FROM sent_payable WHERE tx_hash IN ({})", comma_joined_stringifiable(&hashes_vec, |hash| format!("'{:?}'", hash)) @@ -101,7 +103,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { .collect() } - fn insert_new_records(&self, txs: &Vec) -> Result<(), SentPayableDaoError> { + fn insert_new_records(&self, txs: &[Tx]) -> Result<(), SentPayableDaoError> { if txs.is_empty() { return Err(SentPayableDaoError::EmptyInput); } @@ -124,7 +126,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { tx_hash, receiver_address, amount_high_b, amount_low_b, \ timestamp, gas_price_wei, nonce, status ) VALUES {}", - comma_joined_stringifiable(&txs, |tx| { + comma_joined_stringifiable(txs, |tx| { let amount_checked = checked_conversion::(tx.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); format!( diff --git a/node/src/accountant/db_access_objects/test_utils.rs b/node/src/accountant/db_access_objects/test_utils.rs index 23acf4ec5..3a571ff6a 100644 --- a/node/src/accountant/db_access_objects/test_utils.rs +++ b/node/src/accountant/db_access_objects/test_utils.rs @@ -1,3 +1,6 @@ +// Copyright (c) 2025, MASQ (https://masq.ai) and/or its affiliates. All rights reserved. +#![cfg(test)] + use web3::types::{Address, H256}; use crate::accountant::db_access_objects::sent_payable_dao::Tx; use crate::accountant::db_access_objects::utils::current_unix_timestamp; From a01021534a2664473af929bd40ed4de740743985 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 9 May 2025 12:53:48 +0530 Subject: [PATCH 44/46] GH-608: review 1 changes --- .../db_access_objects/sent_payable_dao.rs | 198 ++++++++++++------ .../lower_level_interface_web3.rs | 2 +- 2 files changed, 132 insertions(+), 68 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 556c181a2..51ee42a74 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -21,9 +21,9 @@ pub enum SentPayableDaoError { } type TxHash = H256; -type RowID = u64; +type RowId = u64; -type TxIdentifiers = HashMap; +type TxIdentifiers = HashMap; type TxUpdates = HashMap; #[derive(Clone, Debug, PartialEq, Eq)] @@ -40,7 +40,7 @@ pub struct Tx { pub enum RetrieveCondition { IsPending, ToRetry, - ByHash(TxHash), + ByHash(Vec), } impl Display for RetrieveCondition { @@ -52,8 +52,12 @@ impl Display for RetrieveCondition { RetrieveCondition::ToRetry => { write!(f, "WHERE status = 'Failed'") } - RetrieveCondition::ByHash(tx_hash) => { - write!(f, "WHERE tx_hash = '{:?}'", tx_hash) + RetrieveCondition::ByHash(tx_hashes) => { + write!( + f, + "WHERE tx_hash IN ({})", + comma_joined_stringifiable(&tx_hashes, |hash| format!("'{:?}'", hash)) + ) } } } @@ -123,9 +127,9 @@ impl SentPayableDao for SentPayableDaoReal<'_> { let sql = format!( "INSERT INTO sent_payable (\ - tx_hash, receiver_address, amount_high_b, amount_low_b, \ - timestamp, gas_price_wei, nonce, status - ) VALUES {}", + tx_hash, receiver_address, amount_high_b, amount_low_b, \ + timestamp, gas_price_wei, nonce, status + ) VALUES {}", comma_joined_stringifiable(txs, |tx| { let amount_checked = checked_conversion::(tx.amount); let (high_bytes, low_bytes) = BigIntDivider::deconstruct(amount_checked); @@ -176,7 +180,7 @@ impl SentPayableDao for SentPayableDaoReal<'_> { stmt.query_map([], |row| { let tx_hash_str: String = row.get(0).expectv("tx_hash"); let hash = H256::from_str(&tx_hash_str[2..]).expect("Failed to parse H256"); - let receiver_address_str: String = row.get(1).expectv("row_id"); + let receiver_address_str: String = row.get(1).expectv("receivable_address"); let receiver_address = Address::from_str(&receiver_address_str[2..]).expect("Failed to parse H160"); let amount_high_b = row.get(2).expectv("amount_high_b"); @@ -239,10 +243,11 @@ impl SentPayableDao for SentPayableDaoReal<'_> { return Err(SentPayableDaoError::EmptyInput); } - let hash_strings: Vec = hashes.iter().map(|h| format!("'{:?}'", h)).collect(); - let hash_list = hash_strings.join(", "); - - let sql = format!("DELETE FROM sent_payable WHERE tx_hash IN ({})", hash_list); + let hashes_vec: Vec = hashes.iter().cloned().collect(); + let sql = format!( + "DELETE FROM sent_payable WHERE tx_hash IN ({})", + comma_joined_stringifiable(&hashes_vec, |hash| { format!("'{:?}'", hash) }) + ); match self.conn.prepare(&sql).expect("Internal error").execute([]) { Ok(deleted_rows) => { @@ -395,22 +400,14 @@ mod tests { #[test] fn insert_new_records_returns_err_if_partially_executed() { let setup_conn = Connection::open_in_memory().unwrap(); - // Inject a deliberately failing statement into the mocked connection. - let failing_stmt = { - setup_conn - .execute("CREATE TABLE example (id integer)", []) - .unwrap(); - setup_conn.prepare("SELECT id FROM example").unwrap() - }; - let failing_stmt_2 = { - setup_conn - .execute("CREATE TABLE example2 (id integer)", []) - .unwrap(); - setup_conn.prepare("SELECT id FROM example2").unwrap() - }; + setup_conn + .execute("CREATE TABLE example (id integer)", []) + .unwrap(); + let pre_insert_stmt = setup_conn.prepare("SELECT id FROM example").unwrap(); + let faulty_insert_stmt = { setup_conn.prepare("SELECT id FROM example").unwrap() }; let wrapped_conn = ConnectionWrapperMock::default() - .prepare_result(Ok(failing_stmt)) - .prepare_result(Ok(failing_stmt_2)); + .prepare_result(Ok(pre_insert_stmt)) + .prepare_result(Ok(faulty_insert_stmt)); let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); @@ -462,19 +459,83 @@ mod tests { .initialize(&home_dir, DbInitializationConfig::test_default()) .unwrap(); let subject = SentPayableDaoReal::new(wrapped_conn); - let hash1 = H256::from_low_u64_le(1); - let hash2 = H256::from_low_u64_le(2); - let hash3 = H256::from_low_u64_le(3); // not present in the database - let hashset = HashSet::from([hash1, hash2, hash3]); - let tx1 = TxBuilder::default().hash(hash1).build(); - let tx2 = TxBuilder::default().hash(hash2).build(); - subject.insert_new_records(&vec![tx1, tx2]).unwrap(); + let present_hash = H256::from_low_u64_le(1); + let absent_hash = H256::from_low_u64_le(2); + let another_present_hash = H256::from_low_u64_le(3); + let hashset = HashSet::from([present_hash, absent_hash, another_present_hash]); + let present_tx = TxBuilder::default().hash(present_hash).build(); + let another_present_tx = TxBuilder::default().hash(another_present_hash).build(); + subject + .insert_new_records(&vec![present_tx, another_present_tx]) + .unwrap(); let result = subject.get_tx_identifiers(&hashset); - assert_eq!(result.get(&hash1), Some(&1u64)); - assert_eq!(result.get(&hash2), Some(&2u64)); - assert_eq!(result.get(&hash3), None); + assert_eq!(result.get(&present_hash), Some(&1u64)); + assert_eq!(result.get(&absent_hash), None); + assert_eq!(result.get(&another_present_hash), Some(&2u64)); + } + + #[test] + fn retrieve_condition_display_works() { + assert_eq!(IsPending.to_string(), "WHERE status = 'Pending'"); + assert_eq!(ToRetry.to_string(), "WHERE status = 'Failed'"); + assert_eq!( + ByHash(vec![ + H256::from_low_u64_be(0x123456789), + H256::from_low_u64_be(0x987654321), + ]) + .to_string(), + "WHERE tx_hash IN (\ + '0x0000000000000000000000000000000000000000000000000000000123456789', \ + '0x0000000000000000000000000000000000000000000000000000000987654321'\ + )" + .to_string() + ); + } + + #[test] + fn can_retrieve_all_txs() { + let home_dir = + ensure_node_home_directory_exists("sent_payable_dao", "can_retrieve_all_txs"); + let wrapped_conn = DbInitializerReal::default() + .initialize(&home_dir, DbInitializationConfig::test_default()) + .unwrap(); + let subject = SentPayableDaoReal::new(wrapped_conn); + let tx1 = TxBuilder::default() + .hash(H256::from_low_u64_le(1)) + .status(TxStatus::Pending) + .build(); + let tx2 = TxBuilder::default() + .hash(H256::from_low_u64_le(2)) + .status(TxStatus::Failed) + .build(); + let tx3 = TxBuilder::default() + .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Succeeded(TransactionBlock { + block_hash: Default::default(), + block_number: Default::default(), + })) + .build(); + let tx4 = TxBuilder::default() + .hash(H256::from_low_u64_le(4)) + .status(TxStatus::Pending) + .build(); + let tx5 = TxBuilder::default() + .hash(H256::from_low_u64_le(5)) + .status(TxStatus::Failed) + .build(); + subject + .insert_new_records(&vec![tx1.clone(), tx2.clone(), tx3.clone()]) + .unwrap(); + subject + .insert_new_records(&vec![tx4.clone(), tx5.clone()]) + .unwrap(); + + let result = subject.retrieve_txs(None); + + assert_eq!(result.len(), 5); + assert_eq!(result, vec![tx1, tx2, tx3, tx4, tx5]); } #[test] @@ -540,23 +601,22 @@ mod tests { .timestamp(old_timestamp) .status(TxStatus::Failed) .build(); + let tx4 = TxBuilder::default() // this should be picked for retry + .hash(H256::from_low_u64_le(6)) + .status(TxStatus::Failed) + .build(); + let tx5 = TxBuilder::default() + .hash(H256::from_low_u64_le(7)) + .timestamp(old_timestamp) + .status(TxStatus::Pending) + .build(); subject - .insert_new_records(&vec![tx1, tx2, tx3.clone()]) + .insert_new_records(&vec![tx1, tx2, tx3.clone(), tx4.clone(), tx5]) .unwrap(); let result = subject.retrieve_txs(Some(RetrieveCondition::ToRetry)); - assert_eq!(result, vec![tx3]); - } - - #[test] - fn retrieve_condition_display_works() { - assert_eq!(IsPending.to_string(), "WHERE status = 'Pending'"); - assert_eq!(ToRetry.to_string(), "WHERE status = 'Failed'"); - assert_eq!( - ByHash(H256::default()).to_string(), - format!("WHERE tx_hash = '{:?}'", H256::default()) - ); + assert_eq!(result, vec![tx3, tx4]); } #[test] @@ -579,7 +639,7 @@ mod tests { .insert_new_records(&vec![tx1.clone(), tx2.clone()]) .unwrap(); - let result = subject.retrieve_txs(Some(ByHash(tx1.hash))); + let result = subject.retrieve_txs(Some(ByHash(vec![tx1.hash]))); assert_eq!(result, vec![tx1]); } @@ -616,12 +676,11 @@ mod tests { let result = subject.change_statuses(&hash_map); - let tx1_updated = subject.retrieve_txs(Some(ByHash(tx1.hash))).pop().unwrap(); - let tx2_updated = subject.retrieve_txs(Some(ByHash(tx2.hash))).pop().unwrap(); + let updated_txs = subject.retrieve_txs(Some(ByHash(vec![tx1.hash, tx2.hash]))); assert_eq!(result, Ok(())); - assert_eq!(tx1_updated.status, TxStatus::Failed); + assert_eq!(updated_txs[0].status, TxStatus::Failed); assert_eq!( - tx2_updated.status, + updated_txs[1].status, TxStatus::Succeeded(TransactionBlock { block_hash: H256::from_low_u64_le(3), block_number: U64::from(1), @@ -644,7 +703,7 @@ mod tests { .hash(existent_hash) .status(TxStatus::Pending) .build(); - subject.insert_new_records(&vec![tx.clone()]).unwrap(); + subject.insert_new_records(&vec![tx]).unwrap(); let hash_map = HashMap::new(); let result = subject.change_statuses(&hash_map); @@ -668,7 +727,7 @@ mod tests { .hash(existent_hash) .status(TxStatus::Pending) .build(); - subject.insert_new_records(&vec![tx.clone()]).unwrap(); + subject.insert_new_records(&vec![tx]).unwrap(); let hash_map = HashMap::from([ (existent_hash, TxStatus::Failed), (non_existent_hash, TxStatus::Failed), @@ -703,7 +762,6 @@ mod tests { .unwrap(); let wrapped_conn = ConnectionWrapperReal::new(read_only_conn); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); - let hash = H256::from_low_u64_le(1); let hash_map = HashMap::from([(hash, TxStatus::Failed)]); @@ -730,32 +788,36 @@ mod tests { .build(); let tx2 = TxBuilder::default() .hash(H256::from_low_u64_le(2)) - .status(TxStatus::Failed) + .status(TxStatus::Pending) .build(); let tx3 = TxBuilder::default() .hash(H256::from_low_u64_le(3)) + .status(TxStatus::Failed) + .build(); + let tx4 = TxBuilder::default() + .hash(H256::from_low_u64_le(4)) .status(TxStatus::Succeeded(TransactionBlock { block_hash: Default::default(), block_number: Default::default(), })) .build(); subject - .insert_new_records(&vec![tx1.clone(), tx2.clone(), tx3.clone()]) + .insert_new_records(&vec![tx1.clone(), tx2.clone(), tx3.clone(), tx4.clone()]) .unwrap(); - let hashset = HashSet::from([tx1.hash, tx2.hash]); + let hashset = HashSet::from([tx1.hash, tx3.hash]); let result = subject.delete_records(&hashset); let remaining_records = subject.retrieve_txs(None); assert_eq!(result, Ok(())); - assert_eq!(remaining_records, vec![tx3]); + assert_eq!(remaining_records, vec![tx2, tx4]); } #[test] - fn delete_records_returns_error_when_input_records_are_invalid() { + fn delete_records_returns_error_when_input_is_empty() { let home_dir = ensure_node_home_directory_exists( "sent_payable_dao", - "delete_records_returns_error_when_input_records_are_invalid", + "delete_records_returns_error_when_input_is_empty", ); let wrapped_conn = DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) @@ -815,9 +877,11 @@ mod tests { } #[test] - fn delete_records_returns_deletion_failed_error_when_an_error_occurs_in_sql() { - let home_dir = - ensure_node_home_directory_exists("sent_payable_dao", "delete_records_can_throw_error"); + fn delete_records_returns_a_general_error_from_sql() { + let home_dir = ensure_node_home_directory_exists( + "sent_payable_dao", + "delete_records_returns_a_general_error_from_sql", + ); { DbInitializerReal::default() .initialize(&home_dir, DbInitializationConfig::test_default()) diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs index 8d5be2b99..d724bed91 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs @@ -33,7 +33,7 @@ impl FromStr for TxStatus { fn from_str(s: &str) -> Result { match s { "Pending" => Ok(TxStatus::Pending), - "Failed" => Ok(TxStatus::Failed), + "Failed" => Ok(TxStatus::Failed), // TODO: GH-631: This should be changed to "Retry" s if s.starts_with("Succeeded") => { // The format is "Succeeded(block_number, block_hash)" let parts: Vec<&str> = s[10..s.len() - 1].split(',').collect(); From 9f47ee6fc5a49ec216d5aafa86a4ef814aa5cfaa Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 9 May 2025 15:32:52 +0530 Subject: [PATCH 45/46] GH-608: remove clippy warnings --- node/src/accountant/db_access_objects/sent_payable_dao.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 51ee42a74..4d5f14c28 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -56,7 +56,7 @@ impl Display for RetrieveCondition { write!( f, "WHERE tx_hash IN ({})", - comma_joined_stringifiable(&tx_hashes, |hash| format!("'{:?}'", hash)) + comma_joined_stringifiable(tx_hashes, |hash| format!("'{:?}'", hash)) ) } } From 13340d5ae33e73f43948e003e289fe17caadb39e Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Thu, 15 May 2025 10:57:24 +0530 Subject: [PATCH 46/46] GH-608: review 2 changes --- node/src/accountant/db_access_objects/sent_payable_dao.rs | 6 +++--- .../blockchain_interface_web3/lower_level_interface_web3.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/node/src/accountant/db_access_objects/sent_payable_dao.rs b/node/src/accountant/db_access_objects/sent_payable_dao.rs index 4d5f14c28..1ef307224 100644 --- a/node/src/accountant/db_access_objects/sent_payable_dao.rs +++ b/node/src/accountant/db_access_objects/sent_payable_dao.rs @@ -403,10 +403,10 @@ mod tests { setup_conn .execute("CREATE TABLE example (id integer)", []) .unwrap(); - let pre_insert_stmt = setup_conn.prepare("SELECT id FROM example").unwrap(); + let get_tx_identifiers_stmt = setup_conn.prepare("SELECT id FROM example").unwrap(); let faulty_insert_stmt = { setup_conn.prepare("SELECT id FROM example").unwrap() }; let wrapped_conn = ConnectionWrapperMock::default() - .prepare_result(Ok(pre_insert_stmt)) + .prepare_result(Ok(get_tx_identifiers_stmt)) .prepare_result(Ok(faulty_insert_stmt)); let tx = TxBuilder::default().build(); let subject = SentPayableDaoReal::new(Box::new(wrapped_conn)); @@ -534,7 +534,6 @@ mod tests { let result = subject.retrieve_txs(None); - assert_eq!(result.len(), 5); assert_eq!(result, vec![tx1, tx2, tx3, tx4, tx5]); } @@ -596,6 +595,7 @@ mod tests { block_number: Default::default(), })) .build(); + // TODO: GH-631: Instead of fetching it from SentPayables, fetch it from the FailedPayables table let tx3 = TxBuilder::default() // this should be picked for retry .hash(H256::from_low_u64_le(5)) .timestamp(old_timestamp) diff --git a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs index d724bed91..25a747907 100644 --- a/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs +++ b/node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs @@ -33,7 +33,7 @@ impl FromStr for TxStatus { fn from_str(s: &str) -> Result { match s { "Pending" => Ok(TxStatus::Pending), - "Failed" => Ok(TxStatus::Failed), // TODO: GH-631: This should be changed to "Retry" + "Failed" => Ok(TxStatus::Failed), // TODO: GH-631: This should be removed s if s.starts_with("Succeeded") => { // The format is "Succeeded(block_number, block_hash)" let parts: Vec<&str> = s[10..s.len() - 1].split(',').collect();