Migrate to new walletconnect protocol

[polarker]

In this PR, I migrate @LeeAlephium 's previous work to the new walletconnect protocol: https://github.com/alephium-web3/alephium-walletconnect

------- Walletconnect Protocol
As you could see from the commit history alephium-walletconnect, I started from @LeeAlephium 's initial work but ended up changing almost everything. I think it's better to push it to a new repo under alephium or keep it under alephium-web3.

------- Desktop Integration
There are a few stuff to improve from my PR:

• Better React code following the best practice
• Check NetworkId for each new connection
• Better error handling
• Session management. for example: how and when to disconnect? shall we persist the session if there is a wallet reload?
• Two other signing Modal could be added: signHexString and signMessage, but the priority is low.
• Maybe better transaction state management. Right now we have separated states for user-fired transactions and walletconnect transactions, maybe we could share the logic and avoid race conditions.

[polarker]

Request: could you guys help me to merge the breaking changes introduced lately? I tried not to touch too much of the codebase, so I hope it will not be too painful to merge

[LeeAlephium]

As you could see from the commit history alephium-walletconnect, I started from @LeeAlephium 's initial work but ended up changing almost everything. I think it's better to push it to a new repo under alephium or keep it under alephium-web3.

Yeah, let's just put it into a new repository, under the same name as you have (alephium-walletconnect) or just "walletconnect" since everything is already scoped under the alephium org/user (I would prefer the latter)

[LeeAlephium]

Is this now stale? #238

[polarker]

Is this now stale? #238

Yes, it is. Closed now.

[nop33]

@polarker could you please rebase so that the conflicts are solved, before I proceed with reviewing? You know best what changes you made. If I was to do the rebase, I'd first need to go through the 2K LOC, understand them, and then rebase :)

[polarker]

@polarker could you please rebase so that the conflicts are solved, before I proceed with reviewing? You know best what changes you made. If I was to do the rebase, I'd first need to go through the 2K LOC, understand them, and then rebase :)

It's hard to rebase now, as it includes many changes from both me and Lee.

Merging master to this PR (i.e. applying master changes to this PR) seems to be more convenient. If we follow this path, it will be easier to do by your guys.

Edit: here is the steps to merge all of the latest changes:

1. git merge origin/master
2. resolve all of the conflicts introduced by the new changes

[nop33]

In my opinion, master should not get merged to a feature branch. Wouldn't that require a force push on master then? Each feature branch should be based on the latest master commit, to avoid spaghetti git log. Conceptually, it also makes sense to develop always on top of master. I can try do the rebase, but I don't know if I'll break anything.

[polarker]

In my opinion, master should not get merged to a feature branch. Wouldn't that require a force push on master then? Each feature branch should be based on the latest master commit, to avoid spaghetti git log. Conceptually, it also makes sense to develop always on top of master. I can try do the rebase, but I don't know if I'll break anything.

If you prefer rebasing please help me do it. This touches changes by Lee and by you, and I don't have the time to do it.

Edit: no force push is needed. We use this approach for months. Again, I am not trying to recommend anything.

Btw, do you guys review rebase & force push btw? We use git merge after mainnnet so all commit histories are preserved and we review merge commit as well.

[nop33]

Btw, do you guys review rebase & force push btw? We use git merge after mainnnet so all commit histories are preserved and we review merge commit as well.

We make sure the feature branch is rebased on master, we review the new additions, and we merge.

[nop33]

If you prefer rebasing please help me do it. This touches changes by Lee and by you, and I don't have the time to do it.

I'll test your PR to make sure it works, then I'll rebase and test again, making sure I didn't breaking anything, and if everything works as expected, I'll force push here.

[polarker]

Btw, do you guys review rebase & force push btw? We use git merge after mainnnet so all commit histories are preserved and we review merge commit as well.

We make sure the feature branch is rebased on master, we review the new additions, and we merge.

I removed the comments, as it's out of topic. Each repo uses its own approach.

I am more asking: rebase / force push are also used in the branch during or after code review, how do you review all of the changes for such rebase / force pushes?

As I have been saying many times, we used the same approach as you guys before main-net launch. I get all of the advantage points you mentioned, but there are advantages of other approaches as well.

[polarker]

Thanks for great improvements ❤️ @nop33

[nop33]

Build is failing because of #204 (comment)

Investigating...

[nop33]

Why not use string literal types again?...

because I wanted to specify that the modalType of the TxDataToModalType type should only be one of the SendTxModalType, not arbitrary strings as it was before in Cheng's solution.

[polarker]

because I wanted to specify that the modalType of the TxDataToModalType type should only be one of the SendTxModalType, not arbitrary strings as it was before in Cheng's solution.

While I get your motivation, it's not really possible to use an arbitrary string in my code. Here are the errors when I replace transfer with transfe:

Argument of type '"script" | "deploy-contract" | "transfe"' is not assignable to parameter of type 'false | TxModalType'.
  Type '"transfe"' is not assignable to type 'false | TxModalType'. Did you mean '"transfer"'?  TS2345

    80 |     (data: Exclude<ContextType['dappTransactionData'], undefined>) => {
    81 |       setDappTransactionData(data)
  > 82 |       setTxModalType(data[0])
       |                      ^
    83 |     },
    84 |     [setDappTransactionData, setTxModalType]
    85 |   )

I am totally fine with your solution.

[polarker]

As we have seen, Walletconnect is very fragile and it's easy to get strange errors when playing with the voting demo.

Let's start to improve Walletconnect on the following aspects:

• Better session management. As far as I see, all of the issues are related to session management.
• Check NetworkId for each new connection. This is not related to the issues observed, but it's one feature I don't have time to work on.
• Better error handling for Walletconnect.

@mvaivre @LeeAlephium Is it possible to work on these topics with high priority?

[mvaivre]

@polarker In order to prepare for the hard fork and the onboarding of new developers, we planned to develop a set of little showcase dApps. This is the best way to test our stack, complete the documentation and identify the bugs.

So YES, definitely, getting Walletconnect to work flawlessly is mandatory and is still considered highest priority (if not bug-free, then the task isn't done).

@LeeAlephium I'm pretty sure that you agree with that :) Let's improve WalletConnect following Cheng's feedback, and once it's running wallet, let's dev some dApps, shall we? 🚀

[polarker]

@polarker In order to prepare for the hard fork and the onboarding of new developers, we planned to develop a set of little showcase dApps. This is the best way to test our stack, complete the documentation and identify the bugs.

Awesome. Looking forward to it 🚀 Let's all build dApps 😃

[LeeAlephium]

I'll do it alongside working on a small dapp because the voting-demo is just too hacked together at this point... Is that ok? Yeah, these errors which make everything explode should not be possible. I spent a couple days when I was working on WalletConnect on trying to catch these but I just couldn't figure it out. A smaller project will make this much easier to dissect...

[polarker]

I'll do it alongside working on a small dapp because the voting-demo is just too hacked together at this point... Is that ok? Yeah, these errors which make everything explode should not be possible. I spent a couple days when I was working on WalletConnect on trying to catch these but I just couldn't figure it out. A smaller project will make this much easier to dissect...

Sounds good to me. The voting demo is too heavy.

[mvaivre]

FYI I've started looking into simplifying the UI.

Specifically:

• We don't want that many buttons in the wallet's navbar, especially not advanced features. I'll merge the two contract related ones into a single item (submenu showing on hover).
• I'll improve the design of the various TX modals.
• I'll improve the wallet connect workflow. The one thing I want the most is deep link support. This will be done later, in a new PR.

[polarker]

We don't want that many buttons in the wallet's navbar, especially not advanced features. I'll merge the two contract related ones into a single item (submenu showing on hover).

Agree. We could remove the two contract-related tx buttons completely. I had them there so I could test them without using walletconnect.

[mvaivre]

I could add an option in the settings, like "Show developers tools", which would make the contract item visible in the navbar. WDYT?

[polarker]

I could add an option in the settings, like "Show developers tools", which would make the contract item visible in the navbar. WDYT?

Looks great to me!

[polarker]

@mvaivre Could you help to add this Devnet setting? This will make it easier to interact with our Devnet stack.

Node host
http://127.0.0.1:22973/
Explorer API host
http://localhost:9090/
Explorer URL
http://localhost:3000/

[nop33]

@mvaivre Could you help to add this Devnet setting? This will make it easier to interact with our Devnet stack.

@polarker this was already updated by me last week. See here: https://github.com/alephium/desktop-wallet/blob/cheng-walletconnect/src/utils/settings.ts#L59-L64

[nop33]

As I had said on Slack, we should still work with beta.26. Ref: https://github.com/alephium/alephium-nft/pull/1/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R24-R26

So, let's revert back to beta.26. From my understanding, @LeeAlephium was trying to fix the session issues by upgrading.

This PR is assigned to the v1.5.0 release https://github.com/orgs/alephium/projects/9, so it should get merged after v1.3.0 and v1.4.0 are out. Maybe by then we have enough time to implement Cheng's improvements and even upgrade to a newer version of WalletConnect that fixes some of the issues of beta.26.