Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

IPC-373: Fix balances returned with bogus rpc #379

Merged
merged 2 commits into from
Nov 6, 2023
Merged

IPC-373: Fix balances returned with bogus rpc #379

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de0ea1f
IPC-373: Fix balances returned with bogus rpc
adlrocha Nov 6, 2023
1ea406e
IPC-373: return errors at the end
adlrocha Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 21 additions & 8 deletions ipc/cli/src/commands/wallet/balances.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ impl CommandLineHandler for WalletBalances {

let wallet_type = WalletType::from_str(&arguments.wallet_type)?;
let subnet = SubnetID::from_str(&arguments.subnet)?;
let mut errors = Vec::new();

match wallet_type {
WalletType::Evm => {
let wallet = provider.evm_wallet()?;
let addresses = wallet.read().unwrap().list()?;
let mut no_balance = addresses.clone();
let r = addresses
.iter()
.map(|addr| {
Expand All @@ -50,15 +51,27 @@ impl CommandLineHandler for WalletBalances {

let v: Vec<anyhow::Result<(TokenAmount, &EthKeyAddress)>> = join_all(r).await;

for r in v.into_iter().filter_map(|r| r.ok()) {
let (balance, addr) = r;
if addr.to_string() != "default-key" {
println!("{} - Balance: {}", addr.to_string(), balance);
no_balance.retain(|a| a != addr);
for r in v.into_iter() {
match r {
Ok(i) => {
let (balance, addr) = i;
if addr.to_string() != "default-key" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this "default-key"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the internal key used by the wallet store to tag the address being used as the default one for the wallet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange to see that returned in the API 🤔
And why do we skip printing the balance? Aren't you interested in seeing the balance of your default (maybe only) address? In JSON this could be {addr: "f410...", balance: 1.23, default: true}

Copy link
Contributor

@jsoares jsoares Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the default still stands as its own entry, no? At least I still see my default address printed out under its address, just not duplicated as default-key. A more elegant approach for the future would be to mark the default address with a > or *.

println!("{} - Balance: {}", addr.to_string(), balance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we talked about this, but jsonline would be a good output format IMO, as it would make it easy to do something with the result using jq. A human readable output format is a great option, but personally I think it's liberating to print out structured data an defer the responsibility of formatting to the consumer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree, but let's tackle this as a joint effort after LabWeek and let's track the final approach we want to follow here: consensus-shipyard/ipc#365. So far depending on who is reviewing the PR they suggest one approach or the other 🙈

For this one specifically, this approach seems more readable for humans than returning a JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure it is more readable, and I agree you can postpone it. I guess like ls -lh you could have a "human readable hint" but it could also end up being an overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ I like this idea of having a flag for human-readable v.s. computer-readable. Will track it in the issue 🙏

}
}
Err(e) => {
errors.push(e);
}
}
}
for addr in no_balance {
println!("{} - Balance: 0", addr.to_string());

if !errors.is_empty() {
let error = errors
.into_iter()
.fold(anyhow::anyhow!("Error fetching balances"), |acc, err| {
acc.context(err)
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

return Err(error);
}
}
WalletType::Fvm => {
Expand Down