safe-global / safe-core-sdk

The Safe{Core} SDK allows builders to add account abstraction functionality into their apps.
https://docs.safe.global/sdk/overview
MIT License
255 stars 202 forks source link

feat(Protocol-kit): H3Error: SafeProxy was not deployed correctly #505

Open franvf opened 1 year ago

franvf commented 1 year ago

Description

I'm trying to deploy a safe wallet, but the function deploySafe() returns the error: SafeProxy was not deployed correctly. But, if I check the tx hash, the SafeProxy is deployed successfully.

I have solved this locally modifying the @safe-global/protocol-kit/src/adapters/web3/contracts/SafeProxyFactory/SafeProxyFactoryWeb3Contract.js file

What I have modified in this file is: Original file (not works)

const txResponse = this.contract.methods
            .createProxyWithNonce(safeMasterCopyAddress, initializer, saltNonce)
            .send(options);
        if (callback) {
            const txResult = await (0, utils_1.toTxResult)(txResponse);
            callback(txResult.hash);
        }
        const txResult = await new Promise((resolve, reject) => txResponse.once('receipt', (receipt) => resolve(receipt)).catch(reject));
        const proxyAddress = (_c = (_b = (_a = txResult.events) === null || _a === void 0 ? void 0 : _a.ProxyCreation) === null || _b === void 0 ? void 0 : _b.returnValues) === null || _c === void 0 ? void 0 : _c.proxy;
        if (!proxyAddress) {
            throw new Error('SafeProxy was not deployed correctly');
        }
        return proxyAddress;
    }

My modified file (work)

 const txResponse = this.contract.methods
            .createProxyWithNonce(safeMasterCopyAddress, initializer, saltNonce)
            .send(options);
        if (callback) {
            const txResult = await (0, utils_1.toTxResult)(txResponse);
            callback(txResult.hash);
        }
        const txResult = await new Promise((resolve, reject) => txResponse.once('receipt', (receipt) => resolve(receipt)).catch(reject));

        //MY CODE START HERE
        const events = await this.contract.getPastEvents("ProxyCreation")
        const proxyAddress = events[0]['returnValues']['0'] //Get the deployed safe proxy address

        if (proxyAddress == "0x0000000000000000000000000000000000000000") {
            throw new Error('SafeProxy was not deployed correctly');
        }
        return proxyAddress;
    }
    //MY CODE FINISH HERE

Environment

@safe-global/protocol-kit version: "^1.2.0", @safe-global/safe-core-sdk-types version: "^2.2.0",

Steps to reproduce

My Code

const safeFactory = await SafeFactory.create({ethAdapter})

    const owners = [userAddress, adminAddress] //Safe wallet will be managed by two owners, the user and us
    const threshold = 1 //Just one signature is required to carry out a tx

    const safeAccountConfig: SafeAccountConfig = {
        owners, 
        threshold
    }

    const callback = (txHash: any) => {
        console.log({txHash}) //Show the tx hash
    }

    const safeSdk = await safeFactory.deploySafe({safeAccountConfig, callback}) //Here is when I get the error

Expected result

The error should not be thrown.

leovigna commented 1 year ago

I have had the same error following the official ProtocolKit tutorial and debugged the root issue by manually deploying the Safe. Issue comes from here:

// src/adapters/ethers/contracts/SafeProxyFactoryEthersContract.ts
async createProxy({
    safeMasterCopyAddress,
    initializer,
    saltNonce,
    options,
    callback
  }: CreateProxyProps): Promise<string> {
   //...

    const proxyAddress = this.contract
      .createProxyWithNonce(safeMasterCopyAddress, initializer, saltNonce, options)
      .then(async (txResponse) => {
        if (callback) {
          callback(txResponse.hash)
        }
        const txReceipt = await txResponse.wait()

        //BUG: Event is emitted but is hash no "ProxyCreation" name, however event signatures match!
        const proxyCreationEvent = txReceipt?.events?.find(
          ({ event }: Event) => event === 'ProxyCreation'
        )
        if (!proxyCreationEvent || !proxyCreationEvent.args) {
          throw new Error('SafeProxy was not deployed correctly')
        }
        const proxyAddress: string = proxyCreationEvent.args[0]
        return proxyAddress
      })
    return proxyAddress
  }

The safe is properly deployed, and the factory emits the correct event with the event signature. However it has a different encoding. is not decoded by ethers, and is left in it's raw format. This is because I assume old versions of the proxy factory used the same event signature ProxyCreation(address,address) but without indexing.

# The following two events have the same signature but different encoding
# old version, or at least what is expected by ethers
event ProxyCreation(address proxy, address singleton); # topics: [signature], data: [proxy, singleton]
# v1.3.0
event ProxyCreation(address indexed proxy, address singleton); # topics: [signature, proxy], data: [singleton]

I discovered this by creating my own interfaces for the contracts in solidity and then deploying manually however a simpler patch can be made to fix this issue at the SDK level using ethers to decode the raw log. I will make a suggested PR with this.

const proxyCreationLog = txReceipt?.events?.find(
  ({ topics }: Event) =>
    topics[0] === '0x4f51faf6c4561ff95f067657e43439f0f856d97c04d9ec9070a6199ad418e235'
) as Event | undefined

let proxyCreationEventArgs: { 0: string; 1: string; proxy: string; singleton: string }
  | undefined
if (proxyCreationLog) {
  if (proxyCreationLog.topics.length == 1) {
    const ifaceNonIndexedProxyAddress = new ethers.utils.Interface([
      'event ProxyCreation(address proxy, address singleton)'
    ])
    proxyCreationEventArgs = ifaceNonIndexedProxyAddress.decodeEventLog(
      'ProxyCreation',
      proxyCreationLog.data,
      proxyCreationLog.topics
    ) as unknown as typeof proxyCreationEventArgs
  } else if (proxyCreationLog.topics.length == 2) {
    const ifaceIndexedProxyAddress = new ethers.utils.Interface([
      'event ProxyCreation(address indexed proxy, address singleton)'
    ])
    proxyCreationEventArgs = ifaceIndexedProxyAddress.decodeEventLog(
      'ProxyCreation',
      proxyCreationLog.data,
      proxyCreationLog.topics
    ) as unknown as typeof proxyCreationEventArgs
    }
}
leovigna commented 1 year ago

Note while this patch (see #512) fixes the decoding, the core underlying issue is the contract object stored by the SDK does not seem to match the deployed contract's interface.

leovigna commented 1 year ago

Decoding the return value also works :+1: However, it seems your solution @franvf will only work with web3.js since ethers does not support getPastEvents out of the box.

franvf commented 1 year ago

Hi @leovigna, you propose a very clean solution. Thank you!

leovigna commented 1 year ago

Thanks. I would also recommend the actual interface used by the SDK (as mentioned in comment above) as this could cause other issues if parts of the code rely on events that aren't properly decoded.

dasanra commented 1 year ago

Hey @franvf @leovigna thank you for your report!

We have been checking but we are not able to replicate this issue, everything works as expected with the latest protocol-kit version. Could you confirm that it also works on your side?

Thank you!

leovigna commented 1 year ago

Was using v1.2.0. I see v1.3.0 was recently released, was the issue fixed there?

leovigna commented 1 year ago

It seems this file was not changed https://github.com/safe-global/safe-core-sdk/blob/main/packages/protocol-kit/src/adapters/ethers/contracts/SafeProxyFactory/SafeProxyFactoryEthersContract.ts as can also be seen by my PR https://github.com/safe-global/safe-core-sdk/pull/512/files

I would assume the error persists due to the above described issue with ethers being unable to decode the event.

Did you test with web3.js or ethers?

leovigna commented 1 year ago

I suppose the issue might have also been fixed itself if the proper ethers contract abi is being instantiated relative to the version.