keep-network / keep-core

The smart contracts and reference client behind the Keep network
https://keep.network
MIT License
113 stars 72 forks source link

Implement Wallet Connect v2 #3768

Closed michalsmiarowski closed 5 months ago

michalsmiarowski commented 5 months ago

🔷 WalletConnect V2 implementation 🔷

Hero Horizontal

Ref: #3764

This Pull Request introduces WalletConnect V2 integration into the Keep Dashboard, replacing the deprecated V1.

Main changes:

Description of changes

- Developed a web3 connector for WalletConnect V2

To ensure seamless WalletConnect V2 functionality in our Keep Dashboard, we created a specialized web3 connector, named WalletConnectV2Connector. This connector is tailored to interact efficiently with our dApp and the wallets it connects to.

Initially, I encountered issues with event handling using this connector and its underlying provider (@walletconnect/ethereum-provider). Fortunately, these were resolved, though it necessitated a revision of our event subscription code (detailed in the following section).

- Reformulated our approach to event subscriptions

As mentioned, to address the event handling issues with WalletConnect, we employed a workaround. This involved creating additional Contract instances that utilize the connector in an explorer mode. These are incorporated into a new instance of the Keep library, termed KeepExplorerMode, which currently is used exclusively for event tracking in newly created react hooks.

The event subscription hooks rely on the patterns established in our redux sagas, specifically within src/saga/subscription.js and src/saga/copy-stake.js. The original subscription listeners in these files have been disabled.

- Integrated the craco library

Incorporating @walletconnect/ethereum-provider necessitated adjustments to our webpack configuration, similar to those made in the Threshold Dashboard. Since such modifications are infeasible in CRA without external assistance, we opted to use craco. This involved appending two plugins in craco.config.js:

module.exports = {
  babel: {
    plugins: [
      "@babel/plugin-proposal-nullish-coalescing-operator",
      "@babel/plugin-proposal-optional-chaining",
    ],
  },
}

Additional changes

KeepExplorerMode

KeepExplorerMode is an another variant of the Keep library. Contrary to the standard Keep library, whose provider changes based on the connected wallet, KeepExplorerMode consistently uses an explorer mode provider (except when no wallet is connected, wherein it defaults to the standard Keep library provider).

This library iteration is solely for event tracking purposes. For a detailed explanation of this implementation, refer to the Reformulated our approach to event subscriptions section above.

@walletconnect/keyvaluestorage package

Initially, there were challenges in displaying the WalletConnect login modal. The debugging process was complex, but it was eventually discovered that the issue stemmed from a dependency chain related to @walletconnect/ethereum-provider. Specifically, a nested dependency utilized a very recent version of @walletconnect/keyvaluestorage which, for unknown reasons, was malfunctioning.

To address this, we explicitly added @walletconnect/keyvaluestorage to our package.json and downgraded its version to match the one used in the Threshold Dashboard. This ensured compatibility and resolved the issues with the login modal display.

michalsmiarowski commented 5 months ago

Overall code looks good to me 🔥

There is only a little bug with Etherscan links generation for Goerli testnet. Seems like the function doesn't recognize Goerli as a testnet and generates link for Ropsten. When I manually replaced https://ropsten.etherscan.io/tx/... with https://goerli.etherscan.io/tx/... the links works well so it's a matter of choosing proper link subdomain depending on chain id.

Thanks for checking it out!

As for the link issue, I would say that it's not necessary to do it, because "officialy" keep dashboard does not support goerli - we've started using it in threshold dashboard and did not introduce it here. Since this is a legacy project, I would say we could ignore it.