Initial support for forest-cli info #2578

creativcoder · 2023-02-21T12:53:54Z

Summary of changes

Changes introduced in this pull request:

Add a new info subcommand to forest-cli with the following information:

Network
Uptime
Chain status information
Chain health

This is ported from https://github.com/filecoin-project/lotus/blob/master/cli/info.go"

forest-cli info output:

Network: calibnet
Uptime: 0h 3m 210s (Started at: 2023-05-11 09:52:09.007870302 +00:00)
Chain: [sync: Ok! (9s behind)] [basefee: 100 atto FIL] [epoch: 549085]
Chain health: 71.58%


Default wallet address: -

Reference issue to close (if applicable)

Closes #2442

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

forest/cli/src/cli/info_cmd.rs

lemmih · 2023-03-01T20:01:18Z

@creativcoder Have you finished the change checklist?

forest/cli/src/cli/info_cmd.rs

lemmih · 2023-03-13T15:53:50Z

@creativcoder Could you show an example of the output?

forest/cli/src/cli/info_cmd.rs

creativcoder · 2023-03-14T10:39:35Z

@lemmih So far we have the following:

➜  forest2 git:(creativcoder/forest-info) ✗ ./target/release/forest-cli --chain calibnet info show
Network: calibnet
Uptime: 10h 27m 53s
Chain: [sync: Behind! (55959663 behind)] [basefee: 0.0000000000000001 pFIL] [epoch: 382117]
Chain health: 60%

Default wallet address: t1ac6ndwj6nghqbmtbovvnwcqo577p6ox2pt52q2y

node/rpc-api/src/lib.rs

node/rpc/src/sync_api.rs

lemmih · 2023-03-14T10:50:57Z

@lemmih So far we have the following:

➜  forest2 git:(creativcoder/forest-info) ✗ ./target/release/forest-cli --chain calibnet info show
Network: calibnet
Uptime: 10h 27m 53s
Chain: [sync: Behind! (55959663 behind)] [basefee: 0.0000000000000001 pFIL] [epoch: 382117]
Chain health: 60%

Default wallet address: t1ac6ndwj6nghqbmtbovvnwcqo577p6ox2pt52q2y

This should be in the PR description.

lemmih · 2023-03-14T10:53:05Z

And concepts such as chain health need to be documented.

LesnyRumcajs · 2023-03-14T13:11:36Z

0.0000000000000001 pFIL I think I lack Maths PhD to decode that on the fly. Could we use attoFIL?

forest/cli/src/cli/info_cmd.rs

lemmih · 2023-06-13T10:57:03Z

forest/cli/src/cli/info_cmd.rs

+        cur_duration: Duration,
+        blocks_per_tipset_last_finality: f64,
+        head: TipsetJson,
+    ) -> anyhow::Result<NodeStatusInfo> {


This function can no longer fail.

lemmih · 2023-06-13T10:57:54Z

forest/cli/src/cli/info_cmd.rs

+        if self.use_color {
+            writeln!(&mut output, "Network: {}", self.network())?;
+            writeln!(&mut output, "Uptime: {}", self.uptime(now))?;
+            writeln!(&mut output, "Chain: {}", self.chain_status())?;
+            writeln!(&mut output, "Chain health: {}", self.health())?;
+            writeln!(
+                &mut output,
+                "Default wallet address: {} {}",
+                self.wallet_address(),
+                self.wallet_balance()
+            )?;
+        } else {
+            writeln!(&mut output, "Network: {}", self.network().clear())?;
+            writeln!(&mut output, "Uptime: {}", self.uptime(now).clear())?;
+            writeln!(&mut output, "Chain: {}", self.chain_status().clear())?;
+            writeln!(&mut output, "Chain health: {}", self.health().clear())?;
+            writeln!(
+                &mut output,
+                "Default wallet address: {} {}",
+                self.wallet_address().clear(),
+                self.wallet_balance().clear()
+            )?;
+        }


Refactor this to avoid duplicate code.

lemmih · 2023-06-13T10:58:15Z

forest/cli/src/cli/info_cmd.rs

+                let blocks_per_tipset_last_finality =
+                    node_status.chain_status.blocks_per_tipset_last_finality;
+
+                let mut node_status_info = NodeStatusInfo::new(


This doesn't need to be mut if you refactor it slightly.

I get https://github.com/ChainSafe/forest/actions/runs/5256500061/jobs/9497916282?pr=2578#step:6:1508 when moving wallet and balance fields as params

refactored to remove use_color field

lemmih · 2023-06-13T14:58:48Z

forest/cli/src/cli/info_cmd.rs

+        let balance_token_amount = TokenAmount::from_atto(bal.parse::<BigInt>()?);
+        Ok(format!("{:.4}", balance_token_amount.pretty()))
+    } else {
+        Err(anyhow::anyhow!("error parsing balance"))


This looks off to me. It returns a parsing error if there's no default wallet.

Reworded error msg

forest/cli/src/cli/info_cmd.rs

dismissing to allow others to review this PR

aatifsyed

nits

aatifsyed · 2023-06-15T13:43:18Z

forest/cli/src/cli/info_cmd.rs

+        output.push(network);
+        output.push(uptime);
+        output.push(chain);
+        output.push(chain_health);
+        output.push(wallet_info);
+
+        output


Just construct a vec with vec![..] here.
It's surprising to return Vec<String> - could you add a comment explaining why?

was needed for color strings fmt, but not needed atm. Changed it to a String

aatifsyed · 2023-06-15T13:44:56Z

forest/cli/src/cli/info_cmd.rs

+fn balance(balance: &Option<String>) -> anyhow::Result<String> {
+    if let Some(bal) = balance {
+        let balance_token_amount = TokenAmount::from_atto(bal.parse::<BigInt>()?);
+        Ok(format!("{:.4}", balance_token_amount.pretty()))
+    } else {
+        Err(anyhow::anyhow!("could not find balance"))
+    }
+}


It's very odd to have a function take in a single &Option<T> - just make the inner function, and call one of the Option combinators on the outer option

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8131fccabf844bba1255f586298b2ac7

Of course, in this example, the mapped function is trivial and should be inlined.

aatifsyed · 2023-06-15T15:00:34Z

forest/cli/src/cli/info_cmd.rs

+        let wallet_info = {
+            let wallet_address = self
+                .default_wallet_address
+                .clone()
+                .unwrap_or("address not set".to_string());
+
+            let wallet_balance = match balance(&self.default_wallet_address_balance) {
+                Ok(bal) => format!("[balance: {}]", bal),
+                Err(e) => e.to_string(),
+            };
+
+            format!(
+                "Default wallet address: {} [{}]",
+                wallet_address, wallet_balance
+            )
+        };


Consider rewriting this as follows:

Suggested change

let wallet_info = {

let wallet_address = self

.default_wallet_address

.clone()

.unwrap_or("address not set".to_string());

let wallet_balance = match balance(&self.default_wallet_address_balance) {

Ok(bal) => format!("[balance: {}]", bal),

Err(e) => e.to_string(),

};

format!(

"Default wallet address: {} [{}]",

wallet_address, wallet_balance

)

};

let wallet_info = match (

&self.default_wallet_address,

&self.default_wallet_address_balance,

) {

(None, None) => String::from("<no default wallet address>"),

(Some(address), Some(balance)) => {

let attos = balance

.parse::<BigInt>()

.expect("RPC returned an invalid string for balance");

let token_amount = TokenAmount::from_atto(attos);

format!(

"Default wallet address: {address} [{:.4}]",

token_amount.pretty()

)

}

(None, Some(_)) | (Some(_), None) => {

unreachable!("If there's a default address, we should have the balance")

}

};

Note a couple of things here:

writing exhaustively and defensively has shown us that there's some bugs in our logic somewhere

we should either have all the default wallet info, or none of it

we should always have a valid wallet string

Okay, so now we look for the causes of those bugs:

The new function should be fallible, we shouldn't have error strings in our format function. It should error if we've got an address but no balance (and vice versa). It should be fine for the caller to unwrap that

Our RPC API is weakly typed. If it ever returns an invalid wallet string, something as gone wrong. We should panic there, as it's a software bug. Else, we should carry one with strong types. That means the RPC API should return TokenAmount, not a String. This is one of the main benefits of using a type system

The "principles" at play here are

making illegal states unrepresentable

fail fast

"defensive programming"

So we shouldn't really have strings anywhere in our NodeStatusInfo - consider actual types

I'm not going to hold up the PR on this, because you'll be doing follow on work, and this has been open a while anyway

Noted. Thanks for the detailed review. We did decide to refactor RPC APIs to return strongly typed values, that'll be part of the follow-up PR and also accordingly the wallet fmt code.

lemmih · 2023-06-15T15:15:59Z

forest/cli/src/cli/info_cmd.rs

+            .contains(&expected_status_fmt));
+    }
+
+    // FIXME: enable/debug this failing when color output is supported.


I'd greatly prefer it if this uncommented code was removed before merging.

removed the commented test

creativcoder force-pushed the creativcoder/forest-info branch 3 times, most recently from 09de7e8 to fb0506c Compare February 24, 2023 15:05

creativcoder marked this pull request as ready for review February 24, 2023 15:51

creativcoder requested review from elmattic, lemmih, LesnyRumcajs, jdjaustin, tyshko5, hanabi1224 and sudo-shashank as code owners February 24, 2023 15:51

hanabi1224 reviewed Feb 24, 2023

View reviewed changes

forest/cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

creativcoder force-pushed the creativcoder/forest-info branch 3 times, most recently from 70375df to c8d9dac Compare March 1, 2023 15:44

lemmih reviewed Mar 2, 2023

View reviewed changes

forest/cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

lemmih reviewed Mar 2, 2023

View reviewed changes

forest/cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

creativcoder force-pushed the creativcoder/forest-info branch from d1ea5ee to b11cead Compare March 3, 2023 11:27

creativcoder force-pushed the creativcoder/forest-info branch from b11cead to c63bd8a Compare March 13, 2023 11:40

lemmih reviewed Mar 14, 2023

View reviewed changes

forest/cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

lemmih requested changes Mar 14, 2023

View reviewed changes

node/rpc-api/src/lib.rs Outdated Show resolved Hide resolved

node/rpc-api/src/lib.rs Outdated Show resolved Hide resolved

node/rpc/src/sync_api.rs Outdated Show resolved Hide resolved

lemmih reviewed Mar 14, 2023

View reviewed changes

forest/cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

creativcoder force-pushed the creativcoder/forest-info branch 2 times, most recently from 65a25e3 to b4cb1a3 Compare March 15, 2023 12:49

creativcoder requested a review from lemmih June 13, 2023 10:48

Merge branch 'main' into creativcoder/forest-info

d5878a3

lemmih requested changes Jun 13, 2023

View reviewed changes

creativcoder added 2 commits June 13, 2023 13:20

Merge branch 'main' into creativcoder/forest-info

f34a267

remove duplicate code

85e165a

lemmih previously requested changes Jun 13, 2023

View reviewed changes

creativcoder added 8 commits June 14, 2023 07:47

minor changes

f33acb4

lint

305cf1f

lint

5319a80

Merge branch 'main' into creativcoder/forest-info

9c8f227

rm use_color param

50d29ae

Merge branch 'main' into creativcoder/forest-info

37503dc

rm dup

41a811a

rm dup

530813f

creativcoder requested a review from lemmih June 14, 2023 11:55

creativcoder added 4 commits June 14, 2023 12:08

Merge branch 'main' into creativcoder/forest-info

0640c68

Merge branch 'main' into creativcoder/forest-info

b74ae6e

rm color support

33f92f2

Merge branch 'main' into creativcoder/forest-info

66ac40b

aatifsyed approved these changes Jun 15, 2023

View reviewed changes

creativcoder added 2 commits June 15, 2023 14:27

review: use string as return type in format fn

7f42fe4

refactor balance fn

7651684

aatifsyed reviewed Jun 15, 2023

View reviewed changes

lemmih reviewed Jun 15, 2023

View reviewed changes

creativcoder added 2 commits June 16, 2023 08:00

rm commented test

4aa73cc

Merge branch 'main' into creativcoder/forest-info

2740b97

creativcoder enabled auto-merge (squash) June 16, 2023 08:15

creativcoder merged commit 105dbe5 into main Jun 16, 2023

creativcoder deleted the creativcoder/forest-info branch June 16, 2023 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for forest-cli info #2578

Initial support for forest-cli info #2578

creativcoder commented Feb 21, 2023 •

edited

Loading

lemmih commented Mar 1, 2023

lemmih commented Mar 13, 2023

creativcoder commented Mar 14, 2023

lemmih commented Mar 14, 2023

lemmih commented Mar 14, 2023

LesnyRumcajs commented Mar 14, 2023

lemmih Jun 13, 2023

lemmih Jun 13, 2023

creativcoder Jun 13, 2023

lemmih Jun 13, 2023

creativcoder Jun 13, 2023

creativcoder Jun 14, 2023

lemmih Jun 13, 2023

creativcoder Jun 14, 2023

aatifsyed left a comment

aatifsyed Jun 15, 2023

creativcoder Jun 15, 2023

aatifsyed Jun 15, 2023

aatifsyed Jun 15, 2023

aatifsyed Jun 15, 2023 •

edited

Loading

aatifsyed Jun 15, 2023

aatifsyed Jun 15, 2023

creativcoder Jun 16, 2023

lemmih Jun 15, 2023

creativcoder Jun 16, 2023

Initial support for forest-cli info #2578

Initial support for forest-cli info #2578

Conversation

creativcoder commented Feb 21, 2023 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

lemmih commented Mar 1, 2023

lemmih commented Mar 13, 2023

creativcoder commented Mar 14, 2023

lemmih commented Mar 14, 2023

lemmih commented Mar 14, 2023

LesnyRumcajs commented Mar 14, 2023

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

aatifsyed 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

aatifsyed Jun 15, 2023 • edited Loading

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

creativcoder commented Feb 21, 2023 •

edited

Loading

aatifsyed Jun 15, 2023 •

edited

Loading