Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bad error-messaging #2437

Merged
merged 8 commits into from
May 17, 2024
Merged

fix bad error-messaging #2437

merged 8 commits into from
May 17, 2024

Conversation

k9ert
Copy link
Collaborator

@k9ert k9ert commented May 15, 2024

In the case of using spectrum, errors which came from the electrum server, did not got shown very well to the user.
In one case we had
image

This is caused by the server not returning json which in turn is caused by the client not adding "Accept:application/json" to the header. Also the errorClass translation from electrum -> spectrum -> specter-ext -> specter -> user was quite improvable.

Copy link

netlify bot commented May 15, 2024

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit b24a2c1
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/66466b77b7ae640008373b69

@k9ert k9ert changed the title fix bad error-message fix bad error-messaging May 15, 2024
@moneymanolis
Copy link
Collaborator

moneymanolis commented May 15, 2024

@k9ert can we remove these console.logs in this PR before the release:

especially the console log of the PSBT should be removed, what do you think?

@moneymanolis
Copy link
Collaborator

getting this error right now when using 1 sat / vbyte fee on testnet:
image

[2024-05-15 19:11:01,406] ERROR in controller: Uncaught exception: string indices must be integers
[2024-05-15 19:11:01,410] ERROR in controller: Traceback (most recent call last):
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/flask/app.py", line 1823, in full_dispatch_request
    rv = self.dispatch_request()
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/flask/app.py", line 1799, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/flask_login/utils.py", line 290, in decorated_view
    return current_app.ensure_sync(func)(*args, **kwargs)
  File "/Users/manolis/coding/repos/specter-desktop/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py", line 182, in broadcast
    app.specter.broadcast(tx)
  File "/Users/manolis/coding/repos/specter-desktop/src/cryptoadvance/specter/specter.py", line 513, in broadcast
    res = self.rpc.sendrawtransaction(raw)
  File "/Users/manolis/coding/repos/specter-desktop/src/cryptoadvance/specter/rpc.py", line 507, in fn
    r = self.multi([(method, *args)], **kwargs)[0]
  File "/Users/manolis/coding/repos/specter-desktop/src/cryptoadvance/specterext/spectrum/bridge_rpc.py", line 102, in multi
    result = [
  File "/Users/manolis/coding/repos/specter-desktop/src/cryptoadvance/specterext/spectrum/bridge_rpc.py", line 103, in <listcomp>
    self.spectrum.jsonrpc(
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/cryptoadvance/spectrum/spectrum.py", line 499, in jsonrpc
    raise e
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/cryptoadvance/spectrum/spectrum.py", line 489, in jsonrpc
    res = m(*args, **kwargs)
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/cryptoadvance/spectrum/spectrum.py", line 56, in wrapper
    return f(*args, **kwargs)
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/cryptoadvance/spectrum/spectrum.py", line 716, in sendrawtransaction
    res = self.sock.call("blockchain.transaction.broadcast", [hexstring])
  File "/Users/manolis/.pyenv/versions/.specter/lib/python3.10/site-packages/cryptoadvance/spectrum/elsock.py", line 564, in call
    raise RPCError(res["error"]["message"], res["error"]["code"])
TypeError: string indices must be integers

@moneymanolis
Copy link
Collaborator

@k9ert do we perhaps have to parse the JSON string into a dictionary correctly?

We should in any case better check whether res is a dict and not just a string with sth. like:

if isinstance(res, dict) and "error" in res:
    error = res.get("error", {})
    error_code = error.get("code")
    error_message = error.get("message")
    if error_code is not None and error_message is not None:
        raise RPCError(error_message, error_code)
    else:
        raise SpectrumInternalException(res)

@moneymanolis moneymanolis self-requested a review May 15, 2024 17:31
@k9ert
Copy link
Collaborator Author

k9ert commented May 16, 2024

especially the console log of the PSBT should be removed, what do you think?

Happy to remove them but they are not part of this PR so maybe we can do that in some other PR.

@k9ert
Copy link
Collaborator Author

k9ert commented May 16, 2024

Can't reproduce the "TypeError: string indices must be integers" issue. But i implemented your proposal anyway here:
cryptoadvance/spectrum#55
Please approve and i create yet another Spectrum release and use it here.

@moneymanolis
Copy link
Collaborator

I could broadcast with Spectrum and 1 sat fee on testnet without error now (might be that we are catching it now gracefully, which might be fine since it aligns with bitcoind).

@k9ert k9ert merged commit e29bc01 into cryptoadvance:master May 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants