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

PoC for Safe simulateTransaction function #789

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mshakeg
Copy link

@mshakeg mshakeg commented Jul 8, 2024

This PR introduces a new function, simulateTransaction, to the Safe contract, allowing users to simulate transactions without requiring the threshold of owner signatures. The simulation helps estimate gas usage and verify if a transaction would succeed without actually executing it, thus providing a reliable method for pre-execution checks.

Fixes #788

Copy link

github-actions bot commented Jul 8, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mshakeg
Copy link
Author

mshakeg commented Jul 8, 2024

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Jul 8, 2024
@mshakeg
Copy link
Author

mshakeg commented Jul 8, 2024

recheck

@mmv08
Copy link
Member

mmv08 commented Jul 8, 2024

Hey, thank you for the contribution. this can be an external contract used through

function simulateAndRevert(address targetContract, bytes memory calldataPayload) external {
. We're quite short on the bytecode size, so I'm almost sure we will not include this in the core contract.

@mshakeg
Copy link
Author

mshakeg commented Jul 8, 2024

@mmv08 thanks, so I took a look at linked StorageAccessible. simulateAndRevert section of code and what from I can tell it should work something like:

  1. say I've got a safeTx that I want to simulate on safe where the safeTx is to, value, data and operation
  2. now I can simulate the above safeTx but I don't have any owner signatures
  3. so I call safe.simulateAndRevert(targetContract, calldataPayload)
    • targetContract: this will be the SimulateTxAccessor address
    • calldataPayload: this will be the encoded bytes of SimulateTxAccessor.simulate(to, value, data, operation)
  4. now success can be extracted from the revert message

Is this understanding correct? And if so, does the ts sdk expose a function to do this simulation? Also, since it reverts eth_estimateGas wouldn't return the gasUsed for the simulation safeTx, so that seems to be a deficiency with this method.

@mshakeg
Copy link
Author

mshakeg commented Jul 8, 2024

@mmv08 I added the gasUsed to the revert data. It seems to work fine for most txs, however the simulations for txs that transfer the native asset all fail:

npx hardhat test test/core/Safe.Execution.spec.ts

@mmv08
Copy link
Member

mmv08 commented Jul 8, 2024

@mmv08 thanks, so I took a look at linked StorageAccessible. simulateAndRevert section of code and what from I can tell it should work something like:

  1. say I've got a safeTx that I want to simulate on safe where the safeTx is to, value, data and operation

  2. now I can simulate the above safeTx but I don't have any owner signatures

  3. so I call safe.simulateAndRevert(targetContract, calldataPayload)

    • targetContract: this will be the SimulateTxAccessor address
    • calldataPayload: this will be the encoded bytes of SimulateTxAccessor.simulate(to, value, data, operation)
  4. now success can be extracted from the revert message

Is this understanding correct? And if so, does the ts sdk expose a function to do this simulation? Also, since it reverts eth_estimateGas wouldn't return the gasUsed for the simulation safeTx, so that seems to be a deficiency with this method.

You can make a library that contains a function similar to your original simulateTransaction method, have this function return whatever you want and then use the return data from there. There are no code changes required in the safe core contract to achieve the functionality you're suggesting

let success := delegatecall(gas(), targetContract, add(calldataPayload, 0x20), mload(calldataPayload), 0, 0)
let gasAfter := gas()
let gasUsed := sub(gasBefore, gasAfter)
Copy link
Member

@mmv08 mmv08 Jul 8, 2024

Choose a reason for hiding this comment

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

This can be returned by the targetContract. You can check the SimulateTxAccessor contract for an example. You can basically combine its approach with your original contract for detailed simulations

@mshakeg
Copy link
Author

mshakeg commented Jul 8, 2024

@mmv08 agreed, no changes are required to core, but does safe-global have a repo with periphery contracts? seems like this functionality would be quite essential to gas estimation and should be included in the sdk(and so would require leveraging a safe deployed contract).

Copy link

@davidaucoin7377 davidaucoin7377 left a comment

Choose a reason for hiding this comment

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

Simulation

let success := delegatecall(gas(), targetContract, add(calldataPayload, 0x20), mload(calldataPayload), 0, 0)
let gasAfter := gas()
let gasUsed := sub(gasBefore, gasAfter)
Copy link
Author

Choose a reason for hiding this comment

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

this is redundant since SimulateTxAccessor.simulate returndata includes the safeTxGas:

(uint256 estimate, bool success, bytes memory returnData)

dodger213 added a commit to dodger213/safe-smart-contract that referenced this pull request Aug 18, 2024
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 this pull request may close these issues.

Add simulateTransaction Function for Static Safe Transaction Simulation
5 participants