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
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
811591f
initial invite api
sethvincent Aug 23, 2023
88d06c9
small type fixes
sethvincent Aug 24, 2023
9011e1a
speculative isMember and addProject function usage
sethvincent Aug 31, 2023
fad5bf7
Merge branch 'main' into invite-api
achou11 Sep 12, 2023
9a82b7f
Merge branch 'main' into invite-api
achou11 Sep 19, 2023
057d8d6
fix type errors in tests
achou11 Sep 19, 2023
754ea9c
revert manual change to generated rpc type
achou11 Sep 19, 2023
09ec579
types improvements
achou11 Sep 19, 2023
1d30037
variable names and types updates
achou11 Sep 20, 2023
284cffe
revert unused type change in keyToId util
achou11 Sep 20, 2023
fafbef5
revert change in tests/rpc.js to clean up PR diff
achou11 Sep 20, 2023
a7f6e85
fix lint errors
achou11 Sep 20, 2023
fc3ef15
fix usage of projectKeyToPublicId
achou11 Sep 20, 2023
7f849ee
move retrieval of invite in respond method
achou11 Sep 20, 2023
d9b147e
fix isMember mock in tests
achou11 Sep 20, 2023
69a560a
small optimizations to respond method
achou11 Sep 20, 2023
08a5eaf
add test for autoresponding with already response
achou11 Sep 20, 2023
8003252
minor tests refactor
achou11 Sep 20, 2023
efe181a
add event typing for InviteApi and update emitted value
achou11 Sep 20, 2023
b2ff8d4
fix addProject mock in tests
achou11 Sep 20, 2023
5c3f473
refactor respond method to not be recursive
achou11 Sep 20, 2023
09738ca
fix lint error
achou11 Sep 20, 2023
8168ebb
Cleanup code and add more tests
gmaclennan Sep 21, 2023
146606f
Merge branch 'main' into invite-api
achou11 Sep 21, 2023
54b9585
add test for when addProject throws
achou11 Sep 21, 2023
cb1a2e7
add test checking payload of invite-received event
achou11 Sep 21, 2023
2c64ed2
add test for sending reject response after invitor disconnects
achou11 Sep 21, 2023
bccebdb
add test for sending already response after invitor disconnects
achou11 Sep 21, 2023
9407be3
Merge branch 'main' into invite-api
achou11 Sep 25, 2023
693af73
use public id instead of project id for public interface
achou11 Sep 25, 2023
370b4be
remove commented-out line
achou11 Sep 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/generated/rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface Invite_ProjectInfo {
}

export interface InviteResponse {
projectKey: Buffer;
projectKey: Buffer | Uint8Array;
decision: InviteResponse_Decision;
}

Expand Down
76 changes: 76 additions & 0 deletions src/invite-api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { TypedEmitter } from 'tiny-typed-emitter'
import { InviteResponse_Decision } from './generated/rpc.js'
import { idToKey, keyToId } from './utils.js'

/** @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.

#invites = new Map()

/**
* @param {Object} options
* @param {MapeoRPC} options.rpc
*/
constructor({ rpc }) {
super()
this.rpc = rpc

// 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.

this.rpc.on('invite', (peerId, invite) => {
const projectId = keyToId(invite.projectKey)
const peerIds = this.#invites.get(projectId) || new Set()
peerIds.add(peerId)
this.#invites.set(projectId, peerIds)

if (peerIds.size === 1) {
this.emit('invite-received', {
projectId,
peerId,
})
}
})
}

/**
* @param {string} projectId
*/
accept(projectId) {
this.#respond({ projectId, decision: InviteResponse_Decision.ACCEPT })
}

/**
* @param {string} projectId
*/
reject(projectId) {
this.#respond({ projectId, decision: InviteResponse_Decision.REJECT })
}

/**
* @param {Object} options
* @param {string} options.projectId
* @param {InviteResponse_Decision} options.decision
*/
#respond({ projectId, decision }) {
const peerIds = this.#getPeerIds(projectId)
const projectKey = idToKey(projectId)

// 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.

for (const peerId of peerIds.values()) {
this.rpc.inviteResponse(peerId, { projectKey, decision })
}

this.#invites.delete(projectId)
}

/**
* @param {string} projectId
* @returns {Set<string>} peerIds
*/
#getPeerIds(projectId) {
return this.#invites.get(projectId) || new Set()
}
}
2 changes: 1 addition & 1 deletion src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function idToKey(id) {

/**
*
* @param {Buffer|String} key
* @param {Buffer|Uint8Array|String} key
* @returns {String}
*/
export function keyToId(key) {
Expand Down
34 changes: 34 additions & 0 deletions tests/helpers/rpc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import NoiseSecretStream from '@hyperswarm/secret-stream'

export function replicate(rpc1, rpc2) {
const n1 = new NoiseSecretStream(true, undefined, {
// Keep keypairs deterministic for tests, since we use peer.publicKey as an identifier.
keyPair: NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(0)),
})
const n2 = new NoiseSecretStream(false, undefined, {
keyPair: NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(1)),
})
n1.rawStream.pipe(n2.rawStream).pipe(n1.rawStream)

rpc1.connect(n1)
rpc2.connect(n2)

return async function destroy() {
return Promise.all([
/** @type {Promise<void>} */
(
new Promise((res) => {
n1.on('close', res)
n1.destroy()
})
),
/** @type {Promise<void>} */
(
new Promise((res) => {
n2.on('close', res)
n2.destroy()
})
),
])
}
}
49 changes: 49 additions & 0 deletions tests/invite-api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// @ts-check
import test from 'brittle'
import { MapeoRPC } from '../src/rpc/index.js'
import { InviteApi } from '../src/invite-api.js'
import { replicate } from './helpers/rpc.js'

test('Accept invite', async (t) => {
t.plan(3)
const r1 = new MapeoRPC()
const r2 = new MapeoRPC()
const inviteApi = new InviteApi({ rpc: r2 })

const projectKey = Buffer.allocUnsafe(32).fill(0)

r1.on('peers', async (peers) => {
t.is(peers.length, 1)
const response = await r1.invite(peers[0].id, { projectKey })
t.is(response, MapeoRPC.InviteResponse.ACCEPT)
})

inviteApi.on('invite-received', ({ projectId }) => {
t.is(projectId, projectKey.toString('hex'))
inviteApi.accept(projectId)
})

replicate(r1, r2)
})

test('Reject invite', async (t) => {
t.plan(3)
const r1 = new MapeoRPC()
const r2 = new MapeoRPC()
const inviteApi = new InviteApi({ rpc: r2 })

const projectKey = Buffer.allocUnsafe(32).fill(0)

r1.on('peers', async (peers) => {
t.is(peers.length, 1)
const response = await r1.invite(peers[0].id, { projectKey })
t.is(response, MapeoRPC.InviteResponse.REJECT)
})

inviteApi.on('invite-received', ({ projectId }) => {
t.is(projectId, projectKey.toString('hex'))
inviteApi.reject(projectId)
})

replicate(r1, r2)
})
35 changes: 1 addition & 34 deletions tests/rpc.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// @ts-check
import test from 'brittle'
import NoiseSecretStream from '@hyperswarm/secret-stream'
import {
MapeoRPC,
PeerDisconnectedError,
Expand All @@ -10,6 +9,7 @@ import {
import FakeTimers from '@sinonjs/fake-timers'
import { once } from 'events'
import { Duplex } from 'streamx'
import { replicate } from './helpers/rpc.js'

test('Send invite and accept', async (t) => {
t.plan(3)
Expand Down Expand Up @@ -365,36 +365,3 @@ test('invalid stream', (t) => {
const regularStream = new Duplex()
t.exception(() => r1.connect(regularStream), 'Invalid stream')
})

function replicate(rpc1, rpc2) {
const n1 = new NoiseSecretStream(true, undefined, {
// Keep keypairs deterministic for tests, since we use peer.publicKey as an identifier.
keyPair: NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(0)),
})
const n2 = new NoiseSecretStream(false, undefined, {
keyPair: NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(1)),
})
n1.rawStream.pipe(n2.rawStream).pipe(n1.rawStream)

rpc1.connect(n1)
rpc2.connect(n2)

return async function destroy() {
return Promise.all([
/** @type {Promise<void>} */
(
new Promise((res) => {
n1.on('close', res)
n1.destroy()
})
),
/** @type {Promise<void>} */
(
new Promise((res) => {
n2.on('close', res)
n2.destroy()
})
),
])
}
}