feat: WalletConnect integration, part 1, session proposal #1959
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
6f70f09 to
021d090
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
021d090 to
6ff0ebf
Compare
There was a problem hiding this comment.
Thank you for the changes @dianasavvatina
I've added some more comments
There was a problem hiding this comment.
Would be good to check existing modals for dApp requests and see what changes we need for the wallet connect implementation (so that designers could start working on that in advance)
This is the latest design for beacon requests that we have: https://www.figma.com/design/l7N0RuVTDuZd2jP77lNqhU/Web-%26-Embed?node-id=6059-103695&node-type=canvas&t=Sc3FdyWv3zI26Sf4-0
There was a problem hiding this comment.
no access to it yet. requested
There was a problem hiding this comment.
It's more of a note for the future, for now I think we should keep the current design and focus on the integration bit 🙂
| function getChainToConfirm(required: (string | string[] | undefined)[]) { | ||
| // take the first required chain which is supported by the wallet | ||
| for (const chain of required.flat()) { | ||
| if (chain && ["tezos:mainnet", "tezos:ghostnet"].includes(chain)) { |
There was a problem hiding this comment.
We should probably use the actual list of networks here by calling useAvailableNetworks()
Also would be better to check by urls not by names - (mainnet & ghostnet are default, but for other networks user can select any name they like)
Could be a TODO for the following changes
There was a problem hiding this comment.
dApp in WalletConnect sends the names of the chains. How can they be selected by url? let's take it next steps
There was a problem hiding this comment.
@dianasavvatina does it mean it's always just "tezos:mainnet" and/or "tezos:ghostnet" for tezos? Could it be parisnet for example?
There was a problem hiding this comment.
If it's just tezos:mainnet & tezos:ghostnet - then there is no need for the check (for now, while we only support tezos). These two networks are always defined in Umami and can't be edited by users. We just need to decide on behaviour in the case of request with multiple chains
If there could be other network options we need to figure out how to check if they're present or not.
Names for other (custom) networks are coming from the user and might not match WalletConnect names.
There was a problem hiding this comment.
it could be anything, but it must match the config on the wallet.
This is how the proposal looks like:
| supportedNamespaces: { | ||
| tezos: { | ||
| chains: [chain], | ||
| methods: ["tezos_getAccounts", "tezos_sign", "tezos_send"], |
There was a problem hiding this comment.
Another product question: maybe we can allow user to select which actions they want to allow for the given dApp?
There was a problem hiding this comment.
maybe, but that's a new feature which nobody has requested. I suggest to leave it until somebody explicitly wants it
There was a problem hiding this comment.
Agree, definitely not needed for this PR
But could be good to discuss it with Aswin and maybe create add a backlog task for later (so that it's not get lost)
| <Text as="span" fontWeight="bold"> | ||
| {name} | ||
| </Text>{" "} | ||
| <br /> |
There was a problem hiding this comment.
Please remove <br />
| export default function VerifyInfobox() { | ||
| return ( | ||
| <Box textAlign="center"> | ||
| <VStack spacing={4}> |
There was a problem hiding this comment.
please use real pixel values instead. here and in all the other places
| }: Props) { | ||
| const form = useFormContext(); | ||
| return ( | ||
| <Box as="footer" padding={4}> |
There was a problem hiding this comment.
Since we are using pixels (px) everywhere it's better to use px instead of Chakra spacing unit here and in the other places too.
1 spacing unit = 4px
| {name} | ||
| </Text>{" "} | ||
| <br /> | ||
| <Heading size="md">wants to {intention ? intention : "connect"}</Heading> |
There was a problem hiding this comment.
intention ? intention : "connect" could be simplified to intention ?? "connect"
| </Link> | ||
| </Box> | ||
| <Flex alignItems="center" justifyContent="center" marginTop={4}> | ||
| <PencilIcon style={{ verticalAlign: "bottom" }} /> |
There was a problem hiding this comment.
<PencilIcon style={{ verticalAlign: "bottom" }} /> is better to replace with => <Icon as={< PencilIcon />} vericalAlign="bottom" /> where Icon is also a component from chakra-ui
| disableApprove, | ||
| disableReject, | ||
| }: IProps) { | ||
| const modalContent = useMemo( |
There was a problem hiding this comment.
useMemo is unnecessary here, so it can be removed, and this component can be simplified to just returning modalContent
| } | ||
|
|
||
| /** | ||
| * Component |
There was a problem hiding this comment.
This comment could be removed
| return chain; | ||
| } | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
return undefined is the same as return; so explicit undefined here is redundant
| }: { | ||
| proposal: WalletKitTypes.SessionProposal; | ||
| }) { | ||
| console.log("SessionProposalModal", proposal); |
There was a problem hiding this comment.
Do we need console.log here and in other places?
| const onReject = () => | ||
| handleAsyncAction(async () => { | ||
| try { | ||
| setIsLoadingReject(true); |
There was a problem hiding this comment.
setIsLoadingReject could be moved to finally block after catch inside handleAsyncAction
the same could be applied to the approve logic
| namespaces, | ||
| sessionProperties: {}, | ||
| }); | ||
| onClose(); |
There was a problem hiding this comment.
No need to call onClose() here since it is called on the line 107
the same thing in the reject logic
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
6ff0ebf to
39da325
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
39da325 to
22d7d7c
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
22d7d7c to
275ec93
Compare
| import { Card, ModalBody, ModalHeader } from "@chakra-ui/react"; | ||
| import { Fragment, type ReactNode } from "react"; | ||
|
|
||
| type IProps = { |
There was a problem hiding this comment.
type IProps => type Props
|
|
||
| export const RequestModalContainer = ({ children, title }: IProps) => ( | ||
| <Fragment> | ||
| {title ? ( |
There was a problem hiding this comment.
{title && (
{title}
)}
| children: ReactNode | ReactNode[]; | ||
| }; | ||
|
|
||
| export const RequestModalContainer = ({ children, title }: IProps) => ( |
There was a problem hiding this comment.
this component is not needed. inline its only usage
| }, | ||
| }, | ||
| }); | ||
| console.debug("approvedNamespaces", namespaces); |
| void initializeWallet(); | ||
| }); | ||
|
|
||
| const onSessionProposal = async (event: WalletKitTypes.SessionProposal) => { |
There was a problem hiding this comment.
this and the next functions do not provide much value, just inline
| <ModalFooter> | ||
| <Button | ||
| width="100%" | ||
| isDisabled={false} |
There was a problem hiding this comment.
this is not needed. it's not disabled by default
| <Button | ||
| width="100%" | ||
| isDisabled={!!error || !isValid} | ||
| isLoading={isLoadingApprove} |
There was a problem hiding this comment.
why do we have separate loading states for those?
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
275ec93 to
000c6e6
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
c66b40d to
8770e10
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
8770e10 to
8e7e459
Compare
dianasavvatina
force-pushed
the
add-wc-connect
branch
from
8e7e459 to
9d8ec00
Compare
This is the first part of the WalletConnect integration. It includes the following components:
Limitations:
Proposed changes
Task link
Types of changes
Steps to reproduce
Rejecting
actions: on dApp - connect, copy link. on Wallet: Connect, reject
result: on Wallet - Approve button is inactive. On reject - modal is closed immediately. on dApp: modal is closed
Approving
actions: on dApp - connect, copy link. on Wallet: on Wallet: Connect, select Account, Approve
result: on dApp - connected, the list of actions is shown. on Wallet: the modal is closed
actions: on dApp - request transaction
result: on dApp - transaction is immediately rejected with USER rejected error
To be covered in the next PR, #1985:
Screenshots
All requests are rejected with
Checklist