Warn users if a new group has the exact same name

aguzmant103 commented 1 year ago

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.

NamanAg0502 commented 1 year ago

Hey @aguzmant103,

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

aguzmant103 commented 1 year ago

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

aguzmant103 commented 1 year ago

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

This should apply for both onchain and offchain groups

aguzmant103 commented 7 months ago

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 commented 7 months ago

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

0xjei commented 6 months ago

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 commented 6 months ago

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

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

aguzmant103 commented 5 months ago

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

0xjei commented 5 months ago

@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

vplasencia commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

GoSTEAN commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

GoSTEAN commented 1 month ago

@vplasencia yes you can assign it to me

vplasencia commented 1 month ago

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

bandada-infra / bandada