-
Notifications
You must be signed in to change notification settings - Fork 972
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 Brave Wallet #445
Add Brave Wallet #445
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Please don't copy the Phantom adapter's behavior though.
I also want to mention that we're working on an actual wallet standard, which will be introduced soon. More context on that here and here.
When that's ready for review, I'd love to get your feedback and work on a migration plan.
86ddf28
to
94a6cae
Compare
I'd love to review this again but the new force pushes caused the context of the review comments to all be lost. Please don't do that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, last issue is something I just discovered -- signMessage
throws the wrong error type.
Thanks so much! I'll plan to release this tomorrow. |
if (!wallet) throw new WalletNotConnectedError(); | ||
|
||
try { | ||
return (await wallet.signTransaction(transaction)) || transaction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would you want to return the original transaction? Is this in cases where it's already signed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't never happen even when input transaction is signed.
Note that most of the code is inherited from Phantom adapter including this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! If Brave's wallet.signTransaction
can never return null
, you should uninherit this part :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed here
Resolve brave/brave-browser#19468
Brave has both
solana.isPhantom
andsolana.isBraveWallet
set to true for maximum compatibilityBrave Wallet for Solana dApp support is only available in Nightly