-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add on-chain group invites #584
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…/onchain-group-invite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @waddaboo great work! Thank you very much. I just left some comments.
What about checking here if there is no other group with the same name when creating a group? This way, future issues can be avoided.
https://github.com/bandada-infra/bandada/blob/main/apps/api/src/app/groups/groups.service.ts#L157
…ated-group Add create associated group to api-sdk
Since the associated groups will only be used for the invite codes, we should make sure that people can't add members to those off-chain groups. What do you think? If you try to use an invite code with the client app, you can see that the new member is added to the associated off-chain group. |
Right, so the associated group will be used solely to generate invite codes and will not store any other information. I'll update it. |
For this unique name check, do you want it to be unique for the whole database or just unique to admin? |
Unique per admin. |
I have implemented the requested changes and new features. Feel free to continue your review. Thanks! |
return | ||
} | ||
} else { | ||
const group = await semaphoreAPI.getGroup(invite.group.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the invite code is used, users can use it again because it was not marked as used. Something we can do is to create an API SDK function for this: https://github.com/bandada-infra/bandada/blob/main/apps/api/src/app/invites/invites.service.ts#L126
(for that we need to add that function to the controller to create a function in the API SDK)
And call that new api sdk function from the client app.
The idea is to have a similar devex as invite code for off-chain groups when using this new invite code for on-chain groups.
More context about the redeemInvite
function: https://github.com/bandada-infra/bandada/blob/main/apps/api/src/app/groups/groups.service.ts#L425
Does it make sense?
As a note to keep in mind for developers when using this functionality is that these actions with invite code for on-chain groups should be done in backend in a real world app because of security issues since users will be able to join a group even if the invite is not correct. But I think it's fine to keep it here in the client app browser for now as a way to showcase the functionality. It should be moved to a serverless function later in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I get it wrong, the new api-sdk is to bundle redeemInvite
together with add member to group.
Depends on whether the invite is for on-chain or off-chain, if off-chain, the api-sdk will add the member into the off-chain group; if on-chain, the api-sdk will run Semaphore addMember
.
Did I miss anything?
In cases of users failed to join the group upon clicking the join group button, should we invalidate the invite code or leave it until the invite has been redeemed successfully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged this PR because this could be part of a new PR.
The idea is to add just one function to the API SDK. The function can be called redeemInvite
and would go here: https://github.com/bandada-infra/bandada/blob/main/libs/api-sdk/src/invites.ts
Then we would call this function in the client app before adding the member with Semaphore.
The other part related to improving devex when using invites for on-chain groups may be part of a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases of users failed to join the group upon clicking the join group button, should we invalidate the invite code or leave it until the invite has been redeemed successfully?
I think wait until the transaction is complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so the new API SDK is just to redeemInvite
? So if developers did not use the redeemInvite
API SDK, they could reuse the invite code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so the new API SDK is just to redeemInvite?
Yes, correct.
So if developers did not use the redeemInvite API SDK, they could reuse the invite code?
Yes, if redeemInvite
(in the Bandada backend) is never called, the invite is still valid. That redeemInvite
function is called in the Bandada infrastructure (backend) when a user joins a group using an invite code, this way the invite code can't be used more than once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the clarifications! Will work on it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome. Thank you very much 🙏
Description
This PR introduces multiple new features to api, dashboard, and api-sdk.
New features included:
Related Issue
Does this introduce a breaking change?
Other information
Associated off-chain group flow:
type="on-chain" && name="groupId"
.getGroupByName(name, type)
.getAssociatedGroups(name)
.