Allow setting store confirmations #20715
Merged
Conversation
This file contains hidden or 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
Jenkins BuildsClick to see older builds (45)
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
2 times, most recently
from
aae6283 to
3e986d8
Compare
J-Son89
reviewed
Jul 11, 2024
| @@ -77,6 +78,15 @@ | |||
| [:wakuv2.ui/toggle-light-client (not light-client-enabled?)]) | |||
| :accessory :switch | |||
| :active light-client-enabled?} | |||
| {:size :small | |||
There was a problem hiding this comment.
should we put these settings in some new section?
cc @ilmotta as I think he has another pr cleaning up the old settings
There was a problem hiding this comment.
I think it was for Privacy & security, but if we have one for advanced, for sure
There was a problem hiding this comment.
Clean-up of the advanced settings should be done ideally, or at least some settings. I just haven't created one issue, but will do. For example, there's a setting for telemetry, but this is super confusing for the user as we already have in-app metrics now.
ilmotta
reviewed
Jul 11, 2024
There was a problem hiding this comment.
@cammellos, could you share more details about what should we expect from this PR if the flag is enabled? If we can't reliably test it, how will we know we should merge? Maybe we can put this PR for QA and see if they can get some more definite results?
it should be a bit more reliable (i.e messages are marked as sent faster).
Is this going to help reduce the time every message currently stays in grayed state while the confirmation hasn't arrived? Something that I guess all of us noticed got worse at some point in the last few months.
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
3e986d8 to
d6a4bb5
Compare
That's correct, it should make messages confirmed faster when in light mode & this is disabled (default) |
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
d6a4bb5 to
e5bca67
Compare
82% of end-end tests have passedFailed tests (8)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (42)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
e5bca67 to
759a7e5
Compare
78% of end-end tests have passedFailed tests (10)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (40)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
30% of end-end tests have passedFailed tests (7)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
759a7e5 to
ef2358f
Compare
67% of end-end tests have passedFailed tests (16)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (34)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
d19a190 to
3abaedc
Compare
80% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (41)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
73% of end-end tests have passedFailed tests (13)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (37)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
3abaedc to
3d355f5
Compare
22% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Passed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
2 times, most recently
from
e2ae118 to
e97311d
Compare
80% of end-end tests have passedFailed tests (9)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (41)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
This commit adds the ability to enable store confirmations in the UI, under advanced settings. status-im/status-go@1ef2434...0d1e5aa
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
e97311d to
f45a8e4
Compare
clauxx
approved these changes
Jul 15, 2024
| :params [{:enabled enabled?}] | ||
| :on-success (fn [] | ||
| (log/info "store confirmation set successfully" enabled?) | ||
| (re-frame/dispatch [:logout])) |
There was a problem hiding this comment.
waku wants to be restarted for this setting to work
flexsurfer
approved these changes
Jul 15, 2024
| @@ -72,7 +72,7 @@ | |||
|
|
|||
| ;; CONFIG VALUES | |||
| (def log-level (string/upper-case (get-config :LOG_LEVEL ""))) | |||
| (def fleet (get-config :FLEET "waku.sandbox")) | |||
| (def fleet (get-config :FLEET "")) | |||
There was a problem hiding this comment.
so there is no default value if FLEET is not set in config file?
There was a problem hiding this comment.
The value is set in status-go as the default fleet if empty, if you override it in the client, it will become "sticky", meaning that it will always be that one unless you change it in the settings, even if we update the default fleet (i.e the same as if you would change it in the settings). So it should only be used for testing, most users will just want to use the default fleet and leave it blank
cammellos
force-pushed
the
feat/flag-sent-messages-check-2
branch
from
f45a8e4 to
bb8aad3
Compare
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 commit adds the ability to enable store confirmations in the UI, under advanced settings.
It's not easy to test that the actual functionality is working. Currently I am struggling with message confirmations, so when store confirmations are disabled (the default in this PR), it should be a bit more reliable (i.e messages are marked as sent faster). You can toggle the settings in advanced and check that is persisted.
How it works with store confirmations enabled (current develop behavior):
If store confirmation is disabled (current PR by default):
The first method gives you better assurance that the message propagated through the network, but it fails when you are not connected to a store node and it's slower (4/5 seconds at times to mark a message as sent. So far from just usage, I noticed a fair amount of false negatives (message made it to the store node, but wasn't marked as sent). That's why we are defaulting for the time being on the second method.
This commit also set the default fleet for testing to
status.staging.