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

Migrate from useDapp -> wagmi #42

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Migrate from useDapp -> wagmi #42

merged 6 commits into from
Jan 24, 2022

Conversation

kristencheung
Copy link
Contributor

@kristencheung kristencheung commented Jan 20, 2022

Description

Spent the past couple of days reading, prototyping, and implementing wagmi code and ethers. Code is far from perfect, but the chunkiest and messiest part of this pr is used for the EndorseContributorModal which will mostly be scrapped anyways for the actual endorse flow that reflects the designs.

Overall took away a couple of learnings on how to use the library - I'm mainly comparing against useDapp since I haven't tried other libraries like web3-react. I'll try to point out some of the learnings via github comments too.

How to connect to a wallet without coinbase and metamask opening up:

Multiple wallets opening up is technically a feature✨ to allow a user to choose which provider they would like to use. Since we're presenting the user with options on the UI this doesn't make sense for our use case. To prevent this, we need to select the provider prior to the user connecting their wallet.
Ref: Uniswap/web3-react#300

AutoConnect functionality

autoConnect is an option that can be specified in the wagmi provider to determine whether we want to auto-connect the user every time they come to station. This keeps the user logged in every time they refresh the page or when they leave and come back. I'm pretty sure I've seen some sites use this, but I don't recommend using it. The users's wallet connector is stored in localStorage and is automatically used to read the user's address even if their metamask wallet isn't connected to the site anymore. Also, when the user disconnects and refreshes the page, it will look like they've connected again in the nav bar.

After playing around with the mirror site, it seems like they're also not using the autoConnect option. I'm not connected when I leave the site and come back after a while but upon refreshing, my account is still connected, so I believe they're storing the connection in state somehow.
image

Ref: https://wagmi-xyz.vercel.app/docs/provider#autoconnect-optional

Disconnect functionality

I was under the impression the disconnect function removes the connection to the site which would also be reflected via metamask settings, but that's not the case. Instead "disconnect" makes it so that "the provider will not accept any new requests until the connection to the chain has been re-restablished, which requires reloading the page" (ref: metamask docs). I'm a little confused by what the docs mean when they say "reloading the page" since I can connect my wallet again without refreshing the page if I click "enter station".
image

Also this is an insightful comment about connecting / disconnecting from metamask:

If you're relying just upon connecting for proving identity, your site is insecure, and you should add a signing challenge so the user is forced to prove their identity. The signing is the login, not the connecting. Then logging out is simply a matter of throwing away the signature. Many sites don't have any need to prove identity, so this might not be a problem for you, but it seemed worth highlighting just in case.

Connecting is not about identification, it's about trust. All "Connect" means is that MetaMask should trust the site (with specific permissions/accounts at least). We didn't think to provide a "Disconnect" method, because why would a site want to declare itself as untrustworthy?

It makes me think that the "disconnect" option shouldn't be featured in the nav bar unless we add a signing challenge.

Wagmi pros / cons

Pros:

  • Sensible hook names and standardized hook structure - makes it easy to find what you're looking for
  • Considerate data returned from each hook. For example, when reading a user's balance via useBalance, the data provides a formatted response of the uint256 according to the number of decimals you'd like the uint256 to be formatted to.
  • Flexible hook use case - the hook can call a contract on mount, but it can also return a function to call the contract manually later on. An optional skip argument can be passed if you don't want the data to be called on mount, and a watch option can be passed if you want the hook to monitor each block that's hashed.

Cons:

  • The library was created just this year. It's updated very frequently to fix the initial bugs that have popped up. The author works for mirror though so I'm guessing they use this library as well and have an incentive to maintain it.
  • According to the wagmi docs, apparently web3-react has more connectors, but that doesn't seem like a problem for us for now.

Documentation is decent / an improvement from useDapp. The documentation isn't a replacement for reviewing ethers and metamask though, I've also had to review the source code multiple times to figure out how everything's working. On a good note, the only library/documentation that you do have to worry about is ethers/ metamask rather than researching multiple library dependencies' documentation.

TODO: add rinkeby api key to vercel

@vercel
Copy link

vercel bot commented Jan 20, 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/0xstation/web/9wigfPRGS3Nf3oXEWbGcAG26NytF
✅ Preview: https://web-git-test-wagmi-0xstation.vercel.app

@kristencheung kristencheung changed the title [WIP]: migrate from useDapp -> wagmi Migrate from useDapp -> wagmi Jan 22, 2022
@@ -1,5 +0,0 @@
interface Account {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw this type already exists here, so deleting this one

// Chains for connectors to support
const chains = defaultChains

const provider = ({ chainId }) => new providers.AlchemyProvider(4, process.env.RINKEBY_API_KEY)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rinkeby provider is required to interact with the contracts

const { ethereum } = window

// @ts-ignore
if (!ethereum?.providers) {
Copy link
Contributor Author

@kristencheung kristencheung Jan 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure where providers is coming from 😐 - metamask doesn't define providers on window.ethereum's type . It appears when I have both the metamask and coinbase extension though. Using @ts-ignore for now, lmk if there's a better solution


const Map = () => {
const activeUser: Account | null = useStore((state) => state.activeUser)
const [contributorBoolean, setContributorBoolean] = useState(false)
const [page, setPage] = useState(0)
const [terminals] = useQuery(getTerminals, { suspense: false })
const [terminals] = useQuery(getTerminals, {}, { suspense: false })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason adding the second argument solved the suspense issue I was experiencing

Error: ReactDOMServer does not yet support Suspense.
    at ReactDOMServerRenderer.read (/Users/kristencheung/projects/station-web/node_modules/react-dom/cjs/react-dom-server.node.development.js:3704:25)
    at renderToString (/Users/kristencheung/projects/station-web/node_modules/react-dom/cjs/react-dom-server.node.development.js:4298:27)
    at Object.renderPage (/Users/kristencheung/projects/station-web/node_modules/next/dist/server/render.js:653:31)
    at Function.getInitialProps (webpack-internal:///./node_modules/next/dist/pages/_document.js:193:19)
    at Object.loadGetInitialProps (/Users/kristencheung/projects/station-web/node_modules/next/dist/shared/lib/utils.js:69:29)
    at Object.renderToHTML (/Users/kristencheung/projects/station-web/node_modules/next/dist/server/render.js:676:40)
    at async doRender (/Users/kristencheung/projects/station-web/node_modules/next/dist/server/next-server.js:1133:38)
    at async /Users/kristencheung/projects/station-web/node_modules/next/dist/server/next-server.js:1227:28
    at async /Users/kristencheung/projects/station-web/node_modules/next/dist/server/response-cache.js:60:36
    ```

Copy link
Member

@ilikesymmetry ilikesymmetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I don't really know what the react code is doing but the other stuff checks out. thanks for being so thorough!!

import { useEthers } from "@usedapp/core"
import { useState, useEffect, useMemo } from "react"
import { useAccount, useBalance } from "wagmi"
import { BigNumberish, utils } from "ethers"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lmao "ish" in BigNumberish

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know.. crypto naming man. wagmi.. BigNumberish...

Comment on lines +22 to +25
const [{ data: accountData }] = useAccount({
fetchEns: true,
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a comment describing what happens to accountData if there is an ENS found for the address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's on the type so I'm gonna leave it out for now.. I still need to figure out how to fetch the ens name from rinkeby too

{
  data?: {
    address: string
    connector: Connector
    ens?: {
      avatar?: string
      name: string
    }
  }
  error?: Error
  loading?: boolean
}

Comment on lines +45 to +51
const { read: getAllowance } = useEndorsementTokenRead({
methodName: "allowance",
})

const { write: approveAllowance } = useEndorsementTokenWrite({ methodName: "approve" })

const { loading, write: endorse } = useEndorsementGraphWrite({ methodName: "endorse" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this work? is this creating functions that we can apply later? maybe a comment to describe what's going on bc it looks not-normal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's actually a learning I had from wagmi:

the hook can call a contract on mount, but it can also return a function to call the contract manually later on. An optional skip argument can be passed if you don't want the data to be called on mount, and a watch option can be passed if you want the hook to monitor each block that's hashed.

It's a core characteristic on every wagmi hook so Ima leave it out, but can add if anyone has trouble using it.

I can also add the wagmi docs in the README so people know where to look for more info.

@kristencheung kristencheung merged commit 4fc671d into main Jan 24, 2022
@kristencheung kristencheung deleted the test-wagmi branch January 24, 2022 20:20
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