GH-606-Review-5 by masqrauder · Pull Request #451 · MASQ-Project/Node

masqrauder · 2024-06-23T20:32:05Z

GH-606: Apply PR review 4 feedback changes

I think I responded to all the view comments. If I missed any please let me know.

* fixed tests failing on Bert's computer * formatting * remove commented out code * change .clone() to reference * fixed test for windows in actions: tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * target_os for dirs crate used only on windows test * implementing Bert's review comments - change DirsWrapperMock to DirsWrapperReal and simplifying home_dir creation * fixing windows data-directory assertion in test tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * fixing home dir assertion for windows * add condition to use of dirs crate only for windows * fixing config toml for windows test tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * fixed windows test * formatting * fixed tilde tests for windows * remove unnecesary function for finding project root, use of current dir for DirsWrapperMock * adjusting name of test and removing commented out code

* GH-791: fixed * GH-791: put some explanatory lines back --------- Co-authored-by: Bert <Bert@Bert.com>

* GH-797: minor docs modification * GH-797: some TODOs * GH-797: add the definitions for min hops and gas price. * GH-797: remove the markdown code container

* GH-728: add the TODOs * GH-728: removed the code of the NewPasswordMessage * GH-728: use ConfigurationChangeMessage while changing password * GH-728: rename configuration_change_msg_sub_opt to update_min_hops_sub_opt * GH-728: rename new_password_subs to update_password_subs * GH-728: fix wrong name for the actor message * GH-728: send ConfigurationChangeMessage to other actors too * GH-728: add a TODO * GH-728: rename to _opt * GH-728: implement handler for ConfigurationChange::UpdatePassword inside Neighborhood * GH-728: add TODO to implement handler for ConfigurationChange::UpdatePassword inside BlockchainBridge * GH-728: add TODO to implement handler for ConfigurationChange::UpdatePassword inside Accountant * GH-728: create new struct for UpdatePasswordSubs * GH-728: add the new TODO * GH-728: send a ConfigurtionChangeMessage when the consuming wallet is generated * GH-728: add a test for the panic case * GH-728: some refactor changes in configurator.rs * GH-728: test drive the case when the wallet is recovered * GH-728: fix some of the tests * GH-728: fix the remaining 2 failing tests * GH-728: reorder fields * GH-728: the received ConfigurationChangeMessage is properly handled * GH-728: update the password when necessary * GH-728: add a todo * GH-728: add the ability to synchronise password * GH-728: add ability for accountant to handle an unexpected configuration change msg * GH-728: improve tests for handling ConfigurationChangeMessage in accountant * GH-728: minor renames * GH-728: remove finished TODO and formatting changes * GH-728: add test in blockchain bridge for ConfigurationChangeMessage * GH-728: add test in neighborhood for ConfigurationChangeMessage * GH-728: remove useless tests * GH-728: revive an old test * GH-728: improve todo * GH-728: test rename and add more TODOs * GH-728: cleanup and creation of new file configuration_change_subs.rs * GH-728: minor refactor and remove tests * GH-728: introduce trait for ConfigurationChangesubs * GH-728: refactor out the subs for UpdateWallets and UpdatePassword * GH-728: some renames * GH-728: add TODO and minor reordering * GH-728: make the test the_password_is_synchronised_among_other_actors_when_modified stronger * GH-728: add formatting changes * GH-728: make enum usage more explicit * GH-728: remove clippy warnings * GH-728: do some renames * GH-728: fix wrong rename and import * GH-728: use peer actors for generating config change subs * GH-728: recreate ConfigChangeSubs * GH-728: remove config_change_subs.rs * GH-728: test for ConfigChangeMsg in Accountant contain assertions * GH-728: test for ConfigChangeMsg in Neighborhood contain assertions * GH-728: test for ConfigChangeMsg in BlockchainBridge contain assertions * GH-728: small rename * GH-728: add context_id to fn * GH-728: refactor the fn to transform associated functions to methods * GH-728: minor fixes * GH-728: add wallet_opt as a new argument in the begin_scan fn * GH-728: remove the use of field consuming_wallet_opt from BlockchainBridge * GH-728: remove the field consuming_wallet_opt from BlockchainBridge * GH-728: don't send the ConfigChangeMsg to the BlockchainBridge * GH-728: use the earning wallet in Receivable Scanner's begin_scan * GH-728: don't save a reference of earning wallet inside Scanners * GH-728: store the earning wallet directly without an Rc * GH-728: fix the failing test * GH-728: formatting changes * GH-728: remove the earning wallet from ReceivableScannerBuilder * GH-728: check wallet in the begin_scan() for ScannerMock * GH-728: remove unnecessary TODO * GH-728: formatting changes * GH-728: fix the failing test * GH-728: remove unused import in blockchain_interaction_test.rs * GH-728: BlockchainBridge receives consuming_wallet instead of an Option<T> * GH-728: Check consuming wallet before sending a msg to BlockchainBridge * GH-728: remove Option<T> from the wallet_opt param in begin_scan fn * GH-728: remove commented out code * GH-728: modify AccountantBuilder to make it's field names contain opt * GH-728: improve test mocks * GH-728: introduce another URL for mumbai testnet * GH-728: change the message in trace log * GH-728: test drive the NoConsumingWallet Error * GH-728: reordering * GH-728: add better logging * GH-728: Review 2 changes * GH-728: Review 2 leftover changes * GH-728: add better assertions for logger

bertllll · 2024-07-09T22:27:12Z

node/src/accountant/scanners/mod.rs

                    Err(e) => panic!("Commit of received transactions failed: {:?}", e),
                }
+            } else {
+                warning!(logger, "The new_start_block ({}) is less than the current_start_block ({}). This is not a problem but by checking we avoid rescanning the same blocks again later.", &new_start_block, &current_start_block);


If I remember correctly, there are plans that the start_block parameter will be eventually hidden from the user. So I like what you did here and if not best then it will go away and solve this tricky caveat on its own. :-)

bertllll · 2024-07-09T22:29:59Z

node/src/db_config/persistent_configuration.rs

@@ -571,11 +571,9 @@ impl PersistentConfigurationReal {
        value: Option<T>,


I know this isn't your fault. Therefore don't feel obligated to do the fixing but it would be nice to have this argument named as value_opt.

Ok changed all value: Option<... to value_opt: Option<... in the file.

Thanks 🙏

masqrauder · 2024-07-21T20:05:55Z

multinode_integration_tests/tests/verify_bill_payment.rs

        token_balance,
-        expected_token_balance
+        expected_token_balance,
+        token_balance - token_balance


@bertllll did you mean this instead?

Suggested change

token_balance - token_balance

token_balance - expected_token_balance

* 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 <Bert@Bert.com>

Signed-off-by: KauriHero <kaurihero@masq.ai>

* GH-539: Don't Panic * GH-539: remove commented out code

- Save start_block_nbr if no msg but send as non-Option - Always commit - Reduce logging levels and simplify - Follow the Option naming pattern

masqrauder · 2024-11-06T00:43:49Z

All changes in this PR already applied from the original -> #374

czarte and others added 4 commits April 24, 2024 12:38

GH-791: Small fixes to stabilize tests in setup_reporter.rs (#443)

55851fc

* GH-791: fixed * GH-791: put some explanatory lines back --------- Co-authored-by: Bert <Bert@Bert.com>

GH-797: Update min-hops command docs in User Interface (#447)

40480d1

* GH-797: minor docs modification * GH-797: some TODOs * GH-797: add the definitions for min hops and gas price. * GH-797: remove the markdown code container

masqrauder requested review from bertllll, czarte, dnwiebe and utkarshg6 June 23, 2024 20:32

masqrauder self-assigned this Jun 23, 2024

masqrauder changed the title ~~GH-606: Review-5~~ GH-606-Review-5 Jun 23, 2024

feat: added dependabot.yml (#342)

25e9a4f

bertllll approved these changes Jul 9, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch from 049fc0e to bda6f39 Compare July 11, 2024 01:35

masqrauder commented Jul 21, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch 3 times, most recently from 3784786 to d93e62c Compare September 24, 2024 00:42

GH-813: Correctly parse max block range error message (#531)

6fcc1cd

masqrauder force-pushed the GH-606 branch from 1d28a0a to f6a59f0 Compare October 2, 2024 00:47

bertllll and others added 2 commits October 2, 2024 16:05

GH-500: Adding Base chains (#510)

a4fb720

* 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 <Bert@Bert.com>

update readme and tag v0.8.1 (#532)

91f3b8c

Signed-off-by: KauriHero <kaurihero@masq.ai>

masqrauder force-pushed the GH-606 branch from f6a59f0 to 0a957aa Compare October 3, 2024 05:28

GH-524: Disable entry_dns (#526)

f5572f3

masqrauder force-pushed the GH-606 branch from 0a957aa to 59ee2b8 Compare October 10, 2024 03:51

utkarshg6 and others added 6 commits October 15, 2024 11:35

GH-539: Don't Panic! (#540)

7693d00

* GH-539: Don't Panic * GH-539: remove commented out code

New Version: v0.8.2 (#542)

8ad336d

GH-606: Initialize start_block to none to use latest block

ed7ca69

GH-606: Apply PR feedback changes

c61be52

GH-606: Apply PR feedback changes

aa54ff0

GH-606: Apply PR feedback changes

2a0e285

masqrauder added 5 commits October 20, 2024 16:07

GH-606: Apply PR review 4 feedback changes

c965035

GH-606: Squashing commits

f7938de

- 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

89e7171

GH-606: PR Feedback - parameterize a test

382f0de

GH-606: Address PR feedback

7c9b69d

masqrauder force-pushed the GH-606 branch from 59ee2b8 to 7c9b69d Compare October 22, 2024 02:21

GH-606: Implement parameterized test without crate macro

a9c7faf

masqrauder closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-606-Review-5#451

GH-606-Review-5#451
masqrauder wants to merge 21 commits intoGH-606-Review-5from
GH-606

masqrauder commented Jun 23, 2024

Uh oh!

bertllll Jul 9, 2024

Uh oh!

bertllll Jul 9, 2024

Uh oh!

masqrauder Jul 11, 2024

Uh oh!

bertllll Jul 12, 2024

Uh oh!

masqrauder Jul 21, 2024

Uh oh!

masqrauder commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -571,11 +571,9 @@ impl PersistentConfigurationReal {
		value: Option<T>,

	token_balance - token_balance
	token_balance - expected_token_balance

Conversation

masqrauder commented Jun 23, 2024

Uh oh!

bertllll Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

bertllll Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

masqrauder Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

bertllll Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

masqrauder Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

masqrauder commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants