Added notifications for Confirmed, Error and Dropped status changes #13330

spylogsster · 2022-05-13T12:46:23Z

Resolves brave/brave-browser#19885

Added WalletNotificationService to show notifications about transactions on UI level.

demo.mp4

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:

Make transaction and check notification for Confirmed and Error statuses

yrliou · 2022-05-13T15:43:20Z

browser/brave_wallet/BUILD.gn

@@ -36,7 +38,6 @@ source_set("brave_wallet") {
    "//services/network/public/cpp",
    "//third_party/abseil-cpp:absl",
  ]
-


Please remove this unrelated change.

yrliou · 2022-05-16T18:05:23Z

browser/brave_wallet/brave_wallet_provider_delegate_impl_helper.cc

@@ -14,7 +14,7 @@

 namespace {

-base::OnceCallback<void()> g_NewSetupNeededForTestingCallback;
+absl::optional<bool> g_NewSetupNeededForTesting;


What's the reason of not just using boolean here?

there is condition of existance on 39th line, replaced by optional to keep same logic

Seems unnecessary, I think a boolean value initialized as false and just check the boolean value is what we needed.

this variable name also needs to be underscore

browser/brave_wallet/sources.gni

yrliou · 2022-05-16T18:11:20Z

browser/sources.gni

@@ -102,8 +103,8 @@ brave_chrome_browser_deps = [
  "//brave/browser:browser_process",
  "//brave/browser/brave_federated",
  "//brave/browser/brave_wallet",
+  "//brave/browser/brave_wallet",


duplicate entry

yrliou · 2022-05-16T18:14:33Z

test/BUILD.gn

@@ -88,6 +88,7 @@ test("brave_unit_tests") {
    "../../components/domain_reliability/test_util.h",
    "//brave/browser/brave_content_browser_client_unittest.cc",
    "//brave/browser/brave_resources_util_unittest.cc",
+    "//brave/browser/brave_wallet/wallet_notification_service_unittest.cc",


Any reasons that it has to be added to this target instead of some wallet test target? In general we don't want to add to this huge target seems it's hard to manage.

I couldn't find any mention of unittests in underlaying sources.gni files, seems this is only way now to add test for this on UI level

yrliou · 2022-05-16T21:32:50Z

browser/brave_wallet/wallet_notification_service.h

+  explicit WalletNotificationService(content::BrowserContext* context,
+                                     TxService* tx_state_manager);


s/tx_state_manager/tx_service

yrliou · 2022-05-16T21:41:17Z

browser/brave_wallet/wallet_notification_service.cc

+    content::BrowserContext* context,
+    TxService* tx_service)
+    : tx_service_(tx_service), context_(context) {
+  tx_service_->AddObserver(tx_observer_receiver_.BindNewPipeAndPassRemote());


This raw_ptr doesn't seem to be needed to be stored if all we need is the AddObserver call here.

yrliou · 2022-05-16T21:43:58Z

browser/brave_wallet/wallet_notification_service.h

+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/remote.h"


These two seems not needed in this header file, and the include for mojo::Receiver is missing.

browser/brave_wallet/wallet_notification_service.cc

yrliou · 2022-05-16T21:54:28Z

browser/brave_wallet/brave_wallet_tab_helper.cc

+  brave_wallet::WalletNotificationServiceFactory::GetServiceForContext(
+      web_contents->GetBrowserContext());


I'm a bit confused about this. Could you explain what's this for? Triggering service creation? How is it related to the tab helper?

yes, this is only to trigger service creation, it needs to be done on UI level for browser context

bridiver · 2022-05-18T18:02:56Z

browser/brave_wallet/wallet_notification_service.cc

+namespace brave_wallet {
+
+WalletNotificationService::WalletNotificationService(
+    content::BrowserContext* context,


do not pass in the BrowserContext, pass in NotificationDisplayService from WalletNotificationServiceFactory

I need BrowserContext inside WalletNotificationService

bridiver · 2022-05-18T18:23:39Z

browser/brave_wallet/sources.gni

+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at http://mozilla.org/MPL/2.0/.
+
+brave_browser_brave_wallet_sources = [


so based on dm discussion brave_wallet_tab_helper shouldn't need to be in sources.gni anymore so I would move it back to BUILD.gn and then create a subdir for notifications so we don't have sources.gni and BUILD.gn in the same directory

yrliou · 2022-05-19T17:01:24Z

test/BUILD.gn

@@ -312,6 +313,7 @@ test("brave_unit_tests") {
    sources += [
      "../utility/importer/chrome_importer_unittest.cc",
      "//brave/app/brave_command_line_helper_unittest.cc",
+      "//brave/browser/brave_wallet/notifications/wallet_notification_service_unittest.cc",


This should not be added to this huge target, should be added to a wallet test target.

yrliou

LGTM

spylogsster added this to the 1.40.x - Nightly milestone May 13, 2022

spylogsster requested a review from yrliou May 13, 2022 12:46

spylogsster requested a review from a team as a code owner May 13, 2022 12:46

spylogsster self-assigned this May 13, 2022

spylogsster marked this pull request as draft May 15, 2022 21:07

spylogsster changed the title ~~Added notifications for Confirmed and Error status changes~~ wip: Added notifications for Confirmed and Error status changes May 15, 2022

spylogsster force-pushed the notification branch 3 times, most recently from 0636bb1 to e8dd2ee Compare May 15, 2022 21:55

spylogsster added the CI/run-draft label May 15, 2022

spylogsster force-pushed the notification branch 7 times, most recently from ca21ac7 to c740887 Compare May 16, 2022 15:03

spylogsster changed the title ~~wip: Added notifications for Confirmed and Error status changes~~ Added notifications for Confirmed and Error status changes May 16, 2022

spylogsster marked this pull request as ready for review May 16, 2022 15:03

spylogsster force-pushed the notification branch 4 times, most recently from 393d7a0 to 7654dfc Compare May 16, 2022 20:43

yrliou reviewed May 16, 2022

View reviewed changes

spylogsster force-pushed the notification branch from 7654dfc to 4bdd9d1 Compare May 17, 2022 07:52

spylogsster changed the title ~~Added notifications for Confirmed and Error status changes~~ Added notifications for Confirmed, Error and Dropped status changes May 17, 2022

spylogsster force-pushed the notification branch 4 times, most recently from 59e6ff2 to acab33c Compare May 18, 2022 09:10

spylogsster force-pushed the notification branch 3 times, most recently from 3e836ff to 414a2c2 Compare May 18, 2022 11:28

bridiver reviewed May 18, 2022

View reviewed changes

spylogsster force-pushed the notification branch from 414a2c2 to 9212cea Compare May 18, 2022 18:10

bridiver reviewed May 18, 2022

View reviewed changes

spylogsster force-pushed the notification branch 3 times, most recently from adb4682 to 6440f59 Compare May 19, 2022 15:23

yrliou reviewed May 19, 2022

View reviewed changes

spylogsster force-pushed the notification branch from 6440f59 to 824dd88 Compare May 19, 2022 17:52

yrliou approved these changes May 19, 2022

View reviewed changes

spylogsster force-pushed the notification branch from 824dd88 to d15297e Compare May 19, 2022 18:26

Added WalletNotificationService to show notifications about transactions

c57093b

spylogsster force-pushed the notification branch from d15297e to c57093b Compare May 20, 2022 06:30

spylogsster merged commit 544dfd7 into master May 20, 2022

spylogsster deleted the notification branch May 20, 2022 08:21

spylogsster modified the milestones: 1.40.x - Beta, 1.41.x - Nightly May 20, 2022

srirambv mentioned this pull request Jul 1, 2022

Add notifications for successful / failed transactions brave/brave-browser#19885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added notifications for Confirmed, Error and Dropped status changes #13330

Added notifications for Confirmed, Error and Dropped status changes #13330

spylogsster commented May 13, 2022 •

edited

Loading

yrliou May 13, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 18, 2022

bridiver May 18, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

yrliou May 16, 2022

spylogsster May 17, 2022

bridiver May 18, 2022

spylogsster May 19, 2022

bridiver May 18, 2022

spylogsster May 19, 2022

yrliou May 19, 2022

spylogsster May 19, 2022

yrliou left a comment

		explicit WalletNotificationService(content::BrowserContext* context,
		TxService* tx_state_manager);

		#include "mojo/public/cpp/bindings/pending_remote.h"
		#include "mojo/public/cpp/bindings/remote.h"

		brave_wallet::WalletNotificationServiceFactory::GetServiceForContext(
		web_contents->GetBrowserContext());

Added notifications for Confirmed, Error and Dropped status changes #13330

Added notifications for Confirmed, Error and Dropped status changes #13330

Conversation

spylogsster commented May 13, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou left a comment

Choose a reason for hiding this comment

spylogsster commented May 13, 2022 •

edited

Loading