GH-606-Review-5 #451
Open
GH-606-Review-5 #451
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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
approved these changes
Jul 9, 2024
| @@ -942,6 +944,8 @@ impl ReceivableScanner { | |||
| } | |||
| 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, ¤t_start_block); | |||
There was a problem hiding this comment.
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. :-)
| @@ -571,11 +571,9 @@ impl PersistentConfigurationReal { | |||
| value: Option<T>, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok changed all value: Option<... to value_opt: Option<... in the file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.