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

Add publishing of message type for exclusion of members #72

Closed
wants to merge 56 commits into from

Conversation

Powersource
Copy link
Contributor

@Powersource Powersource commented Jul 21, 2023

@Powersource Powersource requested a review from mixmix July 21, 2023 09:48
@Powersource
Copy link
Contributor Author

i guess tests are just flaky or something?

@Powersource
Copy link
Contributor Author

all tests passing locally


tangles: {
members: { root: groupInitMsg.key, previous: [groupInitMsg.key] },
group: { root: groupInitMsg.key, previous: [...exclude.content.tangles.group.previous] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this cheating?

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 would say no.

buuut, there seemed to be two entries in the array, which is weird

Copy link
Member

Choose a reason for hiding this comment

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

🌶️ should be same as tangles.members.previous I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm yeah but problem is the members tangle doesn't work yet. there was another review comment about this, i'll make an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#76

Copy link
Contributor Author

Choose a reason for hiding this comment

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

putting this as a later task if that's fine

}

await Promise.all([
p(kaitiaki.close)(true),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tribes.excludeMembers
    ✔ creates group
    kaitiaki  ─> new person
      1 [?]
      2 [?]
      3 group/add-member  ─ new person, undefined

/home/me/prj/ssb/ssb-tribes/index.js:108
        if (err) throw err
                 ^
Error [ReadError]: Database is not open
    at maybeError (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/levelup/lib/levelup.js:339:32)
    at LevelUP.put (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/levelup/lib/levelup.js:211:7)
    at registerAuthor (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/ssb-keyring/models/membership.js:45:8)
    at /home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/ssb-keyring/models/membership.js:26:41
    at /home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/throughs/async-map.js:32:13
    at /home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/sources/values.js:21:7
    at next (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/throughs/async-map.js:27:9)
    at next (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/sinks/drain.js:22:11)
    at /home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/sinks/drain.js:37:15
    at /home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/pull-stream/throughs/async-map.js:39:20 {
  [cause]: undefined
}

Node.js v18.16.0
    ✔ kaitiaki excluded everyone

  test count(48) != plan(null)

  Failed tests: There was 1 failure

    ✖ undefined) undefined

  total:     48
  passing:   48
  failing:   1
  2.529 s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed by adding a 500ms wait at the end and a t.end(), weird 4c98a4a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we never needed t.end() in promise tests in tribes2, i wonder what's the difference. and what exactly is happening in the 500ms wait

Copy link
Member

Choose a reason for hiding this comment

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

that error says that levelDB is trying to do something with a DB which is not open.

at registerAuthor (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/ssb-keyring/models/membership.js:45:8)

it's trying to call registerAuthor in the keyring... but I guess the keyring was closed (unless you waited 500ms)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does the keyring open again after 500ms? that's really confusing

@Powersource
Copy link
Contributor Author

All tests passing locally.

I guess before un-marking as draft, I should also add basic "consumption" of the msg. I'll add tasks above.

@Powersource
Copy link
Contributor Author

hmm maybe for "exclusion v1" i can keep this branch as a feature branch. then we could review and approve this branch in its current state, then the other tasks i've listed above can get their own PRs based on this PR. or something like that

@Powersource Powersource changed the title Add basic exclusion of members Add publishing of message type for exclusion of members Jul 25, 2023
@Powersource Powersource marked this pull request as ready for review July 25, 2023 17:17
Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

Looking good. Doesn't have (that I can see) any changes to the listeners which react to the group/exclude-member yet

README.md Outdated Show resolved Hide resolved
tangles: {
members: {
root,
previous: [root] // TODO calculate previous for members tangle
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made an issue for this that we can do later #76

spec/group/exclude-member.js Outdated Show resolved Hide resolved
Comment on lines +6 to +12
const kaitiaki = Server()
const newPerson = Server()

const name = id => {
if (id === kaitiaki.id) return 'kaitiaki '
if (id === newPerson.id) return 'new person'
}
Copy link
Member

Choose a reason for hiding this comment

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

latest scuttle-testbot supports this

Suggested change
const kaitiaki = Server()
const newPerson = Server()
const name = id => {
if (id === kaitiaki.id) return 'kaitiaki '
if (id === newPerson.id) return 'new person'
}
const kaitiaki = Server({ name: 'kaitiaki' })
const newPerson = Server({ name: 'new-person' })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, however, is not using the latest scuttle-testbot 😉

groupId, authorIds, {}
)

await p(replicate)({ from: kaitiaki, to: newPerson, live: false, name })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await p(replicate)({ from: kaitiaki, to: newPerson, live: false, name })
await p(replicate)({ from: kaitiaki, to: newPerson, live: false })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping this until we upgrade the testbot in another PR

excludes: authorIds,

tangles: {
members: { root: groupInitMsg.key, previous: [groupInitMsg.key] },
Copy link
Member

Choose a reason for hiding this comment

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

🌶️ I think the previous should be the id of the invite message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#76


tangles: {
members: { root: groupInitMsg.key, previous: [groupInitMsg.key] },
group: { root: groupInitMsg.key, previous: [...exclude.content.tangles.group.previous] }
Copy link
Member

Choose a reason for hiding this comment

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

🌶️ should be same as tangles.members.previous I think

}

await Promise.all([
p(kaitiaki.close)(true),
Copy link
Member

Choose a reason for hiding this comment

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

that error says that levelDB is trying to do something with a DB which is not open.

at registerAuthor (/home/me/prj/ssb/ssb-tribes/node_modules/.pnpm/[email protected]/node_modules/ssb-keyring/models/membership.js:45:8)

it's trying to call registerAuthor in the keyring... but I guess the keyring was closed (unless you waited 500ms)

@Powersource
Copy link
Contributor Author

thanks for the review ❤️

Doesn't have (that I can see) any changes to the listeners which react to the group/exclude-member yet

yeah i'm thinking that's for the other PRs that would merge into this one before we merge this

@Powersource
Copy link
Contributor Author

merged #75 into this since that PR is close to getting into master and we need that + this PR to be able to do #73

@Powersource Powersource changed the base branch from master to keyring-no-members September 5, 2023 14:13
@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
private-group-spec 8.0.0 filesystem +0 75.7 kB powersource

@mixmix
Copy link
Member

mixmix commented Sep 18, 2023

Merged via : #83

@mixmix mixmix closed this Sep 18, 2023
@Powersource Powersource mentioned this pull request Sep 18, 2023
9 tasks
@Powersource
Copy link
Contributor Author

Gonna track this in #85 instead

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.

2 participants