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

Enable Solana dApps support by default on desktop #13825

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jun 16, 2022

Resolves brave/brave-browser#23520
security: https://github.com/brave/security/issues/920

performance degradation has been resolved in #13679

Waiting for #13666

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Follow https://github.com/darkdh/solana-provider-test#qa-test
and make sure Solana account is setup

Connect and disconnect

  1. Click "Connect" button
  2. Reject request when prompted, we should see error showing rejected
  3. Click "Eagerly Connect" button
  4. Request should be rejected automatically without asking because it doesn't have permission
  5. Click "Connect" button and grant permission
  6. We should see new entry in brave://settings/content/solana
  7. Also event emitted like [connect] (address)
  8. Click "Disconnect" button, we should see event emitted [disconnect] 👋
  9. Click "Eagerly Connect" button
  10. [connect] (address) should be emitted
  11. Repeat step 1 to 10 with "(Request)" version

Connect and lock

  1. Click "Connect" button and grant permission and disconnect
  2. Lock wallet
  3. Click "Eagerly Connect" button
  4. Request will be rejected because it is locked
  5. Click "Connect" button
  6. And ignore unlock wallet prompt
  7. Click "Connect" button again it will get rejected because it is waiting for previous connect to be unlocked
  8. Unlock the wallet
  9. [connect] address should be emitted

Transactions

Follow test plan in #13666

SignMessage

  1. Connect first
  2. Click "SignMessage" button
  3. Reject the request, we should see [error] signMessage: {"code":4001,"message":"The user rejected the request."}
  4. Click "SignMessage" button again
  5. And we will see signature printed out (compare it with same seed phrase using Phantom, they should be the same)
  6. Click "SignMessage (Hex display)" button and accept the request
  7. Signature result should be same as step 5
  8. Repeat step 1 to 7 with "Request" version

accountChangedEvent

  1. Make sure there are two Solana accounts
  2. Connect first with first account
  3. Select second account and we will see [accountChanged] Switched unknown account
  4. Reject the request and [accountChanged] Failed to re-connect: The user rejected the request. will be shown
  5. Select first account, [accountChanged] Switched account to (account1) emitted
  6. Select second account and grant permission
  7. We should see
[accountChanged] Reconnected successfully
[connect] (account2)

@darkdh darkdh self-assigned this Jun 16, 2022
@darkdh darkdh force-pushed the solana-dapps-default branch from d906db6 to c25cd16 Compare June 21, 2022 22:07
@darkdh darkdh marked this pull request as ready for review June 22, 2022 01:07
@darkdh darkdh requested a review from a team as a code owner June 22, 2022 01:07
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

This is good to go. We've covered the critical concerns and are tracking the non-blocking issues as well from a security perspective. LGTM

Great work on this!

@darkdh
Copy link
Member Author

darkdh commented Jun 22, 2022

web_discovery_project audit failure is not related to this PR
web_discovery_project.py

@darkdh darkdh merged commit 40aa587 into master Jun 22, 2022
@darkdh darkdh deleted the solana-dapps-default branch June 22, 2022 01:16
@darkdh darkdh added this to the 1.42.x - Nightly milestone Jun 22, 2022
yrliou added a commit that referenced this pull request Jun 28, 2022
This reverts commit 40aa587, reversing
changes made to eb936e7.
yrliou added a commit that referenced this pull request Jun 28, 2022
Revert "Merge pull request #13825 from brave/solana-dapps-default"
@darkdh darkdh mentioned this pull request Jun 28, 2022
25 tasks
darkdh added a commit that referenced this pull request Jun 29, 2022
Enable Solana dApps support by default on desktop
darkdh added a commit that referenced this pull request Jun 30, 2022
Enable Solana dApps support by default on desktop
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.

Enable Solana DApps support by default on desktop
3 participants