metamask-mobile

metamask-mobile copied to clipboard

Sentry Issue: METAMASK-MOBILE-2E6V

Sentry Issue: METAMASK-MOBILE-2E7E

Sentry Issue: METAMASK-MOBILE-2F6D

Sentry Issue: METAMASK-MOBILE-2ERR

Sentry Issue: METAMASK-MOBILE-2EPT

Sentry Issue: METAMASK-MOBILE-2FXF

Sentry Issue: METAMASK-MOBILE-2DVD

node_modules/ethjs-ens/node_modules/ethjs-rpc/lib/index.js in self.currentProvider.sendAsync$argument_1 at line 53:35

  self.currentProvider.sendAsync(parsedPayload, function (err, response) {
    var responseObject = response || {};
    if (err || responseObject.error) {
      var payloadErrorMessage = '[ethjs-rpc] ' + (responseObject.error && 'rpc' || '') + ' error with payload ' + JSON.stringify(parsedPaylo {snip}
      var payloadError = new Error(payloadErrorMessage);
      payloadError.value = err || responseObject.error;
      return cb(payloadError, null);
    }
    return cb(null, responseObject.result);

I tracked this down to Engine.ts and the code pointed at in @metamask/json-rpc-engine but I'm not sure how this ends into an exception not being caught and land in Sentry.

@Gudahtt has some notes to add here.

The stack trace suggests that the error is happening as a result of an ethjs-ens call, but the usage of that package in this project is very limited. The only callsites I could find were within try..catch blocks that would not allow the error to reach Sentry. I tried auditing the package for errors thrown out of band as well, and was unable to find a single instance where that seemed possible.

Maybe it's not coming from ethjs-ens, and the stack trace that Sentry is showing us is incorrect? There are issues with the source maps in the affected releases.

Perhaps we could put this issue aside until the sourcemaps are fixed, so that we can verify what the real unminified stack trace is.

Or alternatively, we could manually verify each stack frame using the production bundle, but @NicolasMassart and I discussed this approach already and we ran into issues here because the bundle uses byte code rather than JavaScript (I'm not sure how to decompile it while preserving line numbers, and even if we manage to, the resulting code bears very little resemblance to the source code)

Okay, moving to Blocked for now.

Sentry Issue: METAMASK-MOBILE-2G16

Sentry Issue: METAMASK-MOBILE-2G16

Sentry Issue: METAMASK-MOBILE-2G68

Sourcemap are now correct, we check them regularily and at least for the last ones if matches the source code. We still may have issues as it's not solved on CI side, but if we figure it out fast enough and remove the extra artifacts it works. If sourcemap is wrong, the error message is compeletely unrelated to the code pointed at and it's clearly not the case here:

Error:

[ethjs-rpc] rpc error with payload {"id":7030016583578,"jsonrpc":"2.0","params":[{"to":"**","data":"******************************************5604fb93aada9cfabd08f3c0c91e4d26"},"latest"],"method":"eth_call"} Ty...

Code for node_modules/ethjs-ens/node_modules/ethjs-rpc/lib/index.js in self.currentProvider.sendAsync$argument_1 at line 53:35

  self.currentProvider.sendAsync(parsedPayload, function (err, response) {
    var responseObject = response || {};
    if (err || responseObject.error) {
      var payloadErrorMessage = '[ethjs-rpc] ' + (responseObject.error && 'rpc' || '') + ' error with payload ' + JSON.stringify(parsedPaylo {snip}
      var payloadError = new Error(payloadErrorMessage);
      payloadError.value = err || responseObject.error;
      return cb(payloadError, null);
    }
    return cb(null, responseObject.result);

Edited 10/29/24. I am not sure why I said ethjs-rpc. I meant ethjs-ens.

I looked into this, and there are two ~functions~ methods from ~ethjs-rpc~ the ENS class in ethjs-ens that are used in the mobile app. The two methods are reverse and lookup. Here is where they are used:

• async doENSReverseLookup (app/util/ENSUtils.js)
  • async validateAddressOrENS (app/util/address/index.js)
    • async validateAddressOrENSFromInput (app/components/Views/Settings/Contacts/ContactForm/index.js)
      • onChangeAddress (app/components/Views/Settings/Contacts/ContactForm/index.js)
        • note that the call to validateAddressOrENSFromInput is NOT awaited
    • async validateAddressOrENSFromInput (app/components/Views/confirmations/SendFlow/SendTo/index.js)
      • onToSelectedAddressChange
        • note that the call to validateAddressOrENSFromInput is NOT awaited
    • async validateAddressOrENSFromInput (app/component/Views/confirmations/components/ApproveTransactionReview/AddNickname/index.tsx)
      • useEffect
        • note that the call to validateAddressOrENSFromInput is NOT awaited
  • useEffect (app/components/UI/AccountFromToInfoCard/AccountFromToInfoCard.tsx)
    • note that the call to doENSReverseLookup is NOT awaited
  • ~async doENSLookup (app/components/UI/AccountOverview/index.js)~
    • errors here are caught
  • async updateAccountInfo (app/components/UI/DrawerView/index.js)
    • async componentDidUpdate (app/components/UI/DrawerView/index.js)
  • ~async useEnsNameByAddress (app/components/hooks/useEnsNameByAddress/index.ts)~
    • errors here are caught
  • ~async lookupEns (app/components/Views/EditAccountName/EditAccountName.tsx)~
    • errors here are caught
  • useEffect (app/components/Views/confirmations/SendFlow/AddressFrom/AddressFrom.tsx)
    • note that the call to doENSReverseLookup is effectively NOT awaited
  • async onSelectAccount (app/components/Views/confirmations/SendFlow/AddressFrom/AddressFrom.tsx)
    • I am not sure but I don't think this ends up being awaited
  • async fetchENSName (app/components/Views/confirmations/SendFlow/AddressElement/AddressElement.tsx)
    • useEffect (app/components/Views/confirmations/SendFlow/AddressElement/AddressElement.tsx)
      • note that the call to doENSReverseLookup is NOT awaited
• async doENSLookup (app/utils/ENSUtils.js)
  • async validateAddressOrENS (app/utils/address/index.js)
    • see above

Note that there are a few cases where doENSReverseLookup or doENSLookup is not properly awaited, and so the error may be thrown out of band.

So I have a couple of questions:

• If these errors aren't supposed to show up in Sentry, what is supposed to catch them?
• Was new functionality added recently in any of the files mentioned above that could cause this new error to pop up?

Sentry Issue: METAMASK-MOBILE-2GHZ

Sentry Issue: METAMASK-MOBILE-2GW2

There were some recent type changes in @metamask/json-rpc-engine that may have affected error handling behavior, although the intention was to avoid introducing runtime breaking changes.

Looking at the _promiseHandle method linked above that method calls #handle, whose callback parameter's error parameter was changed from optional to required in this line. Maybe this or another change made in this release is related to this bug? The callback parameter in this link would be directly downstream of cb in the sendAsync method mentioned above.

Another similar change was made here in the handle method while fixing an any type.

I took another look at this ticket. I did not manage to get to the bottom of it, but here is some more information that may be useful.

• First some versioning information. The latest instance of this error occurs on MetaMask Mobile 7.33.0. You must check out the v7.33.0 tag to look into this error.

• 7.33.0 is using:

  • ethjs-ens 2.0.1 (the latest version)
  • ethjs-rpc 0.1.5, vendored within ethjs-ens (NOT the latest version)
  • @metamask/json-rpc-engine 9.0.2 (NOT the latest version)

• I believe this error originates from the ethjs-ens package. That's not where it's ultimately thrown, but that's where it gets created. I know we have been a bit unsure of this, but I can't find any evidence this is not the case.

• More about ethjs-ens:

  • The default export for ethjs-ens is a class called ENS.
  • This class uses two smart contracts to look up the address for an ENS name and vice versa. First a Registry contract is used to find a Resolver and then a Resolver contract is used to actually do the lookup.
  • ethjs-ens uses the ethjs-contract package to interact with the contracts. ethjs-ens is using a specific version of this package (it is not hoisted to the root of the dependency tree).

• More about ethjs-contract:

  • The default export for ethjs-contract is a function which is mistakenly treated as a class in ethjs-ens. This function creates an object with two methods; ethjs-ens uses the at method (so we can ignore the other).
  • The at method creates a contract object on the fly. Methods on this object correspond to the callable methods of the contract.
  • In this case, the contract object for the Registry contract has a resolver method, and the contract object of the Resolver has addr and name methods.
  • All of these contract interactions manifest as eth_call requests.

• Here is how Mobile uses ethjs-ens:

  • Mobile has a file app/util/ENSUtils.js which imports the ethjs-ens package.
  • The ENS class has two methods which are useful:
    • doENSReverseLookup
    • doENSLookup
  • Each of these functions creates an instance of ENS from ethjs-ens, passing a instance of EthQuery which uses the NetworkController provider to talk to the network. (Hence, all requests that ethjs-ens issue go through our standard middleware stack, including @metamask/json-rpc-engine.)
  • Two methods from ENS are used:
    • reverse
    • lookup
  • Both of these methods are asynchronous, and both doENSReverseLookup and doENSLookup wrap calls in try/catch and then discard errors. This means that the errors we see in Sentry should not be surfacing. Despite this, we are still seeing errors in Sentry.

• Although we see one error, there are actually two different variations of the same error, which we can see on the Sentry page under Contexts and then Error:

  • "invalid argument 0: hex string has length 0, want 40 for common.Address"
    • This appears to be happening because the to address that we are passing to the eth_call request is 0x.
    • ethjs-ens uses a getNameHash method to convert an ENS name to a hash which it then passes to the Resolver contract. This method may return 0x.
    • The lookup method in ethjs-ens accounts for this happening and bails early. However, the reverse method (and in fact, all other uses of getNameHash) do not account for this. So it is possible that in some cases we are passing 0x to the contract. We could try adding more logic to bail early in the case of 0x and seeing if it reduces the number of errors we see in Sentry.
  • "TypeError: Network request failed" / "execution reverted" / "Internal JSON-RPC error." with no data
    • These are distinct errors, but is it possible that these are temporary? If so I am curious whether it is worth it to retry ENS requests in case of an error?

---

In summary, I am unsure what we can do about this issue, but perhaps there are some things we can try.

Strange. Maybe attempting a local reproduction would be a good next step? In the ""invalid argument 0: hex string has length 0, want 40 for common.Address"" section, it seems like you've established fairly concretely how this could happen.

Sentry Issue: METAMASK-MOBILE-2ZG2

@Gudahtt Any updates on this? Would be nice if we could move it out of being registered as an error by Sentry, especially if the incidents are not caused by anything in our app

If this should not be added to Sentry, add it to https://github.com/MetaMask/MetaMask-planning/issues/3861