Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simulateTransaction Function for Static Safe Transaction Simulation #788

Open
mshakeg opened this issue Jul 8, 2024 · 13 comments · May be fixed by #789
Open

Add simulateTransaction Function for Static Safe Transaction Simulation #788

mshakeg opened this issue Jul 8, 2024 · 13 comments · May be fixed by #789

Comments

@mshakeg
Copy link

mshakeg commented Jul 8, 2024

Context / issue

IMU Safe contract does not currently have the functionality to simulate a safe transaction without requiring the threshold of owner signatures. This limitation makes it difficult to estimate gas usage or verify if a transaction would succeed without actually executing it. As a result, users and developers face challenges in performing pre-execution checks especially on chains not supported by Tenderly(not too sure how tenderly gets around this issue).

Proposed solution

To address this issue, I propose adding a new function, simulateTransaction, to the Safe contract. This function will allow for the simulation of transactions in a secure manner without requiring the threshold of owner signatures. To enable this the Safe will have to delegatecall to a new contract that does Executor.execute but immediately afterwards reverts to undo any state changes. This feature will enable users to eth_call the simulateTransaction function to obtain transaction success status, gas used, and the transaction hash, enhancing the ability to perform pre-execution checks. This external contract should be immutably configured somewhere, perhaps singularly in the Safe implementation contract.

Alternatives

Do what Tenderly does, though not sure what they do and if it's generally achievable across EVM chains, if so, then ideally this simulation should be exposed via a function in the ts sdk.

@mshakeg mshakeg linked a pull request Jul 8, 2024 that will close this issue
@mmv08
Copy link
Member

mmv08 commented Jul 8, 2024

Do what Tenderly does, though not sure what they do and if it's generally achievable across EVM chains

We use state overrides on Tenderly, it should be also available in many ethereum nodes by default. Check out the stateDiff parameter here: https://geth.ethereum.org/docs/interacting-with-geth/rpc/ns-eth#eth-call. We use it to override the threshold to 1 + use a pre-approved signature for simulation

@nlordell
Copy link
Collaborator

nlordell commented Jul 9, 2024

I also think that there is a SimulateTxAccessor that can be used to estimate executing transactions from a Safe without checking signatures.

I think this already can be used with eth_call quite conveniently, especially with the simulate function provided by the compatibility fallback handler.

const safe = new ethers.Contract(
  SAFE_ADDRESS,
  ["function simulate(address, bytes) view returns (bytes)"],
  provider,
);
const accessor = new ethers.Contract(
  // NOTE: address may vary per network, use `safe-deployments` package to get
  // correct address.
  "0x59AD6735bCd8152B84860Cb256dD9e96b85F69Da",
  ["function simulate(address, uint256, bytes, uint8) returns (uint256, bool, bytes)"],
  provider,
);

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.bytes, tx.operation]),
);
const [gasEstimate, success, returnData] = accessor.interface.decodeFunctionResult("simulate", data);

console.log({
  gasEstimate,
  success,
  returnData,
});

Is there some additional feature that is missing here?

@mmv08
Copy link
Member

mmv08 commented Jul 9, 2024

I also think that there is a SimulateTxAccessor that can be used to estimate executing transactions from a Safe without checking signatures.

I think this already can be used with eth_call quite conveniently, especially with the simulate function provided by the compatibility fallback handler.

const safe = new ethers.Contract(
  SAFE_ADDRESS,
  ["function simulate(address, bytes) view returns (bytes)"],
  provider,
);
const accessor = new ethers.Contract(
  // NOTE: address may vary per network, use `safe-deployments` package to get
  // correct address.
  "0x59AD6735bCd8152B84860Cb256dD9e96b85F69Da",
  ["function simulate(address, uint256, bytes, uint8) returns (uint256, bool, bytes)"],
  provider,
);

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.bytes, tx.operation]),
);
const [gasEstimate, success, returnData] = accessor.interface.decodeFunctionResult("simulate", data);

console.log({
  gasEstimate,
  success,
  returnData,
});

Is there some additional feature that is missing here?

I think the largest thing missing is that this simulation doesn't take into account the guard.

EDIT: Well, I think it's possible to orchestrate multiple calls to different contracts to get a proper simulation, but yeah, I think the initial contract makes sense for more efficiency

@mshakeg
Copy link
Author

mshakeg commented Jul 9, 2024

@nlordell I've actually already made a change to my PoC to simulate safe transactions using the method you described yesterday(see the simulateTx ts function), however it doesn't seem to work when the safeTx involves a native transfer as stated here

@mmv08 the guard would be a pretty major part of getting a more accurate simulation, but I think it's worth adding a standard periphery contract that returns a gas estimate for the safeTx if y'all don't want to make any changes to core. If there isn't a periphery repo should I added the periphery contract here, say under ./contracts/periphery/Simulator.sol

@mmv08
Copy link
Member

mmv08 commented Jul 9, 2024

@mmv08 the guard would be a pretty major part of getting a more accurate simulation, but I think it's worth adding a standard periphery contract that returns a gas estimate for the safeTx if y'all don't want to make any changes to core. If there isn't a periphery repo should I added the periphery contract here, say under ./contracts/periphery/Simulator.sol

What gas are you referring to? Safe transaction has two gas parameters: dataGas (everything outside of the internal call: signature verification, calldata gas costs, etc) and safeTxGas (internal call gas). Both can be estimated without any additional contracts. For the safeTxGas you can use the simulateTxAccessor contract. For data gas you can calculate the tx broadcast cost (21k, calldata costs, signature verification costs, memory expansion etc) + add a pre-defined buffer value

@mmv08
Copy link
Member

mmv08 commented Jul 9, 2024

Safe team ran a relayer service before, you can check its code to see how it was done: https://github.com/5afe/safe-relay-service

@nlordell
Copy link
Collaborator

nlordell commented Jul 9, 2024

however it doesn't seem to work when the safeTx involves a native transfer

Weird, I just ran my snippet with the following modification and it worked:

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [
    tx.to,
    ethers.parseEther("0.001"), // native transfer amount
    tx.bytes,
    tx.operation,
  ]),
);

Does your Safe have enough funds for the native transfer? If not, I would expect it to revert.

@mshakeg
Copy link
Author

mshakeg commented Jul 9, 2024

@nlordell I mean the slightly modified test cases that do simulateTx fail when native transfers are involved.

https://github.com/safe-global/safe-smart-account/pull/789/files#diff-291a4add941d879c99b79d09eed2c4206c6f23edd4f5edbc93d2bdd18e1c91de

@mshakeg
Copy link
Author

mshakeg commented Jul 9, 2024

@mmv08 ah yes, I didn't notice that SimulateTxAccessor.simulate(...) also returns the safeTxGas

@mshakeg
Copy link
Author

mshakeg commented Jul 9, 2024

@nlordell so took another look and the issue with the simulation success mismatch is not actually on safeTxs that transfer native, rather there's a mismatch with the simulation returning success = true instead of false since the other validation checks(e.g. gasPrice) done in execTransaction aren't done in simulateAndRevert.

@nlordell
Copy link
Collaborator

Ah OK - so you want the simulate function to also simulate the relayer refunding code?

@mshakeg
Copy link
Author

mshakeg commented Jul 10, 2024

@nlordell more or less, simulate should simulate the entire execTransaction except for signature validation, basically want to know if whether a safeTx is successful or not without requiring any owner signature.

@nlordell
Copy link
Collaborator

Ok, I see. Yes this isn't directly supported at the moment, but can be "hacked" together with the current contracts (depending on the urgency for getting this to work). You could simulate a multisend which internally sets approved hashes for each owner, then call execTransaction using approved hash signatures. Not pretty but should work.

In general, not against adding an additional accessor to simulate this kind of thing (but it shouldn't change the original contracts IMO).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants