GH-606: Review 2 #429
Open
GH-606: Review 2 #429
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
bertllll
reviewed
Apr 2, 2024
| @@ -1435,6 +1435,7 @@ mod tests { | |||
| .retrieve_transactions_params(&retrieve_transactions_params_arc) | |||
| .retrieve_transactions_result(Ok(expected_transactions.clone())) | |||
| .lower_interface_results(Box::new(lower_interface)); | |||
| let set_start_block_params_arc = Arc::new(Mutex::new(vec![])); | |||
There was a problem hiding this comment.
This probably isn't an issue now but it strikes how did you manage to add this line while no other line in the test has changed. It seems like it must have been completely missing before then. Weird stuff, given I can see these added lines at many places in your PR.
EDIT: Regardless it made the review hard to verify, I think, together with the previous commit (the first one from the only two of them) it can be read us complete and proper code.
| @@ -0,0 +1,18 @@ | |||
| create table config (name text not null, value text, encrypted integer not null) | |||
There was a problem hiding this comment.
Seems like this database state does not need to be preserved. I couldn't even find where you were going to use it.
EDIT: Yeah, I'm confident now that this entire file is unnecessary code with no use.
* GH-781: experimenting with a test utility * GH-781: task might be complete now * GH-781: debugging for windows * GH-781: trying to satisfy Win * GH-781: clippy * GH-781: removed bad solution for issue that now a new card was written for * GH-781: cargo.toml change * GH-781: review answered --------- Co-authored-by: Bert <[email protected]>
bertllll
reviewed
Apr 3, 2024
| @@ -40,6 +40,27 @@ pub fn bring_db_0_back_to_life_and_return_connection(db_path: &Path) -> Connecti | |||
| conn | |||
| } | |||
|
|
|||
| pub fn bring_db_9_back_to_life_and_return_connection(db_path: &Path) -> Connection { | |||
There was a problem hiding this comment.
I double-checked, this function has no use anymore. Should be deleted.
Open
* 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 <[email protected]>
* 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
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.
database_version_9_sql.txtto create the database at version 9 with a nullstart_blockdefined.set_configuration_commandto be resolved with the card where we remove the ability to set the start block number from the masq set configuration commands.