IPC-373: Fix balances returned with bogus rpc #379
Conversation
| Ok(i) => { | ||
| let (balance, addr) = i; | ||
| if addr.to_string() != "default-key" { | ||
| println!("{} - Balance: {}", addr.to_string(), balance); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
^ I like this idea of having a flag for human-readable v.s. computer-readable. Will track it in the issue 🙏
| match r { | ||
| Ok(i) => { | ||
| let (balance, addr) = i; | ||
| if addr.to_string() != "default-key" { |
There was a problem hiding this comment.
What is this "default-key"?
There was a problem hiding this comment.
This is the internal key used by the wallet store to tag the address being used as the default one for the wallet.
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
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 *.
| } | ||
| } | ||
| Err(e) => { | ||
| return Err(e); |
There was a problem hiding this comment.
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<_>>>()?;There was a problem hiding this comment.
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.
| .into_iter() | ||
| .fold(anyhow::anyhow!("Error fetching balances"), |acc, err| { | ||
| acc.context(err) | ||
| }); |
Fixes consensus-shipyard/ipc#364