web3-onboard icon indicating copy to clipboard operation
web3-onboard copied to clipboard

[Bug]: Gnosis Safe provider does not pick gas limit nor reports errors

Open enzoferey opened this issue 3 years ago • 2 comments
trafficstars

Current Behavior

When using the Gnosis Safe provider, the app does not pick the predefined gas limit for the transaction nor it throws an error when the execution of the transaction fails. It didn't use to be the case a few months ago.

Expected Behavior

I expect the Safe App to pick the predefined gas limit I have computed for that transaction and to throw an error whenever the transaction execution fails.

Steps To Reproduce

  1. I'm using:
"@web3-onboard/react": "2.3.2",
"@web3-onboard/gnosis": "^2.1.2",
  1. Go to https://gnosis-safe.io/ and create a safe for your app.
  2. Then connect using the @web3-onboard/gnosis provider.
  3. Start a transaction.
  4. Notice the gas limit is not being picked up by the value you have computed before sending the transaction to the provider.

What package is effected by this issue?

@web3-onboard/gnosis

Is this a build or a runtime issue?

Runtime

Package Version

2

Node Version

No response

What browsers are you seeing the problem on?

Chrome

Relevant log output

No response

Anything else?

The Safe App does report in the console the errors, however those are not surfaced to the application. I'm using ethers@^5.7.0 and calling await transaction.wait() but the promise never resolves nor rejects.

Sanity Check

  • [X] If this is a build issue, I have included my build config. If this is a runtime issue, I have included reproduction steps and/or a Minimal, Reproducible Example.

enzoferey avatar Sep 07 '22 07:09 enzoferey

@enzoferey Thanks for raising this issue! We'll take a look this week and see what the issue is!

taylorjdawson avatar Sep 12 '22 19:09 taylorjdawson

@taylorjdawson any updates ?

enzoferey avatar Sep 19 '22 12:09 enzoferey

Hey! This is an issue for me as well. Can anyone please take a look at this and/or report on the status? Is this planned?

rufuszero avatar Oct 07 '22 11:10 rufuszero

We are going to take a look at this issue this week, we had a bit of a backlog to get through first.

lnbc1QWFyb24 avatar Oct 10 '22 01:10 lnbc1QWFyb24

@enzoferey @RufusZero can you send over the calls you are using to send the transaction? Can you also share the console errors you are seeing?

Adamj1232 avatar Oct 13 '22 20:10 Adamj1232

Hey @Adamj1232, here is a summary of what we are doing to generate the transaction (simplified React code, it won't work if you copy paste because of lifecycles):

// 1. We get a contract instance using Ethers and the provider obtained from @web3-onboard
const [{ wallet}] = useConnectWallet();
const connectedWalletProvider = new ethers.providers.Web3Provider(
    wallet.provider
);
const signer = connectedWalletProvider.getSigner();
const contractInstance = new ethers.Contract(
    contractAddress,
    abi,
    signer
);

// 2. We populate the transaction setting a gas limit explicitly
const populatedTransaction = contractInstance.populateTransaction.someFunction(
    someArg1,
    someArg2,
    {
      gasLimit: 400000,
    }
);

// 3. We send the populated transaction via the signer and wait on it
const transaction = await signer.sendTransaction(populatedTransaction);
await transaction.wait();

After running that, see the gas limit is not being set to the amount specified:

Screenshot 2022-10-14 at 11 04 24

Then, about the transaction failure not being reported. If I execute the same transaction but set the gas limit low enough to force the transaction to fail, I see these errors in the console but the transaction.wait() call never resolves:

Screenshot 2022-10-14 at 12 17 43

You can see the transaction failed on MetaMask:

Screenshot 2022-10-14 at 12 18 24

And here is the transaction on explorer.

While trying this, I have also found that if I set the Nonce value on the Advanced Parameters dialog, the transaction prompt I'm getting on the wallet does not use it but rather uses the next available one:

Screenshot 2022-10-14 at 11 25 50 Screenshot 2022-10-14 at 11 26 47

enzoferey avatar Oct 14 '22 10:10 enzoferey

@enzoferey we have started a conversation with the Gnosis team around this as we are seeing the same behavior even with the usage of their SDK gas setting (safeTxGas) instance which is exposed through the gnosis wallet W3O instance. More info available here under the Sending TXs section. I will update here as we make progress.

Adamj1232 avatar Oct 17 '22 18:10 Adamj1232

Thanks for the update @Adamj1232 👍🏻

enzoferey avatar Oct 17 '22 18:10 enzoferey

@enzoferey upon more discussion with the Gnosis team it seems gas estimation is send by their SDK and handled within the Safe. The Gnosis sdk instance exposed by the web3-onboard must be used to set the safeTxGas prop and send the transaction. The behavior of safeTxGas depends on the gasPrice value of the Safe transaction. Check Gnosis docs on this for full detail as it is a bit confusing. An example of accessing the Gnosis SDK instance and sending a transaction can be found below.

    const tx = {
      to: toAddress,
      value: 1000000000000000,
      data: '0x',
    }
    const params = {
      safeTxGas: 5000000,
    };
    let trans = await wallet.instance.txs.send({txs:[tx], params})

Note: With the safeTxGas you will see additional value on the gasLimit displayed in the Safe. Check gnosis docs for full details there. I will update with any further information on gasLimit/safeTxGas

On the nonce issue, you may want to present that within the Gnosis Safe GH as an issue as we do not have control over that. The transaction handler in W3O accepts and sends the transaction provided by Gnosis Safe.

You may also want to checkout the Gnosis Stack Exchange for issues and support around the Safe app

Adamj1232 avatar Oct 20 '22 18:10 Adamj1232

@enzoferey can you detail the steps to create a failing transaction? I am not able to reproduce through the Safe app within a browser.

Adamj1232 avatar Oct 20 '22 18:10 Adamj1232

Hey, dev from the Safe team here. We do not support the ethereum transaction gasLimit (or, in fact, any other parameters for the ethereum transaction). The wallet will determine them at the time of the transaction execution.

mmv08 avatar Oct 21 '22 11:10 mmv08

Hi @mikhailxyz, thanks for jumping in. Why are you not supporting any Ethereum transaction parameter ? Isn't that a big blocker for Safe apps development ?

@mikhailxyz If I understood well enough, you do have a dedicated safeTxGas field that behaves a bit differently than gasLimit that may allow playing around the gas limit issue. Is that correct ?

@Adamj1232 Would be setting the safeTxGas field on the web3-onboard provider propagate to the Safe SDK as of today ?

@Adamj1232 To reproduce a failing transaction:

  1. Take any transaction
  2. Reduce its gas limit parameter on the Safe app dialog that opens up before asking for the wallet signature
  3. Ensure pressing the "simulate" button tells you the transaction will fail
  4. Sign the transaction on your wallet
  5. It will revert because of the lack of gas and you will see the error is not being picked up by the web3-onboard provider

enzoferey avatar Oct 21 '22 12:10 enzoferey

Hi @mikhailxyz, thanks for jumping in. Why are you not supporting any Ethereum transaction parameter ? Isn't that a big blocker for Safe apps development ?

Why would it be a big blocker?

@mikhailxyz If I understood well enough, you do have a dedicated safeTxGas field that behaves a bit differently than gasLimit that may allow playing around the gas limit issue. Is that correct ?

Yes, safeTxGas is the minimum gas that has to be supplied to the safe transaction.

mmv08 avatar Oct 21 '22 12:10 mmv08

Why would it be a big blocker?

Well, if you want to set some values on the transaction like accurate gas prices or wants to overwrite some previous transaction via the nonce value, you simply cannot.

enzoferey avatar Oct 21 '22 12:10 enzoferey

Why would it be a big blocker?

Well, if you want to set some values on the transaction like accurate gas prices or wants to overwrite some previous transaction via the nonce value, you simply cannot.

The wallet can calculate the gas limit, there's no justified reason to set it

mmv08 avatar Oct 21 '22 12:10 mmv08

Wallet estimations on gas limit are way too often wrong. Gas limit cannot easily be computed via the classic approach when you are batching transactions and you have several proxy levels of delegate calls. Also, the gas limit of a certain operation might depend on the state of the contract and in those cases you want to provide a higher gas limit to protect yourself against front running or activity spikes. Having a "minimum gas spent" approach is the opposite way of how it has been designed at the chain level and I believe should not be the preferred way for setting a transaction gas limit. It is useful in certain scenarios, but it should not be the only way.

Wallet estimations on gas price are also way too often wrong. They would be computed based on some unique algorithm defined by the wallet which some times has flaws itself or just does not fit your dApp needs. That's where providing suggestions makes a lot of sense so users can have a better experience.

enzoferey avatar Oct 21 '22 12:10 enzoferey

Wallet estimations on gas limit are way too often wrong. Gas limit cannot easily be computed via the classic approach when you are batching transactions and you have several proxy levels of delegate calls. Also, the gas limit of a certain operation might depend on the state of the contract and in those cases you want to provide a higher gas limit to protect yourself against front running or activity spikes. Having a "minimum gas spent" approach is the opposite way of how it has been designed at the chain level and I believe should not the preferred way for setting a transaction gas limit. It is useful in certain scenarios, but it should not be the only way.

Wallet estimations on gas price are also way too often wrong. They would be computed based on some unique algorithm defined by the wallet which some times has flaws itself or just does not fit your dApp needs. That's where providing suggestions makes a lot of sense so users can have a better experience.

I (somewhat) understand you, but I can say the same thing: dApp estimations on gas prices and gas limits are also way too often wrong. Especially for smart contract wallets

mmv08 avatar Oct 21 '22 13:10 mmv08

@mikhailxyz thank you for chiming in! I am trying to catch the 803 event(or the error message more closely related to the error occurring) from the Safe to pass along through the provider. The error seems to always be converted to Error: Transaction was rejected even if the transaction was canceled in the wallet or there was a gas error. Do you know if there is way to get the intrinsic gas too low error message from the Safe so it can be passed through the Web3-Onboard package? It seems in this case the MetaMask error is being caught by the Safe and not passed. Screen Shot 2022-10-21 at 10 43 15 AM

Adamj1232 avatar Oct 21 '22 16:10 Adamj1232

@mikhailxyz thank you for chiming in! I am trying to catch the 803 event(or the error message more closely related to the error occurring) from the Safe to pass along through the provider. The error seems to always be converted to Error: Transaction was rejected even if the transaction was canceled in the wallet or there was a gas error. Do you know if there is way to get the intrinsic gas too low error message from the Safe so it can be passed through the Web3-Onboard package? It seems in this case the MetaMask error is being caught by the Safe and not passed. Screen Shot 2022-10-21 at 10 25 24 AM

I'm not sure what's the problem here, the error happened after the "transaction was rejected" error that was returned by the SDK. So why would an error happening later be passed? Perhaps I do not understand the problem very well.

mmv08 avatar Oct 21 '22 16:10 mmv08

@mikhailxyz thank you for chiming in! I am trying to catch the 803 event(or the error message more closely related to the error occurring) from the Safe to pass along through the provider. The error seems to always be converted to Error: Transaction was rejected even if the transaction was canceled in the wallet or there was a gas error. Do you know if there is way to get the intrinsic gas too low error message from the Safe so it can be passed through the Web3-Onboard package? It seems in this case the MetaMask error is being caught by the Safe and not passed. Screen Shot 2022-10-21 at 10 25 24 AM

I'm not sure what's the problem here, the error happened after the "transaction was rejected" error that was returned by the SDK. So why would an error happening later be passed? Perhaps I do not understand the problem very well.

Apologies sent, the wrong screenshot. The error occurs before the SDK return the Transaction was rejected after the intrinsic gas too low error but that verbiage isn't passed along with the error object. Screen Shot 2022-10-21 at 10 43 15 AM Code producing err for context. Happens both with SDK.txs.send and using the bare with eth_sendTransaction

eth_sendTransaction: async ({baseRequest, txs}) => {
  try {
    const transactionHash = await appsSdk.txs.send({txs})
    return transactionHash
  } catch (e) {
    console.log(e)
  }
}

Adamj1232 avatar Oct 21 '22 16:10 Adamj1232

I (somewhat) understand you, but I can say the same thing: dApp estimations on gas prices and gas limits are also way too often wrong. Especially for smart contract wallets

@mikhailxyz Agreed too, OpenSea being one of them as an example. The difference is that you are leaving place for dApp to make their own judgements and go for your recommendations or stick with theirs. As of right now, dApps are forced to follow your recommendations, which fall short for our business as an example. Hope this conversation makes you reconsider this topic or at least start a conversation about it internally. Thank you again for your help here 😁

enzoferey avatar Oct 21 '22 17:10 enzoferey

I (somewhat) understand you, but I can say the same thing: dApp estimations on gas prices and gas limits are also way too often wrong. Especially for smart contract wallets

@mikhailxyz Agreed too, OpenSea being one of them as an example. The difference is that you are leaving place for dApp to make their own judgements and go for your recommendations or stick with theirs. As of right now, dApps are forced to follow your recommendations, which fall short for our business as an example. Hope this conversation makes you reconsider this topic or at least start a conversation about it internally. Thank you again for your help here 😁

Could you please share an example of a transaction where Safe fails to estimate gas? We'd like to fix our estimation in the first place.

mmv08 avatar Oct 22 '22 08:10 mmv08

@mikhailxyz thank you for chiming in! I am trying to catch the 803 event(or the error message more closely related to the error occurring) from the Safe to pass along through the provider. The error seems to always be converted to Error: Transaction was rejected even if the transaction was canceled in the wallet or there was a gas error. Do you know if there is way to get the intrinsic gas too low error message from the Safe so it can be passed through the Web3-Onboard package? It seems in this case the MetaMask error is being caught by the Safe and not passed. Screen Shot 2022-10-21 at 10 25 24 AM

I'm not sure what's the problem here, the error happened after the "transaction was rejected" error that was returned by the SDK. So why would an error happening later be passed? Perhaps I do not understand the problem very well.

Apologies sent, the wrong screenshot. The error occurs before the SDK return the Transaction was rejected after the intrinsic gas too low error but that verbiage isn't passed along with the error object. Screen Shot 2022-10-21 at 10 43 15 AM Code producing err for context. Happens both with SDK.txs.send and using the bare with eth_sendTransaction

eth_sendTransaction: async ({baseRequest, txs}) => {
  try {
    const transactionHash = await appsSdk.txs.send({txs})
    return transactionHash
  } catch (e) {
    console.log(e)
  }
}

We'll check this. Agree the error message could be improved

mmv08 avatar Oct 22 '22 09:10 mmv08

Could you please share an example of a transaction where Safe fails to estimate gas? We'd like to fix our estimation in the first place.

Blocknative has a Simulation platform/endpoint with a really solid record, if interested let me know @mikhailxyz

Adamj1232 avatar Oct 24 '22 22:10 Adamj1232

@mikhailxyz thank you for chiming in! I am trying to catch the 803 event(or the error message more closely related to the error occurring) from the Safe to pass along through the provider. The error seems to always be converted to Error: Transaction was rejected even if the transaction was canceled in the wallet or there was a gas error. Do you know if there is way to get the intrinsic gas too low error message from the Safe so it can be passed through the Web3-Onboard package? It seems in this case the MetaMask error is being caught by the Safe and not passed. Screen Shot 2022-10-21 at 10 25 24 AM

I'm not sure what's the problem here, the error happened after the "transaction was rejected" error that was returned by the SDK. So why would an error happening later be passed? Perhaps I do not understand the problem very well.

Apologies sent, the wrong screenshot. The error occurs before the SDK return the Transaction was rejected after the intrinsic gas too low error but that verbiage isn't passed along with the error object. Screen Shot 2022-10-21 at 10 43 15 AM Code producing err for context. Happens both with SDK.txs.send and using the bare with eth_sendTransaction

eth_sendTransaction: async ({baseRequest, txs}) => {
  try {
    const transactionHash = await appsSdk.txs.send({txs})
    return transactionHash
  } catch (e) {
    console.log(e)
  }
}

I've just checked this, and everything looks fine on our end. Gas estimation failure doesn't reject the transaction, and the user still can put a custom gas limit and execute the transaction.

mmv08 avatar Oct 27 '22 16:10 mmv08

@mikhailxyz ah ok, it is strange that the Code 803 is not surfaced through the catch. I would expect the 803 code to be propagated. I have updated our documentation around gas usage and pointing to your docs. I will close for now. Thank you for looking.

Adamj1232 avatar Oct 28 '22 15:10 Adamj1232

@mikhailxyz ah ok, it is strange that the Code 803 is not surfaced through the catch. I would expect the 803 code to be propagated. I have updated our documentation around gas usage and pointing to your docs. I will close for now. Thank you for looking.

Why would it be?🤔 the error doesn't stop the transaction sending process. You can still execute the transaction

mmv08 avatar Oct 28 '22 22:10 mmv08

Great you updated the docs @Adamj1232 👌🏻

What about transaction execution failures not being propagated to the web3-onboard Gnosis provider ? I don't think your back and forth these past few days have come to any result. As @mikhailxyz said you should be able to execute the transaction, but if it ends up failing for whatever reason during execution the web3-onboard Gnosis provider should be throwing while awaiting for the transaction minting.

enzoferey avatar Oct 29 '22 21:10 enzoferey

You are correct @enzoferey apologies for closing the issue before fully resolving the error handling. Please checkout the video and errors noted below and confirm if this is the proper flow along with the same errors you are seeing(and needing to handle). @mikhailxyz this is the transaction submission flow noted above that is not properly propagating the error out of the Web3-Onboard provider or the Gnosis SDK. Both flows are shown in the video.

@Adamj1232 To reproduce a failing transaction:

  1. Take any transaction
  2. Reduce its gas limit parameter on the Safe app dialog that opens up before asking for the wallet signature
  3. Ensure pressing the "simulate" button tells you the transaction will fail
  4. Sign the transaction on your wallet
  5. It will revert because of the lack of gas and you will see the error is not being picked up by the web3-onboard provider

The errors in the console can be seen in the screenshot below. I have also recorded a video demonstrating the issue and where the error gets hung up. If you see issue with the flow please let me know so we can document or handle properly within Web3-Onboard. Thanks again!

Video link : https://www.loom.com/share/84695cdadb764c4888d5c7caec48f3d7

Screen Shot 2022-10-31 at 10 27 33 AM

Adamj1232 avatar Oct 31 '22 16:10 Adamj1232

You are correct @enzoferey apologies for closing the issue before fully resolving the error handling. Please checkout the video and errors noted below and confirm if this is the proper flow along with the same errors you are seeing(and needing to handle). @mikhailxyz this is the transaction submission flow noted above that is not properly propagating the error out of the Web3-Onboard provider or the Gnosis SDK. Both flows are shown in the video.

@Adamj1232 To reproduce a failing transaction:

  1. Take any transaction
  2. Reduce its gas limit parameter on the Safe app dialog that opens up before asking for the wallet signature
  3. Ensure pressing the "simulate" button tells you the transaction will fail
  4. Sign the transaction on your wallet
  5. It will revert because of the lack of gas and you will see the error is not being picked up by the web3-onboard provider

The errors in the console can be seen in the screenshot below. I have also recorded a video demonstrating the issue and where the error gets hung up. If you see issue with the flow please let me know so we can document or handle properly within Web3-Onboard. Thanks again!

Video link : https://www.loom.com/share/84695cdadb764c4888d5c7caec48f3d7

Screen Shot 2022-10-31 at 10 27 33 AM

This is currently expected due to Safe's asynchronous nature. With safe the eth_sendTransaction is not guaranteed to execute a transaction because it might need additional approvals which can take hours, days, weeks, etc. Instead, the eth_sendTransaction method returns a safe transaction hash which one can use to track the transaction's status using safe transaction service.

mmv08 avatar Oct 31 '22 18:10 mmv08