omgnetwork / optimism

Monorepo implementing the Optimistic Ethereum protocol
https://optimism.io
MIT License
16 stars 6 forks source link

Gateway Feature - more informative error messages #526

Closed CAPtheorem closed 3 years ago

CAPtheorem commented 3 years ago

Some transactions are canceled by the user, for example by rejecting signature. In some places, this results in the vague error message 'transaction failed' which is not true since the transaction did not fail, but was rejected, so the message in that specific case should be 'transaction rejected'. Even more confusingly, in other parts of the code, a MetaMask user reject results in a green 'transaction submitted', which is also not useful, since the transaction was ultimately rejected by the user.

ToDo - starting with networkServices.js:

1/ Implement a common system for error reporting to user 2/ Make sure this system make sense of MetaMask errors 3/ Be consistent and correct about what is reported to the user.

CAPtheorem commented 3 years ago

One example for how to do this correctly is the transfer:

 //Transfer funds from one account to another, on the L2
  async transfer(address, value_Wei_String, currency) {
    try {
      //any ERC20 json will do....
      const tx = await this.L2_TEST_Contract.attach(currency).transfer(
        address,
        value_Wei_String
      )
      await tx.wait()
      return tx
    } catch (error) {
      console.log("NS: transfer error:", error)
      return error
    }
  }

and then createAction correctly interprets the returned error via:

//deal with metamask errors
      if(response && response.hasOwnProperty('message') && response.hasOwnProperty('code')) { 
        dispatch({ type: `UI/ERROR/UPDATE`, payload: response.message })
        // cancel request loading state
        dispatch({ type: `${key}/ERROR` })
        return false
      }

however, other functions do not return the error, but return e.g. false instead of the error payload.

And other functions use a second system, throw new WebWalletError:

 async checkAllowance(
    currencyAddress,
    targetContract
  ) {
    try {
      const ERC20Contract = new ethers.Contract(
        currencyAddress,
        L1ERC20Json.abi, //could use any abi - just something with .allowance
        this.provider.getSigner()
      )
      const allowance = await ERC20Contract.allowance(
        this.account,
        targetContract
      )
      return allowance //.toString()
    } catch (error) {
      console.log("NS: checkAllowance error:", error)
      throw new WebWalletError({
        originalError: error,
        customErrorMessage: 'Could not check ERC20 allowance.',
        reportToSentry: false,
        reportToUi: true,
      })
    }
  }
MitchellUnknown commented 3 years ago

Sorry to hijack this one, but maybe add a fourth point. /4 The duration of some messages are very short, some are oké and some messages won't close at all. Would be nice to make the message duration more consistent overall.

For example this message won't disappear/close by itself, when you reject adding liquidity.

image

CAPtheorem commented 3 years ago

resolved by https://github.com/omgnetwork/optimism/pull/531