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

implement WalletConnect (v1) as a dapp option into hdwallet #499

Closed
2 tasks done
BitHighlander opened this issue Mar 31, 2022 · 29 comments · Fixed by #544
Closed
2 tasks done

implement WalletConnect (v1) as a dapp option into hdwallet #499

BitHighlander opened this issue Mar 31, 2022 · 29 comments · Fixed by #544
Assignees
Labels
keepkey-bounty Bounties specific to KeepKey
Milestone

Comments

@BitHighlander
Copy link
Contributor

BitHighlander commented Mar 31, 2022

Overview

Overview

https://walletconnect.com/

Wallet connect is a protocol allowing wallets to connect with Dapps. ShapeShift as a Dapp would like to allow any v1 integrated wallet in the wallet connect ecosystem to connect to app.shapeshift.com as a wallet, and utilize all supported features. These features are specific to ETH, and ETH compatible EVM’s.

HDwallet is a collection of supported wallet modules that implement a common base class. This bounty is to create a module that supports wallet-connect as a wallet option and conform to this base class of HDwallet.

Wallet connect provides the following example dapp
Example Dapp: https://github.com/WalletConnect/walletconnect-example-dapp

Example MetaMask addition PR
Example: https://github.com/shapeshift/hdwallet/pull/359/files

WalletConnect documentation:
Docs: https://docs.walletconnect.com/

References and additional details

References

I am referencing the MetaMask integration and code snippets here to help explain the process of adding a new wallet to this HDwallet repo. As it is similar in scope/function.

See it complete:
HDwallet sandbox: https://hdwallet-shapeshift.vercel.app/
Screen Shot 2022-03-30 at 2 36 23 PM

Notice “Pair Wallet”

Wallet Connect will have a more interactive pairing process.
Summary of pairing:

For testing manually I would recommend metamask mobile (supports wallet-connect) or trustwallet (mobile).

There will be a walletconnect modal displayed to the user to require the user to scan a QR code in their mobile wallet.

(there are also desktop wallet intents that can launch desktop apps automatically.) as well as intents to launch mobile wallets if the user is viewing the dapp on their mobile wallet.

Example dapp:https://example.walletconnect.org/

Screen Shot 2022-03-30 at 2 39 33 PM

After pairing, the sandbox should implement all supported functions, and display errors on those without support.

Note: asset support is flagged in the wallet class
https://github.com/shapeshift/hdwallet/blob/master/packages/hdwallet-metamask/src/metamask.ts#L23

Metamask package dir
https://github.com/shapeshift/hdwallet/tree/master/packages/hdwallet-metamask

Metamask Tests
https://github.com/shapeshift/hdwallet/blob/master/integration/src/wallets/metamask.ts

Acceptance Criteria

A.C.

  • a complete wallet module with support for ETH functions
  • Some form of tests written into both unit/integration (with mocked response data)
  • A working sandbox wallet-connect option against a live wallet-connect server/wallet (tested manually)

Need By Date

no

Screenshots/Mockups

N/A

Ownership

  • If my bounty needs engineering or needs product I have added the respective labels on the right
  • As the sponsor of this bounty I will review the changes in a preview environment (ops/product) or review the PR (engineering)

Estimated effort

2-3 days

Sponsor / Stakeholder

@BitHighlander

Bounty Hunters

  • Join our discord
  • Include an expected timeline for you to complete work in the work plan when you apply for this bounty!
  • Please refer to this link for some basic info
  • Please do not start work on this issue until you are approved in Gitcoin.
@0xdef1cafe 0xdef1cafe added the needs product Requires product input before bounty label Mar 31, 2022
@DiggyDiggy2
Copy link

DiggyDiggy2 commented Mar 31, 2022

NOTE: Web integration is out of scope for this bounty and will come after this is completed :)

  1. Add "wallet connect" as a wallet provider into the Wallet selection modal. Logo to use can be found here: https://github.com/WalletConnect/walletconnect-assets/blob/master/svg/original/walletconnect-logo.svg

Screen Shot 2022-03-31 at 11 14 08 AM

  1. Then display QR Code for wallet connect
  2. Once the wallet is connected, the user should see the confirmation modal.

Screen Shot 2022-03-31 at 11 17 29 AM

.

@DiggyDiggy2 DiggyDiggy2 added needs engineering Requires engineering input before bounty and removed needs product Requires product input before bounty labels Mar 31, 2022
@0xdef1cafe 0xdef1cafe added needs bounty Issues that should be bountied. and removed needs engineering Requires engineering input before bounty labels Mar 31, 2022
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 5500.0 FOX (1717.99 USD @ $0.31/FOX) attached to it as part of the ShapeShift fund.

@0xean 0xean added bounty Issue is posted on dework.xyz as a bounty and removed needs bounty Issues that should be bountied. labels Apr 1, 2022
@gitcoinbot
Copy link

gitcoinbot commented Apr 1, 2022

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 264 years, 7 months from now.
Please review their action plans below:

1) bielwenass has applied to start work (Funders only: approve worker | reject worker).

Excited about the possibility to work on another Shapeshift bounty!
Going to research the info provided in this bounty and implement a solution with the focus on clean code and compatibility with existing interfaces, then test it thoroughly.
2) prashiyn has applied to start work (Funders only: approve worker | reject worker).

Have already implemented walletConnect in another project. Will research the requirements here
3) k1-r1 has applied to start work (Funders only: approve worker | reject worker).

I have worked with WalletConnect before, and would follow the specification exactly. I work in a testing oriented fashion and in a timely manner.
4) michals92 has been approved to start work.

As senior iOS developer, I already implemented this function for my project.

Learn more on the Gitcoin Issue Details page.

@mrnerdhair
Copy link
Contributor

I think we need to work more on the specification of this issue before starting work.

  • We should specify v1 of WC if that's what we want to target.
  • It should be made clear that displaying any UI is out-of-scope for hdwallet as a package. The UI development aspect here should be in the sandbox only.
  • It looks to me like we're specifying using hdwallet as a "dapp" to connect to someone else's "wallet". Is that correct? If so, it doesn't fit very well into our existing wallet initialization or adapter paradigms.
  • New code should be required to pass eslint rules as implemented in e.g. feat: add-eslint #485 -- plus no-explicit-any, which is only left out of that PR because it in our old code would be a PITA.

Overall, I'm not happy about doing more development on hdwallet's existing interface. We need to talk about hdwallet++ before creating yet another mostly cut-n-paste boilerplate wallet package. Each one of these we do makes doing maintenance harder. I don't think we can afford to start work on new features in this package until addressing things like #454, #456, #457, #458, #459, #462, and #466.

@BitHighlander BitHighlander changed the title implement WalletConnect as a wallet option into hdwallet implement WalletConnect (v1) as a wallet option into hdwallet Apr 1, 2022
@BitHighlander
Copy link
Contributor Author

To clarify this bounty is only for HDwallet. And for v1 version of wallet-connect.

Ive added v1 to title, and specified the exact module to add. I confirmed All the code examples posted are for v1

it looks to me like we're specifying using hdwallet as a "dapp" to connect to someone else's "wallet". Is that correct? If so, it doesn't fit very well into our existing wallet initialization or adapter paradigms.

This was my concern when we added MetaMask. However it does seem like HDwallet has matured into more of a wallet-wrapper more than a wallet library. I do feel that this pattern is already well established, and the MetaMask module is very close in architecture to this bounty and should give clarity to the path forward.

New code should be required to pass eslint rules as implemented in e.g. #485 -- plus no-explicit-any, which is only left out of that PR because it in our old code would be a PITA.

I think that should be its own issue, not sure its relevance in this bounty.

@mrnerdhair
Copy link
Contributor

In WC, the "dapp" has to start the connection procedure by opening the connection to the WC bridge server and delivering a WC url to the client. This is much more complex than in MM, where hdwallet-metamask makes a single call to the provider and can just throws if the user rejects. We don't have a good way to represent a wallet in an intermediate state of connection like this in our current interfaces, hence my concern about needing to work on the adapter/initialization paradigms upfront.

Code we accept to hdwallet must be of high quality. Allowing submissions for this bounty to break eslint rules would mean shifting the burden of fixing it to meet those standards later to NeOMakinG or myself. It's gonna be a PITA to get #485 in as-is; please don't make my life harder.

@prashiyn
Copy link

prashiyn commented Apr 3, 2022

WC accepts supports multiple wallets ( a huge list actually) and each wallet supports multiple/different assets. WC api's do not provide any API for supported assets by each wallet. How do envision managing support for assets?

@0xean
Copy link
Contributor

0xean commented Apr 5, 2022

Per @0xdef1cafe - we are moving forward with this bounty. Will select a bounty hunter today.

@EdgarBarrantes
Copy link

👋 I'm starting this and have a couple of questions/notes to make, some of these things are related to my lack of knowledge of this repo (first time working with it):

  • Do you have any preference regarding the WalletConnect provider? I can see for Portis you're using web3 but for the native package you're going for ethers. For the moment I'll go with ethers being the one I'm the most familiar with.
  • As far as I can see so far, the package will indeed end up being heavily inspired in the Metamask one, I'll try to keep it as clean as possible though but I just wanted to double check that that is what's expected.
  • I've been eyeing the pending PR's... I'll use the eslint rules in my editor to have to change as little as possible later and I'll keep those pending changes in mind.

I'm writing here just to keep things in a place, I'll ping you in #engineering-public if there's something more urgent.

@mrnerdhair
Copy link
Contributor

mrnerdhair commented Apr 7, 2022

  • Portis isn't using standard web3, it's using it's own super outdated fork specialized for talking to Portis servers, so don't go for that. Web3 is super heavy in general; if you can use ethers and thereby avoid importing another dependency, that would be best.
  • hdwallet-metamask has some remaining jank. A plain cut-n-paste will require various fixups; see my initial feedback to Add tally wallet #506 for examples. It may be better to use hdwallet-xdefi as a template; it's a bit more mature since the changes made during its development haven't been backported to hdwallet-metamask yet.
  • I appreciate the eslint attention. I've just rebased and updated feat: add-eslint #485, which includes a lot of eslint fixes; you may want to start your cut-n-paste journey from that branch to minimize headaches.

@EdgarBarrantes
Copy link

Brilliant, I see the eslint branch got merged so I just rebased master in mine. Thank you!

@0xean
Copy link
Contributor

0xean commented Apr 18, 2022

@EdgarBarrantes - can you please provide an update on your progress and expected completion of this bounty?

Thank you!

@EdgarBarrantes
Copy link

@0xean of course, I was set to finish this in the next couple of days (15 days), I haven't really been able to commit as much time to this ticket as I'd like, I have a shell for this package and it's tests from about a week ago. I'd like to pospone my expected delivery time for about 10 days. I'll give more details on chat.
Thanks!

@0xean
Copy link
Contributor

0xean commented Apr 20, 2022

@michals92 - I just approved you on gitcoin, please let me know a rough timeline for completion on this bounty. TY!

@michals92
Copy link

Hi, thanks, I have to go through all the info provided, but I would like to finish it in 14 days.

@0xean
Copy link
Contributor

0xean commented Apr 20, 2022

@michals92 - great, please let us know if you have questions. Discord is always a great place (#engineer-public).

@0xdef1cafe
Copy link
Collaborator

@michals92 - can we please get an update of progress and an ETA for this bounty?

@michals92
Copy link

michals92 commented Apr 28, 2022

Hi, I created package for wallet connect, studied documentation and also went through metamask and xdefi implementation. I want to get it done this weekend.

My fork:
https://github.com/michals92/hdwallet

@michals92
Copy link

I added UI for WalletConnect, dialog pops up in my branch, now I need to finish implementation and write test for it. I want to finish it ASAP.

@0xdef1cafe
Copy link
Collaborator

@michals92 can you please open a draft PR for the current WIP, even if it's unstable? we need this as a top priority and would like to see the progress so far.

@Lychbot
Copy link

Lychbot commented May 3, 2022

@0xdef1cafe posting to dework, how many hours should the estimate be?

@0xdef1cafe
Copy link
Collaborator

@michals92 you initially advised this would be done over the weekend. we need to see tangible progress here, can you please post a draft PR within 24 hours, as we need to consider bringing this bounty in house.

@michals92
Copy link

Ok, sorry that I didnt deliver it on time

@Lychbot
Copy link

Lychbot commented May 20, 2022

Heya @GMSteuart - are you taking on this bounty? if so, i'll assign this to you on Dework if possible. coz I can see contributors applying to this

@MBMaria
Copy link

MBMaria commented May 24, 2022

@GMSteuart can you please advise if you are assigned this bounty? Thank you

@GMSteuart
Copy link

@MBMaria I am! Thanks for checking.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 5500.0 FOX (776.85 USD @ $0.14/FOX) attached to this issue has been cancelled by the bounty submitter

@Lychbot Lychbot added the keepkey-bounty Bounties specific to KeepKey label Jul 12, 2022
@Lychbot Lychbot removed the bounty Issue is posted on dework.xyz as a bounty label Jul 14, 2022
@dework-integration dework-integration bot reopened this Jul 18, 2022
@0xdef1cafe 0xdef1cafe reopened this Jul 19, 2022
@0xdef1cafe
Copy link
Collaborator

reopening - mistakenly closed as walletconnect as a dapp, vs walletconnect as a wallet

@0xdef1cafe 0xdef1cafe changed the title implement WalletConnect (v1) as a wallet option into hdwallet implement WalletConnect (v1) as a dapp option into hdwallet Jul 20, 2022
@0xdef1cafe
Copy link
Collaborator

as discussed with @pastaghost, southbears is working on a different walletconnect feature, where shapeshift is the wallet, rather than the dapp. this is implemented and done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepkey-bounty Bounties specific to KeepKey
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.