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.

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

Warn users if a new group has the exact same name #280

Open
aguzmant103 opened this issue Aug 17, 2023 · 20 comments
Open

Warn users if a new group has the exact same name #280

aguzmant103 opened this issue Aug 17, 2023 · 20 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@aguzmant103
Copy link
Collaborator

aguzmant103 commented Aug 17, 2023

Description

The idea of this issue is to warn the user if a new group contains the exact same name as another they created previously.

Then, every time a user wants to create a new off-chain or on-chain group, it should be checked if there is any other off-chain or on-chain group with the exact same name.

This functionality should be added to the dashboard, not the API or any other packages.

@aguzmant103 aguzmant103 added this to the 3. Tethys milestone Aug 17, 2023
@aguzmant103 aguzmant103 added good first issue Good for newcomers help wanted Extra attention is needed labels Aug 17, 2023
@NamanAg0502
Copy link
Contributor

NamanAg0502 commented Aug 22, 2023

Hey @aguzmant103,

Is it alright if the off-chain group name is same as on-chain group name.

@aguzmant103
Copy link
Collaborator Author

Putting this on hold until we get feedback from UX team :)

@aguzmant103 aguzmant103 changed the title Disallow groups with the exact same name Warn users if a new group has the exact same name Aug 29, 2023
@aguzmant103
Copy link
Collaborator Author

Updating description: "Warn users if a new group has the exact same name"
FYI @NamanAg0502

This should apply for both onchain and offchain groups

@vplasencia vplasencia modified the milestones: 3. Tethys, 4. Dione Jan 2, 2024
@vplasencia vplasencia removed this from the 4. Dione milestone Mar 17, 2024
@aguzmant103 aguzmant103 self-assigned this Apr 8, 2024
@aguzmant103
Copy link
Collaborator Author

aguzmant103 commented Apr 11, 2024

Interesting behavior: if I create an offchain group with the same name as another offchain group, the old group gets overwritten. It seems the groupID is based on the group name

@vplasencia @0xjei is this expected? Woudl the expected behavior then be to NOT allow an offchain group with the same name as another offchain group?

  • i.e. I created group "test" with groupID "85465432599585866400468606240662" with members "4,5,6"
  • i.e. I create a new group "test" with group ID "85465432599585866400468606240662" with members "1,2,3"
  • i.e. I create a group "test" with groupID "85465432599585866400468606240662" with members "7,8,9" with different fingerprint
  • i.e. I create a group "test" with groupID "85465432599585866400468606240662" with members "1,2,3,4" with different fingerprint
  • I query the API and dashboard and I only see the latest

@aguzmant103
Copy link
Collaborator Author

aguzmant103 commented Apr 11, 2024

The same doesnt'y seem to happen with onchain groups. If you try to create an onchain group that already exist with that name i.e. "test" and "test" it'll error out

Even if I change the admin (account address) the error appears if there's a group with that name

  • Improve Error Message when two onchain groups have the same name

image

@0xjei
Copy link
Collaborator

0xjei commented Apr 17, 2024

Interesting behavior: if I create an offchain group with the same name as another offchain group, the old group gets overwritten. It seems the groupID is based on the group name

@vplasencia @0xjei is this expected? Woudl the expected behavior then be to NOT allow an offchain group with the same name as another offchain group?

  • i.e. I created group "test" with groupID "85465432599585866400468606240662" with members "4,5,6"
  • i.e. I create a new group "test" with group ID "85465432599585866400468606240662" with members "1,2,3"
  • i.e. I create a group "test" with groupID "85465432599585866400468606240662" with members "7,8,9" with different fingerprint
  • i.e. I create a group "test" with groupID "85465432599585866400468606240662" with members "1,2,3,4" with different fingerprint
  • I query the API and dashboard and I only see the latest

Thank you @aguzmant103 for the detailed report. This is a strange and unexpected behaviour and should be addressed as soon as possible. In my opinion, groups with the same name SHOULD NOT exist as there will be identifier collisions leading to inconsistency and overlapping of groups. This problem can be solved by checking the ID of the new group against those already created at group creation time. Do you want to work on it @aguzmant103?

@0xjei
Copy link
Collaborator

0xjei commented Apr 17, 2024

The same doesnt'y seem to happen with onchain groups. If you try to create an onchain group that already exist with that name i.e. "test" and "test" it'll error out

Even if I change the admin (account address) the error appears if there's a group with that name

  • Improve Error Message when two onchain groups have the same name

image

Correct. I noticed that for on-chain groups you are calling the semaphore.createGroup() function directly and not the bandadaAPI.createGroup() on the bandada dashboard. So the primer probably has some mechanism to check if a group with the same identifier already exists on the on-chain mapping (contracts). Does this make sense? Let me know! @aguzmant103

@aguzmant103
Copy link
Collaborator Author

@0xjei shall this compare per adminID or across all adminIDs?

In other words, if I create group "BandadaDevs" and you want to create the same group with the same name:
i) shall we allow it? (I think yes)
ii) does this clash with current database implementation? (idk, if so then we have to refactor deeper?)

@0xjei
Copy link
Collaborator

0xjei commented May 2, 2024

@0xjei shall this compare per adminID or across all adminIDs?

In other words, if I create group "BandadaDevs" and you want to create the same group with the same name: i) shall we allow it? (I think yes) ii) does this clash with current database implementation? (idk, if so then we have to refactor deeper?)

i) OK, I'll try to explain myself better. If we aim to have an ecosystem of groups, it would make sense for the identifiers (IDs) and names of the groups to be unique. This is to avoid that if I want to pass a group to a person, I am forced to pass the ID instead of the name (imagine a scenario with X groups with the same name, it could lead to confusion or scam). Thus, the name would act as the first filter, while the ID would act as the second level (the opposite happens in the backend logic). Tell me if you understand what I mean.
(ii) In case we wanted to support the same names for different groups in the same Bandada instance (infra), yes, currently it could be done but only because of that bug I told you about above.

@aguzmant103
Copy link
Collaborator Author

aguzmant103 commented May 6, 2024

After discussing in team meeting: makes sense to prevent the creation of two identical group per admin

@aguzmant103
Copy link
Collaborator Author

@0xjei I'm going to return this to the backlog as I won't have bandwidth for now

@aguzmant103 aguzmant103 removed their assignment May 13, 2024
@0xjei
Copy link
Collaborator

0xjei commented May 13, 2024

@0xjei I'm going to return this to the backlog as I won't have bandwidth for now

yes, makes sense - we can punt this back after the planning meeting accordingly to priorities <3

@aguzmant103 aguzmant103 added good first issue Good for newcomers and removed good first issue Good for newcomers labels Jun 3, 2024
@aguzmant103 aguzmant103 self-assigned this Jun 12, 2024
@vplasencia
Copy link
Member

Hey @aguzmant103! I've just updated the issue description and unassigned you from this issue to allow others to take it. However, feel free to reassign it to yourself if you'd like to work on it.

@SIDHARTH20K4
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have built many smart contracts so I can work on this issue

@vplasencia
Copy link
Member

Hey @SIDHARTH20K4 do you want me to assign this issue to you?

@GoSTEAN
Copy link

GoSTEAN commented Sep 28, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have experience in Typescript and solidity. My skills in front-end development ensure efficient, user-friendly solutions, particularly when handling data validation.

How I plan on tackling this issue

Fetch Existing Groups: On dashboard load, retrieve the list of both on-chain and off-chain groups created by the user.
Implement Name Check: When a user enters a group name, validate it against the existing group names to detect any duplicates.
Trigger Warning: Display a user-friendly warning if a duplicate is detected, preventing submission.
Optimize UX: Ensure smooth UX by integrating real-time validation and providing clear feedback.
Test Thoroughly: Ensure the feature works for both on-chain and off-chain groups and covers edge cases.

@Deepak2623
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

.

How I plan on tackling this issue

.

@vplasencia
Copy link
Member

Hey @GoSTEAN would you like me to assign this issue to you?

@GoSTEAN
Copy link

GoSTEAN commented Oct 1, 2024

@vplasencia yes you can assign it to me

@vplasencia
Copy link
Member

Hey @GoSTEAN! I just assigned the issue to you. Let us know if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: ♻️ Grooming
Development

No branches or pull requests

7 participants