MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.7k stars 4.79k forks source link

RPC errors not bubbled back to Ðapp #8483

Closed danfinlay closed 4 months ago

danfinlay commented 4 years ago

Currently when a Ðapp initiates a call to eth_sendTransaction, if the RPC backend throws an error (like transaction underpriced), this error is not returned to the Ðapp, instead a generic internal wrapper is returned, like "Error: [ethjs-rpc] rpc error with payload {"...

Checked and this behavior continues to exist on develop.

Ideal case:

wighawag commented 4 years ago

Ok, seems I am getting this issue, but I am getting the following error for every metamask tx, even when I use metamask ui to send ether to another address

Here is the error I get when executing one my dapp transaction :

{code: -32603, message: "Error: [ethjs-rpc] rpc error with payload {"id":79…method":"eth_sendRawTransaction"} [object Object]", stack: "Error: Error: [ethjs-rpc] rpc error with payload {…method":"eth_sendRawTransaction"} [object Object]"}
stack : "Error: Error: [ethjs-rpc] rpc error with payload {"id":7973398896595,"jsonrpc":"2.0","params":["0xf8cb2b8504a817c80082bd5d943037e0b21e7b8e0babaf3611d6d0d5cac51e67c080b864c47f0027000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000036473640000000000000000000000000000000000000000000000000000000000820a95a04314f99297ee7e82db567bc8008c3d2739558e9010b9dac763d266a71f72bb0ca03a9aa5658bfc4dde3c754c40a5345aaba47819cc6df5b39c57144fb42c60bf85"],"method":"eth_sendRawTransaction"} [object Object]"
wighawag commented 4 years ago

This was on chrome and loaded a new profile with a new metamask and there my dapp function fine

adrianmcli commented 4 years ago

This is quite frustrating, as things work fine when I am just running scripts in Node.js but suddenly throws a cryptic error when I do it through the frontend. Is there a toString() getting called somewhere that's causing the underlying object (w/ revert reason string) to become [object Object] instead?

MicahZoltu commented 4 years ago

Not only is the RPC mangled, it also is wrapped in a more opaque error. When I inspect the JSON-RPC error I get back from sendAsync, I see:

{
  "code": -32603,
  "message": "[object Object]",
  "data": {
    "code": -32603,
    "message": "revert: ERC20: transfer amount exceeds allowance"
  },
  "stack": "..."
}

Note that the actual JSON-RPC error (the onet hat would be immensely useful for debugging) is burried inside the data field of the error I actually get back.

wbt commented 4 years ago

I am not seeing the data member in Chrome.

Gudahtt commented 3 years ago

Relates to #8975 and #7142

MicahZoltu commented 3 years ago

I received an error report from a user today where it appears MetaMask is throwing a string instead of an error! This means I don't get a stack trace, which is critical for debugging.

This is what is showing up in my code's catch block.

{
    "code":-32000,
    "message":"err: insufficient funds for gas * price + value: address ... have ... want ... (supplied gas ...)"
}
vandan commented 1 year ago

Duplicate of #12646

MicahZoltu commented 1 year ago

I think you mean that #12646 is a duplicate of this. 😉

github-actions[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

adrianmcli commented 10 months ago

I don't think this is stale.

vandan commented 9 months ago

A related PR was created https://github.com/MetaMask/metamask-extension/pull/15205 but resulted in other issues (tradeoffs). Needs further assessment on the approach here.

wighawag commented 9 months ago

@vandan thanks for chiming in here

Could you please do so for all issues that are being closed by bots. This is honestly alienating people like me who would live to see Metamask improve and create issue with that goal in mind, but whose only interlucutor is a bot closing issues :(

I understand that you might have too many issues in your hand, but closing issues blindly will only make the issue count down. The issue wont resolve themeselves.

Sorry if that sounds harsh but some issue are being closed while they exist in the repo for a long time with few or no comment from Metamask team

vandan commented 9 months ago

@vandan thanks for chiming in here

Could you please do so for all issues that are being closed by bots. This is honestly alienating people like me who would live to see Metamask improve and create issue with that goal in mind, but whose only interlucutor is a bot closing issues :(

I understand that you might have too many issues in your hand, but closing issues blindly will only make the issue count down. The issue wont resolve themeselves.

Sorry if that sounds harsh but some issue are being closed while they exist in the repo for a long time with few or no comment from Metamask team

We understand your frustration. We'll see what can be done about the automated actions, but generally the best way we can avoid this is by being more responsive to these issues so they don't become stale in the first place. We absolutely appreciate the effort our community goes through to help MetaMask improve and don't want to lose your goodwill. 🙏

MicahZoltu commented 9 months ago

FWIW, I have stopped filing issues with MetaMask because 90%+ of the issues I encounter just get closed by the bot and remains unfixed.

adrianmcli commented 9 months ago

What's most ironic is that this issue was filed by @danfinlay himself.

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

adrianmcli commented 6 months ago

Not stale. Do we know if this new plan has changed and the necessary fixes inside ethjs-query have been implemented?

image

From here: https://github.com/MetaMask/metamask-extension/pull/15205#issuecomment-1199953855

I'm not sure if I have the energy to say "not stale" again.

MicahZoltu commented 6 months ago

Sadly, I gave up on filing issues and commenting "not stale". 90% of the issues I follow/file just go stale and are never fixed, even after many years of the bug existing.

adrianmcli commented 6 months ago

Sadly, I gave up on filing issues and commenting "not stale". 90% of the issues I follow/file just go stale and are never fixed, even after many years of the bug existing.

With all the money that MetaMask makes. I wonder why they can't have someone do triage on these open source issues?

And I've said it above a few months ago, but the most ironic thing is that this is an issue @danfinlay raised himself.

Dan probably will not remember, but I have fond memories of chatting with Dan when I was part of ConsenSys in 2018. Times have really changed.

BelfordZ commented 5 months ago

Not stale. Do we know if this new plan has changed and the necessary fixes inside ethjs-query have been implemented?

image

From here: #15205 (comment)

I'm not sure if I have the energy to say "not stale" again.

Sorry, I tried to get it sorted out but it turned out to be more complicated then it seemed. I had a this PR that we had to reject becasue it would cause breaking changes for those who had already written code to parse these bad errors. A new plan was hatched, but other priorities came up and I wasn't able to keep at it.

Just know that its on our radar, and it will get better. I also understand asking for patience at this point is almost insulting, so I will spare you on that.

vandan commented 4 months ago

This issue is no longer reproducible. See our notes for the detail about why the issue is resolved.

Please re-open if you are able to reproduce.