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.

Sign up for GitHub

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

fix_: add missing message verification flag when creating an account #5570

Merged
merged 1 commit into from
Jul 26, 2024
Merged

fix_: add missing message verification flag when creating an account #5570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ func SetFleet(fleet string, nodeConfig *params.NodeConfig) error {
// mobile may need override following options
LightClient: specifiedWakuV2Config.LightClient,
EnableStoreConfirmationForMessagesSent: specifiedWakuV2Config.EnableStoreConfirmationForMessagesSent,
EnableMissingMessageVerification: specifiedWakuV2Config.EnableMissingMessageVerification,
Nameserver: specifiedWakuV2Config.Nameserver,
}

Expand Down Expand Up @@ -303,6 +304,10 @@ func defaultNodeConfig(installationID string, request *requests.CreateAccount, o
nodeConfig.WakuV2Config.LightClient = true
}

if request.WakuV2EnableMissingMessageVerification {
nodeConfig.WakuV2Config.EnableMissingMessageVerification = true
Copy link
Contributor

Choose a reason for hiding this comment

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

@richard-ramos, is there a recommendation that we always enable this flag on mobile? Would you say there's a scenario where we don't want this enabled on mobile?

Copy link
Member Author

@richard-ramos richard-ramos Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I imagine it should follow the same criteria as WakuV2EnableStoreConfirmationForMessagesSent

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay 👍🏼 In which case we will have this setting disabled because we set WakuV2EnableStoreConfirmationForMessagesSent to false on profile creation from mobile. Thus the PR should require no changes from mobile. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

we can keep this enabled for mobile by default. The reason WakuV2EnableStoreConfirmationForMessagesSent was disabled is as it was giving bad UX to users where messages were sent but still showing as not sent due to storeNode performance issues.

But this flag doesn't give any bad UX, so we can keep it enabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good @chaitanyaprem, thanks for the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

#5511 is hopefully going to land on 2.30 @richard-ramos. If you prefer to do directly in one status-go PR that seems fine as it's a very safe change, really up to you. On mobile side though, we would implement separately the dynamic usage of the flag because the PR status-im/status-mobile#20730 is already rather large and about to be QAed (we have a separate issue for that status-im/status-mobile#20879)

Since we are planning to cut the release branch tomorrow and are running very low on QA capacity due to other high-priority issues, the dynamic choice of the flag will require QA on mobile, and I'm afraid we can't put it on 2.30. Maybe a patch release like 2.30.1 for internal usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the meantime, to not hold this PR I added the following changes in bd9f98f . If the connection being used is "expensive", skip the missing messages verification. Seems to me like a good middle ground between not having the functionality enabled in mobile and consuming the users' data. Then once #5511 is merged, we can change the code to reuse the functionality to proceed with store queries depending on the users' choice.

@chaitanyaprem @ilmotta if you agree then we can proceed merge this PR with this change, otherwise i can roll it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great @richard-ramos 👍🏼

Copy link
Contributor

@chaitanyaprem chaitanyaprem Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt the hash based store-query going to be as expensive as legacy-store. Because we only query hashes and if we miss any messages we fetch them which would improve message reliability.

#5511 is expensive as it doesn't query hashes rather full messages.
I think we can keep this new flag enabled by default and provide a switch in the app in case it becomes expensive to disable it.

Copy link
Member Author

Choose a reason for hiding this comment

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

alright. Worst case scenario we release a x.x.1 :)

}

if request.WakuV2EnableStoreConfirmationForMessagesSent {
nodeConfig.WakuV2Config.EnableStoreConfirmationForMessagesSent = true
}
Expand Down
1 change: 1 addition & 0 deletions protocol/requests/create_account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type CreateAccount struct {
WakuV2Nameserver *string `json:"wakuV2Nameserver"`
WakuV2LightClient bool `json:"wakuV2LightClient"`
WakuV2EnableStoreConfirmationForMessagesSent bool `json:"wakuV2EnableStoreConfirmationForMessagesSent"`
WakuV2EnableMissingMessageVerification bool `json:"wakuV2EnableMissingMessageVerification"`
WakuV2Fleet string `json:"wakuV2Fleet"`

LogLevel *string `json:"logLevel"`
Expand Down