feat: show more informative rpc error #216
Conversation
| @@ -155,6 +162,9 @@ const watchUserVaults = () => { | |||
| [Kind.Data, path], | |||
| value => { | |||
| console.debug('got update', subscriber, value); | |||
| if (!value) { | |||
| return; | |||
There was a problem hiding this comment.
We might receive the update to offerIdsToPublicSubscribers from the user's wallet and start watching the vault before the vault data is actually published to vstorage, so we don't want to error out in that case. Just wait for the data to show up assuming it will eventually. I'm not sure how else to handle it, but worst case if the contract had a bug the vault would just perpetually show a loading state.
| setError(e); | ||
| setChainConnectionError( | ||
| new Error( | ||
| `Error fetching network config from ${netConfig.url}` + |
There was a problem hiding this comment.
question:
Why is this using
e instanceof Error ? `: ${e.message}` : ''
while just above it's only
e.message
Is it because we know makeAgoricChainStorageWatcher is giving us an instance of Error but we don't know if the catch is?
There was a problem hiding this comment.
There's probably value in introducing a helper for use in both places; something like
const makeError = (msg, { cause } = {}) => {
const longMsg = !cause
? msg
: `${msg}: ${cause instanceof Error ? cause.message : cause}`;
return Error(longMsg, { cause });
};
| setError(e); | ||
| setChainConnectionError( | ||
| new Error( | ||
| `Error fetching network config from ${netConfig.url}` + |
There was a problem hiding this comment.
There's probably value in introducing a helper for use in both places; something like
const makeError = (msg, { cause } = {}) => {
const longMsg = !cause
? msg
: `${msg}: ${cause instanceof Error ? cause.message : cause}`;
return Error(longMsg, { cause });
};| )} | ||
| {rpcNode && ( | ||
| <p> | ||
| RPC Endpoint: <span className="text-blue-500">{rpcNode}</span> |
There was a problem hiding this comment.
Nit: a blue URL looks like a hyperlink. It might be best to choose a different color and/or replace the <span> with <input readonly>.
| </p> | ||
| )} | ||
| <p> | ||
| Error:{' '} |
There was a problem hiding this comment.
We can avoid repeating "error".
| Error:{' '} | |
| Detail:{' '} |
Also, is text:{' '} more idiomatic than {'text: '}?
| const body = ( | ||
| <div className="mt-2 p-1 max-h-96 overflow-y-auto"> | ||
| <p className="mb-2"> | ||
| There was an issue connecting to the chain - likely due to RPC node |
There was a problem hiding this comment.
| There was an issue connecting to the chain - likely due to RPC node | |
| There was a problem connecting to the chain - likely due to RPC node |
| ); | ||
|
|
||
| return ( | ||
| <ActionsDialog |
There was a problem hiding this comment.
Not requesting a change, but I guess this is no longer an actions dialog.
| appStore.setState({ | ||
| watchVbankError: 'Error loading asset display info', | ||
| watchVbankError: `${path} returned undefined`, |
There was a problem hiding this comment.
| watchVbankError: `${path} returned undefined`, | |
| watchVbankError: `No value at ${path}`, |
(better covers non-undefined falsy values in correspondence with the actual test)
| appStore | ||
| .getState() | ||
| .setChainConnectionError( | ||
| new Error( |
There was a problem hiding this comment.
Another good candidate for the suggested helper.
refs Agoric/agoric-sdk#8505
fixes Agoric/ui-kit#57
This uses the new
@agoric/rpcand@agoric/web-componentsto make some improvements to error handling as a precursor to the node selector. The new package versions have a few improvements:onErrorhandler for each vstorage path. For missing data, it now just yields an emptyvalue, and server errors are surfaced to the singleonErrorhandler of thechainStorageWatcheronError(in both wallet connection component andchainStorageWatcher)With all these changes, the dapp can now more gracefully and confidently handle server errors and show a more informative dialog instead of crashing with an unhelpful error message on the screen. In the future we can prompt the user to select a different node in this case, but for now just the dialog is being added.
Screenshot