Gas limit estimation for smart accounts

[compojoom]

Hey @ricmoo

I'm trying to improve the gas limit estimation at safe: https://github.com/safe-global/safe-wallet-web/blob/6e294f31c7d27838eddae7884d711eb00b8cdb27/src/hooks/useGasLimit.ts#L165

Currently we just do estimateGas, but as you know we also need to get the gas for the L1 data. ext-utils-optimism was recommended in the optimism docs and I gave it a try, but I ran into some small problems. Since we have a contract account we estimate the gas by encoding the actual contract call and faking a tx.

    const encodedSafeTx = getEncodedSafeTx(
      safeSDK,
      safeTx,
      isOwner ? walletAddress : undefined,
      safeTx.signatures.size < threshold,
    )


const tx = {
        to: safeAddress,
        from: walletAddress,
        data: encodedSafeTx,
      }

I tried passing this tx to the estimageGas function, but then ran into the following error:
TypeError: unsigned transaction cannot define from -> this comes form the

  const txObj = Transaction.from(<TransactionLike<string>>tx);

call. To go around it, I removed the from field form the tx, but then I ran into:
Error: execution reverted: "GS025" (action="estimateGas", which obviously comes from the smart contract GS025: Hash has not been approved
this is basically now due to the txObj.gasLimit = await provider.estimateGas(tx); call. We need a from there. So after deleting the from for the Transaction.from we need to re-add it.

here are the changes I had to do to get it to work:

export async function estimateGas(_tx: TransactionRequest, _provider?: string | Provider): Promise<GasResult> {
  const { contract, provider } = await getPriceOracle(_provider);

  const tx = copyRequest(_tx);
  tx.type = 2;

  const { to, from } = await resolveProperties({
    to: (tx.to ? resolveAddress(tx.to, provider): undefined),
    from: (tx.from ? resolveAddress(tx.from, provider): undefined)
  });

  if (to != null) { tx.to = to; }
  if (from != null) { tx.from = from; }

  const myfrom = tx.from
  delete tx.from
  const txObj = Transaction.from(<TransactionLike<string>>tx);

  tx.from = myfrom
  // Unsigned transactions need a dummy signature added to correctly
  // simulate the length, but things like nonce could still cause a
  // discrepency. It is recommended passing in a fully populated
  // transaction.
  if (txObj.signature == null) {
    txObj.signature = {
      r: fullBytes32, s: fullBytes32, yParity: 1
    };
  }

  // Get the L2 gas limit (if not present)
  if (_tx.gasLimit == null) {
    txObj.gasLimit = await provider.estimateGas(tx);
  }
  const gasL2 = txObj.gasLimit;

  // Compute the sign of the serialized transaction
  const dataL1 = txObj.serialized;

  // Allow overriding the blockTag
  const options: Overrides = { };
  if (_tx.blockTag) { options.blockTag = _tx.blockTag; }

  // Compute the L1 gas
  const gasL1 = await contract.getL1GasUsed(dataL1, options);

  return { gas: (gasL1 + gasL2), gasL1, gasL2 };
}

Maybe I'm doing something wrong, but I would expect that if our approach worked fine with provider.estimateGas, that switching from provider.estimateGas to this estimateGas function would work out of the box?

[compojoom]

I've been looking a bit more into the code and it is somewhat contradictory.
_tx is set to a TransactionRequest
tx ends up a PreparedTransactionRequest

note that both of those types don't have a signature. They might have a from though.

We call
const txObj = Transaction.from(<TransactionLike<string>>tx)
which would fail if it has a from, but no signature. We explicitly make sure that we set a tx.from = from if from was resolved to something that is not null.
This is strange. Apparently we have to pass a transaction without a from to Transaction.from. We later actually need a from for provider.estimateGas