chore: improve error reporting when joining community #7178
Conversation
Jenkins BuildsClick to see older builds (22)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7178 +/- ##
============================================
+ Coverage 34.87% 59.79% +24.92%
============================================
Files 798 813 +15
Lines 111336 113419 +2083
============================================
+ Hits 38825 67822 +28997
+ Misses 67595 38747 -28848
- Partials 4916 6850 +1934
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| requestToJoin.CalculateID() | ||
|
|
||
| addSignature := len(request.Signatures) == len(request.AddressesToReveal) | ||
| if len(request.Signatures) != len(request.AddressesToReveal) { |
There was a problem hiding this comment.
Do we call CreateRequestToJoin for an open community also?
If so the list of signatures is empty in that case and this will always return.
There was a problem hiding this comment.
Or AddressesToReveal is also empty in that case? I really don't remember.
There was a problem hiding this comment.
I've tested with Status Desktop.
Created a simplest community (auto accept, no permissions, no minted tokens). Member was still required to reveal addresses and input password for signatures. So it's the same flow, we should be good.
| return nil, fmt.Errorf("address %s not found in wallet", address) | ||
| } | ||
|
|
||
| if len(addressesToReveal) > 0 && !containsAddress(addressesToReveal, walletAccount.Address.Hex()) { |
There was a problem hiding this comment.
Hmm, this condition en(addressesToReveal) > 0 was used in some tests. Basically, if it's empty, then all wallet accounts will be returned 🤔 Is this intended?
A few tests were relying on this behaviour, passing empty list of addresses:
status-go/protocol/communities_messenger_shared_member_address_test.go
Lines 207 to 208 in c0f75d6
There was a problem hiding this comment.
Maybe we should explicitly return an error when an empty addressesToReveal is given?
There was a problem hiding this comment.
Added explicit error return here
|
Thank you for opening this pull request! We require pull request titles and commits to follow the Conventional Commits specification and it looks like your PR needs to be adjusted. Details:
|
|
Functional tests are not passing, because we don't set |
4 participants
Some findings during implementing #7113
Description
CreateRequestToJoin- return error if not enoughSignatureswas providedgenerateCommunityRequestsForSigning- return error if any of given addresses to reveal was not found in wallet accounts, or was found, but didn't match requirements (non-chat & non-watch).