GoodDollar / GoodDAPP

GoodDollar.org Wallet is the simplest access point to Claim your daily G$. It Is based on web3 and React native web.
good-dapp.vercel.app
MIT License
101 stars 52 forks source link

Error: _notifyEvents event get/send receipt failed:: Invalid params #4178

Closed sentry-io[bot] closed 4 months ago

sentry-io[bot] commented 6 months ago

It looks like we are passing a non tx hash to fetching receipt or an empty hash Invalid params: invalid type: null, expected a 0x-prefixed hex string with length of 64 and invalid characters encountered in Hex string

Can it be that we are passing non TXs here? like feed? for some reason tx hash is empty? It would be good to add the txhash as a parameter in the error log

Sentry Issue: GOODDAPP-19ZYR

Error: _notifyEvents event get/send receipt failed:: Invalid params
Incorrect parameters count, expected: 1, actual: 0
  at <anonymous> (lib/wallet/MultipleHttpProvider.js:54:7)
L03TJ3 commented 5 months ago

@sirpy @johnsmith-gooddollar question was: Can it be that we are passing non TXs here? like feed? for some reason tx hash is empty?

I have been going through the flow to see what could be causing the null txHash.

The flow:

  1. we only run _notifyEvents when syncing/fetching the feed which only happens on Dashboard (right?)
  2. this can be done from either syncTxFromExplorer/syncTxWithBlockchain
  3. any event or transaction, any api call we do should have a txHash
  4. we set txHash always to null when we cannot get the receipt for it here: https://github.com/GoodDollar/GoodDAPP/blob/04b2bed1655932add667df9096acc3e73ed6502b/src/lib/wallet/GoodWalletClass.js#L675

Additional notes: So from some of the sentry reports you can see the replay where this is triggered by users going through the claim flow maybe I am seeing something which is not there, but there could be correlation between 'claim-failed' and above error (close amount of event counts): https://gooddollar.sentry.io/issues/?environment=production&project=1829798&query=claiming+failed&referrer=issue-list&statsPeriod=24h

my guess(es) so far are:

  1. out of sync explorers/local web3 instance. one fetching a txHash which the other cannot find (yet)
  2. there is recurrent error 'already-known' around claiming, could result in a premature redirect back to dashboard screen while claiming was succesfull (but pending)?
  3. There are actually different errors around claiming to be found on sentry, not sure all of them actually cause a failed transaction, could result in a premature redirect back to dashboard screen while claiming was succesfull (but pending)?
  4. could be null txHash for send/receive items which are still pending?

Receipt is only fetched for 'mined' transactions so any pending transactions that we fetch would result in a null txHash when we run getTransactionReceipt.

I am not necessarily sure what the cleanest way to handle this is

sirpy commented 5 months ago

@L03TJ3 The error is because notifyReceipt fails. notifyReceipt fails because the input TX hash is null. the only reason it can be null is that one of the input events to notifyEvents doesnt have the field transactionhash.

  1. If you can find why this happens, i'd suggest adding a filter search for such events and error logging them so we can learn from production.
  2. maybe some events have the field hash and not transactionHash, just a guess. ie go over again the explorer/tatum/blockchain event format/data
L03TJ3 commented 5 months ago

@sirpy I explain why it likely happens. The api calls we do for all transactions and events includes pending 'unmined' transactions', they already have a txHash. Then in notifyReceipt it does the 'getReceiptWithLogs' Receipt is only available after mining! so if there is a pending transaction, it will not find the transactionHash!

  /**
   * @return an existing (non-pending) transaction receipt information + human readable logs of the transaction
   * @param transactionHash The TX hash to return the data for
   */
  async getReceiptWithLogs(transactionHash: string) {
    const chainId = this.networkId
    const transactionReceipt = await retryCall(() => this.wallet.eth.getTransactionReceipt(transactionHash))

    if (!transactionReceipt) {
      return null
    }

    const logs = filter(abiDecoder.decodeLogs(transactionReceipt.logs))

    return { ...transactionReceipt, logs, chainId } //add network id in case of wallet provider network switch
  }

We wrap this with retryCall. it attempts it three times and then throws the error here: https://github.com/GoodDollar/GoodDAPP/blob/04b2bed1655932add667df9096acc3e73ed6502b/src/lib/utils/async.js#L67

Resulting in the catch from _notifyReceipt being triggered, with the value of 'null' (that is what web3.getTransactionReceipt returns for pending transasctions. Ref: https://web3js.readthedocs.io/en/v1.2.11/web3-eth.html#gettransactionreceipt

L03TJ3 commented 5 months ago

So no. the inputs are not the only reason. the api calls are fully aligned with the documentation and all the 'hash' or 'transactionsHash' fields are mapped correctly as far as I could tell

optional alternative fix could be to replace getTransactionReceipt with getTransactionByHash

L03TJ3 commented 5 months ago

As discussed left over todo's are:

  1. increase polling interval to 3+ seconds
  2. potentially add getTransactionByHash for tracking pending transactions
L03TJ3 commented 5 months ago

added log and is available on dev

L03TJ3 commented 5 months ago

@vldkhh verify on prod

L03TJ3 commented 5 months ago

@vldkhh verify on prod

sirpy commented 5 months ago

@L03TJ3 you can already look at errors on sentry and check why it fails. I see that either event is defined well or that fetching the TX from the rpc fails.

It is still strange to me why fetching from rpc fails with so many rpcs and retries... we can also try to add a fallback to fetch TX from the explorers in getReceiptWithLogs to handle the rpc issues

vldkhh commented 5 months ago

@L03TJ3 I'm monitoring this error, need more time

vldkhh commented 4 months ago

8 events per 24h. closed