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: Add tokens to Wallet #133 - USDV 👛 🔵 #169

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

dimorphic
Copy link
Contributor

Issues solved in this PR:

  • Add USDV icon to burger context submenu & to Tokens listing 🔵
  • Update USDV defaults logoURI to use SVG #crips #pixelz

Depends on: PR #168 + future branch feat/enableUSDV to be merged first.

Preview:
Screenshot 2022-01-19 at 02 12 24

Screenshot 2022-01-19 at 02 23 41

…rted!), fix CSS for small screen devices on #DIY instructions & badges, add hover toggle on Token items to set current active token for #DIY, add clipboard 'easy copy-pasta' handler for contract address badge + cleanup
… add popup-request, refactor page to use new Container & Row components #simplify #prettify
…dd route link to /tokens page + hide button in wallet context menu when route matches #minimal
@vercel
Copy link

vercel bot commented Jan 19, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vaderprotocol/vader-dapp/CkAxB33h6dAA837izVBhJTspWZ8W
✅ Preview: https://vader-dapp-git-fork-dimorphic-feat-133-tok-86b205-vaderprotocol.vercel.app

@yellowgh0st
Copy link
Member

yellowgh0st commented Jan 19, 2022

You don't need any of these

src/hooks/useDetectMetamask.js
src/hooks/useDetectProvider.js

because the information you look for is already in

const wallet = useWallet()

@dimorphic
Copy link
Contributor Author

You don't need any of these

src/hooks/useDetectMetamask.js
src/hooks/useDetectProvider.js

because the information you look for is already in

const wallet = useWallet()

Yes & no. It's faster, and i'm using both of them in the app, useWallet will return some false positives while useDetectProvider will not (status, network id miss-matches), and you can mix both, depending on use case.
I've seen the connector in the useWallet, if that's what you mean.

@yellowgh0st
Copy link
Member

useWallet contains everything that you need. You can even find if it's Metamask. The wallet object should be used in all such cases whenever possible. The wallet object is properly propagated thus can be used and updated from anywhere. This might introduce inconsistency between components. We don't use global state management for reason here. Spawning extra timeouts for such minor things is a waste of resources.

…provider info), rename old hook 'anytime' (doesn't require wallet to be connected)
…oken component - can't detect ahead anymore if meta, needs connected status :(
@dimorphic
Copy link
Contributor Author

useWallet contains everything that you need. You can even find if it's Metamask. The wallet object should be used in all such cases whenever possible. The wallet object is properly propagated thus can be used and updated from anywhere. This might introduce inconsistency between components. We don't use global state management for reason here. Spawning extra timeouts for such minor things is a waste of resources.

Hello Comrade,

useWallet is nice, BUT only if you connect your wallet, otherwise it's pretty dummy. I've tried using it initially but saw some limitations in it's implementation, also i don't really need to 'update it' at all in my use-case.

I agree that we should use useWallet for consistency in code & app, and I know that we ain't using any global state for now. Also that's a Promise that resolves with a web3 Provider if it detected in that given 'max timeout'. And if you look closer it's only the Tokens view / page that's using the hook and propagating it down - no multi / blocking timeouts, only 1 promise (with a timed 'reject' - resolve(null) actually, but details), not rly a waste of resources.

The problem with useWallet is: it detects a provider only after connecting a wallet. Thus it can't detect ahead any provider (injected or not) that emits the ethereum#initialized event (eg: MetaMask).

Also: useWallet package is built upon web3-react -> @web3-react/core -> @web3-react/metamask -> @metamask/detect-provider. :)

This is the same package that I used to build the useDetectProvider & useDetectMetamask hooks for us, especially for these corner / special cases: if and when you want to detect or check the provider ahead and not wait for the user to connect the wallet. (thus, bypassing useWallet design limit, but using the same 'tools', but freely - dooby is free?)

Anyways :) For the sake of 'consistency':

  1. I've refactored the useDetectMetamask hook to use useWallet internally
  2. Was thinking in keeping the 2 extra hooks useDetectProvider & useDetectMetaMaskAnytime (renamed now) - just as fallback helpers in the hooks dir, to have... "just in case" ?

For the Ξmpire!

@yellowgh0st : Thoughts ?

# Conflicts:
#	src/components/WalletConnectionToggle.js
#	src/messages.js
@dimorphic
Copy link
Contributor Author

@yellowgh0st : Merged / synced with latest main branch 🏓

@yellowgh0st
Copy link
Member

yellowgh0st commented Feb 3, 2022

Ok

The problem with useWallet is: it detects a provider only after connecting a wallet. Thus it can't detect ahead any provider (injected or not) that emits the ethereum#initialized event (eg: MetaMask).

Ok this is very solid argument. I haven't realized that. It would allow us to do things before they init.

Also: useWallet package is built upon web3-react -> @web3-react/core -> @web3-react/metamask -> @metamask/detect-provider. :)

Yes, I know that's why I have seen it redundant.

@yellowgh0st
Copy link
Member

From a design perspective, I still have an issue with this because having such dedicated space for such a minor feature seems wrong to me. The elements you are using I have been adding with the intention to reserve them for core features so they can stand out.

@yellowgh0st
Copy link
Member

I suggest putting that into the context menu and making it more comprehensive or alternatively spawn a modal window.

@dimorphic
Copy link
Contributor Author

Ok this is very solid argument. I haven't realized that. It would allow us to do things before they init.

Indeed ;) #iddqd #cheats

From a design perspective, I still have an issue with this because having such dedicated space for such a minor feature seems wrong to me. The elements you are using I have been adding with the intention to reserve them for core features so they can stand out.

Aren't tokens a core feature? No tokens, no nada? Computer says no? I took your advice and remove it from the top menu, to keep that one clean, as I also agree with keeping things clean & simple. But I feel the 'space' is needed for the tokens, so you can add them fast to your wallet (Metamask via add buttons) or copy-paste #doityourself mode for other Web3 wallets.

I suggest putting that into the context menu and making it more comprehensive or alternatively spawn a modal window.

It is in the context menu, but instead of 3 buttons (for each token), they are grouped under 1 button that opens the /tokens route:

Screenshot 2022-01-19 at 02 12 24

The reason(s) for this and why not a modal or 3 tokens buttons (thought of that initially) :

  1. Why not 3 buttons : add to wallet buttons work only in MetaMask, otherwise (and most ppl) will do it manually #doityourself way. I rather help them find the info about the tokens in the Vader app (as in official info), so they don't jump to CMC or Gecko or Etherscan or whatever and search for each one of them.
  2. Why not a modal :
    1. We already have too many 'pop' like things: hamburger menu, context menu, notifications popping. Modal would be overkill for the UX, feels like too many 'levels' Would rather keep the as the 'last resort' for future stuff, not this.
    2. MOST IMPORTANT: having a /tokens route is maximum flexibility + predictable + SHARE-able with infidel friends that you're trying to convert to the Dark Side 🍪 :)

¯\_(ツ)_/¯

@dimorphic
Copy link
Contributor Author

@yellowgh0st : Ship it, Captain? :shipit: 🚀

@vercel
Copy link

vercel bot commented May 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
vader-dapp ❌ Failed (Inspect) May 19, 2022 at 9:25PM (UTC)

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

Successfully merging this pull request may close these issues.

2 participants