GH-744-Review-sub-2 #553
GH-744-Review-sub-2 #553
Conversation
…ess_transaction_receipts
…_receipts_with_an_empty_vector
| @@ -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" | |||
There was a problem hiding this comment.
This is serious. Apparently by the comment above, and from some memory I have of this test, its whole existence is predetermined by establishing an evidence that the next start block will be set to the current head block plus one (if the head block is known - and not represented as "latest") or by the youngest detected transaction (as the opposite of the previous text in brackets) but also plus one. The "plus one" part is integral. If you change your assertion like this, it breaks the intention of the test.
I noticed that the issue might be that somebody had probably set the test up wrong before you guys.
If I'm right, and I'm not sure, then the following line
.response("0x5DC", 1) // eth_blockNumber 1500 isn't correct as it should be response to the query of the latest block number, but it is smaller than the block number which is supposed to hold in the only transaction detected which evaluates to 2000. I think the first value would never be smaller in the real world, because of its interstitial logic...
| @@ -654,6 +654,8 @@ impl PendingPayableScanner { | |||
| msg: ReportTransactionReceipts, | |||
| logger: &Logger, | |||
| ) -> PendingPayableScanReport { | |||
| // TODO: We want to ensure that failed transactions are not marked still pending, | |||
| // and also adjust log levels accordingly. | |||
There was a problem hiding this comment.
What's this? Still relevant or can it be erased?
|
|
||
| #[derive(Debug, PartialEq, Clone)] | ||
| #[allow(clippy::large_enum_variant)] | ||
| pub enum TransactionReceiptResult { | ||
| NotPresent, | ||
| Found(TransactionReceipt), | ||
| Error(String), | ||
| TransactionFailed(TransactionReceipt), // RemoteFailure | ||
| Error(String), // LocalFailure |
There was a problem hiding this comment.
I might appreciate if you succeeded incorporating in this name that it is "ours" or "local" or using any other word of a similar meaning...
It would be more helpful than just a comment because it could co be immediately understood from anywhere it is used at.
| }], | ||
| new_start_block: 1000000000 | ||
| } | ||
| ); |
There was a problem hiding this comment.
I advised you to delete this test as I realized it had been a mistake in the first place to write it and add ex-post. This test was created as some sort of hardening. It didn't test-drive anything. I just became aware back then that we don't really guarantee in our tests that an actor, if it needs a connection to an external data source, ends up equipped with exactly such which would work as we think they should...
Later on, it started to be clearer than ever to me that I was pondering over integration tests which I was trying to squeeze into the suit of rather unit tests. This whole effort was rather sick. And for you guys, seeing you straggle with trying to keep a good maintenance of these, I came up with an argument that there are actually much much higher level integration tests (multinode tests) which prove the same crap. It was to me that you should be liberated from this task and I begun urging you about deleting at least this test in particular.
You seem to have got worried of this action, as you did multiple times when I asked you to delete some code. My opinion is you're overly careful sometimes, but I tend to get why you are too.
Anyway, I do think the best solution is to get rid of this hybrid between unit and integration tests which has never worked well and has been indicating it to us constantly by the impossible elegance to be made over it. It's always a good sign something's just somebody's really wrong thinking, this time mine.
PS: Why not keeping it? Mostly because even if we insisted the test only does half the objectives promised as it covers only the BlockcahinInterface connection (the blockchain service provider's one) and it has no assertions for the PersistentConfig (a database connection).
| } | ||
|
|
||
| #[test] | ||
| fn transaction_receipt_batch_works() { |
There was a problem hiding this comment.
I don't get why you also deleted this test meant for the batched version. My take is that this is going to be in the production code. It should be tested. I find this place quite appropriate then. It should actually have maybe more than one test, with other than just happy results, unless you join all the possible situations together within a single test as it seems you did with this in the pick code.
Otherwise, deleting the suite related to the non-batch receipt processing is what I applaud for.
| &self, | ||
| logger: Logger, | ||
| chain: Chain, | ||
| agent: Box<dyn BlockchainAgent>, |
There was a problem hiding this comment.
It would be quite cool, if the agent could simply answer what the chain is. This agent should know about the details around the blockchain as much as nobody else, therefore it looks to me a bit ugly if I see that there are two arguments, the agent and the chain, instead of passing in only one, the agent.
If you think you don't have time to do it, I'll understand.
| @@ -186,175 +161,13 @@ impl ReceiptResponseBuilder { | |||
|
|
|||
| let rpc_response = RpcResponse { | |||
There was a problem hiding this comment.
I cannot comment in a higher place so I'm doing here:
I'm surprised that you've preserved the ReceiptResponseBuilder. Does it also mean we still fetch the entire TransactionReceipt during the retrieve_transactions process? We discussed and concluded that this structure is unbearably excessive compared our needs (just a little fraction).
| .build(); | ||
| // Get the dns_retry_result recipient so we can partially mock it... | ||
| let dns_retry_result_recipient = peer_actors.proxy_server.route_result_sub; | ||
| peer_actors.proxy_server.route_result_sub = dns_retry_result_recipient; //Partial mocking |
There was a problem hiding this comment.
Lines 2622 - 2624 are meaningless, if you think harder of them. Kick them off please.
| @@ -1228,6 +1235,44 @@ impl Hostname { | |||
| }; | |||
| Self { hostname } | |||
| } | |||
|
|
|||
| fn is_valid(&self) -> Result<(), String> { | |||
There was a problem hiding this comment.
The name is_valid isn't particularly well chosen. These names with "is" imply usually a boolean to be on the function's return.
Maybe you want something like just "validate".
| @@ -128,7 +128,7 @@ impl Wallet { | |||
| WalletKind::Address(address) => H160(address.0), | |||
| WalletKind::PublicKey(public) => H160(*public.address()), | |||
| WalletKind::SecretKey(key_provider) => key_provider.address(), | |||
| WalletKind::Uninitialized => panic!("No address for an uninitialized wallet!"), | |||
| WalletKind::Uninitialized => panic!("No address for an uninitialized wallet!"), // TODO: If we can get rid of it, it'll be awesome! | |||
There was a problem hiding this comment.
Have you tried out guys? Not saying that you necessarily should.
| assert_eq!( | ||
| blockchain_agent_with_context_msg_actual | ||
| .agent | ||
| .agreed_fee_per_computation_unit(), | ||
| 10 | ||
| 9395240960 |
There was a problem hiding this comment.
Again, if this number comes from what the mocked blockchain service client server sends towards us, then I'd like to see the format of the number in the server's setup area being the same as here, in other words, if you use hexadecimal notations first please use the same value in hex here. It is much easier to check then.
| @@ -742,7 +758,7 @@ mod tests { | |||
| blockchain_agent_with_context_msg_actual | |||
| .agent | |||
| .estimated_transaction_fee_total(1), | |||
| 733280 | |||
| 688_934_229_114_880 | |||
| BlockchainTransaction { | ||
| block_number: 6040059, | ||
| from: make_wallet("first_wallet"), // Points to topics of 1 | ||
| wei_amount: 42, // Its points to the field data |
There was a problem hiding this comment.
Sry, I don't understand the comments and so I don't find them useful in the state they are.
| @@ -1652,20 +1584,20 @@ mod tests { | |||
| let earning_wallet = make_wallet("earning_wallet"); | |||
| let amount = 996000000; | |||
| let expected_transactions = RetrievedBlockchainTransactions { | |||
| new_start_block: 1000000001, | |||
| new_start_block: 1000000000, | |||
There was a problem hiding this comment.
Always careful with this...
Are you sure the new form is correct? I think it is, but be it requires you to fully understand when you're doing such changes.
Again, I think everybody would appreciate if you could keep this number in hex as well as the number you fetch through the server's response. My assumption is this equals to 0x3B9ACA00.
| .max_block_count_result(Err(PersistentConfigError::DatabaseError( | ||
| "my tummy hurts".to_string(), | ||
| ))) | ||
| .set_max_block_count_result(Ok(())); |
There was a problem hiding this comment.
This is not enough. I don't like that your assertion at the bottom is a simple DEBUG log. It's an indirect, and therefore a weak assertion. Instead you should provide a container for params in this methods by using
set_max_block_count_params(). You will assert on an actual call of this mocked method and that the value 1000 has been supplied. Such validation is much a stronger proof.
| let accounts = vec![make_payable_account(1), make_payable_account(2)]; | ||
| let os_code = transport_error_code(); | ||
| let os_msg = transport_error_message(); | ||
| let expected_result = Err(Sending { |
| init_test_logging(); | ||
| let test_name = "send_payables_within_batch_all_payments_fail"; | ||
| let accounts = vec![make_payable_account(1), make_payable_account(2)]; | ||
| let expected_result = Ok(vec![ |
| let os_code = transport_error_code(); | ||
| let os_msg = transport_error_message(); | ||
| let expected_result = Err(Sending { | ||
| msg: format!("Transport error: Error(Connect, Os {{ code: {}, kind: ConnectionRefused, message: {:?} }})", os_code, os_msg).to_string(), |
There was a problem hiding this comment.
Hmmm, this error should probably say something regarding the trigger point. So that we know, for example like here, that it happened during the call of submit_batch.
If there are more such places, then allow it to be a message compatible (given its variable part) with them all.
| init_test_logging(); | ||
| let test_name = "send_payables_within_batch_one_payment_works_the_other_fails"; | ||
| let accounts = vec![make_payable_account(1), make_payable_account(2)]; | ||
| let expected_result = Ok(vec![ |
| @@ -1111,7 +826,7 @@ mod tests { | |||
| let web3 = Web3::new(transport.clone()); | |||
| let chain = DEFAULT_CHAIN; | |||
| let amount = 11_222_333_444; | |||
| let gas_price_in_gwei = 123000000000_u64; | |||
| let gas_price_in_wei = 123_000_000_000_000_000_000; | |||
There was a problem hiding this comment.
You can use a pow() call. 123 * 10.pow(18). Also at other places 🤷♂️
| let start_block_nbr = 42u64; | ||
| let start_block = BlockNumber::Number(start_block_nbr.into()); | ||
| let fallback_number = calculate_fallback_start_block_number(start_block_nbr, u64::MAX); | ||
| // let fallback_number = BlockchainBridge::calculate_fallback_start_block_number(start_block_nbr, u64::MAX); |
There was a problem hiding this comment.
Shouldn't you delete this line? It might be needless now.
| ); | ||
| assert_eq!( | ||
| result[5], | ||
| TransactionReceiptResult::Found(TransactionReceipt { |
There was a problem hiding this comment.
Are you guys sure that we want to operate with these massive data structures from Web3. We held a discussion and I thought you might try to replace it with something closely ours with just as much in it as what we will utilize.
No description provided.