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

People not removed in an exclude should be able to post on the new epoch #78

Merged
merged 20 commits into from
Apr 13, 2023

Conversation

Powersource
Copy link
Collaborator

Fixes #72

@Powersource Powersource changed the base branch from master to readd-additions April 5, 2023 12:51
Base automatically changed from readd-additions to master April 6, 2023 08:03
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@Powersource Powersource marked this pull request as ready for review April 6, 2023 08:30
@Powersource Powersource marked this pull request as draft April 6, 2023 08:30
@Powersource Powersource marked this pull request as ready for review April 7, 2023 11:01
index.js Outdated
}
})
)
}),
Copy link
Member

Choose a reason for hiding this comment

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

paraMap(fn) === pull.asyncMap(fn)
You need to add a "width" for it to be running in parallel e.g. run at most 4 at once:

Suggested change
}),
}, 4),

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

index.js Outdated
Comment on lines 429 to 441
pull(
ssb.box2.listGroupIds(),
pull.collect((err, groupIds) => {
// prettier-ignore
if (err) return cb(clarify(err, "Error getting groups we're already in when looking for new epochs"))

if (groupIds.includes(msg.value?.content?.recps?.[0])) {
return cb(null, msg)
} else {
return cb()
}
})
)
Copy link
Member

@mixmix mixmix Apr 12, 2023

Choose a reason for hiding this comment

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

we can move faster by comparing as we go and stopping as soon as we find a match

Suggested change
pull(
ssb.box2.listGroupIds(),
pull.collect((err, groupIds) => {
// prettier-ignore
if (err) return cb(clarify(err, "Error getting groups we're already in when looking for new epochs"))
if (groupIds.includes(msg.value?.content?.recps?.[0])) {
return cb(null, msg)
} else {
return cb()
}
})
)
const groupId = msg.value.content.recps.[0]
if (!groupId) return cb(null, null)
pull(
ssb.box2.listGroupIds(),
pull.filter(id => id === groupId),
pull.take(1),
pull.collect((err, groupIds) => {
// prettier-ignore
if (err) return cb(clarify(err, "Error getting groups we're already in when looking for new epochs"))
if (groupIds.length) return cb(null, msg)
else cb(null, null)
})
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

listGroupIds is basically sync, it's just a stream to allow for its live opt

index.js Outdated
pull.filter(Boolean),
pull.drain(
(msg) => {
const groupId = msg.value?.content?.recps?.[0]
Copy link
Member

Choose a reason for hiding this comment

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

🔥 we should get rid of this with isGroupAdd waaay further up in the stream.
Then we never have to do this oh... do we have this thing or not? That's the whole reason I got into JSON-Schema.
Like in the line below

const newKey = Buffer.from(msg.value?.content?.groupKey, 'base64')

if groupKey is not there this code is gonna blow up in your face

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

index.js Show resolved Hide resolved
index.js Outdated
),
// groups/epochs we're added to
pull.filter((msg) => {
return msg.value?.content?.recps?.includes(myRoot.id)
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
return msg.value?.content?.recps?.includes(myRoot.id)
return isGroupAdd(msg) && msg.value.content.recps.includes(myRoot.id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checked against the spec in a filter before this so removed the ?s

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.

one 🔥 (must change)
minor performance suggestions

Good clear test <3

@Powersource Powersource requested a review from mixmix April 12, 2023 20:39
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 staying in a group when someone else removes someone
3 participants