GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Syther007 · 2024-07-29T10:06:35Z

No description provided.

This reverts commit ffe6b81.

…ding_nonce

…hour_error_back_to_accountant

…s_finished_payments_back_to_accountant

…ds_finished_payments_back_to_accountant

…ts_back_to_accountant

…_if_no_payments_are_detected

masq/tests/startup_shutdown_tests_integration.rs

multinode_integration_tests/tests/blockchain_interaction_test.rs

node/src/accountant/scanners/mid_scan_msg_handling/payable_scanner/agent_web3.rs

node/src/blockchain/batch_web3.rs

bertllll · 2024-09-26T20:11:05Z

node/src/actor_system_factory.rs

+        assert_eq!(system.run(), 0);
+        let recording = accountant_recording.lock().unwrap();
+        let received_payments_message = recording.get_record::<ReceivedPayments>(0);
+        assert!(received_payments_message.scan_result.is_ok());


I was surprised when I saw you guys decided to rework this test. It's one of the most spiteful tests in the codebase, because it wasn't possible to write it elegantly. When I tried for that, I was so delved in the code that I might've forgotten to consider if the goal, to be sure about the two connections (BlockchainInterface - http, PersistentConfig - DB) being functional, hadn't been already included in some other, much higher level tests, and so I'm speaking of the integration ones actually. It would be the more elegant solution I touched, because it would feel smoothing if we could just put this unit test into the garbage because it even seems not to fit in well among the others (except the one just above, which is the same kind of bastard - ironically, also written in my person). So, that's the question... In an ideal word, I think, there should be at least one integration test which would do this favor for us. I'm not so positive there is.

Also, this test is an attempt to have an extremely responsible coverage, maybe beyond how much we should be willing to do for this.
The test was supposed to assure us that if we construct and start the BlockchainBridge, it will end up connected properly two two points. The correct database (file), and the correct Internet domain.

If you do want to keep this test (in other words, if you don't want to discard in on your responsibility), you'll need to do something about the assertions. Now they are just incomplete.

For the blockchain service endpoint, the assertion would be strongest if you could pull the received request out of the mock server and use it to prove the Actor sent it out.
For the database, which must basically be about only the config table (because the blockchain bridge has only such an access, via the PersistentConfiguration), you'd have to prove that the database changed the state, which is hard actually.

Because you moved the test body here from BlockchainBridge, you lost the advantage of being able to use the AssertionMessage and check whatever you want from inside the Actor. It's a nice trick, but the technical side of keeping the fields of the Actor private and still testing such things somewhere outside the Actor's home file, it's all pain.

To be honest, under your new setting I can't think of something what I could advise you. (I know you could theoretically respond by an error to the blockchain scan, saying the requested block range is too wide, because as far as I can recall, that results in using the PersistentConfig's job of writing the parsed max range into the database, and you could check the presence of the value in there, when the test is over).

You can have your private brainstorming over this or you can bring it up in the all-heads meeting. 🤷‍♂️

Would you accept adding further assertions to the received_payments_message to increase the strength of this test? or would you just prefer we delete this test as it could be misleading?

Hi buddy, I pondered how I could help you out and despite I'm in general into testing these niches that are left behind and ignored. What I'm ragging you with is what simply comes across as an integration test that should probably really prove that the blockchain bridge is connected to the right outside points that it depends on somehow. If further looked into, the both outside technologies meet in the receivable scanner cycle.

I first thought I'd give you advice which would be good enough to test it here. I just couldn't shake off the feeling that it would've been copying tightly unit tests testing those collaboration procedures with other processes, and the test itself would end up complicated, atypical and therefore rather confusing, and last but not least also a bit awkward.

So I then checked my other thought of integration tests we already have and it appeased me enough to say that we can stay without this test and you can delete it. The test (or tests in a plural since GH-711) in verify_bill_payments is probably the only one which actually works as an evidence of these connections being properly maintained and functioning. As long as that multimode test exists it's not so terrible important to have another test for a more hardened test coverage.

I'd be glad if you understood my standpoint at least partially, and took something away, as good programming is paved by building hypothesis that are, as time passes, more often demolished then found really useful. Hurrah.

(In short: Delete the whole test)

node/src/blockchain/blockchain_interface/lower_level_interface.rs

node/src/accountant/mod.rs

bertllll · 2024-10-01T17:40:01Z

node/src/blockchain/blockchain_interface/data_structures/errors.rs

-    ServiceFeeBalance(Wallet, BlockchainError),
-    TransactionID(Wallet, BlockchainError),
+    GasPrice(BlockchainError),
+    TransactionFeeBalance(Address, BlockchainError),


On one hand, I can imagine becoming a supporter for this transition from Wallet to Address, on the other one, I have quite a problem with accepting the worsen on-glimpse recognition of what this "address" term belongs to. If you see the type Wallet you can't really wonder about the same thing because it's so obvious.

Another thing I noticed is that Wallet can be made implement traits like Display as we need. However, Address alone is a foreign type (not written by us) and such cannot be treated the same way. It leads to disadvantages like having to use specially formed placeholders for proper displaying. There might be more than that though.

That's why I don't feel convinced that this is a clearly better solution. Despite I know the reason for this is that Wallet can be an unnecessarily complicated structure while it's often used only for remembering the wallet's address.

I think this is for a wider discussion. I'd be happy if we could have some brainstorming on this topic in one of the meetings with Dan.

node/src/blockchain/blockchain_interface/lower_level_interface.rs

node/src/blockchain/blockchain_interface_initializer.rs

node/src/test_utils/persistent_configuration_mock.rs

node/src/neighborhood/mod.rs

bertllll · 2024-10-08T21:54:43Z

masq_lib/src/test_utils/mock_blockchain_client_server.rs

+                )))
+            } else {
+                Left(self.port)
+            },


I was scanning the code up and down for some time being here and this is a place which I don't think is refactored enough.

Have please also a look at the first four lines in start() of MockBlockchainClientServer at lines 195-198. I reasoned about the fact this server is always constructed through its builder. That means what is inside the start() can theoretically be also inside this place without any change in functionality, and if it semantically works.

I think you will be able to get rid of the dubious port_or_local_addr with an Either. Instead, you can take care of the translation from an only port into a socket addr straight here before you pass it on in.

The idea is that the cited other start function does not do anything with the port number except in the socket addr. And we said already that such an operation can happen earlier if wanted. So maybe you want to change the field into:

local_adrr: SocketAddr

and use your if statement to either construct 172:18:0:1:port socket or localhost() + port socket. Quite simple.

another concern of mine is that the line 117 diverges from the former way how the address was picked out, and I have a feeling it could make some difference even though just small. It used to utilize the DockerHostSocketAddr whose effect is that it allows more than just one address to pick from, but two respectively. Probably if one is taken up for a reason the second may still work. I didn't want to go track it down but I feel like it will cost us nothing if you go back to using it as we did before. Thanks.

Hi Bert,

currently we feel this way of doing things adds a lot of convenience when using MBCS inside of tests.

I don't understand, honestly. I was only suggesting some refactoring which actually wouldn't surface on the public interface of the mock server. In other words, it has nothing to do with the way this utility is used in your tests. It only concerns the "hidden" process of transformation from the MBCSBuilder into the MockBlockchainClientServer.

Can you please reconsider it?

masq_lib/src/test_utils/mock_blockchain_client_server.rs

multinode_integration_tests/src/mock_blockchain_client_server.rs

masq_lib/src/test_utils/mock_blockchain_client_server.rs

node/src/blockchain/blockchain_interface/mod.rs

node/src/blockchain/blockchain_interface/test_utils.rs

bertllll

Continue from node/src/blockchain/blockchain_interface_utils.rs at line 33

node/src/accountant/scanners/mod.rs

bertllll · 2024-10-12T22:48:12Z

node/src/accountant/scanners/mod.rs

+                ReceivedPaymentsError::ExceededBlockScanLimit(max_block_count) => {
+                    debug!(logger, "Writing max_block_count({})", max_block_count);
+                    self.persistent_configuration
+                        .set_max_block_count(Some(max_block_count))


Okay, I'm beginning to fully understand what you had in mind when going for these changes. You moved the processing of the exceeded block count over here so that it resides together with the other database writes we do in the config table for the blockchain. Not a vain idea.

I feel like we got to solve discrepancies that deserve to call the tech lead in. We need to make it right. I'm saying that extending the competence of this scanner isn't advisable and we should seek a solution bringing this back off.

The challenging underlaying problem, though, is that we seem to be spending time regularly by questioning the location of set_start_block and its close equivalents. Or at least, I know I've done it myself a lot recently despite it's quite clear to me that there are technical obstacles that probably wouldn't let us get better off than with what we are having now.

In the science fiction, we'd love to bring an open rusqlite transaction with a couple of update statements made on behalf of the receivable table and carry it in an Actor message (!!!) back to BlockchainBridge, just to let it modify the entry for start_block in the config table (as of information that is familiar to this Actor) and commit it. As far as I can see forward this would never work because the transaction constitutes also of a reference to the connection which gave it life. Looking at an Actor message to hypothetically be carrying it, the idea falls apart because the message cannot contain referenced objects by definition. Everything put in must be owned.

Still please go ahead and ask Dan for help in designing it. Discussions like this tend to be beneficial anyway.

The Block count is fully updated and tested. This was part of a new design.

I understand that this works, and is tested probably thoroughly, but it is conceptually wrong in its fundaments. That is from the standpoint of "separate responsibilities", which we want to be serious about even in small units, but the more in the terms of an entire actor and its set of responsibilities. This change you made drift us away from cleanly defined competence of the Accountant.

Again, in the ideal world, this would require keeping this actor clean of anything that refers to the blockchain technology. We've concluded in the past that it cannot be done entirely so, or it would be significantly hard. More was said in my previous comment already.

The problem with your new change is that it could easily have been handled in the former location, the BlockchainBridge, which is the Actor which should be looking after all the things with the Blockchain, and so also setting set_max_block_count.

In my eyes, this could have an actual adverse impact in a long term. Please, at least bring this up with Dan. He should be who decides these quite top-level architectural things.

STATUS: SERIOUS CONCEPTUAL CONCERN. DAN'S COMMENT HIGHLY DESIRABLE. This is interesting though, the text above doesn't mention an important motivation that probably incurred. The persistent configuration seemed impossible to be moved in a closure for a future, because we cannot just clone it from the one stored in the actor's body. There's already a set of ideas I wrote for covering this: #456 (comment)

bertllll · 2024-10-12T23:01:19Z

node/src/accountant/scanners/mod.rs

+                    debug!(logger, "Writing max_block_count({})", max_block_count);
+                    self.persistent_configuration
+                        .set_max_block_count(Some(max_block_count))
+                        .map_or_else(|_| {


Despite this piece of code will very likely have to be erased, map_or_else is believed to be hard to read, at least such voices have been heard in our team (including from me), so it's recommend to replace it by a sequence of two other methods like

something_happening_before + map + map_err

Those two separate maps of different kind read better.

node/src/accountant/scanners/mod.rs

bertllll · 2024-10-12T23:14:12Z

node/src/accountant/scanners/mod.rs

-                new_start_block, e
-            ),
-        }
+    fn handle_new_received_payments(


Even the name alone is a problem a bit. Because it makes you believe some payments were received while you can easily find out soon after you enter, that none was detected.

bertllll · 2024-10-12T23:22:22Z

node/src/accountant/scanners/mod.rs

-                new_start_block, e
-            ),
-        }
+    fn handle_new_received_payments(


The destiny of the one look of this method will hinge on what comes out of the discussion with Dan about this area of our code as suggested before.

node/src/blockchain/blockchain_interface_utils.rs

bertllll · 2024-10-12T23:42:31Z

node/src/blockchain/blockchain_interface_utils.rs

+use web3::types::{Bytes, SignedTransaction, TransactionParameters, H256, U256};
+use web3::Error as Web3Error;
+use web3::Web3;
+


I've already seen some code of this file and understood the philosophy of your actions. I'm not writing this to oppose you, but I just want to record a piece of advice. I can't say from here if it is to work but I'm thinking of limiting the visibility of these functions is really desirable. Therefore the root file of the BlockchainBridge node/src/blockchain/mod.rs should definitely have the line containing a declaration of a bound to this module, that is actually this file, without the keyword pub. Why do I care? I just feel a tension in me because of some many public functions potentially contaminating the namespace for more than just the blockchain bridge but even places from much further away from here.

If you make sure this mod is private and so only reachable from the root mod of the BlockchainBridge, then it should leave you more in peace as it would with me, because only the BlockchainBridge will be aware of them.

Not a fatal problem. I just wish you could remember for future occasions.

Hi Bert,

Could you please shed some more clarification on this.
The file blockchain_interface_utils.rs is not mark as pub inside mod.rs

Good. My apologies, I may've been looking badly at it.

Anyway, still please remember that this is potentially rather an inadvisable practise. Creating a file with a lot of helper functions, all being public, if it's also coupled with the pub identifier where the individual modules are linked together could bring about namespace pollution and the work becoming slightly less comfortable.

bertllll · 2024-10-13T10:23:16Z

node/src/blockchain/test_utils.rs

+}
+
+#[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq)]
+pub struct RpcResponse<S: Serialize> {


This structure doesn't resemble test code. Is it the case it doesn't take place in production at all? If it does, it doesn't belong here.

A good way of pursuing this principle is to add #![cfg(test)] at the top of the file or there is an alternative to this (and I myself hate this inconsistency in the possible treatments) in the respective mod file, situated above this one in the cargo project's file system, that clusters up all the other files in the directory (you may know very well, as these are those lines starting with the mod keyword), which is where you can simply localise your particular file (test_utils.rs) and superscribe it by [cfg(test)].

No matter what solution you chose this will always play in your favour because everything which would be accidentally declared inside but used also in the production code would inevitably direct to a compilation error.

node/src/blockchain/test_utils.rs

bertllll · 2024-10-13T10:29:11Z

node/src/blockchain/test_utils.rs

 #[derive(Default)]
 pub struct BlockchainInterfaceMock {
-    retrieve_transactions_parameters: Arc<Mutex<Vec<(BlockNumber, BlockNumber, Wallet)>>>,
+    retrieve_transactions_parameters: Arc<Mutex<Vec<(BlockNumber, u64, Address)>>>,


Again, this is more addressed to Utkarsh, just looking at the types, I don't feel like it was a change to the better. This kind of types joggling is not really my cup of tea.

bertllll · 2024-10-13T10:34:37Z

node/src/blockchain/test_utils.rs

    lower_interface_result: Option<Box<LowBlockchainIntMock>>,
    arbitrary_id_stamp_opt: Option<ArbitraryIdStamp>,
+    get_chain_results: RefCell<Vec<Chain>>,


Suggestion: Would make sense to have this as Option<Chain> instead, because it's a value that's not believed to be ever changing over time. If you get a certain chain once you will want to get it every time. On the other hand, by that, you lose the chance of a panic because of unexpected count of calls on this method (if you prepared two results in the queue but the exercised code comes across this method three times. Which is supposed to be treated by the params assertions anyway, but I understand that it's already a lot that you'd have to bear in mind writing a good test.) That's why it's a suggestion. I'm not writing this to push you.

bertllll · 2024-10-13T10:56:52Z

node/src/blockchain/test_utils.rs

+        serde_json::to_string(&rpc_response).unwrap()
+    }
+}
+
 #[derive(Default)]
 pub struct BlockchainInterfaceMock {


I'm sorry for causing troubles but I must state that this is another place where I sense some rough abrasive. This mock was turned into pathetic remains of a useful tool. When you scan over its trait implementation you see all methods not needed except one. That's eyebrow-raising and it indicates something's wrong.

Usually it simply says that some code was left untested. I know, however, this is rather an effect of your and Utkarsh's experimenting with heavy usages of the MockBlockchainClientServer by whose configuration you try to achieve the tested code, staying in the production version, unaltered, will receive a response identical to a real-world scenario. It's impressive on one hand, but it goes badly together with the standard mocks which we historically have been using a lot. In such a combination, especially if the standard mock is involved significantly partially only, it isn't simply good.

I'd love if we could also look into this more closely with Dan's eyes with us.

STATUS: MEDIUM CONCEPTUAL CONCERN. DAN'S COMMENT HIGHLY DESIRABLE. AT LEAST A CARD CLEANING THIS UP SHOULD BE MADE.

Hi, I talked a bit on this with Dan and we have this suggestion:
It seems like the desirable thing to do is actually discard this mock, but just ultimately. Before that you'd have to go look at the test using somehow retrieve_transactions. It's the only method that might be in use. If you prove true that there is such a test, check again if that test could also be written more in the way you have applied in those others, with the sturdy BlockchainClientMockServer.

The last thing to consider is how hard it would be to do this extra work within this open card (maybe it wouldn't be). We thought that it might be potentially a good solution for case it's harder, if you wrote a minor card about a clean-up. To rewrite any remaining dependent test and to erase the whole class of this mock off.

We think you have the right knowledge to decide by yourself which path to go.

bertllll · 2024-10-13T22:04:18Z

node/src/blockchain/test_utils.rs

    }

-    fn lower_interface(&self) -> &dyn LowBlockchainInt {
-        self.lower_interface_result.as_ref().unwrap().as_ref()
+    fn lower_interface(&self) -> Box<dyn LowBlockchainInt> {


If you've made it return owned objects, because the use of Box has this attributed to it, it means that you must to construct something every time this method is called. It's in a contradiction to the declaration of the BlockchainInterfaceMock because it's traditional to have a queue with results there, but you implemented Option. It implies that if somebody ever wants to design the production code so that your test could call this method more than once, this design of the mock wouldn't work, it would panic inevitably.

That's why it's better (and it's always of the first choice if not a special case) to pick a vector with an undefined number of returnable results to be prepared for a test.

(I feel like it's too detailed already because I now need to use a teleport and move to the trait declaration and comment on problematics with the boxed structure if we start looking at it using a more general perspective)

node/src/blockchain/blockchain_interface/mod.rs

node/src/blockchain/test_utils.rs

bertllll · 2024-10-13T23:08:36Z

node/src/blockchain/test_utils.rs

+        r: Default::default(),
+        s: Default::default(),
+        raw_transaction: Default::default(),
+        transaction_hash: Default::default(),


Can you just check out if the web3 library's SignedTransaction structure by any chance implements the Default trait as a whole. It would make this much briefer... eh... it would be a straight substitution for this entire piece of code. That's why I believe you had thought of this yourselves.

But this kind of check shouldn't be hard, so please do it to be sure.

node/src/blockchain/test_utils.rs

bertllll

Continue at node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs with the test section from its beginning.

node/src/blockchain/blockchain_interface_utils.rs

bertllll · 2024-10-14T16:34:35Z

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

    0xdd, 0xf2, 0x52, 0xad, 0x1b, 0xe2, 0xc8, 0x9b, 0x69, 0xc2, 0xb0, 0x68, 0xfc, 0x37, 0x8d, 0xaa,
    0x95, 0x2b, 0xa7, 0xf1, 0x63, 0xc4, 0xa1, 0x16, 0x28, 0xf5, 0x5a, 0x4d, 0xf5, 0x23, 0xb3, 0xef,
 ]);

-const TRANSFER_METHOD_ID: [u8; 4] = [0xa9, 0x05, 0x9c, 0xbb];
+pub const TRANSFER_METHOD_ID: [u8; 4] = [0xa9, 0x05, 0x9c, 0xbb];


It's so suspicious that you had to claim the public visibility for these two constants above. This data is so narrowly specific. It cannot be used anywhere but in web3 and you can imagine that this should be looked for deep in the code.

If the scent of it makes us think about some internal code why would it then need to be accessible from outside, because that's what the pub word is good for in a nutshell? Well, I guess these constants are only in the wrong file, unlike to a file in which they are actually planted in the code.

Resumé: You probably want to have the constants declared there where they are actually needed.

We think moving blockchain_interface_utils inside of blockchain_interface_web3 will help with this

I consider it less important now. Feel free to ignore this.

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

bertllll · 2024-10-14T17:30:45Z

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

-            .wait()
+    fn lower_interface(&self) -> Box<dyn LowBlockchainInt> {
+        Box::new(LowBlockchainIntWeb3::new(
+            self.transport.clone(),


As I stated elsewhere, I do believe, it should be entirely possible to sustain a linkage from the web3 transport stored in self.

And I will repeat it again in this place, it may not be so clear why I call out for this, however, that's just because you limit your understanding of the purpose of this code to only the web3 technologies and the derived Rust library making the support, but we need to be ready for eventualities that we will combine more libraries, to support different chains. And a reference here, instead of trying to construct something fully owned, is a more certain solution with regards to the unknown future.

This is part of our new design, it helps us to construct web3 and web3 batch on demand
These new instances although they are new objects they are still pointing to the same transport, which ultimately is pointing to the same connection with the same blockchain service provider.

I know, but this is evident only under the simplified present condition. Maybe this is a moment we should think a bit into future. Still, I commented elsewhere that I consider a hot -fix a comment about the caution implied there for the future.

Close this conversation.

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

node/src/blockchain/blockchain_interface_utils.rs

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

bertllll · 2024-10-17T16:55:02Z

node/src/blockchain/blockchain_interface_utils.rs

+const GWEI_UNIT: u64 = 1_000_000_000; // 1 Gwei = 1e9 Wei
+
+#[derive(Debug)]
+pub struct BlockchainAgentFutureResult {


Honestly, I don't see a reason for this to exist.

I checked the usage and it's so vague.

You first construct this structure of four already resolved futures, so basically ready-made values.
Next you take it and stick in the constructor of the BlockchainAgent. That alone seems to me too trivial to justify making the mid-step.

Please delete it and simply feed the constructor with 6 separate values instead of 3 (2 + this clustering struct).

bertllll · 2024-10-17T17:06:31Z

node/src/blockchain/blockchain_interface_utils.rs

+        .map(
+            |((rpc_result, hash_and_amount), account)| match rpc_result {
+                Ok(_rpc_result) => {
+                    // TODO: This rpc_result should be validated


Well, this is unfinished. That's one thing.

Also, I think it may not need to be used really, as it gives you a hash of the transaction and we know it already.
If you guys think it's right thinking of some sort of validation, and I'm rather skeptical about the need there, I guess you're thinking of something similar to:

We still have our own collection of hashes that are what we got in return when we signed the transactions.
Now, post sending, we also are getting new set of these hashes again. That teases you for maybe creating two Hashsets from both of those groups and then say: Are these HashSets identical? They should be. I don't anticipate there'd ever be a problem on that note but I won't hold you back. You decide as you please.

We have created a new card to resolve this TODO.
Validate RPC result on merged_output_data #547

node/src/blockchain/blockchain_interface_utils.rs

bertllll · 2024-10-17T20:32:26Z

node/src/blockchain/blockchain_interface_utils.rs

+    }
+
+    #[test]
+    fn sign_and_append_payment_just_works() {


This is also unimportant test... this all is already tested thoroughly

bertllll · 2024-10-17T20:41:15Z

node/src/blockchain/blockchain_interface_utils.rs

+        assert_eq!(result.hash, expected_hash);
+        assert_eq!(result.amount, 9000);
+    }
+


I do believe, but Utkarsh's opinion is welcomed, that all the suit of tests below this line are actually an example of huge misunderstanding. We've dragged them along for years, even when Substratum was thriving, but the truth about them is that it has nothing to do with our code and that they test purely and alone code of the web3 library. This efforts is valueless in my eyes. And I'm all for getting rid of the garbage. Except a few tests that do belong into our codebase and those are these three:

#[test] fn gas_limit_for_polygon_mumbai_lies_within_limits_for_raw_transaction() { test_gas_limit_is_between_limits(Chain::PolyAmoy); } #[test] fn gas_limit_for_polygon_mainnet_lies_within_limits_for_raw_transaction() { test_gas_limit_is_between_limits(Chain::PolyMainnet); } #[test] fn gas_limit_for_eth_mainnet_lies_within_limits_for_raw_transaction() { test_gas_limit_is_between_limits(Chain::EthMainnet) }

bertllll · 2024-10-18T17:38:47Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+pub struct LowBlockchainIntWeb3 {
+    web3: Web3<Http>,
+    web3_batch: Web3<Batch<Http>>,
+    contract: Contract<Http>,
    // TODO waiting for GH-707 (note: consider to query the balances together with the id)


This comment is obsolete

bertllll · 2024-10-18T17:43:18Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+            self.web3
+                .eth()
+                .balance(address, None)
+                .map_err(|e| QueryFailed(e.to_string())),


I liked how the old version mentioned also the wallet address, it could be quite useful.

bertllll · 2024-10-18T17:44:15Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+        Box::new(
+            self.contract
+                .query("balanceOf", address, None, Options::default(), None)
+                .map_err(|e| QueryFailed(e.to_string())),


bertllll · 2024-10-18T17:50:11Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+            self.web3
+                .eth()
+                .gas_price()
+                .map_err(|e| QueryFailed(e.to_string())),


I actually wonder if the natively returned error says enough about what the procedure was supposed to achieve.
Like if it was during fetching a gas price or something else. Do you know better, by any chance?

If you think it could bring an enhancement into the error processing, you may want to add it for all these errors returned by these methods enlisted for this trait, using the format!() macro to produce a message embracing also the actual error.

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

bertllll · 2024-10-18T20:37:25Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+    fn get_transaction_receipt_batch(
+        &self,
+        hash_vec: Vec<H256>,
+    ) -> Box<dyn Future<Item = Vec<TransactionReceiptResult>, Error = BlockchainError>> {


The returned native web3 structure is quite big. Keeping it just to read one tiny field later on might not be what we want. But I don't think I have the right feeling for it at the moment.

Maybe Dan could help us out with a sword to this Gordian knot...

Other arguments:

a) You guys even had to make an extra builder among the test utils just to construct this huge, complicated structure of which we need less then 10% information in it. The builder itself takes up quite a lot of space, which could've been saved from this, in the purest honesty.

b) And more importantly, you must not use any third-party data structure in your response because that would mean it couldn't be transposed to a different chain in its implementation. You will need to create your own struct and when you're dirty already, I really don't see a reason why it should hold so many information. It basically should only answer if to the questions we are interested in. No additional information which would be without a use. If such a need arises I'm sure it would be in a distant future but not now and we will ever be able to extend the list of parameters we would rely on from the transaction.

I made sure this is referred properly from another comment.

bertllll · 2024-10-18T20:40:14Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+            .gas_used(gas_used)
+            .status(status)
+            .logs_bloom(logs_bloom)
+            .build();


Yeah, this is quite a monster, too big piece of information where just a little fraction is useful and would likely ever be, and this builder will never have a good usecase of which reason it shouldn't exist.

bertllll · 2024-10-18T20:43:34Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+                    .to_string(),
+                7,
+            )
+            .raw_response(r#"{ "jsonrpc": "2.0", "id": 1, "result": null }"#.to_string())


I don't think this is how it works... you probably cannot just get this null.

bertllll · 2024-10-18T20:46:48Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+
+        let result = subject.lower_interface().get_gas_price().wait().unwrap();
+
+        assert_eq!(result, 1.into());


The most important part, the assertion checking what json was sent out, is missing. Pull it out as a record from the mock server and find a way to assert on it. Especially important piece of string is the opcode of the request produced.

bertllll · 2024-10-18T20:51:56Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+        assert_eq!(
+            result[3],
+            TransactionReceiptResult::Error(
+                "invalid type: string \"trash\", expected struct Receipt".to_string()
            )


Again, you're not asserting on the requests!! It's super important.

bertllll · 2024-10-18T21:51:09Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+                transaction_log_index: None,
+                log_type: None,
+                removed: Some(false),
+            }


This is another place where you need to place the assertions with a focus on the data send out towards the server.

bertllll · 2024-10-18T22:03:34Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+                  "transactionHash": "0x3dc91b98249fa9f2c5c37486a2427a3a7825be240c1c84961dfb3063d9c04d50",
+                  "transactionIndex": "0x1d"
+                }
+              ]


Please elevate somehow the chances the reader checking this test will discover why this data sent back will provoke this error. You are responsible for leaving the test in a state in which it wouldn't be too hard to figure it out, or rephrased, that he won't be in doubts about it and that's what just happened to me. I think it's specially unclear for this test, otherwise I'd hold back complaining.

bertllll · 2024-10-18T22:04:47Z

node/src/lib.rs

@@ -6,6 +6,7 @@ pub mod sub_lib;

 #[macro_use]
 extern crate masq_lib;
+extern crate core;


This might be removable without any troubles (it may not have a use for us)

node/src/neighborhood/mod.rs

bertllll · 2024-10-21T12:53:10Z

node/src/blockchain/blockchain_bridge.rs

                BlockchainInterfaceInitializer {}.initialize_interface(&url, chain)
            }
-            None => Box::new(BlockchainInterfaceNull::default()),
+            None => {
+                info!(logger, "The Blockchain service url is not set yet. its been defaulted to a wild card IP");


Maybe a warn log instead? ...as this literally means that "something" won't be working...

Plus, just correct the spelling of the end of the message: "It's been" instead of "its been".

Would be really nice to have this

bertllll · 2024-10-21T16:19:33Z

node/src/actor_system_factory.rs

+        assert_eq!(system.run(), 0);
+        let recording = accountant_recording.lock().unwrap();
+        let received_payments_message = recording.get_record::<ReceivedPayments>(0);
+        assert!(received_payments_message.scan_result.is_ok());


Hi buddy, I pondered how I could help you out and despite I'm in general into testing these niches that are left behind and ignored. What I'm ragging you with is what simply comes across as an integration test that should probably really prove that the blockchain bridge is connected to the right outside points that it depends on somehow. If further looked into, the both outside technologies meet in the receivable scanner cycle.

I first thought I'd give you advice which would be good enough to test it here. I just couldn't shake off the feeling that it would've been copying tightly unit tests testing those collaboration procedures with other processes, and the test itself would end up complicated, atypical and therefore rather confusing, and last but not least also a bit awkward.

So I then checked my other thought of integration tests we already have and it appeased me enough to say that we can stay without this test and you can delete it. The test (or tests in a plural since GH-711) in verify_bill_payments is probably the only one which actually works as an evidence of these connections being properly maintained and functioning. As long as that multimode test exists it's not so terrible important to have another test for a more hardened test coverage.

I'd be glad if you understood my standpoint at least partially, and took something away, as good programming is paved by building hypothesis that are, as time passes, more often demolished then found really useful. Hurrah.

(In short: Delete the whole test)

bertllll · 2024-10-21T16:33:45Z

node/src/actor_system_factory.rs

+impl ActorFactoryReal {
+    pub fn new() -> Self {
+        Self {
+            logger: Logger::new("ActorFactory"),


Some of the tests in this file should perform the thorough sequence of actor startups. One moment in that process should belong to the BlockchainBridge creation and startup. Those actors alone are actually all just mocked figures.

However, you added a Logger there which is not meant for an actor, it is meant to log messages before the startups, in the common area. That leads me to the idea that this logging must be also performed during such tests. First I thought that you got yourself into a situation which is so hard for testing that I'd spare you from being bothered about it. Later I realised that it shouldn't be such a big deal, also for the reasons exhibited previously.

So I'd wish you could identify a proper test in the suit in this file and add in one log assertion that begins like "INFO: ActorFactory: Blockchain service url has been set to " and continues with the actual host. Can you please complete the assertion to the full?

(Moral: It would expose us to just a marginal risk and damage but theoretically you could have named the Logger somehow silly, or in a misleading way. This test should look after this problematics.)

JUST NICE TO HAVE

bertllll · 2024-10-21T19:57:14Z

node/src/blockchain/blockchain_bridge.rs

-            agent,
-            incoming_message.response_skeleton_opt,
+    ) -> Box<dyn Future<Item = (), Error = String>> {
+        // TODO rewrite this into a batch call as soon as GH-629 gets into master


I'm not challenging you. Just asking you if you think this is this card or some after yours?

bertllll · 2024-10-21T19:58:35Z

node/src/blockchain/blockchain_bridge.rs

+    ) -> Box<dyn Future<Item = (), Error = String>> {
+        // TODO rewrite this into a batch call as soon as GH-629 gets into master
+        let accountant_recipient = self.payable_payments_setup_subs_opt.clone();
+        return Box::new(


The return words used at the last expression is really unusual in Rust. I wonder why Clippy didn't slap you over your fingers. :-)

bertllll · 2024-10-21T20:02:13Z

node/src/blockchain/blockchain_bridge.rs


-        locally_produced_result
+        return Box::new(


The return word again used unusually.

bertllll · 2024-10-21T20:10:18Z

node/src/blockchain/blockchain_bridge.rs

+                    });
+                    format!("ReportAccountsPayable: {}", e)
+                })
+                .and_then(move |payment_result| {


I just realised that the same can be achieved by the more intuitively-looking method .map(). I'm saying it because using the pair .map_err() and .map() might've been a bit cooler. Or it's possible that it's just a matter of personal preferences. For such reasons, I'm not bended towards an imperative tongue. This should be free to you.

However, if you happened to like the idea then you should probably change it at all those other places which you visited with your card and where you planted this .and_then().

You can take not a concern on this. Just a idea which I found interesting enough to share with others.

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-22T19:15:49Z

node/src/blockchain/blockchain_bridge.rs

+                .expect("Accountant is dead");
+        });
+
+        actix::spawn(future);
    }

    fn process_payments(


I think it would nice if this method could a more confusion-avoiding name. Maybe "obey_instructions_for_payments", "generate_payments", "pay_to_creditors",...

I'd really appreciate to have this.

bertllll

Continue with: blockchain_bridge_processes_requests_for_a_complete_and_null_transaction_receipt

bertllll · 2024-10-22T19:25:22Z

node/src/blockchain/blockchain_bridge.rs

    }

    #[test]
-    fn blockchain_interface_null_as_result_of_missing_blockchain_service_url() {
-        let result = BlockchainBridge::initialize_blockchain_interface(None, TEST_DEFAULT_CHAIN);
+    fn blockchain_bridge_receives_bind_message() {


I'm surprised that you wrote a brand new test regarding the BindMessage. If you are serious though, you should add some assertions, as it is, the one limited to the logging is darn weak. Regardless it is just a silly Debug logs which we traditionally don't assert on. @utkarshg6, you teach @Syther007 weird stuff. 🫤

It's a very unusual idea in the context of our codebase. It might be quite hard testing it properly.
I think we never do that mainly because there are so many tests that are actually written for an distinct act but it begins by sending the BindMessage to the subject. One could comment that this way the mechanism is actually being tested as a byproduct of those other tests.

I'd advise you guys just to delete the test.

I'd say this test is really weak. It tests so little that in this particular state it isn't worth keeping.

bertllll · 2024-10-22T20:32:55Z

node/src/blockchain/blockchain_bridge.rs

+        TestLogHandler::new().exists_log_containing(&format!(
+            "INFO: test: Blockchain service url has been set to {}",
+            blockchain_service_url
+        ));


These two tests above have the act set wrong. It is this big part:

let result = BlockchainBridge::initialize_blockchain_interface( Some(blockchain_service_url.to_string()), TEST_DEFAULT_CHAIN, Logger::new("test"), );

not this part

let chain = subject.get_chain();

bertllll · 2024-10-22T20:40:05Z

node/src/blockchain/blockchain_bridge.rs

+        let transaction_id_error = BlockchainAgentBuildError::TransactionID(
+            consuming_wallet.address(),
+            BlockchainError::QueryFailed(
+                "Transport error: Error(IncompleteMessage) for wallet 0xc4e2…3ac6".to_string(),


Please fix it so that the entire wallet address is displayed, without the obfuscation in the middle.

Yes please fix this. Let's have it implemented correctly just from the beginning.

bertllll · 2024-10-22T20:43:37Z

node/src/blockchain/blockchain_bridge.rs

        );
+        let port = find_free_port();
+        // To make submit_batch failed we didn't provide any responses for batch calls


maybe this would be more grammatically correct "To make submit_batch fail we didn't..."?

bertllll · 2024-10-22T20:46:23Z

node/src/blockchain/blockchain_bridge.rs

        );
+        let port = find_free_port();
+        // To make submit_batch failed we didn't provide any responses for batch calls


Also, are you sure there aren't any long-starving time-outs which would accumulate a big amount of time, too big for what we want from a single unit test...? They should run fast, that's my only concern.

bertllll · 2024-10-22T20:51:20Z

node/src/blockchain/blockchain_bridge.rs

-                hashes: vec![transaction_hash],
-            }));
-        let persistent_configuration_mock = PersistentConfigurationMock::new();
+    fn process_payments_works() {


If you changed the name of the the function being the act, as I suggested you, don't forget to change the name there where its included in other names, like in this test's name.

bertllll · 2024-10-22T20:54:26Z

node/src/blockchain/blockchain_bridge.rs

            })
        );
+        let recording = accountant_recording.lock().unwrap();
+        assert_eq!(recording.len(), 1);


It's not obvious which message you consider there. My guess is it is the one carrying the PendingPayableFingerprintsSeeds. And it maybe should be explicitly said.

bertllll · 2024-10-22T20:56:34Z

node/src/blockchain/blockchain_bridge.rs

+        let consuming_wallet = make_paying_wallet(b"consuming_wallet");
+        let system = System::new(test_name);
+        let agent = BlockchainAgentMock::default().consuming_wallet_result(consuming_wallet);
+        let msg = OutboundPaymentsInstructions::new(vec![], Box::new(agent), None);


You don't have to construct this message because you will never use it as a whole in this test.

bertllll · 2024-10-22T21:09:18Z

.../src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs

+    {
+        let web3_batch = self.web3_batch.clone();
+        let get_transaction_id = self.get_transaction_id(consuming_wallet.address());
+        let get_gas_price = self.get_gas_price();


Can you explain why you need to query the already known transaction id and gas_price again. It's both stored inside the blockchain agent.
If the payment adjuster confirms payments coverage by our balances, using quite precise computation, why would you change the inputs by updating the gas price. For Utkarsh: we agreed that any possible momentary fluctuation is going to be handled thanks to the added margin.

This second call of get_gas_price is unnecessary. Please remove it.

The call of get_transaction_id is probably reasonable put as close as possible to the place where the payments are taking place. I'm happy with that. What I'm not, though, it's the blockchain agent constructor because it also includes this operation. Please make sure we do that only once, I don't want to see it repeat because it's so obvious we need to do it only once.

Linked to #456 (comment).

URGENCY STATUS: THIS IS NOT GOOD, BUT A BIG MISTAKE, THAT HAPPENED IN THIS CARD SO IT SHOULD BE FIXED HERE TOO You know, let's allow adding in just useful new stuff, not together with also new issues.

Linked to #456 (comment).

We have added the explanation to this as a comment in the code.

I'm afraid, you may've misunderstood to me. I can't imagine why this could be solved by a comment. The double calls are wrong, both.

If you want to do the second call of get_gas_price() you'd have to change the whole concept.
Maybe something like

[; first gas price fetch] --->

[; fetched gas price augmented by tx_fee_margin (15% at the moment); PA checks for insolvency, the augmented gas price is stored in the OutboundPaymentInstructions msg ] --->

[; You fetch gas price the second time. You don't proceed straight, you must first check the following condition gas_price1 + margin >= gas_price2, if it's correct you can use gas_price2, if it's false, you have to throw out an error. Why? Because we are already behind the checks of PA and if you increase the gas_price it threats with transaction failures which PA is meant to prevent ]

So, you either implement this up here 👆 or you need to remove this second call completely. There's no place for it after we leave the PaymentAdjuster behind. In the simplest solution (not like the more advanced depicted above) we always fetch the gas price only before the PaymentAdjuster.

We plan on removing this second call completely once PaymentAdjuster is in.
we did it in this way because we were fetching the value from the database previously since we aren't storing the values anywhere we need to fetch it from the RPC.
Now this solutions is a mea filling the blank so that we don't break any code.
Ideally blockchain agent will provide the value.

feel free to modify this under your card that fits the new design of PaymentAdjuster

I'm a bid frustrated that you have decided so. Last time I talked to Utkarsh, it was on the meeting with Dan (Syther is excused), he seemed quite assured when I showed him that this call must (there aren't any doubts) while the BlockchainAgent is being made. It definitely needs to disappear here. Utkarsh even wrote down some TODOs in his IDE to remind him this. What I told him too though that I don't care so much about location of get_transaction_id(). If here or together with the get_gas_price() earlier, it's not a big difference.

You say "ideally blockchain agent will provide the value", but it has ever been designed like so. That's the problem, you're breaking the already established order of things. The BlockchainAgent was deployed beforehand, not requiring the PaymentAdjuster to be functioning.
Now, out of a sudden, with your card, the operations are doubled. So why should anybody else but you two fix this to be right?

I urge you to take an excursion to the master branch and check these functions as they go in sequence:
node/src/blockchain/blockchain_bridge.rs handle_outbound_payments_instructions
node/src/blockchain/blockchain_bridge.rs process_payments
node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs send_batch_of_payables
Notice how the agent is being used. Also watch where there are calls done by the BlockchainInterface. The answer is "only once ever". Compare it with your three calls. Also notice how the method send_batch_of_payables cooperates with the supplied agent. It asks, and the agent has answers. There is no interaction with the Internet. The agent knows all that is needed. This wasn't accidentally designed. It is well-thought and has good reasons.
In your code, you go about using some recklessly written code. This is as if going backward, do you know what I mean.

Remember that this is why the agent exists. It keeps knowledge of facts coming from the chain currently being used. The agent works better as immutable. Thus, the idea was to collect important information given the chain and then it was stuffed into the agent. The agent travels by messages and anywhere where somebody is interested in some blockchain specific things the agent is on hand. It's the whole point of its existence.
But you don't follow, you disregard the original concept. 🤷‍♂️ Simply get rid of the second-place RPC calls and let the agent serve its purpose...

So at least don't tell me that you want to preserve some existing code, because these are all your new additions and I'm rightful complaining if you introduce function calls which actually double some work already done.
It's silly that I even have to carry these long discussions. After the months of work you, should have understood the design you took over at the starting line.

(If there are now any tests that you - perhaps - wrote for these calls above the misconception, of course, delete them, as they, together with the doubled calls, don't belong in the codebase)

If you feel it's over your head, ask help from Dan and his supervision.

We have removed the get_transaction_id call from blockchain agent.
and we have also removed the get_gas_price from submit batch.

bertllll · 2024-10-22T21:11:58Z

node/src/blockchain/blockchain_bridge.rs

+    }
+
+    #[test]
+    fn process_payments_fails_on_missing_gas_price() {


This test should go away as I explained in blockchain_interface_web3/lower_level_interface.rs. This belongs only and only into the BlockchainAgent, a few steps before in the course of events...

bertllll · 2024-10-23T16:57:46Z

node/src/blockchain/blockchain_bridge.rs

+        let _blockchain_client_server = MBCSBuilder::new(port)
+            .begin_batch()
+            .raw_response(first_response)
+            // A transaction receipt is null when the transaction is not available


If it's not available, it would mean something else than what you think. As long as the pending transaction was added to the transaction pool, it must always be available for an inspection and therefore the response we get back should definitely not say null, but it would hold enough information to form the web3 TransactionReceipt structure. You are supposed to check the field of the structure with name status, if it's None, it means pending, if Some(1), it means a complete transaction, if Some(0), it's a failure.

bertllll · 2024-10-23T17:01:13Z

node/src/blockchain/blockchain_bridge.rs

+            accountant_recording.get_record::<ReportTransactionReceipts>(0);
+        let scan_error_message = accountant_recording.get_record::<ScanError>(1);
+        let mut expected_receipt = TransactionReceipt::default();
+        expected_receipt.transaction_hash = hash_1;


If you use the TransactionReceiptBuilder elsewhere, then you should here too. However, I still hope you will be convinced that it's wrong to use this web3 structure. We need to manipulate our own one, as I explained before.

bertllll · 2024-10-23T17:06:04Z

node/src/blockchain/blockchain_bridge.rs

+              response_skeleton_opt: None,
+              msg: "Error while retrieving transactions: OtherRPCError(\"Attempted to retrieve received payments but failed: QueryFailed(\\\"Transport error: Error(IncompleteMessage)\\\")\")".to_string()
+          }
+        );


Just a reminder. I don't like you send any retrieved transactions and then you send another message saying the scan failed. How can you call it a failure if you actually completed the cycle, even though it wasn't all the transactions?

bertllll · 2024-10-23T17:14:27Z

node/src/blockchain/blockchain_bridge.rs

        TestLogHandler::new().exists_log_containing(
-            "WARN: BlockchainBridge: Attempted to retrieve \
-         received payments but failed: QueryFailed(\"we have no luck\")",
+            "WARN: BlockchainBridge: Error while retrieving transactions: OtherRPCError(\"Attempted to retrieve received payments but failed: QueryFailed(\\\"Transport error: Error(IncompleteMessage)\\\")\")",
        );
    }

    #[test]
    fn handle_request_transaction_receipts_short_circuits_on_failure_from_remote_process_sends_back_all_good_results_and_logs_abort(


You removed the short-circuit functionality, naturally, because you changed the strategy from a sequence to a batch call.

I'm not opposing this factor, it's cool, the problem is, though, there have remained references in the test names to the previous state, for example here, the old name is definitely not up-to-date and you need to think up one anew that will suit the goal of this test.

For you to better understand the old state: "short-circuit" was supposed to describe that if you have a sequence of 8 pending transactions and the fifth one fails on waiting for the response from the blockchain service provider, then we did not try out the sixth or any following ones and just collected 1,2,3,4 and sent it back to Accountant.

JUST NICE TO HAVE

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T19:12:16Z

node/src/blockchain/blockchain_bridge.rs

@@ -1185,15 +1462,15 @@ mod tests {
    }

    #[test]
-    fn handle_request_transaction_receipts_short_circuits_on_failure_of_the_first_payment_and_it_sends_a_message_with_empty_vector_and_logs(
-    ) {
+    fn handle_request_transaction_receipts_short_circuits_if_submit_batch_fails() {


Again the wording "short_circuits" is out of place here.

bertllll · 2024-10-23T19:14:51Z

node/src/blockchain/blockchain_bridge.rs

-            .get_transaction_receipt_params(&get_transaction_receipt_params_arc)
-            .get_transaction_receipt_result(Err(BlockchainError::QueryFailed("booga".to_string())));
+        let port = find_free_port();
+        let _blockchain_client_server = MBCSBuilder::new(port).start();


You can add a comment as you did some other tests of this type, explaining that the lack of response from the server will cause the error. I quite like this approach.

Would be nice to have this

bertllll · 2024-10-23T19:46:55Z

node/src/blockchain/blockchain_bridge.rs

@@ -1287,26 +1553,18 @@ mod tests {
                },


I'm commenting on the test "handle_retrieve_transactions_uses_latest_block_number_upon_get_block_number_error".
Can you explain me of what use is this cannibalized test?

Do you know what you did?

You changed the prod code so that there is a method retrieve_transactions in the BlockchainInterface which hosts some code that also includes the incriminated call of get_block_number which is grafted into the lower level interface. You modified it slightly but the test still is using the BlockchainInterfaceMock. Now, isn't there anything suspicious to you having the two piece of information I just put forth to you?

The issue is that if you use a mock, everything out of the guts, will never be called, and you have the method, which is this test supposed to be aimed at, missing this way. So you're testing a situation which you can only get closer to in the test, but you stay away from it.

This what I see makes me think that you only fixed a failing test but didn't think beyond it, about what the test is testing. That's a bit sad finding.

What am I advising? Hmm. It depends on whether you've had already the discussion with Dan about the right and durable design of the BlockchainInterface, because there might be some rearranging.

Otherwise, if you cannot test the condition from this level, but the condition is still there, you have two options.
First, I just recalled why I designed the LowerLevelInt in first place. It was so that I could take the real implementation of the higher level (or main) interface, planning that it will engage in the test but yet one component inside would need to be altered to a mockable object. That's the reason, why the lower level interface has its own trait. So that I could mock it out if I fancied. This was to solve the big problem of testing these complex, layered structures.

Another way is to write this with the BlockchainInterfaceReal and use your MockBlockchainClientServer to invoke an error on the call for get_block_number(). You've done something like this many times already, so you might want to chose the second path by intuition.

Nevertheless, I wonder if the old good purpose of the LowLevelInt and the possibilities of use it offered will be missing. So far, you've succeeded in a substitution of the concept by your tedious way of setting up the MBCServer. Maybe that'll be universal enough also into the future. I hope so.

This is interconnected with a different comment now. So it shouldn't end up in the dust, unless you ignore also the other, parental comment.

bertllll · 2024-10-23T19:52:06Z

node/src/blockchain/blockchain_bridge.rs

+        let accountant_addr =
+            accountant.system_stop_conditions(match_every_type_id!(ReceivedPayments));
+        let earning_wallet = make_wallet("earning_wallet");
+        let amount = 996000000;
        let expected_transactions = RetrievedBlockchainTransactions {


This structure should be placed after the act, in the assert area.

bertllll · 2024-10-23T20:37:25Z

node/src/blockchain/blockchain_bridge.rs

-                    wei_amount: amount2,
-                },
-            ],
+            new_start_block: 1000000001,


If this is composed as 0x3B9ACA00 (the single response from your mock server) than maybe prepare the expected value as 0x3B9ACA00 + 1

But you could also pick out a better-looking value. I know this is derived from the decimal being nice for eyes, but let's regard it only from the hexadecimal outlook.

bertllll · 2024-10-23T20:43:09Z

node/src/blockchain/blockchain_bridge.rs

+        let system = System::new(test_name);
+        let logger = Logger::new(test_name);
+        let port = find_free_port();
+        let expected_response_logs = vec![LogObject {


Can you please mark the lines with the invalid topics by comments?

Would be nice to have.

bertllll · 2024-10-23T20:45:26Z

node/src/blockchain/blockchain_bridge.rs

+        subject.received_payments_subs_opt = Some(accountant_addr.clone().recipient());
+        subject.scan_error_subs_opt = Some(accountant_addr.recipient());
+        subject.handle_scan_future(
+            BlockchainBridge::handle_retrieve_transactions,
            ScanType::Receivables,
            retrieve_transactions,
        );


Isolate the act.

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T21:00:10Z

node/src/blockchain/blockchain_bridge.rs

-            }
-        );
-        assert_eq!(accountant_recording.len(), 1);
-        TestLogHandler::new().exists_log_containing("WARN: BlockchainBridge: My tummy hurts");


Maybe you want to add a log assertion with "exists_no_log_containing" to prove that we did not deliver an error to these places and so it didn't have to mention it. The message should actually also name the logger at the very beginning of it, which would be the name of test itself.

Would be nice to have.

bertllll · 2024-10-23T21:42:53Z

node/src/blockchain/blockchain_bridge.rs

+        });
+    }
+
+    fn assert_handle_scan_future_handles_failure(msg: RetrieveTransactions) {


Oh guys...I cannot drag it on anymore... you have the understanding of these tests about the handle_scan or handle_scan_futures somewhat screwed.

Especially, there are supposed to be two tests which you conceded into just one. You missed out the important difference. It touches the ResponseSkeleton. You need to ensure that the ResponseSkeleton, if it is Some - and therefore present, needs to be take over by the ScanErrorMsg.

Because you have only one test for the error situation it's easy to deduce that you lack the second test exercising the flight of the ScanErrorMsg when the skeleton hasn't been populated from the beginning (meaning that the scan wasn't ordered manually by the command intended for it).

It also explains for you why you ended up with this strangely ostracized assert_handle_scan_future_handles_failure. It came to existence only because it was to be shared between two tests whose inputs differ only minimally.

URGENCY STATUS: THIS IS A CASE OF CRIPPLING THE OLD TESTS, AND WE CANNOT TOLERATE LOSING THE TEST COVERAGE BECAUSE WE ARE IN HURRY. PLEASE FIX IT SO THAT BOTH SITUATIONS ARE TESTED See the explanation above.

Hi Bert,

Could you please point to the test that would test this ScanErrorMsg when the ResponseSkeleton is Some (such that two request were generated).
We have checked Master and struggled to find a test that related to these tests and also tested ScanErrorMsg

Regarding the handle_scan method appropriated to the BlockchainBridge, three tests makes up the full coverage on the master branch.

Those three are:
handle_scan_handles_success (the happy path test)
handle_scan_handles_failure_without_skeleton (a general failure while the skeleton is not there)
handle_scan_handles_failure_with_skeleton (a general failure while the skeleton was provided)

Now, how can you understand the skeleton? It's a construct which actually represents a request from a UI. It stores the two numbers that are important for a continued communication: client_id (represents the UI), and the context_id (represents the specific conversation).

You miss the second one. I recommend to try grafting onto this method assert_handle_scan_future_handles_failure and you might find yourselves with two tests sharing this function as a test body thanks to which it will save some space.

Hi Bert,

I'm a little confused by your comment.

It appears we are still testing all 3 cases.
We have created a helper function assert_handle_scan_future_handles_failure to help test both cases for the skeleton using test handle_scan_future_handles_failure.

Here the tests code.

#[test] fn handle_scan_future_handles_failure() { assert_handle_scan_future_handles_failure(RetrieveTransactions { recipient: make_wallet("somewallet"), response_skeleton_opt: Some(ResponseSkeleton { client_id: 1234, context_id: 4321, }), }); assert_handle_scan_future_handles_failure(RetrieveTransactions { recipient: make_wallet("somewallet"), response_skeleton_opt: None, }); }

bertllll · 2024-10-23T21:52:52Z

node/src/blockchain/blockchain_bridge.rs

@@ -1811,117 +2052,4 @@ pub mod exportable_test_parts {
            )
        }
    }


One opinion is, remembering instructing you to go ahead and delete the test this was done for, it can simply be erased along with the test.

Otherwise:
If this is the only impl that has remained since your first run at this card, let me tell you that it doesn't require exclusive public module making a circle around it. Just grab it and move it to the tests module that's used in all other cases.

Please do it

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T22:03:34Z

node/src/blockchain/blockchain_bridge.rs

+                1,
+            )
+            .start();
+


Blank line at a wrong place.

* GH-813: Correctly parse max block range error message (#531) * GH-500: Adding Base chains (#510) * GH-500: ready for an urged live test * GH-500: probably finished the deployment of Base, let's get the QA going * GH-500: removed an eprintln! * GH-500: version changed to 0.8.1 --------- Co-authored-by: Bert <[email protected]> * update readme and tag v0.8.1 (#532) Signed-off-by: KauriHero <[email protected]> * GH-524: Disable `entry_dns` (#526) * GH-539: Don't Panic! (#540) * GH-539: Don't Panic * GH-539: remove commented out code * New Version: v0.8.2 (#542) * GH-744: Review-1 first lot of changes * GH-744: Review-1 - fixed more tests * GH-744: improved wildcard IP check * GH-744: removed unused imports * GH-744: fixed a bunch more comments from review 1 * GH-744: resolved more review comments * GH-744: started converting gas_price units from gwei to wei * GH-744: finish converting gas price from gwei to wei * GH-744: Formating & removed warnings * GH-744: Added TransactionFailed to TransactionReceiptResult * GH-606: Initialize start_block to none to use latest block (#374) * GH-606: Initialize start_block to none to use latest block * GH-606: Apply PR feedback changes * GH-606: Apply PR feedback changes * GH-606: Apply PR feedback changes * GH-606: Apply PR review 4 feedback changes * GH-606: Squashing commits - Save start_block_nbr if no msg but send as non-Option - Always commit - Reduce logging levels and simplify - Follow the Option naming pattern * GH-600: set_start_block only called in accountant/scanners/mod.rs * GH-606: PR Feedback - parameterize a test * GH-606: Address PR feedback * GH-606: Implement parameterized test without crate macro * GH-744: Migrated the guts of get_transaction_receipt_in_batch to process_transaction_receipts * GH-744: Moved submit_payables_in_batch to blockchain_interface * GH-744: removed test: blockchain_bridge_can_return_report_transaction_receipts_with_an_empty_vector * GH-744: Fixed a few more URGENCY comments * GH-744: cleanup & formatting * GH-744: add some TODOs as discussed on Wed and Thu * GH-744: handle_request_transaction_receipts chaged error to a DEBUG log * GH-744: handle_retrieve_transactions inbeded extract_max_block_count * GH-744: code refactor * GH-744: removed transaction_id from Agent * GH-744: Removed get_gas_price from submit_batch call * GH-744: logger is now a reference in send_payables_within_batch * GH-744: send_payables_within_batch web3_batch is now a reference * GH-744: sign_and_append_multiple_payments accounts is now a reference * GH-744 removed Blockchan_interface_mock * GH-744: Refactored all 4 test for send_payables_within_batch * GH-744: cleanup & formatting * GH-744: small fixs * GH-744: handle_normal_client_data detects wildcard IP & localhost with error * GH-744: Finished all urgent comments * GH-744: changed actions macos-12 to 13 * GH-744: increased sleep time for test provided_and_consumed_services_are_recorded_in_databases * GH-744: Fixed multinode tests * GH-744: Added start block +1 to handle_transaction_logs * GH-744: Fixed some tests * GH-744: Fixes from review 2 * GH-744: Fixed test debtors_are_credited_once_but_not_twice * GH-744: removed BlockNumber::Number * GH-744: Resolved TODOs * GH-744: First commit for review-3, fixed tests * GH-744: Refactored TxReceipt * GH-744: Refactored TxResponse * GH-744 moved & renamed blockchain_interface_utils.rs * GH-744: fixed test: dns_resolution_failure_for_wildcard_ip_with_real_nodes * GH-744: add review 4 changes * GH-744: add review 5 changes * GH-744: remove the map_err() * GH-744: migrate the logging code to a different function * GH-744: add review 6 changes --------- Signed-off-by: KauriHero <[email protected]> Co-authored-by: MASQrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: KauriHero <[email protected]> Co-authored-by: Syther007 <[email protected]>

Syther007 added 30 commits September 27, 2023 23:54

Added base comments

e908666

GH-744: removed .wait() from handle_request_balances_to_pay_payables

8b71081

GH-744: added future return type to get_transaction_fee_balance

99e36a5

Fixed error mapping for .get_token_balance

4fba6f1

GH-744: Fixed failing tests, all tests passing

7d0d075

GH-744: Added future return to ReportAccountsPayable

159ad22

GH-744: Fixed more errors

5b25bce

GH-744: Added batch lifetime

ffe6b81

Revert "GH-744: Added batch lifetime"

cb2f3e4

This reverts commit ffe6b81.

GH-744: Fixed more errors

e5482f7

GH-744: small clean up

8093caf

GH-744: Moved batch_payable_tools functions

85529d0

GH-744: send_payables_within_batch accounts changed to Vec

aa811db

GH-744: added blockchain_interface_slim

d1659ec

Added blockchain_interface_utils.js

0877056

GH-744: process_payments is working with futures

a075b5d

GH-744: Fixed a few more tests in blockchain_interface_utils

70ee731

GH-744: Fixed test useing test_gas_limit_is_between_limits

2f03dbe

Changed sign_transaction_data amount data type to u128

ba6a7f9

GH-744 converted handle_retrieve_transactions return type to future

c7725b1

GH-744: Fixed some error with handle_retrieve_transactions

2a16982

GH-744: update_start_block_message_works test works

93189ec

GH-744: Added get_batch_web3 Mock

231cce3

GH-744: Fixed test report_accounts_payable_returns_error_fetching_pen…

25593c5

…ding_nonce

GH-744: fixed test process_payments_returns_error_from_sending_batch

c313533

GH-744: fixed test handle_report_accounts_payable_transmits_eleventh_…

e771cd5

…hour_error_back_to_accountant

GH-744: Edited test handle_report_accounts_payable_transacts_and_send…

d3a3694

…s_finished_payments_back_to_accountant

GH-744: updated test handle_report_accounts_payable_transacts_and_sen…

6c6a146

…ds_finished_payments_back_to_accountant

GH-744: FIxed test handle_retrieve_transactions_sends_received_paymen…

e645549

…ts_back_to_accountant

GH-744: Update to test processing_of_received_payments_continues_even…

5e1dac1

…_if_no_payments_are_detected

Syther007 added 5 commits September 2, 2024 21:15

GH-744: fixed handle_scan_future tests

0ea951c

GH-744: Added a fallback blockchain_service_url

aee1544

GH-744: changes for self review

b9e7572

GH-744: Removed gas_price_result from tests

b6a0b65

GH-744: Formatting

d098022

bertllll reviewed Sep 26, 2024

View reviewed changes

node/src/blockchain/blockchain_interface/lower_level_interface.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 1, 2024

View reviewed changes

bertllll reviewed Oct 7, 2024

View reviewed changes

node/src/blockchain/blockchain_interface_initializer.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 8, 2024

View reviewed changes

node/src/test_utils/persistent_configuration_mock.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 8, 2024

View reviewed changes

node/src/neighborhood/mod.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 8, 2024

View reviewed changes

bertllll reviewed Oct 9, 2024

View reviewed changes

node/src/blockchain/blockchain_interface/mod.rs Show resolved Hide resolved

node/src/blockchain/blockchain_interface/test_utils.rs Outdated Show resolved Hide resolved

node/src/blockchain/blockchain_interface/test_utils.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 12, 2024

View reviewed changes

bertllll reviewed Oct 13, 2024

View reviewed changes

bertllll reviewed Oct 14, 2024

View reviewed changes

bertllll reviewed Oct 17, 2024

View reviewed changes

bertllll reviewed Oct 18, 2024

View reviewed changes

bertllll reviewed Oct 20, 2024

View reviewed changes

node/src/neighborhood/mod.rs Outdated Show resolved Hide resolved

bertllll reviewed Oct 21, 2024

View reviewed changes

bertllll reviewed Oct 22, 2024

View reviewed changes

bertllll reviewed Oct 23, 2024

View reviewed changes

kauri-hero mentioned this pull request Nov 17, 2024

Streamline Messages from BlockchainBridge to the Accountant #557

Open


		let result = subject.lower_interface().get_gas_price().wait().unwrap();

		assert_eq!(result, 1.into());

@@ @@ -1811,117 +2052,4 @@ pub mod exportable_test_parts { @@
                           )
                       }
                   }

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Are you sure you want to change the base?

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Conversation

Syther007 commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll left a comment

Choose a reason for hiding this comment

bertllll Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 29, 2024 •

edited

Loading

bertllll Oct 29, 2024 •

edited

Loading

bertllll Oct 22, 2024 •

edited

Loading