feat: update+notifications for console wallet #3284

Cifko · 2021-09-02T06:18:52Z

Add general notifications support for console wallet.
Add update notification to the wallet.

Description

The wallet now supports notifications. In case of notification the notification tab will change color and show number of notifications so the users knows there is something. Once you sees them the counters goes to zero and the color to white.

Motivation and Context

It's the preparation for the auto update feature. Currently it's just notification for the update availability.

How Has This Been Tested?

The general notifications was tested by adding notification from the base node change.
The auto update was not tested yet.

delta1 · 2021-09-02T07:38:30Z

Testing

sdbondi

Looks good (utAck) - few clippys

integration_tests/features/support/steps.js

applications/tari_console_wallet/src/ui/components/component.rs

applications/tari_console_wallet/src/ui/state/wallet_event_monitor.rs

applications/tari_console_wallet/src/ui/state/app_state.rs

integration_tests/features/BaseNodeAutoUpdate.feature

delta1

Happy in general, left a few minor comments, I also created some TXT records to test with and saw an available update in the logs, but it did not get written to the notification screen - presumably because I didn't do the whole signature verification part.

2021-09-02 11:00:53.817714000 [p2p::auto_update::dns] [Thread:123145565605888] DEBUG Checking [redacted] for updates...
2021-09-02 11:00:53.817700000 [p2p::auto_update::dns] [Thread:123145565605888] DEBUG Checking [redacted] for updates...
2021-09-02 11:00:53.825693000 [p2p::auto_update::dns] [Thread:123145552949248] DEBUG Update found! app = Tari Console Wallet, arch = macos-x86_64, version = 99.8.12, hash = 6e77da12535e0ed7851761ae6c6729f2dd3ae8050840e8c08e10e279dadfde8a
2021-09-02 11:00:53.825740000 [p2p::auto_update] [Thread:123145552949248] DEBUG New unverified update found (app = Tari Console Wallet, arch = macos-x86_64, version = 99.8.12, hash = 6e77da12535e0ed7851761ae6c6729f2dd3ae8050840e8c08e10e279dadfde8a). Verifying...
2021-09-02 11:00:53.825721000 [p2p::auto_update::dns] [Thread:123145552949248] DEBUG Update found! app = Tari Console Wallet, arch = macos-x86_64, version = 99.8.12, hash = 6e77da12535e0ed7851761ae6c6729f2dd3ae8050840e8c08e10e279dadfde8a
2021-09-02 11:00:53.825753000 [p2p::auto_update] [Thread:123145552949248] DEBUG New unverified update found (app = Tari Console Wallet, arch = macos-x86_64, version = 99.8.12, hash = 6e77da12535e0ed7851761ae6c6729f2dd3ae8050840e8c08e10e279dadfde8a). Verifying...

applications/tari_console_wallet/src/ui/state/wallet_event_monitor.rs

applications/tari_console_wallet/src/ui/components/notification_tab.rs

delta1 · 2021-09-02T08:17:57Z

applications/tari_console_wallet/src/ui/components/notification_tab.rs

+
+    fn draw_notifications<B>(&mut self, f: &mut Frame<B>, area: Rect, app_state: &AppState)
+    where B: Backend {
+        let block = Block::default().borders(Borders::ALL).title(Span::styled(


Minor: I think it would be good to have some kind of explanation above the block but that could be added later if we implement notifications for other things too

I've added some explanation at the top of the file. So that everyone can easily see the proper file where the add_notification is called. Let me know if you think it's ok, or should change it.
I also added some TODOs (from top of my head) that we can implement in the future. If you have any ideas for other TODOs let me know, or you can create PR after this one.

Sorry, I meant in the UI above the notifications block, info for the user - but it's not critical

Oh, I see. To me the "notifications" is self-explanatory, but as a programmer I'm biased. But we should definitely be very careful with the message that we added. We can add the explanatory on the top later when will have more types of notification, not just update :-)

delta1 · 2021-09-02T08:24:01Z

applications/tari_console_wallet/src/init/mod.rs

@@ -360,6 +361,15 @@ pub async fn init_wallet(
        config.base_node_event_channel_size,
    );

+    let updater_config = AutoUpdateConfig {
+        name_server: config.dns_seeds_name_server,
+        update_uris: config.autoupdate_dns_hosts.clone(),


Please check that all the config options are noted in tari_config_example.toml

I suppose so, because it's used in base node :-) But I will double check.

Some of them were missing, @sdbondi can you please review the tari_config_example.toml changes?

delta1 · 2021-09-02T09:32:49Z

A nice followup would be to add notifications for sent transaction, received transaction, coinbase output won, coinbase output matured, etc.

sdbondi

Lightly tested - looks good - not sure what UX discussions were had around this feature, but feels like a whole tab for updates is overkill. Something like Version: 0.9.6 (New version: 0.9.7) in the footer should be enough?

EDIT: A general purpose notifications area as @delta1 says also sounds good - will wait for tests to pass to approve because I think there will still be updates due to the changed fn signature on Wallet::new

applications/tari_console_wallet/src/ui/components/notification_tab.rs

Cifko · 2021-09-02T09:53:09Z

Lightly tested - looks good - not sure what UX discussions were had around this feature, but feels like a whole tab for updates is overkill. Something like Version: 0.9.6 (New version: 0.9.7) in the footer should be enough?

EDIT: A general purpose notifications area as @delta1 says also sounds good - will wait for tests to pass to approve because I think there will still be updates due to the changed fn signature on Wallet::new

Yes, it's currently used only for updates. But the purpose is general. You can add add_notifcation in the wallet_event_monitors to any events you like. I did test the notification behavior by adding the state of the base node. Yeah, I suppose the tests will reveal some issues.

Cifko · 2021-09-02T09:55:57Z

A nice followup would be to add notifications for sent transaction, received transaction, coinbase output won, coinbase output matured, etc.

That would be nice. Maybe we can discuss this today.

delta1 · 2021-09-02T18:54:52Z

Some test failures

Cifko · 2021-09-03T08:38:06Z

Some test failures

Changed the updater_service to Option, because not all wallets will use the auto update feature (e.g. ffi wallet will not).
All cargo tests now pass.

stringhandler · 2021-09-03T09:14:32Z

Just needs clippy

Add update notification to the wallet.

Cifko · 2021-09-03T09:23:55Z

Just needs clippy

Done.

stringhandler assigned delta1 Sep 2, 2021

Cifko force-pushed the auto-update-wallet branch 6 times, most recently from c5364b4 to 868c249 Compare September 2, 2021 07:33

sdbondi reviewed Sep 2, 2021

View reviewed changes

Cifko force-pushed the auto-update-wallet branch 2 times, most recently from 39e6fc0 to 939e15f Compare September 2, 2021 09:17

delta1 previously approved these changes Sep 2, 2021

View reviewed changes

Cifko dismissed delta1’s stale review via 497f85e September 2, 2021 09:37

Cifko force-pushed the auto-update-wallet branch from 497f85e to 10c6ce0 Compare September 2, 2021 09:38

sdbondi reviewed Sep 2, 2021

View reviewed changes

applications/tari_console_wallet/src/ui/components/notification_tab.rs Outdated Show resolved Hide resolved

applications/tari_console_wallet/src/ui/components/notification_tab.rs Outdated Show resolved Hide resolved

Cifko force-pushed the auto-update-wallet branch 2 times, most recently from f9d5378 to 1f0d85d Compare September 2, 2021 09:51

Cifko force-pushed the auto-update-wallet branch from 1f0d85d to cd962b6 Compare September 2, 2021 10:59

Cifko force-pushed the auto-update-wallet branch from cd962b6 to 603f19c Compare September 3, 2021 08:36

Add general notifications support for console wallet.

d325ff0

Add update notification to the wallet.

Cifko force-pushed the auto-update-wallet branch from 603f19c to d325ff0 Compare September 3, 2021 09:22

stringhandler added the mq-approved label Sep 3, 2021

stringhandler approved these changes Sep 3, 2021

View reviewed changes

Merge branch 'development' into auto-update-wallet

c191a30

aviator-app bot merged commit faa27fc into tari-project:development Sep 3, 2021

Cifko deleted the auto-update-wallet branch September 8, 2021 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update+notifications for console wallet #3284

feat: update+notifications for console wallet #3284

Cifko commented Sep 2, 2021

delta1 commented Sep 2, 2021

sdbondi left a comment

delta1 left a comment •

edited

Loading

delta1 Sep 2, 2021

Cifko Sep 2, 2021

delta1 Sep 2, 2021

Cifko Sep 2, 2021

delta1 Sep 2, 2021

Cifko Sep 2, 2021

delta1 Sep 2, 2021

Cifko Sep 2, 2021

delta1 commented Sep 2, 2021

sdbondi left a comment •

edited

Loading

Cifko commented Sep 2, 2021

Cifko commented Sep 2, 2021

delta1 commented Sep 2, 2021

Cifko commented Sep 3, 2021

stringhandler commented Sep 3, 2021

Cifko commented Sep 3, 2021

feat: update+notifications for console wallet #3284

feat: update+notifications for console wallet #3284

Conversation

Cifko commented Sep 2, 2021

Description

Motivation and Context

How Has This Been Tested?

delta1 commented Sep 2, 2021

sdbondi left a comment

Choose a reason for hiding this comment

delta1 left a comment • edited Loading

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

delta1 commented Sep 2, 2021

sdbondi left a comment • edited Loading

Choose a reason for hiding this comment

Cifko commented Sep 2, 2021

Cifko commented Sep 2, 2021

delta1 commented Sep 2, 2021

Cifko commented Sep 3, 2021

stringhandler commented Sep 3, 2021

Cifko commented Sep 3, 2021

delta1 left a comment •

edited

Loading

sdbondi left a comment •

edited

Loading