GH-611: Review 2 #217
Merged
GH-611: Review 2 #217
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
…n_is_already_running
…or_receivables_and_delinquencies_works
…ns_in_suppress_mode
bertllll
reviewed
Jan 10, 2023
| None => { | ||
| error!( | ||
| logger, | ||
| "Called scan_finished() for {:?} scanner but timestamp was not found", |
There was a problem hiding this comment.
Also details but I have a feeling that the function cited in the log message is not around anymore. Should you change it to the up-to-date name? Is that supposed to be .mark_as_ended()?
There was a problem hiding this comment.
The scan_finished() is a parent function of mark_as_ended(). While reading the logs the focal point will be the scan_finished(), that's why I kept it this way.
| Rc::new(PaymentThresholds::default()), | ||
| ) | ||
| } | ||
| pub struct PayableScannerBuilder { |
There was a problem hiding this comment.
Hmm. I like these builders. What I don't like is the name. If you see it used somewhere you will not be told that it creates: a PayableScanner stuffed with mock objects. It might be confused with a builder for a production-code-like structure. I'd rather rename slightly each of these three builders to say something as these (probably not best) examples:
PayableScannerTestBuilder,
PayableScannerWithMocksBuilder,
PayableScannerMockedBuilder,
BuilderOfPayableScannerWithMocks,
I'd vote for the second option, but admitting that these all sound more or less clumsy. Will you think up a better name?
There was a problem hiding this comment.
Since we are calling the builder as AccountantBuilder and using it only for tests. I think it's appropriate to use this name.
I personally prefer short names, whenever possible :)
| ) | ||
| } | ||
| } | ||
|
|
||
| pub struct PendingPayableScannerBuilder { |
There was a problem hiding this comment.
I wish you did the same here.
| ) | ||
| } | ||
| } | ||
|
|
||
| pub struct ReceivableScannerBuilder { |
| self | ||
| } | ||
|
|
||
| pub fn payment_thresholds(mut self, payment_thresholds: PaymentThresholds) -> Self { | ||
| self.common.payment_thresholds = Rc::new(payment_thresholds); | ||
| self.payment_thresholds = payment_thresholds; |
There was a problem hiding this comment.
Just curious. Why do you need this method at this scanner while you didn't have a setter for payment thresholds in the previous (mocked) scanner, even though it also contains it. You used the default only. Which is strange...as if somewhere in some test an assertion on exact values for the thresholds might be missing.
There was a problem hiding this comment.
It's something I just did because we were using the custom payment thresholds for a particular test named receivable_scanner_scans_for_delinquencies. There's nothing special about it.
* GH-611: remove the clone from the earning wallet iniside the Accountant's constructor * GH-611: rename the function to simply remove_timestamp() * GH-611: remove clone from the payment_thresholds * GH-611: change the test_name variable in tests for remove_timestamp
bertllll
pushed a commit
that referenced
this pull request
Jan 11, 2023
* GH-611: refactor the Accountant's constructor * GH-611: remove contract test for eth ropsten * GH-611: rename function names that handles scan requests * GH-611: rename message to scan_message * GH-611: remove the eprintln!() from the production code * GH-611: use response_skeleton to geneate logs in different severity * GH-611: remove code duplication in the handling of scan requests * GH-611: simplify the financial_statistics * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running() * GH-611: Add the ability to log complete tx hash * GH-611: refactor test accountant_receives_new_payments_to_the_receivables_dao() * GH-611: refactor accountant_scans_after_startup() * GH-611: minor test improments related to duration and begin_scan_params * GH-611: strengthen the test start_message_triggers_no_scans_in_suppress_mode() * GH-611: minor improvements in tests and renames * GH-611: remove duration from the test accountant_does_not_initiate_another_scan_in_case_it_receives_the_message_and_the_scanner_is_running() * GH-611: log full hash * GH-611: remove the contract test for ropsten * GH-611: rename make_scan_intervals_with_defaults() to default_scan_intervals() * GH-611: use a default implementation for the default scan intervals * GH-611: stop using a mutable reference of BootstrapperConfig to build Accountant * GH-611: rename test in blockchain_bridge.rs * GH-611: finish review changes for scanners.rs * GH-611: change the log when receivable scanner no new payments from the blockchain bridge * GH-611: change the error message when the scan_finished() is called but the timestamp is not found. * GH-611: working on refactoring the mark_as_ended() * GH-611: write a common function for updating the timestamp when a scan is ended. * GH-611: rename function names and add function names in the panic messages for the NullScanner * GH-611: strengthen the test scanners_struct_can_be_constructed_with_the_respective_scanners() * GH-611: improve test payable_scanner_can_initiate_a_scan() * GH-611: minor changes in the scanners.rs * GH-611: change the way we log NothingToProcess error * GH-611: strengthen the test receivable_scanner_scans_for_delinquencies() * GH-611: improve test for handle_none_status() * GH-611: remove the pub keyword from multiple functions inside the impl block of BeginScanError * GH-611: review changes for scanners_tools.rs * GH-611: use builder approach to build the scanners for tests * GH-611: remove the wrapper of migrator_config (risky) * GH-611: remove the wrapper from the when_pending_too_long (risky) * GH-611: put the mistakenly removed contract test back * GH-611: remove some unnecessary comments * GH-611: make the suppress_initial_scans flag just a boolean rather than wrapping * GH-611: minor remaining code changes * GH-611: Review 2 (#217) * GH-611: rename the function names again * GH-611: rename the field to when_pending_too_long_sec * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running * GH-611: use use_logs_containing inside the test periodical_scanning_for_receivables_and_delinquencies_works * GH-611: change the scan intervals back to their unique values * GH-611: improve timestamp_as_string function in scanners.rs * GH-611: rename the function to remove_timestamp_and_log * GH-611: migrate remove_timestamp_and_log to ScannerCommon * GH-611: change to exists_log_containing * GH-611: refactor handle_error() and remove log() inside BeginScanError * GH-611: use macro to remove code duplication in scanners.rs * GH-611: remove unnecessary modifications * GH-611: use the best practices of builder pattern for the individual scanner mocks * GH-611: refactor the constructor of Accountant * GH-611: rename the tests in scanners_tools.rs * GH-611: remove take() from the constructor of Accountant * GH-611: use just borrow for financial statistics * GH-611: remove the test that was testing panic * GH-611: remove assertions from the test start_message_triggers_no_scans_in_suppress_mode * GH 611: Review 3 (#220) * GH-611: remove the clone from the earning wallet iniside the Accountant's constructor * GH-611: rename the function to simply remove_timestamp() * GH-611: remove clone from the payment_thresholds * GH-611: change the test_name variable in tests for remove_timestamp
utkarshg6
added a commit
that referenced
this pull request
Jan 24, 2023
* GH-611: add test to scan for payables in case flag is false * GH-611: add one more test and todos for Scanner * GH-611: change Scanner from a trait to struct * GH-611: use RefCell to eliminate the problem with mutable closure inside tools.rs * GH-611: remove redundant fields from tools.rs * GH-611: migrate flag from Accountant to Scanner * GH-611: fix test accountant_have_proper_defaulted_values * GH-611: rename items in tools.rs * GH-611: add functions to update flag for is_scan_running * GH-611: add a TODO for verifying that scanners are defaulted properly * GH-611: throw error when scan is already running. * GH-611: add a todo to handle the case when UI triggers a scan, but the scan is already running * GH-611: allow scan is running error logs to print the scan type * GH-611: write logs whenever a new scan is requested but scan is already running * GH-611: rename the module to scanners * GH-611: use timestamp instead of boolean flag for marking whether a scan is running * GH-611: change the logs to include timestamp in case scan is already running * GH-611: add todos for ending scans for PendingPayables * GH-611: use a RefCell to update the initiated_at variable * GH-611: add a test for testing whether scan has started * GH-611: use mark_as_started() in the scan() * GH-611: end the scan for payables at handle_sent_payable() * GH-611: allow blockchain bridge to send ReportTransactionReceipts with an empty vector * GH-611: disable starting and ending the scans to fix the tests in accountant/mod.rs * GH-611: remove compiler warnings * GH-611: scanners struct can be constructed with the respective scanners * GH-611: eliminate the BeginMessageWrapper * GH-611: modify PayableDAOMock * GH-611: modify PendingPayableDaoFactoryMock and ReceivableDaoFactoryMock * GH-611: supply DAOs for Scanner inside the tests of accountant/mod.rs * GH-611: introduce scanner mock * GH-611: modify ScannerMock and replace NullScanner with ScannerMock * GH-611: an attempt to migrate the contents of scan_for_payables() inside PayableScanner * GH-611: remove ctx and comment out code * GH-611: remove the commented out code * GH-611: add a todo!() inside payable_exceeded_threshold() to generate a successful build * GH-611: write test for payable_thresholds_real * GH-611: get rid of copy from structs in accountant.rs * GH-611: pull out payment_thresholds out of accountant_config and distribute a reference(counted) to individual scanners * GH-611: refactor tools for Payable Scanner * GH-611: migrate the payable scanner tools to the tools.rs * GH-611: Add a todo and comment out the test * GH-611: refactor tools.rs * GH-611: fix qualified_payables_and_summary() * GH-611: test drive the checks for whether a payable is qualified * GH-611: migrate test for testing debt extremes inside tools.rs * GH-611: refactor the investigate_debt_extremes() * GH-611: migrate tools for payable_scanner inside a different module * GH-611: add test for payable_scanner for initiating a scan * GH-611: add tests for pending payable initating a scan * GH-611: extend tests to assert for log messages * GH-611: migrate the scan_for_receivables to the begin_scan() * GH-611: add test for scanning for delinquency * GH-611: remove referenced counter from the payable_scanner_tools * GH-611: remove some import warnings * GH-611: generate timestamp inside begin_scan() * GH-611: modify BannedDaoFactoryMock * GH-611: fix test accountant_sends_report_accounts_payable_to_blockchain_bridge_when_qualified_payable_found * GH-611: fix the test accountant_sends_a_request_to_blockchain_bridge_to_scan_for_received_payments * GH-611: fix test scan_for_pending_payable_found_unresolved_pending_payable_and_urges_their_processing * GH-611: remove the code from accountant/mod.rs that has been moved to scanners.rs * GH-611: add ScannerError * GH-611: fix accountant_scans_after_startup * GH-611: refactor handlers for scan requests in accountant/mod.rs * GH-611: fix more tests inside accountant/mod.rs * GH-611: reorder dao in tests for Accountant and Scanner, and replace ScannerMock with NullScanner * GH-611: use the timestamp from the function parameter for scanners * GH-611: refactor the handlers for scan requests * GH-611: fix tests related to externally triggered scan * GH-611: fix test accountant_payable_scan_timer_triggers_periodical_scanning_for_payables * GH-611: fix test periodical_scanning_for_pending_payable_works * GH-611: fix periodical_scanning_for_receivables_and_delinquencies_works * GH-611: begin_scan() updates the timestamp * GH-611: remove unnecessary test * GH-611: throw error in case scan is already running * GH-611: handle error message ScanAlreadyRunning * GH-611: fix tests for when the scan is already running * GH-611: remove commented code and change the test name to periodical_scanning_for_payable_works * GH-611: remove warnings * GH-611: add timestamp as a paramneter in the function investigate_debt_extremes() * Test is passing * GH-611: remove the warnings * GH-611: refactor test scan_for_payable_message_triggers_payment_for_balances_over_the_curve and update recorder.rs * GH-611: decouple pending payable and payable daos inside test pending_transaction_is_registered_and_monitored_until_it_gets_confirmed_or_canceled * GH-611: provide correct DAOs to the Accountant after migrating handle_sent_payable() to end_scan() inside the PayableScanner * GH-611: fix tests in accountant to call scan_finished() directly * GH-611: migrate all code of handle_sent_payable() to scan_finished() for the PayableScanner * GH-611: throw errors when a problem happens while handling SentPayable message * GH-611: migrate seperate_early_errors to tools.rs finished * GH-611: remove commented out from accountant/mod.rs * GH-611: return an option of NodeToUiMessage from the scan_finished() * GH-611: format the error messages inside scanners.rs * GH-611: refactor scan_finished() of PayableScanner * GH-611: migration to scan_finished() for PendingPayableScanner in progress * GH-611: directly store fields of accountant config inside Accountant * GH-611: pull fields of accountant config outside * GH-611: refactor utility fn to build bootstrapper config with defaults * GH-611: comment out AccountantConfig * GH-611: migrate process_transaction_by_status() to the scanners.rs * GH-611: return errors instead of panicking inside PendingPayable Scanner * GH-611: pass payable dao inside pending payable scanner * GH-611: share FinancialStatistics with the PendingPayableScanner * GH-611: fix AccountantBuilder's default configuration * GH-611: supply PayableDAO for PendingPayableScanner inside tests * GH-611: fix the handler for the PendingPayable Scanner * GH-611: rename individual scanner in Scanners struct * GH-611: migrate test interpret_transaction_receipt_when_transaction_status_is_none_and_outside_waiting_interval() to scanners.rs * GH-611: migrate test interpret_transaction_receipt_when_transaction_status_is_none_and_within_waiting_interval * GH-611: migrate interpret_transaction_receipt_panics_at_undefined_status_code() to scanners.rs * GH-611: rename the panic message * GH-611: migrate test interpret_transaction_receipt_when_transaction_status_is_a_failure() to scanners.rs * GH-611: migrate handle_pending_tx_handles_none_returned_for_transaction_receipt() to scanners.rs * GH-611: migrate test for report transaction receipts message into scanners.rs * GH-611: remove unnecessary code from accountant.rs * GH-611: migrate some functions for PendingPayable Scanner to tools.rs * GH-611: reorder items inside accountant.rs * GH-611: migrate tests for CancelPendingTransactions inside scanners.rs * GH-611: remove the CancelFailedPendingTransaction message * GH-611: migrate tests for update_payable_fingerprint() * GH-611: migrate tests for confirming pending transactions to scanners.rs * GH-611: remove transaction confirmation tools * GH-611: migrate handling of ReceivedPayments message to the scan_finished() of scanners.rs * GH-611: use mark_as_started() to update the timestamp inside Scanners * GH-611: reanme ScannerError to BeginScanError * GH-611: replace Errors into panics inside scan_finished() of all the Scanners * GH-611: modify tests for Scanners to assert whether scan_finished() stops the scan * GH-611: add logging when scan has ended * GH-611: fix test for the periodical scanning of Payable Scanner * GH-611: use ScannerMock for testing periodical scanning for payables * GH-611: fix test for the periodical scanning of receivables and deliquencies * GH-611: fix test for periodical scanning for pending payables * GH-611: rename the fn name for stopping the system inside ScannerMock * GH-611: remove import warnings * GH-611: improve the implementation of ScannerMock * GH-611: end the scan in case begin_scan() returns an error of nothing to process * GH-611: refactor begin_scan() for Receivable Scanner * GH-611: remove an obsolete assert realted to threshold tools * GH-611: fix the test pending_transaction_is_registered_and_monitored_until_it_gets_confirmed_or_canceled() * GH-611: refactor scanners.rs * GH-611: use default implementation of PaymentThresholds * GH-611: migrate the test utility functions of scanner.rs to accountant/test_utils.rs * GH-611: remove BannedDao and PaymentThresholds as a field of Accountant * GH-611: implement scan_finished() for ScannerMock * GH-611: remove multiple occurences of BannedDao inside tests of accountant/mod.rs * GH-611: rename scan_finished() to finish_scan() * GH-611: remove the file .idea/inspectionProfiles/Project_Default.xml from git tracking * remove rustup check + added rust version override * Update ci-matrix.yml * GH-611: use OffsetDateTime in the logger.rs * GH-611: reorder items in the src/accountant/mod.rs * GH-611: rename the file tools.rs to scanners_tools.rs * GH-611: remove redundant code * GH-611: reorder ReportTransactionReceipts * GH-611: add Eq to the PendingTransactionStatus * GH-611: remove warnings * GH-611: Trigger GitHub Actions * Rust version hotfix (#179) (#182) * remove rustup check + added rust version override * Update ci-matrix.yml * GH-611: fix handling the message with empty vector for PendingPayables * GH-611: clone migrator_config instead of using take() on Option<T> * GH-611: fix test masq_erc20_contract_exists_on_ethereum_ropsten_integration * GH-611: consistently pass DAOs inside Accountant and Scanners * GH-611: Review 1 (#209) * GH-611: refactor the Accountant's constructor * GH-611: remove contract test for eth ropsten * GH-611: rename function names that handles scan requests * GH-611: rename message to scan_message * GH-611: remove the eprintln!() from the production code * GH-611: use response_skeleton to geneate logs in different severity * GH-611: remove code duplication in the handling of scan requests * GH-611: simplify the financial_statistics * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running() * GH-611: Add the ability to log complete tx hash * GH-611: refactor test accountant_receives_new_payments_to_the_receivables_dao() * GH-611: refactor accountant_scans_after_startup() * GH-611: minor test improments related to duration and begin_scan_params * GH-611: strengthen the test start_message_triggers_no_scans_in_suppress_mode() * GH-611: minor improvements in tests and renames * GH-611: remove duration from the test accountant_does_not_initiate_another_scan_in_case_it_receives_the_message_and_the_scanner_is_running() * GH-611: log full hash * GH-611: remove the contract test for ropsten * GH-611: rename make_scan_intervals_with_defaults() to default_scan_intervals() * GH-611: use a default implementation for the default scan intervals * GH-611: stop using a mutable reference of BootstrapperConfig to build Accountant * GH-611: rename test in blockchain_bridge.rs * GH-611: finish review changes for scanners.rs * GH-611: change the log when receivable scanner no new payments from the blockchain bridge * GH-611: change the error message when the scan_finished() is called but the timestamp is not found. * GH-611: working on refactoring the mark_as_ended() * GH-611: write a common function for updating the timestamp when a scan is ended. * GH-611: rename function names and add function names in the panic messages for the NullScanner * GH-611: strengthen the test scanners_struct_can_be_constructed_with_the_respective_scanners() * GH-611: improve test payable_scanner_can_initiate_a_scan() * GH-611: minor changes in the scanners.rs * GH-611: change the way we log NothingToProcess error * GH-611: strengthen the test receivable_scanner_scans_for_delinquencies() * GH-611: improve test for handle_none_status() * GH-611: remove the pub keyword from multiple functions inside the impl block of BeginScanError * GH-611: review changes for scanners_tools.rs * GH-611: use builder approach to build the scanners for tests * GH-611: remove the wrapper of migrator_config (risky) * GH-611: remove the wrapper from the when_pending_too_long (risky) * GH-611: put the mistakenly removed contract test back * GH-611: remove some unnecessary comments * GH-611: make the suppress_initial_scans flag just a boolean rather than wrapping * GH-611: minor remaining code changes * GH-611: Review 2 (#217) * GH-611: rename the function names again * GH-611: rename the field to when_pending_too_long_sec * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running * GH-611: use use_logs_containing inside the test periodical_scanning_for_receivables_and_delinquencies_works * GH-611: change the scan intervals back to their unique values * GH-611: improve timestamp_as_string function in scanners.rs * GH-611: rename the function to remove_timestamp_and_log * GH-611: migrate remove_timestamp_and_log to ScannerCommon * GH-611: change to exists_log_containing * GH-611: refactor handle_error() and remove log() inside BeginScanError * GH-611: use macro to remove code duplication in scanners.rs * GH-611: remove unnecessary modifications * GH-611: use the best practices of builder pattern for the individual scanner mocks * GH-611: refactor the constructor of Accountant * GH-611: rename the tests in scanners_tools.rs * GH-611: remove take() from the constructor of Accountant * GH-611: use just borrow for financial statistics * GH-611: remove the test that was testing panic * GH-611: remove assertions from the test start_message_triggers_no_scans_in_suppress_mode * GH 611: Review 3 (#220) * GH-611: remove the clone from the earning wallet iniside the Accountant's constructor * GH-611: rename the function to simply remove_timestamp() * GH-611: remove clone from the payment_thresholds * GH-611: change the test_name variable in tests for remove_timestamp * GH-611: rearrangement after the merge is practically done; 6 tests remains to be suspicious and will need to be examined for duplication or their utility. * GH-611: renaming file and modules to be more consistent (utils fits to file names around); also refactoring investigate_debt_extremes * GH-611: tests finally passing; next some refactoring * GH-611: todosudo chown -R bert:bert target are gone * GH-611: remove clippy warnings * GH-611: refactored AccountantBuilder to require fewer method calls for injecting individual mock DAOs * GH-611: the best I could do now for Utkarshe's review; let's consider that completed from my part * GH-611: bump the version from 0.7.0 to 0.7.1 * GH-611: change response_skeleton to response_skeleton_opt in ScanError * GH-611: send ScanError message to the Accountant when response_skeleton_opt is None * GH-611: end scan once a ScanError message is received * GH-611: assert on each case for handling scan error * GH-611: remove todos * GH-611: modify AccountantBuilder to accept logger * GH-611: rename the variable from is_scan_running to scan_started_at_opt * GH-611: remove unnecessary assertions from the helper fn assert_scan_error_is_handled_properly Co-authored-by: Dan Wiebe <dnwiebe@gmail.com> Co-authored-by: FinsaasGH <89403560+FinsaasGH@users.noreply.github.com> Co-authored-by: Bert <Bert@Bert.com>
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.
No description provided.