Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

GH-744-Review-sub-2 #553

Open
wants to merge 30 commits into
base: GH-744
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3d35b4c
GH-744: Review-1 first lot of changes
Syther007 Oct 21, 2024
cc709be
GH-744: Review-1 - fixed more tests
Syther007 Oct 22, 2024
e9bddc8
GH-744: improved wildcard IP check
Syther007 Oct 22, 2024
b72c61b
GH-744: removed unused imports
Syther007 Oct 22, 2024
d4c5be9
GH-744: fixed a bunch more comments from review 1
Syther007 Oct 25, 2024
43456e7
GH-744: resolved more review comments
Syther007 Nov 1, 2024
4d7fa8a
GH-744: started converting gas_price units from gwei to wei
Syther007 Nov 1, 2024
6a3ceaf
GH-744: finish converting gas price from gwei to wei
Syther007 Nov 4, 2024
8f131bb
GH-744: Formating & removed warnings
Syther007 Nov 4, 2024
67a9786
GH-744: Added TransactionFailed to TransactionReceiptResult
Syther007 Nov 5, 2024
4db1f91
GH-744: Migrated the guts of get_transaction_receipt_in_batch to proc…
Syther007 Nov 8, 2024
b0aed3a
GH-744: Moved submit_payables_in_batch to blockchain_interface
Syther007 Nov 11, 2024
fac4936
GH-744: removed test: blockchain_bridge_can_return_report_transaction…
Syther007 Nov 12, 2024
c5ddb57
GH-744: Fixed a few more URGENCY comments
Syther007 Nov 13, 2024
237a533
GH-744: cleanup & formatting
Syther007 Nov 13, 2024
1be2642
GH-744: add some TODOs as discussed on Wed and Thu
utkarshg6 Nov 15, 2024
8d22c61
GH-744: handle_request_transaction_receipts chaged error to a DEBUG log
Syther007 Nov 15, 2024
b3cafe4
GH-744: handle_retrieve_transactions inbeded extract_max_block_count
Syther007 Nov 15, 2024
ab32fb6
GH-744: code refactor
Syther007 Nov 15, 2024
1a92588
GH-744: removed transaction_id from Agent
Syther007 Nov 18, 2024
5172c82
GH-744: Removed get_gas_price from submit_batch call
Syther007 Nov 18, 2024
89419ed
GH-744: logger is now a reference in send_payables_within_batch
Syther007 Nov 18, 2024
5181f1c
GH-744: send_payables_within_batch web3_batch is now a reference
Syther007 Nov 18, 2024
54ab85e
GH-744: sign_and_append_multiple_payments accounts is now a reference
Syther007 Nov 18, 2024
e93c0f1
GH-744 removed Blockchan_interface_mock
Syther007 Nov 19, 2024
ef88ab4
GH-744: Refactored all 4 test for send_payables_within_batch
Syther007 Nov 20, 2024
f18b8a6
GH-744: cleanup & formatting
Syther007 Nov 20, 2024
a5e6f68
GH-744: small fixs
Syther007 Nov 20, 2024
3d4f552
GH-744: handle_normal_client_data detects wildcard IP & localhost wit…
Syther007 Nov 22, 2024
4de72c8
GH-744: Finished all urgent comments
Syther007 Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion masq/tests/startup_shutdown_tests_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ fn handles_startup_and_shutdown_integration() {
"--data-directory",
dir_path.to_str().unwrap(),
"--blockchain-service-url",
"https://example.com",
"https://nonexistentblockchainservice.com",
]);

let (stdout, stderr, exit_code) = masq_handle.stop();
Expand Down
13 changes: 7 additions & 6 deletions masq_lib/src/test_utils/mock_blockchain_client_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ lazy_static! {

pub struct MBCSBuilder {
port: u16,
run_on_docker: bool,
run_in_docker: bool,
response_batch_opt: Option<Vec<String>>,
responses: Vec<String>,
notifier: Sender<()>,
Expand All @@ -34,15 +34,15 @@ impl MBCSBuilder {
pub fn new(port: u16) -> Self {
Self {
port,
run_on_docker: false,
run_in_docker: false,
response_batch_opt: None,
responses: vec![],
notifier: unbounded().0,
}
}

pub fn run_on_docker(mut self) -> Self {
self.run_on_docker = true;
pub fn run_in_docker(mut self) -> Self {
self.run_in_docker = true;
self
}

Expand Down Expand Up @@ -112,7 +112,7 @@ impl MBCSBuilder {
pub fn start(self) -> MockBlockchainClientServer {
let requests = Arc::new(Mutex::new(vec![]));
let mut server = MockBlockchainClientServer {
port_or_local_addr: if self.run_on_docker {
port_or_local_addr: if self.run_in_docker {
Right(SocketAddr::V4(SocketAddrV4::new(
Ipv4Addr::new(172, 18, 0, 1),
self.port,
Expand Down Expand Up @@ -413,4 +413,5 @@ struct ConnectionState {
request_accumulator: String,
}

// Test for this are located: multinode_integration_tests/src/mock_blockchain_client_server.rs
// TODO GH-805
// Tests for this are located: multinode_integration_tests/src/mock_blockchain_client_server.rs
17 changes: 9 additions & 8 deletions multinode_integration_tests/src/mock_blockchain_client_server.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) 2022, MASQ (https://masq.ai) and/or its affiliates. All rights reserved.

// Code has been migrated to masq_lib/src/test_utils/mock_blockchain_client_server.rs
// TODO: GH-805
// The actual mock server has been migrated to masq_lib/src/test_utils/mock_blockchain_client_server.rs

#[cfg(test)]
mod tests {
Expand Down Expand Up @@ -29,7 +30,7 @@ mod tests {
let port = find_free_port();
let _subject = MockBlockchainClientServer::builder(port)
.response("Thank you and good night", 40)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
let chunks = vec![
Expand Down Expand Up @@ -61,7 +62,7 @@ mod tests {
let _subject = MockBlockchainClientServer::builder(port)
.response("Welcome, and thanks for coming!", 39)
.response("Thank you and good night", 40)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
client.write (b"POST /biddle HTTP/1.1\r\nContent-Length: 5\r\n\r\nfirstPOST /biddle HTTP/1.1\r\nContent-Length: 6\r\n\r\nsecond").unwrap();
Expand All @@ -85,7 +86,7 @@ mod tests {
let port = find_free_port();
let _subject = MockBlockchainClientServer::builder(port)
.response("irrelevant".to_string(), 42)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
let request = b"POST /biddle HTTP/1.1\r\n\r\nbody";
Expand All @@ -102,7 +103,7 @@ mod tests {
let port = find_free_port();
let _subject = MockBlockchainClientServer::builder(port)
.response("irrelevant".to_string(), 42)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
let request = b"GET /booga\r\nContent-Length: 4\r\n\r\nbody";
Expand All @@ -119,7 +120,7 @@ mod tests {
let port = find_free_port();
let _subject = MockBlockchainClientServer::builder(port)
.response("irrelevant".to_string(), 42)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
let request = b"GET /booga HTTP/2.0\r\nContent-Length: 4\r\n\r\nbody";
Expand Down Expand Up @@ -155,7 +156,7 @@ mod tests {
age: 37,
}),
)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);

Expand Down Expand Up @@ -217,7 +218,7 @@ mod tests {
},
42,
)
.run_on_docker()
.run_in_docker()
.start();
let mut client = connect(port);
let request =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ fn debtors_are_credited_once_but_not_twice() {
}],
1,
)
.run_on_docker()
.run_in_docker()
.start();
// Start a real Node pointing at the mock blockchain client with a start block of 1000
let node_config = NodeStartupConfigBuilder::standard()
Expand Down Expand Up @@ -144,7 +144,7 @@ fn debtors_are_credited_once_but_not_twice() {
let config_dao = config_dao(&node_name);
assert_eq!(
config_dao.get("start_block").unwrap().value_opt.unwrap(),
"2001"
"2000"
Syther007 marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
Expand Down
66 changes: 32 additions & 34 deletions node/src/accountant/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ pub struct ReceivedPayments {
// detects any upcoming delinquency later than the more accurate version would. Is this
// a problem? Do we want to correct the timestamp? Discuss.
pub timestamp: SystemTime,
pub scan_result: Result<PaymentsAndStartBlock, ReceivedPaymentsError>,
pub payments_and_start_block: PaymentsAndStartBlock,
pub response_skeleton_opt: Option<ResponseSkeleton>,
}

Expand Down Expand Up @@ -501,17 +501,17 @@ impl Accountant {
if !self.our_wallet(wallet) {
match self.receivable_dao
.as_ref()
.more_money_receivable(timestamp,wallet, total_charge) {
.more_money_receivable(timestamp, wallet, total_charge) {
Ok(_) => (),
Err(ReceivableDaoError::SignConversion(_)) => error! (
Err(ReceivableDaoError::SignConversion(_)) => error!(
self.logger,
"Overflow error recording service provided for {}: service rate {}, byte rate {}, payload size {}. Skipping",
wallet,
service_rate,
byte_rate,
payload_size
),
Err(e)=> panic!("Recording services provided for {} but has hit fatal database error: {:?}", wallet, e)
Err(e) => panic!("Recording services provided for {} but has hit fatal database error: {:?}", wallet, e)
};
} else {
warning!(
Expand All @@ -535,9 +535,9 @@ impl Accountant {
if !self.our_wallet(wallet) {
match self.payable_dao
.as_ref()
.more_money_payable(timestamp, wallet,total_charge){
.more_money_payable(timestamp, wallet, total_charge) {
Ok(_) => (),
Err(PayableDaoError::SignConversion(_)) => error! (
Err(PayableDaoError::SignConversion(_)) => error!(
self.logger,
"Overflow error recording consumed services from {}: total charge {}, service rate {}, byte rate {}, payload size {}. Skipping",
wallet,
Expand Down Expand Up @@ -1404,7 +1404,7 @@ mod tests {
subject_addr.try_send(BindMessage { peer_actors }).unwrap();
let received_payments = ReceivedPayments {
timestamp: SystemTime::now(),
scan_result: Ok(make_empty_payments_and_start_block()),
payments_and_start_block: make_empty_payments_and_start_block(),
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 4321,
Expand Down Expand Up @@ -1597,6 +1597,10 @@ mod tests {
context_id: 4321,
})
);
assert_eq!(
payments_instructions.agent.arbitrary_id_stamp(),
agent_id_stamp
);
assert_eq!(blockchain_bridge_recording.len(), 1);
test_use_of_the_same_logger(&logger_clone, test_name)
// adjust_payments() did not need a prepared result which means it wasn't reached
Expand Down Expand Up @@ -1709,6 +1713,10 @@ mod tests {
let blockchain_bridge_recording = blockchain_bridge_recording_arc.lock().unwrap();
let payments_instructions =
blockchain_bridge_recording.get_record::<OutboundPaymentsInstructions>(0);
assert_eq!(
payments_instructions.agent.arbitrary_id_stamp(),
agent_id_stamp_second_phase
);
assert_eq!(
payments_instructions.affordable_accounts,
affordable_accounts
Expand Down Expand Up @@ -2039,13 +2047,14 @@ mod tests {
.build();
let system = System::new("accountant_uses_receivables_dao_to_process_received_payments");
let subject = accountant.start();
let mut scan_result = make_empty_payments_and_start_block();
scan_result.payments = vec![expected_receivable_1.clone(), expected_receivable_2.clone()];
scan_result.new_start_block = 123456789;
let mut payments_and_start_block = make_empty_payments_and_start_block();
payments_and_start_block.payments =
vec![expected_receivable_1.clone(), expected_receivable_2.clone()];
payments_and_start_block.new_start_block = 123456789;
subject
.try_send(ReceivedPayments {
timestamp: now,
scan_result: Ok(scan_result),
payments_and_start_block,
response_skeleton_opt: None,
})
.expect("unexpected actix error");
Expand Down Expand Up @@ -3431,10 +3440,10 @@ mod tests {
init_test_logging();
let port = find_free_port();
let pending_tx_hash_1 =
H256::from_str("d89f74084be2601c816fb85b8eac6541437223ad4851d12e9eb3d6f74570b8ae")
H256::from_str("e66814b2812a80d619813f51aa999c0df84eb79d10f4923b2b7667b30d6b33d3")
.unwrap();
let pending_tx_hash_2 =
H256::from_str("05981aa8d6c8ca1661f56a42e6e0c1aa56c9c9d0ecf26755b4388826aad55811")
H256::from_str("0288ef000581b3bca8a2017eac9aea696366f8f1b7437f18d1aad57bccb7032c")
.unwrap();
let _blockchain_client_server = MBCSBuilder::new(port)
// Blockchain Agent Gas Price
Expand All @@ -3446,10 +3455,6 @@ mod tests {
"0x000000000000000000000000000000000000000000000000000000000000FFFF".to_string(),
0,
)
// Blockchain Agent tx_id
.response("0x2".to_string(), 1)
// gas_price
.response("0x3B9ACA00".to_string(), 1)
// Submit payments to blockchain
.response("0xFFF0".to_string(), 1)
.begin_batch()
Expand Down Expand Up @@ -3510,7 +3515,6 @@ mod tests {
)
.end_batch()
.start();

let non_pending_payables_params_arc = Arc::new(Mutex::new(vec![]));
let mark_pending_payable_params_arc = Arc::new(Mutex::new(vec![]));
let return_all_errorless_fingerprints_params_arc = Arc::new(Mutex::new(vec![]));
Expand Down Expand Up @@ -3538,15 +3542,15 @@ mod tests {
gwei_to_wei(DEFAULT_PAYMENT_THRESHOLDS.debt_threshold_gwei + 666);
let wallet_account_1 = make_wallet("creditor1");
let wallet_account_2 = make_wallet("creditor2");
let blockchain_interface = make_blockchain_interface_web3(Some(port));
let blockchain_interface = make_blockchain_interface_web3(port);
let consuming_wallet = make_paying_wallet(b"wallet");
let system = System::new("pending_transaction");
let persistent_config_id_stamp = ArbitraryIdStamp::new();
let persistent_config = PersistentConfigurationMock::default()
.set_arbitrary_id_stamp(persistent_config_id_stamp);
let blockchain_bridge = BlockchainBridge::new(
Box::new(blockchain_interface),
Box::new(persistent_config),
Arc::new(Mutex::new(persistent_config)),
false,
);
let account_1 = PayableAccount {
Expand Down Expand Up @@ -3628,8 +3632,6 @@ mod tests {
no_rowid_results: vec![],
});
let mut pending_payable_dao_for_pending_payable_scanner = PendingPayableDaoMock::new()
.insert_fingerprints_result(Ok(()))
.insert_fingerprints_result(Ok(()))
.return_all_errorless_fingerprints_params(&return_all_errorless_fingerprints_params_arc)
.return_all_errorless_fingerprints_result(vec![])
.return_all_errorless_fingerprints_result(vec![
Expand Down Expand Up @@ -3706,9 +3708,7 @@ mod tests {

assert_eq!(system.run(), 0);
let mut mark_pending_payable_params = mark_pending_payable_params_arc.lock().unwrap();

let mut one_set_of_mark_pending_payable_params = mark_pending_payable_params.remove(0);

assert!(mark_pending_payable_params.is_empty());
let first_payable = one_set_of_mark_pending_payable_params.remove(0);
assert_eq!(first_payable.0, wallet_account_1);
Expand All @@ -3733,7 +3733,7 @@ mod tests {
vec![
vec![rowid_for_account_1, rowid_for_account_2],
vec![rowid_for_account_1, rowid_for_account_2],
vec![rowid_for_account_2]
vec![rowid_for_account_2],
]
);
let mark_failure_params = mark_failure_params_arc.lock().unwrap();
Expand Down Expand Up @@ -3771,16 +3771,16 @@ mod tests {
);
let log_handler = TestLogHandler::new();
log_handler.exists_log_containing(
"WARN: Accountant: Broken transactions 0xd89f74084be2601c816fb85b8eac6541437223ad4\
851d12e9eb3d6f74570b8ae marked as an error. You should take over the care of those to make sure \
"WARN: Accountant: Broken transactions 0xe66814b2812a80d619813f51aa999c0df84eb79d10f\
4923b2b7667b30d6b33d3 marked as an error. You should take over the care of those to make sure \
your debts are going to be settled properly. At the moment, there is no automated process \
fixing that without your assistance");
log_handler.exists_log_matching("INFO: Accountant: Transaction 0x05981aa8d6c8ca1661f56a42e6e\
0c1aa56c9c9d0ecf26755b4388826aad55811 has been added to the blockchain; detected locally at \
log_handler.exists_log_matching("INFO: Accountant: Transaction 0x0288ef000581b3bca8a2017eac9\
aea696366f8f1b7437f18d1aad57bccb7032c has been added to the blockchain; detected locally at \
attempt 4 at \\d{2,}ms after its sending");
log_handler.exists_log_containing(
"INFO: Accountant: Transactions 0x05981aa8d6c8ca1661f56a42e6e0c1aa56c9c9d0e\
cf26755b4388826aad55811 completed their confirmation process succeeding",
"INFO: Accountant: Transactions 0x0288ef000581b3bca8a2017eac9aea696366f8f1b7437f18d1aad5\
7bccb7032c completed their confirmation process succeeding",
);
}

Expand Down Expand Up @@ -3864,7 +3864,6 @@ mod tests {
let amount_1 = 12345;
let hash_2 = make_tx_hash(0x1b207);
let amount_2 = 87654;

let hash_and_amount_1 = HashAndAmount {
hash: hash_1,
amount: amount_1,
Expand All @@ -3873,7 +3872,6 @@ mod tests {
hash: hash_2,
amount: amount_2,
};

let init_params = vec![hash_and_amount_1, hash_and_amount_2];
let init_fingerprints_msg = PendingPayableFingerprintSeeds {
batch_wide_timestamp: timestamp,
Expand Down Expand Up @@ -4735,7 +4733,7 @@ mod tests {
fn checked_conversion_without_panic() {
let result = politely_checked_conversion::<u128, i128>(u128::MAX);

assert_eq!(result,Err("Overflow detected with 340282366920938463463374607431768211455: cannot be converted from u128 to i128".to_string()))
assert_eq!(result, Err("Overflow detected with 340282366920938463463374607431768211455: cannot be converted from u128 to i128".to_string()))
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl BlockchainAgent for BlockchainAgentNull {
}
}

fn agreed_fee_per_computation_unit(&self) -> u64 {
fn agreed_fee_per_computation_unit(&self) -> u128 {
self.log_function_call("agreed_fee_per_computation_unit()");
0
}
Expand All @@ -37,11 +37,6 @@ impl BlockchainAgent for BlockchainAgentNull {
&self.wallet
}

fn pending_transaction_id(&self) -> U256 {
self.log_function_call("pending_transaction_id()");
U256::zero()
}

#[cfg(test)]
fn dup(&self) -> Box<dyn BlockchainAgent> {
intentionally_blank!()
Expand Down Expand Up @@ -176,17 +171,4 @@ mod tests {
assert_eq!(result, &Wallet::null());
assert_error_log(test_name, "consuming_wallet")
}

#[test]
fn null_agent_pending_transaction_id() {
init_test_logging();
let test_name = "null_agent_pending_transaction_id";
let mut subject = BlockchainAgentNull::new();
subject.logger = Logger::new(test_name);

let result = subject.pending_transaction_id();

assert_eq!(result, U256::zero());
assert_error_log(test_name, "pending_transaction_id");
}
}
Loading