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

Test adding someone back into the group #111

Merged
merged 18 commits into from
May 18, 2023
Merged

Test adding someone back into the group #111

merged 18 commits into from
May 18, 2023

Conversation

Powersource
Copy link
Collaborator

@Powersource Powersource commented May 9, 2023

Fixes #89

Based on #106

package.json Outdated Show resolved Hide resolved
@socket-security
Copy link

socket-security bot commented May 9, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Deprecated license ✅ 0 issues
Missing license ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

index.js Outdated Show resolved Hide resolved
index.js Outdated
@@ -503,6 +510,7 @@ module.exports = {
pull.drain(
(msg) => {
const groupId = msg.value.content.recps[0]
// TODO: also check if it's in one of the epoch tips
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm there might be edge cases with this like if the new epoch is more than 1 epoch in the future and replication is slow, and how exactly getTipEpochs acts in that case. but that's definitely a bug we can fix in the future, and the worst that happens is the user gets removed from the group on the clientside

@Powersource Powersource changed the base branch from master to added-late May 10, 2023 11:04
@Powersource Powersource marked this pull request as ready for review May 10, 2023 12:29
@Powersource Powersource requested a review from mixmix May 10, 2023 12:57
@mixmix
Copy link
Member

mixmix commented May 11, 2023

didn't get to this today sorry

@Powersource
Copy link
Collaborator Author

a lot of flakes in ci but seems to work fine locally

Comment on lines 448 to +452
pull.drain(
(groupId) => myGroups.add(groupId),
(groupInfo) =>
groupInfo.readKeys
.map((readKey) => readKey.key.toString('base64'))
.forEach((readKeyString) => myReadKeys.add(readKeyString)),
Copy link
Member

Choose a reason for hiding this comment

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

This works, but you could do it will pull-streams tidily too

pull.flatten()
pull.map(readKey => readKey.gey.toString('base64')),
pull.drain(
  myReadKeys.add,
  err => ...
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just seems slightly more complex?

@@ -80,6 +80,7 @@
"test:only": "if grep -r --exclude-dir=node_modules --exclude-dir=.git --color 'test\\.only' ; then exit 1; fi",
"test:bail": "npm run test:raw | tap-arc --bail",
"test": "npm run test:raw | tap-arc && npm run test:only",
"lint": "eslint .",
Copy link
Member

Choose a reason for hiding this comment

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

Good, I've been missing this - I caught an "unused var" the other day, or maybe undefined var.
Anyway, I think this should be part of npm test script

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my editor's been running it automatically on save but realized we didn't have an official way of doing it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added to npm test

Comment on lines 535 to 540
name: 'bobrestart',
keys: ssbKeys.generate(null, 'bob'),
mfSeed: Buffer.from(
'0000000000000000000000000000000000000000000000000000000000000b0b',
'hex'
),
Copy link
Member

Choose a reason for hiding this comment

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

extract as bobConfig

Suggested change
name: 'bobrestart',
keys: ssbKeys.generate(null, 'bob'),
mfSeed: Buffer.from(
'0000000000000000000000000000000000000000000000000000000000000b0b',
'hex'
),
...bobConfig

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 547 to 548
await p(alice.close)(true)
await p(bob.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.

Suggested change
await p(alice.close)(true)
await p(bob.close)(true)
await Promise.all([p(alice.close)(true), p(bob.close)(true)])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

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.

couple of comments. Feel free to ignore or integrate

@mixmix
Copy link
Member

mixmix commented May 15, 2023

oh... tests failing ("cancelled after 10m") seems like it might be that same "getting stuck" on that publish many messages that might need pruning ... yeah it was that. hmm. Well good news is whatever fixes that in other PR will help here too...

Base automatically changed from added-late to master May 18, 2023 15:48
@Powersource Powersource merged commit be604f9 into master May 18, 2023
@Powersource Powersource deleted the add-back branch May 18, 2023 16:03
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.

Test getting excluded, reinvited, joining a group again
2 participants