Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

Conversation

@dasanra
Copy link
Collaborator

@dasanra dasanra commented Mar 9, 2021

Closes #1970 and #1975

We were trusting safeTxGas would be enough or we would should a "transaction would fail" message when all signatures are gathered and only execution is pending.

But safeTxGas 0 means that the operation can use up to all gasLimit if necessary to execute the operation.
In this case we have to assume this is a correct safeTxGas but estimate the value in order to set a correct gasLimit so transaction will be correctly executed

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

console.info(`Estimating transaction success for execution & approval...`)
let gasEstimation
// If safeTxGas === 0 we still have to estimate the gas limit to execute the transaction so we need to get an estimation
if (approvalAndExecution || safeTxGas === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is || safeTxGas === 0 needed?

shouldn't this enter the ìfno matter the safeTxGas value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, I debug it and I understand it now.

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

@github-actions
Copy link

@liliya-soroka liliya-soroka self-assigned this Mar 10, 2021
Copy link
Member

@liliya-soroka liliya-soroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified
Both issues
https://app.zenhub.com/workspaces/safe-multisig---web-5ce554debb310a35b2f8b6f8/issues/gnosis/safe-react/1975 and
https://app.zenhub.com/workspaces/safe-multisig---web-5ce554debb310a35b2f8b6f8/issues/gnosis/safe-react/1970 are fixed
if the transaction has safeTxGas=0 ( created as contract interaction tx or as cancellation tx via mobile) and signed on mobile before execution it's possible to execute such tx without any issues with gas estimation.

@liliya-soroka
Copy link
Member

Verified
Both issues
https://app.zenhub.com/workspaces/safe-multisig---web-5ce554debb310a35b2f8b6f8/issues/gnosis/safe-react/1975 and
https://app.zenhub.com/workspaces/safe-multisig---web-5ce554debb310a35b2f8b6f8/issues/gnosis/safe-react/1970 are fixed
if the transaction has safeTxGas=0 ( created as contract interaction tx or as cancellation tx via mobile) and signed on mobile before execution it's possible to execute such tx without any issues with gas estimation.

@dasanra dasanra merged commit 26de414 into development Mar 10, 2021
@dasanra dasanra deleted the feature/#1975-execute-transactions-safe-tx-gas-0-fail branch March 10, 2021 18:50
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gas limit is set to 21k for the cancellation tx on web if the cancellation tx was created and got last required confirmation on mobile

5 participants