5afe / contract-proxy-kit

Enable batched transactions and contract account interactions using a unique deterministic Gnosis Safe.
103 stars 38 forks source link

proxy creation and batch transaction execution expected to fail #120

Closed fritzschoff closed 3 years ago

fritzschoff commented 4 years ago

Hey, I try to enable a module for a user that doesn't have a proxy contract yet, assign a executor for a . My three transaction look something like that:

{ 
to: cpk.address, 
operation: CPK.Call,
value: 0, 
data: new utils.Interface(GnosisSafeAbi.abi).encodeFunctionData('enableModule', [this.gelatoCore.address]),
},
{
   to: gelatoCore.address,
            operation: 0,
            value: 1,
            data: new utils.Interface(GelatoCoreAbi.abi)
              .encodeFunctionData('multiProvide', [isDefaultExecutorAssigned
                ? constants.AddressZero : storeValue.executor.gnosis[networkName].address,
              [], // this can be left empty, as it is only relevant for external providers
              isUserProxyModuleWhitelisted ? [] : [this.storeValue.executor.gnosis[networkName].address]]),
},
{
operation: 0,
to: this.gelatoCore.address,
value: 0,
data: new utils.Interface(GelatoCoreAbi.abi).encodeFunctionData('submitTaskCycle', [provider, [task], timestamp <= 0 ? 0 : timestamp, cycles])
}

for the last two transaction, I know they are specific and have nothing to do with the CPK. But I don't understand the error message... Any clue?

fritzschoff commented 4 years ago

I tracked the error down and it is because of the multiprovide from the gelato core contract. If you send ETHER with this function, it breaks!

rmeissner commented 4 years ago

hmm I think this is related to Gelato being based on the 1.2.0 contracts (and a custom CPK factory). Both allow to send along ETH on deployment/ execution. We are currently working on a new factory that would include this too (additionally to the 1.2.0 support).

fritzschoff commented 4 years ago

do you got an idea what would be the steps to fix that? Cause this is quite blocking for me atm. Maybe I can update my forked CPK version.

rmeissner commented 4 years ago

You would need to specify a custom factory and custom mastercopy (see https://github.com/gnosis/contract-proxy-kit#networks-configuration). You need to check with the gelato team which values they use there.

fritzschoff commented 4 years ago

I updated these values with the custom one from gelato. The issue is more coming from that function. image If Eth is send with the transaction array in the executeTransaction function, this function will return false. I saw that the interface NormalizeGas<SendOptions> doesn't have key called value what should be responsible of transferring the Eth? I'm guessing here a bit, so I apologize if this is totally wrong!

cag commented 4 years ago

@fritzschoff I think your deduction is right. BaseTxOptions should be extended to include a value: NumberLike. If you're calling this from JS though, there shouldn't be any logic there to filter the value option out of the options object though, and AFAICT, there's nothing else that should be needed if you're using TS, as I don't think there's any extra processing other than collapsing gas and gasLimit to gas. (I would have preferred gasLimit but the reason why it's gas is a separate discussion)

fritzschoff commented 4 years ago

image I need to take back the missing value property. If I don't send any eth with the transaction it will let me sign the tx but throw me the error above. Maybe your saltNonce change from v1.2 contracts and now it is this string 0xcfe33a586323e7325be6aa6ecd8b4600d232a9037e83c8ece69413b777dabe65 but to be honest, that is a wild guess like above. At the moment it seems like I just need to wait for you guys to finish supporting such "old" contracts but I'm out of ideas! FWIW: this is the factory contract that I'm using : https://rinkeby.etherscan.io/address/0x1f4Ab3778e3FeB06A3D4702B31CE56a561080986#code

cag commented 4 years ago

@fritzschoff Did you also pass along the total value for the overall transaction? For example:

cpk.execTransactions(
  [
    {
      to: cpk.address,
      data: new utils.Interface(GnosisSafeAbi.abi).encodeFunctionData(
        'enableModule',
        [this.gelatoCore.address]
      ),
    },
    {
      to: gelatoCore.address,
      value: 1, // value was originally specified here
      data: new utils.Interface(GelatoCoreAbi.abi).encodeFunctionData(
        'multiProvide',
        [
          isDefaultExecutorAssigned ?
            constants.AddressZero :
            storeValue.executor.gnosis[networkName].address,
          [],
          isUserProxyModuleWhitelisted ?
            [] :
            [this.storeValue.executor.gnosis[networkName].address]
        ]
      ),
    },
    {
      to: this.gelatoCore.address,
      data: new utils.Interface(GelatoCoreAbi.abi).encodeFunctionData(
        'submitTaskCycle',
        [
          provider,
          [task],
          timestamp <= 0 ? 0 : timestamp,
          cycles,
        ]
      ),
    },
  ],
  {
    // this value refers to the amount of Eth sent with the overall transaction
    // which should be the sum of the values from earlier.
    value: 1
  }
);
fritzschoff commented 4 years ago

@cag Hey, good catch, if I put eth value as the second parameter it works. Although I need to cast it to any since value is not part of ExecOptions interface. this.cpk.execTransactions(modulesToWhitelist, { value: 1 } as any); But the gelato core will only receive 1 WEI. I guess this because of the encoding in the factory contract, cause he thinks it is one wei and not one ether! But I can't put in wei in the value property cause the cpk will always call BigNumber.from() on the value property at the transaction object.

cag commented 3 years ago

Closing this as this sounds like it has been resolved on your end. Feel free to reopen if that's not the case though.