-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add support for the Dynamic(xyz) wallet library #2448
Open
rdig
wants to merge
44
commits into
master
Choose a base branch
from
library/992-support-multiple-wallets
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+12,885
−4,631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
from
May 28, 2024 23:19
61596c5
to
f1b1ce6
Compare
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
2 times, most recently
from
June 19, 2024 12:49
4c427d6
to
c594bc9
Compare
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
2 times, most recently
from
July 17, 2024 14:12
80deaea
to
87770b6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
from
December 4, 2024 11:49
87770b6
to
9293d7e
Compare
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
2 times, most recently
from
January 22, 2025 01:08
1996d64
to
e03ed93
Compare
rdig
changed the title
Support Multiple Wallet Types
Add support for Dynamic wallet library system
Jan 22, 2025
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
from
January 22, 2025 14:07
4ca5a7e
to
ab285ec
Compare
rdig
changed the title
Add support for Dynamic wallet library system
Add support for the Dynamic(xyz) wallet library system
Jan 23, 2025
rdig
changed the title
Add support for the Dynamic(xyz) wallet library system
Add support for the Dynamic(xyz) wallet library
Jan 23, 2025
rdig
force-pushed
the
library/992-support-multiple-wallets
branch
from
January 23, 2025 12:35
71fb902
to
a5b5395
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
NOTE: The description was updated with a section on Brave |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces CDapp support for multiple wallet types, trough the use of the Dynamic wallet library / SaaS.
As with
web3-onboard
which we were using previously, it allows wallet abstraction through use of proprietary interfaces and classes removing the need to interact with the specific wallets directly.Due to it being inherently a SaaS, it requires a always-on connection and a API key, meaning it makes running it locally kind of problematic (but not impossible)
Besides the expect and available wallet types, it also has support for custodial wallets (Embedded wallets in Dynamic parlance), which means that you can use a OAuth logic (Google, Github) and have a wallet created automatically for you. This is useful for less web3 savy users.
Improving from last wallet implementation iteration, I've took the opportunity to fix live network and wallet address changes which didn't use to work properly.
Now, we have cases to handle both changing of the user's wallet network, as well as changing the wallet's current address
Screencast.from.2025-01-23.15-38-09.mp4
Milestones
This is part of two milestones related to Wallet support, this being Milestone 0, (M0) from now on.
It is also to be considered the feature branch as, even after deploying into production I have no plans of merging this back into
master
until M1 will be merged back into this.M1 deals with refactoring
web3-onboard
to function as a fallback for the Dynamic wallet system in cases where this is either ran locally, the connection to the dynamic servers fails, or someone wants to run the CDapp trully decentralized.It will also aid in local development as it will bring back software wallets which can auto sign transactions and messages easing the time it takes to test various paths (this is the main reason for not wanting to merge this)
Rabbi Wallet Type
One the most requested new wallet type seems to be Rabbi for some reason, however it does not come without it's issues.
First, it does not have support out of the box for testnets, meaning something like local development and Arbitrum Sepolia will have to be manually added by the user (mainnets like Arbitrum are fine though)
However, one of it's biggest issues is the fact that if the connect popover it spans (which seems to be different from all others) looses focus, like via moving your mouse cursor, it will cancel the connection giving you a instant failure message.
It's made even worse by users running Window Managers which tent to focus on hover, making this problem even worse:
Screencast.from.2025-01-23.12-42-31.mp4
It is not only something that we noticed, as it seems to be tracked via Rabbi's github issues: RabbyHub/Rabby#1920 (which seems to be opened for more than a year at this point)
However, any other popover that Rabbi opens (signing a message, signing a transaction, etc) will work just fine out of focus:
Screencast.from.2025-01-23.12-57-35.mp4
Custodial Wallet (Embedded Wallet)
Custodial wallets are different not only because the user is not in control of them, but also since they don't have their own interface, so one must be provided for them (to change networks, send funds, sign, etc)
Due to this, we now have an extra entry in the User Hamburger menu which essentially gives the user access to the wallet interface whenever they require it
Brave Browser/Wallet/Bitski
Two things I must highlight regarding using the Brave Browser, and inherently it's wallet to connect and use the CDapp.
For one, the Brave Wallet is actually the Bitski under the hood (which was recently acquired by Phantom), but with a different interface, meaning both "Brave Wallet" and "Bitski" will show up as installed when using Brave
Second, developing locally via the Brave/Bitski Wallet directly is currently not possible as due to the security features of Brave, it expects a secure connection to both the frontend and the RPC endpoint, while our local dev environment runs on a standard one.
Using another wallet, like Metamask, through Brave should be possible though.
Implementation Quirks
There's two issues that stood out as more "problematic" during this implementation.
One being that, even though the documentation clearly lists it out https://docs.dynamic.xyz/react-sdk/hooks/usedynamicevents it will not trigger the listener(s) for wallet change events (
primaryWalletChanged
). Due to this, I had to manually set up a makeshift listener by making use ofuseEffect
.The second issue is due to the high level that the
DynamicContextProvider
context needs to be in the rendering hierarchy, it does not have access to the CDapp's light/dark themes from context. For this, I had to tap back intolocalStorage
to fetch the state, that is already set in there byPageThemeContextProvider
. This is basically double dipping.A third is that due to the CDapp only supporting
[email protected]
and dynamic only supporting[email protected]
I had to use an older package, which means there's a bunch of dependency overrides in the tree, which generates a lot of npm install warningsLive Debugging
For live debugging you can enable the
__DYNAMIC_WALLET_DEBUG_ENABLED__
variable in yourlocalStorage
which willRunning Locally
As I mentioned above, this is really not intended to be run locally for development, even though it can be.
To achieve this, you'll need to add a new variable to your
.env.local.secrets
: (ping me for the sandbox key)After which you'll need to run
npm run prepare
.Once this is set up, you can run you local environment as before, however you'll only have access to non-dev wallets, so I suggest adding the ganache development wallets in Metamask and use those.
They can be found at: http://localhost:3006/ganache-accounts.json
Alternatively, generate new wallets from any of the wallet types available, and use one of the development wallets to send funds into it, however, this will only be effecting for the current testing session.
Testing
Due to it working the same both locally, on QA, or in Production, the testing procedure is the same, just with the network being changed (so I suggest, for ease of use, to actually test it in a live environment -- QA)
Prerequisites if running this locally:
.env.local.secrets
file with the newDYNAMIC_ENV_ID
env variablenpm run prepare
npm run dev
node scripts/create-data.js
npm run frontend
Actual testing:
Thanks
As with all my recent PRs, special thanks go out to @jakubcolony for helping the issue with click event bubbling when using Embedded wallets, and @area for helping debug Rabbi's "weirdness"
Resolves #992
Resolves #1022
Resolves #1037
Resolves #2796
Resolves #4184
Resolves #4177
Resolves #4188