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

(Feature) Refactor gas calculation#1756

Merged
dasanra merged 29 commits intofeature/1048-tx-will-fail-warningfrom
feature/refactor-gas-calculation
Jan 11, 2021
Merged

(Feature) Refactor gas calculation#1756
dasanra merged 29 commits intofeature/1048-tx-will-fail-warningfrom
feature/refactor-gas-calculation

Conversation

@Agupane
Copy link
Contributor

@Agupane Agupane commented Jan 6, 2021

Closes #1752
Depends on #1675

@Agupane Agupane self-assigned this Jan 6, 2021
@github-actions
Copy link

github-actions bot commented Jan 6, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@Agupane Agupane changed the base branch from development to feature/1048-tx-will-fail-warning January 6, 2021 16:03
@github-actions
Copy link

github-actions bot commented Jan 6, 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 3 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@Agupane
Copy link
Contributor Author

Agupane commented Jan 6, 2021

Hey @rmeissner we have some questions about the signatures in order to make a call to execTransaction. We are getting this error: execution reverted: Invalid owner provided as far as we understood the contract code seems that is related with the signatures.

We have this code for estimating the gas (you can check the complete function here):

export const estimateGasForTransactionExecution = async ({
  safeAddress,
  txRecipient,
  txConfirmations,
  txAmount = '0',
  txData,
  operation = CALL,
  from,
  gasPrice = '0',
  gasToken = ZERO_ADDRESS,
  refundReceiver = ZERO_ADDRESS,
  safeTxGas = '0',
}: TransactionExecutionEstimationProps): Promise<number> => {
  const safeInstance = await getGnosisSafeInstanceAt(safeAddress)

  const sigs = generateSignaturesFromTxConfirmations(txConfirmations)

  const executeTransactionTxData = await safeInstance.methods
    .execTransaction(
      txRecipient,
      txAmount as string,
      txData,
      operation as number,
      safeTxGas as string,
      0,
      gasPrice as string,
      gasToken as string,
      refundReceiver as string,
      sigs,
    )
    .encodeABI()

  const gasEstimation = await calculateGasOf(executeTransactionTxData, from, safeAddress)

  const gasBatches = [0, 10000, 20000, 40000, 80000, 160000, 320000, 640000, 1280000, 2560000, 5120000]
    .filter((currentGas) => currentGas < gasEstimation)
    // Reorders gas from lowest to highest
    .sort((a, b) => b - a)

  for (const baseGasIterator of gasBatches) {
    const executeTransactionGasCheck = await safeInstance.methods
      .execTransaction(
        txRecipient,
        txAmount as string,
        txData,
        operation as number,
        safeTxGas as string,
        gasEstimation,
        gasPrice as string,
        gasToken as string,
        refundReceiver as string,
        sigs,
      )
      .call()

    if (executeTransactionGasCheck) {
      return baseGasIterator
    }
  }

  // In there is no gasBatches available that could run successfully execTransaction we need to inform the user
  throw new Error('There was no valid value of gas to execute the transaction, the transaction may fail')
}

We are getting that error here:
const gasEstimation = await calculateGasOf(executeTransactionTxData, from, safeAddress)

calculateGasOf will basically do a web3.eth.estimateGas

The signatures that we provided are these ones:
0xed78a7e6ed6c606df81aadaa4da8e9dc59ef6be2dd4baca855305f835f4bffd67007e0ad99128c911f6bfbb9e31c231a63e80c527975b9e94c4526822ec2b1251b55149db32f9bca5e47ba3ec049172e1d03b284b9dcc455c0f0e0c7686bed1d955899b1ec4d7727eb4df73df1031c46aa403239f98fbc2ed9ccfffbd0eb33d2731b

The safe: 0xdCA21b57C5c51dC82a8607ec0F989f5f48975091 (rinkeby) and owners are:
0x0dc0dfD22C6Beab74672EADE5F9Be5234AAa43cC and 0xCC21e6F00036B3E16430e299e200398BD2209aaB

But if we continue executing the transaction, the method processTransaction (here) will do the same calculation, but instead of doing an estimateGas will do a send. The signature value will be the same, but the transaction won't fail.

Do you have any ideas about why this could be happening?

CC: @fernandomg @nicosampler

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 6, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

txConfirmations === threshold || !!preApprovingOwner || threshold === 1

const checkIfTxIsApproveAndExecution = (threshold: number, txConfirmations: number): boolean =>
txConfirmations + 1 === threshold
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure to understand this, are we checking the checkbox status somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed, it's the same logic that uses the checkbox in ApproveTxModal here:

  const oneConfirmationLeft = !thresholdReached && tx.confirmations.size + 1 === threshold
  const isTheTxReadyToBeExecuted = oneConfirmationLeft ? true : thresholdReached

@Agupane Agupane marked this pull request as ready for review January 8, 2021 18:09
}: TransactionExecutionEstimationProps): Promise<number> => {
const safeInstance = await getGnosisSafeInstanceAt(safeAddress)
try {
if (approvalAndExecution) {
Copy link
Contributor Author

@Agupane Agupane Jan 8, 2021

Choose a reason for hiding this comment

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

If we try to use execTransaction .call() to estimate the gas without having all the "confirmed" signatures (no prevalidated with the sentinel) we got an error: Hash has not been approved, so in case that the user wants to do an "approve and execute" we will calculate the gas using the same method as for creation safe.requiredTxGas.

@ghost
Copy link

ghost commented Jan 8, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 8, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

…r-gas-calculation

# Conflicts:
#	src/routes/safe/components/Transactions/TxsTable/ExpandedTx/ApproveTxModal/index.tsx
@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1756--safereact.review.gnosisdev.com/volta/app

@dasanra dasanra merged commit 0f3b305 into feature/1048-tx-will-fail-warning Jan 11, 2021
@dasanra dasanra deleted the feature/refactor-gas-calculation branch January 11, 2021 19:09
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 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.

Refactor of gas calculation

3 participants