feat: Add safe wallet support #77
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| if (config.plugins) { | ||
| config.plugins.push( | ||
| new context.webpack.IgnorePlugin({ | ||
| resourceRegExp: /^(lokijs|pino-pretty|encoding)$/, | ||
| }), | ||
| ); | ||
| } | ||
| return config; |
There was a problem hiding this comment.
Fix error in building for nextjs: WalletConnect/walletconnect-monorepo#1908 (comment)
There was a problem hiding this comment.
best if we add a link to this in the file
There was a problem hiding this comment.
i was extremely confused by the UX here
clicking the submit button on this page should not bring the user to
http://localhost:3001/retryables-tracker/{address}, which it currently does
As a user, I am expecting to stay on this page after clicking "Submit" because the address it was showing was initially my MM address because I connected to the site using it first
There was a problem hiding this comment.
"Enter the destination address" is not clear enough for a normie user
I would put Enter which address you want to send the funds to on Arbitrum One
and also add a red warning
"CAUTION: Only put an address you are 100% sure you control on Arbitrum One, or your funds will be lost forever!"
There was a problem hiding this comment.
we should avoid showing the chain id directly to users, and always use network name
| }, | ||
| }; | ||
|
|
||
| const { provider, webSocketProvider } = configureChains( | ||
| const { provider, chains } = configureChains( |
There was a problem hiding this comment.
for some reason i managed to connect via WalletConnect (Sepolia Safe) the first time, but then WC wouldn't show me a QR code very time I tried.... I am not sure what's happening??
There was a problem hiding this comment.
I think it's the same issue as the bridge, I'll clear the localStorage on first load
| @@ -25,8 +39,11 @@ function RecoverFundsButton({ | |||
| }) { | |||
| const [message, setMessage] = useState(''); | |||
| const [loading, setLoading] = useState(false); | |||
| const { isSmartContractWallet } = useAccountType(); | |||
| const { chain } = useNetwork(); | |||
There was a problem hiding this comment.
is this the connected chain or the first chain from the config?? i'm slightly confused about this
There was a problem hiding this comment.
useNetwork return the currently connected chain https://1.x.wagmi.sh/react/hooks/useNetwork
| const { chain } = useNetwork(); | ||
| const { data: signer } = useSigner({ chainId: chain?.id }); | ||
| const { data: signer } = useSigner({ | ||
| chainId: getL1ChainIdFromL2ChainId(chain?.id), |
There was a problem hiding this comment.
why wouldn't we use the chainID prop directly here?
There was a problem hiding this comment.
chainID is the L2 network, and we need to do the transaction on L1
| @@ -25,8 +39,11 @@ function RecoverFundsButton({ | |||
| }) { | |||
| const [message, setMessage] = useState(''); | |||
| const [loading, setLoading] = useState(false); | |||
| const { isSmartContractWallet } = useAccountType(); | |||
There was a problem hiding this comment.
not sure if we need to use isLoading from this to show the UI because users who are on this page probably have a history of not loading isSmartContractWallet quickly
There was a problem hiding this comment.
We only use this flag to show the popup "To continue, please approve tx on your smart contract wallet.", by this time, user had already made a transaction, so most likely the loading has been completed, but I added the loading flag just in case
| aliasedAddress, | ||
| chainId, | ||
| }; | ||
| } catch (e) { |
There was a problem hiding this comment.
If the RPC fails for whatever reason, the page will tell the user that there are no funds to recover, but that might be misleading.
Is there any other reason why the flow would catch an error here?
If a faulty RPC is the only reason, I would maybe log/show an internal error somewhere, instead of returning a 0 balance.
| <div className="flex flex-col"> | ||
| <span> | ||
| <b> | ||
| To continue, please approve tx on your smart contract wallet. | ||
| </b>{' '} | ||
| If you have k of n signers, then k of n will need to sign. | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
nitpick: text appear aligned to the left, not centered like the rest of the texts
There was a problem hiding this comment.
- Performed e2e test on Sepolia and it worked, but after executing the transaction, the page didn't load the success message, it stayed in the "Recover" page, with the button disabled.
- Reviewed the recovering logic and it looks good
- Checked the crafted transaction sent to the Inbox contract and it looks good
- Left two minor non-blocking comment
Summary