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

masqrauder · 2024-01-21T21:01:02Z

No description provided.

bertllll

There is an important comment at the beginning of the file "migration_9_to_10.rs".

bertllll · 2024-01-23T04:25:39Z

masq/src/commands/set_configuration_command.rs

+    } else {
+        match start_block.parse::<u64>() {
+            Ok(_) => Ok(()),
+            _ => Err(start_block),


I'd like to see a longer err message that would contain also either the parse err or an assistive instruction that it needs to be a valid integer.

bertllll · 2024-01-23T04:29:11Z

masq/src/commands/set_configuration_command.rs

@@ -126,6 +130,7 @@ mod tests {
    fn validate_start_block_works() {


This is actually a bad test because if the function returns an err we want to know if it is an appropriate one, or its message meaningful.

I'd like to see two-sides assertion with Ok(()) or Err("The error message").

I think this now does what you mean... 2 separate test methods one for Err cases and this one for Ok cases.

bertllll · 2024-01-23T04:41:38Z

node/src/blockchain/blockchain_bridge.rs

-            Err (e) => panic! ("Cannot retrieve start block from database; payments to you may not be processed: {:?}", e)
+            Ok(Some(sb)) => sb,
+            Ok(None) => u64::MAX,
+            Err(e) => panic!("Cannot retrieve start block from database; payments to you may not be processed: {:?}", e)
        };
        let max_block_count = match self.persistent_config.max_block_count() {
            Ok(Some(mbc)) => mbc,


I'm having a hard time to accept that we ignore the error. I know it isn't dangerous if we don't have it, on the other hand, at all places I remember we treat such an error from our communication with the database manager, specifically, we always go and panic.

An argument is, though, that there is a card that should make this reaction implicit, ripping the Result structure from all the methods of PersistentConfiguration. Now, advice is worth gold. I don't feel like needing to insist on this.

bertllll · 2024-01-23T04:46:40Z

node/src/blockchain/blockchain_bridge.rs

                if max_block_count == u64::MAX {
+                    info!(
+                        self.logger,
+                        "Using 'latest' block number instead of a literal number. {:?}", e


You first need to think about what to do with the error. Please don't just throw it away. I think logging would be fair, we do it all the time.

The way from here, I think, goes by dividing this message into two logs (or even three, according to the differences I mentioned about the if statement). One for the error, maybe perceived as WARN and another less sever with this little informative message (or actually two like that).

If you want to keep an info log, which means one that "we want the user to see", it maybe should have more information to give so that they understand the context. Like what bigger operation is going on of which this is a smaller part.

Remember there is a rule that all info logs will eventually display to the user directly in the UI, not just write to the file.

I do think this kind of information is useful for us developers rather than anybody else so I'd choose just a debug log instead (or actually two debug logs). The fine think about this is you wouldn't have to make assertions on them.

bertllll · 2024-01-23T04:49:56Z

node/src/blockchain/blockchain_bridge.rs

@@ -314,25 +315,31 @@ impl BlockchainBridge {
            .get_block_number()
        {
            Ok(eb) => {
-                if u64::MAX == max_block_count {
+                if u64::MAX == max_block_count || u64::MAX == start_block_nbr {


You use this boolean condition a couple of times again and again. Maybe you could create a variable before here, with a nice name, that would store the resolution of this expression. Then use the variable where you need to.

I think I'd find it easier to understand since I wouldn't have to check if those are different conditions or the same one all the time.

bertllll · 2024-01-23T10:17:29Z

node/src/database/config_dumper.rs

+    fn assert_none(key: &str, map: &Map<String, Value>) {
+        assert!(!map.get(key).is_none());
+    }
+


I have two notes on this.

First, I somewhat cannot believe this assertion stands right. I think it tests something completely different than what the name of this helper function tries to convey.

It only tests that "a certain parameter (and its row) IS present in the db". That is far from what you intended to get.

Let me suggest a solution:
You should simply reach the value from the set by using 'get(key)' but don't stop there, now you got an enum with variants representing possible values that can take part in a JSON. Look at the body of "assert_value" above your function. Do you see 'Value::String()'? That's one of the variants. Another one is 'Value::Null'. Your assertion should only make sure that you get exactly this Value when you ask for your parameter.

bertllll · 2024-01-23T10:20:08Z

node/src/node_configurator/configurator.rs

+        assert_eq!(*check_start_block_params, vec![Some(166666)]);
+        TestLogHandler::new().exists_log_containing(&format!(
+            "DEBUG: {}: A request from UI received: {:?} from context id: {}",
+            test_name, msg, context_id


We try not to assert on Debug logs 🤷‍♂️

bertllll · 2024-01-23T10:36:53Z

node/src/node_configurator/configurator.rs

@@ -549,7 +549,8 @@ impl Configurator {
            persistent_config.earning_wallet_address(),
            "earningWalletAddressOpt",
        )?;
-        let start_block = Self::value_required(persistent_config.start_block(), "startBlock")?;
+        let start_block_opt =
+            Self::value_not_required(persistent_config.start_block(), "startBlock")?;


You haven't provided any assertion for this where the value was None.

Is this where that is checked or do I need something more?: https://github.com/MASQ-Project/Node/blob/GH-606/node/src/node_configurator/configurator.rs#L2090

bertllll · 2024-01-23T10:47:02Z

node/src/node_configurator/configurator.rs

-        let block_number = match string_number.parse::<u64>() {
-            Ok(num) => num,
-            Err(e) => return Err((NON_PARSABLE_VALUE, format!("start block: {:?}", e))),
+        let block_number_opt = if "none".eq_ignore_ascii_case(&string_number) {


I'm sorry to be adding in more work for you but I need to be thorough. I'll bet the use of this function (from a library) isn't tested.

Once you use something that can behave variously or produce different outcomes you will need to test it separately.

I recommend to test this small function only, on values like "noNe, none, NONE", always passing the same. Using a PersistentConfigurationMock and assertions on the params of set_start_block(), making sure it is supplied by the 'None' value properly.

eq_ignore_ascii_case() looks to me like it's part of the Rust standard library, isn't it? Normally I try not to write tests that only test external library code already having its own tests. Do you agree?

bertllll · 2024-01-23T11:48:27Z

node/src/database/db_migrations/migrations/migration_9_to_10.rs

@@ -0,0 +1,71 @@
+use crate::database::db_migrations::db_migrator::DatabaseMigration;
+use crate::database::db_migrations::migrator_utils::DBMigDeclarator;
+


Ohhhhhhh. This took me a good amount of time before I realised. That what I want to tell you is probably not going to be good news to you.

It will affect a lot of comments that I'd made before this moment.

Thinking of schema of the database, you must admit that with your card there has been no change. Not even a fingernail.

The data types in the columns for the config table and the values of the parameters have ever been declared as an optional TEXT.

Which is exactly what you are using.

Now, let's thing about what older databases could have had in their row for the start_block. Well, it was always a number. (And even if there was a way how it could yield 'Null', it would still be completely in accordance with your design.

That means you don't need any db migration at all. For that reason, you should make sure all the related code will go away. I'm sorry.

Please don't forget delete this whole file.

…vements

masqrauder self-assigned this Jan 21, 2024

masqrauder requested review from dnwiebe, czarte, utkarshg6, bertllll and Syther007 January 21, 2024 21:01

masqrauder force-pushed the GH-606 branch from 893b10c to b7ed3de Compare January 22, 2024 01:37

bertllll reviewed Jan 23, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch from b7ed3de to b22b8e5 Compare March 16, 2024 02:18

masqrauder force-pushed the GH-606 branch from b22b8e5 to b0f9e02 Compare March 31, 2024 17:19

masqrauder force-pushed the GH-606 branch from 1f740b2 to 82f39d6 Compare April 21, 2024 19:02

masqrauder force-pushed the GH-606 branch from 9226df7 to e397bf3 Compare June 16, 2024 02:57

masqrauder added 7 commits July 10, 2024 21:20

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

5039c3a

GH-606: Apply PR feedback changes

bf3877d

GH-606: Apply PR feedback changes

717b966

GH-606: Apply PR feedback changes

5cbfd3a

GH-606: Apply PR review 4 feedback changes

0bcace5

Follow the Option naming pattern

105421b

Rebase with master

bda6f39

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

GH-606: debugging test 'verify bill payment', added some little impro…

e108ef6

…vements

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

masqrauder commented Jan 21, 2024

bertllll left a comment

bertllll Jan 23, 2024

bertllll Jan 23, 2024

masqrauder Apr 7, 2024

bertllll Jan 23, 2024

bertllll Jan 23, 2024

bertllll Jan 23, 2024

bertllll Jan 23, 2024

bertllll Jan 23, 2024

bertllll Jan 23, 2024

masqrauder Apr 7, 2024

bertllll Jan 23, 2024

masqrauder Apr 7, 2024

bertllll Jan 23, 2024

		@@ -126,6 +130,7 @@ mod tests {
		fn validate_start_block_works() {

		@@ -0,0 +1,71 @@
		use crate::database::db_migrations::db_migrator::DatabaseMigration;
		use crate::database::db_migrations::migrator_utils::DBMigDeclarator;

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

Are you sure you want to change the base?

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

Conversation

masqrauder commented Jan 21, 2024

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