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

initial invite api #217

Merged
merged 31 commits into from
Sep 25, 2023
Merged

initial invite api #217

merged 31 commits into from
Sep 25, 2023

Conversation

sethvincent
Copy link
Contributor

@sethvincent sethvincent commented Aug 23, 2023

closes #198

/** @typedef {import('./rpc/index.js').MapeoRPC} MapeoRPC */

export class InviteApi extends TypedEmitter {
// TODO: are invites persisted beyond this api?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what happens when we've already accepted an invite a few days ago and got a new invite for the same project? there may need to be some persistence and lookup.

Copy link
Member

Choose a reason for hiding this comment

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

Good question.

A previously accepted invite is the same issue as "already on this project". This is what the ALREADY invite response for. This means that the InviteApi needs to look up which projects the device is already a member of - @achou11 can you suggest the best API for this? We'll need to pass something down to the InviteApi constructor.

If a device is already part of a project and they receive an invite, we should automatically respond with ALREADY - there is no need for an event since I don't think the UX on the invitee side should show anything. On the invitor side the UX should show a "already invited" UX when receiving an invite response of ALREADY.

We're not going to persist un-responded invites beyond the lifecycle of the app - they require a connection to the invitor peer to respond anyway.

What to do with previously rejected invites - let's not track rejected invites for now, I've created an issue to deal with this question post-MVP

Copy link
Member

@achou11 achou11 Aug 24, 2023

Choose a reason for hiding this comment

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

This means that the InviteApi needs to look up which projects the device is already a member of - @achou11 can you suggest the best API for this? We'll need to pass something down to the InviteApi constructor.

Initial thought is to provide a callback in the constructor to determine membership for a project id e.g.

new InviteApi({
  // can update option name and return a string if there are more complex membership states to represent
  isMember: (projectId: string) => Promise<boolean> 
})

alternatively can pass a sharedDb option like we do for MapeoProject and let the InviteApi handle making the query itself. this approach may be useful if there are other project-related queries that may be needed

don't have a strong preference at the moment, but maybe leaning towards the latter since we kind of have a similar pattern for MapeoProject 🤷

Copy link
Member

Choose a reason for hiding this comment

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

with some additional pondering, thinking the sharedDb option is preferred for me, as it allows code specifically needed for the InviteApi to live with the implementation, as opposed to it leaking to the creator of the instance.

I'm sure my thoughts will bounce back and forth before I end up in this position again 😄

Copy link
Member

@achou11 achou11 Aug 24, 2023

Choose a reason for hiding this comment

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

just as I anticipated, seeing #217 (comment) has me thinking about this again.

So to sum it up, the InviteApi needs a way to do these things:

  1. Check project membership
  2. Add the project when accepting a received invite

Passing the sharedDb option would solve 1 well, but wouldn't (directly) solve 2 without adding duplicate code (I think). Duplicate code isn't necessarily bad though (and I don't think it would be copy-paste identical either...)

If we wanted to solve both, passing specific constructor callback options would be the simplest I guess i.e.

new InviteApi({
  isMember: ...,
  addProject: ...,
})

maybe that's just the way to go, before I start thinking of some (probably) convoluted approach using events 😄

Copy link
Member

Choose a reason for hiding this comment

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

currently using the callback-based approach in https://github.com/digidem/mapeo-core-next/pull/232/files#diff-a2f290f8510d711051a63f76d7d7b06721c1dd5b6ac7d86e8e3024795c2d829dR12, which I think i like right now.

Feels sensible that these API classes don't need to dive into the actual db and do their own queries

Copy link
Member

Choose a reason for hiding this comment

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

leaning into the callback-based approach, maybe all of these API classes can have a singular constructor opt called queries, which is responsible for being the interface between the class and dbs created outside the class.

For example, the InviteApi would have something like:

type InviteApiQueries = {
  isMember: ...
  addProject: ...
}

and instantiation in the MapeoProject will look like:

new InviteApi({
  queries: {
    isMember: async () => ...,
    addProject: async () => ...,
  },
  ...
})

honestly this is mostly aesthetic as it's not functionally different than specifying an opt for each callback. gut feeling is that it'd be a little cleaner to work with this, especially if an API class requires many queries to be specified. i also like that when i revisit the code, i'll have a more immediate idea of what db-related operations the API class requires

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also like the aesthetics of the queries object. Makes it more clear what those methods are for.

Comment on lines 19 to 20
// TODO: I'm not seeing encryption keys used in the inviteResponse in the rpc api
// what is the purpose of the encryption keys at this stage of the process or afterward?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't track the encryptionKeys property of the invite yet as there wasn't a way to use it yet.

Copy link
Member

Choose a reason for hiding this comment

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

The encryptionKeys and projectKey will be needed in the accept() method to add the project. @achou11 is working on the API for this, we should pass something down to the constructor here like addProject(), or an instance of something that has that method.

The keys will need to be stored in the this.#invites Map so we can look them up when we accept(). We don't emit() the keys though (e.g. expose to the API) because we keep these internal at all times.

Comment on lines 59 to 61
// TODO: should this reply to one peer with `ACCEPT` and the rest with `ALREADY`?
// How is the `ALREADY` decision determined? It looks like that isn't used yet.
// Does anything bad happen if we respond to multiple peers with `ACCEPT`?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the answer is send ACCEPT to all peers, just wondering what impact that has if any.

Copy link
Member

Choose a reason for hiding this comment

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

Ah good question. The impact would be that each of the invitors receiving ACCEPT would write a role record for the invitee. This would be confusing to deal with - we would have duplicate records of a device being added to the project. We should handle this in case it does happen (I've added this to #189).

On reflection I think what would make sense would be to respond ACCEPT to one peer (the first that we received an invite from?) and send ALREADY to the others.

Copy link
Member

Choose a reason for hiding this comment

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

In the case of peers disconnecting, we should send ACCEPT to the first still-connected peer that we find.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Looks good! I've added some responses to your questions. You'll need to coordinate with @achou11 for the API for looking up existing projects and joining a new one.

One issue to handle is disconnecting peers. We should emit an event for this, and return an error from accept or reject if trying to respond to a disconnected peer.

I think we should try not to add a project if we are unable to send the ACCEPT message, since we will not know if the invitor has actually written us to the auth core to actually access the project.

/** @typedef {import('./rpc/index.js').MapeoRPC} MapeoRPC */

export class InviteApi extends TypedEmitter {
// TODO: are invites persisted beyond this api?
Copy link
Member

Choose a reason for hiding this comment

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

Good question.

A previously accepted invite is the same issue as "already on this project". This is what the ALREADY invite response for. This means that the InviteApi needs to look up which projects the device is already a member of - @achou11 can you suggest the best API for this? We'll need to pass something down to the InviteApi constructor.

If a device is already part of a project and they receive an invite, we should automatically respond with ALREADY - there is no need for an event since I don't think the UX on the invitee side should show anything. On the invitor side the UX should show a "already invited" UX when receiving an invite response of ALREADY.

We're not going to persist un-responded invites beyond the lifecycle of the app - they require a connection to the invitor peer to respond anyway.

What to do with previously rejected invites - let's not track rejected invites for now, I've created an issue to deal with this question post-MVP

Comment on lines 19 to 20
// TODO: I'm not seeing encryption keys used in the inviteResponse in the rpc api
// what is the purpose of the encryption keys at this stage of the process or afterward?
Copy link
Member

Choose a reason for hiding this comment

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

The encryptionKeys and projectKey will be needed in the accept() method to add the project. @achou11 is working on the API for this, we should pass something down to the constructor here like addProject(), or an instance of something that has that method.

The keys will need to be stored in the this.#invites Map so we can look them up when we accept(). We don't emit() the keys though (e.g. expose to the API) because we keep these internal at all times.

Comment on lines 59 to 61
// TODO: should this reply to one peer with `ACCEPT` and the rest with `ALREADY`?
// How is the `ALREADY` decision determined? It looks like that isn't used yet.
// Does anything bad happen if we respond to multiple peers with `ACCEPT`?
Copy link
Member

Choose a reason for hiding this comment

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

Ah good question. The impact would be that each of the invitors receiving ACCEPT would write a role record for the invitee. This would be confusing to deal with - we would have duplicate records of a device being added to the project. We should handle this in case it does happen (I've added this to #189).

On reflection I think what would make sense would be to respond ACCEPT to one peer (the first that we received an invite from?) and send ALREADY to the others.

Comment on lines 59 to 61
// TODO: should this reply to one peer with `ACCEPT` and the rest with `ALREADY`?
// How is the `ALREADY` decision determined? It looks like that isn't used yet.
// Does anything bad happen if we respond to multiple peers with `ACCEPT`?
Copy link
Member

Choose a reason for hiding this comment

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

In the case of peers disconnecting, we should send ACCEPT to the first still-connected peer that we find.

@achou11 achou11 merged commit 3eca4b2 into main Sep 25, 2023
6 checks passed
@achou11 achou11 deleted the invite-api branch September 25, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create class that implements invite namespace API
3 participants