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

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

Open
franvf opened this issue Aug 9, 2023 · 9 comments · May be fixed by #512
Open

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

franvf opened this issue Aug 9, 2023 · 9 comments · May be fixed by #512
Labels
question Further information is requested

Comments

@franvf
Copy link

franvf commented Aug 9, 2023

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",

  • Safe contract version: SafeL2 (goerli)
  • Environment:
    • Postman
    • non-browser

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
Copy link

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 leovigna linked a pull request Aug 20, 2023 that will close this issue
@leovigna
Copy link

leovigna commented Aug 20, 2023

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
Copy link

leovigna commented Aug 20, 2023

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

@franvf
Copy link
Author

franvf commented Aug 22, 2023

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

@leovigna
Copy link

leovigna commented Aug 22, 2023

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
Copy link
Collaborator

dasanra commented Sep 14, 2023

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!

@dasanra dasanra added the question Further information is requested label Sep 14, 2023
@leovigna
Copy link

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

@leovigna
Copy link

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
Copy link

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

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

Successfully merging a pull request may close this issue.

3 participants