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: WalletConnect, prototype #1933

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dianasavvatina
Copy link

Working WalletConnect integration PROTOTYPE with Umami:

  • listing dApps connected with WalletConnect
  • tezos_getAccounts returns the current account
  • tezos_sign signs payload with the current account
  • tezos_send supports all perations:
    • transaction signing
    • delegate / undelegate
    • origination, calling smart contract
    • stake, unstake, finalize
  • approve and reject by user
  • success and error from Tezos node

Study:

Limitations:

  • the operation result is not shown to the user
  • pairings list doesn't work on remote disconnect
  • no tests
  • no documentation
  • several lint errors

Proposed changes

This is a prototype which is not intended to be Merged.
We are going to use it as a reference for a full implementation.

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

  • run Umami web
  • run test dapp, e.g. WalletConnect example react-dapp-v2

Screenshots

Add the screenshots of how the app used to look like and how it looks now

image image image image image

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

Copy link

vercel bot commented Sep 23, 2024

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

Name Status Preview Comments Updated (UTC)
umami-v2-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:13pm
umami-v2-web-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:13pm

@dianasavvatina dianasavvatina changed the title feat: WalletConnect, first version feat: WalletConnect, prototype Sep 23, 2024
Working WalletConnect integration with Umami:
 - listing dApps connected with WalletConnect
 - tezos_getAccounts returns the current account
 - tezos_sign signs payload with the current account
 - tezos_send supports all perations:
   - transaction signing
   - delegate / undelegate
   - origination, calling smart contract
   - stake, unstake, finalize
 - approve and reject by user
 - success and error from Tezos node

Limitations:
 - the operation result is not shown to the user
 - pairings list doesn't work on remote disconnect
 - no tests
 - no documentation
 - several lint errors
*/
export default function PairingCard({ name, url, topic, onDelete }: IProps) {
return (
<Card className="relative mb-6 min-h-[70px] border border-light">
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use classNames in the app. please check out chakra docs


import PairingCard from "./PairingCard"

export default function PairingsPage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't use default exports and function declarations like this. instead please use fatarrow functions like

export PairingsPage = () => {...}

const { pairings } = useSnapshot(SettingsStore.state)
// const [walletPairings ] = useState(web3wallet.core.pairing.getPairings())

async function onDelete(topic: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here as above

.map(chain => {
const chainData = getChainData(chain!);

if (!chainData) {return null;}
Copy link
Contributor

Choose a reason for hiding this comment

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

please add prettier and eslint extensions to your VSCode

new Serializer()
.deserialize(payload)
.then(parsePeerInfo)
.then(peer => WalletClient.addPeer(peer as ExtendedPeerInfo))
.then(() => refresh())
.catch(e => {
toast({
description:
"Beacon sync code in the clipboard is invalid. Please copy a beacon sync code from the dApp",
description: `Beacon sync code in the clipboard is INVALID. Please copy a beacon sync code from the dApp: ${payload}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't do that. it might be huge and doesn't really help the end user

@@ -101,16 +101,15 @@ export const useAddPeer = () => {
const { refresh } = usePeers();
const toast = useToast();

return (payload: string) =>
return async (payload: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary as we don't use the awaits here

@@ -76,6 +80,7 @@
"react-test-renderer": "^18.3.1",
"redux": "^5.0.1",
"redux-persist": "^6.0.0",
"valtio": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sure we don't need it. most likely you need to use redux that's already included

/**
* Types
*/
interface IProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use interfaces, type is preferred

* Types
*/
interface IProps {
name?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

why all the props are optional?

/**
* Store / Actions
*/
export const SettingsStore = {
Copy link
Contributor

Choose a reason for hiding this comment

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

this whole thing seems to be either already accessible or it could replicate what's already in the beaconSlice

export default function ChainDataMini({ chainId }: Props) {
const chainData = useMemo(() => getChainData(chainId), [chainId]);

if (!chainData) {return <></>;}
Copy link
Contributor

Choose a reason for hiding this comment

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

return null

import { Card, CardBody } from "@chakra-ui/react";
import { type ReactNode } from "react";

interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

this interface can be replaced with PropsWithChildren<CardProps>

const { request, chainId } = params;

// Handle approve action (logic varies based on request method)
const onApprove = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

most of its loading & error handling logic is already in asyncActionHandler

import { type Web3WalletTypes } from "@walletconnect/web3wallet";
import { proxy } from "valtio";

interface ModalData {
Copy link
Contributor

Choose a reason for hiding this comment

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

if I understand the purpose correctly, you can achieve the same using useDynamicModalContext hook

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