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 1 commit
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
20 changes: 11 additions & 9 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 @@ -30,7 +30,6 @@ impl CommandLineHandler for WalletBalances {
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,16 +49,19 @@ 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) => {
return Err(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so we went from ignoring errors with .filter_map(|r| r.ok()) and printing 0 balance for them to printing the first few successful balances until we encounter an error, where we stop going down the list and return an overall failure.

That seems a bit inconsistent, that depending on which of the N requests failed I see more or less results, compared to for example:

  • Print all results if all of them succeeded or none if any of them failed
  • Print all results as JSON regardless of error or failure, but if it failed then print eg.. {addr: "...", error: "oh noes!"}
  • Print all the successful results first and then return an overall error if any failed

The all-or-nothing strategy can be achieved for example like so:

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

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 one is a bit tricky, because there are errors for which all balances will fail with the same error (like RPC unavailable), and others that may be address-specific (like address not initialized on-chain). I agree that my approach may have not been the best because it was non-deterministic, so I decided to go with your suggestion of printing all of the balances that succeed, and then returning the errors (if any) at the end. That way if it this is a balance-wide error (like RPC) we return a single error, and if it is address-specific we print the balances and then the errors. Let me know what you think.

}
}
}
for addr in no_balance {
println!("{} - Balance: 0", addr.to_string());
}
}
WalletType::Fvm => {
let wallet = provider.fvm_wallet()?;
Expand Down
Loading